Skip to content

Commit 7cf99c9

Browse files
thomascorthalsthomascorthals
authored
Bugfix for uninitialised position in PrefetchIterator (#902)
* Bugfix for unset position in PrefetchIterator * Fix misleading comments
1 parent 2b4639e commit 7cf99c9

4 files changed

Lines changed: 120 additions & 12 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
1111
- Helper::rangeQuery() now supports left-inclusive only and right-inclusive only queries
1212

1313
### Fixed
14+
- PrefetchIterator::key() should return 0 instead of NULL on a fresh PrefetchIterator
15+
- PrefetchIterator::next() shouldn't skip fetched results after PrefetchIterator::count() on a fresh PrefetchIterator
1416

1517
### Changed
1618
- Exception message for invalid/unavailable file in Extract query now contains filename

‎src/Plugin/PrefetchIterator.php‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ class PrefetchIterator extends AbstractPlugin implements \Iterator, \Countable
5858
*
5959
* @var int
6060
*/
61-
protected $position;
61+
protected $position = 0;
6262

6363
/**
6464
* Cursor mark.
@@ -173,7 +173,7 @@ public function rewind(): void
173173
{
174174
$this->position = 0;
175175

176-
// this condition prevent useless re-fetching of data if a count is done before the iterator is used
176+
// this condition prevents erroneously fetching the next set of results if a count is done before the iterator is used
177177
if ($this->start !== $this->options['prefetch']) {
178178
$this->start = 0;
179179

@@ -222,7 +222,7 @@ public function valid(): bool
222222
{
223223
$adjustedIndex = $this->position % $this->options['prefetch'];
224224

225-
// this condition prevent useless re-fetching of data if a count is done before the iterator is used
225+
// this condition prevents erroneously fetching the next set of results if a count is done before the iterator is used
226226
if (0 === $adjustedIndex && (0 !== $this->position || null === $this->result)) {
227227
$this->fetchNext();
228228
}
@@ -262,7 +262,7 @@ protected function fetchNext(): self
262262
*/
263263
protected function resetData(): self
264264
{
265-
$this->position = null;
265+
$this->position = 0;
266266
$this->result = null;
267267
$this->documents = null;
268268
$this->start = 0;

‎tests/Integration/AbstractTechproductsTest.php‎

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
use Solarium\Plugin\BufferedAdd\Event\PostFlush as BufferedAddPostFlushEvent;
2222
use Solarium\Plugin\BufferedAdd\Event\PreCommit as BufferedAddPreCommitEvent;
2323
use Solarium\Plugin\BufferedAdd\Event\PreFlush as BufferedAddPreFlushEvent;
24+
use Solarium\Plugin\PrefetchIterator;
2425
use Solarium\QueryType\ManagedResources\Query\Synonyms\Synonyms;
2526
use Solarium\QueryType\Select\Query\Query as SelectQuery;
2627
use Solarium\QueryType\Select\Result\Document;
@@ -1501,15 +1502,20 @@ function (BufferedAddPostCommitEvent $event) use ($bufferSize, $totalDocs) {
15011502
public function testPrefetchIterator()
15021503
{
15031504
$select = self::$client->createSelect();
1505+
$select->addSort('id', SelectQuery::SORT_ASC);
1506+
/** @var PrefetchIterator $prefetch */
15041507
$prefetch = self::$client->getPlugin('prefetchiterator');
15051508
$prefetch->setPrefetch(2);
15061509
$prefetch->setQuery($select);
15071510

1508-
// count() uses getNumFound() on the result set and wouldn't actually test if all results are iterated
1509-
for ($i = 0; $prefetch->valid(); ++$i) {
1510-
$prefetch->next();
1511-
}
1511+
// check upfront that all results are found
1512+
$this->assertCount(32, $prefetch);
15121513

1514+
// verify that each result is iterated in order
1515+
$id = '';
1516+
for ($i = 0; $prefetch->valid(); $prefetch->next(), ++$i) {
1517+
$this->assertLessThan(0, strcmp($id, $id = $prefetch->current()->id));
1518+
}
15131519
$this->assertSame(32, $i);
15141520
}
15151521

@@ -1518,22 +1524,27 @@ public function testPrefetchIteratorWithCursormark()
15181524
$select = self::$client->createSelect();
15191525
$select->setCursormark('*');
15201526
$select->addSort('id', SelectQuery::SORT_ASC);
1527+
/** @var PrefetchIterator $prefetch */
15211528
$prefetch = self::$client->getPlugin('prefetchiterator');
15221529
$prefetch->setPrefetch(2);
15231530
$prefetch->setQuery($select);
15241531

1525-
// count() uses getNumFound() on the result set and wouldn't actually test if all results are iterated
1526-
for ($i = 0; $prefetch->valid(); ++$i) {
1527-
$prefetch->next();
1528-
}
1532+
// check upfront that all results are found
1533+
$this->assertCount(32, $prefetch);
15291534

1535+
// verify that each result is iterated in order
1536+
$id = '';
1537+
for ($i = 0; $prefetch->valid(); $prefetch->next(), ++$i) {
1538+
$this->assertLessThan(0, strcmp($id, $id = $prefetch->current()->id));
1539+
}
15301540
$this->assertSame(32, $i);
15311541
}
15321542

15331543
public function testPrefetchIteratorWithoutAndWithCursormark()
15341544
{
15351545
$select = self::$client->createSelect();
15361546
$select->addSort('id', SelectQuery::SORT_ASC);
1547+
/** @var PrefetchIterator $prefetch */
15371548
$prefetch = self::$client->getPlugin('prefetchiterator');
15381549
$prefetch->setPrefetch(2);
15391550
$prefetch->setQuery($select);

‎tests/Plugin/PrefetchIteratorTest.php‎

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,72 @@ public function testCount()
6161
$this->assertCount(5, $this->plugin);
6262
}
6363

64+
public function testIteratorFlow()
65+
{
66+
$result = $this->getResult();
67+
$mockClient = $this->createMock(Client::class);
68+
69+
// Important: if prefetch or query settings are not changed, the query should be executed only once!
70+
$mockClient->expects($this->exactly(1))->method('execute')->willReturn($result);
71+
72+
$this->plugin->initPlugin($mockClient, []);
73+
$this->plugin->setQuery($this->query);
74+
75+
// run through the entire iterator manually
76+
$this->assertTrue($this->plugin->valid());
77+
$this->assertSame(['id' => 1, 'title' => 'doc1'], $this->plugin->current()->getFields());
78+
$this->assertSame(0, $this->plugin->key());
79+
$this->plugin->next();
80+
$this->assertTrue($this->plugin->valid());
81+
$this->assertSame(['id' => 2, 'title' => 'doc2'], $this->plugin->current()->getFields());
82+
$this->assertSame(1, $this->plugin->key());
83+
$this->plugin->next();
84+
$this->assertTrue($this->plugin->valid());
85+
$this->assertSame(['id' => 3, 'title' => 'doc3'], $this->plugin->current()->getFields());
86+
$this->assertSame(2, $this->plugin->key());
87+
$this->plugin->next();
88+
$this->assertTrue($this->plugin->valid());
89+
$this->assertSame(['id' => 4, 'title' => 'doc4'], $this->plugin->current()->getFields());
90+
$this->assertSame(3, $this->plugin->key());
91+
$this->plugin->next();
92+
$this->assertTrue($this->plugin->valid());
93+
$this->assertSame(['id' => 5, 'title' => 'doc5'], $this->plugin->current()->getFields());
94+
$this->assertSame(4, $this->plugin->key());
95+
$this->plugin->next();
96+
$this->assertFalse($this->plugin->valid());
97+
98+
// rewind at the end and partway through
99+
$this->plugin->rewind();
100+
$this->assertTrue($this->plugin->valid());
101+
$this->assertSame(['id' => 1, 'title' => 'doc1'], $this->plugin->current()->getFields());
102+
$this->assertSame(0, $this->plugin->key());
103+
$this->plugin->next();
104+
$this->assertTrue($this->plugin->valid());
105+
$this->assertSame(['id' => 2, 'title' => 'doc2'], $this->plugin->current()->getFields());
106+
$this->assertSame(1, $this->plugin->key());
107+
$this->plugin->rewind();
108+
$this->assertTrue($this->plugin->valid());
109+
$this->assertSame(['id' => 1, 'title' => 'doc1'], $this->plugin->current()->getFields());
110+
$this->assertSame(0, $this->plugin->key());
111+
}
112+
113+
public function testIteratorEmptyResultFlow()
114+
{
115+
$result = $this->getEmptyResult();
116+
$mockClient = $this->createMock(Client::class);
117+
118+
// Important: if prefetch or query settings are not changed, the query should be executed only once!
119+
$mockClient->expects($this->exactly(1))->method('execute')->willReturn($result);
120+
121+
$this->plugin->initPlugin($mockClient, []);
122+
$this->plugin->setQuery($this->query);
123+
124+
// there is nothing to run through
125+
$this->assertFalse($this->plugin->valid());
126+
$this->plugin->rewind();
127+
$this->assertFalse($this->plugin->valid());
128+
}
129+
64130
public function testIteratorAndRewind()
65131
{
66132
$result = $this->getResult();
@@ -139,6 +205,26 @@ public function testIteratorResetOnSetQuery()
139205
$this->assertSame($result->getDocuments(), $results2);
140206
}
141207

208+
public function testIteratorEmptyResult()
209+
{
210+
$result = $this->getEmptyResult();
211+
$mockClient = $this->createMock(Client::class);
212+
213+
// Important: if prefetch or query settings are not changed, the query should be executed only once!
214+
$mockClient->expects($this->exactly(1))->method('execute')->willReturn($result);
215+
216+
$this->plugin->initPlugin($mockClient, []);
217+
$this->plugin->setQuery($this->query);
218+
219+
$results = [];
220+
foreach ($this->plugin as $doc) {
221+
$results[] = $doc;
222+
}
223+
224+
$this->assertCount(0, $this->plugin);
225+
$this->assertSame([], $results);
226+
}
227+
142228
public function getResult()
143229
{
144230
$numFound = 5;
@@ -154,6 +240,15 @@ public function getResult()
154240
return new SelectDummy(1, 12, $numFound, $docs, []);
155241
}
156242

243+
public function getEmptyResult()
244+
{
245+
$numFound = 0;
246+
247+
$docs = [];
248+
249+
return new SelectDummy(1, 2, $numFound, $docs, []);
250+
}
251+
157252
public function testSetAndGetEndpointAsString()
158253
{
159254
$this->assertNull($this->plugin->getEndpoint());

0 commit comments

Comments
 (0)