From f3789f2fb326c4eabf38caa6601bd9b8dc499e68 Mon Sep 17 00:00:00 2001
From: Russell Smith <mr-russ@smith2001.net>
Date: Tue, 7 Jun 2016 15:15:46 +1000
Subject: [PATCH] MDL-45584 cache: Make identifiers part of the cache creation.

It is now safe to cache a reference to a cache and expect consistent results.

Changing identifiers altered cache results where a reference was
held to the cache. Identifiers have been set to be cached with
identifiers included so the caches are separate.

As a consequence of this it was identified that invalidation events
and identifiers don't easily work together as an event can't determine
which identifiers should be used for cache invalidation.  So invalidation
events have been made incompatible with identifiers being set.  No core
code used this combination as it's not possible to understand any expected
behaviour.

Event invalidation for application and session caches was centralised to the same
location.  The only difference was the name of the lastinvalidation variable. This
improves support and consistency of invalidation code.
---
 cache/classes/definition.php |  10 ++-
 cache/classes/factory.php    |  19 +++--
 cache/classes/helper.php     |  11 ++-
 cache/classes/loaders.php    | 157 ++++++++++++++---------------------
 cache/disabledlib.php        |   8 +-
 cache/tests/cache_test.php   |  24 +++---
 cache/upgrade.txt            |   8 ++
 lib/dml/moodle_database.php  |  21 +++--
 8 files changed, 131 insertions(+), 127 deletions(-)

diff --git a/cache/classes/definition.php b/cache/classes/definition.php
index 0e5c62e44ba..2354dcaae60 100644
--- a/cache/classes/definition.php
+++ b/cache/classes/definition.php
@@ -796,10 +796,12 @@ class cache_definition {
      * @throws coding_exception
      */
     public function set_identifiers(array $identifiers = array()) {
-        // If we are setting the exact same identifiers then just return as nothing really changed.
-        // We don't care about order as cache::make will use the same definition order all the time.
-        if ($identifiers === $this->identifiers) {
-            return false;
+        if ($this->identifiers !== null) {
+            throw new coding_exception("You can only set identifiers on initial definition creation." .
+                " Define a new cache to set different identifiers.");
+        }
+        if (!empty($identifiers) && !empty($this->invalidationevents)) {
+            throw new coding_exception("You cannot use event invalidation and identifiers at the same time.");
         }
 
         foreach ($this->requireidentifiers as $identifier) {
diff --git a/cache/classes/factory.php b/cache/classes/factory.php
index c174befec19..6ae461d7181 100644
--- a/cache/classes/factory.php
+++ b/cache/classes/factory.php
@@ -191,15 +191,18 @@ class cache_factory {
      * @return cache_application|cache_session|cache_request
      */
     public function create_cache_from_definition($component, $area, array $identifiers = array(), $unused = null) {
-        $definitionname = $component.'/'.$area;
+        $identifierstring = empty($identifiers) ? '' : '/'.http_build_query($identifiers);
+        $definitionname = $component.'/'.$area.$identifierstring;
         if (isset($this->cachesfromdefinitions[$definitionname])) {
             $cache = $this->cachesfromdefinitions[$definitionname];
-            $cache->set_identifiers($identifiers);
             return $cache;
         }
         $definition = $this->create_definition($component, $area);
-        $definition->set_identifiers($identifiers);
-        $cache = $this->create_cache($definition);
+        // Identifiers are cached as part of the cache creation, so we store a cloned version of the cache.
+        $cacheddefinition = clone($definition);
+        $cacheddefinition->set_identifiers($identifiers);
+        $cache = $this->create_cache($cacheddefinition);
+
         // Loaders are always held onto to speed up subsequent requests.
         $this->cachesfromdefinitions[$definitionname] = $cache;
         return $cache;
@@ -222,10 +225,14 @@ class cache_factory {
      * @return cache_application|cache_session|cache_request
      */
     public function create_cache_from_params($mode, $component, $area, array $identifiers = array(), array $options = array()) {
-        $key = "{$mode}_{$component}_{$area}";
-        if (array_key_exists($key, $this->cachesfromparams)) {
+        $identifierstring = empty($identifiers) ? '' : '_'.http_build_query($identifiers);
+        $key = "{$mode}_{$component}_{$area}{$identifierstring}";
+        if (isset($this->cachesfromparams[$key])) {
             return $this->cachesfromparams[$key];
         }
+        // Regular cache definitions are cached inside create_definition().  This is not the case for Adhoc definitions
+        // using load_adhoc().  They are built as a new object on each call.
+        // We do not need to clone the definition because we know it's new.
         $definition = cache_definition::load_adhoc($mode, $component, $area, $options);
         $definition->set_identifiers($identifiers);
         $cache = $this->create_cache($definition);
diff --git a/cache/classes/helper.php b/cache/classes/helper.php
index 30f82350699..f15e2ee8fb5 100644
--- a/cache/classes/helper.php
+++ b/cache/classes/helper.php
@@ -229,7 +229,8 @@ class cache_helper {
     /**
      * Invalidates a given set of keys by means of an event.
      *
-     * @todo add support for identifiers to be supplied and utilised.
+     * Events cannot determine what identifiers might need to be cleared. Event based purge and invalidation
+     * are only supported on caches without identifiers.
      *
      * @param string $event
      * @param array $keys
@@ -292,8 +293,9 @@ class cache_helper {
         if ($cache instanceof cache_store) {
             $factory = cache_factory::instance();
             $definition = $factory->create_definition($component, $area, null);
-            $definition->set_identifiers($identifiers);
-            $cache->initialise($definition);
+            $cacheddefinition = clone $definition;
+            $cacheddefinition->set_identifiers($identifiers);
+            $cache->initialise($cacheddefinition);
         }
         // Purge baby, purge.
         $cache->purge();
@@ -303,6 +305,9 @@ class cache_helper {
     /**
      * Purges a cache of all information on a given event.
      *
+     * Events cannot determine what identifiers might need to be cleared. Event based purge and invalidation
+     * are only supported on caches without identifiers.
+     *
      * @param string $event
      */
     public static function purge_by_event($event) {
diff --git a/cache/classes/loaders.php b/cache/classes/loaders.php
index 3c5bd423ccb..9ea6df32080 100644
--- a/cache/classes/loaders.php
+++ b/cache/classes/loaders.php
@@ -275,6 +275,65 @@ class cache implements cache_loader {
         }
     }
 
+    /**
+     * Process any outstanding invalidation events for the cache we are registering,
+     *
+     * Identifiers and event invalidation are not compatible with each other at this time.
+     * As a result the cache does not need to consider identifiers when working out what to invalidate.
+     */
+    protected function handle_invalidation_events() {
+        if (!$this->definition->has_invalidation_events()) {
+            return;
+        }
+
+        $lastinvalidation = $this->get('lastinvalidation');
+        if ($lastinvalidation === false) {
+            // This is a new cache or purged globally, there won't be anything to invalidate.
+            // Set the time of the last invalidation and move on.
+            $this->set('lastinvalidation', self::now());
+            return;
+        } else if ($lastinvalidation == self::now()) {
+            // We've already invalidated during this request.
+            return;
+        }
+
+        // Get the event invalidation cache.
+        $cache = self::make('core', 'eventinvalidation');
+        $events = $cache->get_many($this->definition->get_invalidation_events());
+        $todelete = array();
+        $purgeall = false;
+        // Iterate the returned data for the events.
+        foreach ($events as $event => $keys) {
+            if ($keys === false) {
+                // No data to be invalidated yet.
+                continue;
+            }
+            // Look at each key and check the timestamp.
+            foreach ($keys as $key => $timestamp) {
+                // If the timestamp of the event is more than or equal to the last invalidation (happened between the last
+                // invalidation and now)then we need to invaliate the key.
+                if ($timestamp >= $lastinvalidation) {
+                    if ($key === 'purged') {
+                        $purgeall = true;
+                        break;
+                    } else {
+                        $todelete[] = $key;
+                    }
+                }
+            }
+        }
+        if ($purgeall) {
+            $this->purge();
+        } else if (!empty($todelete)) {
+            $todelete = array_unique($todelete);
+            $this->delete_many($todelete);
+        }
+        // Set the time of the last invalidation.
+        if ($purgeall || !empty($todelete)) {
+            $this->set('lastinvalidation', self::now());
+        }
+    }
+
     /**
      * Retrieves the value for the given key from the cache.
      *
@@ -1214,54 +1273,7 @@ class cache_application extends cache implements cache_loader_with_locking {
             $this->requirelockingwrite = $definition->require_locking_write();
         }
 
-        if ($definition->has_invalidation_events()) {
-            $lastinvalidation = $this->get('lastinvalidation');
-            if ($lastinvalidation === false) {
-                // This is a new session, there won't be anything to invalidate. Set the time of the last invalidation and
-                // move on.
-                $this->set('lastinvalidation', cache::now());
-                return;
-            } else if ($lastinvalidation == cache::now()) {
-                // We've already invalidated during this request.
-                return;
-            }
-
-            // Get the event invalidation cache.
-            $cache = cache::make('core', 'eventinvalidation');
-            $events = $cache->get_many($definition->get_invalidation_events());
-            $todelete = array();
-            $purgeall = false;
-            // Iterate the returned data for the events.
-            foreach ($events as $event => $keys) {
-                if ($keys === false) {
-                    // No data to be invalidated yet.
-                    continue;
-                }
-                // Look at each key and check the timestamp.
-                foreach ($keys as $key => $timestamp) {
-                    // If the timestamp of the event is more than or equal to the last invalidation (happened between the last
-                    // invalidation and now)then we need to invaliate the key.
-                    if ($timestamp >= $lastinvalidation) {
-                        if ($key === 'purged') {
-                            $purgeall = true;
-                            break;
-                        } else {
-                            $todelete[] = $key;
-                        }
-                    }
-                }
-            }
-            if ($purgeall) {
-                $this->purge();
-            } else if (!empty($todelete)) {
-                $todelete = array_unique($todelete);
-                $this->delete_many($todelete);
-            }
-            // Set the time of the last invalidation.
-            if ($purgeall || !empty($todelete)) {
-                $this->set('lastinvalidation', cache::now());
-            }
-        }
+        $this->handle_invalidation_events();
     }
 
     /**
@@ -1615,54 +1627,7 @@ class cache_session extends cache {
         // This will trigger check tracked user. If this gets removed a call to that will need to be added here in its place.
         $this->set(self::LASTACCESS, cache::now());
 
-        if ($definition->has_invalidation_events()) {
-            $lastinvalidation = $this->get('lastsessioninvalidation');
-            if ($lastinvalidation === false) {
-                // This is a new session, there won't be anything to invalidate. Set the time of the last invalidation and
-                // move on.
-                $this->set('lastsessioninvalidation', cache::now());
-                return;
-            } else if ($lastinvalidation == cache::now()) {
-                // We've already invalidated during this request.
-                return;
-            }
-
-            // Get the event invalidation cache.
-            $cache = cache::make('core', 'eventinvalidation');
-            $events = $cache->get_many($definition->get_invalidation_events());
-            $todelete = array();
-            $purgeall = false;
-            // Iterate the returned data for the events.
-            foreach ($events as $event => $keys) {
-                if ($keys === false) {
-                    // No data to be invalidated yet.
-                    continue;
-                }
-                // Look at each key and check the timestamp.
-                foreach ($keys as $key => $timestamp) {
-                    // If the timestamp of the event is more than or equal to the last invalidation (happened between the last
-                    // invalidation and now)then we need to invaliate the key.
-                    if ($timestamp >= $lastinvalidation) {
-                        if ($key === 'purged') {
-                            $purgeall = true;
-                            break;
-                        } else {
-                            $todelete[] = $key;
-                        }
-                    }
-                }
-            }
-            if ($purgeall) {
-                $this->purge();
-            } else if (!empty($todelete)) {
-                $todelete = array_unique($todelete);
-                $this->delete_many($todelete);
-            }
-            // Set the time of the last invalidation.
-            if ($purgeall || !empty($todelete)) {
-                $this->set('lastsessioninvalidation', cache::now());
-            }
-        }
+        $this->handle_invalidation_events();
     }
 
     /**
diff --git a/cache/disabledlib.php b/cache/disabledlib.php
index 565f27e14d6..3a83d1fd643 100644
--- a/cache/disabledlib.php
+++ b/cache/disabledlib.php
@@ -212,6 +212,9 @@ class cache_factory_disabled extends cache_factory {
      * @return cache_application|cache_session|cache_request
      */
     public function create_cache_from_definition($component, $area, array $identifiers = array(), $unused = null) {
+        // Regular cache definitions are cached inside create_definition().  This is not the case for disabledlib.php
+        // definitions as they use load_adhoc().  They are built as a new object on each call.
+        // We do not need to clone the definition because we know it's new.
         $definition = $this->create_definition($component, $area);
         $definition->set_identifiers($identifiers);
         $cache = $this->create_cache($definition);
@@ -233,7 +236,10 @@ class cache_factory_disabled extends cache_factory {
      * @return cache_application|cache_session|cache_request
      */
     public function create_cache_from_params($mode, $component, $area, array $identifiers = array(), array $options = array()) {
-        $definition = cache_definition::load_adhoc($mode, $component, $area);
+        // Regular cache definitions are cached inside create_definition().  This is not the case for disabledlib.php
+        // definitions as they use load_adhoc().  They are built as a new object on each call.
+        // We do not need to clone the definition because we know it's new.
+        $definition = cache_definition::load_adhoc($mode, $component, $area, $options);
         $definition->set_identifiers($identifiers);
         $cache = $this->create_cache($definition);
         return $cache;
diff --git a/cache/tests/cache_test.php b/cache/tests/cache_test.php
index ac6bfed2c08..ef4709670b0 100644
--- a/cache/tests/cache_test.php
+++ b/cache/tests/cache_test.php
@@ -188,7 +188,9 @@ class core_cache_testcase extends advanced_testcase {
     }
 
     /**
-     * Tests set_identifiers resets identifiers and static cache
+     * Tests set_identifiers fails post cache creation.
+     *
+     * set_identifiers cannot be called after initial cache instantiation, as you need to create a difference cache.
      */
     public function test_set_identifiers() {
         $instance = cache_config_testing::instance();
@@ -204,16 +206,8 @@ class core_cache_testcase extends advanced_testcase {
         $this->assertTrue($cache->set('contest', 'test data 1'));
         $this->assertEquals('test data 1', $cache->get('contest'));
 
+        $this->expectException('coding_exception');
         $cache->set_identifiers(array());
-        $this->assertFalse($cache->get('contest'));
-        $this->assertTrue($cache->set('contest', 'empty ident'));
-        $this->assertEquals('empty ident', $cache->get('contest'));
-
-        $cache->set_identifiers(array('area'));
-        $this->assertEquals('test data 1', $cache->get('contest'));
-
-        $cache->set_identifiers(array());
-        $this->assertEquals('empty ident', $cache->get('contest'));
     }
 
     /**
@@ -2022,6 +2016,16 @@ class core_cache_testcase extends advanced_testcase {
         $this->assertEquals('b', $returnedinstance2->name);
     }
 
+    public function test_identifiers_have_separate_caches() {
+        $cachepg = cache::make('core', 'databasemeta', array('dbfamily' => 'pgsql'));
+        $cachepg->set(1, 'here');
+        $cachemy = cache::make('core', 'databasemeta', array('dbfamily' => 'mysql'));
+        $cachemy->set(2, 'there');
+        $this->assertEquals('here', $cachepg->get(1));
+        $this->assertEquals('there', $cachemy->get(2));
+        $this->assertFalse($cachemy->get(1));
+    }
+
     public function test_performance_debug() {
         global $CFG;
         $this->resetAfterTest(true);
diff --git a/cache/upgrade.txt b/cache/upgrade.txt
index e995cfff8eb..16bbbf59a50 100644
--- a/cache/upgrade.txt
+++ b/cache/upgrade.txt
@@ -1,6 +1,14 @@
 This files describes API changes in /cache/stores/* - cache store plugins.
 Information provided here is intended especially for developers.
 
+=== 3.3 ===
+* Identifiers and invalidation events have been explictly been marked as incompatible and will
+  throw a coding exception. Unexpected results would have occurred if the previous behaviour was attempted.
+* Identifiers are now part of loaded caches, so identifiers can only be set at cache::make()
+  a coding_exception will be thrown if attempts are made at other times.
+  Multiple calls to cache::make with different identifiers will produce 2 caches instead of changing the
+  keyspace of a single cache.
+
 === 3.2 ===
 * The following methods have been finally deprecated and should no longer be used.
   - cache_definition::should_be_persistent()
diff --git a/lib/dml/moodle_database.php b/lib/dml/moodle_database.php
index 078c237e32f..82a98abb12c 100644
--- a/lib/dml/moodle_database.php
+++ b/lib/dml/moodle_database.php
@@ -124,6 +124,9 @@ abstract class moodle_database {
     /** @var cache_application for column info */
     protected $metacache;
 
+    /** @var cache_request for column info on temp tables */
+    protected $metacachetemp;
+
     /** @var bool flag marking database instance as disposed */
     protected $disposed;
 
@@ -332,13 +335,14 @@ abstract class moodle_database {
     /**
      * Handle the creation and caching of the databasemeta information for all databases.
      *
-     * TODO MDL-53267 impelement caching of cache::make() results when it's safe to do so.
-     *
      * @return cache_application The databasemeta cachestore to complete operations on.
      */
     protected function get_metacache() {
-        $properties = array('dbfamily' => $this->get_dbfamily(), 'settings' => $this->get_settings_hash());
-        return cache::make('core', 'databasemeta', $properties);
+        if (!isset($this->metacache)) {
+            $properties = array('dbfamily' => $this->get_dbfamily(), 'settings' => $this->get_settings_hash());
+            $this->metacache = cache::make('core', 'databasemeta', $properties);
+        }
+        return $this->metacache;
     }
 
     /**
@@ -347,9 +351,12 @@ abstract class moodle_database {
      * @return cache_application The temp_tables cachestore to complete operations on.
      */
     protected function get_temp_tables_cache() {
-        // Using connection data to prevent collisions when using the same temp table name with different db connections.
-        $properties = array('dbfamily' => $this->get_dbfamily(), 'settings' => $this->get_settings_hash());
-        return cache::make('core', 'temp_tables', $properties);
+        if (!isset($this->metacachetemp)) {
+            // Using connection data to prevent collisions when using the same temp table name with different db connections.
+            $properties = array('dbfamily' => $this->get_dbfamily(), 'settings' => $this->get_settings_hash());
+            $this->metacachetemp = cache::make('core', 'temp_tables', $properties);
+        }
+        return $this->metacachetemp;
     }
 
     /**