From 107ab4b97b95e7463870689dfab7542745f11e33 Mon Sep 17 00:00:00 2001 From: sam marshall Date: Fri, 14 Jun 2019 16:37:57 +0100 Subject: [PATCH] MDL-65931 Web services: get_courses_by_field minor inefficiency When called with a list of course IDs, this change makes it preload the contexts and categories before further processing. This saves a number of queries, depending on how many courses you call it for. (Typically the function is called with a list of all your enrolled courses.) The change may also improve performance for other web services that call the validate_courses function. --- course/externallib.php | 21 ++++++++++++--- lib/externallib.php | 49 +++++++++++++++++++++++++++++----- lib/tests/externallib_test.php | 24 +++++++++++++++++ 3 files changed, 85 insertions(+), 9 deletions(-) diff --git a/course/externallib.php b/course/externallib.php index c91465667ad..7f3a170a75b 100644 --- a/course/externallib.php +++ b/course/externallib.php @@ -3129,7 +3129,20 @@ class core_course_external extends external_api { } if ($params['field'] === 'ids') { - $courses = $DB->get_records_list('course', 'id', explode(',', $value), 'id ASC'); + // Preload categories to avoid loading one at a time. + $courseids = explode(',', $value); + list ($listsql, $listparams) = $DB->get_in_or_equal($courseids); + $categoryids = $DB->get_fieldset_sql(" + SELECT DISTINCT cc.id + FROM {course} c + JOIN {course_categories} cc ON cc.id = c.category + WHERE c.id $listsql", $listparams); + core_course_category::get_many($categoryids); + + // Load and validate all courses. This is called because it loads the courses + // more efficiently. + list ($courses, $warnings) = external_util::validate_courses($courseids, [], + false, true); } else { $courses = $DB->get_records('course', array($params['field'] => $value), 'id ASC'); } @@ -3148,9 +3161,11 @@ class core_course_external extends external_api { // Get the public course information, even if we are not enrolled. $courseinlist = new core_course_list_element($course); - // Now, check if we have access to the course. + // Now, check if we have access to the course, unless it was already checked. try { - self::validate_context($context); + if (empty($course->contextvalidated)) { + self::validate_context($context); + } } catch (Exception $e) { // User can not access the course, check if they can see the public information about the course and return it. if (core_course_category::can_view_course_info($course)) { diff --git a/lib/externallib.php b/lib/externallib.php index c958000820a..ae2b5dd8383 100644 --- a/lib/externallib.php +++ b/lib/externallib.php @@ -1334,18 +1334,53 @@ class external_util { /** * Validate a list of courses, returning the complete course objects for valid courses. * + * Each course has an additional 'contextvalidated' field, this will be set to true unless + * you set $keepfails, in which case it will be false if validation fails for a course. + * * @param array $courseids A list of course ids * @param array $courses An array of courses already pre-fetched, indexed by course id. * @param bool $addcontext True if the returned course object should include the full context object. + * @param bool $keepfails True to keep all the course objects even if validation fails * @return array An array of courses and the validation warnings */ - public static function validate_courses($courseids, $courses = array(), $addcontext = false) { + public static function validate_courses($courseids, $courses = array(), $addcontext = false, + $keepfails = false) { + global $DB; + // Delete duplicates. $courseids = array_unique($courseids); $warnings = array(); // Remove courses which are not even requested. - $courses = array_intersect_key($courses, array_flip($courseids)); + $courses = array_intersect_key($courses, array_flip($courseids)); + + // For any courses NOT loaded already, get them in a single query (and preload contexts) + // for performance. Preserve ordering because some tests depend on it. + $newcourseids = []; + foreach ($courseids as $cid) { + if (!array_key_exists($cid, $courses)) { + $newcourseids[] = $cid; + } + } + if ($newcourseids) { + list ($listsql, $listparams) = $DB->get_in_or_equal($newcourseids); + + // Load list of courses, and preload associated contexts. + $contextselect = context_helper::get_preload_record_columns_sql('x'); + $newcourses = $DB->get_records_sql(" + SELECT c.*, $contextselect + FROM {course} c + JOIN {context} x ON x.instanceid = c.id + WHERE x.contextlevel = ? AND c.id $listsql", + array_merge([CONTEXT_COURSE], $listparams)); + foreach ($newcourseids as $cid) { + if (array_key_exists($cid, $newcourses)) { + $course = $newcourses[$cid]; + context_helper::preload_from_record($course); + $courses[$course->id] = $course; + } + } + } foreach ($courseids as $cid) { // Check the user can function in this context. @@ -1353,14 +1388,16 @@ class external_util { $context = context_course::instance($cid); external_api::validate_context($context); - if (!isset($courses[$cid])) { - $courses[$cid] = get_course($cid); - } if ($addcontext) { $courses[$cid]->context = $context; } + $courses[$cid]->contextvalidated = true; } catch (Exception $e) { - unset($courses[$cid]); + if ($keepfails) { + $courses[$cid]->contextvalidated = false; + } else { + unset($courses[$cid]); + } $warnings[] = array( 'item' => 'course', 'itemid' => $cid, diff --git a/lib/tests/externallib_test.php b/lib/tests/externallib_test.php index 390e70a6429..619ae286a6c 100644 --- a/lib/tests/externallib_test.php +++ b/lib/tests/externallib_test.php @@ -481,6 +481,30 @@ class core_externallib_testcase extends advanced_testcase { $this->assertEquals($c1->id, $courses[$c1->id]->id); } + /** + * Validate courses, but still return courses even if they fail validation. + */ + public function test_validate_courses_keepfails() { + $this->resetAfterTest(true); + + $c1 = $this->getDataGenerator()->create_course(); + $c2 = $this->getDataGenerator()->create_course(); + $c3 = $this->getDataGenerator()->create_course(); + $u1 = $this->getDataGenerator()->create_user(); + $this->getDataGenerator()->enrol_user($u1->id, $c1->id); + $courseids = array($c1->id, $c2->id, $c3->id); + + $this->setUser($u1); + list($courses, $warnings) = external_util::validate_courses($courseids, [], false, true); + $this->assertCount(2, $warnings); + $this->assertEquals($c2->id, $warnings[0]['itemid']); + $this->assertEquals($c3->id, $warnings[1]['itemid']); + $this->assertCount(3, $courses); + $this->assertTrue($courses[$c1->id]->contextvalidated); + $this->assertFalse($courses[$c2->id]->contextvalidated); + $this->assertFalse($courses[$c3->id]->contextvalidated); + } + /** * Validate courses can re-use an array of prefetched courses. */