Add readable error for csv imports #9947
Conversation
|
Warning Rate limit exceeded@ItzNotABug has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 35 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 Walkthrough""" WalkthroughA blank line was added for formatting in the projects configuration file. The migration worker's error handling was refactored: error messages are now stored and transmitted as JSON objects, with stack traces and sensitive fields removed before sending in realtime events. The migration document update method was adjusted to exclude certain attributes from the realtime payload and avoid redundant updates. A new Changes
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Appwrite/Migration/Version/V23.php (1)
62-62: Fix typo in comment.There's a typo in the comment:
direct access.\should not have the backslash.- * direct access.\ + * direct access.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/config/collections/projects.php(2 hunks)src/Appwrite/Migration/Migration.php(1 hunks)src/Appwrite/Migration/Version/V23.php(1 hunks)src/Appwrite/Platform/Workers/Migrations.php(2 hunks)src/Appwrite/Utopia/Response/Model/Migration.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (6)
app/config/collections/projects.php (1)
2345-2361: Excellent schema design for the new error attribute!The addition of the
errorattribute to the migrations collection is well-designed:
- Clear documentation: The multi-line comment effectively explains the purpose and distinguishes it from the existing
errorsfield- Appropriate constraints: 256 character limit is reasonable for concise, human-readable error messages
- Backward compatibility: Non-required and nullable ensures existing migrations won't break
- Targeted use case: Specifically designed for CSV import failures to be displayed on the frontend
The schema follows established patterns and maintains consistency with other attributes in the collection.
src/Appwrite/Migration/Migration.php (1)
92-92: Standard version mapping addition looks correct.The new version mapping for '1.8.0' => 'V23' follows the established pattern and maintains chronological order in the versions array.
src/Appwrite/Utopia/Response/Model/Migration.php (1)
88-94: Well-designed API model addition for the error field.The new
errorrule is properly structured:
- Singular vs. plural distinction: Correctly defined as a single string (
array => false) to complement the existingerrorsarray field- Targeted description: Specifically mentions "CSV migration process" which aligns with the PR objectives
- Proper defaults: Uses
nulldefault to match the database schema's nullable constraint- Consistent with schema: Matches the non-required, nullable design from the collections configuration
This provides a clean API interface for accessing concise error messages from CSV migrations.
src/Appwrite/Platform/Workers/Migrations.php (2)
368-368: LGTM: Good placement for error message extraction.The call to
setMigrationErrorin thefinallyblock ensures that human-readable error messages are captured regardless of how the migration completes, which is the correct approach.
412-434: Well-implemented error message extraction for CSV migrations.The method correctly:
- Validates that migration, source, and destination objects exist
- Restricts functionality to CSV migrations only (aligning with PR objectives)
- Prioritizes source errors over destination errors
- Uses
getMessage()to get clean error messages without stack tracessrc/Appwrite/Migration/Version/V23.php (1)
67-72: Good error handling for attribute creation.The try-catch block properly handles potential failures during attribute creation and provides informative warning messages. This ensures the migration continues even if attribute creation fails.
✨ Benchmark results
⚡ Benchmark Comparison
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Appwrite/Utopia/Response/Model/Migration.php (1)
7-7: Avoid name shadowing – alias the imported class.
Appwrite\Utopia\Response\Model\Documentalready exists in the current namespace. ImportingUtopia\Database\Documentwithout an alias hides the local class and makes the code harder to reason about.-use Utopia\Database\Document; +use Utopia\Database\Document as DatabaseDocument;Follow-up: update the method signature and return type accordingly (see next comment).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Utopia/Response/Model/Migration.php(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/Appwrite/Utopia/Response/Model/Migration.php (1)
src/Appwrite/Utopia/Response/Model/Document.php (1)
Document(8-97)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Appwrite/Utopia/Response/Model/Migration.php (1)
112-139: Consider returningDatabaseDocumentfor consistency with other modelsOther model filters (
Document::filter,User::filter, etc.) returnDatabaseDocument.
Aligning the return type here would keep the public API symmetric and help static analysers.- public function filter(Document $document): Document + public function filter(DatabaseDocument $document): DatabaseDocument(Requires importing
Utopia\Database\Document as DatabaseDocumentor similar.)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Utopia/Response/Model/Migration.php(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/Appwrite/Utopia/Response/Model/Migration.php (1)
src/Appwrite/Utopia/Response/Model/Document.php (1)
Document(8-97)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
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