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 sparse/partial document updates by capturing the caller's
Confidence Score: 4/5Safe to merge with one minor inefficiency: every partial-update call unconditionally issues a DB write even for genuine no-ops. The core partial-update mechanism is well-designed — $inputKeys capture, array_intersect_key scoping, and adapter-level conditional attribute handling all work correctly. The permissions null-ID fix is a real improvement. The one non-blocking concern is that the newly added empty($attributes) guard in all three SQL adapters is dead code: _updatedAt is always injected into $adapterDocument before the guard can fire, so even a completely empty partial update still executes a DB row write. No data is corrupted, but the intended skip-on-empty optimization never triggers. All three SQL adapters share the dead empty($attributes) guard; Database.php is the natural place to add the no-op early-return if the team wants to avoid redundant writes. Important Files Changed
Reviews (21): Last reviewed commit: "fix early return" | 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" |
1 participant
Summary by CodeRabbit
Bug Fixes
Improvements