Create Bugzilla::Object->create and make Bugzilla::Keyword use it

RESOLVED FIXED in Bugzilla 3.0

Status

()

Bugzilla
Administration
--
enhancement
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Max Kanat-Alexander, Assigned: Max Kanat-Alexander)

Tracking

2.23
Bugzilla 3.0
Bug Flags:
approval +

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

12 years ago
I've come up with a way to make a generic "create" function:

1) The parameters are a hashref, they represent the column names that 
   we'll create.

2) Each subclass can have a constant called VALIDATORS. This is a hash
   that looks something like this:

   { name => \&_check_name }

   Then, when you pass "name" to create(), it will validate it by passing it
   to _check_name.

3) Each subclass can also specify REQUIRED_FIELDS => ['name', 'description'] 
   -- a list of fields that you *must* specify for create.
(Assignee)

Comment 1

12 years ago
Created attachment 231829 [details] [diff] [review]
v1

Okay, here it is. :-) This is some of my favorite code that I've ever created. :-)
Attachment #231829 - Flags: review?
(Assignee)

Updated

12 years ago
Attachment #231829 - Flags: review? → review?(bugzilla-mozilla)
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

12 years ago
Blocks: 346597
(Assignee)

Comment 2

12 years ago
Created attachment 231907 [details] [diff] [review]
v1.1

Okay, I updated it against my latest checksetup changes.
Attachment #231829 - Attachment is obsolete: true
Attachment #231907 - Flags: review?(bugzilla-mozilla)
Attachment #231829 - Flags: review?(bugzilla-mozilla)
(Assignee)

Comment 3

12 years ago
Created attachment 231914 [details] [diff] [review]
v1.2

Small error in the code-error template. Fixed now.
Attachment #231907 - Attachment is obsolete: true
Attachment #231914 - Flags: review?(bugzilla-mozilla)
Attachment #231907 - Flags: review?(bugzilla-mozilla)

Comment 4

12 years ago
Comment on attachment 231914 [details] [diff] [review]
v1.2

Lots of files missing from this v1.2 patch.
Attachment #231914 - Flags: review?(bugzilla-mozilla) → review-
(Assignee)

Comment 5

12 years ago
Created attachment 232048 [details] [diff] [review]
v1.3

Wow, you're right. There were all kinds of files missing. How weird. Anyhow, this should have them all.
Attachment #231914 - Attachment is obsolete: true
Attachment #232048 - Flags: review?(bugzilla-mozilla)

Comment 6

12 years ago
Comment on attachment 232048 [details] [diff] [review]
v1.3

Have only done some moderate testing. Also slight bitrot in Bugzilla/Install/DB.pm.

>Index: editkeywords.cgi

>-sub Validate {
>-    my ($name, $description) = @_;
>-    if ($name eq "") {
>-        ThrowUserError("keyword_blank_name");
>-    }

Note that these allow a '0' keyword/description (see comment below).

>-    if ($description eq "") {
>-        ThrowUserError("keyword_blank_description");
>-    }


>Index: Bugzilla/Keyword.pm
>@@ -81,6 +91,30 @@
>     return $keywords;
> }
> 
>+###############################
>+###       Validators        ###
>+###############################
>+
>+sub _check_name {
>+    my ($name) = @_;
>+    $name = trim($name);
>+    $name || ThrowUserError("keyword_blank_name");

This will fail if someone made a keyword named '0'. This was allowed before.

>+sub _check_description {
>+    my ($desc) = @_;
>+    $desc = trim($desc);
>+    $desc || ThrowUserError("keyword_blank_description");

Same as above.

>Index: Bugzilla/Object.pm

>+sub create {

Wonder if this should protect against having an empty REQUIRED_CREATE_FIELDS and no params being given. Although testing this, it seems MySQL will allow a 'INSERT INTO foo () VALUES ()'.

>+    $dbh->do("INSERT INTO $table (" . join(', ', @field_names) 
>+             . ") VALUES ($qmarks)", undef, @values);
>+    my $id = $dbh->bz_last_key($table, 'id');

The requirement of having an SERIAL (autoincrement) 'id' column should be documented.

>+
>+    return $class->new($id);
>+}
>+

>@@ -173,6 +212,19 @@
> in. This should be the name of a database column. Defaults to
> C<name>.
> 
>+=item C<REQUIRED_CREATE_FIELDS>
>+
>+The list of fields that B<must> be specified when the user calls
>+C<create()>. This should be an array.
>+
>+=item C<VALIDATORS>
>+
>+A hashref that points to a function that will validate each param to
>+C<create()>. Each function in this hashref will be passed a single
>+argument, the value passed to C<create()> for that field. These
>+functions should call L<Bugzilla::Error/ThrowUserError> if they fail.

Shouldn't that just be Bugzilla::Error::ThrowUserError?

>+They should return the validated value.

It is not should; they must return the validated value, because that value will be used. That the function would not on ThrowUserError is implicit.

>@@ -210,6 +262,19 @@
> 
> =over
> 
>+=item C<create($params)>
>+
>+Description: Creates a new item in the database.
>+             Throws a User Error if any of the passed-in params
>+             are invalid.
>+
>+Params:      C<$params> - hashref - A value to put in each database
>+               field for this object. Certain values must be set, and
>+               the function will throw a Code Error if you don't set
>+               them.

Repeat the certain values are the ones in REQUIRED_CREATE_FIELDS.

>Index: template/en/default/global/code-error.html.tmpl

>+  [% ELSIF error == "param_required" %]
>+    [% title = "Missing Parameter" %]
>+    The function <code>[% function FILTER html %]</code> requires
>+    a <code>[% param FILTER html %]</code> argument, and you
>+    did not pass one.

You are assuming the person getting this error is the same as the developer. Recommend rewriting without the 'you'.
Attachment #232048 - Flags: review?(bugzilla-mozilla) → review-
(Assignee)

Comment 7

12 years ago
Created attachment 232680 [details] [diff] [review]
v2

  Thank you for the very excellent review, bkor. Attached is a patch that fixes all your comments.

  If I didn't change something, I've responded below:

(In reply to comment #6)
> Wonder if this should protect against having an empty REQUIRED_CREATE_FIELDS
> and no params being given. Although testing this, it seems MySQL will allow a
> 'INSERT INTO foo () VALUES ()'.

    The function will actually fail entirely if REQUIRED_CREATE_FIELDS doesn't exist. And if somebody specifies an empty REQUIRED_CREATE_FIELDS, well...I suppose that's their choice. That means that the table doesn't have any NOT NULL fields. Doing an "INSERT INTO foo () VALUES ()" actually does insert a row, it just inserts it entirely with defaults.

> >+argument, the value passed to C<create()> for that field. These
> >+functions should call L<Bugzilla::Error/ThrowUserError> if they fail.
> 
> Shouldn't that just be Bugzilla::Error::ThrowUserError?

  No--it's a POD link. That's a link to an =item inside of another POD.
Attachment #232048 - Attachment is obsolete: true
Attachment #232680 - Flags: review?(bugzilla-mozilla)

Comment 8

12 years ago
Comment on attachment 232680 [details] [diff] [review]
v2

>Index: editkeywords.cgi

> if ($action eq 'new') {
>-    # Cleanups and validity checks
>+    my $name = $cgi->param('name') || '';
>+    my $desc = $cgi->param('description')  || '';

This actually will still not allow a '0' keyword / description, but this problem already existed in the old code.
Attachment #232680 - Flags: review?(bugzilla-mozilla) → review+

Updated

12 years ago
Flags: approval?
Flags: approval? → approval+
(Assignee)

Comment 9

12 years ago
Checking in editkeywords.cgi;
/cvsroot/mozilla/webtools/bugzilla/editkeywords.cgi,v  <--  editkeywords.cgi
new revision: 1.40; previous revision: 1.39
done
Checking in Bugzilla/Keyword.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Keyword.pm,v  <--  Keyword.pm
new revision: 1.6; previous revision: 1.5
done
Checking in Bugzilla/Object.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Object.pm,v  <--  Object.pm
new revision: 1.2; previous revision: 1.1
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.61; previous revision: 1.60
done
Checking in Bugzilla/Install/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/DB.pm,v  <--  DB.pm
new revision: 1.7; previous revision: 1.6
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v  <--  code-error.html.tmpl
new revision: 1.74; previous revision: 1.73
done
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.