-
Notifications
You must be signed in to change notification settings - Fork 4.4k
Fix indexes for 1.7.x
#9752
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix indexes for 1.7.x
#9752
Conversation
WalkthroughThe changes update index definitions in two configuration files. In the Changes
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
💤 Files with no reviewable changes (4)
🧰 Additional context used🧬 Code Graph Analysis (2)tests/e2e/Services/Projects/ProjectsConsoleClientTest.php (1)
tests/e2e/Services/Sites/SitesCustomServerTest.php (1)
⏰ Context from checks skipped due to timeout of 90000ms (16)
🔇 Additional comments (5)
✨ 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/config/collections/projects.php (1)
2086-2089
: Consider updating the index name to match the new attribute nameWhile the attribute has been updated, the index name
_key_function_internal_id
still references the old attribute name, which could lead to confusion during future maintenance.Consider updating the index name to better reflect its current purpose:
- '$id' => ID::custom('_key_function_internal_id'), + '$id' => ID::custom('_key_resource_internal_id'), 'type' => Database::INDEX_KEY, 'attributes' => ['resourceInternalId'], 'lengths' => [], 'orders' => [],
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/config/collections/platform.php
(1 hunks)app/config/collections/projects.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 (1)
app/config/collections/projects.php (1)
2086-2086
:✅ Verification successful
Index attribute renamed for standardization
The attribute for the
_key_function_internal_id
index has been updated fromfunctionInternalId
toresourceInternalId
, establishing consistent naming patterns across the codebase for resource references.Let's verify that there are no references to the old attribute name that might be affected by this change:
🏁 Script executed:
#!/bin/bash # Search for any code referencing the old attribute name rg "functionInternalId" --type phpLength of output: 2462
Confirming rename of
functionInternalId
→resourceInternalId
Search for “functionInternalId” returns only migration scripts (V15, V17, V19, V20) where the attribute is created and then renamed in V19. All application code (queries, deletes, etc.) already uses
resourceInternalId
. No remaining references to update—ready to approve.
@@ -1420,7 +1420,7 @@ | |||
[ | |||
'$id' => ID::custom('_key_piid_riid_rt'), | |||
'type' => Database::INDEX_KEY, | |||
'attributes' => ['projectInternalId', 'resourceInternalId', 'resourceType'], | |||
'attributes' => ['projectInternalId', 'deploymentInternalId', 'deploymentResourceType'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Index attributes updated to align with deployment terminology
The attributes for the _key_piid_riid_rt
index have been updated from resource-based to deployment-based identifiers, which aligns with modern deployment-focused architecture.
Let's verify that this change has been consistently applied across the codebase:
🏁 Script executed:
#!/bin/bash
# Search for any code that might be using the old index attributes
rg "resourceInternalId.*resourceType" --type php
Length of output: 5420
Inconsistent index definitions: update remaining resource-based indexes
I see the _key_piid_riid_rt
index in app/config/collections/platform.php
has been updated to use deploymentInternalId
and deploymentResourceType
, but there are still other index definitions that reference the old resourceInternalId
/resourceType
fields. Please update these to match the new deployment-based naming:
- app/config/collections/platform.php
• Index on['projectInternalId', 'resourceInternalId', 'resourceType']
- app/config/collections/projects.php
• Multiple indexes with['resourceInternalId', 'resourceType']
•_key_resourceInternalId_resourceType
Once these are updated, all platform- and project-collection indexes will consistently use deploymentInternalId
and deploymentResourceType
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
those work fine because they have the same attributes. the docker build on local went fine and the stack started fine as well without any errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ItzNotABug Thanks for the clarification. I understand that both naming patterns are functionally equivalent and your local testing confirms everything works properly. While standardizing the naming across all indexes could help with code readability and maintenance in the future, it's not causing any functional issues.
Would you like me to stop flagging these naming inconsistencies in this PR since your primary goal is to fix specific indexes for version 1.7.x rather than standardizing all names?
(_/)
( •_•)
/ >🐰 Curious about your priorities for this PR
✏️ Learnings added
Learnt from: ItzNotABug
PR: appwrite/appwrite#9752
File: app/config/collections/platform.php:1423-1423
Timestamp: 2025-05-13T12:02:08.046Z
Learning: In the Appwrite database schema, field names like 'resourceInternalId'/'resourceType' and 'deploymentInternalId'/'deploymentResourceType' are functionally equivalent and can coexist in the codebase without causing errors. The docker build and stack work fine with these naming inconsistencies.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
✨ Benchmark results
⚡ Benchmark Comparison
|
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
Summary by CodeRabbit