Skip to content

Commit

Permalink
MDL-60720 core_search: Indexing halts on failed get_document
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@sammarshallou
sammarshallou committed Nov 7, 2017
1 parent 159b4e5 commit 4b0facc
Show file tree
Hide file tree
Showing 2 changed files with 267 additions and 5 deletions.
22 changes: 17 additions & 5 deletions search/classes/skip_future_documents_iterator.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand All @@ -62,11 +68,16 @@ public function __construct(\Iterator $parent, $cutoff = 0) {
}

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() {
Expand All @@ -79,16 +90,17 @@ public function valid() {
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;
}
}
250 changes: 250 additions & 0 deletions search/tests/skip_future_documents_iterator_test.php
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,250 @@
<?php
// This file is part of Moodle - http://moodle.org/
//
// Moodle is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// Moodle is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.
//
// You should have received a copy of the GNU General Public License
// along with Moodle. If not, see <http://www.gnu.org/licenses/>.

/**
* 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;
}
}

0 comments on commit 4b0facc

Please sign in to comment.