Daniel Kinzler has uploaded a new change for review. (
https://gerrit.wikimedia.org/r/366879 )
Change subject: Simplify edit conflict detection in EditEntity.
......................................................................
Simplify edit conflict detection in EditEntity.
This change does two things:
* Always check for "late" conflicts: when a new revision was added while
the edit was in progress, always fail.
* If no base revision is given, always use the latest revision as base
revision. That removes the "no base revision" mode.
Change-Id: I514a4acfa4e6ead356a5a7f0480a172f2086e012
---
M repo/includes/Api/EntitySavingHelper.php
M repo/includes/EditEntity.php
M repo/includes/EditEntityFactory.php
M repo/includes/Specials/SpecialWikibaseRepoPage.php
M repo/includes/UpdateRepo/UpdateRepoJob.php
M repo/tests/phpunit/includes/EditEntityTest.php
6 files changed, 76 insertions(+), 69 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Wikibase
refs/changes/79/366879/1
diff --git a/repo/includes/Api/EntitySavingHelper.php
b/repo/includes/Api/EntitySavingHelper.php
index 845e2b5..831bfb3 100644
--- a/repo/includes/Api/EntitySavingHelper.php
+++ b/repo/includes/Api/EntitySavingHelper.php
@@ -333,7 +333,7 @@
}
if ( !$this->baseRevisionId ) {
- $this->baseRevisionId = isset( $params['baserevid'] ) ?
(int)$params['baserevid'] : null;
+ $this->baseRevisionId = isset( $params['baserevid'] ) ?
(int)$params['baserevid'] : 0;
}
$editEntityHandler = $this->editEntityFactory->newEditEntity(
diff --git a/repo/includes/EditEntity.php b/repo/includes/EditEntity.php
index b51c76e..3b9b66a 100644
--- a/repo/includes/EditEntity.php
+++ b/repo/includes/EditEntity.php
@@ -8,6 +8,7 @@
use MWException;
use ReadOnlyError;
use RequestContext;
+use RuntimeException;
use Status;
use Title;
use User;
@@ -173,12 +174,12 @@
* May be null when creating a new entity.
* @param User $user the user performing the edit
* @param EditFilterHookRunner $editFilterHookRunner
- * @param int|bool $baseRevId the base revision ID for conflict
checking.
- * Defaults to false, disabling conflict checks.
- * `true` can be used to set the base revision to the latest
revision:
- * This will detect "late" edit conflicts, i.e. someone
squeezing in an edit
- * just before the actual database transaction for saving beings.
- * The empty string and 0 are both treated as `false`, disabling
conflict checks.
+ * @param int $baseRevId the base revision ID for conflict checking.
+ * Use 0 to indicate that the current revision should be used as
the base revision,
+ * effectively disabling conflict detections. true and false
will be accepted for
+ * backwards compatibility, but both will be treated like 0.
Note that the behavior
+ * of this class changed so that "late" conflicts that arise
between edit conflict
+ * detection and database update are always detected, and result
in the update to fail.
* @param IContextSource|null $context the context to use while
processing
* the edit; defaults to RequestContext::getMain().
*
@@ -194,7 +195,7 @@
EntityId $entityId = null,
User $user,
EditFilterHookRunner $editFilterHookRunner,
- $baseRevId = false,
+ $baseRevId = 0,
IContextSource $context = null
) {
$this->entityId = $entityId;
@@ -203,8 +204,8 @@
$baseRevId = (int)$baseRevId;
}
- if ( $baseRevId === 0 ) {
- $baseRevId = false;
+ if ( is_bool( $baseRevId ) ) {
+ $baseRevId = 0;
}
$this->user = $user;
@@ -313,13 +314,14 @@
}
/**
- * If no base revision was supplied to the constructor, this will
return false.
- * In the trivial non-conflicting case, this will be the same as
$this->getLatestRevisionId().
+ * Return the ID of the base revision for the edit. If no base revision
ID was supplied to
+ * the constructor, this returns the ID of the latest revision. If the
entity does not exist
+ * yet, this returns 0.
*
- * @return int|bool
+ * @return int
*/
private function getBaseRevisionId() {
- if ( $this->baseRevId === null || $this->baseRevId === true ) {
+ if ( $this->baseRevId === 0 ) {
$this->baseRevId = $this->getLatestRevisionId();
}
@@ -327,9 +329,9 @@
}
/**
- * Returns the edits base revision.
- * If no base revision was supplied to the constructor, this will
return null.
- * In the trivial non-conflicting case, this will be the same as
$this->getLatestRevision().
+ * Return the the base revision for the edit. If no base revision ID
was supplied to
+ * the constructor, this returns the latest revision. If the entity
does not exist
+ * yet, this returns null.
*
* @return EntityRevision|null
* @throws MWException
@@ -338,9 +340,7 @@
if ( $this->baseRev === null ) {
$baseRevId = $this->getBaseRevisionId();
- if ( $baseRevId === false ) {
- return null;
- } elseif ( $baseRevId === $this->getLatestRevisionId()
) {
+ if ( $baseRevId === $this->getLatestRevisionId() ) {
$this->baseRev = $this->getLatestRevision();
} else {
$id = $this->getEntityId();
@@ -367,9 +367,13 @@
* - revision: Revision the new revision object
* - errorFlags: bit field indicating errors, see the XXX_ERROR
constants.
*
- * @return Status|null
+ * @return Status
*/
public function getStatus() {
+ if ( $this->status === null ) {
+ throw new RuntimeException( 'The status is undefined
until attemptSave() has been called' );
+ }
+
return $this->status;
}
@@ -395,14 +399,19 @@
}
/**
- * Determines whether an edit conflict exists, that is, whether another
user has edited the same item
- * after the base revision was created.
+ * Determines whether an edit conflict exists, that is, whether another
user has edited the
+ * same item after the base revision was created. In other words, this
method checks whether
+ * the base revision (as provided to the constructor) is still current.
If no base revision
+ * was provided to the constructor, this will always return false.
+ *
+ * If the base revision is different from the current revision, this
will return true even if
+ * the edit conflict is resolvable. Indeed, it is used to determine
whether conflict resolution
+ * should be attempted.
*
* @return bool
*/
public function hasEditConflict() {
- return $this->doesCheckForEditConflicts()
- && !$this->isNew()
+ return !$this->isNew()
&& $this->getBaseRevisionId() !==
$this->getLatestRevisionId();
}
@@ -548,7 +557,12 @@
}
/**
- * Attempts to save the new entity content, chile first checking for
permissions, edit conflicts, etc.
+ * Attempts to save the given Entity object.
+ *
+ * This method performs entity level permission checks, checks the edit
toke, enforces rate
+ * limits, resolves edit conflicts, and updates user watchlists if
appropriate.
+ *
+ * Success or failure are reported via the STatus object returned by
this method.
*
* @param EntityDocument $newEntity
* @param string $summary The edit summary.
@@ -558,10 +572,14 @@
* Null will fail the token text, as
will the empty string.
* @param bool|null $watch Whether the user wants to watch the entity.
* Set to null to apply default
according to getWatchDefault().
+ *
* @return Status
+ *
* @throws MWException
* @throws ReadOnlyError
+ *
* @see WikiPage::doEditContent
+ * @see EntityStore::saveEntity
*/
public function attemptSave( EntityDocument $newEntity, $summary,
$flags, $token, $watch = null ) {
if ( wfReadOnly() ) {
@@ -595,10 +613,20 @@
}
//NOTE: Make sure the latest revision is loaded and cached.
- // Would happen on demand anyway, but we want a
well-defined point at which "latest" is frozen
- // to a specific revision, just before the first check for
edit conflicts.
+ // Would happen on demand anyway, but we want a
well-defined point at which "latest" is
+ // frozen to a specific revision, just before the first
check for edit conflicts.
+ // We can use the ID of the latest revision to protect
against race conditions:
+ // if getLatestRevision() was called earlier by
application logic, saving will fail
+ // if any new revisions were created between then and now.
+ // Note that this protection against "late" conflicts is
unrelated to the detection
+ // of edit conflicts during user interaction, which use
the base revision supplied
+ // to the constructor.
$this->getLatestRevision();
- $this->getLatestRevisionId();
+ $raceProtectionRevId = $this->getLatestRevisionId();
+
+ if ( $raceProtectionRevId === 0 || ( $flags & EDIT_NEW ) > 0 ) {
+ $raceProtectionRevId = false;
+ }
$this->applyPreSaveChecks( $newEntity ); // modifies
$this->status
@@ -628,7 +656,7 @@
$summary,
$this->user,
$flags | EDIT_AUTOSUMMARY,
- $this->doesCheckForEditConflicts() ?
$this->getLatestRevisionId() : false
+ $raceProtectionRevId
);
$this->entityId = $newEntity->getId();
@@ -672,15 +700,6 @@
$this->getBaseRevision();
return $this->status;
- }
-
- /**
- * Whether this EditEntity will check for edit conflicts
- *
- * @return bool
- */
- public function doesCheckForEditConflicts() {
- return $this->getBaseRevisionId() !== false;
}
/**
diff --git a/repo/includes/EditEntityFactory.php
b/repo/includes/EditEntityFactory.php
index b815e8d..c56fbaf 100644
--- a/repo/includes/EditEntityFactory.php
+++ b/repo/includes/EditEntityFactory.php
@@ -93,12 +93,12 @@
/**
* @param User $user the user performing the edit
* @param EntityId|null $entityId the id of the entity to edit
- * @param int|bool $baseRevId the base revision ID for conflict
checking.
- * Defaults to false, disabling conflict checks.
- * `true` can be used to set the base revision to the latest
revision:
- * This will detect "late" edit conflicts, i.e. someone
squeezing in an edit
- * just before the actual database transaction for saving beings.
- * The empty string and 0 are both treated as `false`, disabling
conflict checks.
+ * @param int $baseRevId the base revision ID for conflict checking.
+ * Use 0 to indicate that the current revision should be used as
the base revision,
+ * effectively disabling conflict detections. true and false
will be accepted for
+ * backwards compatibility, but both will be treated like 0.
Note that the behavior
+ * of EditEntity was changed so that "late" conflicts that arise
between edit conflict
+ * detection and database update are always detected, and result
in the update to fail.
*
* @return EditEntity
*/
diff --git a/repo/includes/Specials/SpecialWikibaseRepoPage.php
b/repo/includes/Specials/SpecialWikibaseRepoPage.php
index 6383a19..6ad3a13 100644
--- a/repo/includes/Specials/SpecialWikibaseRepoPage.php
+++ b/repo/includes/Specials/SpecialWikibaseRepoPage.php
@@ -130,7 +130,7 @@
* @param Summary $summary
* @param string $token
* @param int $flags The edit flags (see WikiPage::doEditContent)
- * @param bool|int $baseRev the base revision, for conflict detection
+ * @param int $baseRev the base revision, for conflict detection
*
* @return Status
*/
@@ -139,7 +139,7 @@
Summary $summary,
$token,
$flags = EDIT_UPDATE,
- $baseRev = false
+ $baseRev = 0
) {
$editEntity = $this->editEntityFactory->newEditEntity(
$this->getUser(),
diff --git a/repo/includes/UpdateRepo/UpdateRepoJob.php
b/repo/includes/UpdateRepo/UpdateRepoJob.php
index 80f9bcf..8cee916 100644
--- a/repo/includes/UpdateRepo/UpdateRepoJob.php
+++ b/repo/includes/UpdateRepo/UpdateRepoJob.php
@@ -153,7 +153,7 @@
$summaryString = $this->summaryFormatter->formatSummary(
$summary );
- $editEntity = $this->editEntityFactory->newEditEntity( $user,
$item->getId(), true );
+ $editEntity = $this->editEntityFactory->newEditEntity( $user,
$item->getId(), 0 );
$status = $editEntity->attemptSave(
$item,
$summaryString,
diff --git a/repo/tests/phpunit/includes/EditEntityTest.php
b/repo/tests/phpunit/includes/EditEntityTest.php
index af46969..84a71b8 100644
--- a/repo/tests/phpunit/includes/EditEntityTest.php
+++ b/repo/tests/phpunit/includes/EditEntityTest.php
@@ -358,17 +358,7 @@
];
}
- public function provideAttemptSaveWithLateConflict() {
- return [
- [ true, true ],
- [ false, false ],
- ];
- }
-
- /**
- * @dataProvider provideAttemptSaveWithLateConflict
- */
- public function testAttemptSaveWithLateConflict( $baseRevId,
$expectedConflict ) {
+ public function testAttemptSaveWithLateConflict() {
$repo = $this->getMockRepository();
$user = $this->getUser( 'EditEntityTestUser' );
@@ -384,10 +374,8 @@
$entity->setLabel( 'en', 'Trust' );
$titleLookup = $this->getEntityTitleLookup();
- $editEntity = $this->makeEditEntity( $repo, $entity->getId(),
$titleLookup, $user, $baseRevId );
+ $editEntity = $this->makeEditEntity( $repo, $entity->getId(),
$titleLookup, $user, true );
$editEntity->getLatestRevision(); // make sure EditEntity has
page and revision
-
- $this->assertEquals( $baseRevId !== false,
$editEntity->doesCheckForEditConflicts(), 'doesCheckForEditConflicts()' );
// create independent Entity instance for the same entity, and
modify and save it
$entity2 = new Item( new ItemId( 'Q42' ) );
@@ -408,16 +396,16 @@
$statusMessage = "Status ($id): " .
$status->getWikiText();
}
- $this->assertNotEquals( $expectedConflict, $status->isOK(),
- "Saving should have failed late if and only if a base
rev was provided.\n$statusMessage" );
+ $this->assertFalse( $status->isOK(),
+ "Saving should have failed late\n$statusMessage" );
- $this->assertEquals( $expectedConflict, $editEntity->hasError(),
- "Saving should have failed late if and only if a base
rev was provided.\n$statusMessage" );
+ $this->assertTrue( $editEntity->hasError(),
+ "Saving should have failed late\n$statusMessage" );
- $this->assertEquals( $expectedConflict, $status->hasMessage(
'edit-conflict' ),
- "Saving should have failed late if and only if a base
rev was provided.\n$statusMessage" );
+ $this->assertTrue( $status->hasMessage( 'edit-conflict' ),
+ "Saving should have failed late\n$statusMessage" );
- $this->assertEquals( $expectedConflict,
$editEntity->showErrorPage(),
+ $this->assertTrue( $editEntity->showErrorPage(),
"If and only if there was an error, an error page
should be shown.\n$statusMessage" );
}
--
To view, visit https://gerrit.wikimedia.org/r/366879
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I514a4acfa4e6ead356a5a7f0480a172f2086e012
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/Wikibase
Gerrit-Branch: master
Gerrit-Owner: Daniel Kinzler <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
