Conversation
📝 WalkthroughWalkthroughAction::disableSubqueries now accepts an optional array parameter and applySelectQueries returns early when a wildcard ('*') is present. Projects XList gains a cached attribute→subQuery filter map, a new private find() that computes and skips subquery filters based on selected attributes, and its action signature now receives a Request. Projects query validator exposes isSelectQueryAllowed(). Project response model moves nested-field expansion into four private helper methods. A new end-to-end test for select queries was added (appears duplicated). Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas needing extra attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (5)📓 Common learnings📚 Learning: 2025-05-03T05:58:34.294ZApplied to files:
📚 Learning: 2025-12-19T10:57:43.371ZApplied to files:
📚 Learning: 2025-08-11T10:32:38.142ZApplied to files:
📚 Learning: 2025-05-10T04:12:13.037ZApplied to files:
⏰ 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)
🔇 Additional comments (4)
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 |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
✨ Benchmark results
⚡ Benchmark Comparison
|
There was a problem hiding this comment.
Actionable comments posted: 1
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/Response/Model/Project.php (1)
344-417: Watch for default changes onauthMemberships*fields inexpandAuthFieldsThe refactor to
filter()plus the fourexpand*helpers improves readability, and the guards on missingsmtp,services,auths, andoAuthProvidersare sensible.One subtle point in
expandAuthFields:
In the constructor, the rules for:
authMembershipsUserNameauthMembershipsUserEmailauthMembershipsMfaall have
default => false.In
expandAuthFields(), the fallback whenmembershipsUserName/membershipsUserEmail/membershipsMfaare absent in$authValuesistrue(?? true).That means for any project document whose
authspayload doesn’t explicitly include these keys (e.g., older records or partial configs), the runtime value will now betrueeven though the model’s declared default isfalse. If the previous implementation usedfalsehere, this would be a behavior change.It’s worth double‑checking the intended defaults and, if they should stay
false, aligning the helper with the constructor:$document->setAttribute('authMembershipsUserName', $authValues['membershipsUserName'] ?? false); $document->setAttribute('authMembershipsUserEmail', $authValues['membershipsUserEmail'] ?? false); $document->setAttribute('authMembershipsMfa', $authValues['membershipsMfa'] ?? false);
🧹 Nitpick comments (2)
tests/e2e/Services/Projects/ProjectsConsoleClientTest.php (1)
439-667: Good end‑to‑end coverage ofQuery::selecton /projects with minor room to harden assertionsThe new
testListProjectsQuerySelect()nicely exercises:
- simple and multi‑field selects,
- selects combined with filters and limits,
- behavior when including subquery‑backed attributes (
platforms,webhooks,keys),- and error handling for invalid attributes.
Two small optional tweaks you might consider:
- Where comments state “subqueries should still run” (e.g., webhooks/keys cases), also asserting that those arrays are non‑empty would better distinguish “subquery ran” from “default empty array”.
- If the fixture reliably produces at least one webhook/key for the first listed project, this would remain stable; otherwise, you could explicitly filter to a project known to have those relations before asserting.
Functionally this is fine as‑is; these would just make the test more diagnostic if something regresses in the subquery path.
src/Appwrite/Platform/Modules/Projects/Http/Projects/XList.php (1)
131-162: Attribute→subQuery filter cache is efficient but assumes one‑way, non‑overlapping mapping
getAttributeToSubQueryFilters()efficiently builds and caches the mapping from project attributes to theirsubQuery*filters fromcollectionsconfig. That’s a nice optimization for repeated list calls.Two assumptions are baked in:
- The collections config is stable for the process lifetime (reasonable for this context).
- Each
subQuery*filter is safely associated with every attribute it appears under, and no filter is shared between attributes that might be selected independently.In
findWithSelect(), filters are skipped whenever any attribute that references them is not selected. If a singlesubQuery*filter is reused across multiple attributes and at least one of those attributes is unselected, this logic will skip the filter even if another attribute that depends on it is selected.If the current
collections['platform']['projects']['attributes']schema guarantees 1:1 (or at least non‑conflicting) mappings, this is fine; otherwise, you may want to invert the mapping (filter→set of attributes) and only skip a given filter when none of its associated attributes are selected.Based on learnings, you also no longer need to manually separate filters for
count()($dbForPlatform->count()can accept the full query array), but that’s just a simplification opportunity, not a bug.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Appwrite/Platform/Action.php(1 hunks)src/Appwrite/Platform/Modules/Projects/Http/Projects/XList.php(3 hunks)src/Appwrite/Utopia/Database/Validator/Queries/Projects.php(1 hunks)src/Appwrite/Utopia/Response/Model/Project.php(4 hunks)tests/e2e/Services/Projects/ProjectsConsoleClientTest.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-11T10:32:38.142Z
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10297
File: app/controllers/api/users.php:649-657
Timestamp: 2025-08-11T10:32:38.142Z
Learning: In Appwrite's utopia-php/database (version 0.77.* and later), the database layer internally handles filter separation for count operations. When using `$dbForProject->count()` within a `skipFilters` closure, there's no need to manually separate filter queries using `Query::groupByType($queries)['filters']` as the database handles this automatically.
Applied to files:
src/Appwrite/Platform/Action.phpsrc/Appwrite/Platform/Modules/Projects/Http/Projects/XList.php
🧬 Code graph analysis (2)
tests/e2e/Services/Projects/ProjectsConsoleClientTest.php (3)
tests/e2e/Client.php (1)
Client(8-342)tests/e2e/Scopes/ProjectCustom.php (1)
getProject(21-184)tests/e2e/Scopes/Scope.php (2)
getProject(150-150)getHeaders(145-145)
src/Appwrite/Utopia/Response/Model/Project.php (1)
src/Appwrite/Utopia/Response/Model/Document.php (1)
Document(8-115)
🔇 Additional comments (4)
src/Appwrite/Utopia/Database/Validator/Queries/Projects.php (1)
21-24: Explicitly allowing select queries for projects looks correct
isSelectQueryAllowed(): boolreturningtrueis a minimal, clear way to opt projects intoQuery::selectwithout affecting other validators. No issues from a correctness or BC standpoint.src/Appwrite/Platform/Action.php (1)
110-127: Optional filter parameter ondisableSubqueriesis safe and BC‑friendlyDefaulting to
$this->filterswhen$filtersis empty preserves existing behavior, while allowing more granular subquery disabling when needed. Lambdas remain no‑ops, so semantics are unchanged.src/Appwrite/Utopia/Response/Model/Project.php (1)
419-445: OAuth providers expansion logic is consistent with config‑driven behavior
expandOAuthProviders()rebuilding theoAuthProvidersattribute from configuration plus per‑project stored values (and skipping globally disabled providers) keeps response shape predictable and ensures disabled providers never leak. Guarding on$document->isSet('oAuthProviders')also avoids unnecessary work for documents that don’t have any provider data yet. No issues spotted here.src/Appwrite/Platform/Modules/Projects/Http/Projects/XList.php (1)
165-196:findWithSelectcorrectly skips unnecessary subqueries, with clear trade‑offsThe
findWithSelect()helper:
- Extracts the selected attributes from
selections.- Short‑circuits on
'*'to avoid any filter skipping.- Builds a set of
subQuery*filters to skip for attributes not in the selection.- Uses
skipFilters()to runfind()with only the needed subquery filters.This achieves the intended goal of avoiding expensive subqueries (like webhooks, keys, platforms, etc.) when their attributes aren’t selected, while remaining transparent to the rest of the query pipeline.
Two explicit trade‑offs worth documenting in comments (as you already hint at with “nested selects aren’t handled atm”):
- Nested/expanded attributes referenced via select (e.g., if support for
platforms.nameor similar ever appears) will still be treated as flat attributes; the current logic won’t detect such usage and will skip the corresponding subqueries.- The behavior when selecting attributes that are computed from underlying ones (e.g., fields expanded in
Project::filter()) depends on whether those underlying DB attributes are what clients actually select. That contract should stay stable across schema changes.Functionally this is solid; these are just considerations for future evolution of the query language.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/e2e/Services/Projects/ProjectsConsoleClientTest.php (1)
690-690: Consider: Hardcoded error message may be brittle.The exact error message string is hardcoded in the assertion. While this is a common pattern in e2e tests, it may break if the error message format changes. Consider whether asserting on the status code and checking that the message contains a key phrase (e.g., "invalidAttribute") would be more maintainable.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Appwrite/Platform/Action.php(2 hunks)tests/e2e/Services/Projects/ProjectsConsoleClientTest.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-11T10:32:38.142Z
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10297
File: app/controllers/api/users.php:649-657
Timestamp: 2025-08-11T10:32:38.142Z
Learning: In Appwrite's utopia-php/database (version 0.77.* and later), the database layer internally handles filter separation for count operations. When using `$dbForProject->count()` within a `skipFilters` closure, there's no need to manually separate filter queries using `Query::groupByType($queries)['filters']` as the database handles this automatically.
Applied to files:
src/Appwrite/Platform/Action.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). (19)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: E2E General Test
- GitHub Check: Unit Test
- GitHub Check: scan
🔇 Additional comments (3)
src/Appwrite/Platform/Action.php (2)
110-127: LGTM! Backward-compatible signature enhancement.The optional
$filtersparameter allows selective filter management while maintaining backward compatibility through the default empty array. The logic appropriately falls back to$this->filterswhen no specific filters are provided.
194-197: LGTM! Correct wildcard handling.The early return when a wildcard is detected is a sensible optimization that correctly implements the expected behavior—when
'*'is selected, all attributes should be returned without filtering.tests/e2e/Services/Projects/ProjectsConsoleClientTest.php (1)
439-698: Comprehensive test coverage for Query.select functionality.This test thoroughly validates select query behavior including basic field selection, multiple fields, combining with filters/limits, subquery attributes, wildcard handling, and error cases. The test properly isolates itself by creating and cleaning up its own resources.
Note: The AI summary mentions "duplicate test methods named testListProjectsQuerySelect" but only one instance appears in the provided code. This may be a summary error or the duplicate was already removed.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Platform/Modules/Projects/Http/Projects/XList.php(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-03T05:58:34.294Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 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/Platform/Modules/Projects/Http/Projects/XList.php
📚 Learning: 2025-08-11T10:32:38.142Z
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10297
File: app/controllers/api/users.php:649-657
Timestamp: 2025-08-11T10:32:38.142Z
Learning: In Appwrite's utopia-php/database (version 0.77.* and later), the database layer internally handles filter separation for count operations. When using `$dbForProject->count()` within a `skipFilters` closure, there's no need to manually separate filter queries using `Query::groupByType($queries)['filters']` as the database handles this automatically.
Applied to files:
src/Appwrite/Platform/Modules/Projects/Http/Projects/XList.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). (20)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (VCS)
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E General Test
- GitHub Check: Unit Test
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: Benchmark
- GitHub Check: scan
🔇 Additional comments (6)
src/Appwrite/Platform/Modules/Projects/Http/Projects/XList.php (6)
6-6: LGTM: Import updates support the new select query functionality.The updated imports are appropriate:
Appwrite\Platform\Actionaligns with the module structureRequestinjection enablesapplySelectQueriesintegrationConfigprovides access to project attribute metadata for subQuery filter optimizationAlso applies to: 12-12, 14-14
29-30: Good use of static caching for performance.Caching the attribute-to-subQuery filter mapping in a static property is efficient since this metadata is derived from configuration and won't change during the request lifecycle.
61-61: Request injection properly wired.The addition of
Requestinjection and the corresponding action parameter enables theapplySelectQueriesintegration. The directProjects()validator instantiation aligns with past review discussion.Also applies to: 64-64, 70-70
108-112: Correct separation of select and filter queries.The query grouping logic properly distinguishes between projections (
selectQueries) and filtering criteria (filterQueries):
- Select queries drive the optimization in
find()to skip unnecessary subQuery filters- Filter queries are correctly used for the count operation, as projections should not affect totals
117-117: Proper integration of select query response filtering.Calling
applySelectQueriesbefore returning the response ensures that the model output respects the client's field selection. The placement and parameters are correct.
124-155: Well-structured config-driven filter mapping.The method correctly:
- Caches the mapping to avoid redundant config reads
- Filters for
subQuery*prefixed filters from project attribute metadata- Builds a clean attribute-to-filters map for optimization
The implicit assumption that the config path exists is reasonable for internal configuration, though a defensive null-check on
$projectAttributescould prevent edge-case issues.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Appwrite/Platform/Modules/Projects/Http/Projects/XList.php (1)
131-161: Consider adding defensive checks for Config structure.The method assumes a specific structure from
Config::getParam(). If the configuration format changes or is missing keys, the current implementation gracefully handles it by returning an empty array. However, adding explicit checks or comments documenting the expected structure would improve maintainability.🔎 Optional: Add defensive checks
private static function getAttributeToSubQueryFilters(): array { if (self::$attributeToSubQueryFilters !== null) { return self::$attributeToSubQueryFilters; } self::$attributeToSubQueryFilters = []; $collections = Config::getParam('collections', []); + if (!\is_array($collections)) { + return self::$attributeToSubQueryFilters; + } + $projectAttributes = $collections['platform']['projects']['attributes'] ?? []; + if (!\is_array($projectAttributes)) { + return self::$attributeToSubQueryFilters; + } foreach ($projectAttributes as $attribute) { + if (!\is_array($attribute)) { + continue; + } + $attributeId = $attribute['$id'] ?? null; $filters = $attribute['filters'] ?? []; if ($attributeId === null || empty($filters)) { continue; } // extract only subQuery filters $subQueryFilters = \array_filter($filters, function ($filter) { return \str_starts_with($filter, 'subQuery'); }); if (!empty($subQueryFilters)) { self::$attributeToSubQueryFilters[$attributeId] = \array_values($subQueryFilters); } } return self::$attributeToSubQueryFilters; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Platform/Modules/Projects/Http/Projects/XList.php(4 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10297
File: app/controllers/api/users.php:649-657
Timestamp: 2025-08-11T10:32:38.142Z
Learning: In Appwrite's utopia-php/database (version 0.77.* and later), the database layer internally handles filter separation for count operations. When using `$dbForProject->count()` within a `skipFilters` closure, there's no need to manually separate filter queries using `Query::groupByType($queries)['filters']` as the database handles this automatically.
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10990
File: src/Appwrite/Platform/Modules/Projects/Http/Projects/XList.php:157-191
Timestamp: 2025-12-19T10:57:43.371Z
Learning: In Appwrite, dot-path selects (e.g., `Query::select(['platforms.name'])`) are not supported on subquery attributes. The query system does not allow nested field selection on attributes that are populated via subQuery filters.
📚 Learning: 2025-05-03T05:58:34.294Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 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/Platform/Modules/Projects/Http/Projects/XList.php
📚 Learning: 2025-12-19T10:57:43.371Z
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10990
File: src/Appwrite/Platform/Modules/Projects/Http/Projects/XList.php:157-191
Timestamp: 2025-12-19T10:57:43.371Z
Learning: In Appwrite, dot-path selects (e.g., `Query::select(['platforms.name'])`) are not supported on subquery attributes. The query system does not allow nested field selection on attributes that are populated via subQuery filters.
Applied to files:
src/Appwrite/Platform/Modules/Projects/Http/Projects/XList.php
📚 Learning: 2025-08-11T10:32:38.142Z
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10297
File: app/controllers/api/users.php:649-657
Timestamp: 2025-08-11T10:32:38.142Z
Learning: In Appwrite's utopia-php/database (version 0.77.* and later), the database layer internally handles filter separation for count operations. When using `$dbForProject->count()` within a `skipFilters` closure, there's no need to manually separate filter queries using `Query::groupByType($queries)['filters']` as the database handles this automatically.
Applied to files:
src/Appwrite/Platform/Modules/Projects/Http/Projects/XList.php
📚 Learning: 2025-05-10T04:12:13.037Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Indexes/Delete.php:85-86
Timestamp: 2025-05-10T04:12:13.037Z
Learning: The class `Appwrite\Platform\Modules\Databases\Http\Databases\Collections\Indexes\Action` defines a method `getGrantParentNotFoundException()` which is used to handle cases where a parent resource (like a database collection) is not found.
Applied to files:
src/Appwrite/Platform/Modules/Projects/Http/Projects/XList.php
🧬 Code graph analysis (1)
src/Appwrite/Platform/Modules/Projects/Http/Projects/XList.php (3)
src/Appwrite/Platform/Action.php (2)
Action(15-210)applySelectQueries(175-209)src/Appwrite/Utopia/Database/Validator/Queries/Projects.php (1)
Projects(5-25)src/Appwrite/Utopia/Request.php (1)
Request(14-247)
⏰ 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: scan-pr / osv-scan
- GitHub Check: Check if utopia-php/database changed
- GitHub Check: scan
🔇 Additional comments (8)
src/Appwrite/Platform/Modules/Projects/Http/Projects/XList.php (8)
6-6: LGTM!The new imports are necessary and correctly used throughout the file.
Also applies to: 12-12, 14-14
30-31: LGTM!The static caching strategy is appropriate for the attribute-to-subQuery mapping, which is computed once and reused across requests.
70-70: LGTM!The Request injection is necessary for the
applySelectQueries()call in the action method.
76-76: LGTM!The updated signature correctly adds the
Requestparameter needed for selective query handling.
114-118: LGTM!The query handling logic is correct:
- Splitting into
selectQueriesandfilterQueriesusingQuery::groupByType()- Delegating to
find()for selective execution- Using
filterQueriesfor count is appropriate (per learnings, the database layer handles filter separation internally)
123-123: LGTM!The
applySelectQueries()call correctly applies model-level projection after database retrieval, ensuring only selected fields are returned in the response.
163-197: LGTM!The
find()method correctly implements selective query execution:
- Early returns (lines 165-167, 176-178): Properly handles cases with no selections or wildcard selections
- Attribute mapping (lines 180-182): Efficiently uses
array_flip()for O(1) lookups- Filter skipping (lines 184-190): Correctly identifies which subQuery filters to skip based on non-selected attributes
- Conditional skipFilters (lines 194-196): Only uses
skipFilters()when necessaryThe implementation aligns with the constraint that dot-path selects on subquery attributes are not supported (based on learnings).
38-41: ThegetName()method is a required framework method for action registration.Every Action class in the Appwrite framework must implement
getName()to provide a unique identifier used by Service classes when registering actions viaaddAction(ClassName::getName(), new ClassName()). This identifier is also used throughout the codebase for action lookup and routing. Removing or renaming this method would break action registration.
What does this PR do?
Allows
Query.selectwith correct management around subQueries.Test Plan
Manual, E2E Projects service tests.
Related PRs and Issues
N/A.
Checklist