Return 409 if a tranform request is done for a restricted revision

[Pchelolo]

Right now if the page was deleted or a revision was restricted while it was edited in VE, transform endpoints return 400 errors, however this situation is more about an edit conflict.

[d00rman]

LGTM. @subbuss do you think it is a good practice to return a 429 CONFLICT when a transform for a deleted page or revdeleted revision is attempted?

[d00rman]

Hmm, while it is true that the code above will disappear soon, currently it will log something other than 429 (either a 404 or a 403). Should we try to integrate these two so that we can easily match user reports with logs (which was the initial incentive to introduce this temp code) ?

Also, some small nits on this line:
s/the//
s/wkile/while/

[d00rman]

@subbuss rightfully pointed out our mistake. 429 stands for too many requests, which has nothing to do with what we are trying to achieve here.

We have at our disposal 409 (CONFLICT), 410 (GONE) and 423 (LOCKED). Ideally, I'd vote for a 410 for deleted pages and 423 for revdeleteled revisions, but since 423 is a WebDAV code, I propose to go with 410 for deleted pages and 409 for revdeletions.

[gwicke]

In both HTML save and transform end points, we already ask clients to set an If-Match header specifying the original etag (revision/timeuuid). Right now, the transform end points still provide the meta tag hack fall-back (mostly for VE's benefit), but it would be a lot cleaner (and less brittle) to phase that out in favor of working with If-Match only. A page or revision deletion would break the precondition (if provided), which we would indicate with a 412 Precondition Failed response.

[Pchelolo]

@d00rman hah, we've been fighting so much with 429 codes, that it just settled in my head..

@gwicke The if-match header is parsed (required) only in html/to/* endpoints, so it might be not passed on others, like wikitext to html transformation, and this is needed on all transformations.

[d00rman]

Let's keep it like that for now, but I agree with @gwicke that in the long term 412 would be the way to go.

[gwicke]

@gwicke The if-match header is parsed (required) only in html/to/* endpoints, so it might be not passed on others, like wikitext to html transformation, and this is needed on all transformations.

The meta tag hack certainly does not work for wikitext, and the end point does not support passing in the timeuuid at all right now. It is also not absolutely needed, as any html/data-parsoid copy will do for incremental re-parsing, as long as it's consistent.

It would however be more consistent to move to using the ETag for wikitext as well. We can also support passing it in the POST data with a mechanism like https://phabricator.wikimedia.org/T111748.

[Pchelolo]

@gwicke Actually, you've made me think that we could be handling this in a wrong way.

On html->* transforms, the tid of the original content is required now, but in case original html is not in storage, and we are generating it, we font's include that tid in request for revision info, and get the latest info version of revision info. In this particular case, the page's been deleted, so we get these errors. Shouldn't we instead get the revision info for the original tid? (the latest one before the original tid)

[gwicke]

@Pchelolo, I guess you were thinking about a use case where somebody editing a now-deleted revision will still expect a successful save based on that revision? In that case, we don't really have a tid for the revision info -- all we have is the timestamp contained in that tid. It might be possible to retrieve the revision info based on that, but it's a bit icky.

The risk I see with enabling such an edit is that the revision deleted content could be perpetuated in the edit. From a user perspective, triggering an edit conflict workflow would probably make most sense, which fits with the If-Match handling.