feat: ignore argument for @updatedAt
#572
Conversation
📝 WalkthroughWalkthroughExtends the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
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/sdk/src/ts-schema-generator.ts:
- Around line 566-578: The updatedAt attribute handling uses unsafe casts for
ignoreArg.value and items; add explicit type guards to validate that
ignoreArg.value is an ArrayExpr and each item is a ReferenceExpr before mapping:
check with a helper isArrayExpr(ignoreArg.value) and isReferenceExpr(item), and
if validation fails either skip the ignore mapping or throw a clear error
mentioning the field name and attribute (e.g., "invalid @updatedAt(ignore) —
expected array of references"); then only call
ts.factory.createArrayLiteralExpression and use (item as
ReferenceExpr).target.$refText after those guards. Update the block that builds
the 'updatedAt' propertyAssignment to use these guards and produce a clear
error/log instead of letting runtime type assertions throw.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/cli/test/ts-schema-gen.test.tspackages/language/res/stdlib.zmodelpackages/orm/src/client/crud-types.tspackages/orm/src/client/crud/operations/base.tspackages/schema/src/schema.tspackages/sdk/src/ts-schema-generator.tstests/e2e/orm/client-api/update.test.tstests/e2e/orm/schemas/basic/schema.ts
🧰 Additional context used
📓 Path-based instructions (2)
tests/e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
E2E tests should validate real-world schema compatibility with established projects
Files:
tests/e2e/orm/client-api/update.test.tstests/e2e/orm/schemas/basic/schema.ts
**/*.zmodel
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.zmodel: Always runzenstack generateafter modifying ZModel schemas
ZModel schema files should define database structure and policies that compile to TypeScript viazenstack generate
Files:
packages/language/res/stdlib.zmodel
🧠 Learnings (5)
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests
Applied to files:
tests/e2e/orm/client-api/update.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
Applied to files:
tests/e2e/orm/client-api/update.test.tspackages/cli/test/ts-schema-gen.test.tstests/e2e/orm/schemas/basic/schema.ts
📚 Learning: 2025-12-30T15:07:06.254Z
Learnt from: mwillbanks
Repo: zenstackhq/zenstack-v3 PR: 550
File: packages/orm/src/client/crud/operations/base.ts:158-159
Timestamp: 2025-12-30T15:07:06.254Z
Learning: Do not use ts-expect-error in production code within the zenstackhq/zenstack-v3 repo (e.g., packages/*). Use explicit type annotations, targeted type assertions, or refactor to resolve the type error. ts-expect-error may be acceptable only in test files for stubbing or temporary silencing. Ensure production code is type-safe and maintainable.
Applied to files:
packages/schema/src/schema.tspackages/orm/src/client/crud/operations/base.tspackages/orm/src/client/crud-types.tspackages/sdk/src/ts-schema-generator.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization
Applied to files:
packages/orm/src/client/crud-types.ts
📚 Learning: 2025-10-21T16:09:31.218Z
Learnt from: ymc9
Repo: zenstackhq/zenstack-v3 PR: 319
File: packages/runtime/src/client/executor/zenstack-query-executor.ts:63-72
Timestamp: 2025-10-21T16:09:31.218Z
Learning: In ZenStack, TypeDefs can be inherited by models. When a TypeDef contains fields with `map` attributes, those mapped field names need to be processed by the QueryNameMapper since they become part of the inheriting model's schema. Therefore, when checking if a schema has mapped names (e.g., in `schemaHasMappedNames`), both `schema.models` and `schema.typeDefs` must be inspected for `@map` and `map` attributes.
Applied to files:
packages/orm/src/client/crud-types.ts
🧬 Code graph analysis (5)
tests/e2e/orm/client-api/update.test.ts (1)
tests/e2e/orm/client-api/utils.ts (1)
createUser(6-21)
packages/cli/test/ts-schema-gen.test.ts (4)
packages/testtools/src/schema.ts (1)
generateTsSchema(40-74)tests/e2e/orm/schemas/basic/schema.ts (1)
schema(292-292)packages/clients/tanstack-query/test/schemas/basic/schema-lite.ts (1)
schema(172-172)samples/next.js/zenstack/schema-lite.ts (1)
schema(106-106)
packages/orm/src/client/crud/operations/base.ts (2)
packages/orm/src/client/query-utils.ts (1)
isScalarField(134-137)packages/common-helpers/src/clone.ts (1)
clone(6-24)
packages/orm/src/client/crud-types.ts (1)
packages/schema/src/schema.ts (2)
GetModelField(156-160)UpdatedAtInfo(62-64)
packages/sdk/src/ts-schema-generator.ts (3)
packages/language/src/utils.ts (1)
getAttribute(59-61)packages/sdk/src/model-utils.ts (1)
getAttribute(66-68)packages/language/src/generated/ast.ts (6)
DataFieldAttribute(348-353)DataFieldAttribute(355-355)ArrayExpr(185-189)ArrayExpr(191-191)ReferenceExpr(688-693)ReferenceExpr(695-695)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test (22.x, postgresql)
- GitHub Check: build-test (22.x, sqlite)
🔇 Additional comments (9)
packages/schema/src/schema.ts (1)
62-64: LGTM! Clean type definition for the ignore feature.The new
UpdatedAtInfotype and the extension ofFieldDef.updatedAtto acceptboolean | UpdatedAtInfoare well-designed. The use ofreadonly string[]ensures immutability, and the conditional type update inFieldHasDefaultmaintains consistency across the type system.Also applies to: 73-73, 288-288
tests/e2e/orm/schemas/basic/schema.ts (1)
33-38: LGTM! Generated schema correctly reflects the ignore configuration.The generated
updatedAtfield structure with theignorearray matches theUpdatedAtInfotype definition. The inclusion of'id'and'createdAt'in the ignore list provides a practical example for E2E testing.tests/e2e/orm/client-api/update.test.ts (2)
133-167: LGTM! Comprehensive test coverage for ignored fields behavior.This test validates the core feature:
updatedAtshould not change when only ignored fields (id,createdAt) are modified. The test covers multiple scenarios including updating ignored fields individually and in combination, and properly verifies the timestamp remains unchanged by fetching the record after each update.
169-183: LGTM! Validates that non-ignored fields trigger updatedAt.This test ensures the feature doesn't break the default behavior: when any non-ignored field (like
name) is updated alongside ignored fields, theupdatedAttimestamp should still be updated. Good complementary coverage to the previous test.packages/language/res/stdlib.zmodel (1)
406-406: LGTM! Backward-compatible attribute signature update.The addition of the optional
ignore: FieldReference[]?parameter to@updatedAtis well-designed:
- Uses
FieldReference[]for type-safe field references- Optional parameter maintains backward compatibility with existing schemas
- Follows ZModel naming conventions with the underscore prefix
packages/cli/test/ts-schema-gen.test.ts (1)
446-726: LGTM! Comprehensive test coverage for the newignorefeature.The test validates that the generated schema correctly represents
@updatedAt(ignore: [email])with anUpdatedAtInfoobject containing the ignore array. The assertions are thorough and cover both the field structure and the attribute representation.packages/orm/src/client/crud-types.ts (2)
33-33: LGTM! Correct import for the new type.The
UpdatedAtInfoimport is necessary to support the updated type signature at line 984.
984-984: LGTM! Type widening correctly handles the newUpdatedAtInfostructure.The change from
extends truetoextends (true | UpdatedAtInfo)properly accounts for the new schema whereupdatedAtcan be either a boolean or an object with ignore configuration. This ensures that fields with@updatedAtremain optional during create operations regardless of the ignore configuration.packages/orm/src/client/crud/operations/base.ts (1)
997-1005: LGTM! Correct implementation of conditional timestamp updates.The logic correctly implements the
ignorefeature:
- Identifies which scalar fields are being updated
- Constructs an ignore set from the configuration
- Only updates the timestamp if at least one non-ignored scalar field is being updated
Edge cases are handled correctly:
- Only ignored fields updated → timestamp NOT updated ✓
- Only relation fields updated → timestamp NOT updated ✓
- Empty updates → timestamp NOT updated ✓
- Mix of ignored/non-ignored → timestamp updated ✓
|
@coderabbitai why is the test failing in CI when it passes locally? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1116 🏁 Script executed: Length of output: 285018 🏁 Script executed: Length of output: 32454 @sanny-io Thanks for the logs — the failures are coming from the CI Postgres setup, not from the updatedAt ignore logic. What’s different in CI vs local
Why it passes locally
How to fix Option A — Align connection strings to user=postgres
Option B — Create the root role in the CI Postgres container
Quick checks you can make
If you want, I can prep a small PR to:
Let me know which option you prefer and I’ll provide the exact patch. |
|
What an absolute adventure lol. This should be ready now... @ymc9 how are the e2e test schemas supposed to be handled? Am I supposed to run the |
1 participant
Allows the
@updatedAtattribute to accept an array of field references. An update which only contains ignored fields does not update the timestamp.Sample Usage
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.