Skip to content

Commit 459db08

Browse files
committed
fix iterator bug
1 parent 454538d commit 459db08

7 files changed

Lines changed: 420 additions & 16 deletions

File tree

code/SQLite3Query.php

Lines changed: 65 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,22 @@ class SQLite3Query extends Query
2626
protected $handle;
2727

2828
/**
29-
* Hook the result-set given into a Query class, suitable for use by framework.
30-
* @param SQLite3Connector $database The database object that created this query.
31-
* @param SQLite3Result $handle the internal sqlite3 handle that is points to the resultset.
29+
* Buffered rows. Acts as a cache to avoid double-fetching and to allow re-iteration.
30+
*
31+
* @var array
32+
*/
33+
private array $rows = [];
34+
35+
/**
36+
* Whether the native handle has reached EOF. Prevents SQLite from wrapping back to the first row.
37+
*
38+
* @var bool
39+
*/
40+
private bool $exhausted = false;
41+
42+
/**
43+
* @param SQLite3Connector $database
44+
* @param SQLite3Result $handle
3245
*/
3346
public function __construct(SQLite3Connector $database, SQLite3Result $handle)
3447
{
@@ -45,27 +58,65 @@ public function __destruct()
4558

4659
/**
4760
* @todo This looks terrible but there is no SQLite3::get_num_rows() implementation
61+
*
62+
* Drains any remaining rows into the buffer and returns the total count.
4863
*/
4964
public function numRecords()
5065
{
51-
// Some queries are not iterable using fetchArray like CREATE statement
52-
if (!$this->handle->numColumns()) {
53-
return 0;
66+
$this->loadAllRows();
67+
68+
return count($this->rows);
69+
}
70+
71+
/**
72+
* Drains the native handle into the local buffer until EOF.
73+
*/
74+
private function loadAllRows(): void
75+
{
76+
// Some queries are not iterable using fetchArray like CREATE statement.
77+
if ($this->exhausted || !$this->handle->numColumns()) {
78+
$this->exhausted = true;
79+
return;
5480
}
5581

56-
$this->handle->reset();
57-
$c = 0;
58-
while ($this->handle->fetchArray()) {
59-
$c++;
82+
while ($row = $this->handle->fetchArray(SQLITE3_ASSOC)) {
83+
$this->rows[] = $row;
6084
}
61-
$this->handle->reset();
62-
return $c;
85+
86+
// SQLite restarts from the first row after EOF, so never fetch again once exhausted.
87+
$this->exhausted = true;
6388
}
6489

90+
/**
91+
* Yields rows from the buffer first, then fetches incrementally from the native handle.
92+
* Fetched rows are buffered to allow re-iteration and to avoid double-fetch bugs.
93+
*/
6594
public function getIterator(): Traversable
6695
{
67-
while ($data = $this->handle->fetchArray(SQLITE3_ASSOC)) {
68-
yield $data;
96+
$index = 0;
97+
98+
while (true) {
99+
if (array_key_exists($index, $this->rows)) {
100+
yield $this->rows[$index];
101+
$index++;
102+
continue;
103+
}
104+
105+
if ($this->exhausted || !$this->handle->numColumns()) {
106+
$this->exhausted = true;
107+
return;
108+
}
109+
110+
$row = $this->handle->fetchArray(SQLITE3_ASSOC);
111+
if ($row === false) {
112+
// SQLite restarts from the first row after EOF, so never fetch again once exhausted.
113+
$this->exhausted = true;
114+
return;
115+
}
116+
117+
$this->rows[] = $row;
118+
yield $row;
119+
$index++;
69120
}
70121
}
71122
}

composer.json

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,9 +32,14 @@
3232
"SilverStripe\\SQLite\\": "code/"
3333
}
3434
},
35+
"autoload-dev": {
36+
"psr-4": {
37+
"SilverStripe\\SQLite\\Tests\\": "tests/php/"
38+
}
39+
},
3540
"scripts": {
36-
"lint": "phpcs code/",
37-
"lint-clean": "phpcbf code/",
41+
"lint": "phpcs",
42+
"lint-clean": "phpcbf",
3843
"test": "DB=SQLITE vendor/bin/phpunit --testsuite framework",
3944
"test-orm": "DB=SQLITE vendor/bin/phpunit vendor/silverstripe/framework/tests/php/ORM/",
4045
"test-connect": "DB=SQLITE vendor/bin/phpunit vendor/silverstripe/framework/tests/php/ORM/Connect/",

phpcs.xml.dist

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
<description>CodeSniffer ruleset for SilverStripe coding conventions.</description>
44

55
<file>code</file>
6+
<file>tests/php</file>
67

78
<!-- base rules are PSR-2 -->
89
<rule ref="PSR12">

phpunit.xml.dist

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,5 +4,8 @@
44
<testsuite name="framework">
55
<directory>vendor/silverstripe/framework/tests/php</directory>
66
</testsuite>
7+
<testsuite name="sqlite3">
8+
<directory>tests/php</directory>
9+
</testsuite>
710
</testsuites>
811
</phpunit>
Lines changed: 231 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,231 @@
1+
<?php
2+
3+
namespace SilverStripe\SQLite\Tests;
4+
5+
use SilverStripe\Dev\SapphireTest;
6+
use SilverStripe\ORM\DataObject;
7+
use SilverStripe\ORM\DB;
8+
use SilverStripe\Core\Injector\Injector;
9+
use SilverStripe\View\TemplateEngine;
10+
use SilverStripe\View\ViewLayerData;
11+
12+
/**
13+
* Tests for SQLite3 Query Iterator behavior
14+
*
15+
* These tests verify proper DataList iteration behavior with SQLite3.
16+
* They serve as regression tests for potential iterator issues.
17+
*/
18+
class SQLite3QueryIteratorTest extends SapphireTest
19+
{
20+
protected static $fixture_file = null;
21+
22+
protected static $extra_dataobjects = [
23+
TestDataObject::class,
24+
];
25+
26+
protected static $required_extensions = [
27+
TestDataObject::class => [],
28+
];
29+
30+
protected function setUp(): void
31+
{
32+
parent::setUp();
33+
34+
// These tests require SQLite - skip if not using SQLite
35+
$dbClass = DB::get_conn();
36+
if (!$dbClass || !($dbClass instanceof \SilverStripe\SQLite\SQLite3Database)) {
37+
$this->markTestSkipped('This test requires SQLite3 database (use DB=SQLITE)');
38+
}
39+
}
40+
41+
/**
42+
* Test that iterating over a result doesn't duplicate items on rewind
43+
*
44+
* This replicates the bug from GitHub Issue #73 where a single record
45+
* in a DataList would be displayed twice in templates due to iterator issues.
46+
*/
47+
public function testIteratorDoesNotDuplicateItemsOnRewind()
48+
{
49+
// Create a simple test DataObject
50+
$obj = new TestDataObject();
51+
$obj->Title = 'Test Record';
52+
$obj->write();
53+
54+
// Get the record as a DataList (simulating what happens in templates)
55+
$list = TestDataObject::get()->filter('ID', $obj->ID);
56+
57+
// Count should be 1
58+
$this->assertEquals(1, $list->count(), 'List should contain exactly one record');
59+
60+
// First iteration - collect items
61+
$firstPass = [];
62+
foreach ($list as $item) {
63+
$firstPass[] = $item->ID;
64+
}
65+
66+
// Second iteration (simulating what SSViewer_Scope does with rewind)
67+
// This is where the bug would cause duplication
68+
$secondPass = [];
69+
foreach ($list as $item) {
70+
$secondPass[] = $item->ID;
71+
}
72+
73+
// Both passes should have the same single item
74+
$this->assertCount(1, $firstPass, 'First iteration should have exactly one item');
75+
$this->assertCount(1, $secondPass, 'Second iteration should have exactly one item (bug would show 2)');
76+
$this->assertEquals($firstPass, $secondPass, 'Both iterations should return same items');
77+
$this->assertEquals($obj->ID, $firstPass[0], 'First pass should contain the test record ID');
78+
$this->assertEquals($obj->ID, $secondPass[0], 'Second pass should contain the test record ID');
79+
80+
// Clean up
81+
$obj->delete();
82+
}
83+
84+
/**
85+
* Test that numRecords doesn't interfere with iteration
86+
*
87+
* Calling count() before iteration used to cause the iterator to start
88+
* from the wrong position due to the reset() call in numRecords().
89+
*/
90+
public function testCountBeforeIterationDoesNotCauseDuplication()
91+
{
92+
// Create multiple test records
93+
$ids = [];
94+
for ($i = 1; $i <= 3; $i++) {
95+
$obj = new TestDataObject();
96+
$obj->Title = "Record $i";
97+
$obj->write();
98+
$ids[] = $obj->ID;
99+
}
100+
101+
$list = TestDataObject::get()->sort('ID');
102+
103+
// Call count() first (this used to trigger the bug)
104+
$count = $list->count();
105+
$this->assertEquals(3, $count, 'Count should return 3');
106+
107+
// Now iterate - before the fix, this would show items twice
108+
$iteratedIds = [];
109+
foreach ($list as $item) {
110+
$iteratedIds[] = $item->ID;
111+
}
112+
113+
// Should have exactly 3 items, not 6
114+
$this->assertCount(3, $iteratedIds, 'Should iterate exactly 3 items, not duplicated');
115+
$this->assertEquals($ids, $iteratedIds, 'Iterated IDs should match created IDs');
116+
117+
// Clean up
118+
foreach ($list as $obj) {
119+
$obj->delete();
120+
}
121+
}
122+
123+
/**
124+
* Test that SQLite3Query::numRecords() doesn't duplicate rows on later iteration
125+
*/
126+
public function testDirectQueryCountBeforeIterationDoesNotDuplicateRows()
127+
{
128+
$ids = [];
129+
for ($i = 1; $i <= 3; $i++) {
130+
$obj = new TestDataObject();
131+
$obj->Title = "Direct Record $i";
132+
$obj->write();
133+
$ids[] = $obj->ID;
134+
}
135+
136+
$query = DB::query(
137+
sprintf(
138+
'SELECT "ID" FROM "%s" WHERE "ID" IN (%s) ORDER BY "ID"',
139+
TestDataObject::config()->get('table_name'),
140+
implode(', ', $ids)
141+
)
142+
);
143+
144+
$this->assertEquals(3, $query->numRecords(), 'Direct query count should return 3');
145+
146+
$iteratedIds = [];
147+
foreach ($query as $row) {
148+
$iteratedIds[] = (int) $row['ID'];
149+
}
150+
151+
$this->assertSame($ids, $iteratedIds, 'Direct query iteration should return each row once');
152+
153+
TestDataObject::get()->filter('ID', $ids)->removeAll();
154+
}
155+
156+
/**
157+
* Test iterator with single item (the exact scenario from Issue #73)
158+
*/
159+
public function testSingleItemNotDuplicated()
160+
{
161+
$obj = new TestDataObject();
162+
$obj->Title = 'Single Item';
163+
$obj->write();
164+
165+
$list = TestDataObject::get()->filter('ID', $obj->ID);
166+
167+
// Simulate template behavior: check count, then iterate
168+
$count = $list->count();
169+
$this->assertEquals(1, $count);
170+
171+
// Get iterator and convert to array
172+
$items = [];
173+
foreach ($list as $item) {
174+
$items[] = $item;
175+
}
176+
177+
// Should have exactly 1 item (no duplicates)
178+
$this->assertCount(1, $items, 'Single item list should not duplicate');
179+
180+
$obj->delete();
181+
}
182+
183+
/**
184+
* Test DataList iteration through template rendering (SS6 API)
185+
*
186+
* This test uses the SilverStripe 6 TemplateEngine API to render a template
187+
* with a DataList loop. It verifies that items appear exactly once in the
188+
* rendered output, not duplicated.
189+
*
190+
* In SS6, TemplateEngine::renderString() replaces SSViewer::fromString()
191+
*/
192+
public function testTemplateLoopDoesNotDuplicateItems()
193+
{
194+
// Create test records
195+
$expectedTitles = [];
196+
for ($i = 1; $i <= 3; $i++) {
197+
$obj = new TestDataObject();
198+
$obj->Title = "Record $i";
199+
$obj->write();
200+
$expectedTitles[] = "Record $i";
201+
}
202+
203+
// Get DataList
204+
$list = TestDataObject::get()->sort('ID');
205+
206+
// Create template with loop using SS6 TemplateEngine API
207+
$template = '<% loop List %>[{$Title}]<% end_loop %>';
208+
209+
// Render using TemplateEngine (SS6 way)
210+
$engine = Injector::inst()->create(TemplateEngine::class);
211+
$model = new ViewLayerData(['List' => $list]);
212+
$result = $engine->renderString($template, $model);
213+
214+
// Parse the result to extract titles
215+
preg_match_all('/\[([^\]]+)\]/', $result, $matches);
216+
$renderedTitles = $matches[1] ?? [];
217+
218+
// Verify we got exactly 3 items, not 6 (duplicated)
219+
$this->assertCount(
220+
3,
221+
$renderedTitles,
222+
'Template should render exactly 3 items, not duplicated. Got: ' . json_encode($renderedTitles)
223+
);
224+
$this->assertEquals($expectedTitles, $renderedTitles, 'Rendered titles should match expected');
225+
226+
// Clean up
227+
foreach ($list as $obj) {
228+
$obj->delete();
229+
}
230+
}
231+
}

0 commit comments

Comments
 (0)