Skip permissions update #611
Conversation
|
""" WalkthroughA new boolean parameter 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
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (12)
✨ 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
src/Database/Adapter/SQLite.php (1)
630-643:$skipPermissionsparameter is ignored – expensive permission queries still executeThe new boolean parameter is accepted but never referenced.
As a result, the adapter always performs:
- A full read of the
_permstable (SELECT)- Diff calculation + dynamic DELETE / INSERT statements
This defeats the purpose of the “Skip permissions update” feature and keeps the old performance profile.
A minimal fix is to wrap the whole permissions-handling block (
SELECTon line 651 through construction of$stmtAddPermissionson line 781) in a guard:+ $name = $this->filter($collection); + // Short-circuit when permission updates are not required + if ($skipPermissions) { + $stmtRemovePermissions = null; + $stmtAddPermissions = null; + } else { // existing SELECT, diff, DELETE/INSERT logic … - $name = $this->filter($collection); + } // end ! $skipPermissionsFailing to implement this guard means callers cannot rely on the new optimisation.
Please adjust before merging.src/Database/Adapter/Postgres.php (1)
1244-1251: Atomicity: attributes and permissions are updated in separate statements
$stmt,$stmtRemovePermissions, and$stmtAddPermissionsexecute independently; a failure in the later statements leaves the document half-updated.Wrap the three statements in a transaction (or reuse an existing one) so the whole update rolls back on error, e.g.:
$this->startTransaction(); try { $this->execute($stmt); if (isset($stmtRemovePermissions)) { $this->execute($stmtRemovePermissions); } if (isset($stmtAddPermissions)) { $this->execute($stmtAddPermissions); } $this->commit(); } catch (Throwable $e) { $this->rollback(); throw $e; }(or delegate to the higher-level caller if a transaction is already open).
🧹 Nitpick comments (4)
src/Database/Adapter/SQLite.php (1)
630-636: Docblock advertisesDuplicateExceptionbut method throwsDuplicate
@throws DuplicateExceptionwas added, yet thecatchstill instantiatesnew Duplicate(...)(line 835).
Either:
- Change the caught/ re-thrown exception to
DuplicateException, or- Revert the docblock to
@throws Duplicate.Keeping them inconsistent will mislead static analysers and developers.
src/Database/Adapter/MariaDB.php (1)
948-1047: Large inline SQL string concatenation needs indentation + braces fixSeveral multiline strings inside the new permission-sync block are indented with tabs/spaces that do not match PSR-12, and some
if/foreachbraces are mis-aligned – the linter reportsbraces_position/statement_indentation.Run
php-cs-fixer(or the project’s configured CS tool) on this method to avoid CI failures.src/Database/Adapter/Postgres.php (2)
1080-1101: Use associative fetch to avoid numeric duplicates
$permissionsStmt->fetchAll()defaults toPDO::FETCH_BOTH, producing both numeric and associative keys, doubling memory and risking typos. Fetch only associative rows:-$permissions = $permissionsStmt->fetchAll(); +$permissions = $permissionsStmt->fetchAll(PDO::FETCH_ASSOC);Keeps the subsequent
'_type'/'_permission'access explicit and lean.
1127-1166: SQL placeholder names rely on original array indexesPlaceholders like
:_remove_{$type}_{$i}use the original index fromarray_diff.
If the source permission array has sparse keys, placeholders become e.g.:_remove_read_5, leaving 0-4 unused and complicating debugging.Consider re-indexing for predictable sequential keys:
-foreach ($permissions as $i => $permission) { +foreach (array_values($permissions) as $i => $permission) {Repeat for the addition loop.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Database/Adapter.php(3 hunks)src/Database/Adapter/MariaDB.php(2 hunks)src/Database/Adapter/Postgres.php(2 hunks)src/Database/Adapter/SQLite.php(1 hunks)src/Database/Database.php(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Linter
src/Database/Adapter/MariaDB.php
[error] 1-1: PSR 12 linting failed: braces_position and statement_indentation style issues.
src/Database/Adapter/Postgres.php
[error] 1-1: PSR 12 linting failed: braces_position style issue.
src/Database/Database.php
[error] 1-1: PSR 12 linting failed: single_space_around_construct and braces_position style issues.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Setup & Build Docker Image
- GitHub Check: CodeQL
🔇 Additional comments (7)
src/Database/Database.php (1)
4286-4286: Propagate new$skipPermissionsUpdateflag safelyThe extra boolean argument is now forwarded to
Adapter::updateDocument(). Double-check that all adapter implementations declare the new parameter with a default (false) to avoid fatal errors when called from older code paths.src/Database/Adapter.php (2)
24-24: Skip-permissions flag added but never read in the base classThe property is written (
setSkipPermissions) but there is no correspondinggetSkipPermissions()or any reference to the field elsewhere in this abstract class.
If the flag is meant to be consumed only by concrete adapters, consider:
• documenting that intent, or
• promoting the method toprotectedand dropping the public field entirely to avoid leaking an API that does nothing here.
715-718: Signature change may be a breaking API – double-check downstream
updateDocument(..., bool $skipPermissions)is now mandatory.
Confirm that every call-site (and every concrete adapter) was updated; otherwise you’ll get fatal errors.If painless migration is preferred, consider giving the parameter a default:
-abstract public function updateDocument(string $collection, string $id, Document $document, bool $skipPermissions): Document; +abstract public function updateDocument( + string $collection, + string $id, + Document $document, + bool $skipPermissions = false +): Document;src/Database/Adapter/MariaDB.php (2)
930-938: Method signature aligned – good catchThe adapter now implements the new
$skipPermissionsparameter.
No issues spotted with the declaration itself.
1051-1081: Parameter placeholders reused – risk of collision
:_uidand:_tenantplaceholders are bound once, then reused across multiple VALUES rows.
That’s legal for PDO but each VALUES tuple already contains constant:_uid/:_tenant, so only one value is actually transmitted per execute.
If rows may differ (batch updates) you need unique placeholders per tuple.src/Database/Adapter/Postgres.php (2)
1048-1058: Backward-compatibility break: new non-optional parameter without default
updateDocument()now requiresbool $skipPermissions, turning a previously 3-arg call into 4-args.
Any caller that wasn’t updated will now fatal-error.
If you intend optional behaviour, add a default:-public function updateDocument(string $collection, string $id, Document $document, bool $skipPermissions): Document +public function updateDocument( + string $collection, + string $id, + Document $document, + bool $skipPermissions = false +): DocumentOtherwise, please confirm that every invocation site (including userland code extending the adapter) has been migrated.
1067-1074: Style: PSR-12 brace spacing triggers linter failureThe CI reports “braces_position style issue”.
Root cause is the missing space before the opening brace:if (!$skipPermissions){Apply the canonical spacing:
-if (!$skipPermissions){ +if (! $skipPermissions) {(Repeat for the closing
)/{pattern elsewhere if copy-pasted.)
[ suggest_nitpick ]
Co-authored-by: Jake Barnby <jakeb994@gmail.com>
…ptimize-update-document
Summary by CodeRabbit
New Features
Bug Fixes