Page MenuHomePhabricator

Curator undoes a revision
Open, MediumPublic

Description

"As a Curator, I want to undo a revision, so that the changes made by the author are no longer part of the page content."

POST /page/{title}/undo

Request body: JSON

  • id: ID of revision to undo (required)
  • comment: additional comment (optional)

Status code:

200 - successfully undone; the body contains the new revision created by undoing the input one
400 - client error, like no such revision or revision is not in the page history
401 - the client is not authenticated and this wiki requires authentication to undo a revision
403 - the client is authenticated, but the authenticated user is forbidden to undo this revision
404 - No such page
409 - Conflict, the undo can't be done cleanly

Body: JSON, revision created by the undo action, with these properties:

  • id: revision ID of the new revision created by the undo (not the revision that was undone)
  • page: Object with these fields
    • id: ID of the page
    • title: current title of the page (might have been changed)
  • user: user who made the change, object with these fields
    • id: ID of the user or null for anonymous
    • name: name of the user or unique identifier for anonymous
  • comment: User comment ("edit summary") describing the change that generated this revision
  • timestamp: time the revision was made
  • size: size of the revision in "bogobytes" (as from DB, usually bytes)
  • delta: change from previous revision in "bogobytes" (as from DB, usually bytes)
  • minor: whether the revision was minor; boolean

Event Timeline

eprodromou updated the task description. (Show Details)Nov 6 2019, 8:41 PM
eprodromou triaged this task as Medium priority.Nov 6 2019, 9:05 PM

This endpoint is quite similar to rollback endpoint T235072 and a lot of comments/questions/ideas would apply to both or are cross-cutting between them, so I will comment once here, but please be aware that it mostly applies to both.

Important

  • Did we get a go from security for making these endpoints without a csrf tokens? Both Action API edit with undo and rollback endpoints require a form of an edit token.

Proposals/Ideas - medium important

  • As I understand, 'rollback' allows us to remove N revisions from the tip of the page history, while this one allows to remove 1 arbitrary revision from any place in the history of the page. This seems similar enough, why wouldn't we merge them? Instead of taking in a single ID parameter, we could allow to take from and to, with both being inclusive. If to is omitted, that means "from X to the end of the page history". We can leave the possibility to provide a single id if only one revision needs to be undone as syntactic sugar. For the 'rollback' endpoint this approach changes the semantics a bit, you need to know the revision ID that you want to roll back, not the ID which you want to roll back TO. Not sure how important this difference is, we could add a parameter inclusive: boolean to allow the same functionality.
  • The rollback endpoint seems quite dangerous regarding race conditions. What if while I send a request to rollback to revision from revision N to revision N-1, someone has created revision N+1 that is fixing the page? Is it being rolled back as well? This seem prone to accidents. Action API seems to have kinda protection from a situation like this by requiring a name of the user who made the revisions being undone. How do we handle this situation here?

Nitpicks/Bikeshedding

200 - OK

  • Hmmm... Technically, we're creating a rollback/undo revision here right? So maybe 201? It's quite weird to respond with 201 Created for a request to undo (kinda remove) something, so I'm not sure..

403 - Not authorized

  • 403 is actually "Forbidden". Do we want to have 401 for actually unauthrized (technically, it's unauthenticated) vs 403 forbidden (authenticated, but denied)? I think why not? Maybe we should actually respect this distinction within all the REST API endpoints.
Pchelolo added a comment.EditedNov 7 2019, 7:11 PM

One more thing to consider after reading the documentation https://en.wikipedia.org/wiki/Help:Reverting#Undo - it seems like undo on MW has a bit of a different process then revert - a contributor is presented with a diff to review before committing the action. In both of the proposed endpoints there's no way of following the same process for undoing - the compare endpoint wouldn't be able to provide a diff like that. So, if we DO want to support the same process as the core, we need additional endpoints.

Without additional endpoints, we can't support the MW process for undoing, which makes the 2 endpoints (undo and revert) even more similar to each other

Important

  • Did we get a go from security for making these endpoints without a csrf tokens? Both Action API edit with undo and rollback endpoints require a form of an edit token.

There's a new ticket, T237852, to discuss it.

Proposals/Ideas - medium important

  • As I understand, 'rollback' allows us to remove N revisions from the tip of the page history, while this one allows to remove 1 arbitrary revision from any place in the history of the page. This seems similar enough, why wouldn't we merge them?

Because the distinction is made in our Web UI. It's a user-level abstraction, and we should support both.

200 - OK

  • Hmmm... Technically, we're creating a rollback/undo revision here right? So maybe 201?

I think 200 is fine for most endpoints. I appreciate the specificity, but I'd prefer if we use 200 for success unless it really doesn't apply.

403 - Not authorized

I'll change it to "forbidden" and I'll make the semantics clearer. I'll also add a 401 status code.

One more thing to consider after reading the documentation https://en.wikipedia.org/wiki/Help:Reverting#Undo - it seems like undo on MW has a bit of a different process then revert - a contributor is presented with a diff to review before committing the action. In both of the proposed endpoints there's no way of following the same process for undoing - the compare endpoint wouldn't be able to provide a diff like that. So, if we DO want to support the same process as the core, we need additional endpoints.

Without additional endpoints, we can't support the MW process for undoing, which makes the 2 endpoints (undo and revert) even more similar to each other

Sounds good. I'll add another endpoint to view the reverse-diff of a revision. Client developers can use this if they wish before submitting the undo.

eprodromou updated the task description. (Show Details)Nov 10 2019, 2:51 PM
eprodromou updated the task description. (Show Details)

@Pchelolo I added another endpoint T237857 to show the reverse diff for a revision. I'm not going to add it for this sprint, though.

Because the distinction is made in our Web UI. It's a user-level abstraction, and we should support both.

My question if we need to support both in the API, not in the interface or user processes. My comment was about the possibility to have 1 endpoint serving both use-cases.

I added another endpoint T237857 to show the reverse diff for a revision. I'm not going to add it for this sprint, though.

AFAIK our compare handler doesn't care whether 'to' is after 'from', so it can be used for this. If we wanna be able to undo a range of revisions at once, the proposed endpoint won't be enough and we would go to the compare endpoint anyway.

eprodromou added a comment.EditedNov 10 2019, 3:48 PM

@Pchelolo I added a rollback endpoint to handle the MediaWiki rollback functionality. T237858

My question if we need to support both in the API, not in the interface or user processes. My comment was about the possibility to have 1 endpoint serving both use-cases.

Yes, I got that. I'd like to support all three ways of reverting in the API, and I would like the API interfaces to closely mirror the processes in the Web UI.