Bug update document uid with delete#891
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesPermission row migration on document $id change
Sequence DiagramsequenceDiagram
participant Caller
participant Database_updateDocument
participant MariaDB_updateDocument
participant _perms_table
Caller->>Database_updateDocument: updateDocument($id, $document)
Database_updateDocument->>Database_updateDocument: $id differs? → skipPermissionsUpdate = false
Database_updateDocument->>MariaDB_updateDocument: updateDocument(collection, document, skipPermissions=false)
MariaDB_updateDocument->>MariaDB_updateDocument: set _uid in attributes from document→getId()
MariaDB_updateDocument->>_perms_table: DELETE WHERE _uid = oldUID
MariaDB_updateDocument->>_perms_table: INSERT full permission set for newUID
MariaDB_updateDocument->>MariaDB_updateDocument: UPDATE document row (SET clause from $columns, no :_newUid bind)
MariaDB_updateDocument-->>Caller: updated document
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/Database/Adapter/MariaDB.php (1)
1020-1020: 💤 Low valueConsider using
array_keys()to avoid unused variable warning.The
$_variable is intentionally unused since only the index is needed for placeholder generation, but this triggers static analysis warnings. Usingarray_keys()would be more explicit.♻️ Suggested refactor
- foreach ($document->getPermissionsByType($type) as $i => $_) { + foreach (array_keys($document->getPermissionsByType($type)) as $i) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Database/Adapter/MariaDB.php` at line 1020, The foreach loop iterating over getPermissionsByType($type) uses an unused placeholder variable $_ which triggers static analysis warnings. Replace this loop to use array_keys($document->getPermissionsByType($type)) instead, which will iterate only over the keys/indices without requiring an unused value variable. This makes the intent clearer and eliminates the static analysis warning while keeping the same logic for placeholder generation.Source: Linters/SAST tools
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
1668-1677: ⚡ Quick winEnsure cleanup always runs (even when assertions fail).
Line 1673–Line 1677 cleanup is success-path only. If any assertion above fails, roles/collection can leak and make later tests flaky. Please wrap the test body in
try/finallyand move role removal + collection deletion tofinally.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/Adapter/Scopes/DocumentTests.php` around lines 1668 - 1677, The cleanup code that removes roles and deletes the collection (the removeRole calls for alice and bob, and the deleteCollection call) is only executed on the success path. Wrap the entire test body in a try/finally block and move all cleanup operations (both removeRole invocations and the deleteCollection call within the skip callback) to the finally block to ensure they always execute regardless of whether any assertion fails, preventing test pollution and flakiness.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/Database/Adapter/MariaDB.php`:
- Line 1020: The foreach loop iterating over getPermissionsByType($type) uses an
unused placeholder variable $_ which triggers static analysis warnings. Replace
this loop to use array_keys($document->getPermissionsByType($type)) instead,
which will iterate only over the keys/indices without requiring an unused value
variable. This makes the intent clearer and eliminates the static analysis
warning while keeping the same logic for placeholder generation.
In `@tests/e2e/Adapter/Scopes/DocumentTests.php`:
- Around line 1668-1677: The cleanup code that removes roles and deletes the
collection (the removeRole calls for alice and bob, and the deleteCollection
call) is only executed on the success path. Wrap the entire test body in a
try/finally block and move all cleanup operations (both removeRole invocations
and the deleteCollection call within the skip callback) to the finally block to
ensure they always execute regardless of whether any assertion fails, preventing
test pollution and flakiness.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a9e7d0d2-b1db-4ba1-82f2-5116dec5df7c
📒 Files selected for processing (3)
src/Database/Adapter/MariaDB.phpsrc/Database/Database.phptests/e2e/Adapter/Scopes/DocumentTests.php
Greptile SummaryThis PR fixes a longstanding bug where changing a document's
Confidence Score: 5/5Safe to merge — the fix correctly migrates permission rows when a document's UID changes, all three adapters are consistently updated, and the new E2E test directly exercises both the ID-only and combined ID+permissions scenarios. The delete-all + insert-all permission strategy is logically equivalent to the old diff-based approach for end state, the UNIQUE constraint on No files require special attention; all three adapters follow an identical pattern and the logic is straightforward to audit. Important Files Changed
Reviews (11): Last reviewed commit: "Sequence is immutable" | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Database/Adapter/MariaDB.php (1)
979-990:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the same
$newUidfallback for the main row_uid.Line 989 preserves the old-id fallback for permission rows, but Line 983 writes
_uidfrom$document->getId()unconditionally. If the update document does not carry$id, the document row and_permsrows can be written under different UIDs.Proposed fix
$attributes = $document->getAttributes(); $attributes['_createdAt'] = $document->getCreatedAt(); $attributes['_updatedAt'] = $document->getUpdatedAt(); $attributes['_permissions'] = json_encode($document->getPermissions()); - $attributes['_uid'] = $document->getId(); + $newUid = $document->offsetExists('$id') ? $document->getId() : $id; + $attributes['_uid'] = $newUid; $name = $this->filter($collection); $columns = ''; if (!$skipPermissions) { - $newUid = $document->offsetExists('$id') ? $document->getId() : $id; - $sql = "🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Database/Adapter/MariaDB.php` around lines 979 - 990, The _uid attribute assigned to the main document row on line 983 unconditionally uses $document->getId(), while the $newUid fallback for permission rows on lines 989-990 conditionally falls back to the original $id if the update document lacks an $id offset. This creates an inconsistency where the main row and permission rows can be written under different UIDs. Modify line 983 to use the same conditional fallback logic as $newUid, checking if the document has an $id offset before assigning the _uid attribute, so both the main row and permission rows always use the same UID.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Database/Adapter/MariaDB.php`:
- Around line 1008-1011: The permission value being bound in the foreach loop
over getPermissionsByType() is not being normalized before assignment to the
binds array. Apply the same normalization that createDocument() applies to
permission values (stripping double-quote characters) to the $permission
variable before binding it to $binds[":_add_{$type}_{$i}"] so that updated
permission rows match those created by the adapter and authorization lookups
function correctly.
---
Outside diff comments:
In `@src/Database/Adapter/MariaDB.php`:
- Around line 979-990: The _uid attribute assigned to the main document row on
line 983 unconditionally uses $document->getId(), while the $newUid fallback for
permission rows on lines 989-990 conditionally falls back to the original $id if
the update document lacks an $id offset. This creates an inconsistency where the
main row and permission rows can be written under different UIDs. Modify line
983 to use the same conditional fallback logic as $newUid, checking if the
document has an $id offset before assigning the _uid attribute, so both the main
row and permission rows always use the same UID.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6895b94a-6a0a-44c8-b6cc-2bce73e80549
📒 Files selected for processing (1)
src/Database/Adapter/MariaDB.php
2 participants
Summary by CodeRabbit