added encrypt property in the attribute string response model #9868
Conversation
|
Warning Rate limit exceeded@ArnabChatterjee20k has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 9 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)
WalkthroughThe changes introduce an Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Controller
participant Attribute_Model
participant Response
Client->>API_Controller: POST /databases/{id}/collections/{id}/attributes/string
API_Controller->>Attribute_Model: createAttribute(...)
Attribute_Model-->>API_Controller: Attribute Document
API_Controller->>API_Controller: Add 'encrypt' flag to attribute data
API_Controller->>Response: Send Document with 'encrypt' in response
Response-->>Client: 202 Accepted + Attribute data (with 'encrypt')
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)
tests/e2e/Services/Databases/DatabasesBase.php (1)
301-301: Added assertion for the newencryptflag
The newassertFalse($title['body']['encrypt'])correctly validates that theencryptproperty is included and defaults tofalse. Great addition. Consider extending this coverage by asserting theencryptfield in the GET‐based tests (testAttributeResponseModels,testListAttributes) to ensure it’s propagated consistently across all response models.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/controllers/api/databases.php(1 hunks)src/Appwrite/Utopia/Response/Model/AttributeString.php(1 hunks)tests/e2e/Services/Databases/DatabasesBase.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 (2)
src/Appwrite/Utopia/Response/Model/AttributeString.php (1)
27-33: LGTM! Well-implemented encryption flag with proper documentation.The implementation follows best practices:
- Consistent pattern: Matches the existing
addRule()structure used throughout the constructor- Appropriate typing: Boolean type is perfect for this encryption flag
- Comprehensive documentation: The description clearly explains both the security benefit and the important trade-off (encrypted attributes cannot be queried)
- Backward compatibility: Default value of
falseandrequired: falseensures existing code continues to work- Clear example: The example value matches the default, maintaining consistency
The warning about query limitations in the description is particularly valuable for developers who need to understand the implications of enabling encryption.
app/controllers/api/databases.php (1)
1375-1377: LGTM! Good approach to expose encrypt flag in API response.The implementation correctly adds the
encryptproperty to the string attribute response by:
- Creating a copy of the attribute data
- Adding the encrypt flag from the request parameter
- Wrapping it back into a Document object
This provides a clean API interface where clients get the encrypt status directly rather than parsing it from the filters array.
app/controllers/api/databases.php
Outdated
| $stringAttribute = $attribute->getArrayCopy(); | ||
| $stringAttribute['encrypt'] = $encrypt; | ||
| $stringAttribute = new Document($stringAttribute); |
There was a problem hiding this comment.
could simply just be - $attribute->setAttribute('encrypt', $encrypt); since its a document already.
app/controllers/api/databases.php
Outdated
| foreach ($attributes as $attribute) { | ||
| $filters = $attribute->getAttribute('filters', []); | ||
| $attributeArray = $attribute->getArrayCopy(); | ||
| $attributeArray['encrypt'] = in_array('encrypt', $filters); | ||
| $updatedAttribute = new Document($attributeArray); | ||
| $updatedAttributes[] = $updatedAttribute; | ||
| } |
There was a problem hiding this comment.
why do we need this to add encrypt to all the attributes?
There was a problem hiding this comment.
Let's also update here:
appwrite/app/init/database/filters.php
Line 67 in 1e22076
This is called when you get a collection from the database to populate it's attributes. Let's also add a check in the getCollection test to ensure that it's set
app/init/database/filters.php
Outdated
| 8000 | $attributeType = $attribute->getAttribute('type'); | |
| if ($attributeType === Database::VAR_RELATIONSHIP) { | ||
| $options = $attribute->getAttribute('options'); | ||
| foreach ($options as $key => $value) { | ||
| $attribute->setAttribute($key, $value); | ||
| } | ||
| $attribute->removeAttribute('options'); | ||
| } | ||
| if ($attributeType === Database::VAR_STRING) { | ||
| $filters = $attribute->getAttribute('filters', []); | ||
| $attribute->setAttribute('encrypt', in_array('encrypt', $filters)); | ||
| } |
There was a problem hiding this comment.
In these sort of cases, lets always use a switch. Even if we only have two cases now, switch is easier to extend cleanly in future than multiple if's
| foreach ($attributes as $attribute) { | ||
| $this->assertArrayHasKey('encrypt', $attribute); | ||
| if ($attribute['key'] === 'firstName') { | ||
| $firstNameAttribute = $attribute['encrypt']; | ||
| } | ||
| if ($attribute['key'] === 'lastName') { | ||
| $lastNameAttribute = $attribute['encrypt']; | ||
| } | ||
| } | ||
| $this->assertTrue($lastNameAttribute); | ||
| $this->assertFalse($firstNameAttribute); |
There was a problem hiding this comment.
Let's just assert inside the loop, then we will fail fast if we get the wrong value
added encrypt property in the attribute string response model
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