Skip to content

Revert backups endpoints #10958

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
loks0n merged 2 commits into from
Dec 15, 2025
Merged

Revert backups endpoints #10958

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4b58e1a
revert: backups endpoints
loks0n Dec 15, 2025
e931938
revert: backups endpoints
loks0n Dec 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion app/controllers/shared/api.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -628,7 +628,6 @@
$queueForFunctions->setPlatform($platform);
$queueForBuilds->setPlatform($platform);
$queueForMails->setPlatform($platform);
$queueForMigrations->setPlatform($platform);

// Clone the queues, to prevent events triggered by the database listener
// from overwriting the events that are supposed to be triggered in the shutdown hook.
Expand Down
1 change: 1 addition & 0 deletions docker-compose.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -737,6 +737,7 @@ services:
- _APP_MIGRATIONS_FIREBASE_CLIENT_ID
- _APP_MIGRATIONS_FIREBASE_CLIENT_SECRET
- _APP_DATABASE_SHARED_TABLES
- _APP_OPTIONS_FORCE_HTTPS

appwrite-task-maintenance:
entrypoint: maintenance
Expand Down
26 changes: 19 additions & 7 deletions src/Appwrite/Platform/Workers/Migrations.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,6 @@ class Migrations extends Action

protected array $plan;

protected array $platform;

/**
* @var array<string, int>
*/
Expand Down Expand Up @@ -108,7 +106,6 @@ public function action(
$this->deviceForMigrations = $deviceForMigrations;
$this->deviceForFiles = $deviceForFiles;
$this->plan = $plan;
$this->platform = $payload['platform'] ?? [];

if (empty($payload)) {
throw new Exception('Missing payload');
Expand Down Expand Up @@ -144,10 +141,15 @@ protected function processSource(Document $migration): Source
$credentials = $migration->getAttribute('credentials');
$migrationOptions = $migration->getAttribute('options');
$dataSource = Appwrite::SOURCE_API;
$endpoint = $this->platform['endpoint'] ?: ($credentials['endpoint'] ?? 'http://appwrite.test/v1');
$database = null;
$queries = [];

if ($credentials['endpoint'] === 'http://localhost/v1') {
$platform = Config::getParam('platform', []);
$protocol = System::getEnv('_APP_OPTIONS_FORCE_HTTPS') === 'disabled' ? 'http' : 'https';
$credentials['endpoint'] = $protocol . '://' . $platform['apiHostname'] . '/v1';
}
Comment on lines +147 to +151
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add validation for platform configuration and extract duplicated endpoint construction logic.

This endpoint construction logic is duplicated in three locations (lines 147-151, 211-217, and 325-329). Additionally, $platform['apiHostname'] is accessed without verifying the key exists, which could cause undefined array key errors if the platform configuration is malformed.

Consider extracting this into a helper method:

+    protected function buildEndpoint(): string
+    {
+        $platform = Config::getParam('platform', []);
+        if (!isset($platform['apiHostname'])) {
+            throw new Exception('Platform API hostname not configured');
+        }
+        $protocol = System::getEnv('_APP_OPTIONS_FORCE_HTTPS') === 'disabled' ? 'http' : 'https';
+        return $protocol . '://' . $platform['apiHostname'] . '/v1';
+    }

Then use it consistently:

 if ($credentials['endpoint'] === 'http://localhost/v1') {
-    $platform = Config::getParam('platform', []);
-    $protocol = System::getEnv('_APP_OPTIONS_FORCE_HTTPS') === 'disabled' ? 'http' : 'https';
-    $credentials['endpoint'] = $protocol . '://' . $platform['apiHostname'] . '/v1';
+    $credentials['endpoint'] = $this->buildEndpoint();
 }

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In src/Appwrite/Platform/Workers/Migrations.php around lines 147-151 (also
duplicated at 211-217 and 325-329): the endpoint construction is duplicated and
accesses $platform['apiHostname'] without validating that key exists; extract
the logic into a private helper (for example buildPlatformEndpoint()) that reads
Config::getParam('platform', []), validates the presence of apiHostname
(throwing or returning a safe default/error if missing), determines protocol via
System::getEnv('_APP_OPTIONS_FORCE_HTTPS') === 'disabled' ? 'http' : 'https',
and returns the full endpoint string; replace the three inline blocks with calls
to this helper and add a clear error handling path when apiHostname is absent to
avoid undefined array key errors.


if ($source === Appwrite::getName() && $destination === DestinationCSV::getName()) {
$dataSource = Appwrite::SOURCE_DATABASE;
$database = $this->dbForProject;
Expand Down Expand Up @@ -178,7 +180,7 @@ protected function processSource(Document $migration): Source
),
SourceAppwrite::getName() => new SourceAppwrite(
$credentials['projectId'],
$endpoint,
$credentials['endpoint'],
$credentials['apiKey'],
$dataSource,
$database,
Expand Down Expand Up @@ -206,10 +208,13 @@ protected function processDestination(Document $migration, string $apiKey): Dest
$destination = $migration->getAttribute('destination');
$options = $migration->getAttribute('options', []);

$protocol = System::getEnv('_APP_OPTIONS_FORCE_HTTPS') === 'disabled' ? 'http' : 'https';
$platform = Config::getParam('platform', []);

return match ($destination) {
DestinationAppwrite::getName() => new DestinationAppwrite(
$this->project->getId(),
$this->platform['endpoint'],
$protocol . '://' . $platform['apiHostname'] . '/v1',
$apiKey,
$this->dbForProject,
Config::getParam('collections', [])['databases']['collections'],
Expand Down Expand Up @@ -306,15 +311,22 @@ protected function processMigration(

$transfer = $source = $destination = null;



try {
if (
$migration->getAttribute('source') === SourceAppwrite::getName() &&
empty($migration->getAttribute('credentials', []))
) {
$credentials = $migration->getAttribute('credentials', []);
$credentials['projectId'] = $credentials['projectId'] ?? $project->getId();
$credentials['endpoint'] = $credentials['endpoint'] ?? $this->platform['endpoint'];
$credentials['apiKey'] = $credentials['apiKey'] ?? $tempAPIKey;

if (empty($credentials['endpoint'])) {
$platform = Config::getParam('platform', []);
$protocol = System::getEnv('_APP_OPTIONS_FORCE_HTTPS') === 'disabled' ? 'http' : 'https';
$credentials['endpoint'] = $protocol . '://' . $platform['apiHostname'] . '/v1';
}
Comment on lines +324 to +329
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Inconsistent endpoint handling logic.

This is the third instance of duplicated endpoint construction, but the conditional logic differs from processSource. Here you check empty($credentials['endpoint']), while in processSource (lines 147-151) you check for exact string match 'http://localhost/v1'. This inconsistency could lead to different behavior depending on which method processes the endpoint first.

After extracting the endpoint construction to a helper method (as suggested for lines 147-151), ensure consistent logic across both locations:

 if (empty($credentials['endpoint'])) {
-    $platform = Config::getParam('platform', []);
-    $protocol = System::getEnv('_APP_OPTIONS_FORCE_HTTPS') === 'disabled' ? 'http' : 'https';
-    $credentials['endpoint'] = $protocol . '://' . $platform['apiHostname'] . '/v1';
+    $credentials['endpoint'] = $this->buildEndpoint();
 }

Consider whether both the === 'http://localhost/v1' check and the empty() check are needed, or if they should be consolidated into a single standardized approach.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In src/Appwrite/Platform/Workers/Migrations.php around lines 324-329, the
endpoint construction is duplicated and uses a different condition
(empty($credentials['endpoint'])) than the one in processSource (which checks
=== 'http://localhost/v1'); extract the endpoint-building logic into a single
helper (e.g., buildPlatformEndpoint or normalizeEndpoint) and replace both
locations to call it; the helper should standardize the behavior by treating
either a missing/empty endpoint OR the legacy default 'http://localhost/v1' as a
trigger to construct the endpoint from Config::getParam('platform') combined
with the protocol derived from System::getEnv('_APP_OPTIONS_FORCE_HTTPS'), and
return the normalized endpoint string so both call sites behave identically.

$migration->setAttribute('credentials', $credentials);
}

Expand Down
Loading