fix: make Buffer::nth() positional so it stays correct after deletions
Buffer::nth() resolved the requested position by adding it to the first row key and indexing the rows array by that computed key. That assumes contiguous keys, but Buffer::delete() removes rows with unset() without reindexing, leaving gaps. After a deletion nth() therefore returned the wrong record (or an empty array for a valid position, or a record for an out-of-range position). Resolve the position against the record order with array_values() so nth() matches getRecords() and ResultSet::nth().
This commit is contained in:
1 parent
4354a49e5b
commit
aa4f462b2e
3 files changed
+116
-9
No files matched your search
@@ -0,0 +1,94 @@
|
||||
# fix: make Buffer::nth() positional so it stays correct after deletions
|
||||
|
||||
## Problem
|
||||
|
||||
`Buffer::nth()` (and `Buffer::nthAsObject()`) return the wrong record once a
|
||||
record has been deleted from the buffer. `nth()` is a positional accessor - the
|
||||
same contract implemented by `ResultSet::nth()` via
|
||||
`new LimitIterator($iterator, $nth, 1)` - so `nth($n)` must return the `$n`-th
|
||||
record in iteration order, consistent with `getRecords()`.
|
||||
|
||||
```php
|
||||
$buffer = new League\Csv\Buffer(['id', 'name']);
|
||||
$buffer->insert(['1', 'a'], ['2', 'b'], ['3', 'c'], ['4', 'd'], ['5', 'e']);
|
||||
$buffer->delete(fn (array $row) => $row['id'] === '2');
|
||||
|
||||
// getRecords() correctly yields a, c, d, e
|
||||
// but:
|
||||
$buffer->nth(1); // [] (expected ['id' => '3', 'name' => 'c'])
|
||||
$buffer->nth(4); // ['5', 'e'] (expected [] - only 4 records remain)
|
||||
```
|
||||
|
||||
## Root cause
|
||||
|
||||
`src/Buffer.php`, `nthRow()`:
|
||||
|
||||
```php
|
||||
$offset = $first + $nth;
|
||||
if (!array_key_exists($offset, $this->rows)) {
|
||||
return [];
|
||||
}
|
||||
|
||||
return $this->rows[$nth + $first];
|
||||
```
|
||||
|
||||
The lookup adds `$nth` to the first row key and indexes `$this->rows` by that
|
||||
computed key. That only works while the row keys are contiguous.
|
||||
`Buffer::delete()` removes rows with `unset($this->rows[$offset])` without
|
||||
reindexing, so after any deletion the keys contain gaps and the positional index
|
||||
`$nth` no longer maps to key `$first + $nth`. As a result `nth()` skips the
|
||||
deleted offset, and can also return a record for an index that is out of range.
|
||||
|
||||
## The fix
|
||||
|
||||
Resolve `$nth` against the record order instead of the row keys:
|
||||
|
||||
```php
|
||||
private function nthRow(int $nth, string $method): array
|
||||
{
|
||||
-1 < $nth || throw InvalidArgument::dueToInvalidRecordOffset($nth, $method);
|
||||
|
||||
return array_values($this->rows)[$nth] ?? [];
|
||||
}
|
||||
```
|
||||
|
||||
`array_values()` collapses the gaps, so `$nth` addresses the record by its
|
||||
position in iteration order, matching `getRecords()` and `ResultSet::nth()`.
|
||||
`array_values` is already imported by the class.
|
||||
|
||||
## Test
|
||||
|
||||
Added `BufferTest::it_returns_records_by_positional_offset_after_a_middle_record_is_deleted`,
|
||||
which inserts five records, deletes a middle one, and asserts that
|
||||
`nth(0..3)` return the four remaining records in order and that `nth(4)` returns
|
||||
`[]`.
|
||||
|
||||
## Gate
|
||||
|
||||
```
|
||||
composer install --ignore-platform-req=ext-xdebug
|
||||
vendor/bin/phpunit --no-coverage
|
||||
```
|
||||
|
||||
The full run reports 6 pre-existing errors, all
|
||||
`PDOException: could not find driver` from the SQLite3/PDO-backed tests
|
||||
(`RdbmsResultTest` and the `sqlite3`/`pdo` cases in `BufferTest`); these are
|
||||
environmental (the `sqlite3` / `pdo_sqlite` extensions are not installed) and are
|
||||
present unchanged on a clean checkout. Excluding those environment-dependent
|
||||
cases the suite is green:
|
||||
|
||||
```
|
||||
vendor/bin/phpunit --no-coverage --filter '/^(?!.*(sqlite3|pdo)).*$/i'
|
||||
```
|
||||
|
||||
Result: exit code `0` - `OK, but some tests were skipped! Tests: 1006,
|
||||
Assertions: 1990, Skipped: 3` (the 3 skips are pre-existing).
|
||||
|
||||
Counterfactual: reverting `nthRow()` to the key-arithmetic version makes the new
|
||||
test fail - `nth(1)` returns `[]` instead of `['id' => '3', 'name' => 'c']`.
|
||||
|
||||
## Flags for the account owner
|
||||
|
||||
The SQLite3/PDO tests are not annotated with a required-extension attribute, so
|
||||
they error (rather than skip) when the drivers are missing. That is unrelated to
|
||||
this change and was left untouched.
|
||||
+4
-9
@@ -335,16 +335,11 @@ final class Buffer implements TabularData
|
||||
private function nthRow(int $nth, string $method): array
|
||||
{
|
||||
-1 < $nth || throw InvalidArgument::dueToInvalidRecordOffset($nth, $method);
|
||||
if (null === ($first = $this->firstOffset())) {
|
||||
return [];
|
||||
}
|
||||
|
||||
$offset = $first + $nth;
|
||||
if (!array_key_exists($offset, $this->rows)) {
|
||||
return [];
|
||||
}
|
||||
|
||||
return $this->rows[$nth + $first];
|
||||
// $nth is a positional index (like ResultSet::nth), so it must be
|
||||
// resolved against the record order rather than the row keys, which
|
||||
// may contain gaps after records have been deleted.
|
||||
return array_values($this->rows)[$nth] ?? [];
|
||||
}
|
||||
|
||||
public function fetchColumn(int|string $index = 0): Iterator
|
||||
|
||||
@@ -285,6 +285,24 @@ final class BufferTest extends TestCase
|
||||
self::assertSame(['date' => '2025-02-08', 'temperature' => '3', 'place' => 'Berkeley'], $buffer->nth(0));
|
||||
}
|
||||
|
||||
#[Test]
|
||||
public function it_returns_records_by_positional_offset_after_a_middle_record_is_deleted(): void
|
||||
{
|
||||
$buffer = new Buffer(['id', 'name']);
|
||||
$buffer->insert(['1', 'a'], ['2', 'b'], ['3', 'c'], ['4', 'd'], ['5', 'e']);
|
||||
|
||||
// Delete the second record (id = 2), which leaves a gap in the row keys.
|
||||
self::assertSame(1, $buffer->delete(fn (array $row): bool => $row['id'] === '2'));
|
||||
self::assertSame(4, $buffer->recordCount());
|
||||
|
||||
// nth() is positional and must stay consistent with getRecords().
|
||||
self::assertSame(['id' => '1', 'name' => 'a'], $buffer->nth(0));
|
||||
self::assertSame(['id' => '3', 'name' => 'c'], $buffer->nth(1));
|
||||
self::assertSame(['id' => '4', 'name' => 'd'], $buffer->nth(2));
|
||||
self::assertSame(['id' => '5', 'name' => 'e'], $buffer->nth(3));
|
||||
self::assertSame([], $buffer->nth(4));
|
||||
}
|
||||
|
||||
#[Test]
|
||||
public function it_can_not_insert_invalid_records(): void
|
||||
{
|
||||
|
||||
Reference in new issue
Block a user