Page MenuHomePhabricator

VE should send If-Match header to the VRS to send on to RESTbase
Closed, DuplicatePublic8 Estimated Story Points

Description

Currently, RESTBase injects a meta mw:TimeUUID tag in the body of Parsoid HTML it returns, to provide a fall-back for clients not sending the If-Match header. But, this is a hack that caused some unexpected bugs (cf T113163: Cannot save existing pages using IE11). The best solution is to abandon it and switch to the proper solution of VE setting the If-Match header.

Event Timeline

mobrovac created this task.Sep 25 2015, 1:30 PM
mobrovac raised the priority of this task from to Needs Triage.
mobrovac updated the task description. (Show Details)
mobrovac added a subscriber: mobrovac.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 25 2015, 1:30 PM

Applies to all RESTBase clients, not just VE. So, please open tickets against all.

cscott added a subscriber: cscott.Sep 25 2015, 2:14 PM

Will this require changes to the VRS? One of the benefits of T112553: Integrate the Virtual Rest Service (VRS) into core, and make it generally available (from RequestContext?) might be that we can centralize this code so that we only have to fix issues like this in a single place.

ssastry added a comment.EditedSep 25 2015, 2:54 PM

Will this require changes to the VRS? One of the benefits of T112553: Integrate the Virtual Rest Service (VRS) into core, and make it generally available (from RequestContext?) might be that we can centralize this code so that we only have to fix issues like this in a single place.

Only if VRS can maintain state across an edit's lifecycle, which I doubt it can, and which I don't think it should. So, clients aren't absolved from having to do it on their end.

It does seem simpler to me (in terms of client expectations) to leave the meta tag in the <head>. That way, clients that don't need to know or care about it can leave it alone and the only expectation is to not mess with it.

GWicke added a subscriber: GWicke.EditedSep 25 2015, 6:43 PM

It does seem simpler to me (in terms of client expectations) to leave the meta tag in the <head>. That way, clients that don't need to know or care about it can leave it alone and the only expectation is to not mess with it.

Clients already remember the version they worked with; remembering the ETag & sending it back isn't much harder, really. We could even consider getting rid of the {/revision} part of the entry point, as the same information is already contained in the ETag header. This would keep the number of explicit parameters to supply constant.

It does seem simpler to me (in terms of client expectations) to leave the meta tag in the <head>. That way, clients that don't need to know or care about it can leave it alone and the only expectation is to not mess with it.

Clients already remember the version they worked with; remembering the ETag & sending it back isn't much harder, really. We could even consider getting rid of the {/revision} part of the entry point, as the same information is already contained in the ETag header. This would keep the number of explicit parameters to supply constant.

Yup, it is not very difficult. Just simpler since the meta-tag is in the <head> already (unless you are saying the info is coming in the headers and not the HTML).

Anyway, as long as you fail fast, all clients will be forced to comply.

GWicke added a comment.EditedSep 25 2015, 7:02 PM

Anyway, as long as you fail fast, all clients will be forced to comply.

Yeah, but breaking things without warning might not be the best way to achieve consensus on API changes ;)

One thing that's nice about If-Match is that it's very general. It'll work for any kind of content, including section edits, metadata edits, or data-mw edits.

Anyway, as long as you fail fast, all clients will be forced to comply.

Yeah, but breaking things without warning might not be the best way to achieve consensus on API changes ;)

One thing that's nice about If-Match is that it's very general. It'll work for any kind of content, including section edits, metadata edits, or data-mw edits.

Right, I didn't suggest that .. but, once all known existing clients have migrated over, you fail fast.

And, as they say .. I don't have a horse in this race .. so, I'll let you guys figure it out. :)

Jdforrester-WMF renamed this task from VE should send If-Match header to RESTBase to VE should send If-Match header to the VRS to send on to RESTbase.Sep 29 2015, 7:07 PM
Jdforrester-WMF triaged this task as Medium priority.
Jdforrester-WMF set Security to None.
Jdforrester-WMF edited a custom field.
Jdforrester-WMF moved this task from To Triage to TR0: Interrupt on the VisualEditor board.
Jdforrester-WMF added subscribers: Krenair, Esanders.

@Esanders, @Krenair: Are you okay with sending the If-Match header from VE?

I have no objections to someone patching it to use this instead.

@Krenair, is this being resolved as part of the HTML <-> Wikitext switch work?

Krenair added a subscriber: Catrope.Nov 8 2015, 8:09 PM

I think we send an If-Match header containing the provided ETag now, yes. I don't know which part of the code sets mw:TimeUUID though. @Catrope?

Oh... Actually, that probably only works when you access restbase directly, not in any other configuration.

GWicke added a comment.EditedNov 8 2015, 8:25 PM

Thank you, @Krenair.

Contrary to what was stated in the original task description, VE shouldn't need to worry about mw:TimeUuid, as this is something RESTBase injects into the Parsoid HTML it returns. On the way back, it is then used as a fall-back when clients don't send if-match.

This is a fairly gross hack, and we'll stop injecting that meta tag once we know that all clients are indeed sending if-match.

GWicke updated the task description. (Show Details)Nov 8 2015, 8:27 PM

Could someone on the VE team verify whether this can be resolved?