Page MenuHomePhabricator

Core REST API page history endpoint returns invalid links
Closed, ResolvedPublicBUG REPORT

Description

Expected behavior

The page history endpoint returns older, newer, and latest properties with valid API routes to fetch the corresponding set of resources.

Observed behavior

older, newer, and latest URLs are missing the API version, making them invalid.

Request: https://en.wikipedia.org/w/rest.php/v1/page/Agapanthus/history

Response:

...
"latest":"https://en.wikipedia.org/w/rest.php/page/Agapanthus/history",
"older":"https://en.wikipedia.org/w/rest.php/page/Agapanthus/history?older_than=896094138"
...

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 12 2020, 5:00 PM
WDoranWMF triaged this task as High priority.May 12 2020, 5:04 PM
WDoranWMF moved this task from Backlog to Ready on the Platform Team Workboards (Green) board.
WDoranWMF raised the priority of this task from High to Needs Triage.May 12 2020, 5:08 PM
WDoranWMF changed the subtype of this task from "Task" to "Bug Report".
daniel added a subscriber: daniel.May 19 2020, 6:52 PM

PageHistoryHandler::processDbResults() uses Router::getRouteUrl() to construct the URL to return for older, newer, and latest. getRouteUrl() prepends the router's base URL to the path it is given. The path constructed by processDbResults() startes with '/page/', without the 'v1' prefix. The simple fix is to just add that.

However, it would be better if a Handler had a way to discover its own full URL, or its own full path. While a handler can reasonably be expected to know its own "logical" path, it may not know the "path prefix" used in the coreRoutes.json. In particular, the code in Handler should not need to change when the route is changed from experimental ("/coredev/v0/") to production ("/v1"). We currently do not model these "path prefixes" anywhere.

Currently, a handle can discover its own path using $this->getConfig()['path']. In the case of PageHistoryHandler, this would result in "/v1/page/{title}/history". It would be nice to have a utility function that would return the path with placeholders resolved, such as Handler::getRoutePath( $pathParams = [] ). So this line

$path = '/page/' . urlencode( $titleObj->getPrefixedDBkey() ) . '/history';

would become

$path = $this->getRoutePath( [ 'title' => $titleObj->getPrefixedDBkey() ] );

and then

$url = $this->getRouter()->getRouteUrl().

Maybe the two could be combined into Handler::getRouteUrl( $pathParams = [], $queryParams = [] ).

This leaves the problem of discovering another handler's full URL. This would be needed by the page creation endpoint, see T253143: Core REST API page creation endpoint returns invalid redirect.

daniel added a comment.EditedMay 19 2020, 7:01 PM

To support the above, HandlerTestTrait::executeHandler() will have to supply the 'path' key in $config.

To catch this, the integration test on tests/api-testing/REST/PageHistory.js has to have these lines:

assert.include( res.body.latest, `page/${title}/history` );
assert.include( res.body.older, `page/${title}/history?older_than=${expected[ expected.length - 1 ].id}` );

changed to

assert.include( res.body.latest, `/v1/page/${title}/history` );
assert.include( res.body.older, `/v1/page/${title}/history?older_than=${expected[ expected.length - 1 ].id}` );
daniel triaged this task as High priority.May 19 2020, 7:22 PM

@daniel I may be completely missing something. but doesn't a handler have access to its full path via Handler::$request? Could we use something like $this->getRequest()->getUri()->getPath(); in the PageHistoryHandler?

@daniel I may be completely missing something. but doesn't a handler have access to its full path via Handler::$request? Could we use something like $this->getRequest()->getUri()->getPath(); in the PageHistoryHandler?

Yes, but that path may be different from what we want to expose to clients, if we are behind a proxy.

Talking to Tim, I just realized that if we want to support multiple versions for the same route, the version prefix for the target route can't be automatic. It has to be hard coded. So for the use case in T253143, we have to hard code the version. For the use case here, we just make a convenience function based on $this->getConfig['path'].

Change 598516 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] PageHistoryHandler: fix self-reference route

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

daniel claimed this task.May 25 2020, 7:31 PM
daniel moved this task from Ready to Waiting for Review on the Platform Team Workboards (Green) board.

Change 598516 merged by jenkins-bot:
[mediawiki/core@master] PageHistoryHandler: fix self-reference route

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

eprodromou closed this task as Resolved.Jul 1 2020, 6:57 PM
eprodromou added a subscriber: eprodromou.

Tested. This is done! TY @daniel