Lucas Werkmeister (WMDE) has uploaded a new change for review. (
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 -> Q6
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 (Q6). 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.
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, 99 insertions(+), 21 deletions(-)
git pull
ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/WikibaseQualityConstraints
refs/changes/64/368664/1
diff --git a/includes/ConstraintCheck/Helper/TypeCheckerHelper.php
b/includes/ConstraintCheck/Helper/TypeCheckerHelper.php
index d7cb13a..933046e 100644
--- a/includes/ConstraintCheck/Helper/TypeCheckerHelper.php
+++ b/includes/ConstraintCheck/Helper/TypeCheckerHelper.php
@@ -82,17 +82,7 @@
public 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;
- }
+ return false;
}
$item = $this->entityLookup->getEntity( $comparativeClass );
@@ -124,6 +114,26 @@
}
return false;
+ }
+
+ public function isSubclassOfWithSparqlFallback( EntityId
$comparativeClass, array $classesToCheck ) {
+ $entitiesChecked = 0;
+ $maxEntities = $this->config->get(
'WBQualityConstraintsTypeCheckMaxEntities' );
+ $recursiveResult = $this->isSubclassOf( $comparativeClass,
$classesToCheck, $entitiesChecked );
+
+ if ( $recursiveResult ) {
+ return true;
+ } elseif ( $entitiesChecked > $maxEntities &&
$this->sparqlHelper !== null ) {
+ MediaWikiServices::getInstance()->getStatsdDataFactory()
+ ->increment(
'wikibase.quality.constraints.sparql.typeFallback' );
+ return $this->sparqlHelper->hasType(
+ $comparativeClass->getSerialization(),
+ $classesToCheck,
+ /* withInstance = */ false
+ );
+ } else {
+ return false;
+ }
}
/**
@@ -158,7 +168,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..318beb5 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,88 @@
}
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 );
+ $q5 = NewItem::withId( 'Q5' )
+ ->andStatement(
+ NewStatement::forProperty( $subclassPid )
+ ->withValue( new ItemId( 'Q6' ) )
+ )
+ ->build();
+ $lookup->addEntity( $q5 );
+
+ $sparqlHelper = $this->getSparqlHelper(
+ true,
+ [
+ $this->identicalTo( 'Q1' ),
+ $this->identicalTo( [ 'Q6' ] ),
+ $this->identicalTo( false )
+ ]
+ );
+ $helper = $this->getHelper( $this->getMaxEntitiesLookup(
$lookup ), $sparqlHelper );
+
+ $this->assertTrue( $helper->isSubclassOfWithSparqlFallback( new
ItemId( 'Q1' ), [ 'Q6' ] ) );
}
}
--
To view, visit https://gerrit.wikimedia.org/r/368664
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2aaf3d6fa69791eff71718e808085cc99c128595
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/WikibaseQualityConstraints
Gerrit-Branch: master
Gerrit-Owner: Lucas Werkmeister (WMDE) <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
