From be11ee3383e8b53c4cedbed0df62186bcf29e163 Mon Sep 17 00:00:00 2001
From: sam marshall <s.marshall@open.ac.uk>
Date: Mon, 14 Nov 2022 11:11:18 +0000
Subject: [PATCH] MDL-76218 cachestore_redis: delete_many can fail with no keys

In some cases, we get an error message such as:

Wrong parameter count for Redis::zRem()

Within the delete_many function. This function requires at least one
key to be supplied, but if delete_many is called with an empty array,
we will call it with no keys.
---
 cache/stores/redis/lib.php              |  4 +++
 cache/stores/redis/tests/store_test.php | 63 ++++++++++++++++++++++++++++++---
 2 files changed, 63 insertions(+), 4 deletions(-)

diff --git a/cache/stores/redis/lib.php b/cache/stores/redis/lib.php
index 8521812ee72..84b07cf02ae 100644
--- a/cache/stores/redis/lib.php
+++ b/cache/stores/redis/lib.php
@@ -439,6 +439,10 @@ class cachestore_redis extends cache_store implements cache_is_key_aware, cache_
      * @return int The number of keys successfully deleted.
      */
     public function delete_many(array $keys) {
+        // If there are no keys to delete, do nothing.
+        if (!$keys) {
+            return 0;
+        }
         $count = $this->redis->hDel($this->hash, ...$keys);
         if ($this->definition->get_ttl()) {
             // When TTL is enabled, also remove the keys from the TTL list.
diff --git a/cache/stores/redis/tests/store_test.php b/cache/stores/redis/tests/store_test.php
index 87f3c04dfbc..dd44f6b796e 100644
--- a/cache/stores/redis/tests/store_test.php
+++ b/cache/stores/redis/tests/store_test.php
@@ -34,6 +34,7 @@ require_once(__DIR__.'/../lib.php');
  * define('TEST_CACHESTORE_REDIS_TESTSERVERS', '127.0.0.1');
  *
  * @package   cachestore_redis
+ * @covers    \cachestore_redis
  * @copyright Copyright (c) 2015 Moodlerooms Inc. (http://www.moodlerooms.com)
  * @license   http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
@@ -70,13 +71,28 @@ class store_test extends \cachestore_tests {
      * Creates the required cachestore for the tests to run against Redis.
      *
      * @param array $extraconfig Extra configuration options for Redis instance, if any
+     * @param bool $ttl True to use a cache definition with TTL enabled
      * @return cachestore_redis
      */
-    protected function create_cachestore_redis(array $extraconfig = []): cachestore_redis {
-        /** @var cache_definition $definition */
-        $definition = cache_definition::load_adhoc(cache_store::MODE_APPLICATION, 'cachestore_redis', 'phpunit_test');
+    protected function create_cachestore_redis(array $extraconfig = [], bool $ttl = false): cachestore_redis {
+        if ($ttl) {
+            /** @var cache_definition $definition */
+            $definition = cache_definition::load('core/wibble', [
+                'mode' => 1,
+                'simplekeys' => true,
+                'simpledata' => true,
+                'ttl' => 10,
+                'component' => 'core',
+                'area' => 'wibble',
+                'selectedsharingoption' => 2,
+                'userinputsharingkey' => '',
+                'sharingoptions' => 15,
+            ]);
+        } else {
+            /** @var cache_definition $definition */
+            $definition = cache_definition::load_adhoc(cache_store::MODE_APPLICATION, 'cachestore_redis', 'phpunit_test');
+        }
         $configuration = array_merge(cachestore_redis::unit_test_configuration(), $extraconfig);
-
         $store = new cachestore_redis('Test', $configuration);
         $store->initialise($definition);
 
@@ -165,4 +181,43 @@ class store_test extends \cachestore_tests {
         ]);
         $this->assertEquals(111, $store->get_last_io_bytes());
     }
+
+    /**
+     * Data provider for whether cache uses TTL or not.
+     *
+     * @return array Array with true and false options
+     */
+    public static function ttl_or_not(): array {
+        return [
+            [false],
+            [true]
+        ];
+    }
+
+    /**
+     * Tests the delete_many function.
+     *
+     * The behaviour is different with TTL enabled so we need to test with that kind of definition
+     * as well as a 'normal' one.
+     *
+     * @param bool $ttl True to test using a TTL definition
+     * @dataProvider ttl_or_not
+     */
+    public function test_delete_many(bool $ttl): void {
+        $store = $this->create_cachestore_redis([], $ttl);
+
+        // Check it works to delete selected items.
+        $store->set('foo', 'frog');
+        $store->set('bar', 'amphibian');
+        $store->set('hmm', 'undead');
+        $this->store->delete_many(['foo', 'bar']);
+        $this->assertFalse($store->get('foo'));
+        $this->assertFalse($store->get('bar'));
+        $this->assertEquals('undead', $store->get('hmm'));
+
+        // If called with no keys it should do nothing.
+        $store->delete_many([]);
+        $this->assertEquals('undead', $store->get('hmm'));
+    }
+
 }
-- 
2.11.4.GIT