1266588 - [MERGE] REST urls for resources provided by extensions no longer work

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: API, defect)

Description

[David Lawrence [:dkl]]

- REST urls for resources provided by extensions no longer work
    - eg. https://bugzilla-merge.allizom.org/rest/review/suggestions/112233
    - A REST API resource was not found for 'GET /review/suggestions/112233'
    - we cannot break existing urls

Comment 1

[David Lawrence [:dkl]]

(In reply to David Lawrence [:dkl] from comment #0)
> - REST urls for resources provided by extensions no longer work
>     - eg. https://bugzilla-merge.allizom.org/rest/review/suggestions/112233
>     - A REST API resource was not found for 'GET /review/suggestions/112233'
>     - we cannot break existing urls

I have found the issue blocking extension REST API calls from working and is a bug that must also be fixed upstream.

But this will not fix the 'we cannot break existing urls' without some refactoring unfortunately. Upstream, the design was that extension API methods would have to follow the new /rest/namespace/version/method... style. The only methods that would not need a version number (fallback to 1.0) would be core methods. 

Since the new API uses the /rest/ prefix and therefore only looks for stuff in Bugzilla/API/* and extensions/NAME/API/* we would have to either:

1) Create some sort of interim mapping table of all API paths to their equivalent new style extension API paths.
Example: /rest/review/suggestions/(\d+) -> /rest/review/1.0/suggestions/$1
Caveat: Would need to be maintained and we would need to announce a cutoff date.

2) Make the new versioned api use a new path and make the old /rest/ prefix work with the old style of doing REST wrapper around XMLRPC/JSONRPC.
So the new path would be /api/review/1.0/suggestions/112233 and would return the same data as /rest/review/suggestions/112233
but use different code paths on the backend.
We could push people to use the new path as much as possible similar to how we did with BzAPI -> Native REST.
We would also need to come up with a cutoff date as we do not want to keep maintaining the old XMLRPC/JSONRPC code forever.

Do we know if a lot of people are using API methods from the extensions? Most of them were designed for use by the UI elements but people were free to use them if they like. We never guaranteed those to never break from what I recall. Or are they mostly core API methods which should continue to work as before even with the new code?

Thoughts?
dkl
dkl

Comment 2

[David Lawrence [:dkl]]

Attachment: 1266588_1.patch

I will need to push the Bugzilla/API/* related changes upstream as well once this is reviewed as it is affected by this.

dkl

Comment 3

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

In terms of setting a cut off date and communicating this, I'd recommend getting on the schedule for the London Work Week to announce the changes to a broad audience and setting a deadline for the cut over.

Comment 4

[Mark Côté [:mcote]]

Hm how would we do this announcement?  In the past there aren't very many opportunities to actually address a lot of people, aside from the keynotes, where this would be out of place.  I fully agree that we need a cut-off date if we go with option 1, which sounds (I think?) like the least amount of effort.  I think posts to a few newsgroups would be fine.

Comment 5

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

Diane Tate is taking requests for "electives" at the work week, so you could ask for a session on API changes, or I can add the announcement to the triage training I'm planning to run.

Comment 6

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #1)
> Do we know if a lot of people are using API methods from the extensions?

you should be able to tell that from the logs.

> Most of them were designed for use by the UI elements but people were free
> to use them if they like. We never guaranteed those to never break from what
> I recall.

it's true the docs for these say EXPERIMENTAL, however the entire REST api is still tagged as experimental yet that isn't something we should break.  i know i've pointed people at the docs for the reviewer suggestions before, but i don't know if there's any uptake.


i think the way forward here is to look at the logs to determine the usage of endpoints provided by extensions; if there's little to no hits then this becomes a non-issue.

Comment 7

[Dylan Hardison [:dylan] (he/him)]

we could just map the existing routes using .htacces url rewriting, btw.

Comment 8

[:glob ✱]

(In reply to Dylan William Hardison [:dylan] from comment #7)
> we could just map the existing routes using .htacces url rewriting, btw.

+1

Comment 9

[David Lawrence [:dkl]]

Comment on attachment 8745655 [details] [diff] [review]
1266588_1.patch

New patch coming shortly.

Comment 10

[David Lawrence [:dkl]]

Attachment: 1266588_2.patch

Please test your mozreview changes as well. I am sure you have some sort of scripts that will make that easier than I doing it manually.

Thanks
dkl

Comment 11

[Dylan Hardison [:dylan] (he/him)]

This doesn't have REST endpoints for any of the extensions. Was that intentional?

Comment 12

[David Lawrence [:dkl]]

Attachment: 1266588_3.patch

Missing .htaccess changes

Comment 13

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8752047 [details] [diff] [review]
1266588_3.patch

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

r=dylan

This does what it says, but I note there's not code to make the MozReview extension work yet. I'll have to add that, aye?

Comment 14

[David Lawrence [:dkl]]

Attachment: 1266588_4.patch

(In reply to Dylan William Hardison [:dylan] from comment #13)
> This does what it says, but I note there's not code to make the MozReview
> extension work yet. I'll have to add that, aye?

Added support for MozReview. I did not do it initially as MozReview, as I recall, is using XMLRPC still. But having the code in REST will help transition in the future. I had to refactor MozReview/Extension.pm to work with either REST or XMLRPC but should work. Please test it out.

dkl

Comment 15

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8753891 [details] [diff] [review]
1266588_4.patch

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

r=dylan

Comment 16

[David Lawrence [:dkl]]

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   305ba34..473e817  upstream-merge -> upstream-merge