Make Bugzilla::Object able to update objects in the database, and make Bugzilla::Keyword use it

RESOLVED FIXED in Bugzilla 3.0

Status

()

Bugzilla
Bugzilla-General
--
enhancement
RESOLVED FIXED
12 years ago
11 years ago

People

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

Tracking

2.23
Bugzilla 3.0
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 2 obsolete attachments)

v3
30.92 KB, patch
Max Kanat-Alexander
: review+
Details | Diff | Splinter Review
(Assignee)

Description

12 years ago
Now that Bugzilla::Object can do INSERTs, with create(), the last really important thing it needs to be able to do is do UPDATEs in some modular way.

I have a way that this could work, and where we could re-use most of the validator code that we already wrote for create().
(Assignee)

Comment 1

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

Okay, here we go.

The basic idea is that the subclass implements a set_ function for each field, just like they implemented an accessor for each field. The set_ function should always call Bugzilla::Object->set when it's done.

We make the subclasses implement their own set_ functions for two reasons:

1) This allows subclasses to limit what can be set very clearly and very easily.
2) Subclasses can do fancy things for setting certain fields, things that
   the normal Bugzilla::Object->set couldn't handle.

Then the script calls ->update() to actually save the values to the database.

We do this so that things like process_bug could do all their set-ing and validations, and then just call update() at the very end.

You'll see that I also cleaned up editkeywords.cgi so that we only use the object methods now, and very little SQL.

You'll also see that I cleaned up Bugzilla::Keyword->bug_count, which was doing bug counts in a really dumb way. :-)
Assignee: general → mkanat
Status: NEW → ASSIGNED
Attachment #236480 - Flags: review?(LpSolit)
(Assignee)

Updated

12 years ago
Summary: Make Bugzilla::Object able to save objects to the database, and make Bugzilla::Keyword use it → Make Bugzilla::Object able to update objects in the database, and make Bugzilla::Keyword use it
(Assignee)

Updated

12 years ago
Blocks: 319598

Comment 2

12 years ago
Comment on attachment 236480 [details] [diff] [review]
v1

>Index: editkeywords.cgi

> if ($action eq 'edit') {
>+    my $keyword = new Bugzilla::Keyword($cgi->param('id'))
>+        || ThrowCodeError('invalid_keyword_id', { id => $cgi->param('id') });

You should first store $cgi->param('id') in $key_id and then pass $key_id to routines and hashes, see e.g. bug 351182.


> if ($action eq 'update') {

>+    my $keyword = new Bugzilla::Keyword($cgi->param('id'))
>+        || ThrowCodeError('invalid_keyword_id', { id => $cgi->param('id') });

Same comment as above.


> if ($action eq 'delete') {

>+    my $keyword =  new Bugzilla::Keyword($cgi->param('id'))
>+        || ThrowCodeError('invalid_keyword_id', { id => $cgi->param('id') });

Here too.


>+    my $id   = $keyword->id;
>+    my $name = $keyword->name;
> 
>+        my $bugs = $keyword->bug_count;

Nit: much better is to pass the keyword object to the template. This is a much better fix than passing each field individually.



>Index: Bugzilla/Keyword.pm

> sub bug_count {
>+    my ($self) = @_;
>+    ($self->{'bug_count'}) ||=
>+      Bugzilla->dbh->selectrow_array(
>+          'SELECT COUNT(*) FROM keywords WHERE keywordid = ?', 
>+          undef, $self->id);
>+    return $self->{'bug_count'};
> }

Nit: if bug_count = 0, we are going to query the DB all the time. Maybe should we check whether thid field is defined instead of not zero.



>Index: Bugzilla/Object.pm

>+sub set {

>+    # This method is protected. It's used to help implement set_ functions.
>+    caller->isa('Bugzilla::Object')
>+        || ThrowCodeError('protection_violation', 
>+                          { caller    => caller,  
>+                            function  => 'Bugzilla::Object->set' });

I don't really see the interest of protecting this method. Moreover, 'protection_violation' doesn't exist in code-error.html.tmpl.


>+    $self->{$field} = $value;

Nit: I'm not sure it's a good idea to update fields before we are sure they all pass validations, because error messages could then display wrong data (they should mention old values as long as ->update() hasn't been called).


>+sub update {

>+    my $columns = join(' = ?, ', $self->UPDATE_COLUMNS);
>+    $columns .= ' = ?';

Nit: this looks like a small hack to me. Cleaner would be to write:
    my $columns = join(', ', map {"$_ = ?"} $self->UPDATE_COLUMNS);


>+    foreach my $column ($self->UPDATE_COLUMNS) {
>+        my $value = $self->{$column};
>+        trick_taint($value);

Some fields can be NULL, e.g. the default QA contact. trick_taint() will die() in this case.



>Index: template/en/default/admin/keywords/edit.html.tmpl

The INTERFACE must be updated as you now pass a keyword object.


>+        [% IF keyword.bug_count > 0 %]
>           <a href="buglist.cgi?keywords=[% name FILTER url_quote %]">

'name' is now keyword.name.
Attachment #236480 - Flags: review?(LpSolit) → review-

Updated

12 years ago
Blocks: 286936
(Assignee)

Comment 3

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

Okay, I fixed the bugs.

> I don't really see the interest of protecting this method. Moreover,
> 'protection_violation' doesn't exist in code-error.html.tmpl.

  I added in the error message.

  I do think this is important, because I know programmers. :-) Somebody will see this function here and go, "Oh, I can use this directly on my objects," when they *really* shouldn't be doing that, and then some day we'd have to go back and clean up all the times that somebody did this. This function doesn't discriminate--it could allow you to update immutable (meaning "unchangeable") fields.

  However, error messages make things very clear.

  I'm going to be using this error message elsewhere in Bugzilla::Object also.

> Nit: I'm not sure it's a good idea to update fields before we are sure they 
> all pass validations, because error messages could then display wrong data 
> (they should mention old values as long as ->update() hasn't been called).

  For that case we can keep around two copies of the object, like we usually do now. For example, in process_bug, it would be $bug_old and $bug_new.

  It would get too complicated to have a set() function that didn't actually set things.


  Everything else has been fixed.
Attachment #236480 - Attachment is obsolete: true
Attachment #236716 - Flags: review?(LpSolit)

Comment 4

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

>Index: editkeywords.cgi

> if ($action eq 'update') {

>-    $vars->{'name'} = $name;
>+    $vars->{'name'} = $keyword->name;
>     $template->process("admin/keywords/rebuild-cache.html.tmpl", $vars)

rebuild-cache.html.tmpl requires $vars->{'keyword'} = $keyword.


> if ($action eq 'delete') {

>     if (!$cgi->param('reallydelete')) {
>+        my $bugs = $keyword->bug_count;
> 
>         if ($bugs) {

Nit: a good optimization would be to merge both IF:

    if (!$cgi->param('reallydelete') && $keyword->bug_count) {



>Index: Bugzilla/Object.pm

>+=item C<$field> - The name of the hash member to update. This should
>+be the same as the name of the name of the field in L</VALIDATORS> 

"... as the name of the name ..."??



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

>+  [% ELSIF error == 'protection_violation' %]

012throwables.t expects to see double quotes "" instead of ''. This makes it to fail.


r=LpSolit with these comments addressed. Please attach the updated patch.
Attachment #236716 - Flags: review?(LpSolit) → review+

Updated

12 years ago
Flags: approval?
> 1) This allows subclasses to limit what can be set very clearly and very
> easily.

Isn't this what inheritance is for (i.e. a mechanism for subclasses to selectively override methods)?
Flags: approval? → approval+
(Assignee)

Comment 6

12 years ago
(In reply to comment #5)
> > 1) This allows subclasses to limit what can be set very clearly and very
> > easily.
> 
> Isn't this what inheritance is for (i.e. a mechanism for subclasses to
> selectively override methods)?

  Yeah, but this is just *one* method, and subclasses should be implementing *multiple* methods--one for each attribute that they want to set.
(Assignee)

Comment 7

12 years ago
Created attachment 236834 [details] [diff] [review]
v3

Okay, here's the version with LpSolit's comments fixed. Carrying forward r+.
Attachment #236716 - Attachment is obsolete: true
Attachment #236834 - Flags: review+
(Assignee)

Comment 8

12 years ago
Checking in createaccount.cgi;
/cvsroot/mozilla/webtools/bugzilla/createaccount.cgi,v  <--  createaccount.cgi
new revision: 1.54; previous revision: 1.53
done
Checking in editkeywords.cgi;
/cvsroot/mozilla/webtools/bugzilla/editkeywords.cgi,v  <--  editkeywords.cgi
new revision: 1.42; previous revision: 1.41
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.172; previous revision: 1.171
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.147; previous revision: 1.146
done
Checking in Bugzilla/Keyword.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Keyword.pm,v  <--  Keyword.pm
new revision: 1.7; previous revision: 1.6
done
Checking in Bugzilla/Object.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Object.pm,v  <--  Object.pm
new revision: 1.7; previous revision: 1.6
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.130; previous revision: 1.129
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v  <--  filterexceptions.pl
new revision: 1.77; previous revision: 1.76
done
Checking in template/en/default/admin/keywords/confirm-delete.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/keywords/confirm-delete.html.tmpl,v  <--  confirm-delete.html.tmpl
new revision: 1.5; previous revision: 1.4
done
Checking in template/en/default/admin/keywords/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/keywords/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.7; previous revision: 1.6
done
Checking in template/en/default/admin/keywords/rebuild-cache.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/keywords/rebuild-cache.html.tmpl,v  <--  rebuild-cache.html.tmpl
new revision: 1.4; previous revision: 1.3
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.80; previous revision: 1.79
done
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Blocks: 319949

Updated

11 years ago
No longer blocks: 286936
You need to log in before you can comment on or make changes to this bug.