Bug 490322: Fix every single keywords, multi_select, and see_also field/operator

Bugzilla/Search.pm

@@ -282,18 +282,19 @@ use constant OPERATOR_FIELD_OVERRIDE => {
         _non_changed => \&_dependson_nonchanged,
     },
     keywords => {
-        equals       => \&_keywords_exact,
-        anyexact     => \&_keywords_exact,
-        anyword      => \&_keywords_exact,
-        allwords     => \&_keywords_exact,
-
-        notequals     => \&_multiselect_negative,
-        notregexp     => \&_multiselect_negative,
-        notsubstring  => \&_multiselect_negative,
-        nowords       => \&_multiselect_negative,
-        nowordssubstr => \&_multiselect_negative,
-
-        _non_changed  => \&_keywords_nonchanged,
+        notequals      => \&_multiselect_negative,
+        notregexp      => \&_multiselect_negative,
+        notsubstring   => \&_multiselect_negative,
+        nowords        => \&_multiselect_negative,
+        nowordssubstr  => \&_multiselect_negative,
+        
+        allwords       => \&_multiselect_multiple,
+        allwordssubstr => \&_multiselect_multiple,
+        anyexact       => \&_multiselect_multiple,
+        anywords       => \&_multiselect_multiple,
+        anywordssubstr => \&_multiselect_multiple,
+        
+        _non_changed    => \&_multiselect_nonchanged,
     },
     'flagtypes.name' => {
         _default => \&_flagtypes_name,
@@ -2560,58 +2561,6 @@ sub _classification_nonchanged {
         "classifications.id", "classifications", $term);
 }
 
-sub _keywords_exact {
-    my ($self, $args) = @_;
-    my ($chart_id, $joins, $value, $operator) =
-        @$args{qw(chart_id joins value operator)};
-    my $dbh = Bugzilla->dbh;
-    
-    my @keyword_ids;
-    foreach my $word (split(/[\s,]+/, $value)) {
-        next if $word eq '';
-        my $keyword = Bugzilla::Keyword->check($word);
-        push(@keyword_ids, $keyword->id);
-    }
-    
-    # XXX We probably should instead throw an error here if there were
-    # just commas in the field.
-    if (!@keyword_ids) {
-        $args->{term} = '';
-        return;
-    }
-    
-    # This is an optimization for anywords and anyexact, since we already know
-    # the keyword id from having checked it above.
-    if ($operator eq 'anywords' or $operator eq 'anyexact') {
-        my $table = "keywords_$chart_id";
-        $args->{term} = $dbh->sql_in("$table.keywordid", \@keyword_ids);
-        push(@$joins, { table => 'keywords', as => $table });
-        return;
-    }
-    
-    $self->_keywords_nonchanged($args);
-}
-
-sub _keywords_nonchanged {
-    my ($self, $args) = @_;
-    my ($chart_id, $joins) =
-        @$args{qw(chart_id joins)};
-
-    my $k_table = "keywords_$chart_id";
-    my $kd_table = "keyworddefs_$chart_id";
-    
-    push(@$joins, { table => 'keywords', as => $k_table });
-    my $defs_join = {
-        table => 'keyworddefs',
-        as    => $kd_table,
-        from  => "$k_table.keywordid",
-        to    => 'id',
-    };
-    push(@$joins, $defs_join);
-    
-    $args->{full_field} = "$kd_table.name";
-}
-
 # XXX This should be combined with blocked_nonchanged.
 sub _dependson_nonchanged {
     my ($self, $args) = @_;
@@ -2700,16 +2649,7 @@ sub _multiselect_negative {
     my ($self, $args) = @_;
     my ($field, $operator) = @$args{qw(field operator)};
 
-    my $table;
-    if ($field eq 'keywords') {
-        $table = "keywords LEFT JOIN keyworddefs"
-                 . " ON keywords.keywordid = keyworddefs.id";
-        $args->{full_field} = "keyworddefs.name";
-    }
-    else { 
-        $table = "bug_$field";
-        $args->{full_field} = "$table.value";
-    }
+    my $table = $self->_multiselect_table($args);
     $args->{operator} = $self->_reverse_operator($operator);
     $self->_do_operator_function($args);
     my $term = $args->{term};
@@ -2723,19 +2663,21 @@ sub _multiselect_multiple {
         = @$args{qw(chart_id field operator value)};
     my $dbh = Bugzilla->dbh;
     
-    my $table = "bug_$field";
-    $args->{full_field} = "$table.value";
+    # We want things like "cf_multi_select=two+words" to still be
+    # considered a search for two separate words, unless we're using
+    # anyexact. (_all_values would consider that to be one "word" with a
+    # space in it, because it's not in the Boolean Charts).
+    my @words = $operator eq 'anyexact' ? $self->_all_values($args)
+                                        : split(/[\s,]+/, $value);
     
     my @terms;
-    foreach my $word (split(/[\s,]+/, $value)) {
+    foreach my $word (@words) {
         $args->{value} = $word;
-        $args->{quoted} = $dbh->quote($value);
-        $self->_do_operator_function($args);
-        my $term = $args->{term};
-        push(@terms, "bugs.bug_id IN (SELECT bug_id FROM $table WHERE $term)");
+        $args->{quoted} = $dbh->quote($word);
+        push(@terms, $self->_multiselect_term($args));
     }
     
-    if ($operator eq 'anyexact') {
+    if ($operator =~ /^any/) {
         $args->{term} = join(" OR ", @terms);
     }
     else {
@@ -2743,14 +2685,32 @@ sub _multiselect_multiple {
     }
 }
 
+sub _multiselect_table {
+    my ($self, $args) = @_;
+    my ($field, $chart_id) = @$args{qw(field chart_id)};
+    if ($field eq 'keywords') {
+        $args->{full_field} = 'keyworddefs.name';
+        return "keywords INNER JOIN keyworddefs".
+                               " ON keywords.keywordid = keyworddefs.id";
+    }
+    my $table = "bug_$field";
+    $args->{full_field} = "bug_$field.value";
+    return $table;
+}
+
+sub _multiselect_term {
+    my ($self, $args) = @_;
+    my $table = $self->_multiselect_table($args);
+    $self->_do_operator_function($args);
+    my $term = $args->{term};
+    return "bugs.bug_id IN (SELECT bug_id FROM $table WHERE $term)";
+}
+
 sub _multiselect_nonchanged {
     my ($self, $args) = @_;
     my ($chart_id, $joins, $field, $operator) =
         @$args{qw(chart_id joins field operator)};
-
-    my $table = "${field}_$chart_id";
-    $args->{full_field} = "$table.value";
-    push(@$joins, { table => "bug_$field", as => $table });
+    $args->{term} = $self->_multiselect_term($args);
 }
 
 ###############################

xt/lib/Bugzilla/Test/Search/Constants.pm

@@ -253,9 +253,7 @@ use constant NEGATIVE_BROKEN => (
 use constant GREATERTHAN_BROKEN => (
     bug_group => { contains => [1] },
     cc        => { contains => [1] },
-    keywords  => { contains => [1] },
     longdesc  => { contains => [1] },
-    FIELD_TYPE_MULTI_SELECT, { contains => [1] },
 );
 
 # allwords and allwordssubstr have these broken tests in common.
@@ -266,7 +264,6 @@ use constant GREATERTHAN_BROKEN => (
 use constant ALLWORDS_BROKEN => (
     bug_group => { contains => [1] },
     cc        => { contains => [1] },
-    keywords  => { contains => [1] },
     longdesc  => { contains => [1] },
 );
 
@@ -468,13 +465,10 @@ use constant COMMON_BROKEN_NOT => (
     "bug_file_loc"            => { contains => [5] },
     "deadline"                => { contains => [5] },
     "flagtypes.name"          => { contains => [5] },
-    "keywords"                => { contains => [5] },
     "longdescs.isprivate"     => { contains => [1] },
-    "see_also"                => { contains => [5] },
     FIELD_TYPE_BUG_ID,           { contains => [5] },
     FIELD_TYPE_DATETIME,         { contains => [5] },
     FIELD_TYPE_FREETEXT,         { contains => [5] },
-    FIELD_TYPE_MULTI_SELECT,     { contains => [1, 5] },
     FIELD_TYPE_TEXTAREA,         { contains => [5] },
 );
 
@@ -494,10 +488,10 @@ use constant CHANGED_FROM_TO_BROKEN_NOT => (
     'longdescs.count' => { search => 1 },
     "bug_group" => { contains => [1] },
     "cc" => { contains => [1] },
-    "cf_multi_select" => { contains => [1] },
     "estimated_time" => { contains => [1] },
     "flagtypes.name" => { contains => [1] },
     "keywords" => { contains => [1] },    
+    FIELD_TYPE_MULTI_SELECT, { contains => [1] },
 );
 
 # Common broken tests for the "not" or "no" operators.
@@ -515,17 +509,13 @@ use constant BROKEN_NOT => {
         cc => { contains => [1] },
         bug_group => { contains => [1] },
         "flagtypes.name"      => { contains => [1,5] },
-        keywords => { contains => [1,5] },
         longdesc => { contains => [1] },
-        'see_also' => { },
-        FIELD_TYPE_MULTI_SELECT, { },
     },
     'allwords-<1> <2>' => {
         'attach_data.thedata' => { contains => [5] },
         bug_group => { },
         cc => { },
         'flagtypes.name'      => { contains => [5] },
-        'keywords'            => { contains => [5] },
         'longdesc' => { },
         'longdescs.isprivate' => { },
     },
@@ -533,19 +523,13 @@ use constant BROKEN_NOT => {
         COMMON_BROKEN_NOT,
         bug_group => { contains => [1] },
         cc => { contains => [1] },
-        keywords => { contains => [1,5] },
         longdesc => { contains => [1] },
-        see_also => { },
-        FIELD_TYPE_MULTI_SELECT, { },
     },
     'allwordssubstr-<1>,<2>' => {
         bug_group => { },
         cc => { },
-        FIELD_TYPE_MULTI_SELECT, { },
-        keywords => { contains => [5] },
         "longdesc" => { },
         "longdescs.isprivate" => { },
-        "see_also" => { },
     },
     anyexact => {
         COMMON_BROKEN_NOT,
@@ -554,13 +538,9 @@ use constant BROKEN_NOT => {
     },
     'anyexact-<1>, <2>' => {
         bug_group => { contains => [1] },
-        keywords => { contains => [1,5] },
-        see_also => { },
-        FIELD_TYPE_MULTI_SELECT, { },
     },
     anywords => {
         COMMON_BROKEN_NOT,
-        FIELD_TYPE_MULTI_SELECT, { contains => [5] },
     },
     'anywords-<1> <2>' => {
         'attach_data.thedata' => { contains => [5] },
@@ -568,21 +548,14 @@ use constant BROKEN_NOT => {
     anywordssubstr => {
         COMMON_BROKEN_NOT,
     },
-    'anywordssubstr-<1> <2>' => {
-        FIELD_TYPE_MULTI_SELECT, { contains => [5] },
-    },
     casesubstring => {
         COMMON_BROKEN_NOT,
         bug_group => { contains => [1] },
-        keywords  => { contains => [1,5] },
         longdesc  => { contains => [1] },
-        FIELD_TYPE_MULTI_SELECT, { contains => [1,5] },
     },
     'casesubstring-<1>-lc' => {
         bug_group => { },
-        keywords => { contains => [5] },
         longdesc => { },
-        FIELD_TYPE_MULTI_SELECT, { contains => [5] },
     },
     changedafter => {
         "attach_data.thedata"   => { contains => [2, 3, 4] },
@@ -593,7 +566,7 @@ use constant BROKEN_NOT => {
         "requestees.login_name" => { contains => [2, 3, 4] },
         "setters.login_name"    => { contains => [2, 3, 4] },
     },
-    changedbefore=> {
+    changedbefore => {
         CHANGED_BROKEN_NOT,
     },
     changedby => {
@@ -622,19 +595,16 @@ use constant BROKEN_NOT => {
         COMMON_BROKEN_NOT,
         bug_group => { contains => [1] },
         "flagtypes.name" => { contains => [1, 5] },
-        keywords  => { contains => [1,5] },
         longdesc  => { contains => [1] },
     },
     greaterthan => {
         COMMON_BROKEN_NOT,
         cc        => { contains => [1] },
-        FIELD_TYPE_MULTI_SELECT, { contains => [5] },
     },
     greaterthaneq => {
         COMMON_BROKEN_NOT,
         cc               => { contains => [1] },
         "flagtypes.name" => { contains => [2, 5] },
-        FIELD_TYPE_MULTI_SELECT, { contains => [5] },
     },
     lessthan => {
         COMMON_BROKEN_NOT,
@@ -643,12 +613,10 @@ use constant BROKEN_NOT => {
     },
     'lessthan-2' => {
         bug_group => { contains => [1] },
-        keywords  => { contains => [1,5] },
     },
     lessthaneq => {
         COMMON_BROKEN_NOT,
         bug_group => { contains => [1] },
-        keywords  => { contains => [1,5] },
         longdesc  => { contains => [1] },
         'longdescs.isprivate' => { },
     },
@@ -668,7 +636,6 @@ use constant BROKEN_NOT => {
         COMMON_BROKEN_NOT,
         bug_group => { contains => [1] },
         "flagtypes.name" => { contains => [1,5] },
-        keywords  => { contains => [1,5] },
         longdesc  => { contains => [1] },
     },
     'regexp-^1-' => {
@@ -677,7 +644,6 @@ use constant BROKEN_NOT => {
     substring      => {
         COMMON_BROKEN_NOT,
         bug_group => { contains => [1] },
-        keywords  => { contains => [1,5] },
         longdesc  => { contains => [1] },
     },
 };
@@ -735,6 +701,8 @@ use constant GREATERTHAN_OVERRIDE => (
     bug_status   => { contains => [2,3,4,5] },
     component    => { contains => [2,3,4,5] },
     commenter    => { contains => [2,3,4,5] },
+    # keywords matches if *any* keyword matches
+    keywords     => { contains => [1,2,3,4] },
     op_sys       => { contains => [2,3,4,5] },
     priority     => { contains => [2,3,4,5] },
     product      => { contains => [2,3,4,5] },
@@ -748,6 +716,8 @@ use constant GREATERTHAN_OVERRIDE => (
     FIELD_TYPE_SINGLE_SELECT, { contains => [2,3,4,5] },
     # Override SINGLE_SELECT for resolution.
     resolution => { contains => [2,3,4] },
+    # MULTI_SELECTs match if *any* value matches
+    FIELD_TYPE_MULTI_SELECT, { contains => [1,2,3,4] },
 );
 
 # For all positive multi-value types.
@@ -1169,14 +1139,6 @@ use constant INJECTION_BROKEN_FIELD => {
                            nowordssubstr regexp substring anywords
                            notequals nowords equals anyexact)],
     },
-    keywords => {
-        search => 1,
-        operator_ok => [qw(allwordssubstr anywordssubstr casesubstring
-                           changedfrom changedto greaterthan greaterthaneq
-                           lessthan lessthaneq notregexp notsubstring
-                           nowordssubstr regexp substring anywords
-                           notequals nowords)]
-    },
 };
 
 # Operators that do not behave as we expect, for InjectionTest.

xt/lib/Bugzilla/Test/Search/FieldTestNormal.pm

@@ -64,7 +64,7 @@ sub search_params {
     my $operator = $self->operator;
     my $value = $self->translated_value;
     if ($operator eq 'anyexact') {
-        $value = [split(',', $value)];
+        $value = [split ',', $value];
     }
     
     if (my $ch_param = CH_OPERATOR->{$operator}) {