Fix update documents #646
Fix update documents #646
Conversation
WalkthroughThis change refines how document permissions are handled during creation and batch updates. It ensures that all new documents explicitly have a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Database
participant SQLAdapter
Client->>Database: createDocument / createDocuments
Database->>Database: Ensure $permissions is set (empty if missing)
Database->>SQLAdapter: Insert document(s) with $permissions
Client->>Database: updateDocuments (with/without permissions)
Database->>Database: For each document, compare permissions
Database->>Database: Set $skipPermissionsUpdate if unchanged
Database->>SQLAdapter: updateDocuments(batch)
SQLAdapter->>SQLAdapter: For each doc, skip permissions update if $skipPermissionsUpdate
SQLAdapter->>Database: Return updated docs
Database->>Database: Remove $skipPermissionsUpdate from docs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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). (11)
🔇 Additional comments (4)
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/Database/Database.php (1)
6348-6350: Skipping JSON filter for$permissionsmay break persistence
$permissionsis still declared astype=stringwith thejsonfilter (Line 219).
By short-circuiting the encode path here we now try to persist a PHP array into a string column, bypassingjson_encode. Drivers such as PDO/MySQL will error with “Array to string conversion”.Either:
- if ($key === '$permissions') { - continue; - } + // Keep applying json filter so DB still receives a string + // but skip the automatic []-when-empty logic. + if ($key === '$permissions' && !in_array('json', $filters, true)) { + continue; + }or change the column type and adapter logic to accept native JSON.
♻️ Duplicate comments (1)
src/Database/Database.php (1)
3715-3718: Duplicate logic – consider extracting helperThe same if-empty-then-[] block now exists in both
createDocument()andcreateDocuments(). Factor it out (e.g.applyDefaultPermissions(Document $d)) to avoid divergence later.
🧹 Nitpick comments (1)
src/Database/Adapter/SQL.php (1)
434-442: Remove or implement the commented optimization code.The commented-out permission comparison optimization should either be:
- Removed if not needed
- Properly implemented and tested if the optimization is valuable
Dead code should not remain in the codebase.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
phpunit.xml(1 hunks)src/Database/Adapter/SQL.php(3 hunks)src/Database/Database.php(7 hunks)src/Database/Document.php(1 hunks)tests/e2e/Adapter/Base.php(1 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(1 hunks)tests/e2e/Adapter/Scopes/PermissionTests.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: in postgresql adapter methods like getupsertstatement, complexity for database-specific sql generati...
Learnt from: ArnabChatterjee20k
PR: utopia-php/database#613
File: src/Database/Adapter/Postgres.php:1254-1319
Timestamp: 2025-07-01T11:31:37.438Z
Learning: In PostgreSQL adapter methods like getUpsertStatement, complexity for database-specific SQL generation is acceptable when the main business logic is properly separated in the parent SQL adapter class, following the adapter pattern where each database adapter handles its own SQL syntax requirements.
Applied to files:
src/Database/Adapter/SQL.php
🔇 Additional comments (11)
phpunit.xml (1)
10-10: Verify if this configuration change should be permanent.The
stopOnFailureattribute has been changed to"true", which will halt test execution on the first failure. This is typically used during debugging but may not be desired in CI/CD pipelines where you want to see all test failures.Is this change intentional for production, or should it be reverted before merging?
src/Database/Document.php (1)
279-285: LGTM! Good simplification.Removing the conditional check before
unset()is a good optimization. In PHP,unset()is safe to call on non-existent array keys and doesn't produce errors, making the existence check unnecessary.tests/e2e/Adapter/Base.php (1)
20-23: Verify that disabling test suites is intentional and temporary.Four major test trait suites have been commented out (CollectionTests, DocumentTests, AttributeTests, IndexTests), which significantly reduces test coverage. This appears to be for focusing on permission-related testing during development.
Please confirm:
- Is this change temporary for debugging/development purposes?
- Should these test suites be re-enabled before merging?
- Are there specific failures in these test suites that need to be addressed?
Consider using test filtering or separate test configurations instead of commenting out entire test suites to maintain coverage.
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
453-456: LGTM! Proper test enhancement for permission initialization.The added assertions correctly verify that documents returned by
createOrUpdateDocumentshave the$permissionskey present and initialized as an empty array. This aligns well with the broader changes mentioned in the AI summary to ensure$permissionsis always initialized in the database layer.The test enhancement follows good practices:
- Uses appropriate PHPUnit assertions (
assertArrayHasKeyandassertEquals)- Tests both the presence of the key and its expected value
- Complements the existing test logic without disrupting the flow
src/Database/Adapter/SQL.php (2)
430-432: LGTM! Permission detection logic improved.The change from
!empty($updates->getPermissions())to$updates->offsetExists('$permissions')correctly handles cases where permissions are explicitly set to an empty array, ensuring the attribute is properly encoded even when permissions are being cleared.
498-498: LGTM! Consistent permission handling logic.The change maintains consistency with the earlier modification on line 430, ensuring that permission update logic is triggered whenever the
$permissionskey exists, supporting proper handling of permission removal scenarios.tests/e2e/Adapter/Scopes/PermissionTests.php (1)
17-137: Excellent test coverage for permission handling.This test method provides comprehensive coverage of the permission update functionality:
- ✅ Creates documents with permissions
- ✅ Validates permission preservation during updates without permission changes
- ✅ Tests explicit permission removal via empty array
- ✅ Verifies authorization behavior after permission removal
- ✅ Uses
Authorization::disable()to confirm documents still existThe test logic is well-structured and thoroughly validates the core permission handling changes.
src/Database/Database.php (4)
4437-4459: 👍 Rewriting the loop removes the reference trapSwitching to
foreach ($batch as $index => $document)and assigning back with$batch[$index]avoids the by-reference side-effects that previously corrupted$batch. Nice clean-up.
6429-6432: Decode still skips$permissions– verify symmetryOnce the encode side is fixed (see previous comment), keeping decode / casting skipped is fine because callers expect an array.
Just make sure the adapter returns the decoded array; otherwise the application will start receiving raw JSON strings.
6492-6494: Casting exclusion looks goodSince
$permissionsis an array of strings there’s nothing to cast; excluding it here avoids an unnecessaryjson_decodebranch.
3612-3615: Empty permissions default may lock documents down
Automatically defaulting a missing$permissionsvalue to an empty array will deny all access (read/update/delete) on the newly created documents—often even to their creator—unless you’ve layered on higher-level ACL rules. Please confirm that this “zero-access” behavior is intentional. Most schemas instead:
- inherit the parent collection’s default roles, or
- grant at least a
read(and oftenwrite) right to the creating user.Locations to review:
- src/Database/Database.php Lines 3612–3615
- src/Database/Database.php Lines 3715–3718
If you do intend tighter defaults, consider documenting this ACL change. Otherwise, adjust the fallback to include the creator’s role or your collection’s standard defaults.
Summary by CodeRabbit
Bug Fixes
Tests