Conversation
# Conflicts: # app/controllers/api/migrations.php # composer.lock
# Conflicts: # composer.lock
# Conflicts: # composer.lock
# Conflicts: # composer.lock
📝 WalkthroughWalkthroughAdds CSV export support and localization strings for CSV export emails. Introduces POST /v1/migrations/csv/exports and renames the CSV import route to /v1/migrations/csv/imports (alias preserved). Renames public resource deviceForImports → deviceForMigrations and registers deviceForFiles; updates worker and init wiring to inject deviceForMigrations, deviceForFiles, queueForMails, and plan. Extends migration worker to handle DestinationCSV (filename sanitization, CSV destination creation), create bucket file records, send localized emails with JWT download links, and sanitize stored payloads/errors. Adds an "options" JSON rule to the Migration response model, a docker-compose uploads volume, and end-to-end CSV export tests (duplicate test block present). Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas needing extra attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
✨ Benchmark results
⚡ Benchmark Comparison
|
# Conflicts: # composer.json # composer.lock
commit: |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
tests/e2e/Services/Migrations/MigrationsBase.php (1)
1198-1433: CSV export E2E looks comprehensive.Creates data, exports, verifies file, JWT download, and email. Consider reducing flakiness by polling for mail instead of a single getLastEmail call.
- $lastEmail = $this->getLastEmail(); + $lastEmail = []; + $this->assertEventually(function () use (&$lastEmail) { + $lastEmail = $this->getLastEmail(); + return !empty($lastEmail); + }, 15000, 500);app/controllers/api/migrations.php (2)
310-355: CSV import: device swap and file handling look right.Decryption/decompression path then write/transfer into deviceForMigrations is correct. Consider streaming large files to avoid loading entire content into memory when decrypting/decompressing.
Also applies to: 363-419
451-579: New CSV export endpoint is well-shaped; minor validation hardening.You already validate parsed queries against Documents. Consider limiting queries param size earlier (e.g., ArrayList(Text(APP_LIMIT_ARRAY_ELEMENT_SIZE))) and normalizing duplicate columns.
src/Appwrite/Platform/Workers/Migrations.php (1)
225-235: Filename sanitizer — good.Basic replacement covers common forbidden characters. Optionally cap length (e.g., 128) to avoid filesystem/path issues.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (15)
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-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!**/*.lockdocs/references/migrations/migration-csv-export.mdis excluded by!docs/references/**docs/references/migrations/migration-csv-import.mdis excluded by!docs/references/**
📒 Files selected for processing (8)
app/config/locale/translations/en.json(1 hunks)app/controllers/api/migrations.php(7 hunks)app/init/resources.php(1 hunks)app/worker.php(1 hunks)docker-compose.yml(1 hunks)src/Appwrite/Platform/Workers/Migrations.php(16 hunks)src/Appwrite/Utopia/Response/Model/Migration.php(2 hunks)tests/e2e/Services/Migrations/MigrationsBase.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/Appwrite/Utopia/Response/Model/Migration.php (1)
src/Appwrite/Utopia/Response/Model.php (1)
addRule(90-102)
app/worker.php (1)
src/Appwrite/Utopia/Response/Model/Document.php (1)
Document(8-115)
tests/e2e/Services/Migrations/MigrationsBase.php (2)
tests/e2e/Client.php (1)
Client(8-342)tests/e2e/Scopes/Scope.php (2)
getHeaders(145-145)getLastEmail(45-60)
src/Appwrite/Platform/Workers/Migrations.php (4)
src/Appwrite/Event/Mail.php (7)
setSubject(34-39)setPreview(102-107)setBodyTemplate(148-153)setVariables(357-361)setName(125-130)setRecipient(57-62)src/Appwrite/Messaging/Adapter/Realtime.php (1)
Realtime(12-382)src/Appwrite/Template/Template.php (3)
Template(8-178)fromFile(25-33)render(66-85)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/XList.php (2)
action(77-174)getName(29-32)
app/controllers/api/migrations.php (3)
src/Appwrite/Platform/Workers/Migrations.php (1)
action(89-128)src/Appwrite/Utopia/Response.php (2)
Response(158-980)file(838-845)src/Appwrite/SDK/Method.php (1)
Method(9-314)
🪛 PHPMD (2.15.0)
src/Appwrite/Platform/Workers/Migrations.php
470-470: Avoid unused local variables such as '$resourceId'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (8)
docker-compose.yml (2)
77-77: Mount for /storage/uploads in appwrite service looks good.Ensures the HTTP API can serve/export CSVs from uploads.
700-705: Add uploads mount to migrations worker — good alignment with DestinationCSV.Worker can now write CSVs to uploads; matches deviceForFiles usage.
app/config/locale/translations/en.json (1)
60-67: CSV export email strings added correctly.Keys match worker usage (subject, preview, hello, body, footer, thanks, buttonText, signature).
app/init/resources.php (1)
558-561: Init resource rename aligns with worker.Factory still targets APP_STORAGE_IMPORTS/app-{projectId}. All good.
src/Appwrite/Utopia/Response/Model/Migration.php (1)
89-95: Add options field to Migration model — OK.JSON type, sane defaults, fits new export/import configs.
tests/e2e/Services/Migrations/MigrationsBase.php (1)
903-1197: Rename to testCreateCSVImport — OK.Covers CSV import validations and counters; matches renamed endpoint.
app/worker.php (1)
352-355: Verified: Resource rename and path are correct, no legacy references remain.All checks passed:
deviceForMigrationsresource properly defined at app/worker.php:352 and app/init/resources.php:558- No stale references to
deviceForImportsexist in the codebase- Storage path
APP_STORAGE_IMPORTScorrectly targets CSV import storage- Resource is consistently injected and used in Migrations worker and migrations API controller
src/Appwrite/Platform/Workers/Migrations.php (1)
144-149: Source/Destination wiring for Appwrite→CSV is correctly implemented.Verified: Lines 144-149 properly set
$dataSource = Appwrite::SOURCE_DATABASEwith parsed queries for Appwrite sources, and lines 210-221 correctly instantiateDestinationCSVwith$this->deviceForFilesas the first parameter. Device and mail queue injections are properly configured throughout the worker class.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/Appwrite/Platform/Workers/Migrations.php (1)
490-490: Remove unused variable.
$resourceIdis extracted but never used in this method.- $resourceId = $migration->getAttribute('resourceId');
🧹 Nitpick comments (5)
src/Appwrite/Utopia/Response/Model/Migration.php (1)
126-133: Nice fix! Past review concern addressed.The updated logic correctly preserves all error fields (except trace) and handles non-JSON errors appropriately. This resolves the previous issue where errors without a 'trace' field could be lost.
Consider adding a safety check for
json_encodefailure to prevent potential corruption:if (\is_array($decoded)) { if (isset($decoded['trace'])) { unset($decoded['trace']); } - $errors[$index] = \json_encode($decoded); + $sanitized = \json_encode($decoded); + if ($sanitized !== false) { + $errors[$index] = $sanitized; + } }Based on learnings.
src/Appwrite/Platform/Workers/Migrations.php (4)
225-235: Consider additional filename edge cases.The sanitization logic handles most problematic characters, but consider:
- Reserved names on Windows (CON, PRN, AUX, etc.)
- Maximum filename length (typically 255 bytes)
- Leading/trailing dots
Example enhancement:
protected function sanitizeFilename(string $filename): string { - // Replace problematic characters with underscores $sanitized = \preg_replace('/[:\/<>"|*?]/', '_', $filename); $sanitized = \preg_replace('/[^\x20-\x7E]/', '_', $sanitized); - $sanitized = \trim($sanitized); + $sanitized = \trim($sanitized, " \t\n\r\0\x0B."); + + // Truncate to 200 chars to leave room for extensions + if (\strlen($sanitized) > 200) { + $sanitized = \substr($sanitized, 0, 200); + } + return empty($sanitized) ? 'export' : $sanitized; }
505-526: Consider inheriting bucket permissions for files.The file is created with an empty
$permissionsarray. WhenfileSecurityis disabled on the bucket, inheriting the bucket's read permissions would provide more intuitive access control for CSV exports.+ $filePerms = $bucket->getAttribute('fileSecurity', false) + ? [] + : $bucket->getRead(); + $this->dbForProject->createDocument('bucket_' . $bucket->getSequence(), new Document([ '$id' => $fileId, - '$permissions' => [], + '$permissions' => $filePerms,Based on past review comments
540-541: Redundant fallback locale configuration.Both the default and fallback are set to the same value from
_APP_LOCALE.$locale = new Locale(System::getEnv('_APP_LOCALE', 'en')); - $locale->setFallback(System::getEnv('_APP_LOCALE', 'en'));
568-568: Template path is fragile.The relative path
__DIR__ . '/../../../../app/config/locale/templates/email-inner-base.tpl'will break if this file is moved. Consider using a constant or configuration value for template paths.Example:
- $message = Template::fromFile(__DIR__ . '/../../../../app/config/locale/templates/email-inner-base.tpl'); + $templatePath = System::getEnv('_APP_TEMPLATES_PATH', __DIR__ . '/../../../../app/config/locale/templates'); + $message = Template::fromFile($templatePath . '/email-inner-base.tpl');
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Appwrite/Platform/Workers/Migrations.php(16 hunks)src/Appwrite/Utopia/Response/Model/Migration.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/Appwrite/Utopia/Response/Model/Migration.php (1)
src/Appwrite/Utopia/Response/Model.php (1)
addRule(90-102)
src/Appwrite/Platform/Workers/Migrations.php (5)
src/Appwrite/Event/Event.php (2)
Event(10-654)setPayload(206-215)src/Appwrite/Event/Mail.php (7)
setSubject(34-39)setPreview(102-107)setBodyTemplate(148-153)setVariables(357-361)setName(125-130)setRecipient(57-62)src/Appwrite/Messaging/Adapter/Realtime.php (1)
Realtime(12-382)src/Appwrite/Template/Template.php (3)
Template(8-178)fromFile(25-33)render(66-85)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/XList.php (1)
getName(29-32)
🪛 PHPMD (2.15.0)
src/Appwrite/Platform/Workers/Migrations.php
490-490: Avoid unused local variables such as '$resourceId'. (undefined)
(UnusedLocalVariable)
⏰ 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 (Webhooks)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Databases/Legacy)
- 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 (7)
src/Appwrite/Utopia/Response/Model/Migration.php (1)
89-94: LGTM! Options field added correctly.The new
optionsfield follows the established pattern for rule definitions and appropriately uses the JSON type for flexible migration configuration data.src/Appwrite/Platform/Workers/Migrations.php (6)
6-6: LGTM: Imports support CSV export feature.The new imports (Mail, Template, ID, Query, Locale, DestinationCSV, Compression) are appropriate for the CSV export and email notification functionality.
Also applies to: 8-8, 18-20, 23-23, 26-26, 35-35
45-46: LGTM: Dependency injection updated for CSV export.The renaming from
deviceForImportstodeviceForMigrationsimproves clarity, and the addition ofdeviceForFilesandqueueForMailsproperly supports the CSV export and notification features.Also applies to: 80-82, 89-99, 101-102
134-148: LGTM: CSV export from Appwrite correctly configured.The logic properly detects CSV exports from Appwrite and configures the source to read directly from the database with query filters. Note that
Query::parseQueries()may throw, but this is handled by the outer try-catch inprocessMigration().
200-200: LGTM: DestinationCSV properly configured.The CSV destination is correctly instantiated with all necessary options (bucketId, filename, columns, delimiter, enclosure, escape, header) and uses
deviceForFilesfor storage.Also applies to: 210-220
246-277: Error sanitization improved; sensitive fields marked.The current implementation now preserves all errors (both JSON-decodable and plain strings) while stripping trace information, which addresses the main concern from the previous review. The addition of
sensitive: ['options', 'credentials']properly protects sensitive data in event payloads.
331-335: LGTM: Migration flow properly updated.The changes correctly:
- Add
- Call
shutdown()before error checks (ensures proper cleanup)- Consistently strip traces from errors across all code paths
- Conditionally invoke CSV export completion handler
Also applies to: 382-383, 388-446, 453-470, 477-479
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Appwrite/Platform/Workers/Migrations.php (2)
14-21: Guard options.queries and catch query-parse errors.Directly indexing $migrationOptions['queries'] can notice or throw. Add defaults and catch parse errors.
use Utopia\Database\Exception\Restricted; use Utopia\Database\Exception\Structure; +use Utopia\Database\Exception\Query as QueryException; @@ - if ($source === Appwrite::getName() && $destination === DestinationCSV::getName()) { + if ($source === SourceAppwrite::getName() && $destination === DestinationCSV::getName()) { $dataSource = Appwrite::SOURCE_DATABASE; $database = $this->dbForProject; - $queries = Query::parseQueries($migrationOptions['queries']); + try { + $queries = Query::parseQueries($migrationOptions['queries'] ?? []); + } catch (QueryException $e) { + throw new Exception('Invalid queries for CSV export: ' . $e->getMessage()); + } }Also applies to: 140-148
199-223: DestinationCSV: provide safe defaults for options to avoid notices.Undefined indices for bucketId/filename/columns/etc. can produce warnings. Use null-coalesce with sensible defaults.
- DestinationCSV::getName() => new DestinationCSV( - $this->deviceForFiles, - $migration->getAttribute('resourceId'), - $options['bucketId'], - $options['filename'], - $options['columns'], - $options['delimiter'], - $options['enclosure'], - $options['escape'], - $options['header'], - ), + DestinationCSV::getName() => new DestinationCSV( + $this->deviceForFiles, + $migration->getAttribute('resourceId'), + $options['bucketId'] ?? '', + $options['filename'] ?? 'export', + $options['columns'] ?? [], + $options['delimiter'] ?? ',', + $options['enclosure'] ?? '"', + $options['escape'] ?? '\\\\', + $options['header'] ?? true, + ),
🧹 Nitpick comments (3)
src/Appwrite/Platform/Workers/Migrations.php (3)
26-28: Unify Appwrite source import/alias and constant usage.Importing the same class twice (bare and aliased) is confusing. Keep a single alias and use it consistently for constants and getName().
-use Utopia\Migration\Sources\Appwrite; use Utopia\Migration\Sources\Appwrite as SourceAppwrite; @@ - $dataSource = Appwrite::SOURCE_API; + $dataSource = SourceAppwrite::SOURCE_API; @@ - if ($source === Appwrite::getName() && $destination === DestinationCSV::getName()) { + if ($source === SourceAppwrite::getName() && $destination === DestinationCSV::getName()) { $dataSource = Appwrite::SOURCE_DATABASE; $database = $this->dbForProject; - $queries = Query::parseQueries($migrationOptions['queries']); + $queries = Query::parseQueries($migrationOptions['queries']); } @@ - SourceAppwrite::getName() => new SourceAppwrite( + SourceAppwrite::getName() => new SourceAppwrite( $credentials['projectId'], $credentials['endpoint'] === 'http://localhost/v1' ? 'http://appwrite/v1' : $credentials['endpoint'], $credentials['apiKey'], - $dataSource, + $dataSource, $database, $queries, ),Note: also update the remaining Appwrite::SOURCE_DATABASE above to SourceAppwrite::SOURCE_DATABASE.
Also applies to: 140-148, 172-179
228-235: Filename sanitization: include backslash and trim trailing dot/space.Covers Windows-invalid chars and avoids “trailing dot/space” edge cases.
- $sanitized = \preg_replace('/[:\/<>"|*?]/', '_', $filename); + $sanitized = \preg_replace('/[\\\\\/:*?"<>|]/', '_', $filename); $sanitized = \preg_replace('/[^\x20-\x7E]/', '_', $sanitized); - $sanitized = \trim($sanitized); + $sanitized = \rtrim(\trim($sanitized), ". ");
392-406: DRY up error normalization.The error-flattening loop is duplicated. Extract to a private helper (e.g., normalizeErrors(array $errs): array) and reuse.
Also applies to: 432-446
📜 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 (1)
src/Appwrite/Platform/Workers/Migrations.php(16 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Appwrite/Platform/Workers/Migrations.php (5)
src/Appwrite/Event/Event.php (3)
Event(10-654)setEvent(117-122)setPayload(206-215)src/Appwrite/Event/Mail.php (7)
setSubject(34-39)setPreview(102-107)setBodyTemplate(148-153)setVariables(357-361)setName(125-130)setRecipient(57-62)src/Appwrite/Messaging/Adapter/Realtime.php (1)
Realtime(12-382)src/Appwrite/Template/Template.php (3)
Template(8-178)fromFile(25-33)render(66-85)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/XList.php (1)
action(77-174)
🪛 GitHub Actions: Linter
src/Appwrite/Platform/Workers/Migrations.php
[error] 1-1: PSR12: no_whitespace_in_blank_line violation detected by PHP-CS/PINT linter.
🪛 PHPMD (2.15.0)
src/Appwrite/Platform/Workers/Migrations.php
490-490: Avoid unused local variables such as '$resourceId'. (undefined)
(UnusedLocalVariable)
⏰ 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: Benchmark
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build SDK
- GitHub Check: scan
🔇 Additional comments (3)
src/Appwrite/Platform/Workers/Migrations.php (3)
246-262: Errors preservation looks good.Now keeps all errors and strips traces; payload marks options/credentials as sensitive. This resolves the earlier concern.
Also applies to: 269-277
539-542: LGTM: user null-check added.Prevents attribute access on null user during email notify.
1-2: Fix verified: PSR12 no_whitespace_in_blank_line issue resolved.The trailing whitespace violation has been successfully fixed. The linter confirms the file now passes PSR12 standards (marked with ✓ in pint output). No further action required.
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/Platform/Workers/Migrations.php (1)
1-1: Fix PSR-12 linting failure.The pipeline reports a
braces_positionstyle violation. Runvendor/bin/pint --writeto auto-fix the formatting issue.
🧹 Nitpick comments (1)
src/Appwrite/Platform/Workers/Migrations.php (1)
479-479: Consider inheriting bucket read permissions whenfileSecurityis disabled.Currently, the file document is created with an empty
$permissionsarray. WhenfileSecurityisfalse, the bucket's read permissions typically apply to all files. Inheriting them explicitly avoids surprises and aligns with standard bucket behavior.+ $filePerms = $bucket->getAttribute('fileSecurity', false) ? [] : $bucket->getRead(); $this->dbForProject->createDocument('bucket_' . $bucket->getSequence(), new Document([ '$id' => $fileId, - '$permissions' => [], + '$permissions' => $filePerms,
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Platform/Workers/Migrations.php(15 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Appwrite/Platform/Workers/Migrations.php (4)
src/Appwrite/Event/Event.php (1)
Event(10-654)src/Appwrite/Event/Mail.php (7)
setSubject(34-39)setPreview(102-107)setBodyTemplate(148-153)setVariables(357-361)setName(125-130)setRecipient(57-62)src/Appwrite/Template/Template.php (3)
Template(8-178)fromFile(25-33)render(66-85)src/Appwrite/Utopia/Response/Model/Migration.php (1)
Migration(9-140)
🪛 GitHub Actions: Linter
src/Appwrite/Platform/Workers/Migrations.php
[error] 1-1: PSR-12 linting failed. 1 style issue detected across 877 files. Pint reported: 'braces_position' (no_unused_...) in this file. Run 'vendor/bin/pint --write' to fix style issues.
🪛 PHPMD (2.15.0)
src/Appwrite/Platform/Workers/Migrations.php
463-463: Avoid unused local variables such as '$plan'. (undefined)
(UnusedLocalVariable)
⏰ 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 (Storage)
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: Unit Test
- GitHub Check: E2E General Test
- GitHub Check: Benchmark
- GitHub Check: Setup & Build SDK
- GitHub Check: scan
🔇 Additional comments (6)
src/Appwrite/Platform/Workers/Migrations.php (6)
462-475: Plan limit enforcement looks good.The file-size check against
$plan['fileSize']correctly enforces the organization's plan limits, deletes the oversized file, appends an error to the migration document, and throws. This addresses the concern about free-tier50MBlimits.
511-514: User null check properly implemented.The
isEmpty()guard prevents errors when the user is not found, logs a warning, and safely returns. This resolves the past critical issue.
519-526: JWT TTL now correct—token expires in 1 hour.Using
$maxAge = 60 * 60(seconds) as the TTL parameter fixes the past critical bug where an absolute epoch timestamp made tokens valid for decades.
581-588: Filename sanitization is adequate.The method strips filesystem-unsafe characters and falls back to
'export'when the result is empty. Handles path-traversal risks and control characters appropriately.
597-616: Error sanitization correctly preserves all errors.The method now iterates over every error, removes
tracefields when present, and preserves errors that lack traces or fail to decode. This fixes the past critical issue where errors without traces were silently dropped.
463-463: Static analysis false positive—$planis used.PHPMD incorrectly flags
$planas unused. Line 463 accesses$plan['fileSize']to enforce the file-size limit.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/Appwrite/Platform/Workers/Migrations.php (2)
445-453: Fix filename inconsistency and prevent double extension.Three separate issues with filename handling:
- Default missing extension: Line 445 defaults to
'export_' . time()without.csv- Double extension risk: Line 453 blindly appends
.csv, creatingexport.csv.csvif the user already included the extension- Name/path mismatch: Line 480 stores the original
$filenamewhile line 453 uses a different sanitized value with.csvApply this normalization at the start of the method:
- $filename = $options['filename'] ?? 'export_' . \time(); + $baseFilename = $options['filename'] ?? 'export'; + $sanitized = $this->sanitizeFilename($baseFilename); + // Ensure exactly one .csv extension + if (!\str_ends_with(\strtolower($sanitized), '.csv')) { + $sanitized .= '.csv'; + } + $filename = $sanitized; $userInternalId = $options['userInternalId'] ?? ''; @@ - $path = $this->deviceForFiles->getPath($bucketId . '/' . $this->sanitizeFilename($filename) . '.csv'); + $path = $this->deviceForFiles->getPath($bucketId . '/' . $filename);Now
$filenameis used consistently everywhere (lines 480, 494).
475-496: Inherit bucket permissions when fileSecurity is disabled.Line 477 creates the file with empty permissions. When
fileSecurityisfalse, the file should inherit the bucket's read permissions so users can access their exports.+ $filePerms = $bucket->getAttribute('fileSecurity', false) + ? [] + : $bucket->getRead(); + $this->dbForProject->createDocument('bucket_' . $bucket->getSequence(), new Document([ '$id' => $fileId, - '$permissions' => [], + '$permissions' => $filePerms, 'bucketId' => $bucket->getId(),
🧹 Nitpick comments (2)
src/Appwrite/Platform/Workers/Migrations.php (2)
579-586: Consider additional filename sanitization edge cases.The current implementation handles most problematic characters, but two optional improvements:
- Length limit: Filenames could exceed filesystem limits (typically 255 bytes)
- Reserved names: Windows reserved names (CON, PRN, AUX, NUL, COM1-9, LPT1-9) aren't handled
For the CSV export use case, the current implementation is functional. If you want to harden it:
protected function sanitizeFilename(string $filename): string { + // Strip any existing extension for normalization + $extension = ''; + if (\preg_match('/\.csv$/i', $filename)) { + $filename = \substr($filename, 0, -4); + } + // Replace problematic characters with underscores $sanitized = \preg_replace('/[:\/<>"|*?]/', '_', $filename); $sanitized = \preg_replace('/[^\x20-\x7E]/', '_', $sanitized); $sanitized = \trim($sanitized); - return empty($sanitized) ? 'export' : $sanitized; + + // Check for Windows reserved names + if (\preg_match('/^(CON|PRN|AUX|NUL|COM[1-9]|LPT[1-9])$/i', $sanitized)) { + $sanitized = '_' . $sanitized; + } + + // Limit to 200 characters (leaving room for bucket path + .csv) + if (\strlen($sanitized) > 200) { + $sanitized = \substr($sanitized, 0, 200); + } + + return empty($sanitized) ? 'export' : $sanitized; }
595-613: Error sanitization preserves all errors correctly.The implementation now keeps all errors (not just those with traces) and properly strips trace information. The logic is functionally correct.
Optional: The double serialization at line 601 could be optimized:
protected function sanitizeErrors( array $sourceErrors, array $destinationErrors, ): array { $errors = []; foreach ([...$sourceErrors, ...$destinationErrors] as $error) { - $encoded = \json_decode(\json_encode($error), true); + // If already a JSON string, decode directly; else serialize first + $encoded = \is_string($error) + ? \json_decode($error, true) + : \json_decode(\json_encode($error), true); + if (\is_array($encoded)) { if (isset($encoded['trace'])) { unset($encoded['trace']); } $errors[] = \json_encode($encoded); } else { - $errors[] = \json_encode($error); + $errors[] = \is_string($error) ? $error : \json_encode($error); } } return $errors; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Platform/Workers/Migrations.php(15 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Appwrite/Platform/Workers/Migrations.php (3)
src/Appwrite/Event/Mail.php (7)
setSubject(34-39)setPreview(102-107)setBodyTemplate(148-153)setVariables(357-361)setName(125-130)setRecipient(57-62)src/Appwrite/Messaging/Adapter/Realtime.php (1)
Realtime(12-382)src/Appwrite/Template/Template.php (3)
Template(8-178)fromFile(25-33)render(66-85)
🪛 PHPMD (2.15.0)
src/Appwrite/Platform/Workers/Migrations.php
461-461: Avoid unused local variables such as '$plan'. (undefined)
(UnusedLocalVariable)
⏰ 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 (Webhooks)
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (VCS)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: Unit Test
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: E2E General Test
- GitHub Check: Benchmark
- GitHub Check: Setup & Build SDK
- GitHub Check: scan
🔇 Additional comments (11)
src/Appwrite/Platform/Workers/Migrations.php (11)
5-37: LGTM! Imports align with CSV export functionality.The new imports (Mail, Template, DestinationCSV, Locale, Compression, ID, Query) support the CSV export feature and email notifications.
45-50: LGTM! Property updates align with refactoring.The rename from
deviceForImportstodeviceForMigrationsand the addition ofdeviceForFilesandplanproperties support the bidirectional migration flow.
70-85: LGTM! Constructor injection matches updated dependencies.The dependency injection correctly wires the renamed
deviceForMigrations, newdeviceForFiles,queueForMails, andplan.
90-128: LGTM! Action signature and initialization are correct.The expanded signature properly accepts the new dependencies, and the early return when
$eventsis not empty correctly filters event-triggered calls.
197-223: LGTM! Destination processing correctly handles CSV.The
DestinationCSVconstruction properly passes through all CSV formatting options from the migration document.
232-251: LGTM! Error sanitization and sensitive data handling improved.The refactored implementation correctly sanitizes errors and redacts sensitive fields ('options', 'credentials') from the realtime payload.
296-424: LGTM! Migration processing correctly handles CSV exports.The updated flow properly:
- Sanitizes errors using the helper method
- Uses null-safe operators for optional error callbacks
- Invokes CSV export completion handler in the success path
Note: Line 301 re-fetches the project document, likely to ensure up-to-date state before processing.
517-524: LGTM! JWT TTL is now correctly configured.The JWT now uses
$maxAge = 3600(1 hour in seconds) instead of an absolute timestamp, ensuring tokens expire as intended.
505-512: LGTM! User lookup includes null check.The code properly checks if the user document is empty and returns early with a warning, preventing null-pointer errors downstream.
461-473: Plan limit enforcement looks correct, but variable is incorrectly flagged.The file size check against
$plan['fileSize']is appropriate. The static analysis warning about unused$planat line 461 is a false positive—$planis clearly used in the comparison.
526-574: Email notification implementation is well-structured.The localization, template rendering, and mail queuing logic correctly:
- Generates a JWT-protected download URL
- Uses localized strings from the Locale system
- Builds the email body with proper variable interpolation
- Queues the notification email
# Conflicts: # app/config/specs/open-api3-1.8.x-client.json # app/config/specs/open-api3-1.8.x-console.json # app/config/specs/open-api3-1.8.x-server.json # app/config/specs/open-api3-latest-client.json # app/config/specs/open-api3-latest-console.json # app/config/specs/open-api3-latest-server.json # app/config/specs/swagger2-1.8.x-client.json # app/config/specs/swagger2-1.8.x-console.json # app/config/specs/swagger2-1.8.x-server.json # app/config/specs/swagger2-latest-client.json # app/config/specs/swagger2-latest-console.json # app/config/specs/swagger2-latest-server.json
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/Appwrite/Platform/Workers/Migrations.php (1)
445-453: Fix double.csvextension and filename/path inconsistency.Three related issues create filename problems:
- Line 453: If user provides
filename: "export.csv"in options, this createsexport.csv.csv- Line 445: Default includes
time()but no.csv, yet line 453 always appends.csv- Line 491: Stores original
$filenamewhile line 453 uses sanitized version with.csv— the stored name diverges from the actual pathNormalize the filename once at the start:
- $filename = $options['filename'] ?? 'export_' . \time(); + $baseFilename = $options['filename'] ?? 'export_' . \time(); + $sanitized = $this->sanitizeFilename($baseFilename); + // Ensure exactly one .csv extension + $filename = \str_ends_with(\strtolower($sanitized), '.csv') ? $sanitized : $sanitized . '.csv'; $userInternalId = $options['userInternalId'] ?? ''; @@ - $path = $this->deviceForFiles->getPath($bucketId . '/' . $this->sanitizeFilename($filename) . '.csv'); + $path = $this->deviceForFiles->getPath($bucketId . '/' . $filename);
🧹 Nitpick comments (1)
src/Appwrite/Platform/Workers/Migrations.php (1)
486-507: Inherit bucket read permissions whenfileSecurityis disabled.Line 488 sets empty permissions for the exported file. When
fileSecurityis false, the file should inherit the bucket's read permissions so users who can read from the bucket can access the export.$this->dbForProject->createDocument('bucket_' . $bucket->getSequence(), new Document([ '$id' => $fileId, - '$permissions' => [], + '$permissions' => $bucket->getAttribute('fileSecurity', false) ? [] : $bucket->getRead(), 'bucketId' => $bucket->getId(),
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (12)
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-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/**
📒 Files selected for processing (3)
app/config/locale/translations/en.json(1 hunks)docker-compose.yml(1 hunks)src/Appwrite/Platform/Workers/Migrations.php(15 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docker-compose.yml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-23T08:06:38.889Z
Learnt from: abnegate
PR: appwrite/appwrite#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)
src/Appwrite/Platform/Workers/Migrations.php (5)
src/Appwrite/Event/Mail.php (6)
setSubject(34-39)setPreview(102-107)setBodyTemplate(148-153)setName(125-130)setRecipient(57-62)src/Appwrite/Messaging/Adapter/Realtime.php (1)
Realtime(12-382)src/Appwrite/Extend/Exception.php (1)
Exception(7-464)src/Appwrite/Template/Template.php (3)
Template(8-178)fromFile(25-33)render(66-85)src/Appwrite/Utopia/Response/Model/Migration.php (2)
Migration(9-140)getName(103-106)
⏰ 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: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build SDK
- GitHub Check: scan
🔇 Additional comments (1)
app/config/locale/translations/en.json (1)
60-74: LGTM! Well-structured email translations.The new CSV export email translations for both success and failure cases are clear, professional, and follow the established pattern. Placeholders align with the worker implementation.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/Appwrite/Platform/Workers/Migrations.php (1)
478-499: Major: Inherit bucket read permissions when fileSecurity is disabled.Line 480 sets an empty permissions array. When
fileSecurityis false, the file should inherit the bucket's read permissions so users can access it without surprises.Apply this diff to inherit permissions properly:
+ $filePerms = $bucket->getAttribute('fileSecurity', false) ? [] : $bucket->getRead(); $this->dbForProject->createDocument('bucket_' . $bucket->getSequence(), new Document([ '$id' => $fileId, - '$permissions' => [], + '$permissions' => $filePerms, 'bucketId' => $bucket->getId(),
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Platform/Workers/Migrations.php(15 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-23T08:06:38.889Z
Learnt from: abnegate
PR: appwrite/appwrite#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)
src/Appwrite/Platform/Workers/Migrations.php (3)
src/Appwrite/Event/Event.php (2)
Event(10-654)setPayload(206-215)src/Appwrite/Event/Mail.php (7)
setSubject(34-39)setPreview(102-107)setBodyTemplate(148-153)setVariables(357-361)setName(125-130)setRecipient(57-62)src/Appwrite/Template/Template.php (3)
Template(8-178)fromFile(25-33)render(66-85)
🪛 PHPMD (2.15.0)
src/Appwrite/Platform/Workers/Migrations.php
474-474: Avoid unused local variables such as '$message'. (undefined)
(UnusedLocalVariable)
⏰ 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 (Migrations)
- GitHub Check: E2E Service Test (VCS)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: Unit Test
- GitHub Check: E2E General Test
- GitHub Check: Benchmark
- GitHub Check: scan
- GitHub Check: Setup & Build SDK
🔇 Additional comments (9)
src/Appwrite/Platform/Workers/Migrations.php (9)
1-51: LGTM! Clean dependency additions and property updates.The new imports (Mail, Template, Locale, DestinationCSV, Compression) and properties ($deviceForFiles, $plan) properly support the CSV export workflow, and the rename from
$deviceForImportsto$deviceForMigrationsaligns with the broader resource naming improvements.
90-128: LGTM! Dependency injection properly updated.The action method signature correctly adds the new dependencies (deviceForFiles, queueForMails, plan), and initialization is handled properly.
133-192: LGTM! CSV export source handling correctly implemented.The query parsing at Line 147 is safe because queries are validated at the API endpoint level before the migration is queued. The dataSource/database/queries flow for CSV exports is properly implemented.
Based on learnings.
194-223: LGTM! DestinationCSV properly configured.The CSV destination instantiation correctly passes all required options (bucketId, filename, columns, delimiter, enclosure, escape, header) and uses the appropriate device.
225-247: LGTM! Payload handling correctly marks credentials as sensitive.The sensitive parameter on Line 239 properly protects credentials from exposure, and the direct update approach is clean.
292-415: LGTM! Migration processing flow correctly handles CSV exports.The shutdown calls (Lines 343-344) are properly ordered, error sanitization is applied consistently (Lines 352, 378), and the CSV export completion handler (Lines 410-412) is correctly invoked only on successful CSV exports.
503-510: LGTM! JWT TTL correctly set to 1 hour.The JWT is now properly configured with a 3600-second (1 hour) TTL using
$maxAge, fixing the previous bug where an absolute epoch timestamp was passed.
540-618: LGTM! CSV export notification email properly implemented.The email notification flow correctly:
- Checks the
notifyoption (Lines 549-550)- Validates user existence with proper null handling (Lines 557-560) ✓ Past concern addressed
- Uses localization for internationalization (Lines 562-577)
- Builds and queues the email with appropriate variables (Lines 579-615)
620-660: LGTM! Helper methods correctly implement sanitization.Both helper methods are well-implemented:
sanitizeFilename(Lines 626-633): Properly replaces problematic characters and provides a safe defaultsanitizeErrors(Lines 642-660): Correctly removes sensitive trace information and ensures consistent JSON encoding
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Appwrite/Platform/Workers/Migrations.php (1)
477-479: Consider inheriting bucket permissions whenfileSecurityis disabled.Setting
$permissionsto an empty array may prevent users from accessing the exported CSV file. When the bucket hasfileSecurity=false, files typically inherit the bucket's read permissions to allow authorized bucket readers to access files.Apply this diff to inherit bucket permissions appropriately:
+ $filePerms = $bucket->getAttribute('fileSecurity', false) ? [] : $bucket->getRead(); $this->dbForProject->createDocument('bucket_' . $bucket->getSequence(), new Document([ '$id' => $fileId, - '$permissions' => [], + '$permissions' => $filePerms, 'bucketId' => $bucket->getId(),
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Platform/Workers/Migrations.php(15 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-23T08:06:38.889Z
Learnt from: abnegate
PR: appwrite/appwrite#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
⏰ 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: Benchmark
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build SDK
- GitHub Check: scan
🔇 Additional comments (7)
src/Appwrite/Platform/Workers/Migrations.php (7)
552-559: Good: User lookup now includes proper null/empty check.The
isEmpty()check at lines 556-559 correctly handles the case where the user is not found, preventing errors when accessing user attributes later in the method. This addresses the past concern about missing null checks.
641-659: Good: Error sanitization properly preserves all errors while removing traces.The
sanitizeErrors()method correctly handles the error sanitization by:
- Converting exception objects to arrays via JSON encode/decode
- Removing sensitive trace information
- Preserving all error data (code, message, etc.)
- Providing a fallback for edge cases
This addresses the past concern about losing errors during sanitization.
502-509: Good: JWT TTL is now correctly configured.The JWT generation now properly uses
$maxAge = 60 * 60(3600 seconds) for the TTL parameter, ensuring the token expires after exactly one hour as intended. This fixes the previous bug where an absolute epoch timestamp was incorrectly used as the TTL.
452-475: Good: Plan limit validation and error handling are properly implemented.The plan limit check correctly:
- Calculates file size and compares against the plan limit
- Deletes the oversized file from storage
- Records the error in the migration document using the proper JSON format
- Sends a failure notification email to the user
- Throws a clear exception
This addresses multiple past concerns about error format consistency, email notifications for failures, and plan limit enforcement.
232-247: Good: Removed redundant error sanitization.The
updateMigrationDocument()method now correctly avoids redundant error sanitization by lettingprocessMigration()handle it when errors are set. Thesensitive: ['credentials']parameter properly masks sensitive data in realtime payloads.
346-404: Good: Error handling flow is well-structured.The error handling in
processMigration()properly:
- Sanitizes errors from both source and destination using the new
sanitizeErrors()method- Logs only server-side errors (code 0 or >= 500) to avoid noise from expected client errors
- Uses null-safe operators to handle optional source/destination objects
- Includes comprehensive context in error logs
406-414: Good: CSV export completion handling is properly integrated.The CSV export completion flow correctly:
- Calls success handlers for both source and destination
- Conditionally triggers CSV-specific completion logic only when the destination is CSV
- Properly integrates with the existing migration completion flow
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