Add JSON import/export endpoints for migrations#11646
Add JSON import/export endpoints for migrations#11646premtsd-code wants to merge 14 commits into1.9.xfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds two ADMIN-only JSON migration API endpoints: Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Greptile SummaryThis PR adds The routing and request-handling logic is structurally sound, but the implementation is incomplete in two critical ways that will cause every call to the new endpoints to fail at runtime:
Until these gaps are filled the feature is non-functional end-to-end. The test fixtures and queue-trigger plumbing are otherwise well-structured. Confidence Score: 1/5Not safe to merge — both new endpoints will throw fatal errors at runtime due to missing class imports and unimplemented worker handlers. Three independent P0 blockers exist: (1)
Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| '$id' => $migrationId, | ||
| 'status' => 'pending', | ||
| 'stage' => 'init', | ||
| 'source' => JSON::getName(), |
There was a problem hiding this comment.
JSON class is not imported
Both JSON::getName() calls (here and at line 864) reference a class that is never imported in this file. The existing pattern for CSV is:
use Utopia\Migration\Sources\CSV;Without the corresponding use statements for the JSON source and destination classes, PHP will throw a fatal Class 'JSON' not found error whenever either endpoint is called.
The required imports likely look like:
use Utopia\Migration\Sources\JSON;
use Utopia\Migration\Destinations\JSON as DestinationJSON;(Verify the exact namespace against the utopia-php/migration package once it lands in the vendor directory.)
| $queueForEvents->setParam('migrationId', $migration->getId()); | ||
|
|
||
| $queueForMigrations | ||
| ->setMigration($migration) | ||
| ->setProject($project) |
There was a problem hiding this comment.
->setPlatform($platform) on the JSON import trigger
Every other migration endpoint (Appwrite, Firebase, Supabase, NHost, and CSV export) calls ->setPlatform($platform) before ->trigger(). The JSON import omits it. While the worker falls back to Config::getParam('platform', []), the missing call is inconsistent and may cause subtle differences in platform-specific behaviour.
| $queueForEvents->setParam('migrationId', $migration->getId()); | |
| $queueForMigrations | |
| ->setMigration($migration) | |
| ->setProject($project) | |
| $queueForMigrations | |
| ->setMigration($migration) | |
| ->setProject($project) | |
| ->setPlatform($platform) | |
| ->trigger(); |
✨ Benchmark results
⚡ Benchmark Comparison
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/api/migrations.php`:
- Around line 741-757: The migration documents are being created with
source/destination set to JSON via JSON::getName() and export options
(options.path/options.size) are stored, but
src/Appwrite/Platform/Workers/Migrations.php still only recognizes
Appwrite/Firebase/Supabase/NHost/CSV and only handles CSV export completion;
update the worker to accept the JSON source and JSON destination (add
JSON::getName() to the source/destination match/switches in the Migrations
worker), add a JSON-aware export completion path that consumes the options.path
and options.size from the migration document and routes them into the existing
file-finalization/notification logic (so exports do not fail with "Invalid
source/destination" and export options reach finalization), and ensure errors
for unknown types are only raised after these JSON branches are covered.
- Around line 745-746: The code references JSON::getName() which doesn't
exist/wasn't imported and will cause a fatal error; replace JSON::getName() with
the correct migration source (e.g., CSV::getName(), Firebase::getName(),
NHost::getName(), or Supabase::getName() depending on the intended source) and
add the corresponding import from Utopia\Migration\Sources (for example import
Utopia\Migration\Sources\CSV) or, if JSON was intended, add/implement the JSON
source class and import it; update both occurrences of JSON::getName() and
ensure the matching class name is used consistently alongside
Appwrite::getName().
In `@src/Appwrite/Platform/Workers/Migrations.php`:
- Line 792: The code reads an undefined property $this->dataExportType when
calling sendCSVEmail; fix by changing the call site to pass the export type
explicitly (e.g., CSV::getName() as the default) into sendCSVEmail so the
template 'type' receives a value. Locate the sendCSVEmail invocation that
currently builds the payload with 'type' => $this->dataExportType and replace it
to pass a concrete string or CSV::getName() as the third/appropriate argument
(or add an explicit $type parameter to the method call), ensuring sendCSVEmail
and any callers accept/forward this type value instead of relying on the
non-existent dataExportType property.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 70ed7ea5-3c21-4f04-be78-719196cdc260
⛔ Files ignored due to path filters (1)
docs/references/health/get-queue-audits.mdis excluded by!docs/references/**
📒 Files selected for processing (7)
app/controllers/api/migrations.phpsrc/Appwrite/Platform/Workers/Migrations.phptests/e2e/Services/Migrations/MigrationsBase.phptests/resources/json/documents-internals.jsontests/resources/json/documents.jsontests/resources/json/irrelevant-column.jsontests/resources/json/missing-column.json
| 'source' => JSON::getName(), | ||
| 'destination' => Appwrite::getName(), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "JSON::getName() call sites:"
rg -n '\bJSON::getName\s*\(' app/controllers/api/migrations.php
echo
echo "JSON imports in this file:"
rg -n '^\s*use\s+.+\\JSON\s*;' app/controllers/api/migrations.php || trueRepository: appwrite/appwrite
Length of output: 211
🏁 Script executed:
# Check the imports section at the top of the file
head -50 app/controllers/api/migrations.phpRepository: appwrite/appwrite
Length of output: 1719
🏁 Script executed:
# Check context around line 745-746
sed -n '740,750p' app/controllers/api/migrations.phpRepository: appwrite/appwrite
Length of output: 506
🏁 Script executed:
# Check context around line 863-864
sed -n '858,868p' app/controllers/api/migrations.phpRepository: appwrite/appwrite
Length of output: 550
🏁 Script executed:
# Search for JSON class definition in the codebase
rg -l 'class JSON' --type phpRepository: appwrite/appwrite
Length of output: 43
🏁 Script executed:
# Search for JSON migration source class
rg -l 'Utopia.Migration.Sources.JSON' --type phpRepository: appwrite/appwrite
Length of output: 43
🏁 Script executed:
# Look for JSON source class definition in Utopia
rg 'class JSON' --type php -A 3Repository: appwrite/appwrite
Length of output: 43
🏁 Script executed:
# Search for references to JSON class in migration-related contexts
rg '\bJSON::\w+' --type php | head -20Repository: appwrite/appwrite
Length of output: 517
🏁 Script executed:
# Check vendor directory for Utopia\Migration\Sources
find . -path '*/Utopia/Migration/Sources*' -name '*.php' 2>/dev/null | head -20Repository: appwrite/appwrite
Length of output: 43
🏁 Script executed:
# Find all available migration sources
find . -path '*/Migration/Sources/*.php' -type f 2>/dev/nullRepository: appwrite/appwrite
Length of output: 43
🏁 Script executed:
# Check git history or comments for JSON migration source
rg -B5 -A5 'source.*JSON::getName' app/controllers/api/migrations.phpRepository: appwrite/appwrite
Length of output: 506
🏁 Script executed:
# Look for any mention of CSV or other sources to understand the pattern
rg 'CSV::getName|Firebase::getName|Supabase::getName' --type phpRepository: appwrite/appwrite
Length of output: 1122
Resolve JSON::getName() before calling it.
These lines reference JSON::getName() without importing a JSON class. There is no JSON migration source class in the Utopia\Migration\Sources namespace—the available sources are Appwrite, CSV, Firebase, NHost, and Supabase. This will cause a fatal error at runtime. Add the correct migration source class import and replace JSON::getName() with the appropriate source.
Affected lines: 745–746, 863–864.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/api/migrations.php` around lines 745 - 746, The code
references JSON::getName() which doesn't exist/wasn't imported and will cause a
fatal error; replace JSON::getName() with the correct migration source (e.g.,
CSV::getName(), Firebase::getName(), NHost::getName(), or Supabase::getName()
depending on the intended source) and add the corresponding import from
Utopia\Migration\Sources (for example import Utopia\Migration\Sources\CSV) or,
if JSON was intended, add/implement the JSON source class and import it; update
both occurrences of JSON::getName() and ensure the matching class name is used
consistently alongside Appwrite::getName().
|
Found 1 test error and 2 test failures on Blacksmith runners: Error
Failures
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app/controllers/api/migrations.php (2)
742-758:⚠️ Potential issue | 🔴 CriticalWorker cannot process JSON imports — will fail with "Invalid source type".
The migration document sets
sourcetoJSON::getName(), butsrc/Appwrite/Platform/Workers/Migrations.php::processSource()(around line 215) only handlesFirebase,Supabase,NHost,Appwrite, andCSVsources. This migration will fail immediately when the worker processes it.The worker's
processSourcemethod needs aJSON::getName()case that instantiates the JSON source class.#!/bin/bash # Check processSource match statement in worker rg -A 50 "protected function processSource" src/Appwrite/Platform/Workers/Migrations.php | head -70🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/api/migrations.php` around lines 742 - 758, The migration sets source to JSON::getName(), but the worker's processSource() (in Migrations.php) lacks a case for JSON::getName(), causing "Invalid source type"; update processSource() to add a branch for JSON::getName() that instantiates and returns the JSON source class (the same class referenced by JSON::getName()) so the worker can process JSON imports, mirroring how existing cases (Firebase, Supabase, NHost, Appwrite, CSV) construct their source handler.
860-880:⚠️ Potential issue | 🔴 CriticalWorker cannot process JSON exports — missing destination handler and export completion logic.
Two issues prevent JSON exports from working:
processDestination()in the worker (around line 271) only matchesAppwriteandCSVdestinations —JSON::getName()will throw "Invalid destination type".Export completion handling (line 553 in worker) only triggers for
DestinationCSV::getName():if ($migration->getAttribute('destination') === DestinationCSV::getName()) { $this->handleCSVExportComplete(...); }JSON exports won't trigger file finalization or email notifications.
The worker needs:
- A
DestinationJSONcase inprocessDestination()- Either extend
handleCSVExportCompleteor addhandleJSONExportCompletefor JSON destination#!/bin/bash # Check processDestination and export completion handling in worker echo "=== processDestination match ===" rg -A 15 "protected function processDestination" src/Appwrite/Platform/Workers/Migrations.php echo -e "\n=== Export completion handling ===" rg -B2 -A5 "handleCSVExportComplete" src/Appwrite/Platform/Workers/Migrations.php🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/api/migrations.php` around lines 860 - 880, processDestination() currently only handles Appwrite and CSV destinations and will throw for JSON exports; add a case for DestinationJSON::getName() in processDestination() that mirrors the CSV flow (serialize resources to JSON into the internal bucket and return the same structure used for CSV). Also ensure export completion logic that currently calls handleCSVExportComplete(...) when $migration->getAttribute('destination') === DestinationCSV::getName() is extended to handle DestinationJSON::getName()—either by reusing/abstracting the CSV finalization into a common method or by adding a new handleJSONExportComplete(...) that performs file finalization, updates migration status, and sends notifications using the same patterns as handleCSVExportComplete; update references to DestinationJSON::getName(), processDestination(), handleCSVExportComplete(), and optionally handleJSONExportComplete() so JSON exports complete and notify correctly.
🧹 Nitpick comments (1)
app/controllers/api/migrations.php (1)
762-765: MissingsetPlatform($platform)for consistency with export endpoints.Both CSV and JSON export endpoints call
setPlatform($platform)on the queue, but this JSON import endpoint does not. While imports may not trigger email notifications, including it maintains consistency and future-proofs the code.♻️ Suggested change
$queueForMigrations ->setMigration($migration) ->setProject($project) + ->setPlatform($platform) ->trigger();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/api/migrations.php` around lines 762 - 765, Add a call to setPlatform($platform) on the same queue instance to match the CSV/JSON export endpoints: update the chain where $queueForMigrations->setMigration($migration)->setProject($project)->trigger() is invoked so it also calls ->setPlatform($platform) before ->trigger(); this keeps behavior consistent and future-proofs imports for any platform-dependent logic tied to setPlatform.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Appwrite/Platform/Workers/Migrations.php`:
- Line 792: The variable $migration is undefined inside sendCSVEmail; update the
sendCSVEmail method signature to accept a new parameter (e.g., $type) and use
that instead of referencing $migration inside the method, then update all call
sites in handleCSVExportComplete to pass $migration->getAttribute('destination',
'CSV') into sendCSVEmail; ensure references in sendCSVEmail that previously used
$migration->getAttribute('destination', 'CSV') are replaced with the new $type
parameter.
---
Duplicate comments:
In `@app/controllers/api/migrations.php`:
- Around line 742-758: The migration sets source to JSON::getName(), but the
worker's processSource() (in Migrations.php) lacks a case for JSON::getName(),
causing "Invalid source type"; update processSource() to add a branch for
JSON::getName() that instantiates and returns the JSON source class (the same
class referenced by JSON::getName()) so the worker can process JSON imports,
mirroring how existing cases (Firebase, Supabase, NHost, Appwrite, CSV)
construct their source handler.
- Around line 860-880: processDestination() currently only handles Appwrite and
CSV destinations and will throw for JSON exports; add a case for
DestinationJSON::getName() in processDestination() that mirrors the CSV flow
(serialize resources to JSON into the internal bucket and return the same
structure used for CSV). Also ensure export completion logic that currently
calls handleCSVExportComplete(...) when $migration->getAttribute('destination')
=== DestinationCSV::getName() is extended to handle
DestinationJSON::getName()—either by reusing/abstracting the CSV finalization
into a common method or by adding a new handleJSONExportComplete(...) that
performs file finalization, updates migration status, and sends notifications
using the same patterns as handleCSVExportComplete; update references to
DestinationJSON::getName(), processDestination(), handleCSVExportComplete(), and
optionally handleJSONExportComplete() so JSON exports complete and notify
correctly.
---
Nitpick comments:
In `@app/controllers/api/migrations.php`:
- Around line 762-765: Add a call to setPlatform($platform) on the same queue
instance to match the CSV/JSON export endpoints: update the chain where
$queueForMigrations->setMigration($migration)->setProject($project)->trigger()
is invoked so it also calls ->setPlatform($platform) before ->trigger(); this
keeps behavior consistent and future-proofs imports for any platform-dependent
logic tied to setPlatform.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1a214281-7691-469c-be3a-336e65a02b4d
📒 Files selected for processing (3)
app/controllers/api/migrations.phpcomposer.jsonsrc/Appwrite/Platform/Workers/Migrations.php
✅ Files skipped from review due to trivial changes (1)
- composer.json
…destination to worker
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Appwrite/Platform/Workers/Migrations.php (1)
807-807:⚠️ Potential issue | 🔴 CriticalUndefined variable in email variables payload (runtime error).
Line 807 reads
$migrationinsidesendCSVEmail(), but that variable is not in scope in this method, which will fail when email notification is sent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Appwrite/Platform/Workers/Migrations.php` at line 807, sendCSVEmail uses an undefined $migration variable (line with 'type' => $migration->getAttribute('destination', 'CSV')), which will cause a runtime error; fix by using the correct in-scope migration object or value (e.g., $this->migration or the migration variable passed into the caller) or by adding a migration parameter to sendCSVEmail and wiring it from the caller, then call getAttribute('destination','CSV') on that valid object (and add a null check/default if needed) so the 'type' entry uses an existing variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Appwrite/Platform/Workers/Migrations.php`:
- Around line 299-305: The JSON destination was added but the export completion
flow only checks for CSV, so DestinationJSON exports never run the post-export
completion (download-link/email) logic; update the completion gating in the
worker (the block that currently checks DestinationCSV at the export completion
step) to also recognize DestinationJSON (or better, check a common
interface/base like DestinationInterface or a trait such as isDownloadable())
and invoke the same completion steps using the migration options (notify,
filename, bucketId, columns) so JSON exports trigger the download-link/email
pipeline just like CSV exports.
---
Duplicate comments:
In `@src/Appwrite/Platform/Workers/Migrations.php`:
- Line 807: sendCSVEmail uses an undefined $migration variable (line with 'type'
=> $migration->getAttribute('destination', 'CSV')), which will cause a runtime
error; fix by using the correct in-scope migration object or value (e.g.,
$this->migration or the migration variable passed into the caller) or by adding
a migration parameter to sendCSVEmail and wiring it from the caller, then call
getAttribute('destination','CSV') on that valid object (and add a null
check/default if needed) so the 'type' entry uses an existing variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 19a63d7e-344f-465e-bd63-0c5d8f593dec
📒 Files selected for processing (2)
app/controllers/api/migrations.phpsrc/Appwrite/Platform/Workers/Migrations.php
✅ Files skipped from review due to trivial changes (1)
- app/controllers/api/migrations.php
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Appwrite/Platform/Workers/Migrations.php (1)
652-660:⚠️ Potential issue | 🔴 CriticalFix undefined
$migrationinsendExportEmail()(runtime error).Line 809 reads
$migrationinsidesendExportEmail(), but that variable is not in scope in this method. This will fail when building email variables.Suggested fix
protected function sendExportEmail( bool $success, Document $project, Document $user, array $options, Mail $queueForMails, array $platform, + string $type = 'CSV', string $downloadUrl = '', float $sizeMB = 0.0, ): void { @@ - 'type' => $migration->getAttribute('destination', 'CSV'), + 'type' => $type, ];$this->sendExportEmail( success: false, project: $project, user: $user, options: $options, queueForMails: $queueForMails, platform: $platform, + type: $migration->getAttribute('destination', 'CSV'), sizeMB: $sizeMB );$this->sendExportEmail( success: true, project: $project, user: $user, options: $options, queueForMails: $queueForMails, platform: $platform, + type: $migration->getAttribute('destination', 'CSV'), downloadUrl: $downloadUrl );#!/bin/bash set -euo pipefail FILE="src/Appwrite/Platform/Workers/Migrations.php" echo "Method signature and in-method type assignment:" rg -n -C3 "function sendExportEmail|['\"]type['\"]\s*=>" "$FILE" echo echo "Call sites:" rg -n -C2 "sendExportEmail\(" "$FILE"Also applies to: 714-722, 739-748, 809-809
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Appwrite/Platform/Workers/Migrations.php` around lines 652 - 660, The sendExportEmail method references $migration which is not in scope; fix by adding an optional parameter to sendExportEmail (e.g., ?array $migration = null) and updating its implementation to handle a null $migration when building email variables, then update every sendExportEmail(...) call site (calls in this file) to pass the local $migration where it exists or pass null otherwise so the method never reads an undefined variable; reference the sendExportEmail function and the $migration usages inside it when making the changes.
🧹 Nitpick comments (1)
src/Appwrite/Platform/Workers/Migrations.php (1)
739-773: Update export-email docs/text to reflect generic export, not CSV-only.
sendExportEmail()now serves CSV and JSON, but doc/comments and localization key namespace remain CSV-specific, which is misleading for maintenance and future template evolution.
As per coding guidelines, "Include comprehensive PHPDoc comments".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Appwrite/Platform/Workers/Migrations.php` around lines 739 - 773, sendExportEmail currently uses CSV-specific docs and localization keys (emails.csvExport.*) while handling CSV and JSON exports; update the PHPDoc for sendExportEmail to describe generic export notifications (CSV/JSON/other), and replace all Locale key lookups ("emails.csvExport.{$emailType}.*") with a generic namespace such as "emails.export.{$emailType}.*" (and adjust the $buttonText logic accordingly), making sure to also update any related template identifiers and variable names that reference CSV to use export; preserve backward-compatibility by falling back to the old keys if the new keys are missing or update templates and localization files to include the new keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Appwrite/Platform/Workers/Migrations.php`:
- Around line 568-570: The post-processing call to handleDataExportComplete is
currently executed for any export destination even when the migration failed;
update the condition that checks $migration->getAttribute('destination') (the
block using DestinationCSV::getName() and DestinationJSON::getName()) to also
verify the migration status is completed (e.g.
$migration->getAttribute('status') === MigrationStatus::STATUS_COMPLETED or
equivalent constant/method on the Migration object) before invoking
handleDataExportComplete($project, $migration, $queueForMails,
$queueForRealtime, $platform, $authorization), so export file/path operations
and notifications only run for successful migrations.
---
Duplicate comments:
In `@src/Appwrite/Platform/Workers/Migrations.php`:
- Around line 652-660: The sendExportEmail method references $migration which is
not in scope; fix by adding an optional parameter to sendExportEmail (e.g.,
?array $migration = null) and updating its implementation to handle a null
$migration when building email variables, then update every sendExportEmail(...)
call site (calls in this file) to pass the local $migration where it exists or
pass null otherwise so the method never reads an undefined variable; reference
the sendExportEmail function and the $migration usages inside it when making the
changes.
---
Nitpick comments:
In `@src/Appwrite/Platform/Workers/Migrations.php`:
- Around line 739-773: sendExportEmail currently uses CSV-specific docs and
localization keys (emails.csvExport.*) while handling CSV and JSON exports;
update the PHPDoc for sendExportEmail to describe generic export notifications
(CSV/JSON/other), and replace all Locale key lookups
("emails.csvExport.{$emailType}.*") with a generic namespace such as
"emails.export.{$emailType}.*" (and adjust the $buttonText logic accordingly),
making sure to also update any related template identifiers and variable names
that reference CSV to use export; preserve backward-compatibility by falling
back to the old keys if the new keys are missing or update templates and
localization files to include the new keys.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b4f2c0ca-a4c3-4fad-8c86-d341b3fad7ae
📒 Files selected for processing (2)
src/Appwrite/Platform/Workers/Migrations.phptests/e2e/Services/Migrations/MigrationsBase.php
| $destination_type = $migration->getAttribute('destination'); | ||
| if ($destination_type === DestinationCSV::getName() || $destination_type === DestinationJSON::getName()) { | ||
| $this->handleDataExportComplete($project, $migration, $queueForMails, $queueForRealtime, $platform, $authorization); |
There was a problem hiding this comment.
Gate export post-processing to completed migrations only.
handleDataExportComplete() is invoked even when migration status is failed. This can trigger file/path operations and notifications in error paths.
Suggested fix
- $destination_type = $migration->getAttribute('destination');
- if ($destination_type === DestinationCSV::getName() || $destination_type === DestinationJSON::getName()) {
- $this->handleDataExportComplete($project, $migration, $queueForMails, $queueForRealtime, $platform, $authorization);
- }
+ if ($migration->getAttribute('status', '') === 'completed') {
+ $destinationType = $migration->getAttribute('destination');
+ if (\in_array($destinationType, [DestinationCSV::getName(), DestinationJSON::getName()], true)) {
+ $this->handleDataExportComplete($project, $migration, $queueForMails, $queueForRealtime, $platform, $authorization);
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Appwrite/Platform/Workers/Migrations.php` around lines 568 - 570, The
post-processing call to handleDataExportComplete is currently executed for any
export destination even when the migration failed; update the condition that
checks $migration->getAttribute('destination') (the block using
DestinationCSV::getName() and DestinationJSON::getName()) to also verify the
migration status is completed (e.g. $migration->getAttribute('status') ===
MigrationStatus::STATUS_COMPLETED or equivalent constant/method on the Migration
object) before invoking handleDataExportComplete($project, $migration,
$queueForMails, $queueForRealtime, $platform, $authorization), so export
file/path operations and notifications only run for successful migrations.
2 participants
Summary
POST /v1/migrations/json/imports— import documents from a JSON filePOST /v1/migrations/json/exports— export documents to a JSON fileCherry-picked from
import-export-jsonbranch (1.8.x) and adapted for 1.9.x:App::toHttp::routingsetProjectcall in import endpointDepends on: utopia-php/migration PR #141 (already merged)
Test plan