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
