Page MenuHomePhabricator

Transform "Try it out" doesn't work in REST Sandbox
Closed, ResolvedPublicBUG REPORT

Description

Steps to replicate the issue (include links if applicable):

What happens?:

  • The routes do not match the reference documentation; "from" value must be populated by the user instead of pre-populating with 'wikitext'
  • In cases where the user is expected to enter free text (either HTML or wikitext) there is no post body field available in the editor where the data can be populated.
  • Where the title/revision values can be used instead of free text, 415 - Unsupported Media Type Error is returned.

Note: These seem to apply for all endpoints in the Transform space.

What should have happened instead?:

  • Post body editor should be available where expected.
  • Additional parameters should all be present and able to be populated.
  • "from" should be populated within the route, per the reference documentation.
    • Alternatively: consolidate to a single endpoint with supported options listed for to and from values
  • The respective page transformation should be returned.

Software version (on Special:Version page; skip for WMF-hosted wikis like Wikipedia):

Other information (browser name/version, screenshots, etc.):

Event Timeline

BPirkle subscribed.

Issue confirmed. Changes may be needed both to the reference documentation and to the endpoint.

The "transform" endpoints are complicated, in part because we made one handler match a bunch of routes. Maybe there were other ways to do this, maybe there weren't. But for this version of the endpoint, it is what it is.

Keep in mind that Parsoid has its own TransformHandler that derives from the core TransformHandler. So we can't just modify the core TransformHandler without making sure we don't break Parsoid (ask me how I know...) I suspect that some design decisions about the core endpoint were driven by this (Daniel or Derrick might know more). Also, TransformHandler extends ParsoidHandler, which has some opinions about path parameters and is extended by Parsoid. So we have to be careful about anything we change there, too.

The routes do not match the reference documentation; "from" value must be populated by the user instead of pre-populating with 'wikitext'

The reference documentation is wrong vs how the endpoint is coded. The route is defined as /v1/transform/{from}/to/html/{title}. The "from" parameter is a string taken from the path. The OpenAPI spec faithfully represents this. The reference documentation does not. (For the record, this is a GREAT benefit of finally having OpenAPI specs for all these endpoints. This sort of thing is incredibly easy to miss otherwise.) Given the various contexts in which all this is used, I'm inclined to change the reference documentation to match the endpoint for this part. At least as an initial change. Reconsidering how these endpoints are implemented seems like a v2 thing to me.

Compare to the routes exposed by RESTbase. You'll see /transform/wikitext/to/html/{title} as an available route. In that case, "from" is indeed not a parameter. That's perhaps what the reference documentation author was looking at when the wrote that, assuming the core endpoint would be the same. But the endpoints are different between RESTbase and core.

Interestingly, if I submit the curl command from the REST Sandbox myself, from my local machine, I get a 200 and the parsed content, not a 415 I also get the 200 with the response if I submit the equivalent of the curl command via Postman (where it is a little easier to read).

For reference, in case things change, here's the command I see in the REST Sandbox:

curl -X 'POST' \
  'https://test.wikipedia.org/w/rest.php/v1/transform/wikitext/to/html/Earth' \
  -H 'accept: text/html' \
  -d ''

Regarding lack of a field in the REST Sandbox to enter the POST data, that's because neither TransformHandler nor ParsoidHandler have a getBodyParamSettings() function. The OpenAPI spec therefore does not know about anything related to the POST body. Compare to CreationHandler, which is also a POST, and which does define getBodyParamSettings(). This may be something we can reasonably fix in the code without breaking other things. Just having a getBodyParamSettings() function doesn't require anything else in the endpoint to use the POST data via $this->getValidatedBody(). So we can (unverified) probably add a getBodyParamSettings() just to make the OpenAPI spec match expectations without actually using the resulting validated body anywhere. If we go this way, we should add a comment in the new TranformHandler::getBdoyParamSettings() making all this very clear, so our future selves aren't hopelessly confused.

Notes from grooming:

  • We may want to add an additional handler to flatten the transform routes, such that they are definitively declared to end-users in the sandbox documentation for MW Core.
  • Include Content Transform Team in discussion.

We are creating a research spike to create a PoC for possible solution(s).

Spike task is complete, including merging a patch to production. Need to evaluate status and see what work remains.

Just so we don't lose track of it with https://phabricator.wikimedia.org/T392608 being resolved -- we do need to figure out what to do in terms of hiding and/or deprecating the endpoints with the trailing slash. I would be in favor of hiding them from the sandbox and announcing an official deprecation. I'll add subtasks here for both of those efforts.

Just so we don't lose track of it with https://phabricator.wikimedia.org/T392608 being resolved -- we do need to figure out what to do in terms of hiding and/or deprecating the endpoints with the trailing slash. I would be in favor of hiding them from the sandbox and announcing an official deprecation. I'll add subtasks here for both of those efforts.

A general-purpose mechanism for deprecating MW REST API endpoints was added under T405038: [SPIKE] Propose an in-code mechanism for marking endpoints as deprecated. Subsequently, the "trailing slash" endpoints were deprecated in the OpenAPI spec and the REST Sandbox, and callers to them receive the "deprecation" header, all under T404854: Mark and announce transform endpoints with trailing slash as deprecated.