Conversation
📝 WalkthroughWalkthroughReplaces prior array-attribute handling in Databases index creation: instead of assigning default length/order for array attributes, the code now throws an INDEX_INVALID exception with message "Creating indexes on array attributes is not currently supported." The change also removes generation of default system attributes (createdAt/updatedAt) in the oldAttributes metadata, reformats the attribute-metadata mapping, and updates tests to expect 400 responses and the new error message where index creation on array attributes is attempted. Minor formatting and comment cleanup included. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ 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! |
✨ Benchmark results
⚡ Benchmark Comparison
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Indexes/Create.php (1)
171-173: Guarding against indexes on array attributes matches the intent; consider future‑proofingThe explicit guard on
$attributeArray === truewithException::INDEX_INVALIDand a clear message is an appropriate way to block problematic array-attribute indexes and gives clients a precise failure reason. One optional improvement for later would be to gate this on adapter capabilities (or a feature flag) so that when the underlying MySQL issue is resolved or alternative adapters support array indexes, this behavior can be relaxed without touching endpoint logic again.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Indexes/Create.php(2 hunks)tests/e2e/Services/Databases/Legacy/DatabasesBase.php(8 hunks)tests/e2e/Services/Databases/TablesDB/DatabasesBase.php(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/e2e/Services/Databases/TablesDB/DatabasesBase.php
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10800
File: tests/e2e/Services/Databases/TablesDB/Transactions/TransactionsBase.php:5661-5662
Timestamp: 2025-11-12T08:56:08.516Z
Learning: The arrayInsert operator in Appwrite's database transactions uses the format: `{"method":"arrayInsert","attribute":"","values":[index, value]}` where the index is taken from values[0] and the actual value to insert is taken from values[1]. This differs from having separate "index" and "values" fields.
🧬 Code graph analysis (1)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Indexes/Create.php (1)
src/Appwrite/Extend/Exception.php (1)
Exception(7-465)
⏰ 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 (3)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Indexes/Create.php (1)
111-114: Attribute metadata mapping is clear and consistent with later usageBuilding
$oldAttributesviaarray_map(fn ($a) => $a->getArrayCopy(), $collection->getAttribute('attributes'))cleanly normalizes the collection’s attribute documents into arrays and aligns with laterarray_column($oldAttributes, 'key')and direct array access. No issues from a correctness or readability standpoint here.tests/e2e/Services/Databases/Legacy/DatabasesBase.php (2)
1354-1423: Array-attribute index blocking tests look correct and comprehensiveThe updated assertions for
fulltextArray,actorsArray,twoLevelsArray,index1andindex2all consistently expect HTTP 400 and the unified message"Creating indexes on array attributes is not currently supported.", which aligns well with the new behavior and covers single, multi-attribute, and ordered index cases. The reduced index count assertion (4 indexes) also matches the remaining non-array index creations earlier in the test. I don’t see any functional gaps here.
1490-1503: New length+array index negative case is a good edge‑case additionThe extra branch in
testGetIndexByKeyWithLengthsthat attempts a length override on theactorsarray attribute and asserts 400 plus the same “array attributes” error message cleanly exercises the new guard in the lengths path without affecting the existing positive lengths test. This nicely closes a potential hole in validation.
What does this PR do?
Because of a bug in MySQL, we cannot create indexes on array attributes for now, otherwise queries break (by not returning any data). This PR temporarily disables creating indexes on arrays.
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