Skip to content

Revert relationship max queries - #973

Closed
fogelito wants to merge 1 commit into
mainfrom
revert-relationship-max-queries
Closed

fogelito wants to merge 1 commit into
mainfrom
revert-relationship-max-queries

Conversation

@fogelito

Copy link
Copy Markdown
Contributor

No description provided.

@coderabbitai

coderabbitai Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

Next included review available in 55 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 322ecbbc-790d-4d67-94c6-33688d18010c

📥 Commits

Reviewing files that changed from the base of the PR and between 54364c6 and ca33227.

📒 Files selected for processing (2)
  • src/Database/Database.php
  • tests/e2e/Adapter/Scopes/RelationshipTests.php

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 4/5

This PR is not safe to merge because supported lower maxQueryValues configurations will cause sufficiently large relationship reads to fail.

Fix All in Claude CodeFindings

  1. P1 Configured query limit ignored
  2. P2 Behavioral regression coverage removed
Fix with agent prompt
### Issue 1
src/Database/Database.php:5328
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.

### Issue 2
tests/e2e/Adapter/Scopes/RelationshipTests.php:26
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.

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!

---

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

Summary

  • Relationship population no longer respects Database::setMaxQueryValues().
  • Five relationship-loading paths can consequently submit internally invalid queries.
  • The removed test asserted observable relationship results and leaves this boundary behavior uncovered.

Reviews (1) · Last reviewed commit: "Revert"

Comment thread src/Database/Database.php

// 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) {

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

@@ -26,91 +26,6 @@ trait RelationshipTests
use ManyToOneTests;

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

@fogelito fogelito closed this Sep 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant