Skip to content

Revert relationship max queries #973

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
fogelito wants to merge 1 commit into from
Closed

Revert relationship max queries #973

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions src/Database/Database.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ class Database
public const RELATION_SIDE_CHILD = 'child';

public const RELATION_MAX_DEPTH = 3;
public const RELATION_QUERY_CHUNK_SIZE = 5000;

// Orders
public const ORDER_ASC = 'ASC';
Expand Down Expand Up @@ -5324,7 +5325,7 @@ private function populateOneToOneRelationshipsBatch(array $documents, Document $
$relatedDocuments = [];

// Process in chunks to avoid exceeding query value limits
foreach (\array_chunk($uniqueRelatedIds, \max(1, $this->maxQueryValues)) as $chunk) {
foreach (\array_chunk($uniqueRelatedIds, self::RELATION_QUERY_CHUNK_SIZE) as $chunk) {

@greptile-apps greptile-apps Bot Sep 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Configured query limit ignored

When setMaxQueryValues(5) is used, populating six or more unique relationship IDs now creates a chunk larger than the configured limit. The internal find() call validates that chunk against the same limit and throws QueryException, so an otherwise valid relationship read fails instead of being split into executable queries. The same fixed chunk size is used in the other relationship-loading paths at lines 5420, 5517, 5596, and 5626.

Knowledge Base Used:

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/Database/Database.php
Line: 5328

Comment:
**Configured query limit ignored**

When `setMaxQueryValues(5)` is used, populating six or more unique relationship IDs now creates a chunk larger than the configured limit. The internal `find()` call validates that chunk against the same limit and throws `QueryException`, so an otherwise valid relationship read fails instead of being split into executable queries. The same fixed chunk size is used in the other relationship-loading paths at lines 5420, 5517, 5596, and 5626.

**Knowledge Base Used:**
- [Query construction and execution](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/utopia-php/database/-/docs/query-execution.md)
- [Storage adapter architecture](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/utopia-php/database/-/docs/adapter-architecture.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Claude Code Fix in Codex

$chunkDocs = $this->find($relatedCollection->getId(), [
Query::equal('$id', $chunk),
Query::limit(PHP_INT_MAX),
Expand Down Expand Up @@ -5416,7 +5417,7 @@ private function populateOneToManyRelationshipsBatch(

$relatedDocuments = [];

foreach (\array_chunk($parentIds, \max(1, $this->maxQueryValues)) as $chunk) {
foreach (\array_chunk($parentIds, self::RELATION_QUERY_CHUNK_SIZE) as $chunk) {
$chunkDocs = $this->find($relatedCollection->getId(), [
Query::equal($twoWayKey, $chunk),
Query::limit(PHP_INT_MAX),
Expand Down Expand Up @@ -5513,7 +5514,7 @@ private function populateManyToOneRelationshipsBatch(

$relatedDocuments = [];

foreach (\array_chunk($childIds, \max(1, $this->maxQueryValues)) as $chunk) {
foreach (\array_chunk($childIds, self::RELATION_QUERY_CHUNK_SIZE) as $chunk) {
$chunkDocs = $this->find($relatedCollection->getId(), [
Query::equal($twoWayKey, $chunk),
Query::limit(PHP_INT_MAX),
Expand Down Expand Up @@ -5592,7 +5593,7 @@ private function populateManyToManyRelationshipsBatch(

$junctions = [];

foreach (\array_chunk($documentIds, \max(1, $this->maxQueryValues)) as $chunk) {
foreach (\array_chunk($documentIds, self::RELATION_QUERY_CHUNK_SIZE) as $chunk) {
$chunkJunctions = $this->skipRelationships(fn () => $this->find($junction, [
Query::equal($twoWayKey, $chunk),
Query::limit(PHP_INT_MAX)
Expand Down Expand Up @@ -5622,7 +5623,7 @@ private function populateManyToManyRelationshipsBatch(
$uniqueRelatedIds = array_unique($relatedIds);
$foundRelated = [];

foreach (\array_chunk($uniqueRelatedIds, \max(1, $this->maxQueryValues)) as $chunk) {
foreach (\array_chunk($uniqueRelatedIds, self::RELATION_QUERY_CHUNK_SIZE) as $chunk) {
$chunkDocs = $this->find($relatedCollection->getId(), [
Query::equal('$id', $chunk),
Query::limit(PHP_INT_MAX),
Expand Down
85 changes: 0 additions & 85 deletions tests/e2e/Adapter/Scopes/RelationshipTests.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,91 +26,6 @@ trait RelationshipTests
use ManyToOneTests;

@greptile-apps greptile-apps Bot Sep 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Behavioral regression coverage removed

This deleted test exercised observable public behavior: complete relationship population when an internal lookup crosses the configured query-value boundary. It did not inspect chunk sizes or internal calls. Removing the only coverage of this boundary across relationship types and directions allows the exact failure introduced here to pass while ordinary relationship tests remain green. Retain a behavior-focused version and remove only the redundant direct-query exception assertion.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: tests/e2e/Adapter/Scopes/RelationshipTests.php
Line: 26

Comment:
**Behavioral regression coverage removed**

This deleted test exercised observable public behavior: complete relationship population when an internal lookup crosses the configured query-value boundary. It did not inspect chunk sizes or internal calls. Removing the only coverage of this boundary across relationship types and directions allows the exact failure introduced here to pass while ordinary relationship tests remain green. Retain a behavior-focused version and remove only the redundant direct-query exception assertion.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Claude Code Fix in Codex

use ManyToManyTests;

/**
* @return array<string, array{string, string, int}>
*/
public static function relationshipQueryValueLimitProvider(): array
{
$cases = [];
foreach ([Database::RELATION_ONE_TO_ONE, Database::RELATION_ONE_TO_MANY, Database::RELATION_MANY_TO_ONE, Database::RELATION_MANY_TO_MANY] as $type) {
foreach (['parents', 'children'] as $collection) {
$cases[$type . ' ' . $collection] = [$type, $collection, 3];
}
}

// One parent can exceed the value limit when fetching its related documents.
$cases['manyToMany related documents'] = [Database::RELATION_MANY_TO_MANY, 'parents', 1];

return $cases;
}

/**
* @dataProvider relationshipQueryValueLimitProvider
*/
public function testRelationshipQueryValueLimit(string $type, string $collection, int $limit): void
{
$database = $this->getDatabase();

if (!$database->getAdapter()->getSupportForRelationships()) {
$this->expectNotToPerformAssertions();
return;
}

$parents = ID::unique();
$children = ID::unique();
$permissions = [
Permission::read(Role::any()),
Permission::create(Role::any()),
Permission::update(Role::any()),
Permission::delete(Role::any()),
];
$database->createCollection($parents, permissions: $permissions);
$database->createCollection($children, permissions: $permissions);
$database->createRelationship($parents, $children, $type, true, 'children', 'parents');

foreach (['child1', 'child2', 'child3'] as $id) {
$database->createDocument($children, new Document(['$id' => $id]));
}

for ($i = 1; $i <= 3; $i++) {
$related = match ($type) {
Database::RELATION_MANY_TO_MANY => ['child1', 'child2', 'child3'],
Database::RELATION_ONE_TO_MANY => ['child' . $i],
default => 'child' . $i,
};
$database->createDocument($parents, new Document([
'$id' => 'parent' . $i,
'children' => $related,
]));
}

$max = $database->getMaxQueryValues();
$database->setMaxQueryValues(2);

try {
$documents = $database->find($collection === 'parents' ? $parents : $children, [Query::limit($limit)]);

$this->assertCount($limit, $documents);
foreach ($documents as $document) {
$related = $document->getAttribute($collection === 'parents' ? 'children' : 'parents');
$related = $related instanceof Document ? [$related] : $related;
$ids = \array_map(fn (Document $related) => $related->getId(), $related);
$prefix = $collection === 'parents' ? 'child' : 'parent';
$expected = $type === Database::RELATION_MANY_TO_MANY
? [$prefix . '1', $prefix . '2', $prefix . '3']
: [$prefix . \substr($document->getId(), -1)];
$this->assertEqualsCanonicalizing($expected, $ids);
}

$this->expectException(QueryException::class);
$database->find($children, [Query::equal('$id', ['child1', 'child2', 'child3'])]);
} finally {
$database->setMaxQueryValues($max);
$database->deleteCollection($parents);
$database->deleteCollection($children);
}
}

public function testZoo(): void
{
/** @var Database $database */
Expand Down
Loading