450546 - Use visible_bugs() where appropriate instead of/in combination with can_see_bug() to improve performance

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Max Kanat-Alexander]

Anywhere that loops over a list of bugs with can_see_bug should be using visible_bugs instead.

Comment 1

[Frédéric Buclin]

Due to security reasons (see bug 486881), be sure to flush the cache everytime you edit a bug or edit user groups, else the cache will contain old data.

Comment 2

[Koosha KM]

Attachment: patch - v1

I Couldn't find such code in other places.

Comment 3

[Frédéric Buclin]

This is not a bug (even a trivial one) -> enhancement

Comment 4

[Frédéric Buclin]

Comment on attachment 643558 [details] [diff] [review]
patch - v1

There are much more places to fix besides summarize_time.cgi: showdependencygraph.cgi, showdependencytree.cgi, show_bug.cgi, extensions/Voting/Extension.pm.

They all require visible_bugs() to be called early, before each individual call to can_see_bug().

Comment 5

[Koosha KM]

Could you please be more specific? 

How am I supposed to use visible_bugs() for codes like this:

if (!$bugs->{$dep_id}->{'error'}
            && Bugzilla->user->can_see_bug($dep_id)
            && (!$maxdepth || $depth <= $maxdepth)
            && ($bugs->{$dep_id}->isopened || !$hide_resolved))
         {

Comment 6

[Frédéric Buclin]

(In reply to Koosha Khajeh Moogahi [:koosha] from comment #5)
> How am I supposed to use visible_bugs() for codes like this:

You call visible_bugs(\@dependencies) before the |foreach my $dep_id (@dependencies)| loop, this way can_see_bug() won't trigger a call to the DB as the data will already be in the cache thanks to visible_bugs().

Comment 7

[Koosha KM]

Attachment: patch - v1.1

Comment 8

[Koosha KM]

Attachment: patch - v1.1

Comment 9

[Koosha KM]

Attachment: patch - v1.1

Comment 10

[Frédéric Buclin]

Comment on attachment 648324 [details] [diff] [review]
patch - v1.1

>=== modified file 'show_bug.cgi'

>     foreach my $id ($cgi->param('id')) {
>         # Be kind enough and accept URLs of the form: id=1,2,3.
>         my @ids = split(/,/, $id);
>+        detaint_natural($_) foreach @ids;
>+        $user->visible_bugs(\@ids);
>         foreach (@ids) {
>             my $bug = new Bugzilla::Bug($_);

There are two problems here:
- it's legal to pass a bug alias, and detaint_natural() will kill it.
- the "Long Format" of buglists passes one bug ID per 'id' argument instead of concatenating them. If we really want to see an improvement here, we should either collect all IDs first, or fix list/list.html.tmpl to concatenate them (which is easier to do).



>=== modified file 'showdependencygraph.cgi'

>+$user->visible_bugs([keys %seen]);
> foreach my $k (keys(%seen)) {

Nit: as we need keys twice, lets define |my @bug_ids = keys %seen| and use @bug_ids at both places instead.


Otherwise looks good.

Comment 11

[Koosha KM]

Attachment: patch

Comment 12

[Frédéric Buclin]

Comment on attachment 653154 [details] [diff] [review]
patch

>=== modified file 'show_bug.cgi'

>+        trick_taint($_) foreach @ids;
>+        $user->visible_bugs(\@ids);

Wow no. visible_bugs() doesn't accept bug aliases, and you are opening a door for SQL injection, see bug 783794.


Everything else is fine.

Comment 13

[Koosha KM]

Your comment 10 and comment 12 seem contradictory!

Comment 14

[Frédéric Buclin]

(In reply to Koosha Khajeh Moogahi [:koosha] from comment #13)
> Your comment 10 and comment 12 seem contradictory!

They are not. In comment 10, I say it's legal to pass a bug alias to show_bug.cgi, because Bugzilla::Bug->new() knows what to do with it. But your patch includes visible_bugs() which is only able to take bug IDs and bug objects into account, but not bug aliases.

Collecting all bug objects first and then passing them to visible_bugs() would fix the problem.

Comment 15

[Koosha KM]

Attachment: patch

Comment 16

[Frédéric Buclin]

Comment on attachment 653167 [details] [diff] [review]
patch

>=== modified file 'show_bug.cgi'

>+        $user->visible_bugs(\@all_bugs);

Due to the stupid way Bugzilla::Bug->new works (for historical reasons), this query is going to make Bugzilla crash with your patch:

http://localhost/bugzilla/show_bug.cgi?id=vgrrg&format=multiple (assuming you have no alias named "vgrrg")

When creating each bug object, you really have to check $bug->{error} and only add bugs with no errors to @all_bugs. For the other ones, you already know something went wrong, and you can immediately add them to @illegal_bugs.

Comment 17

[Koosha KM]

Attachment: patch - v2

Comment 18

[Frédéric Buclin]

Comment on attachment 658500 [details] [diff] [review]
patch - v2

>=== modified file 'show_bug.cgi'

>+        my @all_bugs;

Now that we no longer add all bugs in this array, we should rename it to @check_bugs to make it clearer that this array will contain bugs which need to be checked.


>+                push(@illegal_bugs, { bug_id => trim($bug->bug_id),

Do not use $bug->bug_id here for bugs which do not exist, because I'm going to remove this hack in bug 509734. Use $bug_id as it's done currently.


>+        foreach my $bug (@all_bugs) {
>+            if ($user->can_see_bug($bug->bug_id)) {
>                 push(@bugs, $bug);
>             }
>-            else {
>-                push(@illegal_bugs, { bug_id => trim($bug_id),
>-                                      error => $bug->{error} || 'NotPermitted' });
>-            }

This must stay, else bugs which you cannot see are no longer listed at all.


This can be fixed on checkin and everything else looks good. r=LpSolit

Comment 19

[Frédéric Buclin]

We are late in the game for 4.4, but this patch can potentially improve performance, so let's take it.

Comment 20

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified show_bug.cgi
modified showdependencygraph.cgi
modified showdependencytree.cgi
modified summarize_time.cgi
modified extensions/Voting/Extension.pm
modified template/en/default/list/list.html.tmpl
Committed revision 8398.


Committing to: bzr+ssh://[e-mail]/bugzilla/4.4/
modified show_bug.cgi
modified showdependencygraph.cgi
modified showdependencytree.cgi
modified summarize_time.cgi
modified extensions/Voting/Extension.pm
modified template/en/default/list/list.html.tmpl
Committed revision 8394.