Merge "add logger to EntityAccessor"

wikimedia · Oct 26, 2020 · acf74d9 · acf74d9
2 parents cdf0662 + aa490c7
commit acf74d9
Show file tree

Hide file tree

Showing 4 changed files with 86 additions and 16 deletions.
diff --git a/client/includes/DataAccess/Scribunto/EntityAccessor.php b/client/includes/DataAccess/Scribunto/EntityAccessor.php
@@ -4,6 +4,8 @@
 
 use InvalidArgumentException;
 use Language;
+use Psr\Log\LoggerInterface;
+use Psr\Log\NullLogger;
 use Serializers\Serializer;
 use Wikibase\Client\Serializer\ClientEntitySerializer;
 use Wikibase\Client\Serializer\ClientStatementListSerializer;
@@ -74,6 +76,11 @@ class EntityAccessor {
 	 */
 	private $fineGrainedLuaTracking;
 
+	/**
+	 * @var LoggerInterface
+	 */
+	private $logger;
+
 	/**
 	 * @param EntityIdParser $entityIdParser
 	 * @param EntityLookup $entityLookup
@@ -97,7 +104,8 @@ public function __construct(
 		TermLanguageFallbackChain $termFallbackChain,
 		Language $language,
 		ContentLanguages $termsLanguages,
-		$fineGrainedLuaTracking
+		$fineGrainedLuaTracking,
+		LoggerInterface $logger = null
 	) {
 		$this->entityIdParser = $entityIdParser;
 		$this->entityLookup = $entityLookup;
@@ -109,6 +117,7 @@ public function __construct(
 		$this->language = $language;
 		$this->termsLanguages = $termsLanguages;
 		$this->fineGrainedLuaTracking = $fineGrainedLuaTracking;
+		$this->logger = $logger ?: new NullLogger();
 	}
 
 	/**
@@ -148,10 +157,7 @@ public function getEntity( $prefixedEntityId ) {
 		try {
 			$entityObject = $this->entityLookup->getEntity( $entityId );
 		} catch ( RevisionedUnresolvedRedirectException $ex ) {
-			// We probably hit a double redirect
-			wfLogWarning(
-				'Encountered a UnresolvedRedirectException when trying to load ' . $prefixedEntityId
-			);
+			$this->logPossibleDoubleRedirect( $prefixedEntityId );
 
 			return null;
 		}
@@ -186,10 +192,7 @@ public function entityExists( $prefixedEntityId ) {
 		try {
 			return $this->entityLookup->hasEntity( $entityId );
 		} catch ( RevisionedUnresolvedRedirectException $ex ) {
-			// We probably hit a double redirect
-			wfLogWarning(
-				'Encountered a UnresolvedRedirectException when trying to check the existence of ' . $prefixedEntityId
-			);
+			$this->logPossibleDoubleRedirect( $prefixedEntityId );
 
 			return false;
 		}
@@ -215,10 +218,7 @@ public function getEntityStatements( $prefixedEntityId, $propertyIdSerialization
 		try {
 			$entity = $this->entityLookup->getEntity( $entityId );
 		} catch ( RevisionedUnresolvedRedirectException $ex ) {
-			// We probably hit a double redirect
-			wfLogWarning(
-				'Encountered a UnresolvedRedirectException when trying to load ' . $prefixedEntityId
-			);
+			$this->logPossibleDoubleRedirect( $prefixedEntityId );
 
 			return null;
 		}
@@ -260,4 +260,16 @@ private function newClientStatementListSerializer() {
 		);
 	}
 
+	/**
+	 * @see RevisionedUnresolvedRedirectException
+	 * @param $prefixedEntityId
+	 */
+	private function logPossibleDoubleRedirect( $prefixedEntityId ) {
+		$this->logger->info( 'Unresolved redirect encountered loading {prefixedEntityId}. This is typically cleaned up asynchronously.',
+			[
+				'prefixedEntityId' => $prefixedEntityId
+			]
+		);
+	}
+
 }
diff --git a/client/includes/DataAccess/Scribunto/Scribunto_LuaWikibaseLibrary.php b/client/includes/DataAccess/Scribunto/Scribunto_LuaWikibaseLibrary.php
@@ -252,7 +252,8 @@ private function newEntityAccessor() {
 			$this->getLanguageFallbackChain(),
 			$this->getLanguage(),
 			$wikibaseClient->getTermsLanguages(),
-			$settings->getSetting( 'fineGrainedLuaTracking' )
+			$settings->getSetting( 'fineGrainedLuaTracking' ),
+			$wikibaseClient->getLogger()
 		);
 	}
 

diff --git a/client/tests/phpunit/unit/includes/DataAccess/Scribunto/EntityAccessorTest.php b/client/tests/phpunit/unit/includes/DataAccess/Scribunto/EntityAccessorTest.php
@@ -6,6 +6,7 @@
 use DataValues\StringValue;
 use InvalidArgumentException;
 use Language;
+use Psr\Log\LoggerInterface;
 use ReflectionMethod;
 use Wikibase\Client\DataAccess\Scribunto\EntityAccessor;
 use Wikibase\Client\Usage\EntityUsage;
@@ -24,6 +25,7 @@
 use Wikibase\DataModel\Statement\Statement;
 use Wikibase\Lib\LanguageFallbackChainFactory;
 use Wikibase\Lib\StaticContentLanguages;
+use Wikibase\Lib\Store\RevisionedUnresolvedRedirectException;
 use Wikibase\Lib\Tests\MockRepository;
 
 /**
@@ -48,7 +50,8 @@ private function getEntityAccessor(
 		EntityLookup $entityLookup = null,
 		UsageAccumulator $usageAccumulator = null,
 		$langCode = 'en',
-		$fineGrainedTracking = true
+		$fineGrainedTracking = true,
+		$logger = null
 	) {
 		$language = Language::factory( $langCode );
 		$serializerFactory = new SerializerFactory(
@@ -79,7 +82,8 @@ private function getEntityAccessor(
 			$fallbackChain,
 			$language,
 			new StaticContentLanguages( [ 'de', $langCode, 'es', 'ja' ] ),
-			$fineGrainedTracking
+			$fineGrainedTracking,
+			$logger
 		);
 	}
 
@@ -450,4 +454,54 @@ public function testEntityExists() {
 		$this->assertSame( false, $entityAccessor->entityExists( 'Q1239' ) );
 	}
 
+	/**
+	 * @dataProvider doubleRedirectMethodProvider
+	 * @param EntityId $entityId
+	 * @param string $methodName
+	 * @param array $methodParameters
+	 * @param string $lookupMethodCalled
+	 */
+	public function testGetEntityStatementsLogsDoubleRedirects(
+		EntityId $entityId,
+		string $methodName,
+		array $methodParameters,
+		string $lookupMethodCalled
+	) {
+		$entityLookup = $this->createMock( EntityLookup::class );
+		$entityLookup->expects( $this->once() )
+			->method( $lookupMethodCalled )
+			->with( $entityId )
+			->willThrowException( new RevisionedUnresolvedRedirectException( $entityId, $entityId ) );
+
+		$logger = $this->createMock( LoggerInterface::class );
+		$logger->expects( $this->once() )
+			->method( 'info' )
+			->with(
+				'Unresolved redirect encountered loading {prefixedEntityId}. This is typically cleaned up asynchronously.',
+				[
+					'prefixedEntityId' => $entityId->serialize()
+				]
+			);
+
+		$entityAccessor = $this->getEntityAccessor(
+			$entityLookup,
+			null,
+			'en',
+			true,
+			$logger
+		);
+
+		$entityAccessor->$methodName( ...$methodParameters );
+	}
+
+	public function doubleRedirectMethodProvider() {
+		$entityId = new ItemId( 'Q1' );
+		$serialization = $entityId->getSerialization();
+		return [
+			[ $entityId, 'getEntityStatements', [ $serialization, 'P1', 'best' ], 'getEntity' ],
+			[ $entityId, 'getEntity', [ $serialization ], 'getEntity' ],
+			[ $entityId, 'entityExists', [ $serialization ], 'hasEntity' ],
+		];
+	}
+
 }
diff --git a/lib/includes/Store/RevisionedUnresolvedRedirectException.php b/lib/includes/Store/RevisionedUnresolvedRedirectException.php
@@ -9,6 +9,9 @@
  * Exception indicating that an attempt was made to access a redirected EntityId
  * without resolving the redirect first.
  *
+ * In the case of double-redirects on wikidata they are currently handled by bots asynchronously
+ * @see https://www.wikidata.org/wiki/Help:Redirects
+ *
  * @license GPL-2.0-or-later
  * @author Daniel Kinzler
  */