feat: fields argument for @updatedAt#2379
Conversation
📝 WalkthroughWalkthroughAdds a Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/orm/src/client/crud/operations/base.ts`:
- Around line 1151-1175: The updatedAt logic (around fieldDef.updatedAt /
hasUpdated) currently only checks keys on the incoming data using
isScalarField/isForeignKeyField so nested relation operations that mutate owned
FKs later via parentUpdates (e.g., author: { connect/create/update } which apply
FK changes through parentUpdates) are ignored; update the hasUpdated
determination to also inspect parentUpdates for relation entries that will
produce FK changes (look at parentUpdates collection and treat relation ops that
result in owned FK writes as equivalent to scalar FK presence), keeping the
existing ignore/fields semantics (respect fieldDef.updatedAt.ignore and
fieldDef.updatedAt.fields) and using the same isScalarField/isForeignKeyField
logic when mapping relation->affected FK names so updatedAt is bumped correctly.
🧹 Nitpick comments (4)
tests/e2e/orm/client-api/updated-at.test.ts (4)
6-19:anyFieldUpdatedAtis declared in both schemas but never asserted on.Both schemas define
anyFieldUpdatedAt DateTime@updatedAt`` but no test case checks its behavior. This field should advance on every update, making it a useful baseline assertion — especially in the negative tests (lines 73–127 and 200–247) where it should still advance even when targeted/ignoredupdatedAtfields don't.Consider adding assertions like:
+ expect(updatedUser1.anyFieldUpdatedAt.getTime()).toBeGreaterThan(user.anyFieldUpdatedAt.getTime());in the negative-case tests to confirm that a plain
@updatedAtstill fires while the field-scoped ones correctly don't.Also applies to: 131-144
22-71: Missing negative assertion:emailUpdatedAtshould NOT advance when onlynameis updated.The first update (line 32–40) changes only
name, soemailUpdatedAtshould remain unchanged. Adding this assertion would validate thatfields: [email]correctly excludes unrelated changes — which is the core value proposition of this feature.Proposed addition after line 49
expect(updatedUser1.nameUpdatedAt.getTime()).toBeGreaterThan(nameUpdatedAt.getTime()); expect(updatedUser1.majorFieldUpdatedAt.getTime()).toBeGreaterThan(majorFieldUpdatedAt.getTime()); + expect(updatedUser1.emailUpdatedAt.getTime()).toEqual(user.emailUpdatedAt.getTime());
200-247: Missing complementary assertion:exceptEmailUpdatedAtshould advance when onlynameis updated.In the first update (lines 210–218), only
nameis changed.exceptEmailUpdatedAt(which ignores[email]) should still advance becausenameis not in its ignore list. This would strengthen the test by confirming that non-ignoredupdatedAtfields correctly fire when their counterpart stays suppressed.Proposed addition
+ const exceptEmailUpdatedAt = user.exceptEmailUpdatedAt; ... + expect(updatedUser1.exceptEmailUpdatedAt.getTime()).toBeGreaterThan(exceptEmailUpdatedAt.getTime());
48-49: Potential flakiness from same-millisecond timestamps.All assertions comparing timestamps use
toBeGreaterThan(previousTimestamp.getTime()). If the DB operations complete within the same millisecond (unlikely but possible under fast CI), these will fail intermittently. Consider adding a small delay (e.g.,await new Promise(r => setTimeout(r, 10))) before the update calls if flakiness is observed, or switching totoBeGreaterThanOrEqualwith an additional not-equal check.Also applies to: 68-70, 101-104, 174-176, 195-197
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/orm/src/client/crud/operations/base.ts (1)
1164-1180:⚠️ Potential issue | 🟠 MajorHandle relation-driven FK writes when computing
hasUpdated.This branch only inspects direct scalar/FK keys on
data. Nested relation ops likeauthor: { connect: ... }can still update owned FK fields later viaprocessRelationUpdates()/parentUpdates, so@updatedAt(ignore: ...)and@updatedAt(fields: ...)can miss real FK changes and skip the timestamp bump.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aab2e7e9-9f87-4de9-96c8-73ad41cdee78
📒 Files selected for processing (5)
packages/orm/src/client/crud/operations/base.tspackages/schema/src/schema.tspackages/sdk/src/ts-schema-generator.tstests/e2e/orm/schemas/basic/schema.tstests/e2e/orm/schemas/basic/schema.zmodel
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/orm/src/client/crud/operations/base.ts`:
- Around line 1166-1179: The current updatedAt detection treats any owned
relation payload as if it updates the parent FK because
getUpdatedAtEffectiveFields is called for every key in data; change the logic in
the hasNonIgnoredFields/hasAnyTargetFields branches so you only expand relation
keys into effective fields when the relation operation will actually produce
parentUpdates/owned-FK writes (i.e., detect operation types like
connect/disconnect/set/upsert/create that mutate the FK or move this entire
updatedAt decision until after relation processing completes), update the code
paths around getUpdatedAtEffectiveFields, fieldDef.updatedAt.fields and the
hasUpdated assignment accordingly, and add a regression test for nested relation
update and the upsert path to ensure nested update payloads (e.g., data: { user:
{ update: { ... } } }) do not falsely trigger FK-based updatedAt logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f053dcb5-888b-4f81-bd31-80a08a736bab
📒 Files selected for processing (2)
packages/orm/src/client/crud/operations/base.tstests/e2e/orm/client-api/updated-at.test.ts
| const hasNonIgnoredFields = Object.keys(data).some((field) => { | ||
| const effectiveFields = this.getUpdatedAtEffectiveFields(modelDef.name, field); | ||
| return ( | ||
| effectiveFields.length > 0 && | ||
| !effectiveFields.some((f) => ignoredFields.has(f)) | ||
| ); | ||
| }); | ||
| hasUpdated = hasNonIgnoredFields; | ||
| } else if (fieldDef.updatedAt.fields) { | ||
| const targetFields = new Set(fieldDef.updatedAt.fields); | ||
| const hasAnyTargetFields = Object.keys(data).some((field) => { | ||
| const effectiveFields = this.getUpdatedAtEffectiveFields(modelDef.name, field); | ||
| return effectiveFields.some((f) => targetFields.has(f)); | ||
| }); |
There was a problem hiding this comment.
Don't treat every owned relation payload as an FK change.
Line 1540 expands any owned relation key to [relation, ...relation.fields], so Line 1176 will also match @updatedAt(fields: [userId]) for a payload like data: { title: 'x', user: { update: { name: 'y' } } }. That only mutates the related row, but the title write gives the parent row a real update so the timestamp bump gets persisted anyway. Please derive the effective fields from operations that actually produce parentUpdates/owned-FK writes, or move this decision after relation processing. A regression test for nested relation update and the matched-upsert path would lock this down.
Also applies to: 1540-1554
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/orm/src/client/crud/operations/base.ts` around lines 1166 - 1179,
The current updatedAt detection treats any owned relation payload as if it
updates the parent FK because getUpdatedAtEffectiveFields is called for every
key in data; change the logic in the hasNonIgnoredFields/hasAnyTargetFields
branches so you only expand relation keys into effective fields when the
relation operation will actually produce parentUpdates/owned-FK writes (i.e.,
detect operation types like connect/disconnect/set/upsert/create that mutate the
FK or move this entire updatedAt decision until after relation processing
completes), update the code paths around getUpdatedAtEffectiveFields,
fieldDef.updatedAt.fields and the hasUpdated assignment accordingly, and add a
regression test for nested relation update and the upsert path to ensure nested
update payloads (e.g., data: { user: { update: { ... } } }) do not falsely
trigger FK-based updatedAt logic.
2 participants
Sample Usage
Mutually exclusive with the
ignoreargument, with an error in ZModel if you try to use both.Prisma issues: prisma/prisma#7602 prisma/prisma#12164
Closes #2354
Summary by CodeRabbit
New Features
@updatedAtnow supports afieldsparameter to specify which fields trigger timestamp updates.Bug Fixes
fieldsandignore.Tests
fieldsandignorebehaviors; adjusted related tests.Documentation
fieldsvsignoresemantics.