Skip to content

Commit bb10697

Browse files
abnegateclaude
andcommitted
fix(find): return only the selected attributes of a distinct row
find() decoded every row with the read's selections, to which validateSelections() always appended $id, $sequence, $collection, $createdAt, $updatedAt and $permissions so a document read with a select still carries them. A distinct read selects only what its select names, so decode() set each of those it never read to null, and a distinct row carried `$id => null`. The library handed out a Document its own constructor refuses: `new Document($row->getArrayCopy())` threw "$id must be of type string", which is how Appwrite's list cache rebuilt a cached distinct read and answered 400 on every cache hit. A distinct read now decodes only the attributes it selects. A distinct row carries $id only when the read selects it, and every row find() returns rebuilds into an equal Document. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 09a4729 commit bb10697

2 files changed

Lines changed: 103 additions & 2 deletions

File tree

‎src/Database/Trait/Documents.php‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4339,7 +4339,7 @@ private function fetch(Collection $collection, array $queries, PermissionType $f
43394339

43404340
$queries = $this->readQueries($collection, $parsed, $joinedByAlias, $outerJoinIds);
43414341

4342-
$selections = $this->validateSelections($collection, $selects);
4342+
$selections = $this->validateSelections($collection, $selects, $distinct);
43434343

43444344
if ($isAggregation) {
43454345
$nestedSelections = [];
@@ -5958,10 +5958,13 @@ private function outerJoinIdSelections(array $selects, array $joins, array $join
59585958
}
59595959

59605960
/**
5961+
* The attributes a read returns: those its selects name and, unless it is distinct, the internal attributes every
5962+
* document carries. A distinct row is not a stored document, so it carries only the attributes it selects.
5963+
*
59615964
* @param array<Query> $queries
59625965
* @return array<string>
59635966
*/
5964-
private function validateSelections(Document $collection, array $queries): array
5967+
private function validateSelections(Document $collection, array $queries, bool $distinct = false): array
59655968
{
59665969
if (empty($queries)) {
59675970
return [];
@@ -6011,6 +6014,10 @@ private function validateSelections(Document $collection, array $queries): array
60116014

60126015
$selections = \array_merge($selections, $relationshipSelections);
60136016

6017+
if ($distinct) {
6018+
return \array_values(\array_unique($selections));
6019+
}
6020+
60146021
$selections[] = Document::ID;
60156022
$selections[] = Document::SEQUENCE;
60166023
$selections[] = Document::COLLECTION;

‎tests/e2e/Adapter/Scopes/DocumentTests.php‎

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11285,6 +11285,100 @@ public function testIncreaseAndDecreaseRoundTheChangeValueOnAnInteger(): void
1128511285
}
1128611286
}
1128711287

11288+
/**
11289+
* A distinct row carried `$id`, `$sequence`, `$createdAt` and `$updatedAt` as null whenever the read did not
11290+
* select them, so `new Document($row->getArrayCopy())` refused it: "$id must be of type string". A distinct row
11291+
* carries only what it selects, and every row a read returns rebuilds into an equal Document.
11292+
*/
11293+
public function testDistinctRowsCarryOnlyWhatTheySelectAndRebuildIntoADocument(): void
11294+
{
11295+
/** @var Database $database */
11296+
$database = $this->getDatabase();
11297+
11298+
$collection = 'distinct_rows_'.\substr(\uniqid(), -6);
11299+
$database->createCollection(Collection::create(
11300+
id: $collection,
11301+
attributes: [
11302+
Attribute::string(key: 'k', size: 32),
11303+
Attribute::integer(key: 'n'),
11304+
],
11305+
permissions: [Permission::create(Role::any()), Permission::read(Role::any())],
11306+
));
11307+
foreach ([['a', 1], ['a', 1], ['b', 2], [null, null]] as [$k, $n]) {
11308+
$database->createDocument($collection, new Document(['k' => $k, 'n' => $n, '$permissions' => [Permission::read(Role::any())]]));
11309+
}
11310+
11311+
$roundTrip = function (array $rows, string $case): void {
11312+
foreach ($rows as $row) {
11313+
$this->assertInstanceOf(Document::class, $row);
11314+
$this->assertEquals($row->getArrayCopy(), (new Document($row->getArrayCopy()))->getArrayCopy(), $case);
11315+
}
11316+
};
11317+
11318+
if (! $database->getAdapter()->supports(Capability::Aggregations)) {
11319+
try {
11320+
$database->find($collection, [Query::distinct(), Query::select(['k'])]);
11321+
$this->fail('A distinct() read is refused where the adapter cannot deduplicate rows');
11322+
} catch (QueryException) {
11323+
$this->addToAssertionCount(1);
11324+
}
11325+
11326+
$roundTrip($database->find($collection, [Query::select(['k'])]), 'a read of selected attributes');
11327+
$database->deleteCollection($collection);
11328+
11329+
return;
11330+
}
11331+
11332+
$values = $database->find($collection, [Query::distinct(), Query::select(['k'])]);
11333+
$roundTrip($values, 'distinct without $id');
11334+
$copies = \array_map(static fn (Document $row): array => $row->getArrayCopy(), $values);
11335+
\usort($copies, static fn (array $a, array $b): int => [$a['k'] !== null, $a['k']] <=> [$b['k'] !== null, $b['k']]);
11336+
$this->assertSame(
11337+
[['k' => null, '$collection' => $collection], ['k' => 'a', '$collection' => $collection], ['k' => 'b', '$collection' => $collection]],
11338+
$copies,
11339+
);
11340+
11341+
$pairs = $database->find($collection, [Query::distinct(), Query::select(['k', 'n']), Query::orderAsc('k'), Query::orderAsc('n')]);
11342+
$this->assertCount(3, $pairs);
11343+
foreach ($pairs as $row) {
11344+
$this->assertFalse($row->offsetExists('$id'));
11345+
$this->assertFalse($row->offsetExists('$sequence'));
11346+
}
11347+
$roundTrip($pairs, 'distinct of two attributes');
11348+
11349+
$stamped = $database->find($collection, [Query::distinct(), Query::select(['k', '$createdAt'])]);
11350+
$this->assertCount(4, $stamped);
11351+
foreach ($stamped as $row) {
11352+
$this->assertFalse($row->offsetExists('$id'));
11353+
$this->assertIsString($row->getAttribute('$createdAt'));
11354+
}
11355+
$roundTrip($stamped, 'distinct of an internal attribute but $id');
11356+
11357+
$identified = $database->find($collection, [Query::distinct(), Query::select(['k', '$id'])]);
11358+
$this->assertCount(4, $identified);
11359+
foreach ($identified as $row) {
11360+
$this->assertIsString($row->getAttribute('$id'));
11361+
$this->assertNotSame('', $row->getAttribute('$id'));
11362+
}
11363+
$roundTrip($identified, 'distinct with $id');
11364+
11365+
$paged = [Query::distinct(), Query::select(['k']), Query::isNotNull('k'), Query::orderAsc('k'), Query::limit(1)];
11366+
$first = $database->find($collection, $paged);
11367+
$next = $database->find($collection, [...$paged, Query::cursorAfter($first[0])]);
11368+
$this->assertSame(['a'], \array_map(static fn (Document $row): mixed => $row->getAttribute('k'), $first));
11369+
$this->assertSame(['b'], \array_map(static fn (Document $row): mixed => $row->getAttribute('k'), $next));
11370+
$roundTrip([...$first, ...$next], 'distinct pages');
11371+
11372+
$documents = $database->find($collection, [Query::select(['k'])]);
11373+
$this->assertCount(4, $documents);
11374+
foreach ($documents as $document) {
11375+
$this->assertIsString($document->getAttribute('$id'));
11376+
}
11377+
$roundTrip($documents, 'a read of selected attributes');
11378+
11379+
$database->deleteCollection($collection);
11380+
}
11381+
1128811382
public function testDistinctIsRefusedWhereTheAdapterCannotDeduplicate(): void
1128911383
{
1129011384
/** @var Database $database */

0 commit comments

Comments
 (0)