Skip permissions updates #636
Conversation
WalkthroughThe update modifies the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Database
participant Adapter
Client->>Database: updateDocument(collection, id, document)
Database->>Database: Compare old and new permissions
Database->>Adapter: updateDocument(collection, id, document, skipPermissions)
Adapter-->>Database: Updated Document
Database-->>Client: Updated Document
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code Graph Analysis (1)src/Database/Adapter/Postgres.php (6)
🪛 PHPMD (2.15.0)src/Database/Adapter/MariaDB.php935-935: Avoid unused parameters such as '$id'. (Unused Code Rules) (UnusedFormalParameter) 1052-1052: Avoid unused local variables such as '$_'. (Unused Code Rules) (UnusedLocalVariable) src/Database/Adapter/Postgres.php1055-1055: Avoid unused parameters such as '$id'. (Unused Code Rules) (UnusedFormalParameter) 1171-1171: Avoid unused local variables such as '$_'. (Unused Code Rules) (UnusedLocalVariable) 🔇 Additional comments (6)
✨ 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. 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/Database/Adapter/SQLite.php (1)
640-845: Critical: TheskipPermissionsparameter is not implemented.The SQLite adapter accepts the
$skipPermissionsparameter but doesn't use it anywhere in the method implementation. According to the AI summary, other adapters (MariaDB, Postgres) conditionally skip permission updates based on this flag, but SQLite continues to execute all permission operations regardless.This breaks the intended optimization and creates inconsistent behavior across different database adapters.
The permission update logic (lines 651-843) should be wrapped in a conditional check:
+ if (!$skipPermissions) { // All the existing permission logic from lines 651-843 $sql = "SELECT _type, _permission..."; // ... rest of permission handling code + }This ensures SQLite behaves consistently with other database adapters when permissions haven't changed.
🧹 Nitpick comments (2)
src/Database/Database.php (2)
4098-4109: Review the permissions comparison logic for potential edge casesThe permissions comparison logic looks generally correct, but there are a few considerations:
- The condition
$document->offsetExists('$permissions')should probably also check if the old document has permissions to ensure consistent comparison- The sorting approach is good for comparing arrays regardless of order
- Consider what happens when one document has permissions and the other doesn't
Consider this more robust approach:
- if ($document->offsetExists('$permissions')){ + if ($document->offsetExists('$permissions') && $old->offsetExists('$permissions')){ $originalPermissions = $old->getPermissions(); $currentPermissions = $document->getPermissions(); sort($originalPermissions); sort($currentPermissions); $skipPermissionsUpdate = ($originalPermissions === $currentPermissions); + } elseif (!$document->offsetExists('$permissions') && !$old->offsetExists('$permissions')) { + $skipPermissionsUpdate = true; // Both have no permissions, so skip update + } else { + $skipPermissionsUpdate = false; // One has permissions, other doesn't - update needed }
4932-4943: Consistent implementation of permissions comparison logicThe permissions comparison logic in
createOrUpdateDocumentsWithIncreaseis consistent with the logic inupdateDocument. However, it has the same potential edge case issue as mentioned in the previous comment.Apply the same improvement suggested for the
updateDocumentmethod to ensure consistent behavior across both methods.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Database/Adapter.php(1 hunks)src/Database/Adapter/MariaDB.php(2 hunks)src/Database/Adapter/Pool.php(1 hunks)src/Database/Adapter/Postgres.php(4 hunks)src/Database/Adapter/SQLite.php(1 hunks)src/Database/Database.php(5 hunks)
🧰 Additional context used
🪛 PHPMD (2.15.0)
src/Database/Adapter/SQLite.php
640-640: Avoid unused parameters such as '$skipPermissions'. (Unused Code Rules)
(UnusedFormalParameter)
src/Database/Adapter/Postgres.php
1056-1056: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
src/Database/Adapter/MariaDB.php
936-936: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
1053-1053: Avoid unused local variables such as '$_'. (Unused Code Rules)
(UnusedLocalVariable)
🔇 Additional comments (12)
src/Database/Adapter/Pool.php (1)
238-241: LGTM! Method signature correctly updated.The Pool adapter properly extends the
updateDocumentmethod signature to include the$skipPermissionsparameter. Since this is a proxy class that uses thedelegatemethod with\func_get_args(), the new parameter will be automatically forwarded to the underlying adapter implementation.src/Database/Adapter.php (1)
700-704: All adapter implementations correctly updated with$skipPermissions.The abstract
updateDocumentsignature insrc/Database/Adapter.phpnow includesbool $skipPermissions, and all concrete adapter classes implement it:
- src/Database/Adapter/SQLite.php
- src/Database/Adapter/Pool.php
- src/Database/Adapter/Postgres.php
- src/Database/Adapter/MariaDB.php
No outstanding mismatches found.
src/Database/Adapter/MariaDB.php (2)
929-936: LGTM! Method signature properly updated.The addition of the
$skipPermissionsboolean parameter is well-implemented with proper type hinting and documentation.
947-1094: LGTM! Permissions logic correctly wrapped in conditional.The implementation properly skips all permission-related database operations when
$skipPermissionsis true while preserving existing functionality. The variable scoping withisset()checks for the prepared statements is handled correctly.src/Database/Adapter/Postgres.php (4)
1047-1051: LGTM: Documentation formatting improved.The docblock formatting has been properly cleaned up with consistent spacing.
1056-1056: Method signature correctly updated with new parameter.The addition of the
$skipPermissionsboolean parameter aligns with the PR objective. The parameter type and naming are appropriate.Note: The static analysis warning about unused parameter
$idis a false positive - the parameter is actively used throughout the method (lines 1080, 1153, 1187, 1224).
1132-1132: Minor SQL query formatting improvement.The SQL parameter binding formatting has been slightly improved for better readability.
1066-1198: Conditional Scope Verified: Wraps All Permission Logic CorrectlyI’ve confirmed that the
if (!$skipPermissions)block opens at line 1066 and its closing brace at line 1196 cleanly encloses every permission‐related operation (reads, diffs, deletes, inserts) and then exits before the attribute‐update section. Braces are properly balanced. No changes needed here.src/Database/Database.php (4)
593-596: LGTM: Proper initialization of permissions attributeThe code correctly initializes the
$permissionsattribute to an empty array when it doesn't exist on the document during creation. This ensures consistency and prevents potential null reference issues later in the code.
4266-4266: LGTM: Proper parameter passing to adapterThe
$skipPermissionsUpdateflag is correctly passed to the adapter'supdateDocumentmethod, maintaining the intended optimization behavior.
4946-4946: Verify the logical condition combinationThe condition combines
empty($attribute)and$skipPermissionsUpdatewith AND logic. This means that when permissions are unchanged AND no specific attribute is being updated, the document update is skipped entirely.Please verify this logic is correct. The current implementation will skip the document update if:
- No specific attribute is being updated (
empty($attribute))- Permissions haven't changed (
$skipPermissionsUpdate)- Document attributes are the same
This seems correct, but please confirm this is the intended behavior, especially in edge cases where other document metadata might need updating.
5002-5004: LGTM: Proper permissions preservation when skipping updateWhen permissions update is being skipped, the code correctly preserves the old document's permissions. This ensures that the optimization doesn't inadvertently change permission values.
Summary by CodeRabbit
New Features
Bug Fixes