jenkins-bot has submitted this change and it was merged. Change subject: Fix behaviour of EntityRevisionLookup::getLatestRevisionId implementations ......................................................................
Fix behaviour of EntityRevisionLookup::getLatestRevisionId implementations The MockRepository one threw an UnresolvedRedirectException if the entity it was called with is a redirect, but the "real" implementation in WikiPageEntityRevisionLookup doesn't do that. In order to fix this, I changed all implementations to work with redirects in a consistent manner by returning false for them. All callers that relied on the implemetation in MockRepository have been changed to use EntityRedirectLookup. Bug: T101625 Change-Id: I8d05f1fb093216cf30168ae52a27ac8dd53e3d04 --- M lib/includes/store/EntityRedirectLookup.php M lib/includes/store/EntityRevisionLookup.php M lib/includes/store/sql/WikiPageEntityMetaDataLookup.php M lib/includes/store/sql/WikiPageEntityRevisionLookup.php M lib/tests/phpunit/MockRepository.php M repo/includes/Interactors/RedirectCreationInteractor.php M repo/includes/WikibaseRepo.php M repo/includes/api/CreateRedirect.php M repo/includes/store/sql/EntityPerPageTable.php M repo/tests/phpunit/includes/Interactors/RedirectCreationInteractorTest.php M repo/tests/phpunit/includes/api/CreateRedirectTest.php M repo/tests/phpunit/includes/specials/SpecialRedirectEntityTest.php 12 files changed, 56 insertions(+), 28 deletions(-) Approvals: Daniel Kinzler: Looks good to me, approved jenkins-bot: Verified diff --git a/lib/includes/store/EntityRedirectLookup.php b/lib/includes/store/EntityRedirectLookup.php index 8bf2aff..40a128e 100644 --- a/lib/includes/store/EntityRedirectLookup.php +++ b/lib/includes/store/EntityRedirectLookup.php @@ -31,10 +31,12 @@ * @since 0.5 * * @param EntityId $entityId + * @param string $forUpdate If "for update" is given the redirect will be + * determined from the canonical master database. * * @return EntityId|null|false The ID of the redirect target, or null if $entityId * does not refer to a redirect, or false if $entityId is not known. */ - public function getRedirectForEntityId( EntityId $entityId ); + public function getRedirectForEntityId( EntityId $entityId, $forUpdate = '' ); } diff --git a/lib/includes/store/EntityRevisionLookup.php b/lib/includes/store/EntityRevisionLookup.php index b43051b..10763fc 100644 --- a/lib/includes/store/EntityRevisionLookup.php +++ b/lib/includes/store/EntityRevisionLookup.php @@ -59,7 +59,7 @@ * @param string $mode LATEST_FROM_SLAVE or LATEST_FROM_MASTER. LATEST_FROM_MASTER would force the * revision to be determined from the canonical master database. * - * @return int|false + * @return int|false Returns false in case the entity doesn't exist (this includes redirects). */ public function getLatestRevisionId( EntityId $entityId, $mode = self::LATEST_FROM_SLAVE ); diff --git a/lib/includes/store/sql/WikiPageEntityMetaDataLookup.php b/lib/includes/store/sql/WikiPageEntityMetaDataLookup.php index 1841a24..1908291 100644 --- a/lib/includes/store/sql/WikiPageEntityMetaDataLookup.php +++ b/lib/includes/store/sql/WikiPageEntityMetaDataLookup.php @@ -105,6 +105,7 @@ 'rev_content_format', 'rev_timestamp', 'page_latest', + 'page_is_redirect', 'old_id', 'old_text', 'old_flags' diff --git a/lib/includes/store/sql/WikiPageEntityRevisionLookup.php b/lib/includes/store/sql/WikiPageEntityRevisionLookup.php index b60a1fd..9bbbf54 100644 --- a/lib/includes/store/sql/WikiPageEntityRevisionLookup.php +++ b/lib/includes/store/sql/WikiPageEntityRevisionLookup.php @@ -126,7 +126,7 @@ $rows = $this->entityMetaDataAccessor->loadRevisionInformation( array( $entityId ), $mode ); $row = $rows[$entityId->getSerialization()]; - if ( $row && $row->page_latest ) { + if ( $row && $row->page_latest && !$row->page_is_redirect ) { return (int)$row->page_latest; } diff --git a/lib/tests/phpunit/MockRepository.php b/lib/tests/phpunit/MockRepository.php index 7a7d2c0..c2014ea 100644 --- a/lib/tests/phpunit/MockRepository.php +++ b/lib/tests/phpunit/MockRepository.php @@ -575,7 +575,11 @@ * @return int|false */ public function getLatestRevisionId( EntityId $entityId, $mode = self::LATEST_FROM_SLAVE ) { - $revision = $this->getEntityRevision( $entityId, $mode ); + try { + $revision = $this->getEntityRevision( $entityId, $mode ); + } catch ( UnresolvedRedirectException $e ) { + return false; + } return $revision === null ? false : $revision->getRevisionId(); } @@ -834,11 +838,12 @@ * @since 0.5 * * @param EntityId $entityId + * @parma string $forUpdate * * @return EntityId|null|false The ID of the redirect target, or null if $entityId * does not refer to a redirect, or false if $entityId is not known. */ - public function getRedirectForEntityId( EntityId $entityId ) { + public function getRedirectForEntityId( EntityId $entityId, $forUpdate = '' ) { $key = $entityId->getSerialization(); if ( isset( $this->redirects[$key] ) ) { diff --git a/repo/includes/Interactors/RedirectCreationInteractor.php b/repo/includes/Interactors/RedirectCreationInteractor.php index d267d6a..d0f307c 100644 --- a/repo/includes/Interactors/RedirectCreationInteractor.php +++ b/repo/includes/Interactors/RedirectCreationInteractor.php @@ -5,6 +5,7 @@ use User; use Wikibase\DataModel\Entity\EntityId; use Wikibase\Lib\Store\EntityRedirect; +use Wikibase\Lib\Store\EntityRedirectLookup; use Wikibase\Lib\Store\EntityRevisionLookup; use Wikibase\Lib\Store\EntityStore; use Wikibase\Lib\Store\StorageException; @@ -56,12 +57,18 @@ private $editFilterHookRunner; /** + * @var EntityRedirectLookup + */ + private $entityRedirectLookup; + + /** * @param EntityRevisionLookup $entityRevisionLookup * @param EntityStore $entityStore * @param EntityPermissionChecker $permissionChecker * @param SummaryFormatter $summaryFormatter * @param User $user * @param EditFilterHookRunner $editFilterHookRunner + * @param EntityRedirectLookup $entityRedirectLookup */ public function __construct( EntityRevisionLookup $entityRevisionLookup, @@ -69,7 +76,8 @@ EntityPermissionChecker $permissionChecker, SummaryFormatter $summaryFormatter, User $user, - EditFilterHookRunner $editFilterHookRunner + EditFilterHookRunner $editFilterHookRunner, + EntityRedirectLookup $entityRedirectLookup ) { $this->entityRevisionLookup = $entityRevisionLookup; $this->entityStore = $entityStore; @@ -77,6 +85,7 @@ $this->summaryFormatter = $summaryFormatter; $this->user = $user; $this->editFilterHookRunner = $editFilterHookRunner; + $this->entityRedirectLookup = $entityRedirectLookup; } /** @@ -98,7 +107,7 @@ $this->checkCompatible( $fromId, $toId ); $this->checkPermissions( $fromId ); - $this->checkExists( $toId ); + $this->checkExistsNoRedirect( $toId ); $this->checkEmpty( $fromId ); $summary = new Summary( 'wbcreateredirect' ); @@ -182,21 +191,22 @@ * * @throws RedirectCreationException */ - private function checkExists( EntityId $entityId ) { - try { - $revision = $this->entityRevisionLookup->getLatestRevisionId( - $entityId, - EntityRevisionLookup::LATEST_FROM_MASTER - ); + private function checkExistsNoRedirect( EntityId $entityId ) { + $redirect = $this->entityRedirectLookup->getRedirectForEntityId( + $entityId, + 'for update' + ); - if ( !$revision ) { - throw new RedirectCreationException( - "Entity $entityId not found", - 'no-such-entity' - ); - } - } catch ( UnresolvedRedirectException $ex ) { - throw new RedirectCreationException( $ex->getMessage(), 'target-is-redirect', $ex ); + if ( $redirect === false ) { + throw new RedirectCreationException( + "Entity $entityId not found", + 'no-such-entity' + ); + } elseif ( $redirect !== null ) { + throw new RedirectCreationException( + "Entity $entityId is a redirect", + 'target-is-redirect' + ); } } diff --git a/repo/includes/WikibaseRepo.php b/repo/includes/WikibaseRepo.php index dd20415..1dbb55a 100644 --- a/repo/includes/WikibaseRepo.php +++ b/repo/includes/WikibaseRepo.php @@ -324,7 +324,8 @@ $this->getEntityTitleLookup(), $this->getEntityContentFactory(), $context - ) + ), + $this->getStore()->getEntityRedirectLookup() ); } diff --git a/repo/includes/api/CreateRedirect.php b/repo/includes/api/CreateRedirect.php index 169dd63..dc94479 100644 --- a/repo/includes/api/CreateRedirect.php +++ b/repo/includes/api/CreateRedirect.php @@ -62,7 +62,8 @@ WikibaseRepo::getDefaultInstance()->getEntityTitleLookup(), WikibaseRepo::getDefaultInstance()->getEntityContentFactory(), $this->getContext() - ) + ), + WikibaseRepo::getDefaultInstance()->getStore()->getEntityRedirectLookup() ) ); } diff --git a/repo/includes/store/sql/EntityPerPageTable.php b/repo/includes/store/sql/EntityPerPageTable.php index 8de0214..2bc4b18 100644 --- a/repo/includes/store/sql/EntityPerPageTable.php +++ b/repo/includes/store/sql/EntityPerPageTable.php @@ -475,21 +475,26 @@ } /** + * @see EntityRedirectLookup::getRedirectForEntityId + * * @since 0.5 * * @param EntityId $entityId + * @paran string $forUpdate * * @return EntityId|null|false The ID of the redirect target, or null if $entityId * does not refer to a redirect, or false if $entityId is not known. */ - public function getRedirectForEntityId( EntityId $entityId ) { + public function getRedirectForEntityId( EntityId $entityId, $forUpdate = '' ) { // Even if we don't have the redirect column, we still want to // check whether the entry is there at all. $redirectColumn = $this->useRedirectTargetColumn ? 'epp_redirect_target' : 'NULL AS epp_redirect_target'; - $dbr = wfGetDB( DB_SLAVE ); + $dbr = wfGetDB( + $forUpdate === 'for update' ? DB_MASTER : DB_SLAVE + ); $row = $dbr->selectRow( 'wb_entity_per_page', diff --git a/repo/tests/phpunit/includes/Interactors/RedirectCreationInteractorTest.php b/repo/tests/phpunit/includes/Interactors/RedirectCreationInteractorTest.php index 6ff4b44..0b3d4a2 100644 --- a/repo/tests/phpunit/includes/Interactors/RedirectCreationInteractorTest.php +++ b/repo/tests/phpunit/includes/Interactors/RedirectCreationInteractorTest.php @@ -140,7 +140,8 @@ $this->getPermissionCheckers(), $summaryFormatter, $user, - $this->getMockEditFilterHookRunner( $efHookCalls, $efHookStatus ) + $this->getMockEditFilterHookRunner( $efHookCalls, $efHookStatus ), + $this->mockRepository ); return $interactor; diff --git a/repo/tests/phpunit/includes/api/CreateRedirectTest.php b/repo/tests/phpunit/includes/api/CreateRedirectTest.php index 11d0c0d..9dfae7b 100644 --- a/repo/tests/phpunit/includes/api/CreateRedirectTest.php +++ b/repo/tests/phpunit/includes/api/CreateRedirectTest.php @@ -135,7 +135,8 @@ $this->getPermissionCheckers(), $summaryFormatter, $user, - $this->getMockEditFilterHookRunner() + $this->getMockEditFilterHookRunner(), + $this->mockRepository ) ); diff --git a/repo/tests/phpunit/includes/specials/SpecialRedirectEntityTest.php b/repo/tests/phpunit/includes/specials/SpecialRedirectEntityTest.php index 2a5aba7..99f2de0 100644 --- a/repo/tests/phpunit/includes/specials/SpecialRedirectEntityTest.php +++ b/repo/tests/phpunit/includes/specials/SpecialRedirectEntityTest.php @@ -120,7 +120,8 @@ $this->getPermissionCheckers(), $summaryFormatter, $user, - $this->getMockEditFilterHookRunner() + $this->getMockEditFilterHookRunner(), + $this->mockRepository ) ); } -- To view, visit https://gerrit.wikimedia.org/r/216514 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: I8d05f1fb093216cf30168ae52a27ac8dd53e3d04 Gerrit-PatchSet: 4 Gerrit-Project: mediawiki/extensions/Wikibase Gerrit-Branch: master Gerrit-Owner: Hoo man <h...@online.de> Gerrit-Reviewer: Addshore <addshorew...@gmail.com> Gerrit-Reviewer: Aude <aude.w...@gmail.com> Gerrit-Reviewer: Daniel Kinzler <daniel.kinz...@wikimedia.de> Gerrit-Reviewer: Hoo man <h...@online.de> Gerrit-Reviewer: JanZerebecki <jan.wikime...@zerebecki.de> Gerrit-Reviewer: Lucie Kaffee <lucie.kaf...@wikimedia.de> Gerrit-Reviewer: jenkins-bot <> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits