diff --git a/lib/upgrade.txt b/lib/upgrade.txt index 68de055d2fa..2f51937a794 100644 --- a/lib/upgrade.txt +++ b/lib/upgrade.txt @@ -1,6 +1,13 @@ This files describes API changes in core libraries and APIs, information provided here is intended especially for developers. +=== 3.5.5 === + +* The \core_rating provider's get_sql_join function now accepts an optional $innerjoin parameter. + It is recommended that privacy providers using this function call rewrite any long query into a number of separate + calls to add_from_sql for improved performance, and that the new argument is used. + This will allow queries to remain backwards-compatible with older versions of Moodle but will have significantly better performance in version supporting the innerjoin parameter. + === 3.5.4 === * New optional parameter $context for the groups_get_members_join() function and ability to filter users that are not members of diff --git a/rating/classes/privacy/provider.php b/rating/classes/privacy/provider.php index e1d87afbfca..864ec4f4872 100644 --- a/rating/classes/privacy/provider.php +++ b/rating/classes/privacy/provider.php @@ -129,26 +129,41 @@ class provider implements /** * Get the SQL required to find all submission items where this user has had any involvements. * + * If possible an inner join should be used. + * * @param string $alias The name of the table alias to use. * @param string $component The na eof the component to fetch ratings for. * @param string $ratingarea The rating area to fetch results for. * @param string $itemidjoin The right-hand-side of the JOIN ON clause. * @param int $userid The ID of the user being stored. + * @param bool $innerjoin Whether to use an inner join (preferred) * @return \stdClass */ - public static function get_sql_join($alias, $component, $ratingarea, $itemidjoin, $userid) { + public static function get_sql_join($alias, $component, $ratingarea, $itemidjoin, $userid, $innerjoin = false) { static $count = 0; $count++; - // Join the rating table with the specified alias and the relevant join params. - $join = "LEFT JOIN {rating} {$alias} ON "; - $join .= "{$alias}.userid = :ratinguserid{$count} AND "; - $join .= "{$alias}.component = :ratingcomponent{$count} AND "; - $join .= "{$alias}.ratingarea = :ratingarea{$count} AND "; - $join .= "{$alias}.itemid = {$itemidjoin}"; + $userwhere = ''; - // Match against the specified user. - $userwhere = "{$alias}.id IS NOT NULL"; + if ($innerjoin) { + // Join the rating table with the specified alias and the relevant join params. + $join = "JOIN {rating} {$alias} ON "; + $join .= "{$alias}.itemid = {$itemidjoin}"; + + $userwhere .= "{$alias}.userid = :ratinguserid{$count} AND "; + $userwhere .= "{$alias}.component = :ratingcomponent{$count} AND "; + $userwhere .= "{$alias}.ratingarea = :ratingarea{$count}"; + } else { + // Join the rating table with the specified alias and the relevant join params. + $join = "LEFT JOIN {rating} {$alias} ON "; + $join .= "{$alias}.userid = :ratinguserid{$count} AND "; + $join .= "{$alias}.component = :ratingcomponent{$count} AND "; + $join .= "{$alias}.ratingarea = :ratingarea{$count} AND "; + $join .= "{$alias}.itemid = {$itemidjoin}"; + + // Match against the specified user. + $userwhere = "{$alias}.id IS NOT NULL"; + } $params = [ 'ratingcomponent' . $count => $component, diff --git a/rating/tests/privacy_provider_test.php b/rating/tests/privacy_provider_test.php index 7f0fb179331..5f92c0f857a 100644 --- a/rating/tests/privacy_provider_test.php +++ b/rating/tests/privacy_provider_test.php @@ -153,6 +153,94 @@ class core_rating_privacy_testcase extends \core_privacy\tests\provider_testcase $this->assertFalse(isset($courses[$course3->id])); } + /** + * Ensure that the get_sql_join function returns valid SQL which returns the correct list of rated itemids. + * This makes use of the optional inner join argument. + */ + public function test_get_sql_join_inner() { + global $DB; + $this->resetAfterTest(); + + $course1 = $this->getDataGenerator()->create_course(); + $course2 = $this->getDataGenerator()->create_course(); + $course3 = $this->getDataGenerator()->create_course(); + + $u1 = $this->getDataGenerator()->create_user(); + $u2 = $this->getDataGenerator()->create_user(); + $u3 = $this->getDataGenerator()->create_user(); + + // Rate the courses. + $rm = new rating_manager(); + $ratingoptions = (object) [ + 'component' => 'core_course', + 'ratingarea' => 'course', + 'scaleid' => 100, + ]; + + // Rate all courses as u1, and something else in the same context. + $this->rate_as_user($u1->id, 'core_course', 'course', $course1->id, \context_course::instance($course1->id), 25); + $this->rate_as_user($u1->id, 'core_course', 'course', $course2->id, \context_course::instance($course2->id), 50); + $this->rate_as_user($u1->id, 'core_course', 'course', $course3->id, \context_course::instance($course3->id), 75); + $this->rate_as_user($u1->id, 'core_course', 'files', $course3->id, \context_course::instance($course3->id), 99); + + // Rate course2 as u2, and something else in a different context/component.. + $this->rate_as_user($u2->id, 'core_course', 'course', $course2->id, \context_course::instance($course2->id), 90); + $this->rate_as_user($u2->id, 'user', 'user', $u3->id, \context_user::instance($u3->id), 10); + + // Return any course which the u1 has rated. + // u1 rated all three courses. + $ratingquery = provider::get_sql_join('r', 'core_course', 'course', 'c.id', $u1->id, true); + $sql = "SELECT c.id FROM {course} c {$ratingquery->join} WHERE {$ratingquery->userwhere}"; + $courses = $DB->get_records_sql($sql, $ratingquery->params); + + $this->assertCount(3, $courses); + $this->assertTrue(isset($courses[$course1->id])); + $this->assertTrue(isset($courses[$course2->id])); + $this->assertTrue(isset($courses[$course3->id])); + + // User u1 rated files in course 3 only. + $ratingquery = provider::get_sql_join('r', 'core_course', 'files', 'c.id', $u1->id, true); + $sql = "SELECT c.id FROM {course} c {$ratingquery->join} WHERE {$ratingquery->userwhere}"; + $courses = $DB->get_records_sql($sql, $ratingquery->params); + + $this->assertCount(1, $courses); + $this->assertFalse(isset($courses[$course1->id])); + $this->assertFalse(isset($courses[$course2->id])); + $this->assertTrue(isset($courses[$course3->id])); + + // Return any course which the u2 has rated. + // User u2 rated only course 2. + $ratingquery = provider::get_sql_join('r', 'core_course', 'course', 'c.id', $u2->id, true); + $sql = "SELECT c.id FROM {course} c {$ratingquery->join} WHERE {$ratingquery->userwhere}"; + $courses = $DB->get_records_sql($sql, $ratingquery->params); + + $this->assertCount(1, $courses); + $this->assertFalse(isset($courses[$course1->id])); + $this->assertTrue(isset($courses[$course2->id])); + $this->assertFalse(isset($courses[$course3->id])); + + // User u2 rated u3. + $ratingquery = provider::get_sql_join('r', 'user', 'user', 'u.id', $u2->id, true); + $sql = "SELECT u.id FROM {user} u {$ratingquery->join} WHERE {$ratingquery->userwhere}"; + $users = $DB->get_records_sql($sql, $ratingquery->params); + + $this->assertCount(1, $users); + $this->assertFalse(isset($users[$u1->id])); + $this->assertFalse(isset($users[$u2->id])); + $this->assertTrue(isset($users[$u3->id])); + + // Return any course which the u3 has rated. + // User u3 did not rate anything. + $ratingquery = provider::get_sql_join('r', 'core_course', 'course', 'c.id', $u3->id, true); + $sql = "SELECT c.id FROM {course} c {$ratingquery->join} WHERE {$ratingquery->userwhere}"; + $courses = $DB->get_records_sql($sql, $ratingquery->params); + + $this->assertCount(0, $courses); + $this->assertFalse(isset($courses[$course1->id])); + $this->assertFalse(isset($courses[$course2->id])); + $this->assertFalse(isset($courses[$course3->id])); + } + /** * Ensure that export_area_ratings exports all ratings that a user has made, and all ratings for a users own content. */