fix: respect query value limits when loading relationships - #964
fix: respect query value limits when loading relationships#964HarshMN2345 wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughRelationship population batches now cap ID lookup chunks at ChangesRelationship query limits
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Merge Risk: ⚪ Minimal · up to Relationship loading now respects configured query-value limits without changing oversized explicit-query behavior. Coverage spans the supported relationship paths, so the change is ready to merge. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| $database->find($children, [Query::equal('$id', ['child1', 'child2', 'child3'])]); | ||
| $this->fail('Explicit queries must still respect the configured value limit.'); | ||
| } catch (QueryException $exception) { | ||
| $this->assertSame('Invalid query: Query on attribute has greater than 2 values: $id', $exception->getMessage()); |
There was a problem hiding this comment.
This assertion copies the validator’s exact wording and the configured value limit instead of testing only the observable behavior that an oversized query is rejected. That violates the repository requirement against implementation-coupled tests and would make harmless message changes break this end-to-end test. This requirement must be satisfied before merging.
| $this->assertSame('Invalid query: Query on attribute has greater than 2 values: $id', $exception->getMessage()); | |
| $this->assertInstanceOf(QueryException::class, $exception); |
Context Used: Call out and harshly judge implementation-coupled ... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/e2e/Adapter/Scopes/RelationshipTests.php
Line: 109
Comment:
**Brittle Exception Assertion**
This assertion copies the validator’s exact wording and the configured value limit instead of testing only the observable behavior that an oversized query is rejected. That violates the repository requirement against implementation-coupled tests and would make harmless message changes break this end-to-end test. This requirement must be satisfied before merging.
```suggestion
$this->assertInstanceOf(QueryException::class, $exception);
```
**Context Used:** Call out and harshly judge implementation-coupled ... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
---
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!
|
|
||
| // Process in chunks to avoid exceeding query value limits | ||
| foreach (\array_chunk($uniqueRelatedIds, self::RELATION_QUERY_CHUNK_SIZE) as $chunk) { | ||
| foreach (\array_chunk($uniqueRelatedIds, \max(1, \min(self::RELATION_QUERY_CHUNK_SIZE, $this->maxQueryValues))) as $chunk) { |
There was a problem hiding this comment.
I think the const RELATION_QUERY_CHUNK_SIZE is useless , will never use it ..
We can always use $this->maxQueryValues
2 participants
What does this PR do?
Relationship loading batches up to 5,000 IDs even when the connection permits fewer query values. A document with 501 related IDs therefore fails to load with a 500-value limit.
Cap all five relationship query batches at the configured limit, using the existing inline chunking pattern. The 5,000-value ceiling and validation of caller-supplied queries remain unchanged.
Test Plan
Appwrite adoption requires a library release and dependency update. The HTTP regression is appwrite/appwrite#13610.
Summary by CodeRabbit
Bug Fixes
Tests