fix(introspection): preserve data validation attributes, enum ordering, and comments during db pull#672
Conversation
- Retain data validation attributes (e.g., @email) on fields after introspection (#670) - Preserve original declaration order of enums instead of moving them to the end of the schema file (#669) - Preserve triple-slash comments above enum declarations (#669) Fixes #669, fixes #670
📝 WalkthroughWalkthroughAdds enum consolidation to pull flow, replaces hardcoded attribute exclusions with a new helper, preserves original declaration/comments in the code generator, extends Json/Bytes default-value handling across providers, adjusts CLI indent parsing, and updates tests and generator defaults. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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.
Pull request overview
This PR improves db pull introspection/merge behavior to better preserve original schema authoring intent (validation attributes, enum positioning, and enum comments) while also addressing MySQL enum-type fragmentation.
Changes:
- Stop re-sorting declarations during codegen so enums keep their original placement in the schema file.
- Preserve triple-slash comments for enum declarations (and enum fields) during generation.
- Add enum consolidation logic during pull (notably for MySQL per-column synthetic enums) and expand CLI/test coverage for these behaviors.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/language/src/zmodel-code-generator.ts | Removes enum-last sorting; emits enum-level and enum-field comments so order/comments are preserved on generation. |
| packages/cli/test/db/pull.test.ts | Adds regression tests for enum ordering, enum comments, and validation attributes; adjusts some existing enum expectations. |
| packages/cli/src/index.ts | Changes --indent option handling (arg parsing removed). |
| packages/cli/src/actions/pull/index.ts | Adds consolidateEnums to map MySQL synthetic per-column enums back to shared enums using old schema references. |
| packages/cli/src/actions/db.ts | Invokes enum consolidation during pull; changes attribute add/delete syncing and generator option defaults. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/actions/db.ts (1)
465-494:⚠️ Potential issue | 🟠 MajorTightened attribute filter may silently drop DB-side attribute changes like
@unique.The filter now only considers
@relationand@db.*for add/remove diffing. This correctly preserves user-defined validation attributes (@length, etc.) that the DB doesn't know about.However, DB-backed attributes like
@uniqueand@updatedAtalso fall outside this filter. If a column gains or loses a unique constraint in the DB, the change won't be reflected in the schema during a merge pull — the old@uniquewon't be removed, and a new one won't be added.Please verify this is the intended trade-off, or consider also including
@unique,@id, and@updatedAtin the filter.#!/bin/bash # Check how `@unique`, `@id`, `@updatedAt` are handled during the merge phase # to see if there's separate handling outside the attribute diff rg -n '@unique|@id|@updatedAt' packages/cli/src/actions/db.ts -C 5
🤖 Fix all issues with AI agents
In `@packages/cli/src/index.ts`:
- Line 173: Commander option for '--indent' currently lacks an arg parser so the
value becomes a string and violates PullOptions.indent: number; update the
Option creation for '--indent <number>' to provide an argParser that converts
the incoming string to a number (e.g., parseInt/Number) and validate it (ensure
it's an integer >= 0) before applying the .default(4) so PullOptions.indent is
always a proper number at runtime.
In `@packages/cli/test/db/pull.test.ts`:
- Around line 1265-1275: The test's expected substring spacing doesn't match the
formatter output; update the contains assertions against restoredSchema so the
model block uses four spaces before the id field's directive and one space
before the email field's directive (i.e., change the expected lines in the User
model to "id Int `@id` `@default`(autoincrement())" and "email String
`@unique`") so the restoredSchema.contains checks match the zmodel-formatter.ts
output.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/cli/src/actions/pull/utils.ts`:
- Around line 31-33: The isDatabaseManagedAttribute function currently returns
['@relation','@id','@unique'] and checks name.startsWith('@db.'), but misses
'@updatedAt' so db pull/introspection-generated `@updatedAt` attributes are
filtered out; update the whitelist inside isDatabaseManagedAttribute to include
'@updatedAt' (i.e., ensure the returned array contains '@updatedAt') so
attribute diffing in db.ts will treat `@updatedAt` as database-managed and sync
correctly.
In `@packages/cli/test/db/pull.test.ts`:
- Around line 1265-1275: The zero-restoration test assumes enum consolidation
from an old schema but for MySQL the synthetic enum name (e.g., UserStatus) is
produced when there is no old schema; update the test in pull.test.ts (the
assertions that inspect restoredSchema) to handle MySQL: either seed a non-empty
old schema so index.ts's enum consolidation logic can map synthetic enums back
to shared names, or change the MySQL branch of the assertions to expect the
synthetic enum name (e.g., expect(restoredSchema).contains("enum UserStatus {"))
while keeping the existing expectation for PostgreSQL/native enums; detect the
current connector (MySQL vs Postgres) in the test and assert the appropriate
enum name accordingly.
Prevents field name collisions during introspection by refining the naming strategy for self-referencing relations with multiple foreign keys. Extends support for JSON and Bytes default values across MySQL, PostgreSQL, and SQLite providers to ensure consistent schema restoration. Adds test cases for self-referencing models to verify the avoidance of duplicate fields.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/cli/src/actions/pull/index.ts`:
- Around line 638-647: consolidateEnums currently renames keepEnum to oldEnum
but leaves the synthetic @@map attribute added by syncEnums, so
getDbName(keepEnum) still returns the old mapped name and prevents matching;
after the line keepEnum.name = oldEnum.name (inside consolidateEnums) remove any
@@map attribute from keepEnum (or alternatively copy attributes from oldEnum
onto keepEnum) so that getDbName(keepEnum) reflects the renamed enum; reference
the functions/symbols consolidateEnums, keepEnum, oldEnum, syncEnums and
getDbName when making the change.
🧹 Nitpick comments (2)
packages/cli/src/actions/pull/index.ts (2)
650-661: Castingfield.typetoanyto assignreference— consider a narrower assertion.The
(field.type as any).reference = { ... }pattern works but bypasses type safety on the AST node. A narrower cast such as(field.type as { reference: ... }).reference = ...would preserve partial type checking.This is a minor concern given the AST mutation context, but worth noting per repo conventions around avoiding untyped casts in production code. Based on learnings, production code in
packages/*should prefer explicit type annotations or targeted type assertions over broadanycasts.
576-611: Enum mapping relies on the first field match — may miss mappings if the first matched field's old counterpart changed type.The inner loop breaks after finding the first field that references
newEnum(line 607). If that field's old counterpart no longer references an enum (e.g., it was changed toString), the mapping is lost even though other fields may still reference the shared enum.In practice this is unlikely for the consolidation use case (MySQL synthetic enums), but a
continueinstead ofbreakat line 607 (falling through to check more fields) would make it more robust.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/cli/src/actions/pull/index.ts`:
- Around line 567-674: The code assigns oldEnum.attributes directly to
keepEnum.attributes which leaves attribute AST nodes with their $container still
pointing at oldEnum; in consolidateEnums, after preparing keepEnum and before
updating references, replace keepEnum.attributes with a shallow copy of
oldEnum.attributes and re-parent each attribute node by setting its $container
(and $parent if used elsewhere) to keepEnum so the attributes' parent pointers
are correct; operate on the symbols keepEnum and oldEnum within consolidateEnums
and ensure you mutate the attribute objects' container references rather than
leaving them pointing to oldEnum.
🧹 Nitpick comments (1)
packages/cli/src/actions/pull/index.ts (1)
654-666:as anycast to mutatereference— consider a narrower type assertion.Line 660 uses
(field.type as any).referenceto rewrite the enum reference. While functional, this bypasses type safety entirely. A narrower assertion (e.g., casting to a type with a writablereferenceproperty) would be preferable to avoid accidentally setting the wrong shape.
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
2 participants
introspection (#670)
to the end of the schema file (#669)
Fixes zenstackhq/zenstack#2342, fixes zenstackhq/zenstack#2341
Summary by CodeRabbit
New Features
Improvements
Tests