930410 - Switch TBPL from BzAPI to the new Bugzilla REST API

Status: RESOLVED FIXED

(Tree Management Graveyard :: TBPL, defect)

Description

[Ed Morley [:emorley]]

I'm not entirely clear whether the native bugzilla REST API is now in a state where we can use it, or if we need bug 880669 too.

Either way, this bug is to cover us switching TBPL to use it, rather than the not-officially-supported bzapi (eg to prevent things like bug 930383; though that shouldn't have affected the crons, so not sure what happened there).

The current uses are:
https://hg.mozilla.org/webtools/tbpl/file/16e6a0cc29c0/php/inc/AnnotatedSummaryGenerator.php#l195
https://hg.mozilla.org/webtools/tbpl/file/16e6a0cc29c0/php/submitBugzillaComment.php#l24
https://hg.mozilla.org/webtools/tbpl/file/16e6a0cc29c0/js/Data.js#l31

Bug 924405 covers mcMerge (and isn't as critical, since mcMerge downtime won't close the trees).

Comment 1

[:glob ✱]

(In reply to Ed Morley [:edmorley UTC+1] from comment #0)
> I'm not entirely clear whether the native bugzilla REST API is now in a
> state where we can use it, or if we need bug 880669 too.

it should be in a state where you can use it.

it's methods, parameters, and returned data are _not_ compatible with bzapi, so it'll be more than switching out the endpoints.  bug 880669 is about making a compatibility shim to avoid this.

Comment 2

[Ed Morley [:emorley]]

(In reply to Byron Jones ‹:glob› from comment #1)
> it's methods, parameters, and returned data are _not_ compatible with bzapi,
> so it'll be more than switching out the endpoints.  bug 880669 is about
> making a compatibility shim to avoid this.

Ah I follow now :-)

Comment 3

[Phil Ringnalda (:philor)]

The last time I switched us, in bug 652454, doing so was trivial except that BzAPI returns search results sorted.

Comment 4

[Ed Morley [:emorley]]

(Not working on this yet, so anyone else feel free to take, but don't want to forget about it and this way it'll appear in bugzilla todos)

Comment 5

[David Lawrence [:dkl]]

(In reply to Ed Morley [:edmorley UTC+0] from comment #0)
> I'm not entirely clear whether the native bugzilla REST API is now in a
> state where we can use it, or if we need bug 880669 too.
> 
> Either way, this bug is to cover us switching TBPL to use it, rather than
> the not-officially-supported bzapi (eg to prevent things like bug 930383;
> though that shouldn't have affected the crons, so not sure what happened
> there).
> 
> The current uses are:
> https://hg.mozilla.org/webtools/tbpl/file/16e6a0cc29c0/php/inc/
> AnnotatedSummaryGenerator.php#l195

Change:

195 $apiURL = "https://api-dev.bugzilla.mozilla.org/latest/bug" .
196           "?keywords=intermittent-failure" .
197           "&include_fields=id,summary,status,resolution" .
198           "&changed_after=-3m" .
199           "&summary=" . urlencode($searchTerm);

to:

195 $apiURL = "https://bugzilla.mozilla.org/rest/bug" .
196           "?keywords=intermittent-failure" . 
197           "&keywords_type=allwords" . 
198           "&include_fields=id,summary,status,resolution" . 
199           "&chfieldfrom=-3m&chfieldto=Now" . 
200           "&short_desc_type=allwordssubstr" . 
201           "&short_desc=" . urlencode($searchTerm);

> https://hg.mozilla.org/webtools/tbpl/file/16e6a0cc29c0/php/
> submitBugzillaComment.php#l24

Change:

24 $url = "https://api-dev.bugzilla.mozilla.org/latest/bug/$bugid/comment?username=" . 
       urlencode(TBPLBOT_EMAIL) . "&password=" . urlencode(TBPLBOT_PASSWORD);
25 $json = new Services_JSON();
26 $data = array(
27     "text" => sanitize($_POST["comment"])
28 );

to:

24 $url = "https://bugzilla.mozilla.org/rest/bug/$bugid/comment?login=" . 
          urlencode(TBPLBOT_EMAIL) . "&password=" . urlencode(TBPLBOT_PASSWORD);
25 $json = new Services_JSON();
26 $data = array(
27     "comment" => sanitize($_POST["comment"])
28 );

> https://hg.mozilla.org/webtools/tbpl/file/16e6a0cc29c0/js/Data.js#l31

Change:

31 var apiURL = "https://api-dev.bugzilla.mozilla.org/latest/bug?include_fields=status,summary&id=" + id;

to:

31 var apiURL = "https://bugzilla.mozilla.org/rest/bug?include_fields=status,summary&id=" + id;

> Bug 924405 covers mcMerge (and isn't as critical, since mcMerge downtime
> won't close the trees).

Comment 6

[Ed Morley [:emorley]]

The results returned are sorted differently, is this intentional?

eg:

Old: https://api-dev.bugzilla.mozilla.org/latest/bug?keywords=intermittent-failure&include_fields=id,summary,status,resolution&changed_after=-3m&summary=browser_profiler

New: https://bugzilla.mozilla.org/rest/bug?keywords=intermittent-failure&keywords_type=allwords&include_fields=id,summary,status,resolution&chfieldfrom=-3m&chfieldto=Now&short_desc_type=allwordssubstr&short_desc=browser_profiler

Comment 7

[Ed Morley [:emorley]]

(bzAPI sorts by status/resolution; the new REST interface by bug number)

Comment 8

[Ed Morley [:emorley]]

Attachment: Switch to the new bugzilla.mozilla.org REST API

WIP

Comment 9

[Ed Morley [:emorley]]

TBPL currently displays the bug suggestions in the sort order that they were returned in, we'll either need to get the REST API to return by resolution, or else sort in AnnotatedSummaryGenerator.php

Comment 10

[Ed Morley [:emorley]]

Appending "&order=status" (guessing based on the web interface params) doesn't work - is there a way to do this?

Comment 11

[David Lawrence [:dkl]]

(In reply to Ed Morley [:edmorley UTC+0] from comment #10)
> Appending "&order=status" (guessing based on the web interface params)
> doesn't work - is there a way to do this?

I just realized that this is true for the native search api in that order is ignored. I will file upstream bug for this.

dkl

Comment 12

[David Lawrence [:dkl]]

(In reply to David Lawrence [:dkl] from comment #11)
> (In reply to Ed Morley [:edmorley UTC+0] from comment #10)
> > Appending "&order=status" (guessing based on the web interface params)
> > doesn't work - is there a way to do this?
> 
> I just realized that this is true for the native search api in that order is
> ignored. I will file upstream bug for this.
> 
> dkl

Actually I was incorrect. It does still honor order= but uses the internal field names instead of the simpler API names. For example if you do &order=bug_status it will work as you expect. Another example would be &order=bug_id+DESC instead of order=id+DESC. I will file a different bug for this issue ;)

dkl

Comment 13

[Ed Morley [:emorley]]

(In reply to Ed Morley [:edmorley UTC+0] from comment #6)
> The results returned are sorted differently, is this intentional?
> 
> eg:
> 
> Old:
> https://api-dev.bugzilla.mozilla.org/latest/bug?keywords=intermittent-
> failure&include_fields=id,summary,status,resolution&changed_after=-
> 3m&summary=browser_profiler
> 
> New:

Using the internal field name makes the new example:
https://bugzilla.mozilla.org/rest/bug?keywords=intermittent-failure&keywords_type=allwords&include_fields=id,summary,status,resolution&chfieldfrom=-3m&chfieldto=Now&short_desc_type=allwordssubstr&short_desc=browser_profiler&order=bug_status

Weirdly, the the sort order inside each bug status (ie the order of all the "NEW"s) is non-deterministic - try refreshing that page a few times and comparing.

I tried using &order=bug_status+bug_id to fix that, but it sorts by bug ID first, not status, so that doesn't help.

Do you have any other ideas? (A non-deterministic sort order would really increase the time taken to star items which have multiple bug suggestions, since the sheriffs cannot rely on muscle-memory).

Comment 14

[David Lawrence [:dkl]]

(In reply to Ed Morley [:edmorley UTC+0] from comment #13)
> I tried using &order=bug_status+bug_id to fix that, but it sorts by bug ID
> first, not status, so that doesn't help.

Try: &order=bug_status,bug_id

dkl

Comment 15

[Ed Morley [:emorley]]

Ah looking good :-)
https://bugzilla.mozilla.org/rest/bug?keywords=intermittent-failure&keywords_type=allwords&include_fields=id,summary,status,resolution&chfieldfrom=-3m&chfieldto=Now&short_desc_type=allwordssubstr&short_desc=browser_profiler&order=bug_status,bug_id

I'd tried comma delimited too previously, but must have screwed it up.

Thank you :-)

Comment 16

[Ed Morley [:emorley]]

With bug 966277 worked around, testing locally seems to be working well. The sort order is still a little different, but it is at least something sensible (bug status, then bugzilla component [no idea why, since I didn't request that], then bug number) and more importantly consistent from one request to the next.

I'll see how bug 966277 pans out before uploading the workaround patch. I'll continue testing the annotated summary generator + bug commenting locally in the meantime.

Comment 17

[Ed Morley [:emorley]]

Attachment: Switch to the new bugzilla.mozilla.org REST API - v2

I've tested each of the three places where we use the API (tooltip generation on bug number rollover in the UI, bug suggestion generation on the server, and TBPLbot comment submission on the server) and everything seems fine now.

This patch either needs bug 966277 fixed (I've attached a patch there), or if that's WONTFIXed then the patch after this will work around the problem.

Comment 18

[Ed Morley [:emorley]]

Attachment: Set the accept header to workaround bug 966277

Won't be necessary with the patch in bug 966277.

Comment 19

[David Lawrence [:dkl]]

Comment on attachment 8369409 [details] [diff] [review]
Switch to the new bugzilla.mozilla.org REST API - v2

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

::: js/Data.js
@@ +27,5 @@
>    getBug: function Data_getBug(id, callback) {
>      if (id in this._bugcache)
>        return callback(this._bugcache[id]);
>      var self = this;
> +    var apiURL = "https://bugzilla.mozilla.org/rest/bug?include_fields=status,summary&id=" + id;

"https://bugzilla.mozilla.org/rest/bug/" + id + "?include_fields=status,summary"

Also it is a good idea to urlescape(id)

Otherwise, r=dkl

Comment 20

[David Lawrence [:dkl]]

(In reply to Ed Morley (Away most of 4th-17th Feb) [:edmorley UTC+0] from comment #18)
> Created attachment 8369410 [details] [diff] [review]
> Set the accept header to workaround bug 966277
> 
> Won't be necessary with the patch in bug 966277.

bug 966277 and the BMO backport have been committed.

dkl

Comment 21

[Ed Morley [:emorley]]

(In reply to David Lawrence [:dkl] from comment #19)
> > +    var apiURL = "https://bugzilla.mozilla.org/rest/bug?include_fields=status,summary&id=" + id;
> 
> "https://bugzilla.mozilla.org/rest/bug/" + id + "?include_fields=status,summary"

Done :-)

> Also it is a good idea to urlescape(id)

'id' comes from UserInterface.js::_linkBugs(), via the 2nd match of:
/(bug\s*|b=)([1-9][0-9]*)(?!\s*<\/a>)\b/ig
(https://hg.mozilla.org/webtools/tbpl/file/e34157f10528/js/UserInterface.js#l721)

...it will therefore always be a number, so I don't believe we need to urlescape it?

Comment 22

[Ed Morley [:emorley]]

Landed as:
remote:   https://hg.mozilla.org/webtools/tbpl/rev/a1099f542347

Will check tbpl-dev tomorrow to see if it's working as well as when testing in the VM :-)

Comment 23

[Ed Morley [:emorley]]

In production :-)

(Note bug 924405 is still needed for mcMerge, albeit that is blocked on bz.js being updated upstream)