Bug 413772: Eliminate sqlify_criteria() in Bugzilla::Flag and replace match() there with Bugzilla::Object::match() - Patch by Max Kanat-Alexander <mkanat@bugzilla.org> r/a=LpSolit

7 files changed, 35 insertions, 85 deletions

diff --git a/Bugzilla/Attachment.pm b/Bugzilla/Attachment.pm
index 954765f03..b4ac9e6f0 100644
--- a/Bugzilla/Attachment.pm
+++ b/Bugzilla/Attachment.pm
@@ -426,7 +426,7 @@ sub flags {
     my $self = shift;
     return $self->{flags} if exists $self->{flags};
 
-    $self->{flags} = Bugzilla::Flag::match({ 'attach_id' => $self->id });
+    $self->{flags} = Bugzilla::Flag->match({ 'attach_id' => $self->id });
     return $self->{flags};
 }
 
@@ -918,7 +918,7 @@ sub insert_attachment_for_bug {
     Bugzilla->error_mode(ERROR_MODE_DIE);
     eval {
         Bugzilla::Flag::validate($cgi, $bug->bug_id, -1, SKIP_REQUESTEE_ON_ERROR);
-        Bugzilla::Flag::process($bug, $attachment, $timestamp, $cgi, $hr_vars);
+        Bugzilla::Flag->process($bug, $attachment, $timestamp, $cgi, $hr_vars);
     };
     Bugzilla->error_mode($error_mode_cache);
     if ($@) {
diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index bc50ccff4..364f5730c 100755
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -2312,7 +2312,7 @@ sub flag_types {
          'component_id' => $self->{'component_id'} });
 
     foreach my $flag_type (@$flag_types) {
-        $flag_type->{'flags'} = Bugzilla::Flag::match(
+        $flag_type->{'flags'} = Bugzilla::Flag->match(
             { 'bug_id'      => $self->bug_id,
               'type_id'     => $flag_type->{'id'},
               'target_type' => 'bug' });
@@ -2432,7 +2432,7 @@ sub show_attachment_flags {
         { 'target_type'  => 'attachment',
           'product_id'   => $self->{'product_id'},
           'component_id' => $self->{'component_id'} });
-    my $num_attachment_flags = Bugzilla::Flag::count(
+    my $num_attachment_flags = Bugzilla::Flag->count(
         { 'target_type'  => 'attachment',
           'bug_id'       => $self->bug_id });
 
diff --git a/Bugzilla/Flag.pm b/Bugzilla/Flag.pm
index 9dc6a2ef5..dc298634d 100644
--- a/Bugzilla/Flag.pm
+++ b/Bugzilla/Flag.pm
@@ -202,16 +202,21 @@ and returns an array of matching records.
 =cut
 
 sub match {
+    my $class = shift;
     my ($criteria) = @_;
-    my $dbh = Bugzilla->dbh;
-
-    my @criteria = sqlify_criteria($criteria);
-    $criteria = join(' AND ', @criteria);
 
-    my $flag_ids = $dbh->selectcol_arrayref("SELECT id FROM flags
-                                             WHERE $criteria");
+    # If the caller specified only bug or attachment flags,
+    # limit the query to those kinds of flags.
+    if (my $type = delete $criteria->{'target_type'}) {
+        if ($type eq 'attachment') {
+            $criteria->{'attach_id'} = NOT_NULL;
+        }
+        else {
+            $criteria->{'attach_id'} = IS_NULL;
+        }
+    }
 
-    return Bugzilla::Flag->new_from_list($flag_ids);
+    return $class->SUPER::match(@_);
 }
 
 =pod
@@ -229,15 +234,8 @@ and returns an array of matching records.
 =cut
 
 sub count {
-    my ($criteria) = @_;
-    my $dbh = Bugzilla->dbh;
-
-    my @criteria = sqlify_criteria($criteria);
-    $criteria = join(' AND ', @criteria);
-
-    my $count = $dbh->selectrow_array("SELECT COUNT(*) FROM flags WHERE $criteria");
-
-    return $count;
+    my $class = shift;
+    return scalar @{$class->match(@_)};
 }
 
 ######################################################################
@@ -485,10 +483,10 @@ sub _validate {
 }
 
 sub snapshot {
-    my ($bug_id, $attach_id) = @_;
+    my ($class, $bug_id, $attach_id) = @_;
 
-    my $flags = match({ 'bug_id'    => $bug_id,
-                        'attach_id' => $attach_id });
+    my $flags = $class->match({ 'bug_id'    => $bug_id,
+                                'attach_id' => $attach_id });
     my @summaries;
     foreach my $flag (@$flags) {
         my $summary = $flag->type->name . $flag->status;
@@ -517,7 +515,7 @@ object used to obtain the flag fields that the user submitted.
 =cut
 
 sub process {
-    my ($bug, $attachment, $timestamp, $cgi, $hr_vars) = @_;
+    my ($class, $bug, $attachment, $timestamp, $cgi, $hr_vars) = @_;
     my $dbh = Bugzilla->dbh;
 
     # Make sure the bug (and attachment, if given) exists and is accessible
@@ -533,7 +531,7 @@ sub process {
     $timestamp ||= $dbh->selectrow_array('SELECT NOW()');
 
     # Take a snapshot of flags before any changes.
-    my @old_summaries = snapshot($bug_id, $attach_id);
+    my @old_summaries = $class->snapshot($bug_id, $attach_id);
 
     # Cancel pending requests if we are obsoleting an attachment.
     if ($attachment && $cgi->param('isobsolete')) {
@@ -586,7 +584,7 @@ sub process {
     }
 
     # Take a snapshot of flags after changes.
-    my @new_summaries = snapshot($bug_id, $attach_id);
+    my @new_summaries = $class->snapshot($bug_id, $attach_id);
 
     update_activity($bug_id, $attach_id, $timestamp, \@old_summaries, \@new_summaries);
 }
@@ -865,7 +863,7 @@ sub retarget {
 
     foreach my $flagtype (@$flagtypes) {
         # Get the number of flags of this type already set for this target.
-        my $has_flags = count(
+        my $has_flags = __PACKAGE__->count(
             { 'type_id'     => $flagtype->id,
               'bug_id'      => $bug->bug_id,
               'attach_id'   => $flag->attach_id });
@@ -989,7 +987,7 @@ sub FormToNewFlags {
         next unless scalar(grep { $_ == $type_id } @type_ids);
 
         # Get the number of flags of this type already set for this target.
-        my $has_flags = count(
+        my $has_flags = __PACKAGE__->count(
             { 'type_id'     => $type_id,
               'target_type' => $attachment ? 'attachment' : 'bug',
               'bug_id'      => $bug->bug_id,
@@ -1111,7 +1109,8 @@ sub CancelRequests {
     return if (!scalar(@$request_ids));
 
     # Take a snapshot of flags before any changes.
-    my @old_summaries = snapshot($bug->bug_id, $attachment->id) if ($timestamp);
+    my @old_summaries = __PACKAGE__->snapshot($bug->bug_id, $attachment->id)
+        if ($timestamp);
     my $flags = Bugzilla::Flag->new_from_list($request_ids);
     foreach my $flag (@$flags) { clear($flag, $bug, $attachment) }
 
@@ -1119,60 +1118,11 @@ sub CancelRequests {
     return unless ($timestamp);
 
     # Take a snapshot of flags after any changes.
-    my @new_summaries = snapshot($bug->bug_id, $attachment->id);
+    my @new_summaries = __PACKAGE__->snapshot($bug->bug_id, $attachment->id);
     update_activity($bug->bug_id, $attachment->id, $timestamp,
                     \@old_summaries, \@new_summaries);
 }
 
-######################################################################
-# Private Functions
-######################################################################
-
-=begin private
-
-=head1 PRIVATE FUNCTIONS
-
-=over
-
-=item C<sqlify_criteria($criteria)>
-
-Converts a hash of criteria into a list of SQL criteria.
-
-=back
-
-=cut
-
-sub sqlify_criteria {
-    # a reference to a hash containing the criteria (field => value)
-    my ($criteria) = @_;
-
-    # the generated list of SQL criteria; "1=1" is a clever way of making sure
-    # there's something in the list so calling code doesn't have to check list
-    # size before building a WHERE clause out of it
-    my @criteria = ("1=1");
-    
-    # If the caller specified only bug or attachment flags,
-    # limit the query to those kinds of flags.
-    if (defined($criteria->{'target_type'})) {
-        if    ($criteria->{'target_type'} eq 'bug')        { push(@criteria, "attach_id IS NULL") }
-        elsif ($criteria->{'target_type'} eq 'attachment') { push(@criteria, "attach_id IS NOT NULL") }
-    }
-    
-    # Go through each criterion from the calling code and add it to the query.
-    foreach my $field (keys %$criteria) {
-        my $value = $criteria->{$field};
-        next unless defined($value);
-        if    ($field eq 'type_id')      { push(@criteria, "type_id      = $value") }
-        elsif ($field eq 'bug_id')       { push(@criteria, "bug_id       = $value") }
-        elsif ($field eq 'attach_id')    { push(@criteria, "attach_id    = $value") }
-        elsif ($field eq 'requestee_id') { push(@criteria, "requestee_id = $value") }
-        elsif ($field eq 'setter_id')    { push(@criteria, "setter_id    = $value") }
-        elsif ($field eq 'status')       { push(@criteria, "status       = '$value'") }
-    }
-    
-    return @criteria;
-}
-
 =head1 SEE ALSO
 
 =over
diff --git a/attachment.cgi b/attachment.cgi
index 65a8aa539..449abde65 100755
--- a/attachment.cgi
+++ b/attachment.cgi
@@ -423,7 +423,7 @@ sub edit {
                                                'product_id'   => $product_id ,
                                                'component_id' => $component_id });
   foreach my $flag_type (@$flag_types) {
-    $flag_type->{'flags'} = Bugzilla::Flag::match({ 'type_id'   => $flag_type->id,
+    $flag_type->{'flags'} = Bugzilla::Flag->match({ 'type_id'   => $flag_type->id,
                                                     'attach_id' => $attachment->id });
   }
   $vars->{'flag_types'} = $flag_types;
@@ -529,7 +529,7 @@ sub update {
   # to attachments so that we can delete pending requests if the user
   # is obsoleting this attachment without deleting any requests
   # the user submits at the same time.
-  Bugzilla::Flag::process($bug, $attachment, $timestamp, $cgi, $vars);
+  Bugzilla::Flag->process($bug, $attachment, $timestamp, $cgi, $vars);
 
   # Update the attachment record in the database.
   $dbh->do("UPDATE  attachments 
diff --git a/editusers.cgi b/editusers.cgi
index 6dda0e97a..11bd62bde 100755
--- a/editusers.cgi
+++ b/editusers.cgi
@@ -484,14 +484,14 @@ if ($action eq 'search') {
 
     foreach (@$buglist) {
         my ($bug_id, $attach_id) = @$_;
-        my @old_summaries = Bugzilla::Flag::snapshot($bug_id, $attach_id);
+        my @old_summaries = Bugzilla::Flag->snapshot($bug_id, $attach_id);
         if ($attach_id) {
             $sth_flagupdate_attachment->execute($bug_id, $attach_id, $otherUserID);
         }
         else {
             $sth_flagupdate_bug->execute($bug_id, $otherUserID);
         }
-        my @new_summaries = Bugzilla::Flag::snapshot($bug_id, $attach_id);
+        my @new_summaries = Bugzilla::Flag->snapshot($bug_id, $attach_id);
         # Let update_activity do all the dirty work, including setting
         # the bug timestamp.
         Bugzilla::Flag::update_activity($bug_id, $attach_id, $timestamp,
diff --git a/post_bug.cgi b/post_bug.cgi
index 0f23e7d98..89edca2f4 100755
--- a/post_bug.cgi
+++ b/post_bug.cgi
@@ -230,7 +230,7 @@ my $error_mode_cache = Bugzilla->error_mode;
 Bugzilla->error_mode(ERROR_MODE_DIE);
 eval {
     Bugzilla::Flag::validate($cgi, $id, undef, SKIP_REQUESTEE_ON_ERROR);
-    Bugzilla::Flag::process($bug, undef, $timestamp, $cgi, $vars);
+    Bugzilla::Flag->process($bug, undef, $timestamp, $cgi, $vars);
 };
 Bugzilla->error_mode($error_mode_cache);
 if ($@) {
diff --git a/process_bug.cgi b/process_bug.cgi
index 8eb7aaf33..7c5902e73 100755
--- a/process_bug.cgi
+++ b/process_bug.cgi
@@ -550,7 +550,7 @@ foreach my $bug (@bug_objects) {
     }
 
     # Set and update flags.
-    Bugzilla::Flag::process($bug, undef, $timestamp, $cgi, $vars);
+    Bugzilla::Flag->process($bug, undef, $timestamp, $cgi, $vars);
 
     $dbh->bz_commit_transaction();