From 4b0facc98448226cc7aec9e9762571a3d919d73b Mon Sep 17 00:00:00 2001 From: sam marshall Date: Tue, 7 Nov 2017 12:14:06 +0000 Subject: [PATCH] MDL-60720 core_search: Indexing halts on failed get_document The recordsets used for search indexing sometimes return results which are invalid (e.g. cannot be found in database). When this happens, the result in the iterator for the recordset will be false. Due to a bug, the iterator used to stop when it encountered a false value, which prevented indexing from getting past the problematic record. In addition, the iterator that skips future data resulted in the current() function of its parent indicator being called twice per entry, which meant that search indexing called get_document() twice as many times. --- .../skip_future_documents_iterator.php | 22 +- .../skip_future_documents_iterator_test.php | 250 ++++++++++++++++++ 2 files changed, 267 insertions(+), 5 deletions(-) create mode 100644 search/tests/skip_future_documents_iterator_test.php diff --git a/search/classes/skip_future_documents_iterator.php b/search/classes/skip_future_documents_iterator.php index c5a0279ae15..6297fd80503 100644 --- a/search/classes/skip_future_documents_iterator.php +++ b/search/classes/skip_future_documents_iterator.php @@ -46,6 +46,12 @@ class skip_future_documents_iterator implements \Iterator { /** @var int Cutoff time; anything later than this will cause the iterator to stop */ protected $cutoff; + /** @var mixed Current value of iterator */ + protected $currentdoc; + + /** @var bool True if current value is available */ + protected $gotcurrent; + /** * Constructor. * @@ -62,11 +68,16 @@ class skip_future_documents_iterator implements \Iterator { } public function current() { - return $this->parent->current(); + if (!$this->gotcurrent) { + $this->currentdoc = $this->parent->current(); + $this->gotcurrent = true; + } + return $this->currentdoc; } public function next() { $this->parent->next(); + $this->gotcurrent = false; } public function key() { @@ -79,16 +90,17 @@ class skip_future_documents_iterator implements \Iterator { return false; } - if ($doc = $this->parent->current()) { + if ($doc = $this->current()) { // This document is valid if the modification date is before the cutoff. return $doc->get('modified') <= $this->cutoff; + } else { + // If the document is false/null, allow iterator to continue. + return true; } - - return false; - } public function rewind() { $this->parent->rewind(); + $this->gotcurrent = false; } } diff --git a/search/tests/skip_future_documents_iterator_test.php b/search/tests/skip_future_documents_iterator_test.php new file mode 100644 index 00000000000..3e9a40b49c7 --- /dev/null +++ b/search/tests/skip_future_documents_iterator_test.php @@ -0,0 +1,250 @@ +. + +/** + * Test iterator that skips future documents + * + * @package core_search + * @category test + * @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(); + +/** + * Test iterator that skips future documents + * + * @package core_search + * @category test + * @copyright 2017 The Open University + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class skip_future_documents_iterator_testcase extends \basic_testcase { + + /** + * Test normal case with all documents in the past. + */ + public function test_iterator_all_in_past() { + $past = strtotime('2017-11-01'); + $documents = [ + self::make_doc($past, 1), + self::make_doc($past + 1, 2), + self::make_doc($past + 2, 3) + ]; + $this->assertEquals('mod_x-frog-1.mod_x-frog-2.mod_x-frog-3.', + self::do_iterator($documents)); + } + + /** + * Confirm that the iterator does not call its parent iterator current() function too many + * times. + */ + public function test_iterator_performance() { + $counter = new test_counting_iterator(); + $iterator = new skip_future_documents_iterator($counter); + $items = 0; + foreach ($iterator as $value) { + $this->assertEquals(false, $value); + $items++; + } + $this->assertEquals(3, $items); + $this->assertEquals(3, $counter->get_count()); + } + + /** + * Test with no documents at all. + */ + public function test_iterator_empty() { + $this->assertEquals('', self::do_iterator([])); + } + + /** + * Test if some documents are in the future. + */ + public function test_iterator_some_in_future() { + $past = strtotime('2017-11-01'); + $future = time() + 1000; + $documents = [ + self::make_doc($past, 1), + self::make_doc($past + 1, 2), + self::make_doc($future, 3) + ]; + $this->assertEquals('mod_x-frog-1.mod_x-frog-2.', + self::do_iterator($documents)); + } + + /** + * Test if all documents are in the future. + */ + public function test_iterator_all_in_future() { + $future = time() + 1000; + $documents = [ + self::make_doc($future, 1), + self::make_doc($future + 1, 2), + self::make_doc($future + 2, 3) + ]; + $this->assertEquals('', self::do_iterator($documents)); + } + + /** + * Test when some documents return error. + */ + public function test_iterator_some_false() { + $past = strtotime('2017-11-01'); + $documents = [ + self::make_doc($past, 1), + false, + self::make_doc($past + 2, 3) + ]; + $this->assertEquals('mod_x-frog-1.false.mod_x-frog-3.', + self::do_iterator($documents)); + } + + /** + * Test when all documents return error. + */ + public function test_iterator_all_false() { + $documents = [ + false, + false, + false + ]; + $this->assertEquals('false.false.false.', + self::do_iterator($documents)); + } + + /** + * Test iterator with all cases. + */ + public function test_iterator_past_false_and_future() { + $past = strtotime('2017-11-01'); + $future = time() + 1000; + $documents = [ + false, + self::make_doc($past, 1), + false, + self::make_doc($past + 1, 2), + false, + self::make_doc($future, 3), + false + ]; + $this->assertEquals('false.mod_x-frog-1.false.mod_x-frog-2.false.', + self::do_iterator($documents)); + } + + /** + * Helper function to create a search document. + * + * @param int $time Modified time + * @param int $index Item id + * @return document Search document + */ + protected static function make_doc($time, $index) { + $doc = new document($index, 'mod_x', 'frog'); + $doc->set('modified', $time); + return $doc; + } + + /** + * Puts documents through the iterator and returns result as a string for easy testing. + * + * @param document[] $documents Array of documents + * @return string Documents converted to string + */ + protected static function do_iterator(array $documents) { + $parent = new \ArrayIterator($documents); + $iterator = new skip_future_documents_iterator($parent); + $result = ''; + foreach ($iterator as $rec) { + if (!$rec) { + $result .= 'false.'; + } else { + $result .= $rec->get('id') . '.'; + } + } + return $result; + } +} + +/** + * Fake iterator just for counting how many times current() is called. It returns 'false' 3 times. + * + * @package core_search + * @category test + * @copyright 2017 The Open University + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class test_counting_iterator implements \Iterator { + + /** @var int Current position in iterator */ + protected $pos = 0; + /** @var int Number of calls to current() function */ + protected $count = 0; + + /** + * Returns the current element. + * + * @return mixed Can return any type. + */ + public function current() { + $this->count++; + return false; + } + + /** + * Counts iterator usage. + * + * @return int Number of times current() was called + */ + public function get_count() { + return $this->count; + } + + /** + * Goes on to the next element. + */ + public function next() { + $this->pos++; + } + + /** + * Gets the key (not supported) + * + * @throws \coding_exception Always + */ + public function key() { + throw new \coding_exception('Unsupported'); + } + + /** + * Checks if iterato is valid (still has entries). + * + * @return bool True if still valid + */ + public function valid() { + return $this->pos < 3; + } + + /** + * Rewinds the iterator. + */ + public function rewind() { + $this->pos = 0; + } +}