From da12cec61f8c7e667b00fc5fc39c827d3593f021 Mon Sep 17 00:00:00 2001 From: Byron Jones Date: Thu, 22 Nov 2012 16:23:53 +0800 Subject: Bug 780820: Allows for multiple custom search criteria to match one field r=dkl,a=LpSolit --- Bugzilla/DB.pm | 6 +- Bugzilla/DB/Oracle.pm | 6 +- Bugzilla/Search.pm | 71 ++++++--- Bugzilla/Search/Clause.pm | 7 +- Bugzilla/Search/ClauseGroup.pm | 97 ++++++++++++ js/custom-search.js | 174 +++++++++++++++++++-- template/en/default/global/user-error.html.tmpl | 11 ++ .../en/default/search/boolean-charts.html.tmpl | 13 +- 8 files changed, 333 insertions(+), 52 deletions(-) create mode 100644 Bugzilla/Search/ClauseGroup.pm diff --git a/Bugzilla/DB.pm b/Bugzilla/DB.pm index 7756ff53b..877e6cb15 100644 --- a/Bugzilla/DB.pm +++ b/Bugzilla/DB.pm @@ -385,8 +385,10 @@ sub sql_string_until { } sub sql_in { - my ($self, $column_name, $in_list_ref) = @_; - return " $column_name IN (" . join(',', @$in_list_ref) . ") "; + my ($self, $column_name, $in_list_ref, $negate) = @_; + return " $column_name " + . ($negate ? "NOT " : "") + . "IN (" . join(',', @$in_list_ref) . ") "; } sub sql_fulltext_search { diff --git a/Bugzilla/DB/Oracle.pm b/Bugzilla/DB/Oracle.pm index fcc4e88a9..01c37a028 100644 --- a/Bugzilla/DB/Oracle.pm +++ b/Bugzilla/DB/Oracle.pm @@ -203,16 +203,16 @@ sub sql_position { } sub sql_in { - my ($self, $column_name, $in_list_ref) = @_; + my ($self, $column_name, $in_list_ref, $negate) = @_; my @in_list = @$in_list_ref; - return $self->SUPER::sql_in($column_name, $in_list_ref) if $#in_list < 1000; + return $self->SUPER::sql_in($column_name, $in_list_ref, $negate) if $#in_list < 1000; my @in_str; while (@in_list) { my $length = $#in_list + 1; my $splice = $length > 1000 ? 1000 : $length; my @sub_in_list = splice(@in_list, 0, $splice); push(@in_str, - $self->SUPER::sql_in($column_name, \@sub_in_list)); + $self->SUPER::sql_in($column_name, \@sub_in_list, $negate)); } return "( " . join(" OR ", @in_str) . " )"; } diff --git a/Bugzilla/Search.pm b/Bugzilla/Search.pm index 8746c2706..3d09def65 100644 --- a/Bugzilla/Search.pm +++ b/Bugzilla/Search.pm @@ -23,6 +23,7 @@ use Bugzilla::Group; use Bugzilla::User; use Bugzilla::Field; use Bugzilla::Search::Clause; +use Bugzilla::Search::ClauseGroup; use Bugzilla::Search::Condition qw(condition); use Bugzilla::Status; use Bugzilla::Keyword; @@ -1151,8 +1152,8 @@ sub _translate_join { die "join with no table: " . Dumper($join_info) if !$join_info->{table}; die "join with no 'as': " . Dumper($join_info) if !$join_info->{as}; - - my $from_table = "bugs"; + + my $from_table = $join_info->{bugs_table} || "bugs"; my $from = $join_info->{from} || "bug_id"; if ($from =~ /^(\w+)\.(\w+)$/) { ($from_table, $from) = ($1, $2); @@ -1557,7 +1558,7 @@ sub _charts_to_conditions { my $clause = $self->_charts; my @joins; $clause->walk_conditions(sub { - my ($condition) = @_; + my ($clause, $condition) = @_; return if !$condition->translated; push(@joins, @{ $condition->translated->{joins} }); }); @@ -1577,7 +1578,7 @@ sub _params_to_data_structure { my ($self) = @_; # First we get the "special" charts, representing all the normal - # field son the search page. This may modify _params, so it needs to + # fields on the search page. This may modify _params, so it needs to # happen first. my $clause = $self->_special_charts; @@ -1586,7 +1587,7 @@ sub _params_to_data_structure { # And then process the modern "custom search" format. $clause->add( $self->_custom_search ); - + return $clause; } @@ -1617,7 +1618,7 @@ sub _boolean_charts { my $identifier = "$chart_id-$and_id-$or_id"; my $field = $params->{"field$identifier"}; my $operator = $params->{"type$identifier"}; - my $value = $params->{"value$identifier"}; + my $value = $params->{"value$identifier"}; $or_clause->add($field, $operator, $value); } $and_clause->add($or_clause); @@ -1636,13 +1637,19 @@ sub _custom_search { my @field_ids = $self->_field_ids; return unless scalar @field_ids; - my $current_clause = new Bugzilla::Search::Clause($params->{j_top}); + my $joiner = $params->{j_top} || ''; + my $current_clause = $joiner eq 'AND_G' + ? new Bugzilla::Search::ClauseGroup() + : new Bugzilla::Search::Clause($joiner); + my @clause_stack; foreach my $id (@field_ids) { my $field = $params->{"f$id"}; if ($field eq 'OP') { - my $joiner = $params->{"j$id"}; - my $new_clause = new Bugzilla::Search::Clause($joiner); + my $joiner = $params->{"j$id"} || ''; + my $new_clause = $joiner eq 'AND_G' + ? new Bugzilla::Search::ClauseGroup() + : new Bugzilla::Search::Clause($joiner); $new_clause->negate($params->{"n$id"}); $current_clause->add($new_clause); push(@clause_stack, $current_clause); @@ -1681,14 +1688,12 @@ sub _field_ids { } sub _handle_chart { - my ($self, $chart_id, $condition) = @_; + my ($self, $chart_id, $clause, $condition) = @_; my $dbh = Bugzilla->dbh; my $params = $self->_params; my ($field, $operator, $value) = $condition->fov; - - $field = FIELD_MAP->{$field} || $field; - return if (!defined $field or !defined $operator or !defined $value); + $field = FIELD_MAP->{$field} || $field; my $string_value; if (ref $value eq 'ARRAY') { @@ -1719,15 +1724,19 @@ sub _handle_chart { # on multiple values, like anyexact. my %search_args = ( - chart_id => $chart_id, - sequence => $chart_id, - field => $field, - full_field => $full_field, - operator => $operator, - value => $string_value, - all_values => $value, - joins => [], + chart_id => $chart_id, + sequence => $chart_id, + field => $field, + full_field => $full_field, + operator => $operator, + value => $string_value, + all_values => $value, + joins => [], + bugs_table => 'bugs', + table_suffix => '', ); + $clause->update_search_args(\%search_args); + $search_args{quoted} = $self->_quote_unless_numeric(\%search_args); # This should add a "term" selement to %search_args. $self->do_search_function(\%search_args); @@ -1743,7 +1752,12 @@ sub _handle_chart { field => $field, type => $operator, value => $string_value, term => $search_args{term}, }); - + + foreach my $join (@{ $search_args{joins} }) { + $join->{bugs_table} = $search_args{bugs_table}; + $join->{table_suffix} = $search_args{table_suffix}; + } + $condition->translated(\%search_args); } @@ -1899,8 +1913,14 @@ sub _quote_unless_numeric { } sub build_subselect { - my ($outer, $inner, $table, $cond) = @_; - return "$outer IN (SELECT $inner FROM $table WHERE $cond)"; + my ($outer, $inner, $table, $cond, $negate) = @_; + # Execute subselects immediately to avoid dependent subqueries, which are + # large performance hits on MySql + my $q = "SELECT DISTINCT $inner FROM $table WHERE $cond"; + my $dbh = Bugzilla->dbh; + my $list = $dbh->selectcol_arrayref($q); + return $negate ? "1=1" : "1=2" unless @$list; + return $dbh->sql_in($outer, $list, $negate); } # Used by anyexact to get the list of input values. This allows us to @@ -2653,8 +2673,7 @@ sub _multiselect_term { my $term = $args->{term}; $term .= $args->{_extra_where} || ''; my $select = $args->{_select_field} || 'bug_id'; - my $not_sql = $not ? "NOT " : ''; - return "bugs.bug_id ${not_sql}IN (SELECT $select FROM $table WHERE $term)"; + return build_subselect("$args->{bugs_table}.bug_id", $select, $table, $term, $not); } ############################### diff --git a/Bugzilla/Search/Clause.pm b/Bugzilla/Search/Clause.pm index ff50cb944..77969440e 100644 --- a/Bugzilla/Search/Clause.pm +++ b/Bugzilla/Search/Clause.pm @@ -30,6 +30,11 @@ sub children { return $self->{children}; } +sub update_search_args { + my ($self, $search_args) = @_; + # abstract +} + sub joiner { return $_[0]->{joiner} } sub has_translated_conditions { @@ -71,7 +76,7 @@ sub walk_conditions { my ($self, $callback) = @_; foreach my $child (@{ $self->children }) { if ($child->isa('Bugzilla::Search::Condition')) { - $callback->($child); + $callback->($self, $child); } else { $child->walk_conditions($callback); diff --git a/Bugzilla/Search/ClauseGroup.pm b/Bugzilla/Search/ClauseGroup.pm new file mode 100644 index 000000000..98c9779c9 --- /dev/null +++ b/Bugzilla/Search/ClauseGroup.pm @@ -0,0 +1,97 @@ +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, You can obtain one at http://mozilla.org/MPL/2.0/. +# +# This Source Code Form is "Incompatible With Secondary Licenses", as +# defined by the Mozilla Public License, v. 2.0. + +package Bugzilla::Search::ClauseGroup; + +use 5.10.1; +use strict; + +use base qw(Bugzilla::Search::Clause); + +use Bugzilla::Error; +use Bugzilla::Search::Condition qw(condition); +use Bugzilla::Util qw(trick_taint); +use List::MoreUtils qw(uniq); + +use constant UNSUPPORTED_FIELDS => qw( + attach_data.thedata + classification + commenter + component + longdescs.count + product + owner_idle_time +); + +sub new { + my ($class) = @_; + my $self = bless({ joiner => 'AND' }, $class); + # Add a join back to the bugs table which will be used to group conditions + # for this clause + my $condition = Bugzilla::Search::Condition->new({}); + $condition->translated({ + joins => [{ + table => 'bugs', + as => 'bugs_g0', + from => 'bug_id', + to => 'bug_id', + extra => [], + }], + term => '1 = 1', + }); + $self->SUPER::add($condition); + $self->{group_condition} = $condition; + return $self; +} + +sub add { + my ($self, @args) = @_; + my $field = scalar(@args) == 3 ? $args[0] : $args[0]->{field}; + + # We don't support nesting of conditions under this clause + if (scalar(@args) == 1 && !$args[0]->isa('Bugzilla::Search::Condition')) { + ThrowUserError('search_grouped_invalid_nesting'); + } + + # Ensure all conditions use the same field + if (!$self->{_field}) { + $self->{_field} = $field; + } elsif ($field ne $self->{_field}) { + ThrowUserError('search_grouped_field_mismatch'); + } + + # Unsupported fields + if (grep { $_ eq $field } UNSUPPORTED_FIELDS ) { + ThrowUserError('search_grouped_field_invalid', { field => $field }); + } + + $self->SUPER::add(@args); +} + +sub update_search_args { + my ($self, $search_args) = @_; + + # No need to change things if there's only one child condition + return unless scalar(@{ $self->children }) > 1; + + # we want all the terms to use the same join table + if (!exists $self->{_first_chart_id}) { + $self->{_first_chart_id} = $search_args->{chart_id}; + } else { + $search_args->{chart_id} = $self->{_first_chart_id}; + } + + my $suffix = '_g' . $self->{_first_chart_id}; + $self->{group_condition}->{translated}->{joins}->[0]->{as} = "bugs$suffix"; + + $search_args->{full_field} =~ s/^bugs\./bugs$suffix\./; + + $search_args->{table_suffix} = $suffix; + $search_args->{bugs_table} = "bugs$suffix"; +} + +1; diff --git a/js/custom-search.js b/js/custom-search.js index 89a788073..61284c50d 100644 --- a/js/custom-search.js +++ b/js/custom-search.js @@ -28,7 +28,7 @@ function custom_search_new_row() { var row = document.getElementById('custom_search_last_row'); var clone = row.cloneNode(true); - _cs_fix_ids(clone); + _cs_fix_row_ids(clone); // We only want one copy of the buttons, in the new row. So the old // ones get deleted. @@ -43,13 +43,28 @@ function custom_search_new_row() { // Always make sure there's only one row with this id. row.id = null; row.parentNode.appendChild(clone); + cs_reconfigure(row); fix_query_string(row); return clone; } +var _cs_source_any_all; function custom_search_open_paren() { var row = document.getElementById('custom_search_last_row'); + // create a copy of j_top and use that as the source, so we can modify + // j_top if required + if (!_cs_source_any_all) { + var j_top = document.getElementById('j_top'); + _cs_source_any_all = j_top.cloneNode(true); + } + + // find the parent any/all select, and remove the grouped option + var structure = _cs_build_structure(row); + var old_id = _cs_get_row_id(row); + var parent_j = document.getElementById(_cs_get_join(structure, 'f' + old_id)); + _cs_remove_and_g(parent_j); + // If there's an "Any/All" select in this row, it needs to stay as // part of the parent paren set. var old_any_all = _remove_any_all(row); @@ -66,21 +81,20 @@ function custom_search_open_paren() { var not_for_paren = new_not[0].cloneNode(true); // Preserve the values when modifying the row. - var id = _cs_fix_ids(row, true); + var id = _cs_fix_row_ids(row, true); var prev_id = id - 1; var paren_row = row.cloneNode(false); paren_row.id = null; paren_row.innerHTML = '('; + + '" id="f' + prev_id + '" value="OP">'; paren_row.insertBefore(not_for_paren, paren_row.firstChild); row.parentNode.insertBefore(paren_row, row); - + // New paren set needs a new "Any/All" select. var any_all_container = document.createElement('div'); YAHOO.util.Dom.addClass(any_all_container, ANY_ALL_SELECT_CLASS); - var j_top = document.getElementById('j_top'); - var any_all = j_top.cloneNode(true); + var any_all = _cs_source_any_all.cloneNode(true); any_all.name = 'j' + prev_id; any_all.id = any_all.name; any_all_container.appendChild(any_all); @@ -92,6 +106,7 @@ function custom_search_open_paren() { YAHOO.util.Dom.setStyle(row, 'margin-left', new_margin + 'em'); YAHOO.util.Dom.removeClass('cp_container', 'bz_default_hidden'); + cs_reconfigure(any_all_container); fix_query_string(any_all_container); } @@ -100,7 +115,7 @@ function custom_search_close_paren() { // We need to up the new row's id by one more, because we're going // to insert a "CP" before it. - var id = _cs_fix_ids(new_row); + var id = _cs_fix_row_ids(new_row); var margin = YAHOO.util.Dom.getStyle(new_row, 'margin-left'); var int_match = margin.match(/\d+/); @@ -110,7 +125,7 @@ function custom_search_close_paren() { var paren_row = new_row.cloneNode(false); paren_row.id = null; paren_row.innerHTML = ')'; + + '" id="f' + (id - 1) + '" value="CP">'; new_row.parentNode.insertBefore(paren_row, new_row); @@ -118,6 +133,7 @@ function custom_search_close_paren() { YAHOO.util.Dom.addClass('cp_container', 'bz_default_hidden'); } + cs_reconfigure(new_row); fix_query_string(new_row); } @@ -160,19 +176,17 @@ function redirect_html4_browsers() { document.location = url; } -function _cs_fix_ids(parent, preserve_values) { +function _cs_fix_row_ids(row, preserve_values) { // Update the label of the checkbox. - var label = YAHOO.util.Dom.getElementBy(function() { return true }, - 'label', parent); + var label = YAHOO.util.Dom.getElementBy(function() { return true }, 'label', row); var id_match = label.htmlFor.match(/\d+$/); var id = parseInt(id_match[0]) + 1; label.htmlFor = label.htmlFor.replace(/\d+$/, id); - // Sets all the inputs in the parent back to their default + // Sets all the inputs in the row back to their default // and fixes their id. var fields = - YAHOO.util.Dom.getElementsByClassName('custom_search_form_field', null, - parent); + YAHOO.util.Dom.getElementsByClassName('custom_search_form_field', null, row); for (var i = 0; i < fields.length; i++) { var field = fields[i]; @@ -184,15 +198,141 @@ function _cs_fix_ids(parent, preserve_values) { field.value = ''; } } - - // Update the numeric id for the new row. + + // Update the numeric id for the row. field.name = field.name.replace(/\d+$/, id); field.id = field.name; } - + return id; } +function _cs_build_structure(form_member) { + // build a map of the structure of the custom fields + var form = YAHOO.util.Dom.getAncestorByTagName(form_member, 'form'); + var last_id = _get_last_cs_row_id(form); + var structure = [ 'j_top' ]; + var nested = [ structure ]; + for (var id = 1; id <= last_id; id++) { + var f = form['f' + id]; + if (!f || !f.parentNode.parentNode) continue; + + if (f.value == 'OP') { + var j = [ 'j' + id ]; + nested[nested.length - 1].push(j); + nested.push(j); + continue; + } else if (f.value == 'CP') { + nested.pop(); + continue; + } else { + nested[nested.length - 1].push('f' + id); + } + } + return structure; +} + +function cs_reconfigure(form_member) { + var structure = _cs_build_structure(form_member); + _cs_add_listeners(structure); + _cs_trigger_j_listeners(structure); + fix_query_string(form_member); + + var j = _cs_get_join(structure, 'f' + _get_last_cs_row_id()); + document.getElementById('op_button').disabled = document.getElementById(j).value == 'AND_G'; +} + +function _cs_add_listeners(parents) { + for (var i = 0, l = parents.length; i < l; i++) { + if (typeof(parents[i]) == 'object') { + // nested + _cs_add_listeners(parents[i]); + } else if (i == 0) { + // joiner + YAHOO.util.Event.removeListener(parents[i], 'change', _cs_j_change); + YAHOO.util.Event.addListener(parents[i], 'change', _cs_j_change, parents); + } else { + // field + YAHOO.util.Event.removeListener(parents[i], 'change', _cs_f_change); + YAHOO.util.Event.addListener(parents[i], 'change', _cs_f_change, parents); + } + } +} + +function _cs_trigger_j_listeners(fields) { + var has_children = false; + for (var i = 0, l = fields.length; i < l; i++) { + if (typeof(fields[i]) == 'undefined') { + continue; + } else if (typeof(fields[i]) == 'object') { + // nested + _cs_trigger_j_listeners(fields[i]); + has_children = true; + } else if (i == 0) { + _cs_j_change(undefined, fields); + } + } + if (has_children) { + _cs_remove_and_g(document.getElementById(fields[0])); + } +} + +function _cs_get_join(parents, field) { + for (var i = 0, l = parents.length; i < l; i++) { + if (typeof(parents[i]) == 'object') { + // nested + var result = _cs_get_join(parents[i], field); + if (result) return result; + } else if (parents[i] == field) { + return parents[0]; + } + } + return false; +} + +function _cs_remove_and_g(join_field) { + var index = bz_optionIndex(join_field, 'AND_G'); + join_field.options[index] = null; + join_field.options[bz_optionIndex(join_field, 'AND')].innerHTML = cs_and_label; + join_field.options[bz_optionIndex(join_field, 'OR')].innerHTML = cs_or_label; +} + +function _cs_j_change(evt, fields, field) { + var j = document.getElementById(fields[0]); + if (j && j.value == 'AND_G') { + for (var i = 1, l = fields.length; i < l; i++) { + if (typeof(fields[i]) == 'object') continue; + if (!field) { + field = document.getElementById(fields[i]).value; + } else { + document.getElementById(fields[i]).value = field; + } + } + if (evt) { + fix_query_string(j); + } + if ('f' + _get_last_cs_row_id() == fields[fields.length - 1]) { + document.getElementById('op_button').style.display = 'none'; + } + } else { + document.getElementById('op_button').style.display = ''; + } +} + +function _cs_f_change(evt, args) { + var field = YAHOO.util.Event.getTarget(evt); + _cs_j_change(evt, args, field.value); +} + +function _get_last_cs_row_id() { + return _cs_get_row_id('custom_search_last_row'); +} + +function _cs_get_row_id(row) { + var label = YAHOO.util.Dom.getElementBy(function() { return true }, 'label', row); + return parseInt(label.htmlFor.match(/\d+$/)[0]); +} + function _remove_any_all(parent) { var any_all = YAHOO.util.Dom.getElementsByClassName( ANY_ALL_SELECT_CLASS, null, parent); diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl index 6a34cbd0d..a7d1de6f9 100644 --- a/template/en/default/global/user-error.html.tmpl +++ b/template/en/default/global/user-error.html.tmpl @@ -1597,6 +1597,17 @@ and the "matches" search can only be used with the "content" field. + [% ELSIF error == "search_grouped_field_invalid" %] + [% terms.Bugzilla %] does not support using the + "[%+ field_descs.$field FILTER html %]" ([% field FILTER html %]) + field with grouped search conditions. + + [% ELSIF error == "search_grouped_invalid_nesting" %] + You cannot nest clauses within grouped search conditions. + + [% ELSIF error == "search_grouped_field_mismatch" %] + All conditions under a groups search must use the same field. + [% ELSIF error == "search_field_operator_invalid" %] [% terms.Bugzilla %] does not support using the "[%+ field_descs.$field FILTER html %]" ([% field FILTER html %]) diff --git a/template/en/default/search/boolean-charts.html.tmpl b/template/en/default/search/boolean-charts.html.tmpl index e37d9d80e..df369eb29 100644 --- a/template/en/default/search/boolean-charts.html.tmpl +++ b/template/en/default/search/boolean-charts.html.tmpl @@ -64,6 +64,10 @@ @@ -120,7 +124,8 @@ ( [% indent_level = indent_level + 1 %] [% ELSIF condition.f == "CP" %] - + ) [% ELSE %] - + + Match ANY of the following separately: + [% IF with_advanced_link %]