Improvement in retrieving a classification name from bug

RESOLVED FIXED in Bugzilla 5.0

Status

()

Bugzilla
Bugzilla-General
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Simon Green, Assigned: Simon Green)

Tracking

Bugzilla 5.0
Bug Flags:
approval +

Details

Attachments

(1 attachment, 2 obsolete attachments)

1.01 KB, patch
glob
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Created attachment 757189 [details] [diff] [review]
v1 patch

This is one of the trivial things that will save about 1ms per call, but when you make the call a few thousand times (as is the case for the Bug.search RPC call), that means seconds shaved off.

Calling Bugzilla::Bug->classification means that two calls are made to database, once to get the classification_id from the product, the second to get the classification name from it's id.

This changes that so only one call is made.
Attachment #757189 - Flags: review?(glob)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
Comment on attachment 757189 [details] [diff] [review]
v1 patch

while this works, it would be better to return $self->product_obj->classification->name (with a similar change to the classification_id sub).

the object cache will ensure we don't hit the database multiple times when there is more than one bug in the same product.
Attachment #757189 - Flags: review?(glob) → review-
(Assignee)

Comment 2

5 years ago
(In reply to Byron Jones ‹:glob› from comment #1)
> Comment on attachment 757189 [details] [diff] [review]
> v1 patch
> 
> while this works, it would be better to return
> $self->product_obj->classification->name (with a similar change to the
> classification_id sub).
> 
> the object cache will ensure we don't hit the database multiple times when
> there is more than one bug in the same product.

Unless I am missing something, there is no object caching of products in Bugzilla. $bug->product_obj(<id>) will do one select call in Bugzilla::Object::_init for each bug. I can't see anywhere in the code where this is cached.
(Assignee)

Comment 3

5 years ago
Created attachment 757750 [details] [diff] [review]
v2 patch
Attachment #757750 - Flags: review?(glob)
(In reply to Simon Green from comment #2)
> Unless I am missing something, there is no object caching of products in
> Bugzilla. $bug->product_obj(<id>)

> $self->{product_obj} ||=
>     new Bugzilla::Product({ id => $self->{product_id}, cache => 1 });

note "cache => 1".  the object cache was added via bug 811280.

>-    return $self->{classification} if exists $self->{classification};
>-    return '' if $self->{error};
>-    ($self->{classification}) = Bugzilla->dbh->selectrow_array(
>-        'SELECT name FROM classifications WHERE id = ?',
>-        undef, $self->classification_id);
>+    $self->{classification} //= $self->product_obj->classification->name;

are you sure it's ok to no longer return an empty string if {error} is set?

Comment 5

5 years ago
Comment on attachment 757750 [details] [diff] [review]
v2 patch

>-    return 0 if $self->{error};

>-    return '' if $self->{error};

If the bug is not accessible, then these values must be returned. So these security checks must stay.
Attachment #757750 - Flags: review?(glob) → review-
(Assignee)

Comment 6

5 years ago
Created attachment 758279 [details] [diff] [review]
v3 patch
Attachment #757189 - Attachment is obsolete: true
Attachment #757750 - Attachment is obsolete: true
Attachment #758279 - Flags: review?(glob)
(Assignee)

Comment 7

5 years ago
(In reply to Frédéric Buclin from comment #5)
> If the bug is not accessible, then these values must be returned. So these
> security checks must stay.

Totally agree. I was a bit over-eager in my cutting of the old code, and didn't notice it when looking at the diff. New patch submitted.
Comment on attachment 758279 [details] [diff] [review]
v3 patch

r=glob
Attachment #758279 - Flags: review?(glob) → review+
(Assignee)

Updated

5 years ago
Flags: approval?

Updated

5 years ago
Flags: approval? → approval+
Target Milestone: --- → Bugzilla 5.0

Comment 9

5 years ago
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Bug.pm
Committed revision 8632.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.