Adapt Bug.search to use Search.pm

RESOLVED FIXED in Bugzilla 5.0

Status

()

Bugzilla
WebService
--
enhancement
RESOLVED FIXED
9 years ago
3 years ago

People

(Reporter: Rosie, Assigned: dkl)

Tracking

(Blocks: 1 bug)

unspecified
Bugzilla 5.0
Dependency tree / graph
Bug Flags:
approval +
testcase ?

Details

Attachments

(1 attachment, 9 obsolete attachments)

(Reporter)

Description

9 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.6) Gecko/2009020410 Fedora/3.0.6-1.fc9 Firefox/3.0.6
Build Identifier: 

Bug.search should be adapted to use Search.pm so that eventually it can become a complete search with the full functionality of query.cgi 

Reproducible: Always
(Reporter)

Comment 1

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

Patch reproduces functionality of Bug.search but uses Search.pm
Attachment #361268 - Flags: review?(mkanat)
(Reporter)

Updated

9 years ago
Blocks: 475754

Comment 2

8 years ago
Wow, I can't believe it took me so long to get to this. I hope that you can forgive me, Rosie!!!
Status: UNCONFIRMED → NEW
Ever confirmed: true
Target Milestone: --- → Bugzilla 3.8

Comment 3

8 years ago
Comment on attachment 361268 [details] [diff] [review]
v1

  Thank you so much for writing this patch--it's actually really nice, and it's one of the most important things we want in the WebServices.

>+    my $query = new Bugzilla::Search('fields' => [qw(bugs.bug_id)],

 In modern Bugzilla that would just be 'bug_id' now.

>+    my $dbh = Bugzilla->dbh;
>+    my $limit_str='';
>+    #eventually limit should be moved to Search.pm

  Wow. Yeah, it really should be moved to Search.pm. Would you possibly be interested in writing the patch to do that before we implement this? I'm concerned about the spread of this custom limit code all over Bugzilla, slowly, instead of just centralizing it now.

>+    my $results = $dbh->selectall_arrayref($query->getSQL().' 

  Just make that selectcol_arrayref, and then you'll just get back the bug_id.

>+    my $return = Bugzilla::Bug->new_from_list(\@list);
>+    return { bugs => $return };

  Oh, you can't just return the bug objects directly--look at what the current code does.

>+sub _search_to_cgi{
>+    #search should eventually use intelligent operarators for different values - here we do exact matches

  You'll want to put XXX in front of that comment. Also, usually we put a space between the # and the first word of the comment, and format them as standard sentences (so starting with a capital letter, ending with a period, etc.).

>+    my $chart_count=0;
>+    my %cgi_hash;

>+    while ( my ($key, $value) = each(%$params) ) {
>+        if ($key ne 'limit' and $key ne 'offset'){

  I think what would be better here is to do this as "eq" and then do "next" at the end of the block or something. That is, boolean conditions should generally be positive, not negative--it tends to make reading the code easier.

>+            my $key_str = FIELD_MAP->{$key} || $key;

  I think we have a function to do that now, in our current code.

>+            if(scalar grep($_ eq $key_str,('assigned_to', 'reporter', 'qa_contact', 'votes'))>0){

  You don't need the ">0" there. Also, you don't really need the "scalar". Usually we just put the grep in there directly. Also, a simpler formatting is probably:

  if (grep($_ eq $key_str, qw(assigned_to reporter qa_contact votes))) {

>+                $cgi_hash{"field$chart_count-0-0"}=$key_str;
>+                $cgi_hash{"type$chart_count-0-0"}='anyexact';
>+                $key_str="value$chart_count-0-0";

  Hmmm. Shouldn't everything actually be going into the same chart with an AND join? Or are there terms that require more than one row in the same chart?

>+            #summary, url, whiteboard, assigned_to, reporter and qa contact need a type set

  Actually, they should default to something sensible in Search.pm, I thought. Do they default to "equal" or something instead of "anyexact"?

>+            if(scalar grep($_ eq $key_str,('short_desc', 'bug_file_loc', 'status_whiteboard'))>0){

  Same note about the grep, here.

>+            #delta_ts needs to have both ch_fields set so set 'from' and change key to 'to'

  The current API documentation says "may not be an array".
Attachment #361268 - Flags: review?(mkanat) → review-
(Reporter)

Comment 4

8 years ago
Created attachment 418194 [details] [diff] [review]
v2

Thanks for the review Max!!

The attached patch uses the patch attached to bug 535571 but still works with the current Search.pm if you don't specify limit/offsets. 

I've addressed all your changes I think plus added some more conditions in the _search_to_cgi loop as not all searches are for exact matches any more
Attachment #361268 - Attachment is obsolete: true
Attachment #418194 - Flags: review?(mkanat)
(Reporter)

Updated

8 years ago
Depends on: 535571

Comment 5

8 years ago
Comment on attachment 418194 [details] [diff] [review]
v2

>+    if(exists $params->{limit}){

  Nit: Space after "if" and before "{" at the end.

>+        $query = new Bugzilla::Search('fields' => [qw(bug_id)],

  Nit: Since there's only one field, might as well do 'bug_id'.

>+                                      'params' => $cgi_hash,
>+                                      'limit' => $params->{limit},
>+                                      'offset' => $offset

  I don't think you defined $offset.

>+    }
>+    else{
>+        $query = new Bugzilla::Search('fields' => [qw(bug_id)],
>+                                      'params' => $cgi_hash
>+                                     );

  Another option, instead of re-writing the parameters for Bugzilla::Search in two blocks, is to create an %options hash and just add limit and offset as necessary.

>+    # Search.pm won't return bugs that the user shouldn't see so no filtering is needed.
>+    my @bugs;
>+    foreach my $bug_id (@$results) {
>+        my $bug = Bugzilla::Bug->check($bug_id);

  You don't need to do check() (because of your comment above, there). Just use Bugzilla::Bug->new_from_list on the whole list.

>+sub _search_to_cgi {
>+    #search should eventually use intelligent operarators for different values -
>+    # here we mostly do exact matches.

  I think that's actually not true anymore--I think we do use intelligent operators for each value that can use them.

>+    while ( my ($key, $value) = each(%$params) ) {
>+        if ($key eq 'limit' or $key eq 'offset'){
>+            next;
>+        }

  Cool. That could be even shorter as a "next if".

>+        # Make sure Summary and Status Whiteboard default to 'contains any of the strings'
>+        if (grep($_ eq $key, qw(short_desc status_whiteboard))) {
>+            $cgi_hash{"field0-$and-0"}=$key;
>+            $cgi_hash{"type0-$and-0"}='anywordsubstr';
>+            $key="value0-$and-0";
>+            $and++;
>+        }

  You don't need to do those as boolean charts, you can just do "${field}_type".

>+        }
>+        if($key eq 'creation_ts'){

  Same for this. Also, this needs to be "greater than or equal to", to maintain API compatibility.

>+            $key="value0-$and-0";

  Nit: Spaces around "=". (This should be changed in a lot of places.)

>+        if(ref($value) eq 'ARRAY'){
>+                $cgi_hash{$key}=join(',',@$value);

  Ohhh, right, I forgot about that--the whole "you can't search for commas" problem. I don't know if that's an OK API change....
Attachment #418194 - Flags: review?(mkanat) → review-
(Reporter)

Comment 6

8 years ago
Created attachment 418369 [details] [diff] [review]
v3

(In reply to comment #5)
Cheers Max

I've hopefully formatted all my code properly now, I've also put the parameters to Search in a hash and used Bug->new_from_list on my list of bug ids. Thanks for the 'next if' pointer too.

>  Same for this. Also, this needs to be "greater than or equal to", to maintain
API compatibility.

I think I still need to use a boolean chart for creation_date don't I? If you use the chfield and chfrom values you can't search for last_changed_date and creation_date together. There doesn't seem to be a greater than or equal operator in the boolean charts so I've had to search for greaterthan and create an or equal in the same chart (if that makes sense :P) 

>   Ohhh, right, I forgot about that--the whole "you can't search for commas"
> problem. I don't know if that's an OK API change....

... sorry I don't understand - it's a while since I wrote this but I thought that was how arrays should be passed to Search.pm. Should I be passing it an array ref instead?
Attachment #418194 - Attachment is obsolete: true
Attachment #418369 - Flags: review?(mkanat)

Comment 7

8 years ago
(In reply to comment #6)
> I think I still need to use a boolean chart for creation_date don't I?

  I'm not entirely certain that you do, if there's a field in fielddefs that represents creation_ts. I recently modified Search.pm to take anything in fielddefs without a period as a valid field, even if that field isn't used by query.cgi. 

> >   Ohhh, right, I forgot about that--the whole "you can't search for commas"
> > problem. I don't know if that's an OK API change....
> 
> ... sorry I don't understand - it's a while since I wrote this but I thought
> that was how arrays should be passed to Search.pm. Should I be passing it an
> array ref instead?

  Well, it's actually an old bug in Search.pm that I'm talking about. (bug 67036). The problem is that with the current Search.pm implementation, users can indeed search for things that contain commas. With your new implementation, they can't.
(Reporter)

Comment 8

8 years ago
Created attachment 420071 [details] [diff] [review]
v4

(In reply to comment #7)
>   I'm not entirely certain that you do, if there's a field in fielddefs that
> represents creation_ts. I recently modified Search.pm to take anything in
> fielddefs without a period as a valid field, even if that field isn't used by
> query.cgi. 

Well you can search for creation_ts > a value but there's no >= operator in Search.pm so to get the same behaviour as the old function you need to construct a boolean search.

>   Well, it's actually an old bug in Search.pm that I'm talking about. (bug
> 67036). The problem is that with the current Search.pm implementation, users
> can indeed search for things that contain commas. With your new implementation,
> they can't.

That should be fixed with this new patch which just passes arrays of values rather than joining them with commas
Attachment #418369 - Attachment is obsolete: true
Attachment #420071 - Flags: review?(mkanat)
Attachment #418369 - Flags: review?(mkanat)

Comment 9

8 years ago
(In reply to comment #8)
> Well you can search for creation_ts > a value but there's no >= operator in
> Search.pm so to get the same behaviour as the old function you need to
> construct a boolean search.

  Hmm. Do you think you could add a >= and <= operator, in another bug, to handle that? I think that would simplify things for us here.

> That should be fixed with this new patch which just passes arrays of values
> rather than joining them with commas

  Oh, if that works, then that's OK. :-)
(Reporter)

Updated

8 years ago
Depends on: 538348
(Reporter)

Comment 10

8 years ago
Created attachment 420536 [details] [diff] [review]
v5

This is the new patch using the offset and limit params and greaterthaneq operator in Search.pm
Attachment #420071 - Attachment is obsolete: true
Attachment #420536 - Flags: review?(mkanat)
Attachment #420071 - Flags: review?(mkanat)

Updated

8 years ago
Attachment #420536 - Flags: review?(mkanat) → review+

Comment 11

8 years ago
Comment on attachment 420536 [details] [diff] [review]
v5

This looks great. Thank you so much, Rosie. :-)

Updated

8 years ago
Flags: approval?

Comment 12

8 years ago
Comment on attachment 420536 [details] [diff] [review]
v5

>+    if (exists $params->{limit}) {
>+        $options{limit} = $params->{limit};
>+        $options{offset} = $params->{offset} || 0;
>+     }

  Though, that shouldn't be necessary anymore, right?
(Reporter)

Comment 13

8 years ago
oops yes thought I'd taken that all out - obviously not!

Updated

8 years ago
Assignee: webservice → rosie.clarkson
Status: NEW → ASSIGNED

Updated

8 years ago
Whiteboard: [licensing issue, don't check in until resolved]

Updated

8 years ago
Target Milestone: Bugzilla 4.0 → Bugzilla 5.0

Comment 14

8 years ago
Created attachment 464088 [details] [diff] [review]
patch to enable use of Search.pm

this should obsolete the current attachment but it's not letting me do that.

apologies if this doesn't apply ... or if it doesn't work anymore.  i've had to edit the file by hand to change the license stuff.  and i've not been able to apply the patch to trunk or to get trunk running in order to test it.

sorry i won't have any time to make changes or test anything over the coming weeks.  hopefully just sorting out the license is enough to enable others to complete the work.  if not i'll see what i can do.

Comment 15

8 years ago
P.S. when i say i couldn't "get trunk running" i don't mean it's broken - i just didn't have the right version of perl (well, CGI.pm) available on fedora 13 and don't have time to figure out what to do about that.

Updated

8 years ago
Attachment #420536 - Attachment is obsolete: true

Comment 16

8 years ago
Fixing the license is absolutely enough. Thank you so much, Chris! :-)

Comment 17

7 years ago
Why doesn't this patch have a review request now that the licensing issue is resolved? Or if it's a trivial fix, why isn't the bug approved yet?

Updated

7 years ago
Attachment #464088 - Flags: review?(mkanat)

Comment 18

7 years ago
thanks for reminder - done now, also for bug 535571.  can someone clean the whiteboard on these two bugs?

Updated

7 years ago
Whiteboard: [licensing issue, don't check in until resolved]

Updated

7 years ago
Flags: approval?

Comment 19

7 years ago
Comment on attachment 464088 [details] [diff] [review]
patch to enable use of Search.pm

>Index: Bugzilla/WebService/Bug.pm

>+use Bugzilla::CGI;

Unless I miss something, Search.pm no longer depends on CGI, see bug 398308 (checked in a year ago).
Attachment #464088 - Flags: review?(mkanat) → review-

Comment 20

6 years ago
Created attachment 586441 [details] [diff] [review]
patch against trunk

here's a new initial patch.  i've made a couple of HUGE assumptions:
1) our comment "The following helper function can be removed once bug 398308 is completed" can be taken literally.
2) the 'anywordsubstr' and 'greaterthaneq' parts of the helper function don't need to be moved into the main method.

so i've passed the received 'params' directly to the Search.pm function.

this patch passes the runtest.pl script but i've so far been unable to test the actual search functionality as i can't get the python bugzilla library to work against trunk - it gets an XML error parsing the first response.

i read somewhere that the runtest.pl script tests ALL of the web service API - do you know if that means it has done a sample search?  presumably it can't check the results as there are no bugs in my new instance.

so, if i can get the python bugzilla working then i'll try some searches but if someone else can beat me to it (or see problems without the need to try it out) please go for it.

hope this is helpful.
Attachment #464088 - Attachment is obsolete: true
Attachment #586441 - Flags: review?(mkanat)

Comment 21

6 years ago
meh - next time i'll install the relevant perl packages before claiming the web service isn't working ;-)
i've told the python bugzilla library to treat v4.3 as v3.6 for now.  search crashes.  :-(

Comment 22

6 years ago
Comment on attachment 586441 [details] [diff] [review]
patch against trunk

Review of attachment 586441 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome, thank you!! :-D Some comments:

::: Bugzilla/WebService/Bug.pm
@@ +401,4 @@
>                         { param => 'limit', function => 'Bug.search()' });
>      }
>      
> +    #$params = Bugzilla::Bug::map_fields($params);

Removing map_fields is likely to break stuff, since that's what makes the API work. In fact, we're likely to need a more-complex version of map_fields for this. Or perhaps instead, we can implement support for the API names of fields inside of Bugzilla::Search, which seems like a better solution (since it has a centralized place that it can rename fields in).

@@ +401,5 @@
>                         { param => 'limit', function => 'Bug.search()' });
>      }
>      
> +    #$params = Bugzilla::Bug::map_fields($params);
> +    #delete $params->{WHERE};

This line can just be deleted. However, there are some other parameters that you have to delete. At the very least, include_fields and exclude_fields should be moved out, since they're not search criteria.

@@ +411,5 @@
> +    my %options = ('fields' => ['bug_id'], 'params' => $params);
> +    if (exists $params->{limit}) {
> +        $options{limit} = $params->{limit};
> +        $options{offset} = $params->{offset} || 0;
> +     }

You should only send {offset} if {offset} was sent by the client. Also, you should delete these from params as you set them, like:

$options{limit} = delete $params->{limit}

@@ +416,5 @@
> +
> +    my $query = new Bugzilla::Search(%options);
> +    my $dbh = Bugzilla->dbh;
> +
> +    my $results = $dbh->selectcol_arrayref($query->getSQL());

I'm pretty sure there's no getSQL function anymore. You probably just want ->sql.

@@ +423,5 @@
> +
> +    my @bugs;
> +    foreach my $bug (@$bug_objects) {
> +        push(@bugs, $self->_bug_to_hash($bug));
> +    }

This can be replaced with:

my @bugs = map { $self->_bug_to_hash($_, $params) } @bugs;
Attachment #586441 - Flags: review?(mkanat) → review-

Comment 23

6 years ago
hi Max, thanks for the comments.  sorry for the delays - bit busy at the moment.
the getSQL was causing the crash.  changing it to ->sql fixed the crash but at the moment i'm not getting any search results.  will keep working on it but happy for someone else to pick this up.
(Assignee)

Comment 24

6 years ago
I can take this over as I need this to finish up the BMO REST extension I am working on.

One thing I would like to note, there may be quite a bit of current clients our there who are still relying on the old Bug.search behavior and this will break those clients. Should we implement this as a new method named Bug.full_search or are we ok in making people fix their clients for 4.4 to work with the new Bug.search?

dkl
Assignee: rosie.clarkson → dkl
(Assignee)

Comment 25

6 years ago
Created attachment 603161 [details] [diff] [review]
Patch to make Bug.search use Search.pm (v1)

Patch that changes Bug.search to use Search.pm. Trying to achieve backwards compatibility with the old Bug.search as well. Couple additional features is the ability to pass in 'quicksearch' as well as use any saved searches of the user or another user who is sharing the search.

dkl
Attachment #603161 - Flags: review?(LpSolit)

Comment 26

6 years ago
(In reply to David Lawrence [:dkl] from comment #24)
> are we ok in making people fix their clients for 4.4 to
> work with the new Bug.search?

We are. This method is marked unstable, and the searching system is already complex enough to not support the old version too. About your patch, does it make sense to support regetlastlist? I totally understand the support for quicksearch and saved searches, but I have some problems understanding if regetlastlist makes sense. And is the BUGLIST cookie really passed to the method anyway?

Comment 27

6 years ago
Comment on attachment 603161 [details] [diff] [review]
Patch to make Bug.search use Search.pm (v1)

Review of attachment 603161 [details] [diff] [review]:
-----------------------------------------------------------------

In general, it's a bad idea to make the internal API of Search.pm the external API of Bug.search. Instead of passing in things that look just like the CGI parameters, we want to pass in data structures. For an idea of what these might look like, See Bugzilla::Test::Search::Constants::CUSTOM_SEARCH_TESTS. One of the motivations behind my redesign of Search.pm was enabling the ability to pass in data structures much like that.

I also have a few more specific comments:

::: Bugzilla/WebService/Bug.pm
@@ +401,5 @@
> +    delete $params->{'exclude_fields'};
> +
> +    if ($params->{quicksearch} || $params->{savedsearch}) {
> +        # Force CGI.pm to return as query string and not POSTDATA
> +        $ENV{REQUEST_METHOD} = 'GET';

This seems dangerous, because you're in the WebService, which makes logical decisions based on this. You could convert this into a "local" with a trailing "if" instead.

@@ +408,5 @@
> +    # Determine whether this is a quicksearch query
> +    if ($params->{quicksearch}) {
> +        my $quicksearch = quicksearch(delete $params->{'quicksearch'});
> +        my $cgi = Bugzilla::CGI->new($quicksearch);
> +        $params = $cgi->Vars;

Maybe you should have a method for quicksearch to return a hash or a series of Clause objects or something. At least, seems like a worthwhile XXX comment.

@@ +412,5 @@
> +        $params = $cgi->Vars;
> +    }
> +
> +    # Load previously saved query and use it if specified
> +    if ($params->{savedsearch}) {

This is normally the sort of thing I would recommend goes into a separate patch.

@@ +438,5 @@
>      
> +        my $url = $saved_search_obj->url;
> +        my $cgi = Bugzilla::CGI->new($url);
> +        $params = $cgi->Vars;
> +        $order = $params->{'order'};

This is definitely a case where Bugzilla::Search::Saved should be able to return the object you need, instead, I would have to think. Or perhaps even return a Bugzilla::Search object.
Attachment #603161 - Flags: review-

Comment 28

6 years ago
Comment on attachment 603161 [details] [diff] [review]
Patch to make Bug.search use Search.pm (v1)

Review of attachment 603161 [details] [diff] [review]:
-----------------------------------------------------------------

In general, it's a bad idea to make the internal API of Search.pm the external API of Bug.search. Instead of passing in things that look just like the CGI parameters, we want to pass in data structures. For an idea of what these might look like, See Bugzilla::Test::Search::Constants::CUSTOM_SEARCH_TESTS. One of the motivations behind my redesign of Search.pm was enabling the ability to pass in data structures much like that.

I also have a few more specific comments:

::: Bugzilla/WebService/Bug.pm
@@ +401,5 @@
> +    delete $params->{'exclude_fields'};
> +
> +    if ($params->{quicksearch} || $params->{savedsearch}) {
> +        # Force CGI.pm to return as query string and not POSTDATA
> +        $ENV{REQUEST_METHOD} = 'GET';

This seems dangerous, because you're in the WebService, which makes logical decisions based on this. You could convert this into a "local" with a trailing "if" instead.

@@ +408,5 @@
> +    # Determine whether this is a quicksearch query
> +    if ($params->{quicksearch}) {
> +        my $quicksearch = quicksearch(delete $params->{'quicksearch'});
> +        my $cgi = Bugzilla::CGI->new($quicksearch);
> +        $params = $cgi->Vars;

Maybe you should have a method for quicksearch to return a hash or a series of Clause objects or something. At least, seems like a worthwhile XXX comment.

@@ +412,5 @@
> +        $params = $cgi->Vars;
> +    }
> +
> +    # Load previously saved query and use it if specified
> +    if ($params->{savedsearch}) {

This is normally the sort of thing I would recommend goes into a separate patch.

@@ +438,5 @@
>      
> +        my $url = $saved_search_obj->url;
> +        my $cgi = Bugzilla::CGI->new($url);
> +        $params = $cgi->Vars;
> +        $order = $params->{'order'};

This is definitely a case where Bugzilla::Search::Saved should be able to return the object you need, instead, I would have to think. Or perhaps even return a Bugzilla::Search object.

@@ +463,5 @@
> +        }
> +        # set up the params for this new query
> +        my $cgi = $params = Bugzilla::CGI->new({ bug_id => $bug_ids, order => $order });
> +        $params = $cgi->Vars;
> +    }

This looks like there's some cut-and-paste from buglist.cgi; perhaps some of this should be abstracted out. (And also this might be a good thing to put into a separate patch, in general.)

@@ +484,5 @@
>          delete $params->{$_} foreach qw(estimated_time remaining_time deadline);
>      }
>  
> +    # Some fields require a search type such as short desc, etc.
> +    foreach my $param (qw(short_desc longdesc status_whiteboard bug_file_loc)) {

The custom fields that work similarly would need similar default types as well, no?

@@ +508,5 @@
> +        $params->{"field${last_chart_id}-0-0"} = $email;
> +        $params->{"type${last_chart_id}-0-0"}  = "anywordssubstr";
> +        $params->{"value${last_chart_id}-0-0"} = ref $value ? join(" ", @{$value}) : $value;
> +        $last_chart_id++;
> +    }

Also, just as a note, no new code should be relying on boolean chart syntax. It would be better to use the new Custom Search syntax.

Comment 29

6 years ago
Oh, and note Splinter bug: when I made new comments, it published my entire review again. Sorry about that.

Updated

6 years ago
Attachment #603161 - Flags: review?(LpSolit)

Updated

6 years ago
Blocks: 747823

Comment 30

6 years ago
We are going to branch for Bugzilla 4.4 next week and this bug is too invasive or too risky to be accepted for 4.4 at this point. The target milestone is set to 5.0 which is our next major release.

I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else on time for 5.0.
Target Milestone: Bugzilla 4.4 → Bugzilla 5.0

Comment 31

5 years ago
What's the status of this bug? brc want this implemented yesterday :)

I'm happy to pick it up if David (the current assignee) is not going to work on it.
Flags: needinfo?(rosie.clarkson)

Updated

5 years ago
Flags: needinfo?(rosie.clarkson) → needinfo?(dkl)
(Assignee)

Comment 32

5 years ago
(In reply to Simon Green from comment #31)
> What's the status of this bug? brc want this implemented yesterday :)
> 
> I'm happy to pick it up if David (the current assignee) is not going to work
> on it.

We need it as well due to trying to move the BzAPI REST proxy over to BMO natively and we need to preserve the full search ability that BzAPI provides currently. And since Bug.search is still classified as UNSTABLE, now is the best time to switch over to a fully functional search method. 

How bout I revise the patch and you review it?

dkl
Flags: needinfo?(dkl) → needinfo?(sgreen)

Comment 33

5 years ago
(In reply to David Lawrence [:dkl] from comment #32)
> How bout I revise the patch

Sounds great.

> and you review it?

I don't have review privileges, so it will need an additional review from an approved person.
Flags: needinfo?(sgreen)
(Assignee)

Comment 34

5 years ago
Created attachment 765768 [details] [diff] [review]
Patch to make Bug.search use Search.pm (v3)

Updated patch. Passes webservice_bug_search.t as well.

dkl
Attachment #603161 - Attachment is obsolete: true
Attachment #765768 - Flags: review?(sgreen)
(Assignee)

Updated

5 years ago
Blocks: 880669
(Assignee)

Comment 35

5 years ago
Hi Simon. Do you think you will be able to look at this soon? We need it for some work being done internally so if you don't have the time, I can see if glob will take it. Let me know.

Thanks
dkl
Flags: needinfo?(sgreen)

Updated

5 years ago
Attachment #586441 - Attachment is obsolete: true

Comment 36

5 years ago
Comment on attachment 765768 [details] [diff] [review]
Patch to make Bug.search use Search.pm (v3)

Review of attachment 765768 [details] [diff] [review]:
-----------------------------------------------------------------

I'd also like to see the POD in B/W/Bug.pm update to mention you can include custom search values.

::: Bugzilla/WebService/Bug.pm
@@ +426,5 @@
>          }
>      }
>      else {
> +        delete $params->{'limit'};
> +        delete $params->{'offset'};

Nit: Bugzilla doesn't have a specified preferences for quoted vs. non quoted strings in a hash. Don't change them for the sake of changing them.

@@ +438,5 @@
> +    my @field_ids = grep(/^f(\d+)$/, keys %$params);
> +    my $last_field_id = 1;
> +    if (@field_ids) {
> +        @field_ids = sort { $a <=> $b } uniq @field_ids;
> +        $last_field_id = (@field_ids)[-1];

Use List::Util's max function instead.

@@ +443,4 @@
>      }
>  
>      # Do special search types for certain fields.
> +    if (my $change_when = delete $params->{delta_ts}) {

http://www.bugzilla.org/docs/4.4/en/html/api/Bugzilla/WebService/Bug.html#search says the field is last_change_time, not delta_ts

@@ +448,5 @@
> +        $params->{"o${last_field_id}"} = 'greaterthaneq';
> +        $params->{"v${last_field_id}"} = $change_when;
> +        $last_field_id++;
> +    }
> +    if (my $creation_when = delete $params->{creation_ts}) {

http://www.bugzilla.org/docs/4.4/en/html/api/Bugzilla/WebService/Bug.html#search says the field is creation_time, not creation_ts

@@ +473,5 @@
> +        @email_ids = sort { $a <=> $b } uniq @email_ids;
> +        $last_email_id = (@email_ids)[-1];
> +    }
> +
> +    foreach my $role (qw(assigned_to reporter qa_contact longdesc cc)) {

http://www.bugzilla.org/docs/4.4/en/html/api/Bugzilla/WebService/Bug.html#search says the fields are creator, not reporter (but reporter is also accepted for backwards compatibility

@@ +474,5 @@
> +        $last_email_id = (@email_ids)[-1];
> +    }
> +
> +    foreach my $role (qw(assigned_to reporter qa_contact longdesc cc)) {
> +        next if !exists $params->{$role};

Wouldn't it be better to just use the custom search for this? Means would wouldn't need to use the @email_ids in the section above, and can use last_field_id instead.

@@ +477,5 @@
> +    foreach my $role (qw(assigned_to reporter qa_contact longdesc cc)) {
> +        next if !exists $params->{$role};
> +        my $value = delete $params->{$role};
> +        $params->{"email$role$last_email_id"} = 1;
> +        $params->{"emailtype$last_email_id"}  = "anywordssubstr";

Shouldn't this be 'equals' instead of 'anywordssubstr'
Attachment #765768 - Flags: review?(sgreen) → review-

Comment 37

5 years ago
(In reply to David Lawrence [:dkl] from comment #35)
> Do you think you will be able to look at this soon?

Sorry about the delay. Finally got my VPS set up with some decent data (a sanitised BMO database) so am able do reviews now.
Flags: needinfo?(sgreen)
(Assignee)

Comment 38

5 years ago
Created attachment 787632 [details] [diff] [review]
477601_4.patch

(In reply to Simon Green from comment #36)
> @@ +443,4 @@
> > +    if (my $change_when = delete $params->{delta_ts}) {
> 
> http://www.bugzilla.org/docs/4.4/en/html/api/Bugzilla/WebService/Bug.
> html#search says the field is last_change_time, not delta_ts

last_change_time is mapped to creation_ts earlier in the map_fields() call.

> @@ +448,5 @@
> > +    if (my $creation_when = delete $params->{creation_ts}) {
> 
> http://www.bugzilla.org/docs/4.4/en/html/api/Bugzilla/WebService/Bug.
> html#search says the field is creation_time, not creation_ts

same as above.

> @@ +477,5 @@
> > +        $params->{"emailtype$last_email_id"}  = "anywordssubstr";
> 
> Shouldn't this be 'equals' instead of 'anywordssubstr'

The old Search method allowed for certain values such as assignee, qa_contact, etc. to be a list of values so it was safer to use anywordssubstr just in case we are looking for multiple values.
Attachment #765768 - Attachment is obsolete: true
Attachment #787632 - Flags: review?(sgreen)

Comment 39

5 years ago
Comment on attachment 787632 [details] [diff] [review]
477601_4.patch

+    if (!grep(!/(limit|offset)/i, keys %$params)
should be

+    if (!grep(!/^(limit|offset)$/i, keys %$params)

in case we ever have a field with limit or offset (for example a custom field). Other than that, I'm happy with the review, and the above can be fixed on checkin.
Attachment #787632 - Flags: review?(sgreen) → review+

Updated

5 years ago
Flags: approval?
Flags: approval? → approval+
(Assignee)

Updated

5 years ago
Keywords: relnote, testcase
(Assignee)

Comment 40

5 years ago
\o/

Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bugzilla/trunk
modified Bugzilla/WebService/Bug.pm
Committed revision 8713.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Blocks: 909430
(Assignee)

Updated

5 years ago
Flags: testcase?
Keywords: testcase
\o/

Gerv

Updated

4 years ago
Blocks: 918362

Updated

4 years ago
Blocks: 918443

Updated

4 years ago
Duplicate of this bug: 960993

Updated

4 years ago
Blocks: 637642
(Assignee)

Updated

3 years ago
No longer blocks: 475754
Duplicate of this bug: 475754

Comment 44

3 years ago
Added to relnotes for 5.0rc1.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.