Conversation
📝 WalkthroughWalkthroughThis pull request switches CSV export storage to an internal platform "default" bucket and removes the external bucketId parameter from the CSV export API. Controller action signatures were updated to inject dbForPlatform and no longer accept bucketId; bucket resolution now reads the 'default' bucket from the platform DB and migration options hardcode bucketId to 'default'. The migration worker and email flow now use a Document $user (not a userInternalId), create export file documents in the platform DB with explicit read permission for the exporting user, and generate JWT-protected download URLs. The storage push endpoint gained dbForPlatform and selects platform vs. project DB based on an internal JWT flag. Tests, a maintenance task, Deletes worker handling, and a new DELETE_TYPE_CSV_EXPORTS constant were added to support internal CSV export lifecycle and cleanup. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
✨ 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 |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
✨ Benchmark results
⚡ Benchmark Comparison
|
commit: |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/Appwrite/Platform/Workers/Migrations.php (1)
570-573: Simplify the empty user check logic.Calling
getInternalId()on an empty Document in the warning message is unnecessary and potentially confusing since the user is already empty.Apply this diff to simplify:
if ($user->isEmpty()) { - Console::warning("User not found for CSV export notification: {$user->getInternalId()}"); + Console::warning("User not found for CSV export notification"); return; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
app/config/specs/open-api3-1.8.x-console.jsonis excluded by!app/config/specs/**app/config/specs/open-api3-latest-console.jsonis excluded by!app/config/specs/**app/config/specs/swagger2-1.8.x-console.jsonis excluded by!app/config/specs/**app/config/specs/swagger2-latest-console.jsonis excluded by!app/config/specs/**
📒 Files selected for processing (3)
app/controllers/api/storage.php(2 hunks)src/Appwrite/Platform/Workers/Migrations.php(12 hunks)tests/e2e/Services/Migrations/MigrationsBase.php(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/Services/Migrations/MigrationsBase.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/Workers/Migrations.php
🧬 Code graph analysis (1)
app/controllers/api/storage.php (4)
src/Appwrite/Platform/Workers/Migrations.php (1)
action(93-131)src/Appwrite/Platform/Modules/Projects/Http/Projects/XList.php (1)
action(69-117)src/Appwrite/Platform/Modules/Proxy/Http/Rules/XList.php (1)
action(62-120)src/Appwrite/Auth/Auth.php (3)
Auth(18-515)isAppUser(441-448)isPrivilegedUser(421-432)
🔇 Additional comments (6)
src/Appwrite/Platform/Workers/Migrations.php (4)
14-22: LGTM: Import changes align with new permission handling.The updated imports properly support the new platform-scoped bucket and file permissions functionality.
492-516: LGTM: Platform file document created with proper user-scoped permissions.The file document is correctly created in the platform database with explicit read permissions for the requesting user using
Permission::read(Role::user(...)).
522-527: LGTM: JWT payload correctly includes internal flag.The
'internal' => trueflag in the JWT payload is essential for the storage endpoints to differentiate between platform-scoped and project-scoped files.
449-452: No issues found—verification confirms the 'default' bucket is properly initialized.The 'default' bucket is created during platform initialization in app/http.php, with a defensive check ensuring it only creates if absent. The error handling at lines 451-452 is a reasonable safeguard for unexpected scenarios. The code functions as designed.
app/controllers/api/storage.php (2)
1480-1484: LGTM: Platform database injection added correctly.The
dbForPlatformdependency is properly injected and added to the action signature following Appwrite conventions.
1501-1513: LGTM: Internal file routing logic is correct and backward compatible.The implementation properly:
- Extracts the
internalflag from JWT with a safe default (false)- Selects the appropriate database (platform vs. project) based on the flag
- Maintains authorization skip for both internal and external file access paths
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/controllers/api/storage.php (1)
1502-1508: I need to examine the actual code context in storage.php to understand how the JWT is decoded and validated. Let me search the repository:Based on my verification, I can now provide the rewritten review comment. The key findings are:
- The JWT with the
internalflag is only generated in the trusted Migrations worker process using the system's OpenSSL key (a server-side secret)- JWTs with HS256 cannot be forged without the server secret key
- The concern about variable naming remains valid and should be addressed
Rename the reassigned database variable for clarity.
The reassignment of
$dbForProjecton line 1503 is misleading—the variable may reference either the platform or project database depending on theinternalflag. To improve code maintainability, use a more descriptive name like$dbForStorage:- $isInternal = $decoded['internal'] ?? false; - $dbForProject = $isInternal ? $dbForPlatform : $dbForProject; + $isInternal = $decoded['internal'] ?? false; + $dbForStorage = $isInternal ? $dbForPlatform : $dbForProject; - $bucket = Authorization::skip(fn () => $dbForProject->getDocument('buckets', $bucketId)); + $bucket = Authorization::skip(fn () => $dbForStorage->getDocument('buckets', $bucketId));Apply this change to line 1513 for the file lookup as well.
The JWT signature is verified server-side using the system's OpenSSL key, so forging the
internalflag is not possible without access to the server secret.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
app/config/specs/open-api3-1.8.x-console.jsonis excluded by!app/config/specs/**app/config/specs/open-api3-latest-console.jsonis excluded by!app/config/specs/**app/config/specs/swagger2-1.8.x-console.jsonis excluded by!app/config/specs/**app/config/specs/swagger2-latest-console.jsonis excluded by!app/config/specs/**
📒 Files selected for processing (4)
app/controllers/api/storage.php(2 hunks)app/init/constants.php(1 hunks)src/Appwrite/Platform/Tasks/Maintenance.php(2 hunks)src/Appwrite/Platform/Workers/Deletes.php(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-09T04:06:04.338Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Delete.php:88-95
Timestamp: 2025-05-09T04:06:04.338Z
Learning: It's acceptable to use DATABASE_TYPE_DELETE_COLLECTION event type for both collections and tables because workers process these events based on whether setCollection() or setTable() was called, not on the event type itself.
Applied to files:
app/init/constants.php
🧬 Code graph analysis (1)
app/controllers/api/storage.php (5)
src/Appwrite/Platform/Tasks/Maintenance.php (1)
action(35-100)src/Appwrite/Platform/Workers/Deletes.php (1)
action(73-196)src/Appwrite/Platform/Workers/Migrations.php (1)
action(93-131)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/XList.php (1)
action(73-128)src/Appwrite/Auth/Auth.php (4)
Auth(18-515)isAppUser(441-448)getRoles(456-502)isPrivilegedUser(421-432)
⏰ 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 (Messaging)
- GitHub Check: E2E Service Test (VCS)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Avatars)
- 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: Setup & Build SDK
- GitHub Check: scan
🔇 Additional comments (4)
app/controllers/api/storage.php (1)
1481-1481: LGTM: Platform database injection added correctly.The injection of
dbForPlatformfollows the established pattern and is necessary for supporting internal bucket access in CSV export flows.Also applies to: 1485-1485
app/init/constants.php (1)
141-141: LGTM: New deletion type constant added correctly.The
DELETE_TYPE_CSV_EXPORTSconstant follows the established naming convention and is properly positioned with other deletion type constants.src/Appwrite/Platform/Tasks/Maintenance.php (1)
98-98: LGTM: CSV export deletion maintenance task added correctly.The
notifyDeleteCSVExportsmethod follows the same pattern as other maintenance notification methods and is properly integrated into the maintenance loop.Also applies to: 110-115
src/Appwrite/Platform/Workers/Deletes.php (1)
182-184: LGTM: CSV exports deletion case added correctly.The new case for
DELETE_TYPE_CSV_EXPORTSis properly integrated into the deletion worker's switch statement.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Appwrite/Platform/Workers/Migrations.php (1)
437-447: Validate userInternalId before use and avoid exposing internal IDs.The code doesn't validate that
userInternalIdexists or is non-empty before querying, and the error message exposes the internal user ID.This issue was previously flagged. Apply this diff to add validation and improve the error message:
$bucketId = 'default'; // Always use platform default bucket $filename = $options['filename'] ?? 'export_' . \time(); $userInternalId = $options['userInternalId'] ?? ''; + +if (empty($userInternalId)) { + throw new \Exception('User internal ID is required for CSV export'); +} + $user = $this->dbForPlatform->findOne('users', [ Query::equal('$sequence', [$userInternalId]) ]); if ($user->isEmpty()) { - throw new \Exception('User ' . $userInternalId . ' not found'); + throw new \Exception('User not found'); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Platform/Workers/Migrations.php(12 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/Workers/Migrations.php
📚 Learning: 2025-06-19T09:20:03.312Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 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/Workers/Migrations.php
🧬 Code graph analysis (1)
src/Appwrite/Platform/Workers/Migrations.php (1)
src/Appwrite/Utopia/Response/Model/Document.php (1)
Document(8-115)
⏰ 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). (7)
- GitHub Check: Unit Test
- GitHub Check: E2E General Test
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Check if utopia-php/database changed
- GitHub Check: Setup & Build SDK
- GitHub Check: Linter
- GitHub Check: scan
🔇 Additional comments (6)
src/Appwrite/Platform/Workers/Migrations.php (6)
14-22: LGTM! Authorization and permission imports are appropriate.The new imports for
AuthorizationException,Permission,Role, andAuthorizationvalidator align with the platform DB access patterns and explicit permission handling introduced in this PR.
449-449: LGTM! Authorization skip is appropriate for platform bucket access.Using
Authorization::skip()to access the platform's default bucket is correct, as this is an internal operation that needs to bypass normal authorization checks.
492-516: LGTM! Platform DB usage and explicit permissions are correctly implemented.The document creation in the platform DB with explicit read permissions for the exporting user (
Permission::read(Role::user($user->getId()))) properly secures access to the CSV export file.
519-527: LGTM! JWT internal flag is appropriate.Adding
'internal' => trueto the JWT payload correctly marks this as an internal/platform resource, which aligns with the platform DB storage approach.
646-653: LGTM! Filename sanitization is appropriate.The
sanitizeFilenamemethod correctly removes problematic filesystem characters and handles edge cases with a fallback to 'export'.
549-549: Fix the parameter type in docblock.The docblock incorrectly states
@param string $userwhen the actual parameter type isDocument $user.Apply this diff:
- * @param Document $user The user who triggered the operation + * @param Document $user The user who triggered the operationWait, I see the docblock on line 549 already says
@param Document $user. Let me re-check... Actually, looking at the code, line 549 shows:* @param Document $user The user who triggered the operationThis appears to already be correct. The past review comment may have been addressed. Let me skip this as it's already fixed.
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