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

RESOLVED FIXED

Status

()

bugzilla.mozilla.org
API
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dkl, Assigned: dkl)

Tracking

(Blocks: 1 bug)

Merge

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

2 years ago
- 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
(Assignee)

Updated

2 years ago
Blocks: 974387
No longer blocks: 987387
(Assignee)

Updated

2 years ago
Assignee: nobody → dkl
Status: NEW → ASSIGNED
(Assignee)

Comment 1

2 years ago
(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
Flags: needinfo?(glob)
Flags: needinfo?(dylan)
(Assignee)

Comment 2

2 years ago
Created attachment 8745655 [details] [diff] [review]
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
Attachment #8745655 - Flags: review?(dylan)
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

2 years ago
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.
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.
(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.
Flags: needinfo?(glob)
we could just map the existing routes using .htacces url rewriting, btw.
Flags: needinfo?(dylan)
(In reply to Dylan William Hardison [:dylan] from comment #7)
> we could just map the existing routes using .htacces url rewriting, btw.

+1
(Assignee)

Comment 9

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

New patch coming shortly.
Attachment #8745655 - Flags: review?(dylan)
(Assignee)

Comment 10

2 years ago
Created attachment 8748915 [details] [diff] [review]
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
Attachment #8745655 - Attachment is obsolete: true
Attachment #8748915 - Flags: review?(dylan)
This doesn't have REST endpoints for any of the extensions. Was that intentional?
Flags: needinfo?(dkl)
(Assignee)

Comment 12

2 years ago
Created attachment 8752047 [details] [diff] [review]
1266588_3.patch

Missing .htaccess changes
Attachment #8748915 - Attachment is obsolete: true
Attachment #8748915 - Flags: review?(dylan)
Flags: needinfo?(dkl)
Attachment #8752047 - Flags: review?(dylan)
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?
Attachment #8752047 - Flags: review?(dylan) → review+
(Assignee)

Comment 14

2 years ago
Created attachment 8753891 [details] [diff] [review]
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
Attachment #8752047 - Attachment is obsolete: true
Attachment #8753891 - Flags: review?(dylan)
Comment on attachment 8753891 [details] [diff] [review]
1266588_4.patch

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

r=dylan
Attachment #8753891 - Flags: review?(dylan) → review+
(Assignee)

Comment 16

2 years ago
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   305ba34..473e817  upstream-merge -> upstream-merge
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.