From 892c3383dd288288fdfd07b8a0fa34d19fc0ca94 Mon Sep 17 00:00:00 2001
From: Aaron Schulz <aschulz@wikimedia.org>
Date: Wed, 16 Mar 2022 13:45:40 -0700
Subject: [PATCH] objectcache: clarify usage of getMultiWithUnionSetCallback()

The examples and comments were misleading with regard to the handling
of missing entities.

Bug: T303092
Change-Id: Ib672fd18d1270c35253683eb54b48be056e6dc3c
---
 .../objectcache/wancache/WANObjectCache.php     | 17 ++++++++++-------
 .../libs/objectcache/WANObjectCacheTest.php     |  8 ++++----
 2 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/includes/libs/objectcache/wancache/WANObjectCache.php b/includes/libs/objectcache/wancache/WANObjectCache.php
index c6d7418d2b9..bdfb44b09d1 100644
--- a/includes/libs/objectcache/wancache/WANObjectCache.php
+++ b/includes/libs/objectcache/wancache/WANObjectCache.php
@@ -1982,7 +1982,8 @@ class WANObjectCache implements
 	 *
 	 * This works the same as getWithSetCallback() except:
 	 *   - a) The $keys argument must be the result of WANObjectCache::makeMultiKeys()
-	 *   - b) The $callback argument must be a callback that takes the following arguments:
+	 *   - b) The $callback argument expects a function that returns an entity value, using
+	 *        boolean "false" if it does not exist. The callback takes the following arguments:
 	 *         - $id: ID of the entity to query
 	 *         - $oldValue: prior cache value or false if none was present
 	 *         - &$ttl: reference to the TTL to be assigned to the new value (alterable)
@@ -2061,8 +2062,9 @@ class WANObjectCache implements
 			return $callback( $params['id'], $oldValue, $ttl, $setOpts, $oldAsOf );
 		};
 
+		// Get the order-preserved result map using the warm-up cache
 		$values = [];
-		foreach ( $keyedIds as $key => $id ) { // preserve order
+		foreach ( $keyedIds as $key => $id ) {
 			$values[$key] = $this->getWithSetCallback(
 				$key,
 				$ttl,
@@ -2082,8 +2084,9 @@ class WANObjectCache implements
 	 *
 	 * This works the same as getWithSetCallback() except:
 	 *   - a) The $keys argument expects the result of WANObjectCache::makeMultiKeys()
-	 *   - b) The $callback argument expects a callback returning a map of (ID => new value)
-	 *        for all entity IDs in $ids and it takes the following arguments:
+	 *   - b) The $callback argument expects a function that returns a map of (ID => new value),
+	 *        using boolean "false" for entities that could not be found, for all entity IDs in
+	 *        $ids. The callback takes the following arguments:
 	 *          - $ids: list of entity IDs that require value generation
 	 *          - &$ttls: reference to the (entity ID => new TTL) map (alterable)
 	 *          - &$setOpts: reference to the new value set() options (alterable)
@@ -2112,7 +2115,7 @@ class WANObjectCache implements
 	 *             $setOpts += Database::getCacheSetOptions( $dbr );
 	 *
 	 *             // Load the rows for these files
-	 *             $rows = [];
+	 *             $rows = array_fill_keys( $ids, false );
 	 *             $queryInfo = File::getQueryInfo();
 	 *             $res = $dbr->select(
 	 *                 $queryInfo['tables'],
@@ -2201,9 +2204,9 @@ class WANObjectCache implements
 			return $newValue;
 		};
 
-		// Run the cache-aside logic using warmupCache instead of persistent cache queries
+		// Get the order-preserved result map using the warm-up cache
 		$values = [];
-		foreach ( $keyedIds as $key => $id ) { // preserve order
+		foreach ( $keyedIds as $key => $id ) {
 			$values[$key] = $this->getWithSetCallback(
 				$key,
 				$ttl,
diff --git a/tests/phpunit/unit/includes/libs/objectcache/WANObjectCacheTest.php b/tests/phpunit/unit/includes/libs/objectcache/WANObjectCacheTest.php
index 2afd2bdea04..da543a6f2f8 100644
--- a/tests/phpunit/unit/includes/libs/objectcache/WANObjectCacheTest.php
+++ b/tests/phpunit/unit/includes/libs/objectcache/WANObjectCacheTest.php
@@ -1075,7 +1075,7 @@ class WANObjectCacheTest extends PHPUnit\Framework\TestCase {
 		$this->assertSame( 1, $wasSet, "Value process cached while deleted" );
 
 		$calls = 0;
-		$ids = [ 1, 2, 3, 4, 5, 6 ];
+		$ids = [ 1, 2, 3, 4, 5, 6, 7 ];
 		$keyFunc = static function ( $id, WANObjectCache $wanCache ) {
 			return $wanCache->makeKey( 'test', $id );
 		};
@@ -1084,7 +1084,7 @@ class WANObjectCacheTest extends PHPUnit\Framework\TestCase {
 			$newValues = [];
 			foreach ( $ids as $id ) {
 				++$calls;
-				$newValues[$id] = "val-{$id}";
+				$newValues[$id] = ( $id <= 6 ) ? "val-{$id}" : false;
 			}
 
 			return $newValues;
@@ -1092,7 +1092,7 @@ class WANObjectCacheTest extends PHPUnit\Framework\TestCase {
 		$values = $cache->getMultiWithUnionSetCallback( $keyedIds, 10, $genFunc );
 
 		$this->assertSame(
-			[ "val-1", "val-2", "val-3", "val-4", "val-5", "val-6" ],
+			[ "val-1", "val-2", "val-3", "val-4", "val-5", "val-6", false ],
 			array_values( $values ),
 			"Correct values in correct order"
 		);
@@ -1104,7 +1104,7 @@ class WANObjectCacheTest extends PHPUnit\Framework\TestCase {
 		$this->assertSame( count( $ids ), $calls );
 
 		$cache->getMultiWithUnionSetCallback( $keyedIds, 10, $genFunc );
-		$this->assertSame( count( $ids ), $calls, "Values cached" );
+		$this->assertSame( count( $ids ) + 1, $calls, "Values cached, except for missing entity" );
 	}
 
 	public static function getMultiWithUnionSetCallback_provider() {