Maniphest T197685

Clarify semantics of "base revision" and "parent revision" in EditPage, WikiPage, and PageUpdater
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	daniel
	Jun 19 2018, 1:35 PM

Description

There is considerable confusion about what "parent revision" and "base revision" means in EditPage, WikiPage, and PageUpdater.

Status Quo

EditPage:

editRevId and getBaseRevision() return the latest current revision as known to the client. This can be used to detect edit conflicts.
oldId is the revision the edit was based on. When editing based on an old revision, it's the old revision's ID.
parentRevId and getParentRevId() - The documentation says The "parent" revision is the ancestor that should be recorded in this page's revision history. It is either the revision ID of the in-memory article content, or in the case of a 3-way merge in order to rebase across a recoverable edit conflict, the ID of the newer revision to which we have rebased this page. However, this value does not to be recorded or in fact be used for anything at all. The semantics as described matches the rev_parent_id, but there is no need for EditPage to track this or set it explicitly. WikiPage (resp PageUpdater) just do this automatically.
A CAS check is performed based on rev_id, ensuring the that current revision did not change after it was compared against editRevId to detect an edit conflict.
see also T58849: Edit conflict detection by timestamp should be deprecated for how editRevId and parentRevId where introduced.

WikiPage:

$baseRevId of doEditContent: The revision ID this edit reverts to, if any. Not stored, but passed to some hooks. Doesn't seems to be used for anything by core, but some extensions, like TimedMediaHandler, use it to detect reverts.
rev_parent_id field: The ID of the revision that was current immediately before an edit. Used to display size differences in the page history (compare T193690)

NewRevisionFromEditComplete hook:

$baseID: this hook parameter contains the value of $baseRevId from WikiPage::doEditContent. It's also set to the previous revision ID by MovePage. So the itnended semantics is "ID of an earlier revision that is restored or repeated by the edit".

PageUpdater:

parent revision: the revision that is current before the edit is performed. Used for CAS checks.
baseRevisionId: the latest current revision as known to the client. This can be used to detect edit conflicts. However, it is currently called set by doEditContent() with WikiPage's semantics in mind, causing confusion. Should probably be renamed to revertsToRevisionId.

Proposed Solution

Improve documentation (and field names?) in EditPage to avoid confusion with the concepts as used in WikiPage.
Improve documentation and parameter name in WikiPage::doEditContent to specify semantics to by "ID of an earlier revision that is restored or repeated by the edit"".
Change PageUpdater::hasEditConflict() to not be based on the revision provided by setBaseRevision, but take a parameter and check against that instead.
Change name of PageUpdater::setBaseRevisionId() to setOriginalRevisionId().
Remove parentRevId logic from EditPage. getParentRevId() could just always return the current revision, if needed for B/C. But it does not seem like anything calls this.

Details

Subject	Repo	Branch	Lines +/-
Kill unused and confusing EditPage::getParentRevId().	mediawiki/core	master	+9 -34
Replace parentRevisionId in event log	mediawiki/extensions/TwoColConflict	master	+5 -4
MCR: rename $baseRevId paramter to match actual semantics.	mediawiki/core	master	+65 -124
Pass EditPage->oldid to WikiPage::doEditContent as $baseRevId.	mediawiki/core	master	+6 -1
Improve documentation of fields in EditPage	mediawiki/core	master	+39 -10

Customize query in gerrit

Related Objects
Search...

View Standalone Graph

This task is connected to more than 200 other tasks. Only direct parents and subtasks are shown here. Use View Standalone Graph to show more of the graph.

Status	Assigned	Task
		· · ·
Resolved	daniel	T174038 Initial implementation of MCR page update interface
Resolved	daniel	T197685 Clarify semantics of "base revision" and "parent revision" in EditPage, WikiPage, and PageUpdater
		· · ·

Event Timeline

daniel triaged this task as High priority.Jun 19 2018, 1:35 PM

daniel created this task.

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 19 2018, 1:35 PM

GoranSMilovanovic awarded a token.Jun 19 2018, 1:57 PM

Change 441045 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] Change PageUpdater::hasEditConflict to take a parameter.

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

Change 441046 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] Pass EditPage->oldid to WikiPage::doEditContent as $baseRevId.

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

Change 441047 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] Kill unused and confusing EditPage::getParentRevId().

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

daniel mentioned this in T88734: Use parentRevId field for section change merging instead of timestamps.Jun 19 2018, 2:36 PM

daniel mentioned this in T58849: Edit conflict detection by timestamp should be deprecated.

daniel updated the task description. (Show Details)

Change 441065 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] Improve documentation of fields in EditPage

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

daniel moved this task from Backlog to Blocked on Review on the Multi-Content-Revisions (MCR-SDC Storage Layer - phase 1) board.Jun 19 2018, 5:15 PM

GoranSMilovanovic subscribed.Jun 20 2018, 10:22 AM

Change 441065 merged by jenkins-bot:
[mediawiki/core@master] Improve documentation of fields in EditPage

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

It looks like EditPage editRevId/getBaseRevision() and parentRevId/getParentRevId() should be equivalent until the edit is actually saved. They differ after EditPage successfully does merge3() during the save, at which point editRevId continues to represent the parent of the user's submission while parentRevId represents the parent of the content actually being saved.

It looks like EditPage's oldid and WikiPage's $baseRevId should be equivalent when oldid != 0. When oldid == 0, I believe $baseRevId should be editRevId. It more or less indicates the revision that was used to initialize the textarea.

The rev_parent_id saved should match EditPage's parentRevId. It looks like there's a possible race where edits A→B→C could have C's rev_parent_id as A even though merge3() used B's content.[1]

PageUpdater's baseRevId seems to be used as both parentRevId and oldid in different places. It's used like parentRevId to detect conflicts (and maybe in checkFlags() too), and like oldid in being passed to the 'NewRevisionFromEditComplete' and 'PageContentSaveComplete' hooks.

[1]: EditPage::internalAttemptSave() calls $this->page->getLatest(), which caches inside WikiPage. 🏎 Then mergeChangesIntoContent() loads the current revision via Revision::loadFromTitle() instead of getting it from WikiPage. Pre-PageUpdater, WikiPage::doEditContent() reuses that cached ->getLatest() value for rev_parent_id. Post-PageUpdater, DerivedPageDataUpdater::grabCurrentRevision() calls WikiPage::getRevision() which uses that cached ->getLatest(). If edit B gets saved at the 🏎, that's the race.

It might help with the terminology to differentiate between the history parent (the preceding row in the page history, ie. the current verion of the page before saving the edit) and the logical parent (the revision the user based their edit on, e.g. the one pointed to by the oldid parameter, or the rollback target). There are then four possible kinds of parents:

history parent as seen by the client (last at the time of starting the edit) - this is EditPage::$editRevId / EditPage::getBaseRevision()
actual history parent (differs from the previous one if there were concurrent edits) - this is rev_parent_id
logical parent as seen by the client (oldid, or last version at the time of starting the edit if the user was not editing an oldid, or the conflicting version, if the user is doing manual edit conflict resolution) - this is EditPage::$oldId (except it's only set when it's different from the current revision at time of starting the edit)
actual logical parent (same as previous one except after a successful automatic edit conflict resolution, in which case it points to the (non)conflicting revision) - this is EditPage::getParentRevId() (and EditPage::$parentRevId when set)

$baseRevId in WikiPage::doEditContent and PageContentSaveComplete and $baseId in NewRevisionFromEditComplete are probably meant to be the logical parent (actual, since they are only called after automatic edit conflict resolution), although given the limited documentation and that the paramterer is not actually passed in the one case where it would be relevant, it's hard to tell. At least FlaggedRevs seems to assume instead that it is the revision the edit is identical to (rollback target or null revision parent), which is the current behavior.

PageUpdater::$baseRevId is documented as "the last revision known to the client" but is set after automatic edit conflict resolution, at which point that is not relevant anymore, so it probably should be the actual logical parent. PageUpdater::grabParentRevision() is the history parent as seen in the early phase of the edit submission logic (same as the actual history parent if the edit actually gets saved; might differ from the latest version of the page at the time of attempting the DB write, but in that case the save will fail).

The semantics [of parentRevId and getParentRevId()] as described matches the rev_parent_id

Not really, when editing an old version, parentRevId points to that version (the code is confusingly written; $out->addHTML( Html::hidden( 'parentRevId', $this->getParentRevId() ) ) does the real work), while rev_parent_id (in its current incarnation) always points to the previous DB row for the same title.

$baseRevId of doEditContent: The revision ID this edit was based off, if any. Not stored, but passed to some hooks. Doesn't seems to be used for anything by core.

It seems that some hook implementations interpret it as "the ID of the revision this edit is identical to" (ie. rollback target or null edit parent). Which is the current behavior but probably not the intent (although the hook documentation is minimal so it's hard to tell).

rev_parent_id field: The ID of the revision that was current immediately before an edit. Used to display size differences in the page history (compare T193690)

Might be used for other things as well as the revision object is passed to some hooks.

It looks like EditPage editRevId/getBaseRevision() and parentRevId/getParentRevId() should be equivalent until the edit is actually saved.

getBaseRevision() is the last revision at the time of starting the edit. getParentRevId() is the logical parent. They are only equivalent if the edit was based on the latest revision.

It looks like EditPage's oldid and WikiPage's $baseRevId should be equivalent when oldid != 0. When oldid == 0, I believe $baseRevId should be editRevId.

See the FlaggedRevs part of this comment for why that's not necessarily a good idea.

It looks like there's a possible race where edits A→B→C could have C's rev_parent_id as A even though merge3() used B's content.[1]

rev_parent_id is set in PageUpdater::makeNewRevision() based on PageUpdater::grabParentRevision(), which is frozen early in saveRevision() (possibly earlier, due to in-process caching of WikiPage::getRevision()). There cannot be any race after that; PageUpdater::doModify() uses WikiPage::lockAndGetLatest() which gets the current DB value and locks it against later races, and that gets compared to the frozen value. So that part is OK.

PageUpdater::hasEditConflict() would compare the frozen value with PageUpdater::getBaseRevisionId(), which prevents conflicts in theory. It is never actually called though, so as long as the concurrent edit happens after the edit conflict detection in EditPage and before PageUpdater::saveRevision() freezing the parent, nothing will stop it. Also, for that to work, the check would have to be against EditPage::$parentRevId but that does not match how getBaseRevisionId() is documented (as the task description already notes).

Change 441046 abandoned by Daniel Kinzler:
Pass EditPage->oldid to WikiPage::doEditContent as $baseRevId.

Reason:
Changed Ib78257d4d6ee7c4e to reflect current semantics

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

pmiazga subscribed.Jun 21 2018, 4:43 PM

daniel updated the task description. (Show Details)Jun 21 2018, 4:46 PM

In T197685#4305402, @Tgr wrote:

logical parent as seen by the client (oldid, or last version at the time of starting the edit if the user was not editing an oldid, or the conflicting version, if the user is doing manual edit conflict resolution) - this is EditPage::$oldId (except it's only set when it's different from the current revision at time of starting the edit)

I think you may be giving too much weight to this "logical parent". Nothing should be using this value except the 'NewRevisionFromEditComplete' and 'PageContentSaveComplete' hooks. Anything else that is using it probably should be using editRevId instead.

GTrang mentioned this in T193690: RFC: How should we fix the undeletion system?.Jun 21 2018, 8:38 PM

daniel updated the task description. (Show Details)Jun 22 2018, 11:19 AM

Change 441045 merged by jenkins-bot:
[mediawiki/core@master] MCR: rename $baseRevId paramter to match actual semantics.

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

With the updated documentation in EditPage and the clarified semantics in WikiPage and PageUpdater, this is now resolved. The remaining patch is not crucial.

daniel mentioned this in T174038: Initial implementation of MCR page update interface.Jun 22 2018, 3:40 PM

Change 441824 had a related patch set uploaded (by WMDE-Fisch; owner: WMDE-Fisch):
[mediawiki/extensions/TwoColConflict@master] Replace parentRevisionId in event log

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

WMDE-Fisch mentioned this in rESCC2cf2663306cc: Replace parentRevisionId in event log.Jun 25 2018, 11:01 AM

WMDE-Fisch mentioned this in rESCC37b85c070ba4: Replace parentRevisionId in event log.

WMDE-Fisch mentioned this in rESCC76a5008aa1d0: Replace parentRevisionId in event log.Jun 25 2018, 11:20 AM

Change 441824 merged by jenkins-bot:
[mediawiki/extensions/TwoColConflict@master] Replace parentRevisionId in event log

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

CCicalese_WMF moved this task from Blocked on Review to Done on the Multi-Content-Revisions (MCR-SDC Storage Layer - phase 1) board.Jun 26 2018, 4:25 AM

ReleaseTaggerBot added a project: MW-1.32-notes (WMF-deploy-2018-07-10 (1.32.0-wmf.12)).Jun 30 2018, 2:01 PM

• Vvjjkkii renamed this task from Clarify semantics of "base revision" and "parent revision" in EditPage, WikiPage, and PageUpdater to 2naaaaaaaa.Jul 1 2018, 1:02 AM

• Vvjjkkii reopened this task as Open.

• Vvjjkkii removed daniel as the assignee of this task.

• Vvjjkkii edited projects, added CheckUser, Connected-Open-Heritage-Batch-uploads (RAÄ-KMB_1_2017-02), Tamil-Sites, Gamepress, Hashtags, Jade, KartoEditor, Language-2018-Apr-June, New-Editor-Experiences, Mail, TCB-Team (now WMDE-TechWish); removed MW-1.32-notes (WMF-deploy-2018-07-10 (1.32.0-wmf.12)).

• Vvjjkkii updated the task description. (Show Details)

• Vvjjkkii removed subscribers: gerritbot, Aklapper.

CommunityTechBot renamed this task from 2naaaaaaaa to Clarify semantics of "base revision" and "parent revision" in EditPage, WikiPage, and PageUpdater.Jul 2 2018, 11:47 AM

CommunityTechBot closed this task as Resolved.

CommunityTechBot assigned this task to daniel.

CommunityTechBot updated the task description. (Show Details)