From 0191113d244c9153cdd5f210a933002abb807afc Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Tue, 19 Jun 2018 14:42:26 +0100 Subject: [PATCH] MDL-62747 accesslib: rdef caching should handle roles with no caps --- lib/accesslib.php | 19 +++++++--- lib/tests/accesslib_test.php | 68 ++++++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 5 deletions(-) diff --git a/lib/accesslib.php b/lib/accesslib.php index fc93ae55405..536504cf847 100644 --- a/lib/accesslib.php +++ b/lib/accesslib.php @@ -286,7 +286,11 @@ function get_role_definitions(array $roleids) { // Grab all keys we have not yet got in our static cache. if ($uncached = array_diff($roleids, array_keys($ACCESSLIB_PRIVATE->cacheroledefs))) { $cache = cache::make('core', 'roledefs'); - $ACCESSLIB_PRIVATE->cacheroledefs += array_filter($cache->get_many($uncached)); + foreach ($cache->get_many($uncached) as $roleid => $cachedroledef) { + if (is_array($cachedroledef)) { + $ACCESSLIB_PRIVATE->cacheroledefs[$roleid] = $cachedroledef; + } + } // Check we have the remaining keys from the MUC. if ($uncached = array_diff($roleids, array_keys($ACCESSLIB_PRIVATE->cacheroledefs))) { @@ -313,20 +317,25 @@ function get_role_definitions_uncached(array $roleids) { return array(); } - list($sql, $params) = $DB->get_in_or_equal($roleids); + // Create a blank results array: even if a role has no capabilities, + // we need to ensure it is included in the results to show we have + // loaded all the capabilities that there are. $rdefs = array(); + foreach ($roleids as $roleid) { + $rdefs[$roleid] = array(); + } + // Load all the capabilities for these roles in all contexts. + list($sql, $params) = $DB->get_in_or_equal($roleids); $sql = "SELECT ctx.path, rc.roleid, rc.capability, rc.permission FROM {role_capabilities} rc JOIN {context} ctx ON rc.contextid = ctx.id WHERE rc.roleid $sql"; $rs = $DB->get_recordset_sql($sql, $params); + // Store the capabilities into the expected data structure. foreach ($rs as $rd) { if (!isset($rdefs[$rd->roleid][$rd->path])) { - if (!isset($rdefs[$rd->roleid])) { - $rdefs[$rd->roleid] = array(); - } $rdefs[$rd->roleid][$rd->path] = array(); } $rdefs[$rd->roleid][$rd->path][$rd->capability] = (int) $rd->permission; diff --git a/lib/tests/accesslib_test.php b/lib/tests/accesslib_test.php index 514e002939b..3ff61927e08 100644 --- a/lib/tests/accesslib_test.php +++ b/lib/tests/accesslib_test.php @@ -1805,6 +1805,74 @@ class core_accesslib_testcase extends advanced_testcase { $this->assertFalse(has_all_capabilities($sca, $coursecontext, 0)); } + /** + * Test that the caching in get_role_definitions() and get_role_definitions_uncached() + * works as intended. + */ + public function test_role_definition_caching() { + global $DB; + + $this->resetAfterTest(); + + // Get some role ids. + $authenticatedrole = $DB->get_record('role', array('shortname' => 'user'), '*', MUST_EXIST); + $studentrole = $DB->get_record('role', array('shortname' => 'student'), '*', MUST_EXIST); + $emptyroleid = create_role('No capabilities', 'empty', 'A role with no capabilties'); + $course = $this->getDataGenerator()->create_course(); + $coursecontext = context_course::instance($course->id); + + // Instantiate the cache instance, since that does DB queries (get_config) + // and we don't care about those. + cache::make('core', 'roledefs'); + + // One database query is not necessarily one database read, it seems. Find out how many. + $startdbreads = $DB->perf_get_reads(); + $rs = $DB->get_recordset('user'); + $rs->close(); + $readsperquery = $DB->perf_get_reads() - $startdbreads; + + // Now load some role definitions, and check when it queries the database. + + // Load the capabilities for two roles. Should be one query. + $startdbreads = $DB->perf_get_reads(); + get_role_definitions([$authenticatedrole->id, $studentrole->id]); + $this->assertEquals(1 * $readsperquery, $DB->perf_get_reads() - $startdbreads); + + // Load the capabilities for same two roles. Should not query the DB. + $startdbreads = $DB->perf_get_reads(); + get_role_definitions([$authenticatedrole->id, $studentrole->id]); + $this->assertEquals(0 * $readsperquery, $DB->perf_get_reads() - $startdbreads); + + // Include a third role. Should do one DB query. + $startdbreads = $DB->perf_get_reads(); + get_role_definitions([$authenticatedrole->id, $studentrole->id, $emptyroleid]); + $this->assertEquals(1 * $readsperquery, $DB->perf_get_reads() - $startdbreads); + + // Repeat call. No DB queries. + $startdbreads = $DB->perf_get_reads(); + get_role_definitions([$authenticatedrole->id, $studentrole->id, $emptyroleid]); + $this->assertEquals(0 * $readsperquery, $DB->perf_get_reads() - $startdbreads); + + // Alter a role. + role_change_permission($studentrole->id, $coursecontext, 'moodle/course:tag', CAP_ALLOW); + + // Should now know to do one query. + $startdbreads = $DB->perf_get_reads(); + get_role_definitions([$authenticatedrole->id, $studentrole->id]); + $this->assertEquals(1 * $readsperquery, $DB->perf_get_reads() - $startdbreads); + + // Now clear the in-memory cache, and verify that it does not query the DB. + // Cannot use accesslib_clear_all_caches_for_unit_testing since that also + // clears the MUC cache. + global $ACCESSLIB_PRIVATE; + $ACCESSLIB_PRIVATE->cacheroledefs = array(); + + // Get all roles. Should not need the DB. + $startdbreads = $DB->perf_get_reads(); + get_role_definitions([$authenticatedrole->id, $studentrole->id, $emptyroleid]); + $this->assertEquals(0 * $readsperquery, $DB->perf_get_reads() - $startdbreads); + } + /** * Tests get_user_capability_course() which checks a capability across all courses. */