Fix encode permissions #645
Conversation
WalkthroughThe changes update how the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Database
participant Adapter
Client->>Database: createDocument(data, permissions)
Database->>Database: If permissions empty, set as []
Database->>Adapter: createDocument(data with permissions)
Adapter-->>Database: Document created
Client->>Database: decode(document)
Database->>Database: For each attribute
alt attribute == "$permissions"
Database->>Database: Skip decoding/casting
else
Database->>Database: Decode/cast attribute
end
Database-->>Client: Decoded document
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 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 (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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: 1
🧹 Nitpick comments (3)
src/Database/Database.php (2)
3612-3614: Prefer anoffsetExistscheck overempty()
empty($document->getPermissions())evaluates to true even when the caller deliberately passed an empty array.
A more precise guard is:if (!$document->offsetExists('$permissions')) { $document->setAttribute('$permissions', []); }(Same duplication appears in both create paths – consider extracting into a small helper to keep things DRY.)
Also applies to: 3715-3717
6346-6349: Remove dead, commented-out codeThe fallback that injected empty permissions is now obsolete—commenting it out just adds noise. Please delete these lines to keep the method lean.
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
452-456: Use a strict comparison via the dedicated accessorA stricter and slightly clearer assertion improves both intent and failure diagnostics:
foreach ($results as $result) { $this->assertArrayHasKey('$permissions', $result); - $this->assertEquals([], $result->getAttribute('$permissions')); + // Rely on the public accessor and a strict check + $this->assertSame([], $result->getPermissions()); }
assertSameavoids accidental truthiness matches, andgetPermissions()is the canonical way to read the permissions field.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Database/Database.php(4 hunks)src/Database/Document.php(1 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(1 hunks)tests/e2e/Adapter/Scopes/PermissionTests.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/Database/Database.php (2)
src/Database/Document.php (2)
getPermissions(87-90)setAttribute(236-253)src/Database/Query.php (1)
setAttribute(161-166)
🔇 Additional comments (2)
src/Database/Document.php (1)
279-285: Good simplification - removing unnecessary conditional check.The removal of the conditional existence check before
unset($this[$key])is a positive change. In PHP,unset()is safe to call on non-existent array keys and doesn't produce errors or warnings, making the existence check redundant. This simplification improves performance by eliminating an unnecessaryisset()call while maintaining the same functionality.tests/e2e/Adapter/Scopes/PermissionTests.php (1)
17-132: Well-structured test for permission unsetting behavior.The test comprehensively validates the permission handling changes:
- Creates documents with full permissions and verifies creation
- Updates without specifying permissions and confirms existing permissions are preserved
- Updates with empty permissions array and verifies permissions are cleared
- Uses proper assertions and follows testing best practices
| if (\array_key_exists($key, (array)$this)) { | ||
| unset($this[$key]); | ||
| } | ||
| unset($this[$key]); |
There was a problem hiding this comment.
No need to check + copy array since unset is silent
Summary by CodeRabbit