Page MenuHomePhabricator

Reading List REST Interface: make error messages compatible with RESTBase
Closed, ResolvedPublic

Description

Error responses from the new MW REST API endpoints for Reading Lists (see T348491: Reading List REST Interface: create REST endpoints) differ from the existing RESTBase Reading Lists endpoints. Our mobile apps access fields within the existing JSON response, so this would be a breaking change. In particular, error responses are used to determine whether reading lists are set up for a user, so this would affect normal operations and not just error situations.

Because users may choose to run older versions of mobile apps, there would be no realistic way to change this on the mobile app side, even if we wanted to. So to avoid breakage, adjust error responses from the new endpoints to match error responses from the existing endpoints.

Example error response from RESTBase:

{
  "type": "https://mediawiki.org/wiki/HyperSwitch/errors/bad_request",
  "title": "readinglists-db-error-not-set-up",
  "method": "post",
  "detail": "Reading lists have not been set up for this user.",
  "uri": "/en.wikipedia.org/v1/data/lists/teardown"
}

Example error response from current MW REST endpoint:

{
    "errorKey": "readinglists-db-error-not-set-up",
    "httpCode": 400,
    "httpReason": "Bad Request",
    "messageTranslations": {
        "en": "Reading lists have not been set up for this user."
    }
}

The teardown endpoint was used as an example, but this applies to all the MW REST Reading Lists endpoints:

  • POST lists/setup endpoint
  • POST lists/teardown endpoint
  • GET lists endpoint
  • GET lists/pages endpoint
  • GET lists/changes/since endpoint
  • POST lists endpoint
  • POST lists/batch endpoint
  • PUT lists endpoint
  • DELETE lists endpoint
  • GET lists/{id}/entries endpoint
  • POST lists/{id}/entries endpoint
  • POST lists/{id}/entries/batch endpoint
  • DELETE lists/{id}/entries endpoint

Event Timeline

@Dbrant , question that I didn't think to ask during our synchronous meeting: are extraneous fields problematic for the app? In other words, if the error response json had the values from both the RESTBase error AND the MW REST error, would the app be happy? So for example:

{
  "type": "https://mediawiki.org/wiki/HyperSwitch/errors/bad_request",
  "title": "readinglists-db-error-not-set-up",
  "method": "post",
  "detail": "Reading lists have not been set up for this user.",
  "uri": "/en.wikipedia.org/v1/data/lists/teardown"
  "errorKey": "readinglists-db-error-not-set-up",
  "httpCode": 400,
  "httpReason": "Bad Request",
  "messageTranslations": {
      "en": "Reading lists have not been set up for this user."
  }
}

I'm not saying we would (or wouldn't) do that, just confirming what the behavior would be.

I'm not saying we would (or wouldn't) do that, just confirming what the behavior would be.

It would certainly makes things a lot simpler. Adding fields is trivial. Replacing error handling would be more work.

Intersingly, the RBServiceError class already has the errorKey and messageTranslations fields. How are they being populated currently?

I'm not saying we would (or wouldn't) do that, just confirming what the behavior would be.

It would certainly makes things a lot simpler. Adding fields is trivial. Replacing error handling would be more work.

Screenshot 2024-05-22 at 5.33.24 PM.png (1×1 px, 1 MB)

That'd just be a matter of piping the additional fields into LocalizedHttpException via the $errorData array, right? Most (but not quite all) of the Reading Lists handlers create exceptions through the RestUtilTrait::die() and RestUtilTrait::dieIf() functions. At a glance, most of the extra info (title, method, uri, detail) would be easy enough to add those. Could consider a helper function or maybe even a ReadingListRestException class that extends LocalizedHttpException to add the extras.

I'm assuming (@Dbrant , please correct me if I'm wrong) that the apps don't care exactly what string is in the "type" field, just that one exists and has something sensical in it.

Will talk to @FJoseph-WMF regarding who we assign this to, but assuming extraneous fields are not problematic, this shouldn't be a huge deal.

That'd just be a matter of piping the additional fields into LocalizedHttpException via the $errorData array, right?

I think so, yes.

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

[mediawiki/extensions/ReadingLists@master] Add error response fields per caller requirements

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

BPirkle changed the task status from Open to In Progress.May 28 2024, 3:33 PM

@Dbrant , question that I didn't think to ask during our synchronous meeting: are extraneous fields problematic for the app? In other words, if the error response json had the values from both the RESTBase error AND the MW REST error, would the app be happy?
I'm not saying we would (or wouldn't) do that, just confirming what the behavior would be.

Sorry, missed this question! Yes, the app doesn't care about extraneous fields, and will tolerate both variations in one response. Agreed that this a good way to proceed.

Intersingly, the RBServiceError class already has the errorKey and messageTranslations fields. How are they being populated currently?

Also a good observation: we are already (!) using a single model class to represent either a RESTBase or MW REST error response. (even though we're currently not doing anything with the errorKey field, so we will definitely still need your change that outputs both error responses at once.)

Change #1036590 merged by jenkins-bot:

[mediawiki/extensions/ReadingLists@master] Add error response fields per caller requirements

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

BPirkle moved this task from In Progress to Done on the MW-Interfaces-Team board.