From: Aaron Schulz <aschulz@wikimedia.org>
Date: Mon, 27 Nov 2017 02:45:09 +0000 (-0800)
Subject: objectcache: only give current format keys getWithSetCallback() callbacks
X-Git-Tag: 1.31.0-rc.0~1377^2
X-Git-Url: https://git.heureux-cyclage.org/w/index.php?a=commitdiff_plain;h=ea22e3d1f6677884d8490d744ca15c65fd34c769;p=lhc%2Fweb%2Fwiklou.git

objectcache: only give current format keys getWithSetCallback() callbacks

Callback code that happens to make use of $oldValue might not be able
to handle missing, new, or changed fields due to key version changes.

Overhaul testGetWithSetCallback_versions() to be cleaner and cover the
case of unversioned => versioned keys.

Change-Id: If108a73078c530c985d30bdadcbfa9ddd53dc2be
---

diff --git a/includes/libs/objectcache/WANObjectCache.php b/includes/libs/objectcache/WANObjectCache.php
index 830200acaa..727e3c12f4 100644
--- a/includes/libs/objectcache/WANObjectCache.php
+++ b/includes/libs/objectcache/WANObjectCache.php
@@ -914,11 +914,14 @@ class WANObjectCache implements IExpiringStore, LoggerAwareInterface {
 					use ( $callback, $version ) {
 						if ( is_array( $oldValue )
 							&& array_key_exists( self::VFLD_DATA, $oldValue )
+							&& array_key_exists( self::VFLD_VERSION, $oldValue )
+							&& $oldValue[self::VFLD_VERSION] === $version
 						) {
 							$oldData = $oldValue[self::VFLD_DATA];
 						} else {
 							// VFLD_DATA is not set if an old, unversioned, key is present
 							$oldData = false;
+							$oldAsOf = null;
 						}
 
 						return [
diff --git a/tests/phpunit/includes/libs/objectcache/WANObjectCacheTest.php b/tests/phpunit/includes/libs/objectcache/WANObjectCacheTest.php
index a518ceea9b..df8228dcb0 100644
--- a/tests/phpunit/includes/libs/objectcache/WANObjectCacheTest.php
+++ b/tests/phpunit/includes/libs/objectcache/WANObjectCacheTest.php
@@ -1115,53 +1115,69 @@ class WANObjectCacheTest extends PHPUnit_Framework_TestCase {
 		$cache = $this->cache;
 
 		$key = wfRandomString();
-		$value = wfRandomString();
+		$valueV1 = wfRandomString();
+		$valueV2 = [ wfRandomString() ];
 
 		$wasSet = 0;
-		$func = function ( $old, &$ttl ) use ( &$wasSet, $value ) {
+		$funcV1 = function () use ( &$wasSet, $valueV1 ) {
 			++$wasSet;
-			return $value;
+
+			return $valueV1;
+		};
+
+		$priorValue = false;
+		$priorAsOf = null;
+		$funcV2 = function ( $oldValue, &$ttl, $setOpts, $oldAsOf )
+		use ( &$wasSet, $valueV2, &$priorValue, &$priorAsOf ) {
+			$priorValue = $oldValue;
+			$priorAsOf = $oldAsOf;
+			++$wasSet;
+
+			return $valueV2; // new array format
 		};
 
 		// Set the main key (version N if versioned)
 		$wasSet = 0;
-		$v = $cache->getWithSetCallback( $key, 30, $func, $extOpts );
-		$this->assertEquals( $value, $v, "Value returned" );
+		$v = $cache->getWithSetCallback( $key, 30, $funcV1, $extOpts );
+		$this->assertEquals( $valueV1, $v, "Value returned" );
 		$this->assertEquals( 1, $wasSet, "Value regenerated" );
-		$cache->getWithSetCallback( $key, 30, $func, $extOpts );
+		$cache->getWithSetCallback( $key, 30, $funcV1, $extOpts );
 		$this->assertEquals( 1, $wasSet, "Value not regenerated" );
-		// Set the key for version N+1 (if versioned)
+		$this->assertEquals( $valueV1, $v, "Value not regenerated" );
+
 		if ( $versioned ) {
+			// Set the key for version N+1 format
 			$verOpts = [ 'version' => $extOpts['version'] + 1 ];
-
-			$wasSet = 0;
-			$v = $cache->getWithSetCallback( $key, 30, $func, $verOpts + $extOpts );
-			$this->assertEquals( $value, $v, "Value returned" );
-			$this->assertEquals( 1, $wasSet, "Value regenerated" );
-
-			$wasSet = 0;
-			$v = $cache->getWithSetCallback( $key, 30, $func, $verOpts + $extOpts );
-			$this->assertEquals( $value, $v, "Value returned" );
-			$this->assertEquals( 0, $wasSet, "Value not regenerated" );
+		} else {
+			// Start versioning now with the unversioned key still there
+			$verOpts = [ 'version' => 1 ];
 		}
 
+		// Value goes to secondary key since V1 already used $key
 		$wasSet = 0;
-		$cache->getWithSetCallback( $key, 30, $func, $extOpts );
-		$this->assertEquals( 0, $wasSet, "Value not regenerated" );
+		$v = $cache->getWithSetCallback( $key, 30, $funcV2, $verOpts + $extOpts );
+		$this->assertEquals( $valueV2, $v, "Value returned" );
+		$this->assertEquals( 1, $wasSet, "Value regenerated" );
+		$this->assertEquals( false, $priorValue, "Old value not given due to old format" );
+		$this->assertEquals( null, $priorAsOf, "Old value not given due to old format" );
 
 		$wasSet = 0;
-		$cache->delete( $key );
-		$v = $cache->getWithSetCallback( $key, 30, $func, $extOpts );
-		$this->assertEquals( $value, $v, "Value returned" );
+		$v = $cache->getWithSetCallback( $key, 30, $funcV2, $verOpts + $extOpts );
+		$this->assertEquals( $valueV2, $v, "Value not regenerated (secondary key)" );
+		$this->assertEquals( 0, $wasSet, "Value not regenerated (secondary key)" );
+
+		// Clear out the older or unversioned key
+		$cache->delete( $key, 0 );
+
+		// Set the key for next/first versioned format
+		$wasSet = 0;
+		$v = $cache->getWithSetCallback( $key, 30, $funcV2, $verOpts + $extOpts );
+		$this->assertEquals( $valueV2, $v, "Value returned" );
 		$this->assertEquals( 1, $wasSet, "Value regenerated" );
 
-		if ( $versioned ) {
-			$wasSet = 0;
-			$verOpts = [ 'version' => $extOpts['version'] + 1 ];
-			$v = $cache->getWithSetCallback( $key, 30, $func, $verOpts + $extOpts );
-			$this->assertEquals( $value, $v, "Value returned" );
-			$this->assertEquals( 1, $wasSet, "Value regenerated" );
-		}
+		$v = $cache->getWithSetCallback( $key, 30, $funcV2, $verOpts + $extOpts );
+		$this->assertEquals( $valueV2, $v, "Value not regenerated (main key)" );
+		$this->assertEquals( 1, $wasSet, "Value not regenerated (main key)" );
 	}
 
 	public static function getWithSetCallback_versions_provider() {