diff options
| -rw-r--r-- | Bugzilla/Search.pm | 178 | ||||
| -rwxr-xr-x | buglist.cgi | 47 | ||||
| -rwxr-xr-x | collectstats.pl | 3 | ||||
| -rw-r--r-- | extensions/MyDashboard/lib/Queries.pm | 7 | ||||
| -rwxr-xr-x | report.cgi | 8 | ||||
| -rw-r--r-- | template/en/default/list/list.html.tmpl | 11 | ||||
| -rw-r--r-- | template/en/default/reports/report.html.tmpl | 4 | ||||
| -rwxr-xr-x | whine.pl | 9 | ||||
| -rw-r--r-- | xt/lib/Bugzilla/Test/Search/FieldTest.pm | 11 |
9 files changed, 222 insertions, 56 deletions
diff --git a/Bugzilla/Search.pm b/Bugzilla/Search.pm index 0cf618a4c..5fb2352a5 100644 --- a/Bugzilla/Search.pm +++ b/Bugzilla/Search.pm @@ -57,9 +57,10 @@ use Data::Dumper; use Date::Format; use Date::Parse; use Scalar::Util qw(blessed); -use List::MoreUtils qw(all part uniq); +use List::MoreUtils qw(all firstidx part uniq); use POSIX qw(INT_MAX); use Storable qw(dclone); +use Time::HiRes qw(gettimeofday tv_interval); # Description Of Boolean Charts # ----------------------------- @@ -708,7 +709,70 @@ sub new { # Public Accessors # #################### -sub sql { +sub data { + my $self = shift; + return $self->{data} if $self->{data}; + my $dbh = Bugzilla->dbh; + + # If all fields belong to the 'bugs' table, there is no need to split + # the original query into two pieces. Else we override the 'fields' + # argument to first get bug IDs based on the search criteria defined + # by the caller, and the desired fields are collected in the 2nd query. + my @orig_fields = $self->_input_columns; + my $all_in_bugs_table = 1; + foreach my $field (@orig_fields) { + next if $self->COLUMNS->{$field}->{name} =~ /^bugs\.\w+$/; + $self->{fields} = ['bug_id']; + $all_in_bugs_table = 0; + last; + } + + my $start_time = [gettimeofday()]; + my $sql = $self->_sql; + # Do we just want bug IDs to pass to the 2nd query or all the data immediately? + my $func = $all_in_bugs_table ? 'selectall_arrayref' : 'selectcol_arrayref'; + my $bug_ids = $dbh->$func($sql); + my @extra_data = ({sql => $sql, time => tv_interval($start_time)}); + # Restore the original 'fields' argument, just in case. + $self->{fields} = \@orig_fields unless $all_in_bugs_table; + + # If there are no bugs found, or all fields are in the 'bugs' table, + # there is no need for another query. + if (!scalar @$bug_ids || $all_in_bugs_table) { + $self->{data} = $bug_ids; + return wantarray ? ($self->{data}, \@extra_data) : $self->{data}; + } + + # Make sure the bug_id will be returned. If not, append it to the list. + my $pos = firstidx { $_ eq 'bug_id' } @orig_fields; + if ($pos < 0) { + push(@orig_fields, 'bug_id'); + $pos = $#orig_fields; + } + + # Now create a query with the buglist above as the single criteria + # and the fields that the caller wants. No need to redo security checks; + # the list has already been validated above. + my $search = $self->new('fields' => \@orig_fields, + 'params' => {bug_id => $bug_ids, bug_id_type => 'anyexact'}, + 'sharer' => $self->_sharer_id, + 'user' => $self->_user, + 'allow_unlimited' => 1, + '_no_security_check' => 1); + + $start_time = [gettimeofday()]; + $sql = $search->_sql; + my $unsorted_data = $dbh->selectall_arrayref($sql); + push(@extra_data, {sql => $sql, time => tv_interval($start_time)}); + # Let's sort the data. We didn't do it in the query itself because + # we already know in which order to sort bugs thanks to the first query, + # and this avoids additional table joins in the SQL query. + my %data = map { $_->[$pos] => $_ } @$unsorted_data; + $self->{data} = [map { $data{$_} } @$bug_ids]; + return wantarray ? ($self->{data}, \@extra_data) : $self->{data}; +} + +sub _sql { my ($self) = @_; return $self->{sql} if $self->{sql}; my $dbh = Bugzilla->dbh; @@ -742,7 +806,7 @@ sub search_description { # Make sure that the description has actually been generated if # people are asking for the whole thing. else { - $self->sql; + $self->_sql; } return $self->{'search_description'}; } @@ -1100,6 +1164,7 @@ sub _standard_joins { my ($self) = @_; my $user = $self->_user; my @joins; + return () if $self->{_no_security_check}; my $security_join = { table => 'bug_group_map', @@ -1176,6 +1241,7 @@ sub _translate_join { # group security. sub _standard_where { my ($self) = @_; + return ('1=1') if $self->{_no_security_check}; # If replication lags badly between the shadow db and the main DB, # it's possible for bugs to show up in searches before their group # controls are properly set. To prevent this, when initially creating @@ -3020,3 +3086,109 @@ sub translate_old_column { } 1; + +__END__ + +=head1 NAME + +Bugzilla::Search - Provides methods to run queries against bugs. + +=head1 SYNOPSIS + + use Bugzilla::Search; + + my $search = new Bugzilla::Search({'fields' => \@fields, + 'params' => \%search_criteria, + 'sharer' => $sharer_id, + 'user' => $user_obj, + 'allow_unlimited' => 1}); + + my $data = $search->data; + my ($data, $extra_data) = $search->data; + +=head1 DESCRIPTION + +Search.pm represents a search object. It's the single way to collect +data about bugs in a secure way. The list of bugs matching criteria +defined by the caller are filtered based on the user privileges. + +=head1 METHODS + +=head2 new + +=over + +=item B<Description> + +Create a Bugzilla::Search object. + +=item B<Params> + +=over + +=item C<fields> + +An arrayref representing the bug attributes for which data is desired. +Legal attributes are listed in the fielddefs DB table. At least one field +must be defined, typically the 'bug_id' field. + +=item C<params> + +A hashref representing search criteria. Each key => value pair represents +a search criteria, where the key is the search field and the value is the +value for this field. At least one search criteria must be defined if the +'search_allow_no_criteria' parameter is turned off, else an error is thrown. + +=item C<sharer> + +When a saved search is shared by a user, this is his user ID. + +=item C<user> + +A L<Bugzilla::User> object representing the user to whom the data is addressed. +All security checks are done based on this user object, so it's not safe +to share results of the query with other users as not all users have the +same privileges or have the same role for all bugs in the list. If this +parameter is not defined, then the currently logged in user is taken into +account. If no user is logged in, then only public bugs will be returned. + +=item C<allow_unlimited> + +If set to a true value, the number of bugs retrieved by the query is not +limited. + +=back + +=item B<Returns> + +A L<Bugzilla::Search> object. + +=back + +=head2 data + +=over + +=item B<Description> + +Returns bugs matching search criteria passed to C<new()>. + +=item B<Params> + +None + +=item B<Returns> + +In scalar context, this method returns a reference to a list of bugs. +Each item of the list represents a bug, which is itself a reference to +a list where each item represents a bug attribute, in the same order as +specified in the C<fields> parameter of C<new()>. + +In list context, this methods also returns a reference to a list containing +references to hashes. For each hash, two keys are defined: C<sql> contains +the SQL query which has been executed, and C<time> contains the time spent +to execute the SQL query, in seconds. There can be either a single hash, or +two hashes if two SQL queries have been executed sequentially to get all the +required data. + +=back diff --git a/buglist.cgi b/buglist.cgi index 517aa4507..2d81ed56d 100755 --- a/buglist.cgi +++ b/buglist.cgi @@ -51,7 +51,6 @@ use Bugzilla::Status; use Bugzilla::Token; use Date::Parse; -use Time::HiRes qw(gettimeofday tv_interval); my $cgi = Bugzilla->cgi; my $dbh = Bugzilla->dbh; @@ -776,8 +775,6 @@ my $search = new Bugzilla::Search('fields' => \@selectcolumns, 'params' => scalar $params->Vars, 'order' => \@orderstrings, 'sharer' => $sharer_id); -my $query = $search->sql; -$vars->{'search_description'} = $search->search_description; # We don't want saved searches and other buglist things to save # our default limit. @@ -787,21 +784,6 @@ $params->delete('limit') if $vars->{'default_limited'}; # Query Execution ################################################################################ -if ($cgi->param('debug') - && Bugzilla->params->{debug_group} - && $user->in_group(Bugzilla->params->{debug_group}) -) { - $vars->{'debug'} = 1; - $vars->{'query'} = $query; - # Explains are limited to admins because you could use them to figure - # out how many hidden bugs are in a particular product (by doing - # searches and looking at the number of rows the explain says it's - # examining). - if (Bugzilla->user->in_group('admin')) { - $vars->{'query_explain'} = $dbh->bz_explain($query); - } -} - # Time to use server push to display an interim message to the user until # the query completes and we can display the bug list. if ($serverpush) { @@ -834,11 +816,28 @@ $::SIG{TERM} = 'DEFAULT'; $::SIG{PIPE} = 'DEFAULT'; # Execute the query. -my $start_time = [gettimeofday()]; -my $buglist_sth = $dbh->prepare($query); -$buglist_sth->execute(); -$vars->{query_time} = tv_interval($start_time); +my ($data, $extra_data) = $search->data; +$vars->{'search_description'} = $search->search_description; +if ($cgi->param('debug') + && Bugzilla->params->{debug_group} + && $user->in_group(Bugzilla->params->{debug_group}) +) { + $vars->{'debug'} = 1; + $vars->{'queries'} = $extra_data; + my $query_time = 0; + $query_time += $_->{'time'} foreach @$extra_data; + $vars->{'query_time'} = $query_time; + # Explains are limited to admins because you could use them to figure + # out how many hidden bugs are in a particular product (by doing + # searches and looking at the number of rows the explain says it's + # examining). + if ($user->in_group('admin')) { + foreach my $query (@$extra_data) { + $query->{explain} = $dbh->bz_explain($query->{sql}); + } + } +} ################################################################################ # Results Retrieval @@ -870,14 +869,14 @@ my @bugidlist; my @bugs; # the list of records -while (my @row = $buglist_sth->fetchrow_array()) { +foreach my $row (@$data) { my $bug = {}; # a record # Slurp the row of data into the record. # The second from last column in the record is the number of groups # to which the bug is restricted. foreach my $column (@selectcolumns) { - $bug->{$column} = shift @row; + $bug->{$column} = shift @$row; } # Process certain values further (i.e. date format conversion). diff --git a/collectstats.pl b/collectstats.pl index 1487e5a72..c5db30b5f 100755 --- a/collectstats.pl +++ b/collectstats.pl @@ -504,8 +504,7 @@ sub CollectSeriesData { 'fields' => ["bug_id"], 'allow_unlimited' => 1, 'user' => $user); - my $sql = $search->sql; - $data = $shadow_dbh->selectall_arrayref($sql); + $data = $search->data; }; if (!$@) { diff --git a/extensions/MyDashboard/lib/Queries.pm b/extensions/MyDashboard/lib/Queries.pm index 47778336f..1dc0ecf5a 100644 --- a/extensions/MyDashboard/lib/Queries.pm +++ b/extensions/MyDashboard/lib/Queries.pm @@ -145,13 +145,10 @@ sub query_bugs { params => scalar $params->Vars, order => [ QUERY_ORDER ]); - my $query = $search->sql(); - my $sth = $dbh->prepare($query); - $sth->execute(); - my $rows = $sth->fetchall_arrayref(); + my $data = $search->data; my @bugs; - foreach my $row (@$rows) { + foreach my $row (@$data) { my $bug = {}; foreach my $column (SELECT_COLUMNS) { $bug->{$column} = shift @$row; diff --git a/report.cgi b/report.cgi index 7bff62be9..decbaeeb2 100755 --- a/report.cgi +++ b/report.cgi @@ -131,13 +131,12 @@ my $search = new Bugzilla::Search( params => scalar $params->Vars, allow_unlimited => 1, ); -my $query = $search->sql; $::SIG{TERM} = 'DEFAULT'; $::SIG{PIPE} = 'DEFAULT'; -my $dbh = Bugzilla->switch_to_shadow_db(); -my $results = $dbh->selectall_arrayref($query); +Bugzilla->switch_to_shadow_db(); +my ($results, $extra_data) = $search->data; # We have a hash of hashes for the data itself, and a hash to hold the # row/col/table names. @@ -224,8 +223,7 @@ if ($width && $formatparam eq "bar") { $vars->{'width'} = $width if $width; $vars->{'height'} = $height if $height; - -$vars->{'query'} = $query; +$vars->{'queries'} = $extra_data; if ($cgi->param('debug') && Bugzilla->params->{debug_group} diff --git a/template/en/default/list/list.html.tmpl b/template/en/default/list/list.html.tmpl index 055cd2e8c..cda06ac21 100644 --- a/template/en/default/list/list.html.tmpl +++ b/template/en/default/list/list.html.tmpl @@ -60,10 +60,13 @@ [% IF debug %] <div class="bz_query_debug"> - <p>[% query FILTER html %]</p> - <p>Execution time: [% query_time FILTER html %] seconds</p> - [% IF query_explain.defined %] - <pre>[% query_explain FILTER html %]</pre> + <p>Total execution time: [% query_time FILTER html %] seconds</p> + [% FOREACH query = queries %] + <p>[% query.sql FILTER html %]</p> + <p>Execution time: [% query.time FILTER html %] seconds</p> + [% IF query.explain %] + <pre>[% query.explain FILTER html %]</pre> + [% END %] [% END %] </div> [% END %] diff --git a/template/en/default/reports/report.html.tmpl b/template/en/default/reports/report.html.tmpl index 94725ae81..38b64df0b 100644 --- a/template/en/default/reports/report.html.tmpl +++ b/template/en/default/reports/report.html.tmpl @@ -81,7 +81,9 @@ %] [% IF debug %] - <p>[% query FILTER html %]</p> + [% FOREACH query = queries %] + <p>[% query.sql FILTER html %]</p> + [% END %] [% END %] <div align="center"> @@ -453,7 +453,7 @@ sub run_queries { 'user' => $args->{'recipient'}, # the search runs as the recipient ); # If a query fails for whatever reason, it shouldn't kill the script. - my $sqlquery = eval { $search->sql }; + my $data = eval { $search->data }; if ($@) { print STDERR get_text('whine_query_failed', { query_name => $thisquery->{'name'}, author => $args->{'author'}, @@ -461,15 +461,12 @@ sub run_queries { next; } - $sth = $dbh->prepare($sqlquery); - $sth->execute; - - while (my @row = $sth->fetchrow_array) { + foreach my $row (@$data) { my $bug = {}; for my $field (@searchfields) { my $fieldname = $field; $fieldname =~ s/^bugs\.//; # No need for bugs.whatever - $bug->{$fieldname} = shift @row; + $bug->{$fieldname} = shift @$row; } if ($thisquery->{'onemailperbug'}) { diff --git a/xt/lib/Bugzilla/Test/Search/FieldTest.pm b/xt/lib/Bugzilla/Test/Search/FieldTest.pm index 283a90d16..bd5fd905a 100644 --- a/xt/lib/Bugzilla/Test/Search/FieldTest.pm +++ b/xt/lib/Bugzilla/Test/Search/FieldTest.pm @@ -544,13 +544,13 @@ sub do_tests { my $sql; TODO: { local $TODO = $search_broken if $search_broken; - lives_ok { $sql = $search->sql } "$name: generate SQL"; + lives_ok { $sql = $search->_sql } "$name: generate SQL"; } my $results; SKIP: { skip "Can't run SQL without any SQL", 1 if !defined $sql; - $results = $self->_test_sql($sql); + $results = $self->_test_sql($search); } $self->_test_content($results, $sql); @@ -567,12 +567,11 @@ sub _test_search_object_creation { } sub _test_sql { - my ($self, $sql) = @_; - my $dbh = Bugzilla->dbh; + my ($self, $search) = @_; my $name = $self->name; my $results; - lives_ok { $results = $dbh->selectall_arrayref($sql) } "$name: Run SQL Query" - or diag($sql); + lives_ok { $results = $search->data } "$name: Run SQL Query" + or diag($search->_sql); return $results; } |