Page MenuHomePhabricator

Confirm Reading Lists endpoint changes with callers
Closed, ResolvedPublic

Description

tl;dr: after the new Reading Lists endpoints are complete, discuss API changes with callers to ensure everything is satisfactory.

I've subscribed some folks from the mobile apps teams to this task. But we're not quite ready for the conversations yet, because the endpoints are still being finalized. Shouldn't be too long now - days or at worst weeks rather than months.

Some changes to endpoint behavior are inevitable as part of reimplementing the Reading Lists endpoints. For example, some error messages include the word "Hyperswitch", which is a RESTBase-related technology that we will no longer be using. These error messages will therefor contain different text in the new endpoints. Some headers may also be sent differently by MediaWiki than by RESTBase.

Additionally, there may be intentional changes to the contract, such as adding parameters previously supported by the Action API endpoints that RESTBase forwarded to but not exposed by RESTBase (limit, etc.) Callers should be made aware of these parameters, in case they wish to make use of them.

Completion criteria:

  • Changes have been documented in a way conducive to a conversation with the. mobile apps teams
  • IOS app team has confirmed they are comfortable with the changes (and/or any necessary code adjustments have been made)
  • Android app has confirmed they are comfortable with the changes (and/or any necessary code adjustments have been made)

Event Timeline

Status:

Initial implementation of new handlers have been merged and is available on production. Nothing is routing to them, so there is no change to callers at this time. WMF Mobile apps are still hitting the RESTBase endpoints, with no changes. This will be the case until we proactively reroute calls to the new endpoints via the API Gateway. We do not plan to do this until callers have had the opportunity to review changes, and any necessary adjustments are made.

The new endpoints are at "v0" paths in the MW REST API, which essentially means "no promises". In other words, we can still make any changes we desire to these endpoints without violating anything we have promised to other callers. And with that said, we're not aware of callers we need to support other than the WMF mobile apps, and we have also not announced the new endpoints in any way (other than phabricator tasks, gerrit changes, and their presence in the MW REST API Open API spec).

I've closed the phab tasks for the initial implementations of the new endpoints. Any subsequent changes can be handled under new tasks.

Open API spec for all new REST endpoints is available here: https://meta.wikimedia.beta.wmflabs.org/w/rest.php/

For comparison, the RESTBase spec for the existing endpoints is available here: https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/services/restbase/+/d544e9da48b9c79afb2338b7bab6960a94db6c61/v1/lists.yaml

I'm not aware of a great way to directly compare the two specs. The MW REST one includes all endpoints provided by MW REST, not just the Reading Lists endpoints. (We have an open phab task for filtering, but that is not being actively worked on at this time.) Also, the RESTBase one is very ... well ... RESTBase-ish while the MW REST one is generated from the endpoint parameter validation code and is a little underwhelming, particularly on things like response formats.

Instead, I will summarize contract changes in this comment. If it would be more helpful, I can present this in a different format or in a synchronous meeting. Just let me know.

General contract changes:

requests:

  • endpoints that require csrf tokens will now accept them by either the name "csrf_token" (per RESTBase spec) or "token" (as preferred by the MediaWiki REST API infrastructure). We would prefer to see callers use the name "token", because that would allow us to remove some special-case code. But that change can be made over time after the switchover, at caller's convenience.

response:

  • endpoints will return 403 (vs 400) in some error situations (ex. bad token).
  • error response format changed to match MW REST API conventions. See related code here.

Example error responses:

RESTBase:

{
  "type": "https://mediawiki.org/wiki/HyperSwitch/errors/bad_request",
  "title": "badtoken",
  "method": "post",
  "detail": "Invalid CSRF token.",
  "uri": "/en.wikipedia.org/v1/data/lists/setup"
}

MW REST:

{
    "errorKey": "rest-badtoken",
    "messageTranslations": {
        "en": "The <var>token</var> parameter is required unless using a CSRF-safe authentication method."
    },
    "httpCode": 403,
    "httpReason": "Forbidden"
}

Another example (this one for data/lists/?sort=invalidSort):

RESTBase:

{"type":"https://mediawiki.org/wiki/HyperSwitch/errors/bad_request","title":"Invalid parameters","method":"get","detail":"data.query.sort should be equal to one of the allowed values: [name, updated]","uri":"/en.wikipedia.org/v1/data/lists/"}

MW REST:

{
    "error":"parameter-validation-failed",
    "name":"sort",
    "value":"invalidSort",
    "failureCode":"badvalue",
    "failureData":[],
    "errorKey":"badvalue",
    "messageTranslations":{
        "en":"Unrecognized value for parameter \"sort\": invalidSort."
    },
    "httpCode":400,
    "httpReason":"Bad Request"
}

If these changes are problematic to callers, we could look into matching most closely the existing error responses. Some things seem infeasible to match (for example, it would be nonsensical to include the term "HyperSwitch" anywhere). But we could get a lot closer to the existing responses if need be.

GET /lists endpoint

Added the following parameters that were previously exposed by the underlying Action API endpoint, but not by RESTBase:

namesourcerequiredtypeexampledefaultpossible valuesdescription
dirquerynostringdescendingascendingascending, descendingsort direction
limitquerynointeger5101..10Maximum number of values to return
  • GET /lists/{id} endpoint **

This handler did not existing in the set of endpoints exposed by RESTBase, but mirrors functionality that was available through the Action API. It returns a single list, specified by id. Although unnecessary for callers (they haven't needed it before now, and nobody asked for it), it was trivial to add, and lack of this endpoint was glaring from a CRUD perspective. It will be available if callers choose to use it.

GET /lists/pages endpoint

Added the following parameter that was previously exposed by the underlying Action API endpoint, but not by RESTBase:

namesourcerequiredtypeexampledefaultpossible valuesdescription
limitquerynointeger5101..10Maximum number of values to return
  • GET /lists/changes/since endpoint **

Added the following parameters that were previously exposed by the underlying Action API endpoint, but not by RESTBase:

namesourcerequiredtypeexampledefaultpossible valuesdescription
sortquerynostringupdatednamename, updatedsort type
dirquerynostringdescendingascendingascending, descendingsort direction
limitquerynointeger5101..10Maximum number of values to return
  • GET /lists/{id}/entries endpoint **

Added the following parameters that were previously exposed by the underlying Action API endpoint, but not by RESTBase:

namesourcerequiredtypeexampledefaultpossible valuesdescription
dirquerynostringdescendingascendingascending, descendingsort direction
limitquerynointeger5101..100Maximum number of values to return
FJoseph-WMF changed the task status from Open to In Progress.Apr 5 2024, 2:43 PM

Change #1027482 had a related patch set uploaded (by BPirkle; author: BPirkle):

[mediawiki/extensions/ReadingLists@master] Make certain REST API endpoints available with trailing slashes

https://gerrit.wikimedia.org/r/1027482

Change #1027482 merged by jenkins-bot:

[mediawiki/extensions/ReadingLists@master] Make certain REST API endpoints available with trailing slashes

https://gerrit.wikimedia.org/r/1027482

The following endpoints were exposed with trailing slashes under RESTBase:

GET /lists/
POST /lists/
GET /lists/{id}/entries/
POST /lists/{id}/entries/

Per the above patch, they will all be available with or without the trailing slash under MW REST.

Per discussion, changes in error responses are problematic, as the apps care about specific fields in the error JSON. The MW REST endpoints need to be adjusted for compatibility.

See T365535: Reading List REST Interface: make error messages compatible with RESTBase for details.

@BPirkle OK! I made a branch of the app that communicates with all the new endpoints for reading list management, and everything works great so far (in the context of the new MW REST system).

I did, however, discover one other sticking point for the old apps to communicate with the new endpoints:
The new MW REST endpoints seem to be very strict about disallowing extraneous fields in the request body, and unfortunately old app versions do indeed send extraneous fields in a few of the requests. Would it be possible to allow extraneous fields in these specific cases, or would this be a deal breaker?

Namely:

  • When creating a new list, the expected parameters are name and description, but the app also sends parameters called created and updated, which are timestamps that were intended for use locally, but got sloppily serialized into the request body.
  • Same as above for creating a new entry inside a list.
  • When sending a batch of items, the expected parameter is batch, but the app also sends a parameter called entries that contains the same list of entries (facepalm). This is actually a bug, which was completely invisible until now (i.e. until extraneous parameters started being validated).

I did, however, discover one other sticking point for the old apps to communicate with the new endpoints:
The new MW REST endpoints seem to be very strict about disallowing extraneous fields in the request body, and unfortunately old app versions do indeed send extraneous fields in a few of the requests. Would it be possible to allow extraneous fields in these specific cases, or would this be a deal breaker?

Not a problem at all. MW REST is very picky about that by default, but (by design) it is easy for individual endpoints to turn off that check. I'll post a patch allowing extraneous fields in the endpoints you mentioned. If we find more affected endpoints, it'll be straightforward to turn it off for them as well.

Waiting on confirmation from iOS that the API is compatible

Waiting on confirmation from iOS that the API is compatible

Any news on this?

You probably saw this from the automatic messages above, but just to be explicit: extraneous body fields are now allowed, and error messages should now include all the fields you're already getting from RESTBase (although the values are a bit different).

@BPirkle Sorry about the delay on this! I'll make some time to look at it this week.

@BPirkle Things are working well for our existing iOS app against the new endpoints. Details can be found in this comment. You may want to take a look at the "Notables" section and confirm it sounds fine.

Quoting @Dbrant:

Finally, in your "new" version of the app, only the new endpoint URLs should be used, and only the new error format should be expected.

@BPirkle When do you need these client-side changes done by? Changes are minimal and we can potentially have it released in the next few weeks if our testing looks good.

@Tsevener as to the new endpoint URLs: these are still v0 (experimental), so they would change again before they can be considered stable. We plan to route the old URLs to the new endpoints, so using the old URLs in the clinets is the best option for now. For now, the /api/rest_v1 path is still the preferred one, we just needed to be sure that it is safe to swap out the code that handles that path.

I think this task (confirming with callers that the new endpoints are acceptable) is complete. I'm therefore marking this task as done. If I'm wrong, please reopen.

Actually rerouting calls to the new endpoints has not yet occurred. That will be tracked on T348493: Reading List REST Interface: reroute calls (and maybe also on one or more separate tasks that Service Ops create, if they choose to). I'll coordinate with them on scheduling that work. We'll alert before the switchover actually occurs, and will monitor during and after for issues. But hopefully no more work at all will be required on the part of the Mobile Apps teams as part of this switchover.

Thanks for all the input, feedback, and confirmation on this, which I know was unplanned work for you.