Partial update#886
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:
📝 WalkthroughWalkthroughDatabase::updateDocument sanitizes in-flight updates (removes internals, optional $createdAt, tenant normalization), validates with PartialStructure against persisted document, restores persisted $sequence before adapter call, and merges persisted data with adapter-updated attributes. MariaDB/Postgres/SQLite adapters now conditionally map internal fields, adjust permission id bindings, and trim UPDATE SET/bindings. ChangesDocument update sanitization and adapter metadata updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
Greptile SummaryThis PR implements partial document updates across all adapters, allowing callers to pass a sparse
Confidence Score: 3/5Not safe to merge in its current state — debug output introduced in this diff is still present in the MariaDB adapter and will write the full UPDATE SQL to stdout on every document update in production. The partial-update logic in Database.php and the adapter changes are structurally correct, but MariaDB.php still contains a var_dump($sql) call added in this PR that leaks query internals to PHP output on every single document update. In addition, stopOnFailure was flipped to true in phpunit.xml, which halts the test suite on the first failure and can mask the true pass/fail status of the full suite. Both issues were raised in prior review rounds and remain unresolved in the current head commit. src/Database/Adapter/MariaDB.php (var_dump debug statement at line 1090) and phpunit.xml (stopOnFailure configuration) need attention before merging. Important Files Changed
Reviews (27): Last reviewed commit: "formatting" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Claude could not apply patches from: healing (merge conflicts with current branch tip). These tasks need manual attention. |
…pters The partial-update change in Database.php only sets internal attributes (`$createdAt`, `$updatedAt`, `$permissions`, `$id`) on the Document when the user explicitly provided them. MariaDB's updateDocument was updated to honor that (offsetExists checks before writing `_createdAt`, `_uid`, `_permissions`), but SQLite and Postgres still unconditionally wrote those columns — using `$document->getCreatedAt()`/`getPermissions()` (returning null/`"[]"`) and `_uid = :_newUid` bound to `$document->getId()` (empty string for partial updates without `$id`). SQLite especially blew up because the row's `_uid` got rewritten to an empty string, so subsequent `getDocument` calls returned null and any operator-driven update appeared to lose its result. Also makes `_updatedAt` conditional in MariaDB so partial updates with `shouldUpdate=false` (e.g. relationship-only updates) don't overwrite the parent row's `_updatedAt` with NULL. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Claude pushed fixes from: healing |
|
Claude could not apply patches from: healing (merge conflicts with current branch tip). These tasks need manual attention. |
|
Claude could not apply patches from: healing (merge conflicts with current branch tip). These tasks need manual attention. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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)
980-991:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRoot cause: Conditional internal metadata mapping can leave SET clause empty.
Both adapters now conditionally add
_updatedAt,_uid,_createdAt, and_permissionsonly when the corresponding offsets exist. When the Database layer performs a permissions-only update ($shouldUpdate=false), none of these offsets may be present, and if no user attributes exist, the SET clause becomes empty.Consider coordinating the fix across both adapters or addressing this at the Database layer by ensuring at least one column is always present when calling the adapter.
🤖 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 980 - 991, The adapter builds $attributes conditionally from $document->offsetExists checks which can produce an empty SET clause for permissions-only updates; update src/Database/Adapter/MariaDB.php so the update builder always supplies at least one column when composing $attributes — for example, ensure '_updatedAt' is always added (use $document->getUpdatedAt() if available or a fallback like current timestamp) or add a dedicated internal no-op field key (e.g., '_internal_update_flag') when none of '_updatedAt','_uid','_createdAt','_permissions' were added; keep the checks for existing offsets but guarantee $attributes is non-empty before building the SET, and mirror the same change in the other adapter to keep behavior consistent.
🤖 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.
Outside diff comments:
In `@src/Database/Adapter/MariaDB.php`:
- Around line 980-991: The adapter builds $attributes conditionally from
$document->offsetExists checks which can produce an empty SET clause for
permissions-only updates; update src/Database/Adapter/MariaDB.php so the update
builder always supplies at least one column when composing $attributes — for
example, ensure '_updatedAt' is always added (use $document->getUpdatedAt() if
available or a fallback like current timestamp) or add a dedicated internal
no-op field key (e.g., '_internal_update_flag') when none of
'_updatedAt','_uid','_createdAt','_permissions' were added; keep the checks for
existing offsets but guarantee $attributes is non-empty before building the SET,
and mirror the same change in the other adapter to keep behavior consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0cd5a620-5c8e-445d-bfe2-b0692881fd19
📒 Files selected for processing (4)
src/Database/Adapter/MariaDB.phpsrc/Database/Adapter/Postgres.phpsrc/Database/Adapter/SQLite.phpsrc/Database/Database.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Database/Database.php
|
Claude could not apply patches from: healing (merge conflicts with current branch tip). These tasks need manual attention. |
| convertWarningsToExceptions="true" | ||
| processIsolation="false" | ||
| stopOnFailure="false" | ||
| stopOnFailure="true" |
There was a problem hiding this comment.
stopOnFailure was flipped to true, which halts the entire suite on the first failure. Combined with the always-failing assertion in testPartialUpdateDocument, no tests after that point will execute, hiding the real pass/fail status of the rest of the suite. This should be reverted to false (the project's established default) before merging.
| stopOnFailure="true" | |
| stopOnFailure="false" |
…date # Conflicts: # src/Database/Adapter/MariaDB.php # src/Database/Adapter/Postgres.php # src/Database/Adapter/SQLite.php # src/Database/Database.php
1 participant
Summary by CodeRabbit
Bug Fixes
Improvements