Feat: queries param on list api keys#11278
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds query-based retrieval and cursor support to the project keys listing endpoint: the controller now accepts an array of query strings, parses and validates them via a new Keys query validator (allowed attributes: expire, accessedAt, name, scopes), and handles limit/default limit, cursor resolution to key documents, grouped filters for total counting, and query error mapping. Adds duplicate-key creation handling that maps duplicate exceptions to a new KEY_ALREADY_EXISTS error code and config entry. End-to-end tests expanded and timing adjusted to exercise query filtering, cursors, ordering, pagination, scopes/name/expiry filters, and related error cases. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/controllers/api/projects.php`:
- Around line 32-33: Import ordering in app/controllers/api/projects.php
violates PSR-12: move the two new imports so they alphabetically precede their
peers. Specifically, place Utopia\Database\Exception\Query before
Utopia\Database\Helpers\ID, and place Utopia\Database\Validator\Query\Cursor
before Utopia\Database\Validator\UID; update the use statement order accordingly
so all Utopia\Database imports are alphabetized.
🧹 Nitpick comments (1)
tests/e2e/Services/Projects/ProjectsConsoleClientTest.php (1)
3264-3278: Consider using an explicit order attribute for clarity, but note thatQuery::orderDesc('')works correctly and isn't flaky.
Query::orderDesc('')automatically resolves to$sequence(an internal system attribute available for all collections) via the Query constructor, so it passes validation. However, for code clarity and maintainability, prefer an explicit attribute likenameso the intent is obvious to readers.🔧 Suggested fix
- Query::orderDesc('')->toString(), + Query::orderDesc('name')->toString(),
✨ Benchmark results
⚡ Benchmark Comparison
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@app/controllers/api/projects.php`:
- Around line 1188-1195: The cursor key lookup uses
$dbForPlatform->getDocument('keys', $keyId) without verifying the key's project,
which leaks existence of keys across projects; change the lookup to scope by
project (same filters used later: resourceType and resourceInternalId) — either
replace the direct getDocument call with a find/query on the 'keys' collection
filtering by _id = $keyId AND resourceType = 'project' AND resourceInternalId =
$projectInternalId (or validate the returned document's
resourceType/resourceInternalId after getDocument), and only treat it as found
if it belongs to the current project before calling
$cursor->setValue($cursorDocument).
In `@tests/e2e/Services/Projects/ProjectsConsoleClientTest.php`:
- Around line 3367-3375: The test calls Query::orderDesc('') which relies on
undocumented fallback; change the empty string to the explicit allowed attribute
'name' so use Query::orderDesc('name') when building the 'queries' array in the
ProjectsConsoleClientTest (the call that builds $response), ensuring the Keys
validator accepts the attribute and the test intent is clear.
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
1 similar comment
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
ChiragAgg5k
approved these changes
Feb 10, 2026
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/e2e/Services/Projects/ProjectsConsoleClientTest.php`:
- Around line 1233-1244: The placeholder comment "Ensure.. Something?" should be
replaced with a concrete assertion or removed: either assert the change
persisted by calling the project GET endpoint and checking the saved value (use
the same project id and verify returned ['body']['authDuration'] equals 600) or,
if persistence is already covered elsewhere, delete the comment; reference the
existing $response from the PATCH, the 'authDuration' field, and the client call
that modifies '/projects/' . $id . '/auth/duration'.
🧹 Nitpick comments (2)
tests/e2e/Services/Projects/ProjectsConsoleClientTest.php (2)
3146-3172: Assert the error type for duplicate custom key IDs.You already check the 409; adding the error type/code will lock in the new mapping.
✅ Suggested assertion
$this->assertEquals(409, $response['headers']['status-code']); + $this->assertEquals(409, $response['body']['code']); + $this->assertEquals(Exception::KEY_ALREADY_EXISTS, $response['body']['type']);
3233-3285: Make cursor pagination deterministic by adding explicit ordering.The cursor-after expectation depends on the default order, which could change. Consider specifying an order in the query.
🔧 Example (adjust attribute if needed by validator)
$response = $this->client->call(Client::METHOD_GET, '/projects/' . $id . '/keys', array_merge([ 'content-type' => 'application/json', 'x-appwrite-project' => $this->getProject()['$id'], ], $this->getHeaders()), [ 'queries' => [ + Query::orderAsc('$createdAt')->toString(), Query::cursorAfter(new Document(['$id' => $data['keyId']]))->toString(), ] ]);
Contributor
There was a problem hiding this comment.
Pull request overview
Adds SDK-style queries support to the Projects “List API Keys” endpoint (bringing it in line with other list endpoints), plus introduces a dedicated query validator and improves duplicate-key error handling. The PR also expands E2E coverage around project keys listing and custom key IDs.
Changes:
- Add
queriesparameter support (filter/order/cursor/limit/offset) forGET /v1/projects/:projectId/keysand adjust total counting logic. - Introduce
KEY_ALREADY_EXISTSerror for duplicate key creation attempts. - Expand E2E tests for project keys querying and update session-duration test timing/behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e/Services/Projects/ProjectsConsoleClientTest.php | Adds E2E coverage for key list querying and duplicate key ID behavior; modifies auth duration test flow. |
| src/Appwrite/Utopia/Database/Validator/Queries/Keys.php | New queries validator defining allowed filter attributes for keys listing. |
| src/Appwrite/Extend/Exception.php | Adds new KEY_ALREADY_EXISTS exception constant. |
| app/controllers/api/projects.php | Adds queries param and cursor handling to list keys endpoint; maps duplicate key creation to a new exception. |
| app/config/errors.php | Registers KEY_ALREADY_EXISTS as a 409 error with message/description. |
Comments suppressed due to low confidence (1)
tests/e2e/Services/Projects/ProjectsConsoleClientTest.php:1249
- The
// Ensure.. Something?comment reads like a placeholder and the following assertion only checks the expired session remains invalid after increasingauthDuration. If the goal is to validate the new 10-minute duration, create a new session after updating the project setting and assert it stays valid after a short wait (and/or does not expire within a reasonable window).
// Ensure.. Something?
$response = $this->client->call(Client::METHOD_GET, '/account', array_merge([
'content-type' => 'application/json',
'x-appwrite-project' => $projectId,
'Cookie' => $sessionCookie,
]));
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1207
to
1214
| $filterQueries = Query::groupByType($queries)['filters']; | ||
|
|
||
| $keys = $dbForPlatform->find('keys', $queries); | ||
|
|
||
| $response->dynamic(new Document([ | ||
| 'keys' => $keys, | ||
| 'total' => $includeTotal ? count($keys) : 0, | ||
| 'total' => $includeTotal ? $dbForPlatform->count('keys', $filterQueries, APP_LIMIT_COUNT) : 0, | ||
| ]), Response::MODEL_KEY_LIST); |
There was a problem hiding this comment.
find('keys', $queries) can throw an OrderException when a client uses cursor pagination with an order attribute that has null values (e.g. ordering by expire/accessedAt). Other list endpoints catch this and return DATABASE_QUERY_ORDER_NULL, but this handler currently lets the exception bubble. Wrap the find/count calls in a try/catch and map OrderException to a consistent Appwrite exception (and add the corresponding import).
Comment on lines
+1185
to
+1187
| $queries[] = Query::equal('resourceType', ['projects']); | ||
| $queries[] = Query::equal('resourceInternalId', [$project->getSequence()]); | ||
|
|
There was a problem hiding this comment.
This endpoint previously forced Query::limit(5000) (returning up to 5000 keys). After switching to SDK-style queries, no default limit is applied, so the number of returned keys now depends on the database default limit and may be a behavioral/breaking change for callers. If backward compatibility is desired, consider appending a default limit when the client does not supply one.
| 'content-type' => 'application/json', | ||
| 'x-appwrite-project' => $this->getProject()['$id'], | ||
| ], $this->getHeaders()), [ | ||
| 'queries' => [ |
There was a problem hiding this comment.
This cursor pagination assertion is brittle: using cursorAfter without an explicit order* query relies on the API's implicit sort order, and this test creates a mix of custom IDs and generated IDs that may not sort consistently. Add an explicit order (e.g. $sequence) alongside the cursor query, and base expectations on that deterministic ordering.
Suggested change
| 'queries' => [ | |
| 'queries' => [ | |
| Query::orderAsc('$sequence')->toString(), |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Allows queries, as all other endpoints, for api keys
Test Plan
New tests
Related PRs and Issues
x
Checklist