Fix nested filter selects #10393
Fix nested filter selects #10393
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (12)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the 📝 WalkthroughWalkthroughRemoved several Console::info debug logs related to database event processing in app/controllers/shared/api.php and the Databases worker. Changed HTTP action descriptions to "Upsert" for Collections/Documents and Tables/Rows endpoints. Reworked the V20 request filter: normalized and filtered select queries (dropping empty entries), introduced a needsRelationships flag to control relationship inclusion, implemented cycle-safe depth-limited recursive related-collection key resolution with a visited set, and ensured parsed queries are serialized back to strings. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
commit: |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Rows/Upsert.php (1)
34-34: Terminology update looks good; tiny copy tweak for consistency"Upsert a row" aligns with TablesDB wording. Consider updating the data param description to say "to be upserted" for consistency with the action verb.
Apply:
- ->param('data', [], new JSON(), 'Row data as JSON object. Include all required columns of the row to be created or updated.', true) + ->param('data', [], new JSON(), 'Row data as JSON object. Include all required columns of the row to be upserted.', true)src/Appwrite/Utopia/Request/Filters/V20.php (5)
63-75: Confirm: apply relationship auto-selects to getDocument too?Retrieved learning says wildcard select handling was previously needed only for listDocuments. Here, parse() applies manageSelectQueries to both getDocument and listDocuments. Validate product intent; applying to getDocument may have perf implications.
If you decide to scope to listDocuments only:
- case 'databases.getDocument': - case 'databases.listDocuments': + case 'databases.listDocuments': $content = $this->manageSelectQueries($content); break;Note: I referenced your prior learning about V19 behavior while making this suggestion.
80-92: Back-compat select synthesis reads well; minor stability nitDeduping via array_unique is good. For deterministic SDK snapshots, consider sorting the synthesized selects after dedupe.
Apply:
- $selects = \array_values(\array_unique(\array_merge(['*'], $relatedKeys))); + $selects = \array_values(\array_unique(\array_merge(['*'], $relatedKeys))); + \sort($selects);
126-133: Cycle check: prefer strict comparison in in_arrayIDs are strings; use strict mode to avoid edge-case coercions.
Apply:
- if (in_array($collectionId, $visited)) { + if (\in_array($collectionId, $visited, true)) {
165-186: Skip-if-visited and recursion are correct; add strict check and unify attribute access
- Use strict
in_arrayfor$relatedCollectionId.- Optional: for consistency with other files (e.g., Documents Upsert), prefer
$attr->getAttribute(...)over array access if$attributesare Document instances. If array access is intentional, please confirm that Utopia Document implements ArrayAccess in this context.Apply:
- if ($relatedCollectionId && in_array($relatedCollectionId, $visited)) { + if ($relatedCollectionId && \in_array($relatedCollectionId, $visited, true)) { continue; }If unifying to object-style access:
- foreach ($attributes as $attr) { - if ( - ($attr['type'] ?? null) !== Database::VAR_RELATIONSHIP || - $attr['status'] !== 'available' - ) { + foreach ($attributes as $attr) { + if ( + ($attr->getAttribute('type') ?? null) !== Database::VAR_RELATIONSHIP || + $attr->getAttribute('status') !== 'available' + ) { continue; } - $key = $attr['key']; + $key = $attr->getAttribute('key'); $fullKey = $prefix ? $prefix . '.' . $key : $key; - $relatedCollectionId = $attr['relatedCollection'] ?? null; + $relatedCollectionId = $attr->getAttribute('relatedCollection') ?? null;
55-60: Add focused tests for new behaviorPlease add tests covering:
- No selects provided → synthesized ['', related....] is added.
- Explicit wildcard present → existing selects replaced by synthesized list.
- Explicit field selects only → no synthesis.
- Nested relationships (2–3 levels) and cycle prevention.
I can draft PHPUnit tests for V20::manageSelectQueries and getRelatedCollectionKeys with mocked Documents/DB.
Also applies to: 94-101
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (13)
app/config/specs/open-api3-1.8.x-client.jsonis excluded by!app/config/specs/**app/config/specs/open-api3-1.8.x-console.jsonis excluded by!app/config/specs/**app/config/specs/open-api3-1.8.x-server.jsonis excluded by!app/config/specs/**app/config/specs/open-api3-latest-client.jsonis excluded by!app/config/specs/**app/config/specs/open-api3-latest-console.jsonis excluded by!app/config/specs/**app/config/specs/open-api3-latest-server.jsonis excluded by!app/config/specs/**app/config/specs/swagger2-1.8.x-client.jsonis excluded by!app/config/specs/**app/config/specs/swagger2-1.8.x-console.jsonis excluded by!app/config/specs/**app/config/specs/swagger2-1.8.x-server.jsonis excluded by!app/config/specs/**app/config/specs/swagger2- 8000 latest-client.jsonis excluded by!app/config/specs/**app/config/specs/swagger2-latest-console.jsonis excluded by!app/config/specs/**app/config/specs/swagger2-latest-server.jsonis excluded by!app/config/specs/**composer.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
app/controllers/shared/api.php(0 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Upsert.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Rows/Upsert.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Workers/Databases.php(0 hunks)src/Appwrite/Utopia/Request/Filters/V20.php(4 hunks)
💤 Files with no reviewable changes (2)
- app/controllers/shared/api.php
- src/Appwrite/Platform/Modules/Databases/Workers/Databases.php
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-19T09:20:03.312Z
Learnt from: ItzNotABug
PR: appwrite/appwrite#9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php:57-59
Timestamp: 2025-06-19T09:20:03.312Z
Learning: In table-related endpoints (such as `src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php`), parameter descriptions should use "table" and "row" terminology instead of "collection" and "document" for clarity and consistency.
Applied to files:
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Upsert.phpsrc/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Rows/Upsert.php
📚 Learning: 2025-05-03T05:58:34.294Z
Learnt from: ItzNotABug
PR: appwrite/appwrite#9669
File: src/Appwrite/Utopia/Request/Filters/V19.php:56-71
Timestamp: 2025-05-03T05:58:34.294Z
Learning: According to the developer, wildcard select handling in V19 request filter is only needed for `listDocuments` operations, not for `getDocument` operations, despite the doc-block mentioning both.
Applied to files:
src/Appwrite/Utopia/Request/Filters/V20.php
🪛 GitHub Actions: Linter
src/Appwrite/Utopia/Request/Filters/V20.php
[error] 1-1: Pint lint failed. PSR-12 violation: 'single_space_around_construct' in this file. Command 'vendor/bin/pint --test --config pint.json' exited with code 1. Run 'vendor/bin/pint --write' to fix code style issues.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build SDK
- GitHub Check: scan
🔇 Additional comments (3)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Upsert.php (1)
48-48: "Upsert" wording is consistent with deprecation pathThe description change matches the deprecation notice pointing to tablesDB.upsertRow. No further action needed here.
src/Appwrite/Utopia/Request/Filters/V20.php (2)
36-37: Initialize missing queries to empty arrayGood defensive default before parsing.
106-115: Docblock andvisitedparam addition are clearExplicitly documenting depth and cycle prevention is helpful. No issues.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Appwrite/Utopia/Request/Filters/V20.php (1)
39-52: Fix Pint/PSR-12 cast spacing; consider concise early-return predicate
- Style: add a space after the (array) cast.
- Readability: optional early-return form reduces branches. This was suggested earlier; spacing on the closure name is already fixed.
- $content['queries'] = \array_filter($content['queries'], function ($q) { - if (\is_object($q) && empty((array)$q)) { - return false; - } - if (\is_string($q) && \trim($q) === '') { - return false; - } - if (empty($q)) { - return false; - } - return true; - }); + $content['queries'] = \array_filter($content['queries'], function ($q) { + if (\is_object($q) && empty((array) $q)) return false; + if (\is_string($q) && \trim($q) === '') return false; + return !empty($q); + });
🧹 Nitpick comments (6)
src/Appwrite/Utopia/Request/Filters/V20.php (6)
55-59: Normalize input type for Query::parseQueries to avoid TypeErrorIf a single query string slips in, normalize it to an array before parsing.
- try { + // Normalize a single query string to array to satisfy parser expectations + if (\is_string($content['queries'])) { + $content['queries'] = [$content['queries']]; + } + try { 8000 $parsed = Query::parseQueries($content['queries']); } catch (QueryException) { return $content; }
104-108: Avoid hard-coding “3 levels” in docs; tie to constant to prevent driftUpdate the docblock to reference Database::RELATION_MAX_DEPTH.
- * Recursively includes nested relationships up to 3 levels deep. - * Prevents infinite loops by tracking all visited collections in the current path. + * Recursively includes nested relationships up to Database::RELATION_MAX_DEPTH. + * Prevents cycles by tracking visited collections along the current path.
127-134: Use strict, namespaced in_array for cycle checkPrevents loose comparisons and aligns with global function prefixing used elsewhere.
- if (in_array($collectionId, $visited)) { + if (\in_array($collectionId, $visited, true)) { return []; }
156-161: Guard against missing 'status' key to avoid noticesUse null coalescing when checking availability.
- if ( - ($attr['type'] ?? null) !== Database::VAR_RELATIONSHIP || - $attr['status'] !== 'available' - ) { + if ( + ($attr['type'] ?? null) !== Database::VAR_RELATIONSHIP || + (($attr['status'] ?? null) !== 'available') + ) { continue; }
165-170: Also strict-check visited when skipping related collectionConsistent strictness and namespacing.
- if ($relatedCollectionId && in_array($relatedCollectionId, $visited)) { + if ($relatedCollectionId && \in_array($relatedCollectionId, $visited, true)) { continue; }
16-21: Double-check: should parse() apply select backfill to getDocument?If not, constrain to listDocuments only.
- case 'databases.getDocument': - case 'databases.listDocuments': + case 'databases.listDocuments': $content = $this->manageSelectQueries($content); break;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/Appwrite/Utopia/Request/Filters/V20.php(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ItzNotABug
PR: appwrite/appwrite#9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php:57-59
Timestamp: 2025-06-19T09:20:03.312Z
Learning: In table-related endpoints (such as `src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php`), parameter descriptions should use "table" and "row" terminology instead of "collection" and "document" for clarity and consistency.
📚 Learning: 2025-05-03T05:58:34.294Z
Learnt from: ItzNotABug
PR: appwrite/appwrite#9669
File: src/Appwrite/Utopia/Request/Filters/V19.php:56-71
Timestamp: 2025-05-03T05:58:34.294Z
Learning: According to the developer, wildcard select handling in V19 request filter is only needed for `listDocuments` operations, not for `getDocument` operations, despite the doc-block mentioning both.
Applied to files:
src/Appwrite/Utopia/Request/Filters/V20.php
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Linter
- GitHub Check: Setup & Build SDK
- GitHub Check: scan
🔇 Additional comments (4)
src/Appwrite/Utopia/Request/Filters/V20.php (4)
36-37: LGTM: sensible default for missing queriesInitializing to an empty array is safe and avoids downstream type issues.
63-72: Confirm intended scope: apply 1.7-style relationship expansion to getDocument?V19 learning notes suggest wildcard select handling was only needed for listDocuments. Please confirm V20 should also expand relationships for getDocument; otherwise gate this to listDocuments only.
78-92: LGTM: relationship backfill only when no selects or wildcard presentThe flow avoids overriding explicit non-wildcard selects and re-serializes cleanly.
114-115: LGTM: visited set parameter enables cycle safety without global stateAdding $visited with a default is appropriate for path-scoped cycle detection.
✨ Benchmark results
⚡ Benchmark Comparison
|
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Appwrite/Utopia/Request/Filters/V20.php (1)
157-159: Guard optional keys to avoid Undefined array key notices$status and $key may not exist on all attribute shapes.
Apply:
- if ( - ($attr['type'] ?? null) !== Database::VAR_RELATIONSHIP || - $attr['status'] !== 'available' - ) { + if ( + ($attr['type'] ?? null) !== Database::VAR_RELATIONSHIP || + (($attr['status'] ?? null) !== 'available') + ) { continue; } + $key = $attr['key'] ?? null; + if ($key === null || $key === '') { + continue; + } - $key = $attr['key'];
♻️ Duplicate comments (2)
src/Appwrite/Utopia/Request/Filters/V20.php (2)
41-52: PSR-12 cast spacing + simplify predicate (duplicate of earlier suggestion)Spacing after the (array) cast is non-PSR-12; also the predicate can be shorter.
Apply:
- $content['queries'] = \array_filter($content['queries'], function ($q) { - if (\is_object($q) && empty((array)$q)) { - return false; - } - if (\is_string($q) && \trim($q) === '') { - return false; - } - if (empty($q)) { - return false; - } - return true; - }); + $content['queries'] = \array_filter($content['queries'], function ($q) { + if (\is_object($q) && empty((array) $q)) return false; + if (\is_string($q) && \trim($q) === '') return false; + return !empty($q); + });
175-186: Nice fix: replace array_push(...$nestedKeys) with array_mergeRemoves the ArgumentCountError risk when $nestedKeys is empty.
🧹 Nitpick comments (2)
src/Appwrite/Utopia/Request/Filters/V20.php (2)
127-133: Use strict and FQN for in_array; minor styleAvoids accidental truthiness and matches global-call style used for array_*.
Apply:
- if (in_array($collectionId, $visited)) { + if (\in_array($collectionId, $visited, true)) {
165-170: Repeat: strict and FQN for in_array on relatedCollectionIdSame rationale as earlier.
Apply:
- if ($relatedCollectionId && in_array($relatedCollectionId, $visited)) { + if ($relatedCollectionId && \in_array($relatedCollectionId, $visited, true)) { continue; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/Appwrite/Utopia/Request/Filters/V20.php(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ItzNotABug
PR: appwrite/appwrite#9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php:57-59
Timestamp: 2025-06-19T09:20:03.312Z
Learning: In table-related endpoints (such as `src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php`), parameter descriptions should use "table" and "row" terminology instead of "collection" and "document" for clarity and consistency.
📚 Learning: 2025-05-03T05:58:34.294Z
Learnt from: ItzNotABug
PR: appwrite/appwrite#9669
File: src/Appwrite/Utopia/Request/Filters/V19.php:56-71
Timestamp: 2025-05-03T05:58:34.294Z
Learning: According to the developer, wildcard select handling in V19 request filter is only needed for `listDocuments` operations, not for `getDocument` operations, despite the doc-block mentioning both.
Applied to files:
src/Appwrite/Utopia/Request/Filters/V20.php
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Check if utopia-php/database changed
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build SDK
- GitHub Check: scan
🔇 Additional comments (6)
src/Appwrite/Utopia/Request/Filters/V20.php (6)
16-21: Confirm: apply relationship auto-selects to getDocument as well?V19 learnings indicated wildcard handling only for listDocuments. Here, manageSelectQueries runs for both getDocument and listDocuments. If this is an intentional product change for V20, great—otherwise, restrict to listDocuments to avoid unexpected payload expansion on getDocument.
Would you like a follow-up patch to gate this to listDocuments only?
36-38: Good defaulting of missing queriesInitializing to an empty array prevents parse errors downstream.
63-71: Logic for needsRelationships is soundCovers both no-select and explicit wildcard cases.
80-92: Correctly rebuilds selects and avoids duplicate TYPE_SELECT entriesPrunes existing selects and injects ['*', ...relatedKeys] once. Good.
104-115: Doc + API shape of getRelatedCollectionKeys look goodDepth/visited parameters document the cycle-safe recursion clearly.
61-61: Verification needed: Confirm groupByType return keysWe were unable to locate the source of Utopia\Database\Query — it’s defined outside the Appwrite repository (e.g. in vendor/Utopia/src/Database/Query.php). Please verify the following:
In Query::groupByType(), are selections returned under the constant key Query::TYPE_SELECT rather than the literal string 'selections'?
If so, update src/Appwrite/Utopia/Request/Filters/V20.php (and other call sites using ['selections']) to use:
$grouped = Query::groupByType($parsed); $selections = $grouped[Query::TYPE_SELECT] ?? [];This will ensure your selects aren’t always empty and prevent unintended wildcard/relationship fallbacks.
What does this PR do?
(Provide a description of what this PR does and why it's needed.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)
Related PRs and Issues
Checklist