jenkins-bot has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/368664 )
Change subject: Fix type checking SPARQL fallback
......................................................................
Fix type checking SPARQL fallback
The SPARQL fallback needs to happen at the outermost level, with the
original entity, not on some inner nested entity. Consider the following
subclass hierarchy:
v---------<
Q1 -> Q2 -> Q3 -> Q4 -^
\
> Q5
The PHP subtype checker (isSubclassOf()) will run through the Q3<->Q4
cycle until it runs into the check limit. (This could of course be
optimized, but that’s beside the point: there are enough hierarchies on
Wikidata that exceed the limit without cycles.) Previously, it would
then fall back to SPARQL, asking it if Q3 or Q4 are subclasses of the
desired class (Q5). This is incorrect, since the entity we’re actually
interested in is Q1.
The solution to this is to perform the fallback at an outer level,
outside of the recursive type check. To this end, a new method
isSubclassOfWithSparqlFallback() is added, which performs this fallback,
while isSubclassOf() throws an exception to indicate the fallback case
instead of returning false. hasClassInRelation() and all the tests then
use this new function instead of calling isSubclassOf() directly.
Bug: T169326
Change-Id: I2aaf3d6fa69791eff71718e808085cc99c128595
---
M includes/ConstraintCheck/Helper/TypeCheckerHelper.php
M tests/phpunit/Checker/TypeChecker/TypeCheckerHelperTest.php
2 files changed, 109 insertions(+), 26 deletions(-)
Approvals:
Jonas Kress (WMDE): Looks good to me, approved
jenkins-bot: Verified
diff --git a/includes/ConstraintCheck/Helper/TypeCheckerHelper.php
b/includes/ConstraintCheck/Helper/TypeCheckerHelper.php
index d7cb13a..b3b698a 100644
--- a/includes/ConstraintCheck/Helper/TypeCheckerHelper.php
+++ b/includes/ConstraintCheck/Helper/TypeCheckerHelper.php
@@ -4,6 +4,7 @@
use Config;
use MediaWiki\MediaWikiServices;
+use OverflowException;
use Wikibase\DataModel\Entity\EntityId;
use Wikibase\DataModel\Entity\EntityIdValue;
use Wikibase\DataModel\Entity\PropertyId;
@@ -68,31 +69,19 @@
* of one of the item ID serializations in $classesToCheck.
* If the class hierarchy is not exhausted before
* the configured limit (WBQualityConstraintsTypeCheckMaxEntities) is
reached,
- * the injected {@link SparqlHelper} is consulted if present,
- * otherwise the check aborts and returns false.
+ * an OverflowException is thrown.
*
* @param EntityId $comparativeClass
* @param string[] $classesToCheck
* @param int &$entitiesChecked
*
* @return bool
- *
- * @throws SparqlHelperException if SPARQL is used and the query times
out or some other error occurs
+ * @throws OverflowException if $entitiesChecked exceeds the configured
limit
*/
- public function isSubclassOf( EntityId $comparativeClass, array
$classesToCheck, &$entitiesChecked = 0 ) {
+ private function isSubclassOf( EntityId $comparativeClass, array
$classesToCheck, &$entitiesChecked = 0 ) {
$maxEntities = $this->config->get(
'WBQualityConstraintsTypeCheckMaxEntities' );
if ( ++$entitiesChecked > $maxEntities ) {
- if ( $entitiesChecked === $maxEntities + 1 &&
$this->sparqlHelper !== null ) {
-
MediaWikiServices::getInstance()->getStatsdDataFactory()
- ->increment(
'wikibase.quality.constraints.sparql.typeFallback' );
- return $this->sparqlHelper->hasType(
- $comparativeClass->getSerialization(),
- $classesToCheck,
- /* withInstance = */ false
- );
- } else {
- return false;
- }
+ throw new OverflowException( 'Too many entities to
check' );
}
$item = $this->entityLookup->getEntity( $comparativeClass );
@@ -127,6 +116,39 @@
}
/**
+ * Checks if $comparativeClass is a subclass
+ * of one of the item ID serializations in $classesToCheck.
+ * If isSubclassOf() aborts due to hitting the configured limit,
+ * the injected {@link SparqlHelper} is consulted if present,
+ * otherwise the check returns false.
+ *
+ * @param EntityId $comparativeClass
+ * @param string[] $classesToCheck
+ * @param int &$entitiesChecked
+ *
+ * @return bool
+ *
+ * @throws SparqlHelperException if SPARQL is used and the query times
out or some other error occurs
+ */
+ public function isSubclassOfWithSparqlFallback( EntityId
$comparativeClass, array $classesToCheck ) {
+ try {
+ return $this->isSubclassOf( $comparativeClass,
$classesToCheck );
+ } catch ( OverflowException $e ) {
+ if ( $this->sparqlHelper !== null ) {
+
MediaWikiServices::getInstance()->getStatsdDataFactory()
+ ->increment(
'wikibase.quality.constraints.sparql.typeFallback' );
+ return $this->sparqlHelper->hasType(
+ $comparativeClass->getSerialization(),
+ $classesToCheck,
+ /* withInstance = */ false
+ );
+ } else {
+ return false;
+ }
+ }
+ }
+
+ /**
* Checks, if one of the itemId serializations in $classesToCheck
* is contained in the list of $statements
* via property $relationId or if it is a subclass of
@@ -158,7 +180,7 @@
return true;
}
- if ( $this->isSubclassOf( $comparativeClass,
$classesToCheck ) ) {
+ if ( $this->isSubclassOfWithSparqlFallback(
$comparativeClass, $classesToCheck ) ) {
return true;
}
}
diff --git a/tests/phpunit/Checker/TypeChecker/TypeCheckerHelperTest.php
b/tests/phpunit/Checker/TypeChecker/TypeCheckerHelperTest.php
index c4173a6..746cc8e 100644
--- a/tests/phpunit/Checker/TypeChecker/TypeCheckerHelperTest.php
+++ b/tests/phpunit/Checker/TypeChecker/TypeCheckerHelperTest.php
@@ -4,12 +4,15 @@
use PHPUnit_Framework_TestCase;
use Wikibase\DataModel\Services\Lookup\EntityLookup;
+use Wikibase\DataModel\Services\Lookup\InMemoryEntityLookup;
use Wikibase\DataModel\Statement\Statement;
use Wikibase\DataModel\Snak\PropertyValueSnak;
use Wikibase\DataModel\Entity\EntityIdValue;
use Wikibase\DataModel\Entity\ItemId;
use Wikibase\DataModel\Entity\PropertyId;
use Wikibase\DataModel\Statement\StatementList;
+use Wikibase\Repo\Tests\NewItem;
+use Wikibase\Repo\Tests\NewStatement;
use WikibaseQuality\ConstraintReport\ConstraintCheck\Helper\SparqlHelper;
use WikibaseQuality\ConstraintReport\ConstraintCheck\Helper\TypeCheckerHelper;
use WikibaseQuality\ConstraintReport\Tests\ConstraintParameters;
@@ -46,11 +49,14 @@
}
/**
+ * @param EntityLookup $lookup The backing lookup of the mock (defaults
to JsonFileEntityLookup).
* @return EntityLookup Expects that getEntity is called
* exactly WBQualityConstraintsTypeCheckMaxEntities times.
*/
- private function getMaxEntitiesLookup() {
- $lookup = new JsonFileEntityLookup( __DIR__ );
+ private function getMaxEntitiesLookup( EntityLookup $lookup = null ) {
+ if ( $lookup === null ) {
+ $lookup = new JsonFileEntityLookup( __DIR__ );
+ }
$maxEntities = $this->getDefaultConfig()->get(
'WBQualityConstraintsTypeCheckMaxEntities' );
$spy = $this->getMock( EntityLookup::class );
@@ -63,15 +69,20 @@
/**
* @param boolean $return
+ * @param array|null $arguments
* @return SparqlHelper expects that {@link SparqlHelper::hasType}
* is called exactly once and returns $return.
*/
- private function getSparqlHelper( $return ) {
+ private function getSparqlHelper( $return, array $arguments = null ) {
+ if ( $arguments === null ) {
+ $arguments = [ $this->anything(), $this->anything(),
$this->anything() ];
+ }
$mock = $this->getMockBuilder( SparqlHelper::class )
->disableOriginalConstructor()
->getMock();
$mock->expects( $this->once() )
->method( 'hasType' )
+ ->withConsecutive( $arguments )
->willReturn( $return );
return $mock;
}
@@ -98,31 +109,81 @@
}
public function testCheckIsSubclassOfValidWithIndirection() {
- $this->assertTrue( $this->getHelper()->isSubclassOf( new
ItemId( 'Q6' ), [ 'Q100', 'Q101' ] ) );
+ $this->assertTrue(
$this->getHelper()->isSubclassOfWithSparqlFallback( new ItemId( 'Q6' ), [
'Q100', 'Q101' ] ) );
}
public function testCheckIsSubclassOfInvalid() {
- $this->assertFalse( $this->getHelper()->isSubclassOf( new
ItemId( 'Q6' ), [ 'Q200', 'Q201' ] ) );
+ $this->assertFalse(
$this->getHelper()->isSubclassOfWithSparqlFallback( new ItemId( 'Q6' ), [
'Q200', 'Q201' ] ) );
}
public function testCheckIsSubclassCyclic() {
$helper = $this->getHelper( $this->getMaxEntitiesLookup() );
- $this->assertFalse( $helper->isSubclassOf( new ItemId( 'Q7' ),
[ 'Q100', 'Q101' ] ) );
+ $this->assertFalse( $helper->isSubclassOfWithSparqlFallback(
new ItemId( 'Q7' ), [ 'Q100', 'Q101' ] ) );
}
public function testCheckIsSubclassCyclicWide() {
$helper = $this->getHelper( $this->getMaxEntitiesLookup() );
- $this->assertFalse( $helper->isSubclassOf( new ItemId( 'Q9' ),
[ 'Q100', 'Q101' ] ) );
+ $this->assertFalse( $helper->isSubclassOfWithSparqlFallback(
new ItemId( 'Q9' ), [ 'Q100', 'Q101' ] ) );
}
public function testCheckIsSubclassCyclicWideWithSparqlTrue() {
$helper = $this->getHelper( $this->getMaxEntitiesLookup(),
$this->getSparqlHelper( true ) );
- $this->assertTrue( $helper->isSubclassOf( new ItemId( 'Q9' ), [
'Q100', 'Q101' ] ) );
+ $this->assertTrue( $helper->isSubclassOfWithSparqlFallback( new
ItemId( 'Q9' ), [ 'Q100', 'Q101' ] ) );
}
public function testCheckIsSubclassCyclicWideWithSparqlFalse() {
$helper = $this->getHelper( $this->getMaxEntitiesLookup(),
$this->getSparqlHelper( false ) );
- $this->assertFalse( $helper->isSubclassOf( new ItemId( 'Q9' ),
[ 'Q100', 'Q101' ] ) );
+ $this->assertFalse( $helper->isSubclassOfWithSparqlFallback(
new ItemId( 'Q9' ), [ 'Q100', 'Q101' ] ) );
+ }
+
+ public function testCheckIsSubclassTree() {
+ $lookup = new InMemoryEntityLookup();
+ $subclassPid = $this->getDefaultConfig()->get(
'WBQualityConstraintsSubclassOfId' );
+
+ $q1 = NewItem::withId( 'Q1' )
+ ->andStatement(
+ NewStatement::forProperty( $subclassPid )
+ ->withValue( new ItemId( 'Q2' ) )
+ )
+ ->build();
+ $lookup->addEntity( $q1 );
+ $q2 = NewItem::withId( 'Q2' )
+ ->andStatement(
+ NewStatement::forProperty( $subclassPid )
+ ->withValue( new ItemId( 'Q3' ) )
+ )
+ ->andStatement(
+ NewStatement::forProperty( $subclassPid )
+ ->withValue( new ItemId( 'Q5' ) )
+ )
+ ->build();
+ $lookup->addEntity( $q2 );
+ $q3 = NewItem::withId( 'Q3' )
+ ->andStatement(
+ NewStatement::forProperty( $subclassPid )
+ ->withValue( new ItemId( 'Q4' ) )
+ )
+ ->build();
+ $lookup->addEntity( $q3 );
+ $q4 = NewItem::withId( 'Q4' )
+ ->andStatement(
+ NewStatement::forProperty( $subclassPid )
+ ->withValue( new ItemId( 'Q3' ) )
+ )
+ ->build();
+ $lookup->addEntity( $q4 );
+
+ $sparqlHelper = $this->getSparqlHelper(
+ true,
+ [
+ $this->identicalTo( 'Q1' ),
+ $this->identicalTo( [ 'Q5' ] ),
+ $this->identicalTo( false )
+ ]
+ );
+ $helper = $this->getHelper( $this->getMaxEntitiesLookup(
$lookup ), $sparqlHelper );
+
+ $this->assertTrue( $helper->isSubclassOfWithSparqlFallback( new
ItemId( 'Q1' ), [ 'Q5' ] ) );
}
}
--
To view, visit https://gerrit.wikimedia.org/r/368664
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I2aaf3d6fa69791eff71718e808085cc99c128595
Gerrit-PatchSet: 4
Gerrit-Project: mediawiki/extensions/WikibaseQualityConstraints
Gerrit-Branch: master
Gerrit-Owner: Lucas Werkmeister (WMDE) <[email protected]>
Gerrit-Reviewer: Jonas Kress (WMDE) <[email protected]>
Gerrit-Reviewer: Lucas Werkmeister (WMDE) <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
