Skip to content

Commit

Permalink
MDL-57515 Resource: 'not very efficient' with a large number of files
Browse files Browse the repository at this point in the history 
Fixes bug where the resource module loads metadata for all files while
building course modinfo, even though it only needs the first file.
(This causes problems if you have ~10k files.)
  • Loading branch information
@sammarshallou
sammarshallou committed Jan 10, 2017
1 parent 8ed0851 commit 66234de
Show file tree
Hide file tree
Showing 4 changed files with 91 additions and 6 deletions.
20 changes: 15 additions & 5 deletions lib/filestorage/file_storage.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -799,10 +799,12 @@ public function get_external_files($repositoryid, $sort = '') {
* @param string $sort A fragment of SQL to use for sorting
* @param bool $includedirs whether or not include directories
* @param int $updatedsince return files updated since this time
* @param int $limitfrom return a subset of records, starting at this point (optional).
* @param int $limitnum return a subset comprising this many records in total (optional, required if $limitfrom is set).
* @return stored_file[] array of stored_files indexed by pathanmehash
*/
public function get_area_files($contextid, $component, $filearea, $itemid = false, $sort = "itemid, filepath, filename",
$includedirs = true, $updatedsince = 0) {
$includedirs = true, $updatedsince = 0, $limitfrom = 0, $limitnum = 0) {
global $DB;

list($areasql, $conditions) = $DB->get_in_or_equal($filearea, SQL_PARAMS_NAMED);
Expand All @@ -824,25 +826,33 @@ public function get_area_files($contextid, $component, $filearea, $itemid = fals
$updatedsincesql = 'AND f.timemodified > :time';
}

$includedirssql = '';
if (!$includedirs) {
$includedirssql = 'AND f.filename != :dot';
$conditions['dot'] = '.';
}

if ($limitfrom && !$limitnum) {
throw new coding_exception('If specifying $limitfrom you must also specify $limitnum');
}

$sql = "SELECT ".self::instance_sql_fields('f', 'r')."
FROM {files} f
LEFT JOIN {files_reference} r
ON f.referencefileid = r.id
WHERE f.contextid = :contextid
AND f.component = :component
AND f.filearea $areasql
$includedirssql
$updatedsincesql
$itemidsql";
if (!empty($sort)) {
$sql .= " ORDER BY {$sort}";
}

$result = array();
$filerecords = $DB->get_records_sql($sql, $conditions);
$filerecords = $DB->get_records_sql($sql, $conditions, $limitfrom, $limitnum);
foreach ($filerecords as $filerecord) {
if (!$includedirs and $filerecord->filename === '.') {
continue;
}
$result[$filerecord->pathnamehash] = $this->get_file_instance($filerecord);
}
return $result;
Expand Down
11 changes: 11 additions & 0 deletions lib/filestorage/tests/file_storage_test.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,17 @@ public function test_get_area_files() {
$this->assertEquals($key, $file->get_pathnamehash());
}

// Test the limit feature to retrieve each individual file.
$limited = $fs->get_area_files($user->ctxid, 'user', 'private', false, 'filename', false,
0, 0, 1);
$mapfunc = function($f) {
return $f->get_filename();
};
$this->assertEquals(array('1.txt'), array_values(array_map($mapfunc, $limited)));
$limited = $fs->get_area_files($user->ctxid, 'user', 'private', false, 'filename', false,
0, 1, 50);
$this->assertEquals(array('2.txt', '3.txt'), array_values(array_map($mapfunc, $limited)));

// Test with an itemid with no files.
$areafiles = $fs->get_area_files($user->ctxid, 'user', 'private', 666, 'sortorder', false);
// Should be none.
Expand Down
4 changes: 3 additions & 1 deletion mod/resource/lib.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -213,8 +213,10 @@ function resource_get_coursemodule_info($coursemodule) {
$info->icon ='i/invalid';
return $info;
}

// See if there is at least one file.
$fs = get_file_storage();
$files = $fs->get_area_files($context->id, 'mod_resource', 'content', 0, 'sortorder DESC, id ASC', false); // TODO: this is not very efficient!!
$files = $fs->get_area_files($context->id, 'mod_resource', 'content', 0, 'sortorder DESC, id ASC', false, 0, 0, 1);
if (count($files) >= 1) {
$mainfile = reset($files);
$info->icon = file_file_icon($mainfile, 24);
Expand Down
62 changes: 62 additions & 0 deletions mod/resource/tests/lib_test.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,4 +89,66 @@ public function test_resource_view() {
$this->assertEquals(1, $completiondata->completionstate);

}

/**
* Tests the resource_get_coursemodule_info function.
*
* Note: This currently doesn't test every aspect of the function, mainly focusing on the icon.
*/
public function test_get_coursemodule_info() {
global $DB, $USER;

$this->resetAfterTest();
$this->setAdminUser();

// Create course.
$generator = $this->getDataGenerator();
$course = $generator->create_course();

// Create a resource with no files.
$draftid = file_get_unused_draft_itemid();
$resource1 = $generator->create_module('resource', array('course' => $course->id,
'name' => 'R1', 'files' => $draftid));

// Create a resource with one file.
$draftid = file_get_unused_draft_itemid();
$contextid = context_user::instance($USER->id)->id;
$filerecord = array('component' => 'user', 'filearea' => 'draft', 'contextid' => $contextid,
'itemid' => $draftid, 'filename' => 'r2.txt', 'filepath' => '/');
$fs = get_file_storage();
$fs->create_file_from_string($filerecord, 'Test');
$resource2 = $generator->create_module('resource', array('course' => $course->id,
'name' => 'R2', 'files' => $draftid));

// Create a resource with two files.
$draftid = file_get_unused_draft_itemid();
$filerecord = array('component' => 'user', 'filearea' => 'draft', 'contextid' => $contextid,
'itemid' => $draftid, 'filename' => 'r3.txt', 'filepath' => '/', 'sortorder' => 1);
$fs->create_file_from_string($filerecord, 'Test');
$filerecord['filename'] = 'r3.doc';
$filerecord['sortorder'] = 2;
$fs->create_file_from_string($filerecord, 'Test');
$resource3 = $generator->create_module('resource', array('course' => $course->id,
'name' => 'R3', 'files' => $draftid));

// Try get_coursemodule_info for first one.
$info = resource_get_coursemodule_info(
$DB->get_record('course_modules', array('id' => $resource1->cmid)));

// The name should be set. There is no overridden icon.
$this->assertEquals('R1', $info->name);
$this->assertEmpty($info->icon);

// For second one, there should be an overridden icon.
$info = resource_get_coursemodule_info(
$DB->get_record('course_modules', array('id' => $resource2->cmid)));
$this->assertEquals('R2', $info->name);
$this->assertEquals('f/text-24', $info->icon);

// For third one, it should use the highest sortorder icon.
$info = resource_get_coursemodule_info(
$DB->get_record('course_modules', array('id' => $resource3->cmid)));
$this->assertEquals('R3', $info->name);
$this->assertEquals('f/document-24', $info->icon);
}
}

0 comments on commit 66234de

Please sign in to comment.