diff options
| -rw-r--r-- | Bugzilla.pm | 63 | ||||
| -rw-r--r-- | Bugzilla/Constants.pm | 15 | ||||
| -rw-r--r-- | Bugzilla/Error.pm | 16 | ||||
| -rw-r--r-- | Bugzilla/Field.pm | 2 | ||||
| -rw-r--r-- | Bugzilla/Search.pm | 61 | ||||
| -rw-r--r-- | Bugzilla/Template.pm | 4 | ||||
| -rw-r--r-- | Bugzilla/User.pm | 50 | ||||
| -rw-r--r-- | xt/lib/Bugzilla/Test/Search.pm | 2 |
8 files changed, 159 insertions, 54 deletions
diff --git a/Bugzilla.pm b/Bugzilla.pm index 33df05efb..6ecbc27db 100644 --- a/Bugzilla.pm +++ b/Bugzilla.pm @@ -523,6 +523,45 @@ sub switch_to_main_db { return $class->dbh_main; } +sub fields { + my ($class, $criteria) = @_; + $criteria ||= {}; + my $cache = $class->request_cache; + + # We create an advanced cache for fields by type, so that we + # can avoid going back to the database for every fields() call. + # (And most of our fields() calls are for getting fields by type.) + # + # We also cache fields by name, because calling $field->name a few + # million times can be slow in calling code, but if we just do it + # once here, that makes things a lot faster for callers. + if (!defined $cache->{fields}) { + my @all_fields = Bugzilla::Field->get_all; + my (%by_name, %by_type); + foreach my $field (@all_fields) { + my $name = $field->name; + $by_type{$field->type}->{$name} = $field; + $by_name{$name} = $field; + } + $cache->{fields} = { by_type => \%by_type, by_name => \%by_name }; + } + + my $fields = $cache->{fields}; + my %requested; + if (my $types = $criteria->{type}) { + $types = ref($types) ? $types : [$types]; + %requested = map { %{ $fields->{by_type}->{$_} || {} } } @$types; + } + else { + %requested = %{ $fields->{by_name} }; + } + + my $do_by_name = $criteria->{by_name}; + + return $do_by_name ? \%requested : [values %requested]; +} + +# DEPRECATED. Use fields() instead. sub get_fields { my $class = shift; my $criteria = shift; @@ -768,6 +807,30 @@ Essentially, causes calls to C<Bugzilla-E<gt>user> to return C<undef>. This has effect of logging out a user for the current request only; cookies and database sessions are left intact. +=item C<fields> + +This is the standard way to get arrays or hashes of L<Bugzilla::Field> +objects when you need them. It takes the following named arguments +in a hashref: + +=over + +=item C<by_name> + +If false (or not specified), this method will return an arrayref of +the requested fields. The order of the returned fields is random. + +If true, this method will return a hashref of fields, where the keys +are field names and the valules are L<Bugzilla::Field> objects. + +=item C<type> + +Either a single C<FIELD_TYPE_*> constant or an arrayref of them. If specified, +the returned fields will be limited to the types in the list. If you don't +specify this argument, all fields will be returned. + +=back + =item C<error_mode> Call either C<Bugzilla->error_mode(Bugzilla::Constants::ERROR_MODE_DIE)> diff --git a/Bugzilla/Constants.pm b/Bugzilla/Constants.pm index d58fe9388..2285f89b0 100644 --- a/Bugzilla/Constants.pm +++ b/Bugzilla/Constants.pm @@ -35,6 +35,7 @@ use base qw(Exporter); # For bz_locations use File::Basename; +use Memoize; @Bugzilla::Constants::EXPORT = qw( BUGZILLA_VERSION @@ -404,11 +405,11 @@ use constant EMPTY_DATETIME_REGEX => qr/^[0\-:\sA-Za-z]+$/; # See the POD for Bugzilla::Field/is_abnormal to see why these are listed # here. -use constant ABNORMAL_SELECTS => qw( - classification - product - component -); +use constant ABNORMAL_SELECTS => { + classification => 1, + component => 1, + product => 1, +}; # The fields from fielddefs that are blocked from non-timetracking users. # work_time is sometimes called actual_time. @@ -619,4 +620,8 @@ sub bz_locations { }; } +# This makes us not re-compute all the bz_locations data every time it's +# called. +BEGIN { memoize('bz_locations') }; + 1; diff --git a/Bugzilla/Error.pm b/Bugzilla/Error.pm index 649fdd486..395cc0dc9 100644 --- a/Bugzilla/Error.pm +++ b/Bugzilla/Error.pm @@ -53,12 +53,6 @@ sub _throw_error { $vars ||= {}; $vars->{error} = $error; - # Don't show function arguments, in case they contain confidential data. - local $Carp::MaxArgNums = -1; - # Don't show the error as coming from Bugzilla::Error, show it as coming - # from the caller. - local $Carp::CarpInternal{'Bugzilla::Error'} = 1; - $vars->{traceback} = Carp::longmess(); # Make sure any transaction is rolled back (if supported). # If we are within an eval(), do not roll back transactions as we are @@ -159,6 +153,16 @@ sub ThrowUserError { } sub ThrowCodeError { + my (undef, $vars) = @_; + + # Don't show function arguments, in case they contain + # confidential data. + local $Carp::MaxArgNums = -1; + # Don't show the error as coming from Bugzilla::Error, show it + # as coming from the caller. + local $Carp::CarpInternal{'Bugzilla::Error'} = 1; + $vars->{traceback} = Carp::longmess(); + _throw_error("global/code-error.html.tmpl", @_); } diff --git a/Bugzilla/Field.pm b/Bugzilla/Field.pm index 26025015a..c0e88fc37 100644 --- a/Bugzilla/Field.pm +++ b/Bugzilla/Field.pm @@ -544,7 +544,7 @@ This method returns C<1> if the field is "abnormal", C<0> otherwise. sub is_abnormal { my $self = shift; - return grep($_ eq $self->name, ABNORMAL_SELECTS) ? 1 : 0; + return ABNORMAL_SELECTS->{$self->name} ? 1 : 0; } sub legal_values { diff --git a/Bugzilla/Search.pm b/Bugzilla/Search.pm index 3653cff67..63e458655 100644 --- a/Bugzilla/Search.pm +++ b/Bugzilla/Search.pm @@ -461,13 +461,11 @@ sub init { my %special_order = %{SPECIAL_ORDER()}; my %special_order_join = %{SPECIAL_ORDER_JOIN()}; - my @select_fields = - Bugzilla->get_fields({ type => FIELD_TYPE_SINGLE_SELECT }); + my $select_fields = Bugzilla->fields({ type => FIELD_TYPE_SINGLE_SELECT }); - my @multi_select_fields = Bugzilla->get_fields({ - type => [FIELD_TYPE_MULTI_SELECT, FIELD_TYPE_BUG_URLS], - obsolete => 0 }); - foreach my $field (@select_fields) { + my $multi_select_fields = Bugzilla->fields({ + type => [FIELD_TYPE_MULTI_SELECT, FIELD_TYPE_BUG_URLS]}); + foreach my $field (@$select_fields) { next if $field->is_abnormal; my $name = $field->name; $special_order{$name} = [ "$name.sortkey", "$name.value" ], @@ -522,7 +520,7 @@ sub init { push(@supptables, "LEFT JOIN longdescs AS ldtime " . "ON ldtime.bug_id = bugs.bug_id"); } - foreach my $field (@multi_select_fields) { + foreach my $field (@$multi_select_fields) { my $field_name = $field->name; next if !grep($_ eq $field_name, @fields); push(@supptables, "LEFT JOIN bug_$field_name AS map_bug_$field_name" @@ -594,15 +592,18 @@ sub init { # All fields that don't have a . in their name should be specifyable # in the URL directly. - my @legal_fields = grep { $_->name !~ /\./ } Bugzilla->get_fields; + my $legal_fields = Bugzilla->fields({ by_name => 1 }); if (!$user->is_timetracker) { - foreach my $field (TIMETRACKING_FIELDS) { - @legal_fields = grep { $_->name ne $field } @legal_fields; + foreach my $name (TIMETRACKING_FIELDS) { + delete $legal_fields->{$name}; } } + foreach my $name (keys %$legal_fields) { + delete $legal_fields->{$name} if $name =~ /\./; + } foreach my $field ($params->param()) { - if (grep { $_->name eq $field } @legal_fields) { + if ($legal_fields->{$field}) { my $type = $params->param("${field}_type"); if (!$type) { if ($field eq 'keywords') { @@ -940,12 +941,11 @@ sub init { # $suppstring = String which is pasted into query containing all table names # get a list of field names to verify the user-submitted chart fields against - my %chartfields = @{$dbh->selectcol_arrayref( - q{SELECT name, id FROM fielddefs}, { Columns=>[1,2] })}; + my $chart_fields = Bugzilla->fields({ by_name => 1 }); if (!$user->is_timetracker) { foreach my $tt_field (TIMETRACKING_FIELDS) { - delete $chartfields{$tt_field}; + delete $chart_fields->{$tt_field}; } } @@ -976,12 +976,12 @@ sub init { # chart -1 is generated by other code above, not from the user- # submitted form, so we'll blindly accept any values in chart -1 - if (!$chartfields{$field} and $chart != -1) { + if (!$chart_fields->{$field} and $chart != -1) { ThrowCodeError("invalid_field_name", { field => $field }); } # This is either from the internal chart (in which case we - # already know about it), or it was in %chartfields, so it is + # already know about it), or it was in $chart_fields, so it is # a valid field name, which means that it's ok. trick_taint($field); my $quoted = $dbh->quote($value); @@ -996,13 +996,13 @@ sub init { operator => $operator, value => $value, quoted => $quoted, - multi_fields => \@multi_select_fields, + multi_fields => $multi_select_fields, joins => \@supptables, where => \@wherepart, having => \@having, group_by => \@groupby, fields => \@fields, - chart_fields => \%chartfields, + chart_fields => $chart_fields, ); # This should add a "term" selement to %search_args. $self->do_search_function(\%search_args); @@ -2358,13 +2358,16 @@ sub _nowords { sub _changedbefore_changedafter { my ($self, $args) = @_; - my ($chart_id, $joins, $field, $operator, $value) = - @$args{qw(chart_id joins field operator value)}; + my ($chart_id, $joins, $field, $operator, $value, $chart_fields) = + @$args{qw(chart_id joins field operator value chart_fields)}; my $dbh = Bugzilla->dbh; my $sql_operator = ($operator =~ /before/) ? '<' : '>'; my $table = "act_$chart_id"; - my $field_id = get_field_id($field); + my $field_object = $chart_fields->{$field} + || ThrowCodeError("invalid_field_name", { field => $field }); + my $field_id = $field_object->id; + my $sql_date = $dbh->quote(SqlifyDate($value)); push(@$joins, "LEFT JOIN bugs_activity AS $table" @@ -2376,12 +2379,14 @@ sub _changedbefore_changedafter { sub _changedfrom_changedto { my ($self, $args) = @_; - my ($chart_id, $joins, $field, $operator, $quoted) = - @$args{qw(chart_id joins field operator quoted)}; + my ($chart_id, $joins, $field, $operator, $quoted, $chart_fields) = + @$args{qw(chart_id joins field operator quoted chart_fields)}; my $column = ($operator =~ /from/) ? 'removed' : 'added'; my $table = "act_$chart_id"; - my $field_id = get_field_id($field); + my $field_object = $chart_fields->{$field} + || ThrowCodeError("invalid_field_name", { field => $field }); + my $field_id = $field_object->id; push(@$joins, "LEFT JOIN bugs_activity AS $table" . " ON $table.bug_id = bugs.bug_id" @@ -2392,11 +2397,13 @@ sub _changedfrom_changedto { sub _changedby { my ($self, $args) = @_; - my ($chart_id, $joins, $field, $operator, $value) = - @$args{qw(chart_id joins field operator value)}; + my ($chart_id, $joins, $field, $operator, $value, $chart_fields) = + @$args{qw(chart_id joins field operator value chart_fields)}; my $table = "act_$chart_id"; - my $field_id = get_field_id($field); + my $field_object = $chart_fields->{$field} + || ThrowCodeError("invalid_field_name", { field => $field }); + my $field_id = $field_object->id; my $user_id = login_to_id($value, THROW_ERROR); push(@$joins, "LEFT JOIN bugs_activity AS $table" diff --git a/Bugzilla/Template.pm b/Bugzilla/Template.pm index 4bd3d2d77..2c2d402a3 100644 --- a/Bugzilla/Template.pm +++ b/Bugzilla/Template.pm @@ -765,8 +765,8 @@ sub create { # A way for all templates to get at Field data, cached. 'bug_fields' => sub { my $cache = Bugzilla->request_cache; - $cache->{template_bug_fields} ||= - { map { $_->name => $_ } Bugzilla->get_fields() }; + $cache->{template_bug_fields} ||= + Bugzilla->fields({ by_name => 1 }); return $cache->{template_bug_fields}; }, diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm index 78e92eca0..09af233ec 100644 --- a/Bugzilla/User.pm +++ b/Bugzilla/User.pm @@ -603,18 +603,28 @@ sub groups { return $self->{groups}; } +# It turns out that calling ->id on objects a few hundred thousand +# times is pretty slow. (It showed up as a significant time contributor +# when profiling xt/search.t.) So we cache the group ids separately from +# groups for functions that need the group ids. +sub _group_ids { + my ($self) = @_; + $self->{group_ids} ||= [map { $_->id } @{ $self->groups }]; + return $self->{group_ids}; +} + sub groups_as_string { my $self = shift; - my @ids = map { $_->id } @{ $self->groups }; - return scalar(@ids) ? join(',', @ids) : '-1'; + my $ids = $self->_group_ids; + return scalar(@$ids) ? join(',', @$ids) : '-1'; } sub groups_in_sql { my ($self, $field) = @_; $field ||= 'group_id'; - my @ids = map { $_->id } @{ $self->groups }; - @ids = (-1) if !scalar @ids; - return Bugzilla->dbh->sql_in($field, \@ids); + my $ids = $self->_group_ids; + $ids = [-1] if !scalar @$ids; + return Bugzilla->dbh->sql_in($field, $ids); } sub bless_groups { @@ -1096,7 +1106,7 @@ sub queryshare_groups { } } else { - @queryshare_groups = map { $_->id } @{ $self->groups }; + @queryshare_groups = @{ $self->_group_ids }; } } @@ -1848,15 +1858,31 @@ sub is_available_username { return 1; } +# This is used in a few performance-critical areas where we don't want to +# do check() and pull all the user data from the database. sub login_to_id { my ($login, $throw_error) = @_; my $dbh = Bugzilla->dbh; - # No need to validate $login -- it will be used by the following SELECT - # statement only, so it's safe to simply trick_taint. - trick_taint($login); - my $user_id = $dbh->selectrow_array("SELECT userid FROM profiles WHERE " . - $dbh->sql_istrcmp('login_name', '?'), - undef, $login); + my $cache = Bugzilla->request_cache->{user_login_to_id} ||= {}; + + # We cache lookups because this function showed up as taking up a + # significant amount of time in profiles of xt/search.t. However, + # for users that don't exist, we re-do the check every time, because + # otherwise we break is_available_username. + my $user_id; + if (defined $cache->{$login}) { + $user_id = $cache->{$login}; + } + else { + # No need to validate $login -- it will be used by the following SELECT + # statement only, so it's safe to simply trick_taint. + trick_taint($login); + $user_id = $dbh->selectrow_array( + "SELECT userid FROM profiles + WHERE " . $dbh->sql_istrcmp('login_name', '?'), undef, $login); + $cache->{$login} = $user_id; + } + if ($user_id) { return $user_id; } elsif ($throw_error) { diff --git a/xt/lib/Bugzilla/Test/Search.pm b/xt/lib/Bugzilla/Test/Search.pm index af595e373..87df927ca 100644 --- a/xt/lib/Bugzilla/Test/Search.pm +++ b/xt/lib/Bugzilla/Test/Search.pm @@ -147,7 +147,7 @@ sub all_fields { my $self = shift; if (not $self->{all_fields}) { $self->_create_custom_fields(); - my @fields = Bugzilla->get_fields; + my @fields = @{ Bugzilla->fields }; @fields = sort { $a->name cmp $b->name } @fields; $self->{all_fields} = \@fields; } |