jenkins-bot has submitted this change and it was merged.
Change subject: Generic handling of baserevid for EntitySavingHelper
......................................................................
Generic handling of baserevid for EntitySavingHelper
The same baserevid should be used during loading and saving.
Consolidating this idea into a single class helper to avoid confusion.
Bug: T140760
Change-Id: I5618c6bede4dce927522c7ec5294a310e0e24757
---
M repo/includes/Api/CreateClaim.php
M repo/includes/Api/EntityLoadingHelper.php
M repo/includes/Api/EntitySavingHelper.php
M repo/includes/Api/GetClaims.php
M repo/includes/Api/RemoveClaims.php
M repo/includes/Api/RemoveQualifiers.php
M repo/includes/Api/RemoveReferences.php
M repo/includes/Api/SetClaim.php
M repo/includes/Api/SetClaimValue.php
M repo/includes/Api/SetQualifier.php
M repo/includes/Api/SetReference.php
M repo/tests/phpunit/includes/Api/EntityLoadingHelperTest.php
M repo/tests/phpunit/includes/Api/EntitySavingHelperTest.php
13 files changed, 135 insertions(+), 96 deletions(-)
Approvals:
Hoo man: Looks good to me, but someone else must approve
Thiemo Mättig (WMDE): Looks good to me, approved
jenkins-bot: Verified
diff --git a/repo/includes/Api/CreateClaim.php
b/repo/includes/Api/CreateClaim.php
index 4e27490..6c5d237 100644
--- a/repo/includes/Api/CreateClaim.php
+++ b/repo/includes/Api/CreateClaim.php
@@ -82,15 +82,7 @@
$this->validateParameters( $params );
$entityId = $this->modificationHelper->getEntityIdFromString(
$params['entity'] );
- if ( isset( $params['baserevid'] ) ) {
- $entityRevision =
$this->entitySavingHelper->loadEntityRevision(
- $entityId,
- (int)$params['baserevid']
- );
- } else {
- $entityRevision =
$this->entitySavingHelper->loadEntityRevision( $entityId );
- }
- $entity = $entityRevision->getEntity();
+ $entity = $this->entitySavingHelper->loadEntity( $entityId );
$propertyId = $this->modificationHelper->getEntityIdFromString(
$params['property'] );
if ( !( $propertyId instanceof PropertyId ) ) {
diff --git a/repo/includes/Api/EntityLoadingHelper.php
b/repo/includes/Api/EntityLoadingHelper.php
index bf239f1..35ba2e4 100644
--- a/repo/includes/Api/EntityLoadingHelper.php
+++ b/repo/includes/Api/EntityLoadingHelper.php
@@ -2,14 +2,17 @@
namespace Wikibase\Repo\Api;
+use ApiBase;
use LogicException;
use UsageException;
+use Wikibase\DataModel\Entity\EntityDocument;
use Wikibase\DataModel\Entity\EntityId;
use Wikibase\EntityRevision;
use Wikibase\Lib\Store\BadRevisionException;
use Wikibase\Lib\Store\EntityRevisionLookup;
use Wikibase\Lib\Store\StorageException;
use Wikibase\Lib\Store\RevisionedUnresolvedRedirectException;
+use Wikimedia\Assert\Assert;
/**
* Helper class for api modules to load entities.
@@ -31,12 +34,33 @@
*/
protected $errorReporter;
+ /**
+ * @var string See the LATEST_XXX constants defined in
EntityRevisionLookup
+ */
+ protected $defaultRetrievalMode =
EntityRevisionLookup::LATEST_FROM_SLAVE;
+
public function __construct(
EntityRevisionLookup $entityRevisionLookup,
ApiErrorReporter $errorReporter
) {
$this->entityRevisionLookup = $entityRevisionLookup;
$this->errorReporter = $errorReporter;
+ }
+
+ /**
+ * @return string
+ */
+ public function getDefaultRetrievalMode() {
+ return $this->defaultRetrievalMode;
+ }
+
+ /**
+ * @param string $defaultRetrievalMode Use the LATEST_XXX constants
defined
+ * in EntityRevisionLookup
+ */
+ public function setDefaultRetrievalMode( $defaultRetrievalMode ) {
+ Assert::parameterType( 'string', $defaultRetrievalMode,
'$defaultRetrievalMode' );
+ $this->defaultRetrievalMode = $defaultRetrievalMode;
}
/**
@@ -48,16 +72,22 @@
* @since 0.5
*
* @param EntityId $entityId EntityId of the page to load the revision
for
- * @param int|string $revId revision to load. If not given, the current
revision will be loaded.
+ * @param int|string|null $revId revision to load, or the retrieval
mode,
+ * see the LATEST_XXX constants defined in EntityRevisionLookup.
+ * If not given, the current revision will be loaded, using the
default retrieval mode.
*
* @throws UsageException
* @throws LogicException
* @return EntityRevision
*/
- public function loadEntityRevision(
+ protected function loadEntityRevision(
EntityId $entityId,
- $revId = EntityRevisionLookup::LATEST_FROM_MASTER
+ $revId = null
) {
+ if ( $revId === null ) {
+ $revId = $this->defaultRetrievalMode;
+ }
+
try {
$revision =
$this->entityRevisionLookup->getEntityRevision( $entityId, $revId );
@@ -79,4 +109,13 @@
throw new LogicException( 'ApiErrorReporter::dieException did
not throw a UsageException' );
}
+ /**
+ * @param EntityId $entityId
+ * @return EntityDocument
+ */
+ public function loadEntity( EntityId $entityId ) {
+ $entityRevision = $this->loadEntityRevision( $entityId );
+ return $entityRevision->getEntity();
+ }
+
}
diff --git a/repo/includes/Api/EntitySavingHelper.php
b/repo/includes/Api/EntitySavingHelper.php
index 1d4eaea..a828a74 100644
--- a/repo/includes/Api/EntitySavingHelper.php
+++ b/repo/includes/Api/EntitySavingHelper.php
@@ -7,6 +7,7 @@
use Status;
use UsageException;
use Wikibase\DataModel\Entity\EntityDocument;
+use Wikibase\DataModel\Entity\EntityId;
use Wikibase\EditEntity as EditEntityHandler;
use Wikibase\EditEntityFactory;
use Wikibase\Lib\Store\EntityRevisionLookup;
@@ -49,6 +50,27 @@
$this->apiBase = $apiBase;
$this->summaryFormatter = $summaryFormatter;
$this->editEntityFactory = $editEntityFactory;
+
+ $this->defaultRetrievalMode =
EntityRevisionLookup::LATEST_FROM_MASTER;
+ }
+
+ /**
+ * Returns the given EntityDocument.
+ *
+ * @param EntityId $entityId
+ * @return EntityDocument
+ */
+ public function loadEntity( EntityId $entityId ) {
+ $params = $this->apiBase->extractRequestParams();
+
+ // If a base revision is given, use if for consistency!
+ $baseRev = isset( $params['baserevid'] )
+ ? (int)$params['baserevid']
+ : $this->defaultRetrievalMode;
+
+ $entityRevision = $this->loadEntityRevision( $entityId,
$baseRev );
+
+ return $entityRevision->getEntity();
}
/**
diff --git a/repo/includes/Api/GetClaims.php b/repo/includes/Api/GetClaims.php
index 1b2fc61..04270a9 100644
--- a/repo/includes/Api/GetClaims.php
+++ b/repo/includes/Api/GetClaims.php
@@ -96,11 +96,7 @@
}
/** @var EntityId $entityId */
- $entityRevision =
$this->entityLoadingHelper->loadEntityRevision(
- $entityId,
- EntityRevisionLookup::LATEST_FROM_SLAVE
- );
- $entity = $entityRevision->getEntity();
+ $entity = $this->entityLoadingHelper->loadEntity( $entityId );
$statements = $this->getStatements( $entity, $guid );
$this->resultBuilder->addStatements( $statements, null,
$params['props'] );
diff --git a/repo/includes/Api/RemoveClaims.php
b/repo/includes/Api/RemoveClaims.php
index 305f447..5e3ccee 100644
--- a/repo/includes/Api/RemoveClaims.php
+++ b/repo/includes/Api/RemoveClaims.php
@@ -91,15 +91,7 @@
public function execute() {
$params = $this->extractRequestParams();
$entityId = $this->getEntityId( $params );
- if ( isset( $params['baserevid'] ) ) {
- $entityRevision =
$this->entitySavingHelper->loadEntityRevision(
- $entityId,
- (int)$params['baserevid']
- );
- } else {
- $entityRevision =
$this->entitySavingHelper->loadEntityRevision( $entityId );
- }
- $entity = $entityRevision->getEntity();
+ $entity = $this->entitySavingHelper->loadEntity( $entityId );
if ( $entity instanceof StatementListProvider ) {
$this->assertStatementListContainsGuids(
$entity->getStatements(), $params['claim'] );
diff --git a/repo/includes/Api/RemoveQualifiers.php
b/repo/includes/Api/RemoveQualifiers.php
index 9b89e1c..780c526 100644
--- a/repo/includes/Api/RemoveQualifiers.php
+++ b/repo/includes/Api/RemoveQualifiers.php
@@ -91,15 +91,8 @@
$guid = $params['claim'];
$entityId = $this->guidParser->parse( $guid )->getEntityId();
- if ( isset( $params['baserevid'] ) ) {
- $entityRevision =
$this->entitySavingHelper->loadEntityRevision(
- $entityId,
- (int)$params['baserevid']
- );
- } else {
- $entityRevision =
$this->entitySavingHelper->loadEntityRevision( $entityId );
- }
- $entity = $entityRevision->getEntity();
+ $entity = $this->entitySavingHelper->loadEntity( $entityId );
+
$summary = $this->modificationHelper->createSummary( $params,
$this );
$claim = $this->modificationHelper->getStatementFromEntity(
$guid, $entity );
diff --git a/repo/includes/Api/RemoveReferences.php
b/repo/includes/Api/RemoveReferences.php
index c3685b2..b4597d1 100644
--- a/repo/includes/Api/RemoveReferences.php
+++ b/repo/includes/Api/RemoveReferences.php
@@ -91,15 +91,7 @@
$guid = $params['statement'];
$entityId = $this->guidParser->parse( $guid )->getEntityId();
- if ( isset( $params['baserevid'] ) ) {
- $entityRevision =
$this->entitySavingHelper->loadEntityRevision(
- $entityId,
- (int)$params['baserevid']
- );
- } else {
- $entityRevision =
$this->entitySavingHelper->loadEntityRevision( $entityId );
- }
- $entity = $entityRevision->getEntity();
+ $entity = $this->entitySavingHelper->loadEntity( $entityId );
$summary = $this->modificationHelper->createSummary( $params,
$this );
$claim = $this->modificationHelper->getStatementFromEntity(
$guid, $entity );
diff --git a/repo/includes/Api/SetClaim.php b/repo/includes/Api/SetClaim.php
index 85b38f8..7bbd68d 100644
--- a/repo/includes/Api/SetClaim.php
+++ b/repo/includes/Api/SetClaim.php
@@ -121,15 +121,7 @@
}
$entityId = $statementGuid->getEntityId();
- if ( isset( $params['baserevid'] ) ) {
- $entityRevision =
$this->entitySavingHelper->loadEntityRevision(
- $entityId,
- (int)$params['baserevid']
- );
- } else {
- $entityRevision =
$this->entitySavingHelper->loadEntityRevision( $entityId );
- }
- $entity = $entityRevision->getEntity();
+ $entity = $this->entitySavingHelper->loadEntity( $entityId );
if ( !( $entity instanceof StatementListProvider ) ) {
$this->errorReporter->dieError( 'The given entity
cannot contain statements', 'not-supported' );
diff --git a/repo/includes/Api/SetClaimValue.php
b/repo/includes/Api/SetClaimValue.php
index 73d9859..3f92a43 100644
--- a/repo/includes/Api/SetClaimValue.php
+++ b/repo/includes/Api/SetClaimValue.php
@@ -89,15 +89,7 @@
$guid = $params['claim'];
$entityId = $this->guidParser->parse( $guid )->getEntityId();
- if ( isset( $params['baserevid'] ) ) {
- $entityRevision =
$this->entitySavingHelper->loadEntityRevision(
- $entityId,
- (int)$params['baserevid']
- );
- } else {
- $entityRevision =
$this->entitySavingHelper->loadEntityRevision( $entityId );
- }
- $entity = $entityRevision->getEntity();
+ $entity = $this->entitySavingHelper->loadEntity( $entityId );
$claim = $this->modificationHelper->getStatementFromEntity(
$guid, $entity );
diff --git a/repo/includes/Api/SetQualifier.php
b/repo/includes/Api/SetQualifier.php
index c786e0f..36625b4 100644
--- a/repo/includes/Api/SetQualifier.php
+++ b/repo/includes/Api/SetQualifier.php
@@ -90,15 +90,7 @@
$this->validateParameters( $params );
$entityId = $this->guidParser->parse( $params['claim']
)->getEntityId();
- if ( isset( $params['baserevid'] ) ) {
- $entityRevision =
$this->entitySavingHelper->loadEntityRevision(
- $entityId,
- (int)$params['baserevid']
- );
- } else {
- $entityRevision =
$this->entitySavingHelper->loadEntityRevision( $entityId );
- }
- $entity = $entityRevision->getEntity();
+ $entity = $this->entitySavingHelper->loadEntity( $entityId );
$summary = $this->modificationHelper->createSummary( $params,
$this );
diff --git a/repo/includes/Api/SetReference.php
b/repo/includes/Api/SetReference.php
index bdb9657..6ed8c49 100644
--- a/repo/includes/Api/SetReference.php
+++ b/repo/includes/Api/SetReference.php
@@ -98,15 +98,7 @@
$this->validateParameters( $params );
$entityId = $this->guidParser->parse( $params['statement']
)->getEntityId();
- if ( isset( $params['baserevid'] ) ) {
- $entityRevision =
$this->entitySavingHelper->loadEntityRevision(
- $entityId,
- (int)$params['baserevid']
- );
- } else {
- $entityRevision =
$this->entitySavingHelper->loadEntityRevision( $entityId );
- }
- $entity = $entityRevision->getEntity();
+ $entity = $this->entitySavingHelper->loadEntity( $entityId );
$summary = $this->modificationHelper->createSummary( $params,
$this );
diff --git a/repo/tests/phpunit/includes/Api/EntityLoadingHelperTest.php
b/repo/tests/phpunit/includes/Api/EntityLoadingHelperTest.php
index aa19333..b0cfd7b 100644
--- a/repo/tests/phpunit/includes/Api/EntityLoadingHelperTest.php
+++ b/repo/tests/phpunit/includes/Api/EntityLoadingHelperTest.php
@@ -3,8 +3,8 @@
namespace Wikibase\Test\Repo\Api;
use Exception;
-use Revision;
use UsageException;
+use Wikibase\DataModel\Entity\EntityDocument;
use Wikibase\DataModel\Entity\ItemId;
use Wikibase\EntityRevision;
use Wikibase\Lib\Store\BadRevisionException;
@@ -43,7 +43,7 @@
$mock->expects( $this->once() )
->method( 'getEntityRevision' )
->will( $this->throwException(
$entityRevisionReturn ) );
- } else {
+ } elseif ( $entityRevisionReturn instanceof EntityRevision ) {
$mock->expects( $this->once() )
->method( 'getEntityRevision' )
->will( $this->returnValue(
$entityRevisionReturn ) );
@@ -76,12 +76,20 @@
}
/**
- * @return Revision
+ * @return EntityRevision
*/
protected function getMockRevision() {
- return $this->getMockBuilder( Revision::class )
+ $entity = $this->getMock( EntityDocument::class );
+
+ $revision = $this->getMockBuilder( EntityRevision::class )
->disableOriginalConstructor()
->getMock();
+
+ $revision->expects( $this->any() )
+ ->method( 'getEntity' )
+ ->will( $this->returnValue( $entity ) );
+
+ return $revision;
}
/**
@@ -100,23 +108,25 @@
);
}
- public function testRevision_returnsRevision() {
+ public function testLoadEntity() {
$revision = $this->getMockRevision();
+ $entity = $revision->getEntity();
+
$helper = $this->newEntityLoadingHelper( $revision );
- $return = $helper->loadEntityRevision( new ItemId( 'Q1' ) );
+ $return = $helper->loadEntity( new ItemId( 'Q1' ) );
- $this->assertSame( $revision, $return );
+ $this->assertSame( $entity, $return );
}
- public function testNullRevision_callsErrorReporter() {
+ public function testLoadEntity_NullRevision() {
$helper = $this->newEntityLoadingHelper( null, null,
'cant-load-entity-content' );
$this->setExpectedException( UsageException::class );
- $helper->loadEntityRevision( new ItemId( 'Q1' ) );
+ $helper->loadEntity( new ItemId( 'Q1' ) );
}
- public function testUnresolvedRedirectException_callsErrorReporter() {
+ public function testLoadEntity_UnresolvedRedirectException() {
$helper = $this->newEntityLoadingHelper(
new RevisionedUnresolvedRedirectException(
new ItemId( 'Q1' ),
@@ -126,21 +136,21 @@
);
$this->setExpectedException( UsageException::class );
- $helper->loadEntityRevision( new ItemId( 'Q1' ) );
+ $helper->loadEntity( new ItemId( 'Q1' ) );
}
- public function testBadRevisionException_callsErrorReporter() {
+ public function testLoadEntity_BadRevisionException() {
$helper = $this->newEntityLoadingHelper( new
BadRevisionException(), 'nosuchrevid' );
$this->setExpectedException( UsageException::class );
- $helper->loadEntityRevision( new ItemId( 'Q1' ) );
+ $helper->loadEntity( new ItemId( 'Q1' ) );
}
- public function testStorageException_callsErrorReporter() {
+ public function testLoadEntity_StorageException() {
$helper = $this->newEntityLoadingHelper( new
StorageException(), 'cant-load-entity-content' );
$this->setExpectedException( UsageException::class );
- $helper->loadEntityRevision( new ItemId( 'Q1' ) );
+ $helper->loadEntity( new ItemId( 'Q1' ) );
}
}
diff --git a/repo/tests/phpunit/includes/Api/EntitySavingHelperTest.php
b/repo/tests/phpunit/includes/Api/EntitySavingHelperTest.php
index 7115a9a..a1de57f 100644
--- a/repo/tests/phpunit/includes/Api/EntitySavingHelperTest.php
+++ b/repo/tests/phpunit/includes/Api/EntitySavingHelperTest.php
@@ -16,7 +16,6 @@
use Wikibase\EditEntity;
use Wikibase\EditEntityFactory;
use Wikibase\EntityRevision;
-use Wikibase\Repo\Api\ApiErrorReporter;
use Wikibase\Repo\Api\EntityLoadingHelper;
use Wikibase\Repo\Api\EntitySavingHelper;
use Wikibase\SummaryFormatter;
@@ -92,6 +91,42 @@
return $context;
}
+ public function testLoadEntity_baserevid() {
+ $itemId = new ItemId( 'Q1' );
+
+ $revision = $this->getMockRevision();
+ $entity = $revision->getEntity();
+
+ $mockApiBase = $this->getMockApiBase();
+ $mockApiBase->expects( $this->any() )
+ ->method( 'isWriteMode' )
+ ->will( $this->returnValue( true ) );
+ $mockApiBase->expects( $this->any() )
+ ->method( 'getContext' )
+ ->will( $this->returnValue( $this->newContext() ) );
+ $mockApiBase->expects( $this->any() )
+ ->method( 'extractRequestParams' )
+ ->will( $this->returnValue( array( 'baserevid' => 17 )
) );
+
+ $revisionLookup = $this->getMockEntityRevisionLookup( true );
+ $revisionLookup->expects( $this->once() )
+ ->method( 'getEntityRevision' )
+ ->with( $itemId, 17 )
+ ->will( $this->returnValue( $revision ) );
+
+ $helper = new EntitySavingHelper(
+ $mockApiBase,
+ $revisionLookup,
+ $this->getMockErrorReporter(),
+ $this->getMockSummaryFormatter(),
+ $this->getMockEditEntityFactory()
+ );
+
+ $return = $helper->loadEntity( $itemId );
+
+ $this->assertSame( $entity, $return );
+ }
+
public function testAttemptSave() {
$mockApiBase = $this->getMockApiBase();
$mockApiBase->expects( $this->once() )
--
To view, visit https://gerrit.wikimedia.org/r/303600
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I5618c6bede4dce927522c7ec5294a310e0e24757
Gerrit-PatchSet: 3
Gerrit-Project: mediawiki/extensions/Wikibase
Gerrit-Branch: master
Gerrit-Owner: Daniel Kinzler <[email protected]>
Gerrit-Reviewer: Addshore <[email protected]>
Gerrit-Reviewer: Aude <[email protected]>
Gerrit-Reviewer: Daniel Kinzler <[email protected]>
Gerrit-Reviewer: Hoo man <[email protected]>
Gerrit-Reviewer: Jonas Kress (WMDE) <[email protected]>
Gerrit-Reviewer: Thiemo Mättig (WMDE) <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
