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

Patch By Max Kanat-Alexander <mkanat@bugzilla.org> r=LpSolit, a=myk

4 files changed, 187 insertions, 53 deletions

diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index c984c8a98..e8f90dfdf 100755
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -228,26 +228,26 @@ sub run_create_validators {
     my $class  = shift;
     my $params = shift;
 
-    my $product = _check_product($params->{product});
+    my $product = $class->_check_product($params->{product});
     $params->{product_id} = $product->id;
     delete $params->{product};
 
     ($params->{bug_status}, $params->{everconfirmed})
-        = _check_bug_status($product, $params->{bug_status});
+        = $class->_check_bug_status($product, $params->{bug_status});
 
-    $params->{target_milestone} = _check_target_milestone($product,
+    $params->{target_milestone} = $class->_check_target_milestone($product,
         $params->{target_milestone});
 
-    $params->{version} = _check_version($product, $params->{version});
+    $params->{version} = $class->_check_version($product, $params->{version});
 
-    my $component = _check_component($product, $params->{component});
+    my $component = $class->_check_component($product, $params->{component});
     $params->{component_id} = $component->id;
     delete $params->{component};
 
     $params->{assigned_to} = 
-        _check_assigned_to($component, $params->{assigned_to});
+        $class->_check_assigned_to($component, $params->{assigned_to});
     $params->{qa_contact} =
-        _check_qa_contact($component, $params->{qa_contact});
+        $class->_check_qa_contact($component, $params->{qa_contact});
     # Callers cannot set Reporter, currently.
     $params->{reporter} = Bugzilla->user->id;
 
@@ -332,7 +332,7 @@ sub remove_from_db {
 #####################################################################
 
 sub _check_alias {
-   my ($alias) = @_;
+   my ($invocant, $alias) = @_;
    $alias = trim($alias);
    return undef if (!Bugzilla->params->{'usebugaliases'} || !$alias);
    ValidateBugAlias($alias);
@@ -340,7 +340,7 @@ sub _check_alias {
 }
 
 sub _check_assigned_to {
-    my ($component, $name) = @_;
+    my ($invocant, $component, $name) = @_;
     my $user = Bugzilla->user;
 
     $name = trim($name);
@@ -355,21 +355,21 @@ sub _check_assigned_to {
 }
 
 sub _check_bug_file_loc {
-    my ($url) = @_;
+    my ($invocant, $url) = @_;
     # If bug_file_loc is "http://", the default, use an empty value instead.
     $url = '' if $url eq 'http://';
     return $url;
 }
 
 sub _check_bug_severity {
-    my ($severity) = @_;
+    my ($invocant, $severity) = @_;
     $severity = trim($severity);
     check_field('bug_severity', $severity);
     return $severity;
 }
 
 sub _check_bug_status {
-    my ($product, $status) = @_;
+    my ($invocant, $product, $status) = @_;
     my $user = Bugzilla->user;
 
     my @valid_statuses = VALID_ENTRY_STATUS;
@@ -396,7 +396,7 @@ sub _check_bug_status {
 }
 
 sub _check_cc {
-    my ($ccs) = @_;
+    my ($invocant, $ccs) = @_;
     return [] unless $ccs;
 
     my %cc_ids;
@@ -409,7 +409,7 @@ sub _check_cc {
 }
 
 sub _check_comment {
-    my ($comment) = @_;
+    my ($invocant, $comment) = @_;
 
     if (!defined $comment) {
         ThrowCodeError('undefined_field', { field => 'comment' });
@@ -430,7 +430,7 @@ sub _check_comment {
 }
 
 sub _check_component {
-    my ($product, $name) = @_;
+    my ($invocant, $product, $name) = @_;
     $name = trim($name);
     $name || ThrowUserError("require_component");
     my $obj = Bugzilla::Component::check_component($product, $name);
@@ -441,7 +441,7 @@ sub _check_component {
 }
 
 sub _check_deadline {
-    my ($date) = @_;
+    my ($invocant, $date) = @_;
     $date = trim($date);
     my $tt_group = Bugzilla->params->{"timetrackinggroup"};
     return undef unless $date && $tt_group 
@@ -455,7 +455,7 @@ sub _check_deadline {
 # Takes two comma/space-separated strings and returns arrayrefs
 # of valid bug IDs.
 sub _check_dependencies {
-    my ($depends_on, $blocks) = @_;
+    my ($invocant, $depends_on, $blocks) = @_;
 
     # Only editbugs users can set dependencies on bug entry.
     return ([], []) unless Bugzilla->user->in_group('editbugs');
@@ -482,11 +482,11 @@ sub _check_dependencies {
 }
 
 sub _check_estimated_time {
-    return _check_time($_[0], 'estimated_time');
+    return $_[0]->_check_time($_[1], 'estimated_time');
 }
 
 sub _check_keywords {
-    my ($keyword_string) = @_;
+    my ($invocant, $keyword_string) = @_;
     $keyword_string = trim($keyword_string);
     return [] if (!$keyword_string || !Bugzilla->user->in_group('editbugs'));
 
@@ -501,7 +501,7 @@ sub _check_keywords {
 }
 
 sub _check_product {
-    my ($name) = @_;
+    my ($invocant, $name) = @_;
     # Check that the product exists and that the user
     # is allowed to enter bugs into this product.
     Bugzilla->user->can_enter_product($name, THROW_ERROR);
@@ -515,14 +515,14 @@ sub _check_product {
 }
 
 sub _check_op_sys {
-    my ($op_sys) = @_;
+    my ($invocant, $op_sys) = @_;
     $op_sys = trim($op_sys);
     check_field('op_sys', $op_sys);
     return $op_sys;
 }
 
 sub _check_priority {
-    my ($priority) = @_;
+    my ($invocant, $priority) = @_;
     if (!Bugzilla->params->{'letsubmitterchoosepriority'}) {
         $priority = Bugzilla->params->{'defaultpriority'};
     }
@@ -533,18 +533,18 @@ sub _check_priority {
 }
 
 sub _check_remaining_time {
-    return _check_time($_[0], 'remaining_time');
+    return $_[0]->_check_time($_[1], 'remaining_time');
 }
 
 sub _check_rep_platform {
-    my ($platform) = @_;
+    my ($invocant, $platform) = @_;
     $platform = trim($platform);
     check_field('rep_platform', $platform);
     return $platform;
 }
 
 sub _check_short_desc {
-    my ($short_desc) = @_;
+    my ($invocant, $short_desc) = @_;
     # Set the parameter to itself, but cleaned up
     $short_desc = clean_text($short_desc) if $short_desc;
 
@@ -554,11 +554,11 @@ sub _check_short_desc {
     return $short_desc;
 }
 
-sub _check_status_whiteboard { return defined $_[0] ? $_[0] : ''; }
+sub _check_status_whiteboard { return defined $_[1] ? $_[1] : ''; }
 
 # Unlike other checkers, this one doesn't return anything.
 sub _check_strict_isolation {
-    my ($product, $cc_ids, $assignee_id, $qa_contact_id) = @_;
+    my ($invocant, $product, $cc_ids, $assignee_id, $qa_contact_id) = @_;
 
     return unless Bugzilla->params->{'strict_isolation'};
 
@@ -588,7 +588,7 @@ sub _check_strict_isolation {
 }
 
 sub _check_target_milestone {
-    my ($product, $target) = @_;
+    my ($invocant, $product, $target) = @_;
     $target = trim($target);
     $target = $product->default_milestone if !defined $target;
     check_field('target_milestone', $target,
@@ -597,7 +597,7 @@ sub _check_target_milestone {
 }
 
 sub _check_time {
-    my ($time, $field) = @_;
+    my ($invocant, $time, $field) = @_;
     my $tt_group = Bugzilla->params->{"timetrackinggroup"};
     return 0 unless $tt_group && Bugzilla->user->in_group($tt_group);
     $time = trim($time) || 0;
@@ -606,7 +606,7 @@ sub _check_time {
 }
 
 sub _check_qa_contact {
-    my ($component, $name) = @_;
+    my ($invocant, $component, $name) = @_;
     my $user = Bugzilla->user;
 
     return undef unless Bugzilla->params->{'useqacontact'};
@@ -625,7 +625,7 @@ sub _check_qa_contact {
 }
 
 sub _check_version {
-    my ($product, $version) = @_;
+    my ($invocant, $product, $version) = @_;
     $version = trim($version);
     check_field('version', $version, [map($_->name, @{$product->versions})]);
     return $version;
diff --git a/Bugzilla/Keyword.pm b/Bugzilla/Keyword.pm
index fead77821..2152b338d 100644
--- a/Bugzilla/Keyword.pm
+++ b/Bugzilla/Keyword.pm
@@ -42,6 +42,11 @@ use constant VALIDATORS => {
     description => \&_check_description,
 };
 
+use constant UPDATE_COLUMNS => qw(
+    name
+    description
+);
+
 ###############################
 ####      Accessors      ######
 ###############################
@@ -49,16 +54,23 @@ use constant VALIDATORS => {
 sub description       { return $_[0]->{'description'}; }
 
 sub bug_count {
-    ($_[0]->{'bug_count'}) ||=
-      Bugzilla->dbh->selectrow_array('SELECT COUNT(keywords.bug_id) AS bug_count
-                                        FROM keyworddefs
-                                   LEFT JOIN keywords
-                                          ON keyworddefs.id = keywords.keywordid
-                                       WHERE keyworddefs.id=?', undef, ($_[0]->id));
-    return $_[0]->{'bug_count'};
+    my ($self) = @_;
+    return $self->{'bug_count'} if defined $self->{'bug_count'};
+    ($self->{'bug_count'}) =
+      Bugzilla->dbh->selectrow_array(
+          'SELECT COUNT(*) FROM keywords WHERE keywordid = ?', 
+          undef, $self->id);
+    return $self->{'bug_count'};
 }
 
 ###############################
+####       Mutators       #####
+###############################
+
+sub set_name        { $_[0]->set('name', $_[1]); }
+sub set_description { $_[0]->set('description', $_[1]); }
+
+###############################
 ####      Subroutines    ######
 ###############################
 
@@ -96,20 +108,26 @@ sub get_all_with_bug_count {
 ###############################
 
 sub _check_name {
-    my ($name) = @_;
+    my ($self, $name) = @_;
+
     $name = trim($name);
     $name eq "" && ThrowUserError("keyword_blank_name");
     if ($name =~ /[\s,]/) {
         ThrowUserError("keyword_invalid_name");
     }
-    my $keyword = new Bugzilla::Keyword({ name => $name });
-    ThrowUserError("keyword_already_exists", { name => $name }) if $keyword;
+
+    # We only want to validate the non-existence of the name if
+    # we're creating a new Keyword or actually renaming the keyword.
+    if (!ref($self) || $self->name ne $name) {
+        my $keyword = new Bugzilla::Keyword({ name => $name });
+        ThrowUserError("keyword_already_exists", { name => $name }) if $keyword;
+    }
 
     return $name;
 }
 
 sub _check_description {
-    my ($desc) = @_;
+    my ($self, $desc) = @_;
     $desc = trim($desc);
     $desc eq '' && ThrowUserError("keyword_blank_description");
     return $desc;
diff --git a/Bugzilla/Object.pm b/Bugzilla/Object.pm
index 67fb707e6..f89a371f2 100644
--- a/Bugzilla/Object.pm
+++ b/Bugzilla/Object.pm
@@ -12,6 +12,10 @@
 #
 # The Original Code is the Bugzilla Bug Tracking System.
 #
+# The Initial Developer of the Original Code is Everything Solved.
+# Portions created by Everything Solved are Copyright (C) 2006 
+# Everything Solved. All Rights Reserved.
+#
 # Contributor(s): Max Kanat-Alexander <mkanat@bugzilla.org>
 
 use strict;
@@ -116,6 +120,48 @@ sub id                { return $_[0]->{'id'};          }
 sub name              { return $_[0]->{'name'};        }
 
 ###############################
+####        Methods        ####
+###############################
+
+sub set {
+    my ($self, $field, $value) = @_;
+
+    # This method is protected. It's used to help implement set_ functions.
+    caller->isa('Bugzilla::Object')
+        || ThrowCodeError('protection_violation', 
+                          { caller     => caller,
+                            superclass => __PACKAGE__,
+                            function   => 'Bugzilla::Object->set' });
+
+    my $validators = $self->VALIDATORS;
+    if (exists $validators->{$field}) {
+        my $validator = $validators->{$field};
+        $value = $self->$validator($value);
+    }
+
+    $self->{$field} = $value;
+}
+
+sub update {
+    my $self = shift;
+
+    my $dbh      = Bugzilla->dbh;
+    my $table    = $self->DB_TABLE;
+    my $id_field = $self->ID_FIELD;
+    
+    my $columns = join(', ', map {"$_ = ?"} $self->UPDATE_COLUMNS);
+    my @values;
+    foreach my $column ($self->UPDATE_COLUMNS) {
+        my $value = $self->{$column};
+        trick_taint($value) if defined $value;
+        push(@values, $value);
+    }
+
+    $dbh->do("UPDATE $table SET $columns WHERE $id_field = ?", undef, 
+             @values, $self->id);
+}
+
+###############################
 ####      Subroutines    ######
 ###############################
 
@@ -152,7 +198,8 @@ sub run_create_validators {
     foreach my $field (sort keys %$params) {
         my $value;
         if (exists $validators->{$field}) {
-            $value = &{$validators->{$field}}($params->{$field});
+            my $validator = $validators->{$field};
+            $value = $class->$validator($params->{$field});
         }
         else {
             $value = $params->{$field};
@@ -254,15 +301,34 @@ 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.
-They must return the validated value.
+L</create>. 
+
+Validators are called both by L</create> and L</set>. When
+they are called by L</create>, the first argument will be the name
+of the class (what we normally call C<$class>).
+
+When they are called by L</set>, the first argument will be
+a reference to the current object (what we normally call C<$self>).
+
+The second argument will be the value passed to L</create> or 
+L</set>for that field. 
+
+These functions should call L<Bugzilla::Error/ThrowUserError> if they fail.
+
+The validator must return the validated value.
+
+=item C<UPDATE_COLUMNS>
+
+A list of columns to update when L</update> is called.
+If a field can't be changed, it shouldn't be listed here. (For example,
+the L</ID_FIELD> usually can't be updated.)
 
 =back
 
 =head1 METHODS
 
+=head2 Constructors
+
 =over
 
 =item C<new($param)>
@@ -293,7 +359,7 @@ They must return the validated value.
 
 =back
 
-=head1 SUBROUTINES
+=head2 Database Manipulation
 
 =over
 
@@ -330,6 +396,56 @@ Returns:     Two arrayrefs. The first is an array of database field names.
              The second is an untainted array of values that should go 
              into those fields (in the same order).
 
+=item C<update>
+
+Saves the values currently in this object to the database.
+Only the fields specified in L</UPDATE_COLUMNS> will be
+updated. Returns nothing and takes no parameters.
+
+=back
+
+=head2 Subclass Helpers
+
+These functions are intended only for use by subclasses. If
+you call them from anywhere else, they will throw a C<CodeError>.
+
+=over
+
+=item C<set>
+
+=over
+
+=item B<Description>
+
+Sets a certain hash member of this class to a certain value.
+Used for updating fields. Calls the validator for this field,
+if it exists. Subclasses should use this function
+to implement the various C<set_> mutators for their different
+fields.
+
+See L</VALIDATORS> for more information.
+
+=item B<Params>
+
+=over
+
+=item C<$field> - The name of the hash member to update. This should
+be the same as the name of the field in L</VALIDATORS>, if it exists there.
+
+=item C<$value> - The value that you're setting the field to.
+
+=back
+
+=item B<Returns> (nothing)
+
+=back
+
+=back
+
+=head1 CLASS FUNCTIONS
+
+=over
+
 =item C<get_all>
 
  Description: Returns all objects in this table from the database.
diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm
index 81bbb7fc1..4cb2c4469 100644
--- a/Bugzilla/User.pm
+++ b/Bugzilla/User.pm
@@ -121,13 +121,13 @@ sub new {
 # Validators
 ################################################################################
 
-sub _check_disable_mail { return $_[0] ? 1 : 0; }
-sub _check_disabledtext { return trim($_[0]) || ''; }
+sub _check_disable_mail { return $_[1] ? 1 : 0; }
+sub _check_disabledtext { return trim($_[1]) || ''; }
 
 # This is public since createaccount.cgi needs to use it before issuing
 # a token for account creation.
 sub check_login_name_for_creation {
-    my ($name) = @_;
+    my ($self, $name) = @_;
     $name = trim($name);
     $name || ThrowUserError('user_login_required');
     validate_email_syntax($name)
@@ -138,7 +138,7 @@ sub check_login_name_for_creation {
 }
 
 sub _check_password {
-    my ($pass) = @_;
+    my ($self, $pass) = @_;
 
     # If the password is '*', do not encrypt it or validate it further--we 
     # are creating a user who should not be able to log in using DB 
@@ -150,7 +150,7 @@ sub _check_password {
     return $cryptpassword;
 }
 
-sub _check_realname { return trim($_[0]) || ''; }
+sub _check_realname { return trim($_[1]) || ''; }
 
 ################################################################################
 # Methods