Conversation
📝 WalkthroughWalkthroughThis PR refactors the Migrations worker to remove direct platform configuration propagation and instead dynamically derive endpoint information at runtime. The changes remove the platform initialization call in the queue setup, eliminate the protected Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
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 |
commit: |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
✨ Benchmark results
⚡ Benchmark Comparison
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/Appwrite/Platform/Workers/Migrations.php (1)
211-217: Duplicate endpoint construction logic.This is the second instance of the duplicated endpoint construction code. The same validation and refactoring concerns apply here as mentioned in the comment for lines 147-151.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/controllers/shared/api.php(0 hunks)docker-compose.yml(1 hunks)src/Appwrite/Platform/Workers/Migrations.php(4 hunks)
💤 Files with no reviewable changes (1)
- app/controllers/shared/api.php
🧰 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-07-08T01:20:14.364Z
Learnt from: stnguyen90
Repo: appwrite/appwrite PR: 10119
File: app/controllers/api/account.php:1226-1232
Timestamp: 2025-07-08T01:20:14.364Z
Learning: In Appwrite, `_APP_DOMAIN` is a required environment variable that must always be set for the system to function properly.
Applied to files:
docker-compose.yml
⏰ 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). (15)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: E2E General Test
- GitHub Check: Unit Test
- GitHub Check: scan
🔇 Additional comments (3)
docker-compose.yml (1)
740-740: LGTM! Environment variable addition is appropriate.Adding
_APP_OPTIONS_FORCE_HTTPSto the migrations worker aligns with the refactoring to dynamically construct endpoints at runtime based on protocol requirements.src/Appwrite/Platform/Workers/Migrations.php (2)
181-188: LGTM! Endpoint now sourced from credentials.The change to use
credentials['endpoint']directly aligns with the refactoring to remove the stored platform configuration and derive endpoints dynamically at runtime.
147-151: The code at these three locations is correct and consistent.The
_APP_OPTIONS_FORCE_HTTPSenvironment variable is properly documented inapp/config/variables.phpwith default value'disabled'and valid values of'disabled'(use HTTP) and'enabled'(use HTTPS). All three referenced locations—lines 147–151, 211–217, and 324–329—correctly implement the identical protocol selection logic: defaulting to HTTPS except when explicitly set to'disabled'. When the environment variable is unset, it defaults to'disabled'per configuration, resulting in HTTP. This behavior is consistent across the codebase and properly documented.
| 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'; | ||
| } |
There was a problem hiding this comment.
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 (empty($credentials['endpoint'])) { | ||
| $platform = Config::getParam('platform', []); | ||
| $protocol = System::getEnv('_APP_OPTIONS_FORCE_HTTPS') === 'disabled' ? 'http' : 'https'; | ||
| $credentials['endpoint'] = $protocol . '://' . $platform['apiHostname'] . '/v1'; | ||
| } |
There was a problem hiding this comment.
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.
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