Page MenuHomePhabricator

API: Allow edit conflict detection based on revid
Closed, ResolvedPublic

Description

Please allow edit conflict detection based on revid for the api. This should an alternative for the client, because sometimes it is easy to store a id instead of a full timestamp.

Thanks.


Version: 1.20.x
Severity: enhancement
See Also:
https://bugzilla.wikimedia.org/show_bug.cgi?id=22783

Details

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 12:00 AM
bzimport set Reference to bz32037.
bzimport added a subscriber: Unknown Object (MLST).

How is the timestamp a problem? 14 digits can't be stored?

Timestamp is very old MediaWiki way for this, since the revision id exist, that id should be used.

The revision id of the latest revision is stored within page table, that makes it easy to retrieve this value for api calls and the revision id is already part of mw.config inside the gui (wgCurRevisionId)

CC-ing Catrope and Reedy for their insight, I don't know the API well enough. If revision IDs is technically superior / more logical (internally) then go for it.

On the other hand, in defense of the current system:

page_touched/page_latest are both similarly easy to retrieve as is rev_timestamp/rev_id.

In order to make an edit with the API, one should also retrieve the wikitext of a page (through the api, prop=revisions) and in that api ids and timestamp are both just as easy to retrieve.

What is inside mw.config should not be relevant because that is not part of the API and not be relied on for API interaction (generally use API output for API input). Moreover because wgCurRevisionId is hardcoded in the page which means there is two unreliability factors:

  • It may be cached by the browser
  • The page can be edited since the page has been viewed (e.g. use viewing the page, and performing all kinds of actions, then initiates your script and by that time all kinds of edits could've been made, including but not limited to ajax edits by the user himself, so always get the info from the API when you need it)

Consider the following use case: A user wants to add a maintenance template at the top of the article he is reading using a fancy ajaxy gadget.

Currently the script has to do the following steps:

  1. Get the wikitext, timestamps and token via API.
  2. Check whether the wikitext belongs to the version the user currently is looking at (perhaps somebody else has added exactly that template since the page has been viewed)
  3. Prepend the article to the text and use the edit API to save the change.

If revids could be used, this would be simpler:

  1. Get edit token and revid from the data available to JavaScript.
  2. Call the edit API to make the change (with prependtext parameter there is no need to get the wikitext first)

If the page was edited in the meantime, an edit conflict will be detected (revid sent to API != revid of last version in DB), which is exactly the desired behavior.

The current step #2 is not easy at all (e.g. when the user wants to add {{db-nonsense}}, the script should notice if {{TeMpLaTe:Db-g1}} was added since the user viewed the page), and we currently need two API calls where one would be enough.

(In reply to comment #4)

If the page was edited in the meantime, an edit conflict will be detected
(revid sent to API != revid of last version in DB), which is exactly the
desired behavior.

It's not correct. A conflict won't be raised if an automatic merge succeeds.

(In reply to comment #4)

Consider the following use case: A user wants to add a maintenance template
at
the top of the article he is reading using a fancy ajaxy gadget. [...]

So in short this is bug 34580 aka bug 22783?

(In reply to comment #6)

So in short this is bug 34580 aka bug 22783?

No. In my use case I want an edit conflict, even if I'm only prepending text (as it could be the same text that was added by the other user causing the potential edit conflict).

IMHO in an ideal world edit conflict detection would work this way:

  1. The user didn't send the baseRevision parameter -> Just edit the page according to the other data the user sent, ignore any possible conflict.
  2. The user sent baseRevision === 0 -> If the page is still missing, create it. -> If the page exists, but all edits are from the current user, edit the page without edit conflict. -> Otherwise try to resolve the edit conflict with diff3 (base: empty article), report conflict if that fails.
  3. The user sent baseRevision !== 0 -> If the page exists and the current revision is the user's baseRevision, edit the article. -> If the page exists in a different revision, but all edits between the baseRevision and the current revision are made by the current user, edit without conflict. -> If the page exists in a different revision and edits by other users, try to resolve the conflict with diff3 (base determined by content of baseRevision), report conflict if that fails. -> If the page doesn't exist, tell the user it was deleted while he was editing.

It seems to me that the editRevId field that has been in EditPage for a couple of years now does what this ticket asks for. Am I missing something?

You're missing that ApiEditPage was never updated to allow clients to specify it?

@Anomie Oh, right. ApiStashEdit has baserevid which corresponds to the editRevId field. I suggest to simply add the same to ApiEditPage. Since the behavior of ApiEditPage should be consistent with the HTML form, and editRevId is used to detect edit conflicts in the form, ApiEditPage should behave exactly the same. The the question is just waht to call the parameter. baserevid is what ApiStashEdit uses, so I suggest to use the same here. editRevId is not a good name.

While working on this in the context of T230843, I discovered an unexpected wrinkle: conflict detection based on basetimestamp will ignore self-conflicts, but detection based on revision ID will not (could be implemented, but would require the base revision to be loaded to retrieve the timestamp). I personally think we shouldn't ignore self-conflicts (see T175745), and if this feature is useful, it's only in the context of the UI. However, having this slight difference in semantics between basetimestamp and baserevid is somewhat surprising.

Change 577345 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] ApiEditPage: add baserevid parameter

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

Pchelolo subscribed.

Daniel has started working on this, moving to green sub team work board to be handled accordingly.

Change 577345 merged by jenkins-bot:
[mediawiki/core@master] ApiEditPage: add baserevid parameter

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

matmarex assigned this task to daniel.

Thanks!