fix(core): apply beforeUpsert value changes to query payload#18219
fix(core): apply beforeUpsert value changes to query payload#18219mccabemj wants to merge 6 commits into
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:
📝 WalkthroughWalkthroughModel.upsert now invokes the beforeUpsert hook earlier: it snapshots pre-hook values, deep-clones them when hooks are enabled, runs beforeUpsert, computes changed keys by deep equality, applies only those changed keys to the instance, and recalculates primary-key/fields handling. A later duplicate beforeUpsert call was removed. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Model
participant Hook as beforeUpsert
participant Instance
participant DB
Caller->>Model: upsert(values, options)
Model->>Instance: build(instanceValues)
Model->>Hook: beforeUpsert(values, options)
Hook-->>Model: mutated values
Model->>Instance: set(only changed keys from mutated values)
Model->>Model: compute insertValues / updateValues / options.fields / hasPrimary
Model->>DB: execute upsert (insert or update) with computed payload
DB-->>Model: result
Model-->>Caller: return result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/model.js`:
- Around line 2191-2197: The current diffing uses pickBy over values so keys
removed by a beforeUpsert hook are ignored; update the logic that builds
hookChangedValues (replace the pickBy usage) to iterate the union of
Object.keys(values) and Object.keys(beforeHookValues) and include any key where
!isEqual(values[key], beforeHookValues[key]); for keys absent from values, set
hookChangedValues[key] to undefined (or the appropriate "deleted" sentinel) so
instance.set(hookChangedValues) will remove stale properties; modify the code
around hookChangedValues, pickBy, beforeHookValues, and instance.set to
implement this behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b7a3fc04-dd17-49e4-ad05-c1ab7b861388
📒 Files selected for processing (3)
packages/core/src/model.jspackages/core/test/integration/hooks/upsert.test.jspackages/core/test/unit/model/upsert.test.js
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/test/unit/model/upsert.test.js (1)
154-171: Consider clarifying the test title for readability.The test title "uses beforeUpsert deleted values in insert and update values" is slightly ambiguous—it could be read as if deleted values are being used. Consider a clearer phrasing such as:
- it('uses beforeUpsert deleted values in insert and update values', async function () { + it('excludes values deleted by beforeUpsert from insert and update payloads', async function () {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/test/unit/model/upsert.test.js` around lines 154 - 171, The test title is ambiguous; update the "it" description for the test that exercises the beforeUpsert hook to make intent clearer (e.g., indicate that deleted properties are removed from insert/update values). Edit the string passed to the it(...) for the test containing the beforeUpsert hook (the test that calls this.User.hooks.addListener('beforeUpsert', ...) and then awaits this.User.upsert(...)) so it reads something like "removes properties deleted in beforeUpsert from insert and update values" to improve readability while leaving the hook logic and assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/test/unit/model/upsert.test.js`:
- Around line 154-171: The test title is ambiguous; update the "it" description
for the test that exercises the beforeUpsert hook to make intent clearer (e.g.,
indicate that deleted properties are removed from insert/update values). Edit
the string passed to the it(...) for the test containing the beforeUpsert hook
(the test that calls this.User.hooks.addListener('beforeUpsert', ...) and then
awaits this.User.upsert(...)) so it reads something like "removes properties
deleted in beforeUpsert from insert and update values" to improve readability
while leaving the hook logic and assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 13f2284c-ff15-4e3d-a04e-19b2325bdb98
📒 Files selected for processing (2)
packages/core/src/model.jspackages/core/test/unit/model/upsert.test.js
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/model.js`:
- Around line 2202-2203: The code currently computes hasPrimary from the
original values while recomputing changed from the finalized instance after
beforeUpsert hooks, causing a mismatch when hooks or options.instance mutate the
PK; update the logic that sets hasPrimary to derive it from the post-hook
instance (the same source used to recompute changed) instead of from values so
the PK-pruning branch uses the same finalized instance state (look for symbols:
beforeUpsert, options.instance, instance.set, hookChangedValues, hasPrimary,
values and the PK-pruning code path) and ensure any subsequent checks/branches
reference this recomputed hasPrimary.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c336b35c-8631-4893-b4bd-23707aa876db
📒 Files selected for processing (2)
packages/core/src/model.jspackages/core/test/integration/hooks/upsert.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/test/integration/hooks/upsert.test.js
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)
packages/core/src/model.js (1)
2178-2210:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate after applying
beforeUpsertmutations.Line 2178 validates the pre-hook instance, but Lines 2189-2210 now change the persisted state afterward. That means
beforeUpsertcan introduce an invalid value that still gets written throughinsertValues/updateValues, which is a new validation bypass in this flow.Suggested fix
- if (options.validate) { - await instance.validate(options); - } - // Map conflict fields to column names if (options.conflictFields) { options.conflictFields = options.conflictFields.map(attrName => { return modelDefinition.getColumnName(attrName); }); @@ if (!fieldsSpecified) { options.fields = changed; } } + + if (options.validate) { + await instance.validate(options); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/model.js` around lines 2178 - 2210, The pre-hook can mutate instance after the initial validate, so re-run validation when options.validate is truthy after running the beforeUpsert hook and applying mutations: after this.hooks.runAsync('beforeUpsert', values, options) and after instance.set(hookChangedValues) call await instance.validate(options) (or only when options.validate was set), then recalc hasPrimary/changed and set options.fields as currently done; ensure you reference instance.validate, this.hooks.runAsync('beforeUpsert'), instance.set(hookChangedValues), hasPrimary and changed so the second validation applies to the mutated state before insertValues/updateValues proceed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/test/unit/model/upsert.test.js`:
- Around line 163-179: The test currently creates a nested sinon stub for
current.queryInterface.upsert which conflicts with the existing stub from
beforeEach; instead, reuse and reconfigure the existing stub (e.g., this.stub or
the shared stub used in beforeEach) to resolve with [Item.build(), true] for the
Item.upsert scenario, remove the local stub.restore() call (afterEach will clean
up), keep the unhook removal via Item.hooks.addListener/unhook(), and when
asserting read the args from the reused stub (this.stub.getCall(0).args) to
verify insertValues.id === 42.
---
Outside diff comments:
In `@packages/core/src/model.js`:
- Around line 2178-2210: The pre-hook can mutate instance after the initial
validate, so re-run validation when options.validate is truthy after running the
beforeUpsert hook and applying mutations: after
this.hooks.runAsync('beforeUpsert', values, options) and after
instance.set(hookChangedValues) call await instance.validate(options) (or only
when options.validate was set), then recalc hasPrimary/changed and set
options.fields as currently done; ensure you reference instance.validate,
this.hooks.runAsync('beforeUpsert'), instance.set(hookChangedValues), hasPrimary
and changed so the second validation applies to the mutated state before
insertValues/updateValues proceed.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6cb37645-27ed-484f-b3fa-ca20c360c388
📒 Files selected for processing (2)
packages/core/src/model.jspackages/core/test/unit/model/upsert.test.js
ephys
left a comment
There was a problem hiding this comment.
Thank you for your PR
The problem with changing hooks is that it's very easy to break existing workflows that depend on the current behavior
I've highlighted places where regressions are likely
| options.fields = changed; | ||
| } | ||
|
|
||
| if (options.validate) { |
There was a problem hiding this comment.
this is a breaking change as beforeUpsert can now be called with invalid data
This validation should be restored. If data changes during beforeUpsert, validation should be re-run after the hook
Don't forget to add a unit test for this :)
| instance._changed.has(this.primaryKeyField); | ||
| changed = [...instance._changed]; | ||
| if (!fieldsSpecified) { | ||
| options.fields = changed; |
There was a problem hiding this comment.
this will override the changes to the fields property that might have happened during beforeUpsert
Maybe users should have to update it themselves, this could be documented in the documentation of beforeUpsert on the website
A test should be added to cover this regression
| } | ||
|
|
||
| if (options.hooks) { | ||
| const beforeHookValues = cloneDeep(values); |
There was a problem hiding this comment.
there is no need to clone deep, a shallow clone is enough
| const beforeHookValues = cloneDeep(values); | ||
| await this.hooks.runAsync('beforeUpsert', values, options); | ||
|
|
||
| const hookChangedValues = {}; |
There was a problem hiding this comment.
| const hookChangedValues = {}; | |
| const hookChangedValues = pojo(); |
|
|
||
| const hookChangedValues = {}; | ||
| for (const key of union(Object.keys(beforeHookValues), Object.keys(values))) { | ||
| if (!isEqual(values[key], beforeHookValues[key])) { |
There was a problem hiding this comment.
a deep comparison is too expensive, it should be a shallow comparison
Our philosophy is that users should not mutate nested data, but should immutably replace it. We've documented it here for save, this can be documented as well in the documentation of beforeUpsert
|
|
||
| instance.set(hookChangedValues); | ||
|
|
||
| hasPrimary = |
There was a problem hiding this comment.
hasPrimary is not used before it gets re-computed. It should be computed once after the hook instead
Pull Request Checklist
Description of Changes
Fixes #8829.
Model.upsertranbeforeUpsertafterinsertValuesandupdateValueshad already been derived from the built instance. As a result, mutations made bybeforeUpsertwere visible on the original values object but were not sent to the database.This changes
upsertto runbeforeUpsertbefore mapping the insert/update payloads, sync hook-mutated values back into the built instance, and preserve explicitfieldsbehavior so hook changes do not widen the update payload when fields are user-specified.List of Breaking Changes
None
Summary by CodeRabbit
Bug Fixes
Tests