If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Implement Bugzilla::User::visible_bugs and have can_see_bug use it

RESOLVED FIXED in Bugzilla 3.4

Status

()

Bugzilla
Creating/Changing Bugs
--
enhancement
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: Max Kanat-Alexander, Assigned: Max Kanat-Alexander)

Tracking

(Blocks: 1 bug)

3.1.4
Bugzilla 3.4
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 1 obsolete attachment)

v2
5.33 KB, patch
Frédéric Buclin
: review+
Details | Diff | Splinter Review
(Assignee)

Description

9 years ago
Created attachment 325940 [details] [diff] [review]
v1

We should have a method that takes an array of bugs and returns only the ones you can see. We could then use this to implement things like Bugzilla::Bug->check_list. I also plan to remove all the permissions-checking code from Search.pm's "big query" and just have it use this, at some point.

Anyhow, the simplest way to get started is just to have can_see_bug use it.

I've tested this pretty well and it worked in all the cases I tested it in.

Note that I also modified the SQL query to use a DISTINCT instead of a GROUP BY, which I *think* will be faster and is definitely cleaner.

This still keeps the $sth->prepare optimization that can_see_bug had.

I also cache whether or not a bug is visible, so that we never check more than once in a single page whether or not any particular bug is visible to the logged-in user. This makes it safe to call can_see_bug or visible_bugs many times on the same bug, if that makes your code simpler.
Attachment #325940 - Flags: review?(bugreport)
Attachment #325940 - Flags: review?(LpSolit)

Comment 1

9 years ago
Comment on attachment 325940 [details] [diff] [review]
v1

>+    # Allow users to pass in Bug objects and bug ids both.
>+    my @bug_ids = map { blessed $_ ? $_->id : $_ } @$bugs;

I don't think you need Scalar::Util::blessed to do this. ref() should work nicely (one less dependency).


>+    return [grep { $visible_cache->{$_} } @bug_ids];

I think it's a bad idea to return an arrayref. If I pass a list of bugs to validate, I would prefer to get a hashref back (with the bug ID as key and 0/1 as value), so that I don't need to review the returned result in the same order I passed bugs to the method. This is also less prone to errors for customized code (imagine someone looking for the position of a bug ID in the list, and is wrong by one position for some reason; too dangerous).
Attachment #325940 - Flags: review?(bugreport)
Attachment #325940 - Flags: review?(LpSolit)
Attachment #325940 - Flags: review-
(Assignee)

Comment 2

9 years ago
(In reply to comment #1)
> I don't think you need Scalar::Util::blessed to do this. ref() should work
> nicely (one less dependency).

  Actually, Scalar::Util::blessed is considered the "right" way to do this, and "ref" is considered the "wrong" way. (As "ref" would still return true if it was an unblessed hashref or arrayref.) Scalar::Util is part of Perl core.

> I think it's a bad idea to return an arrayref. If I pass a list of bugs to
> validate, I would prefer to get a hashref back (with the bug ID as key and 0/1
> as value), so that I don't need to review the returned result in the same order
> I passed bugs to the method. This is also less prone to errors for customized
> code (imagine someone looking for the position of a bug ID in the list, and is
> wrong by one position for some reason; too dangerous).

  Actually, the reason I returned an arrayref is that I was passed an arrayref, and I want to preserve the order. That's the whole reason I wrote the function like I did--so that we could re-use this code in Search.pm and preserve the order of the passed-in arrayref. If you want to know if a bug is visible or not, you should be using can_see_bug (or a possible can_see_bugs that we'll make in the future). If you just want invisible bugs filtered out of the list, you should be using visible_bugs.
(Assignee)

Comment 3

9 years ago
Comment on attachment 325940 [details] [diff] [review]
v1

Re-requesting review, given above comment.
Attachment #325940 - Flags: review- → review?(LpSolit)
(Assignee)

Comment 4

9 years ago
Created attachment 329150 [details] [diff] [review]
v2

This version returns bug objects if you pass it bug objects. (It also deals just fine with a mix of bug objects and bug ids, but I hope that no Bugzilla code ever does that.)

I definitely want this to return an arrayref. For example, this would be extremely handy for filtering the results of Bugzilla::Bug->match.
Attachment #325940 - Attachment is obsolete: true
Attachment #329150 - Flags: review?(bbaetz)
Attachment #329150 - Flags: review?(LpSolit)
Attachment #325940 - Flags: review?(LpSolit)
Comment on attachment 329150 [details] [diff] [review]
v2

Hmm. The distinct doesn't actually filter out duplicate rows, because a bug may be in multiple groups that the user isn't in - you'll get one row for each group, but the previous query only returned one row total. I don't think that that matters in practice since most bugs will be in a very small number of groups.

Do you need to flush the cache for mod_perl users? (ie is Bugzilla->user cached across page loads?)

Do you know why 'creation_ts IS NOT NULL' is in there, BTW?

It looks OK, but I need to think about it a bit more.

(Also, you can't use this for buglist.cgi as-is, because LIMIT clauses have to be applied after the security checks. A subselect could work - I think we require new enough versions that that would be OK now)
(Assignee)

Comment 6

9 years ago
(In reply to comment #5)
> (From update of attachment 329150 [details] [diff] [review])
> Hmm. The distinct doesn't actually filter out duplicate rows, because a bug may
> be in multiple groups that the user isn't in

  No, it does filter them, because I selected *bug_id* for those groups. :-)

> Do you need to flush the cache for mod_perl users? (ie is Bugzilla->user cached
> across page loads?)

  Bugzilla->user is not cached across page loads.

> Do you know why 'creation_ts IS NOT NULL' is in there, BTW?

  Yes--when post_bug is creating a bug, it creates it with a NULL creation_ts, then adds it to a group, then makes the creation_ts NOW(). It does this because during replication lag, a security bug once showed up in the search list before it was added to groups. Now that we have transactions, it's probably unnecessary--we should test that, but not in this bug.

> (Also, you can't use this for buglist.cgi as-is, because LIMIT clauses have to
> be applied after the security checks. A subselect could work - I think we
> require new enough versions that that would be OK now)

  Yes, subselects work. Or we could do something else for the LIMIT, but yeah, if we want the LIMIT to work perfectly, it would be ideal to use this as some form of wrapper, you're right.
(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 329150 [details] [diff] [review] [details])
> > Hmm. The distinct doesn't actually filter out duplicate rows, because a bug may
> > be in multiple groups that the user isn't in
> 
>   No, it does filter them, because I selected *bug_id* for those groups. :-)

Oh, right. Thats evil :)

Anyway, it looks right, but I'll play around with it tomorrow.

Updated

9 years ago
Summary: Implement Bugzilla::Bug::visible_bugs and have can_see_bug use it → Implement Bugzilla::User::visible_bugs and have can_see_bug use it

Comment 8

9 years ago
Comment on attachment 329150 [details] [diff] [review]
v2

I did some testing with your patch, to compare what the perf is. It's pretty interesting:

1) Without your patch, I looped over 100 bugs and called can_see_bug() for each of them. I repeated the test 10 times to have better average values. I ran it in the command-line, to avoid interferences from CGI and TT:

  time elapsed: 0.369212865829468
  time elapsed: 0.234969139099121
  ...

The first time, it needs some extra time, probably to create $sth. On subsequent calls, the script is only slightly faster, probably because $sth is already set (I repeated the test 10 times within the script itself, so that Bugzilla->user is already cached on subsequent calls).

2) Now with your patch applied:

  time elapsed: 0.37495493888855
  time elapsed: 0.0031740665435791
  ...

Here, the first iteration is mostly the same as above (0.37) because it mostly does the same job, but subsequent iterations are *much* faster thanks to the cache which already knows if these bugs are visible or not (0.003 vs 0.23).

3) Now with your patch again, but I first call visible_bugs() will all bugs I'm going to check individually later:

  time elapsed: 0.132414102554321
  time elapsed: 0.00389599800109863

The initial call is much faster than before (0.13 vs 0.37) as you do a single SQL query for all bugs at once, and subsequent iterations remain the same as above as it does exactly the same job, i.e. looks at the cache (0.003).


We will have to look at all scripts to use visible_bugs() where appropriate. The benefit is real. Great patch .r=LpSolit
Attachment #329150 - Flags: review?(bbaetz)
Attachment #329150 - Flags: review?(LpSolit)
Attachment #329150 - Flags: review+

Comment 9

9 years ago
Max, could you file a bug to clean up existing code to use visible_bugs()?
Flags: approval+
(Assignee)

Updated

9 years ago
Blocks: 450546
(Assignee)

Comment 10

9 years ago
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.169; previous revision: 1.168
done
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: Bugzilla 4.0 → Bugzilla 3.4

Updated

9 years ago
Blocks: 486881

Updated

5 years ago
Blocks: 364160
You need to log in before you can comment on or make changes to this bug.