Conversation
📝 WalkthroughWalkthroughThe migration worker refactors endpoint normalization logic within a single PHP file. Runtime endpoint normalization that previously mapped localhost/v1 to the platform hostname with protocol in Estimated code review effort🎯 2 (Simple) | ⏱️ ~10-15 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
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)
320-336: Critical logic error: condition prevents endpoint normalization when credentials exist.Line 322 checks
empty($migration->getAttribute('credentials', [])), which means the block only executes when credentials are completely absent. If credentials exist but are incomplete (e.g.,projectIdandapiKeyare present butendpointis missing), the condition evaluates tofalseand the block never executes. This means the endpoint won't be normalized, andprocessSourceat line 342 will fail when accessing$credentials['endpoint'].This contradicts both the AI summary ("when credentials are present but endpoint is empty") and the PR intent to fix entrypoint issues.
Apply this diff to fix the logic:
try { if ( $migration->getAttribute('source') === SourceAppwrite::getName() && - empty($migration->getAttribute('credentials', [])) + !empty($migration->getAttribute('credentials', [])) ) { $credentials = $migration->getAttribute('credentials', []); $credentials['projectId'] = $credentials['projectId'] ?? $project->getId(); $credentials['apiKey'] = $credentials['apiKey'] ?? $tempAPIKey(); /** * endpoint set */ if (empty($credentials['endpoint'])) { $protocol = System::getEnv('_APP_OPTIONS_FORCE_HTTPS') === 'disabled' ? 'http' : 'https'; $credentials['endpoint'] = $protocol . '://' . $platform['apiHostname'] . '/v1'; } $migration->setAttribute('credentials', $credentials); }Alternatively, remove the
empty()check entirely to handle both empty and incomplete credentials:try { - if ( - $migration->getAttribute('source') === SourceAppwrite::getName() && - empty($migration->getAttribute('credentials', [])) - ) { + if ($migration->getAttribute('source') === SourceAppwrite::getName()) { $credentials = $migration->getAttribute('credentials', []); $credentials['projectId'] = $credentials['projectId'] ?? $project->getId(); $credentials['apiKey'] = $credentials['apiKey'] ?? $tempAPIKey; /** * endpoint set */ if (empty($credentials['endpoint'])) { $protocol = System::getEnv('_APP_OPTIONS_FORCE_HTTPS') === 'disabled' ? 'http' : 'https'; $credentials['endpoint'] = $protocol . '://' . $platform['apiHostname'] . '/v1'; } $migration->setAttribute('credentials', $credentials); }The second approach is simpler and leverages the null coalescing operators to handle all cases properly.
🧹 Nitpick comments (1)
src/Appwrite/Platform/Workers/Migrations.php (1)
328-330: Improve comment to explain the WHY, not just the WHAT.The comment "endpoint set" simply restates what the code does. A better comment would explain why this normalization is needed and when it applies.
Consider this more informative comment:
- /** - * endpoint set - */ + // Normalize endpoint to platform hostname when not explicitly provided + // This handles migrations where endpoint is empty or uses localhost
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Platform/Workers/Migrations.php(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9661
File: docs/references/migrations/migration-csv.md:1-1
Timestamp: 2025-04-17T08:08:59.449Z
Learning: Documentation files in docs/references/migrations/ follow a minimalist pattern with 1-2 sentences describing the endpoint's functionality, without including detailed API specifications like HTTP methods, parameters, headers, or examples.
📚 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-04-17T08:08:59.449Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9661
File: docs/references/migrations/migration-csv.md:1-1
Timestamp: 2025-04-17T08:08:59.449Z
Learning: Documentation files in docs/references/migrations/ follow a minimalist pattern with 1-2 sentences describing the endpoint's functionality, without including detailed API specifications like HTTP methods, parameters, headers, or examples.
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: moderate
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
✨ Benchmark results
⚡ Benchmark Comparison
|
3 participants
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