From 5137b07bae62f27dbacee3fbd82a529df1ee8b46 Mon Sep 17 00:00:00 2001 From: Max Kanat-Alexander Date: Tue, 1 Mar 2011 05:04:07 -0800 Subject: Bug 490322: Fix every single keywords, multi_select, and see_also field/operator combination in Search.pm. r=mkanat, a=mkanat (module owner) --- Bugzilla/Search.pm | 132 +++++++++---------------- xt/lib/Bugzilla/Test/Search/Constants.pm | 50 ++-------- xt/lib/Bugzilla/Test/Search/FieldTestNormal.pm | 2 +- 3 files changed, 53 insertions(+), 131 deletions(-) diff --git a/Bugzilla/Search.pm b/Bugzilla/Search.pm index 2bd4c06c9..bf9dbcb2e 100644 --- a/Bugzilla/Search.pm +++ b/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); } ############################### diff --git a/xt/lib/Bugzilla/Test/Search/Constants.pm b/xt/lib/Bugzilla/Test/Search/Constants.pm index ef0220ed1..256917ec7 100644 --- a/xt/lib/Bugzilla/Test/Search/Constants.pm +++ b/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. diff --git a/xt/lib/Bugzilla/Test/Search/FieldTestNormal.pm b/xt/lib/Bugzilla/Test/Search/FieldTestNormal.pm index 1262e19fb..b891c1587 100644 --- a/xt/lib/Bugzilla/Test/Search/FieldTestNormal.pm +++ b/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}) { -- cgit v1.2.3-24-g4f1b