Page MenuHomePhabricator

RESTBase should 404 if it cannot satisfy requested TID
Closed, ResolvedPublic

Description

Currently, for requests for Pardoid content with a specific TID, it the content cannot be served from storage, RESTBase creates a new render and responds with 200. It might be wrong. If a specific TID was requested and can not be satisfied, RESTBase should probably 404.

However, it's up for discussion. For VE serving matching-tid is crucial - doing the data-parsoid by-tid requests on the best effort principle could be catastrophic and it can create a dirty diff without the user realizing what's happening. But here, again, we are facing a product decision - what's better - reject saving an edit that the user might have been working on for a while, or force-saving it regardless of the possibility to create a dirty diff. Maybe someone from VisualEditor could provide some thoughts on that?

For reading content, like MCS and PCS, non-matching pieces of content (tids) might not be such a big deal and rejecting the user from reading the content that might be a bit stale seems worse.

Please note, that events of TIDs not being found should, in theory, be impossible and should be results of a bug in the software, so the decision we're making whether to make TID requests strict or best effort is more theoretical than practical

Event Timeline

Pchelolo created this task.Sep 19 2018, 8:25 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 19 2018, 8:25 PM
Pchelolo updated the task description. (Show Details)

In my mind, asking for a non-existent tid is the same as asking for a non-existent revision, and should, hence, result in a 404. You do have a good point that it might affect Visual Editor, but in practice the 24h window is more than ample time to finish an edit. We should also take into account that VE edits are commonly based off of the latest revisions which we have in the latest bucket rather than the stash one. But I agree, let's see what @Esanders thinks.

mobrovac triaged this task as High priority.Sep 20 2018, 6:04 AM
cscott added a subscriber: cscott.Sep 20 2018, 4:33 PM

Agreed that non-existent tid should result in 404. In the original design, tids enabled deterministic parses, so they specify a particular set of templates/media/etc. In the current implementation, if the tid isn't in storage, we can't actually recreate it, so we should return 404. In the future if we even implement deterministic parses, we'd propagate tids through the process to select appropriate revisions of templates/media/seed "random" numbers/etc, and in that (hypothetical) future we'd start returning non-404 responses here. But for now we'd just be lying to the client, and that makes it hard for clients who actually do care about tid consistency to figure out how to Do The Right Thing (whatever that is).

Ok, I guess you're right. For the sake of correctness here's a PR https://github.com/wikimedia/restbase/pull/1066

Mentioned in SAL (#wikimedia-operations) [2018-09-25T08:25:06Z] <mobrovac@deploy1001> Started deploy [restbase/deploy@40b81a8]: Do not dynamically generate Parsoid content if TID is provided - T204880

Mentioned in SAL (#wikimedia-operations) [2018-09-25T08:37:05Z] <mobrovac@deploy1001> Finished deploy [restbase/deploy@40b81a8]: Do not dynamically generate Parsoid content if TID is provided - T204880 (duration: 12m 00s)

mobrovac closed this task as Resolved.Sep 25 2018, 8:51 AM
mobrovac edited projects, added Services (done); removed Services (doing).

Merged and deployed.

Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptSep 25 2018, 8:51 AM