From a685402bd850fdb01b3acaacbacc68ad0b1021ab Mon Sep 17 00:00:00 2001 From: sam marshall Date: Thu, 5 Oct 2017 16:04:44 +0100 Subject: [PATCH] MDL-60357 core_search: Future modified times cause serious problems --- search/classes/manager.php | 4 +- .../skip_future_documents_iterator.php | 94 +++++++++++++++++++ search/tests/manager_test.php | 47 ++++++++++ 3 files changed, 144 insertions(+), 1 deletion(-) create mode 100644 search/classes/skip_future_documents_iterator.php diff --git a/search/classes/manager.php b/search/classes/manager.php index f6f7599abdf..b27c217b008 100644 --- a/search/classes/manager.php +++ b/search/classes/manager.php @@ -569,7 +569,8 @@ class manager { // Pass get_document as callback. $fileindexing = $this->engine->file_indexing_enabled() && $searcharea->uses_file_indexing(); $options = array('indexfiles' => $fileindexing, 'lastindexedtime' => $prevtimestart); - $iterator = new \core\dml\recordset_walk($recordset, array($searcharea, 'get_document'), $options); + $iterator = new skip_future_documents_iterator(new \core\dml\recordset_walk( + $recordset, array($searcharea, 'get_document'), $options)); foreach ($iterator as $document) { if (!$document instanceof \core_search\document) { continue; @@ -594,6 +595,7 @@ class manager { $lastindexeddoc = $document->get('modified'); $numrecords++; } + $recordset->close(); if (CLI_SCRIPT && !PHPUNIT_TEST) { if ($numdocs > 0) { diff --git a/search/classes/skip_future_documents_iterator.php b/search/classes/skip_future_documents_iterator.php new file mode 100644 index 00000000000..3204c833f37 --- /dev/null +++ b/search/classes/skip_future_documents_iterator.php @@ -0,0 +1,94 @@ +. + +/** + * Iterator for skipping search recordset documents that are in the future. + * + * @package core_search + * @copyright 2017 The Open University + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +namespace core_search; + +defined('MOODLE_INTERNAL') || die(); + +/** + * Iterator for skipping search recordset documents that are in the future. + * + * This iterator stops iterating if it receives a document that was modified later than the + * specified cut-off time (usually current time). + * + * This iterator assumes that its parent iterator returns documents in modified order (which is + * required to be the case for search indexing). This means we will stop retrieving data from the + * recordset + * + * @copyright 2017 The Open University + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class skip_future_documents_iterator implements \Iterator { + /** @var \Iterator Parent iterator */ + protected $parent; + + /** @var int Cutoff time; anything later than this will cause the iterator to stop */ + protected $cutoff; + + /** + * Constructor. + * + * @param \Iterator $parent Parent iterator, must return search documents in modified order + * @param int $cutoff Cut-off time, default is current time + */ + public function __construct(\Iterator $parent, $cutoff = 0) { + $this->parent = $parent; + if ($cutoff) { + $this->cutoff = $cutoff; + } else { + $this->cutoff = time(); + } + } + + public function current() { + return $this->parent->current(); + } + + public function next() { + $this->parent->next(); + } + + public function key() { + return $this->parent->key(); + } + + public function valid() { + // Check parent. + if (!$this->parent->valid()) { + return false; + } + + // See if document is after the cut-off time. + $doc = $this->parent->current(); + if ($doc->get('modified') > $this->cutoff) { + return false; + } + + return true; + } + + public function rewind() { + $this->parent->rewind(); + } +} diff --git a/search/tests/manager_test.php b/search/tests/manager_test.php index a0b60b69bd5..98cb5379b77 100644 --- a/search/tests/manager_test.php +++ b/search/tests/manager_test.php @@ -151,6 +151,53 @@ class search_manager_testcase extends advanced_testcase { $this->assertEquals(0, $configs[$this->forumpostareaid]->indexingend); } + /** + * Tests that documents with modified time in the future are NOT indexed (as this would cause + * a problem by preventing it from indexing other documents modified between now and the future + * date). + */ + public function test_future_documents() { + $this->resetAfterTest(); + + // Create a course and a forum. + $generator = $this->getDataGenerator(); + $course = $generator->create_course(); + $forum = $generator->create_module('forum', ['course' => $course->id]); + + // Index everything up to current. Ensure the course is older than current second so it + // definitely doesn't get indexed again next time. + $this->waitForSecond(); + $search = testable_core_search::instance(); + $search->index(false, 0); + + // Add 2 discussions to the forum, one of which happend just now, but the other is + // incorrectly set to the future. + $now = time(); + $userid = get_admin()->id; + $generator->get_plugin_generator('mod_forum')->create_discussion(['course' => $course->id, + 'forum' => $forum->id, 'userid' => $userid, 'timemodified' => $now, + 'name' => 'Frog']); + $generator->get_plugin_generator('mod_forum')->create_discussion(['course' => $course->id, + 'forum' => $forum->id, 'userid' => $userid, 'timemodified' => $now + 100, + 'name' => 'Toad']); + + // Wait for a second so we're not actually on the same second as the forum post (there's a + // 1 second overlap between indexing; it would get indexed in both checks below otherwise). + $this->waitForSecond(); + + // Index. + $search->index(false); + + // Check latest time - it should be the same as $now, not the + 100. + $searcharea = $search->get_search_area($this->forumpostareaid); + list($componentname, $varname) = $searcharea->get_config_var_name(); + $this->assertEquals($now, get_config($componentname, $varname . '_lastindexrun')); + + // Index again - there should be nothing to index this time. + $search->index(false); + $this->assertEquals($now, get_config($componentname, $varname . '_lastindexrun')); + } + /** * Adding this test here as get_areas_user_accesses process is the same, results just depend on the context level. *