Daniel Kinzler has uploaded a new change for review.
https://gerrit.wikimedia.org/r/67251
Change subject: EntityLookup should fail on bad revision
......................................................................
EntityLookup should fail on bad revision
This makes EntityLookup be stricter about bad revision IDs.
Change-Id: I37a5fecc0ee22fc92c28a8fcd1810a4559767db1
---
M client/includes/store/sql/EntityCacheTable.php
M lib/WikibaseLib.classes.php
M lib/includes/store/EntityLookup.php
A lib/includes/store/StorageException.php
M lib/includes/store/sql/CachingEntityLoader.php
M lib/includes/store/sql/WikiPageEntityLookup.php
M lib/tests/phpunit/EntityLookupTest.php
M lib/tests/phpunit/MockRepository.php
M lib/tests/phpunit/store/CachingEntityLoaderTest.php
M lib/tests/phpunit/store/WikiPageEntityLookupTest.php
10 files changed, 180 insertions(+), 124 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Wikibase
refs/changes/51/67251/1
diff --git a/client/includes/store/sql/EntityCacheTable.php
b/client/includes/store/sql/EntityCacheTable.php
index 7c5f943..019742a 100644
--- a/client/includes/store/sql/EntityCacheTable.php
+++ b/client/includes/store/sql/EntityCacheTable.php
@@ -183,22 +183,23 @@
}
/**
- * @see EntityCache::getEntity
- *
* @since 0.1
+ * @see EntityLookup::getEntity
*
- * @param EntityId $entityId The entity's ID
- * @param bool|int $revision The desired Revision
+ * @param EntityID $entityId
+ * @param int $revision The desired revision id, 0 means "current".
*
- * @return null|Entity
+ * @return Entity|null
+ *
+ * @throw StorageException
*/
- public function getEntity( EntityID $entityId, $revision = false ) {
+ public function getEntity( EntityID $entityId, $revision = 0 ) {
$where = array(
'entity_type' => $entityId->getEntityType(),
'entity_id' => $entityId->getNumericId(),
);
- if ( $revision !== false ) {
+ if ( $revision !== 0 ) {
//FIXME: this field does not yet exist in the database!
$where['entity_revision'] = $revision;
}
diff --git a/lib/WikibaseLib.classes.php b/lib/WikibaseLib.classes.php
index 302096b..3e779da 100644
--- a/lib/WikibaseLib.classes.php
+++ b/lib/WikibaseLib.classes.php
@@ -118,6 +118,7 @@
'Wikibase\EntityUsageIndex' =>
'includes/store/EntityUsageIndex.php',
'Wikibase\SiteLinkCache' => 'includes/store/SiteLinkCache.php',
'Wikibase\SiteLinkLookup' =>
'includes/store/SiteLinkLookup.php',
+ 'Wikibase\StorageException' =>
'includes/store/StorageException.php',
'Wikibase\TermIndex' => 'includes/store/TermIndex.php',
'Wikibase\TermCombinationMatchFinder' =>
'includes/store/TermCombinationMatchFinder.php',
'Wikibase\TermMatchScoreCalculator' =>
'includes/store/TermMatchScoreCalculator.php',
diff --git a/lib/includes/store/EntityLookup.php
b/lib/includes/store/EntityLookup.php
index f61b21b..48c0ca5 100644
--- a/lib/includes/store/EntityLookup.php
+++ b/lib/includes/store/EntityLookup.php
@@ -34,16 +34,19 @@
/**
* Returns the entity with the provided id or null if there is no such
* entity. If a $revision is given, the requested revision of the
entity is loaded.
- * If the revision does not belong to the given entity, null is
returned.
+ * If that revision does not exist or does not belong to the given
entity,
+ * an exception is thrown.
*
* @since 0.3
*
* @param EntityID $entityId
- * @param int|bool $revision
+ * @param int $revision The desired revision id, 0 means "current".
*
* @return Entity|null
+ *
+ * @throw StorageException
*/
- public function getEntity( EntityID $entityId, $revision = false );
+ public function getEntity( EntityID $entityId, $revision = 0 );
/**
* Fetches the entities with provided ids and returns them.
diff --git a/lib/includes/store/StorageException.php
b/lib/includes/store/StorageException.php
new file mode 100644
index 0000000..f27d0a6
--- /dev/null
+++ b/lib/includes/store/StorageException.php
@@ -0,0 +1,38 @@
+<?php
+ /**
+ *
+ * Copyright © 06.06.13 by the authors listed below.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ * http://www.gnu.org/copyleft/gpl.html
+ *
+ * @license GPL 2+
+ * @file
+ *
+ * @author Daniel Kinzler
+ */
+
+
+namespace Wikibase;
+
+
+/**
+ * Class StorageException
+ * @package Wikibase
+ */
+
+class StorageException extends \MWException {
+
+}
\ No newline at end of file
diff --git a/lib/includes/store/sql/CachingEntityLoader.php
b/lib/includes/store/sql/CachingEntityLoader.php
index 362a49e..3412ce6 100644
--- a/lib/includes/store/sql/CachingEntityLoader.php
+++ b/lib/includes/store/sql/CachingEntityLoader.php
@@ -60,7 +60,7 @@
*
* @return string
*/
- protected function getCacheKey( $entityId, $revision = false ) {
+ protected function getCacheKey( $entityId, $revision = 0 ) {
$key = $entityId->getPrefixedId();
if ( $revision ) {
@@ -71,16 +71,17 @@
}
/**
- * @see EntityLookup::getEntity
- *
* @since 0.4
+ * @see EntityLookup::getEntity
*
- * @param EntityId $entityId
- * @param boolean|int $revision
+ * @param EntityID $entityId
+ * @param int $revision The desired revision id, 0 means "current".
*
* @return Entity|null
+ *
+ * @throw StorageException
*/
- public function getEntity( EntityId $entityId, $revision = false ) {
+ public function getEntity( EntityId $entityId, $revision = 0 ) {
wfProfileIn( __METHOD__ );
$key = $this->getCacheKey( $entityId, $revision );
@@ -94,16 +95,16 @@
}
/**
- * @see EntityLookup::getEntities
+ * @see EntityLookup::getEntities
*
* @since 0.4
*
* @param array $entityIds
- * @param array|bool $revision
*
* @return Entity|null[]
+ * @throws \MWException
*/
- public function getEntities( array $entityIds, $revision = false ) {
+ public function getEntities( array $entityIds ) {
wfProfileIn( __METHOD__ );
$loaded = array();
diff --git a/lib/includes/store/sql/WikiPageEntityLookup.php
b/lib/includes/store/sql/WikiPageEntityLookup.php
index aac9929..2964e02 100644
--- a/lib/includes/store/sql/WikiPageEntityLookup.php
+++ b/lib/includes/store/sql/WikiPageEntityLookup.php
@@ -2,6 +2,7 @@
namespace Wikibase;
use MWException;
+use Wikibase\Lib\EntityRetrievingDataTypeLookup;
/**
* Implements an entity repo based on blobs stored in wiki pages on a locally
reachable
@@ -112,10 +113,15 @@
*
* @return Entity|null
*/
- public function getEntity( EntityID $entityId, $revision = false ) {
+ public function getEntity( EntityID $entityId, $revision = 0 ) {
wfProfileIn( __METHOD__ );
wfDebugLog( __CLASS__, __FUNCTION__ . ": Looking up entity " .
$entityId->getPrefixedId()
- . " (rev $revision)" );
+ . " (rev $revision)" );
+
+ if ( $revision === false ) { // default changed from false to 0
+ wfWarn( 'getEntity() called with $revision = false, use
0 instead.' );
+ $revision = 0;
+ }
$cache = null;
$cacheKey = false;
@@ -161,7 +167,7 @@
$where = array();
$join = array();
- if ( $revision ) {
+ if ( $revision > 0 ) {
// pick revision by id
$where['rev_id'] = $revision;
@@ -219,6 +225,20 @@
$this->releaseConnection( $db );
+ if ( $entity && !$entityId->equals( $entity->getId() ) ) {
+ // This can happen when giving a revision ID that
doesn't belong to the given entity
+ wfDebugLog( __CLASS__, __FUNCTION__ . ": Loaded wrong
entity: expected " . $entityId
+ . ", got " . $entity->getId());
+
+ $entity = null;
+ }
+
+ if ( $entity === null && $revision > 0 ) {
+ // If a revision was specified, that revision doesn't
exist or doesn't belong to
+ // the given entity. Throw an error.
+ throw new StorageException( "No such revision found for
$entityId: $revision" );
+ }
+
// cacheable if it's the latest revision.
if ( $cache && $row && $entity
&& $row->page_latest === $row->rev_id ) {
@@ -246,26 +266,16 @@
* @since 0.4
*
* @param EntityID[] $entityIds
- * @param array|bool $revision
*
* @return Entity|null[]
*/
- public function getEntities( array $entityIds, $revision = false ) {
+ public function getEntities( array $entityIds ) {
$entities = array();
// TODO: we really want batch lookup here :)
foreach ( $entityIds as $key => $entityId ) {
- $rev = $revision;
- if ( is_array( $rev ) ) {
- if ( !array_key_exists( $key, $rev ) ) {
- throw new MWException( '$entityId has
no revision specified' );
- }
-
- $rev = $rev[$key];
- }
-
- $entities[$entityId->getPrefixedId()] =
$this->getEntity( $entityId, $rev );
+ $entities[$entityId->getPrefixedId()] =
$this->getEntity( $entityId );
}
return $entities;
@@ -305,7 +315,7 @@
$entity = EntityFactory::singleton()->newFromBlob( $entityType,
$blob, $format );
wfDebugLog( __CLASS__, __FUNCTION__ . ": Created entity object
from revision blob: "
- . $entity->getId()->getPrefixedId() );
+ . $entity->getId() );
wfProfileOut( __METHOD__ );
return $entity;
diff --git a/lib/tests/phpunit/EntityLookupTest.php
b/lib/tests/phpunit/EntityLookupTest.php
index caaaef2..c2b4ae5 100644
--- a/lib/tests/phpunit/EntityLookupTest.php
+++ b/lib/tests/phpunit/EntityLookupTest.php
@@ -58,14 +58,21 @@
if ( $entities === null ) {
$item = Item::newEmpty();
$item->setId( 42 );
- $entities[$item->getPrefixedId()] = $item;
+
+ $entities[1] = $item;
+
+ $item = $item->copy();
+ $item->setLabel( 'en', "Foo" );
+
+ $entities[2] = $item;
$dtf = $factory = new DataTypeFactory(
$GLOBALS['wgDataTypes'] );
$prop = Property::newEmpty();
$prop->setId( 753 );
$prop->setDataType( $dtf->getType( "string" ) );
- $entities[$prop->getPrefixedId()] = $prop;
+
+ $entities[3] = $prop;
}
return $entities;
@@ -78,16 +85,32 @@
return $lookup;
}
+ protected function resolveLogicalRevision( $revision ) {
+ return $revision;
+ }
+
public static function provideGetEntity() {
$cases = array(
- array( // #0
- 'q42', false, true,
+ array( // #0: any revision
+ 'q42', 0, true,
),
- array( // #1
- 'q753', false, false,
+ array( // #1: first revision
+ 'q42', 1, true,
),
- array( // #2
- 'p753', false, true,
+ array( // #2: second revision
+ 'q42', 2, true,
+ ),
+ array( // #3: bad revision
+ 'q42', 600000, false,
'Wikibase\StorageException',
+ ),
+ array( // #4: wrong type
+ 'q753', 0, false,
+ ),
+ array( // #5: bad revision
+ 'p753', 1, false, 'Wikibase\StorageException',
+ ),
+ array( // #6: some revision
+ 'p753', 0, true,
),
);
@@ -97,30 +120,28 @@
/**
* @dataProvider provideGetEntity
*
- * @param string|EntityId $id The entity to get
- * @param bool|int $revision The revision to get (or null)
- * @param bool|int $expectedRev The expected revision
+ * @param string|EntityId $id The entity to get
+ * @param int $revision The revision to get (or 0)
+ * @param bool $shouldExist
+ * @param string|null $expectException
*/
- public function testGetEntity( $id, $revisionOffset, $expectedRev ) {
- if ( $revisionOffset !== false ) {
- $this->markTestIncomplete( "can't test revision IDs
yet" );
+ public function testGetEntity( $id, $revision, $shouldExist,
$expectException = null ) {
+ if ( $expectException !== null ) {
+ $this->setExpectedException( $expectException );
}
-
- //TODO: get the actual revision ID and add the offset.
- $revision = $revisionOffset;
if ( is_string( $id ) ) {
$id = EntityId::newFromPrefixedId( $id );
}
+ $revision = $this->resolveLogicalRevision( $revision );
+
$lookup = $this->getLookup();
$entity = $lookup->getEntity( $id, $revision );
- if ( $expectedRev == true ) {
+ if ( $shouldExist == true ) {
$this->assertNotNull( $entity, "ID " .
$id->getPrefixedId() );
$this->assertEquals( $id->getPrefixedId(),
$entity->getPrefixedId() );
-
- //TODO: check revision ID
} else {
$this->assertNull( $entity, "ID " .
$id->getPrefixedId() );
}
diff --git a/lib/tests/phpunit/MockRepository.php
b/lib/tests/phpunit/MockRepository.php
index c9512d1..d05c687 100644
--- a/lib/tests/phpunit/MockRepository.php
+++ b/lib/tests/phpunit/MockRepository.php
@@ -6,10 +6,10 @@
use Wikibase\EntityId;
use Wikibase\EntityLookup;
use Wikibase\Item;
-use Wikibase\PropertyLabelResolver;
use Wikibase\SiteLink;
use Wikibase\SiteLinkLookup;
use Wikibase\Property;
+use Wikibase\StorageException;
/**
* Mock repository for use in tests.
@@ -46,34 +46,42 @@
private $maxId = 0;
/**
- * Returns the entity with the provided id or null is there is no such
- * entity. If a $revision is given, the requested revision of the
entity is loaded.
- * The the revision does not belong to the given entity, null is
returned.
+ * @see EntityLookup::getEntity
*
- * @param EntityID $entityId
- * @param int|bool $revision
+ * @param EntityID $entityId
+ * @param \Wikibase\EntityId $entityId
+ * @param int $revision The desired revision id, 0 means
"current".
*
+ * @throws \Wikibase\StorageException
* @return Entity|null
*/
- public function getEntity( EntityId $entityId, $revision = false ) {
+ public function getEntity( EntityId $entityId, $revision = 0 ) {
$key = $entityId->getPrefixedId();
if ( !isset( $this->entities[$key] ) || empty(
$this->entities[$key] ) ) {
return null;
}
+ if ( $revision === false ) { // default changed from false to 0
+ wfWarn( 'getEntity() called with $revision = false, use
0 instead.' );
+ $revision = 0;
+ }
+
+ /* @var Entity[] $revisions */
$revisions = $this->entities[$key];
- if ( $revision === false ) {
+ if ( $revision === 0 ) { // note: be robust and accept false
too.
$revIds = array_keys( $revisions );
$n = count( $revIds );
$revision = $revIds[$n-1];
} else if ( !isset( $revisions[$revision] ) ) {
- return null;
+ throw new StorageException( "no such revision for
entity $key: $revision" );
}
- $entity = $revisions[$revision]->copy(); // return a copy!
+ $entity = $revisions[$revision];
+ $entity = $entity->copy();
+
return $entity;
}
@@ -210,9 +218,10 @@
* ID is given, the entity with the highest revision ID is considered
the current one.
*
* @param \Wikibase\Entity $entity
- * @param bool $revision
+ * @param bool|int $revision
+ * @param int|string $timestamp
*/
- public function putEntity( Entity $entity, $revision = false ) {
+ public function putEntity( Entity $entity, $revision = false,
$timestamp = 0 ) {
if ( $entity->getId() === null ) {
//NOTE: assign ID to original object, not clone
$entity->setId( $this->maxId +1 );
@@ -382,29 +391,19 @@
* @since 0.4
*
* @param EntityID[] $entityIds
- * @param array|bool $revision
*
* @return Entity|null[]
*/
- public function getEntities( array $entityIds, $revision = false ) {
+ public function getEntities( array $entityIds ) {
$entities = array();
foreach ( $entityIds as $key => $entityId ) {
- $rev = $revision;
if ( is_string( $entityId ) ) {
$entityId = EntityId::newFromPrefixedId(
$entityId );
}
- if ( is_array( $rev ) ) {
- if ( !array_key_exists( $key, $rev ) ) {
- throw new \MWException( '$entityId has
no revision specified' );
- }
-
- $rev = $rev[$key];
- }
-
- $entities[$entityId->getPrefixedId()] =
$this->getEntity( $entityId, $rev );
+ $entities[$entityId->getPrefixedId()] =
$this->getEntity( $entityId );
}
return $entities;
diff --git a/lib/tests/phpunit/store/CachingEntityLoaderTest.php
b/lib/tests/phpunit/store/CachingEntityLoaderTest.php
index 895e47e..4c389b7 100644
--- a/lib/tests/phpunit/store/CachingEntityLoaderTest.php
+++ b/lib/tests/phpunit/store/CachingEntityLoaderTest.php
@@ -50,8 +50,8 @@
protected function newEntityLoader( array $entities ) {
$mock = new MockRepository();
- foreach ( $entities as $entity ) {
- $mock->putEntity( $entity );
+ foreach ( $entities as $rev => $entity ) {
+ $mock->putEntity( $entity, $rev );
}
return new CachingEntityLoader( $mock );
diff --git a/lib/tests/phpunit/store/WikiPageEntityLookupTest.php
b/lib/tests/phpunit/store/WikiPageEntityLookupTest.php
index 1b9636e..5009df4 100644
--- a/lib/tests/phpunit/store/WikiPageEntityLookupTest.php
+++ b/lib/tests/phpunit/store/WikiPageEntityLookupTest.php
@@ -1,6 +1,9 @@
<?php
namespace Wikibase\Test;
+use Wikibase\Entity;
+use Wikibase\EntityContentFactory;
+use Wikibase\EntityId;
use \Wikibase\EntityLookup;
use \Wikibase\WikiPageEntityLookup;
@@ -40,6 +43,9 @@
*/
class WikipageEntityLookupTest extends EntityLookupTest {
+ /**
+ * @var array[]
+ */
protected static $testEntities = array();
public function setUp( ) {
@@ -56,54 +62,19 @@
* @return EntityLookup
*/
protected function newEntityLoader( array $entities ) {
- // make sure all test entities are in the database, but only do
that once.
- /* @var \Wikibase\Entity $entity */
- foreach ( $entities as $entity ) {
- if ( !isset(
self::$testEntities[$entity->getPrefixedId()] ) ) {
- self::storeTestEntity( $entity );
- $testEntities[$entity->getPrefixedId()] =
$entity->getPrefixedId();
+ // make sure all test entities are in the database.
+ /* @var Entity $entity */
+ foreach ( $entities as $logicalRev => $entity ) {
+ if ( !isset( self::$testEntities[$logicalRev] ) ) {
+ $revId = self::storeTestEntity( $entity );
+ self::$testEntities[$logicalRev] = array(
$entity, $revId );
}
}
return new WikiPageEntityLookup( false, CACHE_DB );
}
- /*
- protected static function getTestEntityId( $handle ) {
- $entities = self::getTestEntities();
-
- if ( !isset( $entities[$handle] ) ) {
- return null;
- }
-
- return $entities[$handle]->getId();
- }
-
- protected static function getTestEntity( $handle ) {
- $entities = self::initTestEntities();
-
- return $entities[$handle];
- }
-
- protected static function initTestEntities() {
- static $initialized = false;
-
- if ( !$initialized ) {
- $entities = self::getTestEntities();
-
- foreach ( $entities as $handle => $entity ) {
- self::storeTestEntity( $entity );
- $testEntities[$handle] =
$entity->getPrefixedId();
- }
-
- $initialized = true;
- }
-
- return self::$testEntities;
- }
- */
-
- protected static function storeTestEntity( \Wikibase\Entity $entity ) {
+ protected static function storeTestEntity( Entity $entity ) {
//NOTE: We are using EntityContent here, which is not available
on the client.
// For now, this test case will only work on the
repository.
@@ -112,12 +83,23 @@
}
// FIXME: this is using repo functionality
- $content =
\Wikibase\EntityContentFactory::singleton()->newFromEntity( $entity );
+ $content = EntityContentFactory::singleton()->newFromEntity(
$entity );
$status = $content->save( "storeTestEntity" );
if ( !$status->isOK() ) {
throw new \MWException( "couldn't create " .
$content->getTitle()->getFullText()
. ":\n" . $status->getWikiText() );
}
+
+ return $content->getWikiPage()->getRevision()->getId();
}
+
+ protected function resolveLogicalRevision( $revision ) {
+ if ( is_int( $revision ) && isset(
self::$testEntities[$revision] ) ) {
+ list( , $revision ) = self::$testEntities[$revision];
+ }
+
+ return $revision;
+ }
+
}
--
To view, visit https://gerrit.wikimedia.org/r/67251
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I37a5fecc0ee22fc92c28a8fcd1810a4559767db1
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
