Conversation
📝 WalkthroughWalkthroughThis pull request removes the legacy monolithic storage controller at app/controllers/api/storage.php and introduces a modular Storage platform under src/Appwrite/Platform/Modules/Storage/. The new module registers an HTTP service that exposes many granular action classes (Create, Get, Update, Delete, XList) for buckets and files, plus specialized handlers for file Upload/Create, Download, Preview, View, Push, and Usage endpoints. The Appwrite platform registration is updated to include the Storage module. Individual actions encapsulate validation, authorization, storage device interaction, encryption/compression, streaming, and event queuing. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 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 |
lohanidamodar
changed the title
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
✨ Benchmark results
⚡ Benchmark Comparison
|
lohanidamodar
force-pushed
the
feat-storage-module
branch
from
57c1e22 to
8cf12f6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (10)
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Create.php (2)
96-109: Unused$modeparameter should be removed.The
$modeparameter is injected (line 90) but never used in the action method. This is flagged by static analysis.🔎 Proposed fix
public function action( string $bucketId, string $fileId, mixed $file, ?array $permissions, Request $request, Response $response, Database $dbForProject, Document $user, Event $queueForEvents, - string $mode, Device $deviceForFiles, Device $deviceForLocal ) {Also remove line 90:
->inject('queueForEvents') - ->inject('mode') ->inject('deviceForFiles')
376-386: Consider extracting duplicate authorization check into a helper.The create permission validation logic is duplicated in two places (lines 382-385 and 427-430). While functional, extracting this into a private method would improve maintainability.
Also applies to: 421-436
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/View/Get.php (1)
75-85: Remove unused$modeparameter.The
$modeparameter is injected but never used in the action method.🔎 Proposed fix
Remove from action signature and injection:
- ->inject('mode')public function action( string $bucketId, string $fileId, ?string $token, Response $response, Request $request, Database $dbForProject, - string $mode, Document $resourceToken, Device $deviceForFiles ) {src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/XList.php (1)
67-75: Remove unused$modeparameter.The
$modeparameter is injected (line 63) but never used in the action method.🔎 Proposed fix
- ->inject('mode')public function action( string $bucketId, array $queries, string $search, bool $includeTotal, Response $response, Database $dbForProject, - string $mode ) {src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Push/Get.php (2)
57-88: LGTM on JWT validation, but remove unused$modeparameter.The JWT validation and database selection logic is well-implemented. However, the
$modeparameter is unused and should be removed.🔎 Proposed fix for unused parameter
- ->inject('mode')public function action( string $bucketId, string $fileId, string $jwt, Response $response, Request $request, Database $dbForProject, Database $dbForPlatform, Document $project, - string $mode, Device $deviceForFiles ) {
102-205: Significant code duplication withView/Get.php.The file reading, decryption, decompression, range handling, and streaming logic (lines 102-205) is nearly identical to
View/Get.php(lines 119-223). Consider extracting this shared functionality into a common trait or helper class to improve maintainability.src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Download/Get.php (1)
74-84: Unused parameters$tokenand$modein action signature.As noted by static analysis,
$tokenand$modeare declared but unused. The comment on line 63 explains$tokenis for SDK generation and handled viaresourceTokeninjection, which is acceptable. However,$modehas no such explanation.Consider adding a comment explaining why
$modeis injected but unused, or remove it if not needed:public function action( string $bucketId, string $fileId, - ?string $token, + ?string $token, // Used by SDK generator, handled via resourceToken injection Request $request, Response $response, Database $dbForProject, - string $mode, + string $mode, // Reserved for future use or required by framework Document $resourceToken, Device $deviceForFiles ) {src/Appwrite/Platform/Modules/Storage/Http/Usage/Get.php (1)
78-125: Consider extracting shared usage aggregation logic.The data aggregation pattern (querying stats, formatting time-series data) is nearly identical between this file and
XList.php. Consider extracting this logic into a shared helper method or trait to improve maintainability and reduce duplication.src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Preview/Get.php (2)
110-110: Clarify unused parameter intent.Static analysis correctly identifies that
$tokenis unused in the action method. The comment on lines 84-85 explains it's for the SDK generator and utilized inresources.phpfor resource token handling. Consider prefixing the parameter with an underscore (e.g.,$_token) to make this intent explicit and suppress the static analysis warning.
241-243: Simplify redundant opacity condition.The condition
if (!empty($opacity) || $opacity === 0)will always evaluate to true for any valid float in the range [0,1], making it redundant. Consider simplifying toif ($opacity !== 1)if the intent is to skip the default value, or remove the condition entirely ifsetOpacity()should always be called.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
app/controllers/api/storage.phpsrc/Appwrite/Platform/Appwrite.phpsrc/Appwrite/Platform/Modules/Storage/Http/Buckets/Create.phpsrc/Appwrite/Platform/Modules/Storage/Http/Buckets/Delete.phpsrc/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Create.phpsrc/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Delete.phpsrc/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Download/Get.phpsrc/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Get.phpsrc/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Preview/Get.phpsrc/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Push/Get.phpsrc/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Update.phpsrc/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/View/Get.phpsrc/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/XList.phpsrc/Appwrite/Platform/Modules/Storage/Http/Buckets/Get.phpsrc/Appwrite/Platform/Modules/Storage/Http/Buckets/Update.phpsrc/Appwrite/Platform/Modules/Storage/Http/Buckets/XList.phpsrc/Appwrite/Platform/Modules/Storage/Http/Usage/Get.phpsrc/Appwrite/Platform/Modules/Storage/Http/Usage/XList.phpsrc/Appwrite/Platform/Modules/Storage/Module.phpsrc/Appwrite/Platform/Modules/Storage/Services/Http.php
💤 Files with no reviewable changes (1)
- app/controllers/api/storage.php
🧰 Additional context used
🧠 Learnings (1)
📚 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/Storage/Http/Buckets/Get.php
🧬 Code graph analysis (13)
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Get.php (3)
src/Appwrite/Extend/Exception.php (1)
Exception(7-472)src/Appwrite/Utopia/Response.php (2)
Response(159-983)dynamic(703-736)src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Get.php (2)
Get(17-89)getName(21-24)
src/Appwrite/Platform/Modules/Storage/Http/Usage/Get.php (1)
src/Appwrite/Platform/Modules/Storage/Http/Usage/XList.php (3)
getName(22-25)__construct(27-53)action(55-119)
src/Appwrite/Platform/Modules/Storage/Module.php (1)
src/Appwrite/Platform/Appwrite.php (1)
Appwrite(17-32)
src/Appwrite/Platform/Modules/Storage/Services/Http.php (7)
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Delete.php (2)
Delete(18-89)getName(22-25)src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Get.php (2)
Get(17-89)getName(21-24)src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/View/Get.php (2)
Get(29-225)getName(33-36)src/Appwrite/Platform/Modules/Storage/Http/Usage/Get.php (2)
Get(20-137)getName(24-27)src/Appwrite/Platform/Modules/Storage/Http/Buckets/Get.php (2)
Get(15-65)getName(19-22)src/Appwrite/Platform/Modules/Storage/Http/Usage/XList.php (2)
XList(18-120)getName(22-25)src/Appwrite/Platform/Modules/Storage/Http/Buckets/XList.php (2)
XList(22-117)getName(26-29)
src/Appwrite/Platform/Modules/Storage/Http/Buckets/XList.php (2)
src/Appwrite/Utopia/Database/Validator/Queries/Buckets.php (1)
Buckets(5-25)src/Appwrite/Platform/Modules/Storage/Http/Usage/XList.php (4)
XList(18-120)getName(22-25)__construct(27-53)action(55-119)
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Get.php (1)
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Get.php (4)
Get(15-65)getName(19-22)__construct(24-50)action(52-64)
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Create.php (3)
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Update.php (3)
getName(31-34)__construct(36-76)action(78-130)src/Appwrite/Platform/Modules/Storage/Http/Buckets/Get.php (3)
getName(19-22)__construct(24-50)action(52-64)src/Appwrite/Platform/Modules/Storage/Http/Buckets/XList.php (3)
getName(26-29)__construct(31-59)action(61-116)
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/XList.php (2)
src/Appwrite/Platform/Modules/Storage/Http/Usage/XList.php (4)
XList(18-120)getName(22-25)__construct(27-53)action(55-119)src/Appwrite/Platform/Modules/Storage/Http/Buckets/XList.php (4)
XList(22-117)getName(26-29)__construct(31-59)action(61-116)
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/View/Get.php (1)
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Get.php (4)
Get(17-89)getName(21-24)__construct(26-53)action(55-88)
src/Appwrite/Platform/Modules/Storage/Http/Usage/XList.php (1)
src/Appwrite/Platform/Modules/Storage/Http/Usage/Get.php (3)
getName(24-27)__construct(29-58)action(60-136)
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Update.php (3)
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Update.php (4)
Update(24-156)getName(28-31)__construct(33-66)action(68-155)src/Appwrite/Platform/Modules/Storage/Http/Buckets/Delete.php (3)
getName(22-25)__construct(27-59)action(61-88)src/Appwrite/Platform/Modules/Storage/Http/Buckets/Get.php (3)
getName(19-22)__construct(24-50)action(52-64)
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Update.php (2)
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Delete.php (3)
getName(26-29)__construct(31-68)action(70-149)src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Get.php (3)
getName(21-24)__construct(26-53)action(55-88)
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Delete.php (2)
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Get.php (3)
getName(19-22)__construct(24-50)action(52-64)src/Appwrite/Platform/Modules/Storage/Http/Buckets/XList.php (3)
getName(26-29)__construct(31-59)action(61-116)
🪛 PHPMD (2.15.0)
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Create.php
106-106: Avoid unused parameters such as '$mode'. (undefined)
(UnusedFormalParameter)
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Push/Get.php
66-66: Avoid unused parameters such as '$mode'. (undefined)
(UnusedFormalParameter)
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Download/Get.php
77-77: Avoid unused parameters such as '$token'. (undefined)
(UnusedFormalParameter)
81-81: Avoid unused parameters such as '$mode'. (undefined)
(UnusedFormalParameter)
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/XList.php
74-74: Avoid unused parameters such as '$mode'. (undefined)
(UnusedFormalParameter)
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/View/Get.php
78-78: Avoid unused parameters such as '$token'. (undefined)
(UnusedFormalParameter)
82-82: Avoid unused parameters such as '$mode'. (undefined)
(UnusedFormalParameter)
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Preview/Get.php
110-110: Avoid unused parameters such as '$token'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (19)
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Create.php (3)
119-162: LGTM!The permission handling logic is well-structured with proper authorization checks, role validation for non-privileged users, and appropriate default permission assignment.
190-214: LGTM!The chunked upload logic correctly handles content-range parsing with appropriate backward compatibility considerations noted in the TODO comments.
265-300: LGTM!The antivirus scanning and compression logic correctly handles size limits and storage type constraints. The compression algorithm fallback to NONE for large files is properly documented.
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Update.php (1)
112-129: LGTM!The bucket update logic correctly applies all attribute changes, updates the collection permissions, and queues the appropriate event.
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/View/Get.php (2)
145-147: Range validation may reject valid single-byte ranges.The condition
$start >= $endrejects ranges where$start == $end, but RFC 7233 allows single-byte ranges likebytes=0-0. Consider whether this is intentional.
195-223: LGTM!The file streaming logic correctly handles multiple scenarios: in-memory decrypted/decompressed content, range requests, and chunked streaming for large files.
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/XList.php (1)
128-149: LGTM!The query execution logic correctly handles authorization based on
fileSecurity, and exception handling forNotFoundException,OrderException, andQueryExceptionis comprehensive.src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Update.php (1)
68-154: LGTM!The file update action correctly handles authorization, permission validation, and conditional updates based on
fileSecurity. The pattern is consistent with other file endpoints.src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Delete.php (2)
107-139: LGTM!The deletion logic correctly handles both complete files and incomplete chunked uploads, with proper ordering (device deletion before database deletion) and comprehensive error handling.
141-149: LGTM!Event handling correctly includes the file payload for downstream consumers before returning the 204 No Content response.
src/Appwrite/Platform/Appwrite.php (1)
13-30: LGTM!The Storage module import and registration follow the established pattern used by other modules in the platform.
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Get.php (1)
52-64: LGTM!The action method correctly fetches the bucket document, validates its existence, and returns the appropriate response. Error handling is consistent with other storage endpoints.
src/Appwrite/Platform/Modules/Storage/Services/Http.php (1)
25-50: Action registration is well-structured.The constructor cleanly organizes actions by category (Buckets, Files, Usage) with consistent naming patterns. The use of static
getName()methods for action registration ensures type-safe wiring.src/Appwrite/Platform/Modules/Storage/Http/Buckets/Delete.php (1)
61-88: LGTM!The delete action properly validates bucket existence, removes the document, queues the deletion for downstream processing (files cleanup), and emits the bucket-delete event with the correct payload. The response correctly returns 204 No Content.
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Get.php (1)
55-88: LGTM!The authorization flow correctly handles the bucket/file security model:
- API keys and privileged users bypass the disabled bucket check
- File-level security is respected by conditionally applying authorization during file fetch
- Appropriate exceptions are thrown for missing resources and unauthorized access
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Create.php (2)
103-107: Good defensive check for files collection configuration.The validation that the files collection schema exists in the config prevents cryptic errors during bucket creation if the configuration is missing.
136-154: The suggested optimization is not feasible. ThecreateDocumentmethod does not populate the$sequencefield for single document operations, which is only available after a separategetSequences()database query. ThegetDocumentrefetch at line 152 is necessary to obtain the sequence number required for thecreateCollectioncall on line 154.Likely an incorrect or invalid review comment.
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Download/Get.php (1)
154-191: Decryption and decompression flow is well-structured.The code correctly handles the pipeline: decrypt first (if encrypted), then decompress (if compressed), with proper fallback to reading from device when no transformation is needed. Range handling for transformed content also works correctly.
src/Appwrite/Platform/Modules/Storage/Http/Buckets/XList.php (1)
61-116: LGTM!The action method correctly implements bucket listing with:
- Query parsing and validation
- Search filter support
- Cursor-based pagination with proper document resolution
- Total count calculation (optional)
- Appropriate exception mapping for query and order errors
| if ($end === null || $end - $start > APP_STORAGE_READ_BUFFER) { | ||
| $end = min(($start + MAX_OUTPUT_CHUNK_SIZE - 1), ($size - 1)); | ||
| } | ||
|
|
||
| if ($unit !== 'bytes' || $start >= $end || $end >= $size) { | ||
| throw new Exception(Exception::STORAGE_INVALID_RANGE); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
RFC 7233 HTTP range request single byte range specification
💡 Result:
Key points from RFC 7233 (byte ranges):
-
Syntax (byte ranges): byte-ranges-specifier = "bytes" "=" byte-range-set
byte-range-spec = first-byte-pos "-" [ last-byte-pos ] (first/last are decimal, positions are inclusive, zero-based). [1][2] -
A single-byte range is expressed by specifying the same index for first and last, e.g.:
- bytes=0-0 (the first byte)
- bytes=999-999 (the single byte at offset 999). [1][2]
-
A suffix single-byte (last byte of representation) can be requested with a suffix-byte-range-spec of length 1:
- bytes=-1 (the final byte). [1][2]
-
A byte-range-spec is invalid if last-byte-pos < first-byte-pos. Servers may ignore Range or coalesce/omit unsatisfiable ranges; a satisfiable set requires at least one range overlapping the representation. [1][2]
Sources:
[1] RFC 7233 (IETF / RFC Editor).
[2] RFC 7233 (HTTP Working Group / httpwg HTML).
🏁 Script executed:
# Let me examine the actual code context around these lines
cat -n src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Download/Get.php | sed -n '120,145p'Repository: appwrite/appwrite
Length of output: 178
🏁 Script executed:
# Try to find and read the file
find . -name "Get.php" -path "*Storage*Download*" 2>/dev/null | head -5Repository: appwrite/appwrite
Length of output: 136
🏁 Script executed:
# Also search for context around the range validation
rg "start >= end" --type php -A 3 -B 3 2>/dev/null | head -40Repository: appwrite/appwrite
Length of output: 43
🏁 Script executed:
# Read the file to understand the context and index semantics
wc -l ./src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Download/Get.phpRepository: appwrite/appwrite
Length of output: 140
🏁 Script executed:
# Read the relevant section
cat -n ./src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Download/Get.php | sed -n '100,150p'Repository: appwrite/appwrite
Length of output: 2482
Fix range validation to allow single-byte requests.
The condition $start >= $end at line 136 incorrectly rejects valid single-byte ranges. Per RFC 7233, ranges like bytes=0-0 (requesting a single byte at position 0) are valid. Since the code uses inclusive zero-based indices (evidenced by the Content-Length formula $end - $start + 1 at line 143), the condition should be $start > $end instead.
🤖 Prompt for AI Agents
In src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Download/Get.php
around lines 132 to 138, the range validation currently rejects single-byte
ranges due to the check `$start >= $end`; change this to `$start > $end` so
inclusive ranges like `bytes=0-0` are accepted, keep the `$unit !== 'bytes'` and
`$end >= $size` checks intact, and ensure the logic still enforces
APP_STORAGE_READ_BUFFER/MAX_OUTPUT_CHUNK_SIZE clipping as before.
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/XList.php
Outdated
Show resolved
Hide resolved
| $format = match ($days['period']) { | ||
| '1h' => 'Y-m-d\\TH:00:00.000P', | ||
| '1d' => 'Y-m-d\\T00:00:00.000P', | ||
| }; |
There was a problem hiding this comment.
Add a default case to the match expression.
The match expression lacks a default case. If $days['period'] contains a value other than '1h' or '1d', PHP will throw an UnhandledMatchError, causing the endpoint to fail.
🔎 Proposed fix
$format = match ($days['period']) {
'1h' => 'Y-m-d\\TH:00:00.000P',
'1d' => 'Y-m-d\\T00:00:00.000P',
+ default => 'Y-m-d\\T00:00:00.000P',
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $format = match ($days['period']) { | |
| '1h' => 'Y-m-d\\TH:00:00.000P', | |
| '1d' => 'Y-m-d\\T00:00:00.000P', | |
| }; | |
| $format = match ($days['period']) { | |
| '1h' => 'Y-m-d\\TH:00:00.000P', | |
| '1d' => 'Y-m-d\\T00:00:00.000P', | |
| default => 'Y-m-d\\T00:00:00.000P', | |
| }; |
🤖 Prompt for AI Agents
In src/Appwrite/Platform/Modules/Storage/Http/Usage/Get.php around lines 108 to
111, the match expression for $days['period'] is missing a default arm which can
cause an UnhandledMatchError for unexpected values; add a default branch to the
match that either assigns a sensible fallback format (e.g., same as '1d' or a
safe ISO format) or throws a clear, descriptive exception with a helpful
message, ensuring all possible inputs are handled and the endpoint cannot fail
with an unhandled match error.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/XList.php (1)
74-74: Consider removing the unused$modeparameter.The
$modeparameter is injected but never used in the action method. If it's not needed for future functionality, consider removing it from both the injection (line 63) and the method signature to keep the code clean.🔎 Proposed fix
If the parameter is not needed:
->inject('response') ->inject('dbForProject') - ->inject('mode') ->callback($this->action(...));public function action( string $bucketId, array $queries, string $search, bool $includeTotal, Response $response, - Database $dbForProject, - string $mode + Database $dbForProject ) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/XList.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-23T08:06:38.889Z
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10546
File: src/Appwrite/Platform/Workers/Migrations.php:144-148
Timestamp: 2025-10-23T08:06:38.889Z
Learning: In the Appwrite codebase, migration workers receive already-validated data from queued jobs. Query validation using Query::parseQueries() happens at the API endpoint level (with try-catch for QueryException) before jobs are queued, so workers in src/Appwrite/Platform/Workers/Migrations.php don't need redundant validation.
Applied to files:
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/XList.php
🪛 PHPMD (2.15.0)
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/XList.php
74-74: Avoid unused parameters such as '$mode'. (undefined)
(UnusedFormalParameter)
⏰ 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). (2)
- GitHub Check: Benchmark
- GitHub Check: scan
🔇 Additional comments (6)
src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/XList.php (6)
35-65: LGTM! Endpoint configuration is well-structured.The constructor properly configures the HTTP endpoint with appropriate validators, parameters, and SDK metadata. The use of the Files validator for queries and the inclusion of multiple auth types follow best practices.
76-83: LGTM! Bucket validation logic is correct.The bucket retrieval appropriately uses
Authorization::skipsince bucket-level permissions are validated subsequently. The privileged user check correctly allows API keys and privileged users to access disabled buckets.
92-96: LGTM! Query parsing properly handles exceptions.The try-catch block correctly handles
QueryExceptionand converts it to a standardized API exception. This properly addresses the concern raised in the previous review.
102-130: LGTM! Cursor pagination is correctly implemented.The cursor validation and document retrieval logic properly handles authorization:
- When
fileSecurityis enabled and the user lacks bucket-level read permission, file-level permissions are checked- Otherwise, authorization is skipped for users with bucket-level access
- Error handling for missing cursor documents is appropriate
132-148: LGTM! Query execution and error handling are robust.The authorization logic correctly mirrors the cursor handling:
- File-level permissions are checked when
fileSecurityis enabled and bucket-level read permission is absent- Bucket-level access bypasses file-level checks
- Exception handling is comprehensive, with particularly helpful error messages for
OrderExceptionin cursor pagination
150-153: LGTM! Response construction is correct.The response properly wraps the files array and total count in a Document and uses the appropriate
MODEL_FILE_LISTmodel.
2 participants
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