added temporary blocking the delete of null metadata docs #10178
Conversation
📝 Walkthrough""" WalkthroughThe Estimated code review effort2 (10–30 minutes) Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (3)
✨ 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: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Platform/Workers/Deletes.php(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ItzNotABug
PR: appwrite/appwrite#9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Indexes/Delete.php:40-40
Timestamp: 2025-06-25T06:25:45.054Z
Learning: In Appwrite's database module, the event label for deleting a table index intentionally uses 'update' (e.g., 'databases.[databaseId].tables.[tableId].indexes.[indexId].update') instead of 'delete', to match legacy behavior as seen in previous controller implementations.
Learnt from: ItzNotABug
PR: appwrite/appwrite#9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php:57-59
Timestamp: 2025-06-19T09:20:03.312Z
Learning: In table-related endpoints (such as `src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php`), parameter descriptions should use "table" and "row" terminology instead of "collection" and "document" for clarity and consistency.
Learnt from: ItzNotABug
PR: appwrite/appwrite#9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Delete.php:88-95
Timestamp: 2025-05-09T04:06:04.338Z
Learning: It's acceptable to use DATABASE_TYPE_DELETE_COLLECTION event type for both collections and tables because workers process these events based on whether setCollection() or setTable() was called, not on the event type itself.
src/Appwrite/Platform/Workers/Deletes.php (3)
Learnt from: ItzNotABug
PR: #9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php:57-59
Timestamp: 2025-06-19T09:20:03.312Z
Learning: In table-related endpoints (such as src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php), parameter descriptions should use "table" and "row" terminology instead of "collection" and "document" for clarity and consistency.
Learnt from: ItzNotABug
PR: #9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Indexes/Delete.php:40-40
Timestamp: 2025-06-25T06:25:45.054Z
Learning: In Appwrite's database module, the event label for deleting a table index intentionally uses 'update' (e.g., 'databases.[databaseId].tables.[tableId].indexes.[indexId].update') instead of 'delete', to match legacy behavior as seen in previous controller implementations.
Learnt from: ItzNotABug
PR: #9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Delete.php:88-95
Timestamp: 2025-05-09T04:06:04.338Z
Learning: It's acceptable to use DATABASE_TYPE_DELETE_COLLECTION event type for both collections and tables because workers process these events based on whether setCollection() or setTable() was called, not on the event type itself.
⏰ 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). (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (2)
src/Appwrite/Platform/Workers/Deletes.php (2)
600-602: Code looks good for configuration retrieval.The metadata UID collection and audit addition is correctly implemented.
604-607: Consider performance implications of unlimited query.Using
PHP_INT_MAXas the limit retrieves all matching metadata documents, which could impact performance for large datasets. However, this appears necessary for complete tenant validation.
| $sequencesToDelete = []; | ||
|
|
||
| foreach ($metadataDocuments as $metadataDoc) { | ||
| if (!is_null($metadataDoc->getTenant()) && $metadataDoc->getTenant() === (int)$document->getSequence()) { | ||
| Console::log("Found metadata sequence: {$metadataDoc->getSequence()} (tenant: {$metadataDoc->getTenant()})"); | ||
| $sequencesToDelete[] = $metadataDoc->getSequence(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Address type safety and logging concerns.
Several issues with this implementation:
-
Type coercion risk: The comparison
$metadataDoc->getTenant() === (int)$document->getSequence()mixes types and could lead to unexpected behavior ifgetTenant()doesn't return an integer. -
Information exposure: The console log on line 613 exposes internal sequence and tenant information that might be sensitive.
Apply this diff to improve type safety and remove sensitive logging:
- foreach ($metadataDocuments as $metadataDoc) {
- if (!is_null($metadataDoc->getTenant()) && $metadataDoc->getTenant() === (int)$document->getSequence()) {
- Console::log("Found metadata sequence: {$metadataDoc->getSequence()} (tenant: {$metadataDoc->getTenant()})");
- $sequencesToDelete[] = $metadataDoc->getSequence();
- }
- }
+ foreach ($metadataDocuments as $metadataDoc) {
+ $tenant = $metadataDoc->getTenant();
+ if ($tenant !== null && (int)$tenant === $document->getSequence()) {
+ $sequencesToDelete[] = $metadataDoc->getSequence();
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $sequencesToDelete = []; | |
| foreach ($metadataDocuments as $metadataDoc) { | |
| if (!is_null($metadataDoc->getTenant()) && $metadataDoc->getTenant() === (int)$document->getSequence()) { | |
| Console::log("Found metadata sequence: {$metadataDoc->getSequence()} (tenant: {$metadataDoc->getTenant()})"); | |
| $sequencesToDelete[] = $metadataDoc->getSequence(); | |
| } | |
| } | |
| $sequencesToDelete = []; | |
| foreach ($metadataDocuments as $metadataDoc) { | |
| $tenant = $metadataDoc->getTenant(); | |
| if ($tenant !== null && (int)$tenant === $document->getSequence()) { | |
| $sequencesToDelete[] = $metadataDoc->getSequence(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/Appwrite/Platform/Workers/Deletes.php around lines 609 to 616, improve
type safety by ensuring both values compared are of the same expected type
without coercion, for example by explicitly casting or validating types before
comparison. Also, remove or replace the Console::log statement that outputs
internal sequence and tenant details to avoid exposing sensitive information.
Adjust the code to safely handle type checks and eliminate sensitive logging
accordingly.
✨ Benchmark results
⚡ Benchmark Comparison
|
7e6c6ce to
4453038
Compare
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