feat: db pull implementation #268
Conversation
📝 WalkthroughWalkthroughAdds a DB introspection ("db pull") flow to the CLI with provider-specific introspectors (MySQL/Postgres/SQLite), mapping utilities, a pull workflow, AST factory builders, document/service-loading enhancements, package export/build updates, and extensive tests for pull/push flows. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
svetch
force-pushed
the
main
branch
2 times, most recently
from
85bbb6e to
08006cf
Compare
|
Just a heads up, I'm renaming "@zenstackhq/runtime" package to "@zenstackhq/orm". |
svetch
force-pushed
the
main
branch
8 times, most recently
from
a460646 to
0bae20d
Compare
svetch
force-pushed
the
main
branch
2 times, most recently
from
5db51e6 to
68708b4
Compare
svetch
force-pushed
the
main
branch
8 times, most recently
from
c5ed543 to
3482534
Compare
Removes hardcoded file system path debugging and unnecessary console logging from the introspector and test suites. Silences CLI command output during tests to provide a cleaner test execution environment.
Wraps JSON_ARRAYAGG calls in subqueries with explicit ORDER BY clauses to maintain correct metadata ordering. This addresses a limitation in MySQL versions prior to 8.0.21, where ORDER BY is not supported directly within the JSON_ARRAYAGG function, ensuring consistent introspection results across different database versions.
Ensures database columns are sorted by their ordinal position during the introspection process. This maintains the original schema structure and provides a consistent output that matches the physical database layout.
Eliminates redundant schema and database name fields from the MySQL introspection query. Since MySQL does not support multi-schema architectures internal to a single connection in this context, removing these fields simplifies the data structure and avoids unnecessary metadata overhead.
Refines the database pull process to better handle MySQL-specific patterns. Improves unique constraint detection to prevent redundant mapping attributes when default naming conventions are used. Updates the MySQL introspection logic to correctly identify boolean types, handle timestamp precision in default values, and normalize numeric defaults. Also ensures auto-incrementing columns and primary key indexes are correctly mapped to prevent schema duplication.
Ensures that the default schema prelude correctly reflects the database provider specified in test options. This prevents inconsistencies when generating test projects with non-default providers.
Disables NativeEnum support for MySQL to prevent loss of schema-level enums since MySQL enums are column-specific. Refines boolean and numeric type mapping to better handle synthetic boolean types and preserve decimal precision in default values. Updates default value parsing logic to correctly identify unquoted strings and avoid misinterpreting numeric literals as booleans.
Refines how default values are handled during database introspection for MySQL by considering the specific field type. This ensures that boolean variants and numeric literals for Float and Decimal types are correctly formatted and preserved. Also clarifies unsupported features in the SQLite provider to improve codebase maintainability.
Enhances the test suite for the database pull command by adding comprehensive coverage for common schema features and PostgreSQL-specific functionality. Includes new test cases for: - Restoring complex schemas from scratch, including relations and indexes - Preserving existing imports in multi-file schema setups - Handling PostgreSQL-specific features like multi-schema support and native enums - Verifying schema preservation for field and table mappings The tests are restructured for better clarity across different database providers.
Extracts PostgreSQL type casting logic into a reusable helper function to improve maintainability and ensure consistent attribute handling across all field types. Adjusts the table index sorting logic to better preserve the original database creation order while maintaining the priority of unique indexes.
Prevents foreign key indexes created automatically by MySQL from appearing in the introspected schema. This ensures the output reflects manually defined indexes and avoids redundancy in schema definitions.
Enhances the test utility helpers to allow passing extra datasource properties, such as multi-schema configurations for PostgreSQL. Refactors existing database pull tests to use these extra properties, ensuring the generated ZModel schema correctly reflects multi-schema environments while simplifying assertions.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Fix all issues with AI agents
In `@packages/cli/package.json`:
- Around line 41-43: Duplicate dependency key: remove the extra
"@zenstackhq/language" entry in package.json so the dependency only appears
once; locate the duplicate entries (the two "@zenstackhq/language" lines) and
delete one of them, ensuring the remaining dependency line keeps the intended
version spec "workspace:*" and that JSON formatting/commas remain valid.
In `@packages/cli/src/actions/db.ts`:
- Around line 393-395: The current check using
fs.lstatSync(options.out).isFile() will throw if options.out does not exist;
update the validation around options.out to first ensure options.out is truthy,
compute the parent directory with path.dirname(options.out) and verify the
parent directory exists via fs.existsSync(parentDir) (throw if it doesn't), then
only call fs.lstatSync(options.out) if fs.existsSync(options.out) and throw a
clear error if the existing path is not a file; keep the overall behavior of
rejecting non-file targets but allow creating a new file when the parent
directory exists.
- Around line 220-221: The filter is comparing d.$type (a string) against the
class/type identifiers DataModel and Enum, which will never match; change the
comparison to check string names instead by replacing the includes array with
the literal type name strings (e.g., 'DataModel' and 'Enum') so the expression
becomes something like ['DataModel','Enum'].includes(d.$type), keeping the
reference to newModel.declarations and d.$type.
- Line 152: Rename the misspelled variable simmilarRelations to similarRelations
throughout the function: update the const declaration (const simmilarRelations
=> const similarRelations) and all subsequent references/usages where
simmilarRelations is accessed (including the filter and any later reads) so the
identifier is consistent (search for simmilarRelations and replace with
similarRelations in the scope around resolvedRelations and its consumers).
In `@packages/cli/src/actions/pull/index.ts`:
- Around line 75-83: The fallback enum reconstruction currently only copies
names, losing top-level attributes and comments from the original enum; update
the block that iterates oldModel.declarations.filter(isEnum) to clone metadata
from each old enum declaration (d) into the EnumFactory before pushing: copy
top-level attributes such as @@map and @@schema (or any attribute list on d)
into the factory (e.g., via the factory's attribute/schema/map setters),
preserve the enum-level comments, and when iterating d.fields call
factory.addField with a builder that not only sets the field name but also
copies field-level attributes and comments from the original field object into
the builder; finally call factory.get({ $container: model }) and push to
model.declarations so the rebuilt enums retain the original attributes and
comments.
- Around line 325-331: uniqueColumns currently only inspects column-level flags
(table.columns.filter(c => c.unique || c.pk)) so tables that rely on composite
unique/index constraints are treated as having no unique identifier; update the
logic that determines uniqueness to also consider table.indexes where i.unique
is true and i.columns is non-empty (i.e. any composite @@unique/index), e.g.
compute a boolean like hasUnique = uniqueColumns.length > 0 ||
table.indexes?.some(i => i.unique && i.columns?.length > 0) and use that to
decide whether to call modelFactory.addAttribute(getAttributeRef('@@ignore',
services)) and push the comment; keep references to uniqueColumns,
table.indexes, modelFactory.addAttribute, getAttributeRef and '@@ignore' when
making the change.
In `@packages/cli/src/actions/pull/provider/mysql.ts`:
- Around line 188-320: The getDefaultValue function currently compares boolean
defaults using case-sensitive literals ('true' / 'false'), causing uppercase
TRUE/FALSE to fall through; normalize the trimmed input once (e.g., const
valLower = val.toLowerCase()) and replace all boolean comparisons in
getDefaultValue (the Boolean case block and the fallback boolean checks near the
bottom that test val === 'true' / 'false') to use valLower (while preserving
checks for b'1' and b'0' and numeric '1'/'0'); ensure other keyword checks that
already use toLowerCase() remain unchanged so TRUE/FALSE map to
BooleanLiteral.setValue(true/false) instead of the dbgenerated fallback.
In `@packages/cli/src/actions/pull/provider/postgresql.ts`:
- Around line 443-475: The value extraction currently strips all apostrophes
(replace(/'/g, '')), which corrupts escaped SQL strings like 'O''Reilly'; update
typeCastingConvert to split val into value and type first, then if value is
wrapped in single quotes remove only the outer quotes and unescape doubled
single-quotes (e.g., value = value.replace(/^'(.*)'$/s, '$1').replace(/''/g,
"'")); keep the existing NULL check and use the unescaped value when creating
StringLiteral; reference function typeCastingConvert and symbols enums,
getDbName, and services to locate and apply this change.
- Around line 79-90: The introspect function creates a PostgreSQL Client but
never closes it; wrap the connection and query logic inside a try/finally in the
introspect method so the client is always cleaned up and call await client.end()
in the finally block; locate the Client instantiation and the queries in the
introspect function and ensure the connect(), the two client.query(...) calls,
and the returned { enums, tables } remain inside the try while client.end() runs
in finally to prevent connection leaks.
In `@packages/cli/src/actions/pull/provider/provider.ts`:
- Line 69: The exported signature for getDefaultDatabaseType has a typo in the
returned object key: change the property name from precisition to precision in
the declaration of getDefaultDatabaseType and any related types/usages (update
the return type { precisition?: number; type: string } to { precision?: number;
type: string } and fix all references to precisition in functions, callers, and
tests such as getDefaultDatabaseType, any type aliases, and places that read
this property).
In `@packages/cli/src/actions/pull/provider/sqlite.ts`:
- Around line 106-145: The uniqueness inference currently includes partial
unique indexes; update the uniqueIndexRows computation (derived from idxList) to
exclude partial indexes by using idx.partial !== 1 (i.e., uniqueIndexRows =
idxList.filter(r => r.unique === 1 && r.partial !== 1)) so partial indexes are
not added to uniqueSingleColumn, and in the indexes mapping (the object produced
for indexes) set predicate to idx.partial === 1 ? '[partial]' : null (instead of
always null) so downstream logic can detect and skip partial indexes; refer to
symbols tableNameEsc, idxList, uniqueSingleColumn, uniqueIndexRows, and the
indexes mapping when making these changes.
- Around line 248-345: The getDefaultValue function currently does
case-sensitive comparisons on default value keywords; add const lowerVal =
val.toLowerCase() after computing val and use lowerVal for all keyword checks
(e.g., DateTime check for CURRENT_TIMESTAMP/now(), Int/BigInt autoincrement,
Boolean checks, and the fallback keyword checks) while keeping original val for
string-literal and numeric parsing; update all instances that compare against
'CURRENT_TIMESTAMP', 'now()', 'autoincrement', 'true'/'false' to use lowerVal
(but keep numeric checks and quoted string handling unchanged) so comparisons
are case-insensitive.
In `@packages/cli/src/index.ts`:
- Around line 146-169: The CLI options for model and field casing default to
'none' but the descriptions for the Option instances '--model-casing' and
'--field-casing' (in the dbCommand.pull setup) do not mention 'none'; update the
option descriptions to include 'none' as a valid choice (e.g., append "|none" or
"(or none)" to the description strings) so the help output accurately documents
the default value shown by .default('none'); ensure both Option('--model-casing
<pascal|camel|snake|kebab>') and Option('--field-casing
<pascal|camel|snake|kebab>') are updated, and keep their .default('none') calls
unchanged.
In `@packages/language/src/factory/attribute.ts`:
- Around line 23-27: The setDecl method uses a non-null assertion (decl!) which
can store null/undefined into this.decl.ref; update setDecl to handle nullish
decl safely: change the parameter type to Attribute | undefined (or keep
Attribute and ensure callers never pass null), and when decl is undefined set
this.decl to { $refText: '', ref: undefined } (or omit/refactor to not store a
ref), otherwise set { $refText: decl.name, ref: decl }; ensure you remove the
decl! assertion and use explicit conditional assignment so downstream code never
receives a forced-null via ref.
🧹 Nitpick comments (8)
packages/cli/test/db/push.test.ts (1)
12-17: Potential test duplication withpackages/cli/test/db.test.ts.This test is identical to the first test case in
db.test.ts(same describe block name, same model, same assertions). Consider whether this file serves a distinct purpose (e.g., dedicated push tests that will expand) or if it should be removed to avoid maintaining duplicate tests.packages/cli/test/utils.ts (1)
23-43: Potential runtime error if called outside test context.
expect.getState()requires an active Vitest test context. IfgetTestDbNameis called during module initialization or outside a test (e.g., in abeforeAllat the wrong scope),currentTestNameandtestPathwill be undefined, producing a generic hash. This may cause DB name collisions across tests.Consider adding a guard or documenting this requirement.
packages/cli/src/actions/db.ts (2)
40-40: Consider using a union type instead ofany.Using
anyloses type safety. A discriminated union or overloads would preserve type checking.♻️ Proposed fix
-export async function run(command: string, options: any) { +export async function run(command: 'push', options: PushOptions): Promise<void>; +export async function run(command: 'pull', options: PullOptions): Promise<void>; +export async function run(command: string, options: PushOptions | PullOptions) {
295-295:Object.freezeon mutable reference is ineffective.Freezing
originalFieldhere doesn't prevent modifications to its nestedattributesarray, which is spliced at line 321. If the intent is to preserve the original state for comparison, consider deep-cloning instead.packages/cli/src/actions/pull/utils.ts (2)
87-92: Redundant checks and unsafe type assertion.Lines 90-92 have redundant conditional checks and an unsafe double cast through
any. The logic flow is confusing.♻️ Proposed fix for clearer logic
const schemaAttr = decl.attributes.find((a) => a.decl.ref?.name === '@@schema'); - const schemaAttrValue = schemaAttr?.args[0]?.value; - let schema: string; - if (schemaAttrValue?.$type !== 'StringLiteral') schema = 'public'; - if (!schemaAttr) schema = 'public'; - else schema = (schemaAttr.args[0]?.value as any)?.value as string; + let schema = 'public'; + const schemaAttrValue = schemaAttr?.args[0]?.value; + if (schemaAttr && schemaAttrValue?.$type === 'StringLiteral') { + schema = schemaAttrValue.value; + }
66-70: Minor: Awkward formatting could be simplified.♻️ Proposed simplification
const schemasField = datasource.fields.find((f) => f.name === 'schemas'); - const schemas = - (schemasField && - getLiteralArray(schemasField.value) - ?.filter((s) => s !== undefined)) as string[] || - []; + const schemas = schemasField + ? (getLiteralArray<string>(schemasField.value) ?? []) + : [];packages/language/src/factory/attribute.ts (1)
17-73: Consider extracting shared logic betweenDataFieldAttributeFactoryandDataModelAttributeFactory.These two classes are nearly identical, differing only in the AST node type. A shared base class or factory function could reduce duplication.
packages/language/src/factory/declaration.ts (1)
149-162: API inconsistency:addAttributeaccepts both builder and instance.
DataFieldFactory.addAttributeaccepts either a builder function or a directDataFieldAttributeFactoryinstance, unlike otheradd*methods which only accept builders. While flexible, this inconsistency may confuse users.Consider documenting this or making other add methods consistent.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/cli/src/actions/pull/index.ts`:
- Around line 434-449: Rename the misspelled parameter simmilarRelations to
similarRelations in the syncRelation function signature and its type annotation,
then update every usage of simmilarRelations inside syncRelation (references
such as where it's read/compared/iterated) to the new name; finally update the
corresponding caller(s) in db.ts that pass that argument so they use
similarRelations as well to keep call sites and the function signature
consistent.
In `@packages/language/src/factory/attribute.ts`:
- Around line 55-64: The setDecl method currently uses unsafe nullish handling
(decl?.name ?? '' and decl!) which can store null/undefined refs; update setDecl
in this class to mirror DataFieldAttributeFactory.setDecl: validate that the
incoming decl is non-null (throw a clear Error if it is), then use decl.name and
decl as non-null values when assigning this.decl and calling this.update,
removing any use of the non-null assertion or optional chaining for decl.
🧹 Nitpick comments (3)
packages/language/src/factory/attribute.ts (2)
111-120: Consider adding null validation for consistency withDataFieldAttributeFactory.
DataFieldAttributeFactory.setDeclnow validates thatdeclis non-null. For consistency and better error messages if a caller bypasses TypeScript's type checking, consider applying the same defensive pattern here.🛡️ Proposed fix
setDecl(decl: Attribute) { + if (!decl) { + throw new Error('Attribute declaration is required'); + } this.decl = { $refText: decl.name, ref: decl,
17-133: Consider extracting sharedsetDecl/addArglogic to reduce duplication.
DataFieldAttributeFactory,DataModelAttributeFactory, andInternalAttributeFactoryall implement nearly identicalsetDeclandaddArgmethods. This duplication increases maintenance burden (as seen with the inconsistent null-check fix). Consider a shared mixin or abstract base class.♻️ Example approach using a mixin
// Mixin for attribute factories with decl and args function WithDeclAndArgs<T extends new (...args: any[]) => AstFactory<any>>(Base: T) { return class extends Base { args: AttributeArgFactory[] = []; decl?: Reference<Attribute>; setDecl(decl: Attribute) { if (!decl) { throw new Error('Attribute declaration is required'); } this.decl = { $refText: decl.name, ref: decl, }; this.update({ decl: this.decl }); return this; } addArg(builder: (b: ExpressionBuilder) => AstFactory<Expression>, name?: string) { const factory = new AttributeArgFactory().setValue(builder); if (name) { factory.setName(name); } this.args.push(factory); this.update({ args: this.args }); return this; } }; }packages/cli/src/actions/pull/index.ts (1)
171-186: Unused parameteroldModelinsyncTablefunction.The
oldModelparameter is declared but never used in the function body. Consider removing it if not needed or adding a TODO comment if planned for future use.♻️ Proposed fix
export function syncTable({ model, provider, table, services, options, defaultSchema, }: { table: IntrospectedTable; model: Model; - oldModel: Model; provider: IntrospectionProvider; services: ZModelServices; options: PullOptions; defaultSchema: string; }) {
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@packages/cli/src/actions/db.ts`:
- Around line 368-390: The forEach callbacks using expression-bodied arrow
functions (e.g., deletedModels.forEach((msg) => console.log(msg))) violate
Biome's useIterableCallbackReturn rule; update each forEach callback for
deletedModels, deletedEnums, addedFields, deletedAttributes, and deletedFields
to use block-bodied functions (for example (msg) => { console.log(msg); }) so
they no longer implicitly return a value. Ensure all five occurrences are
changed consistently.
In `@packages/cli/src/actions/pull/index.ts`:
- Around line 88-94: The inner arrow used in d.fields.forEach →
v.comments.forEach((c) => builder.addComment(c)) currently uses an expression
body which implicitly returns a value and triggers the useIterableCallbackReturn
lint; change that callback to an explicit block body so it doesn't return
anything (e.g., v.comments.forEach((c) => { builder.addComment(c); })); keep the
surrounding structure (within factory.addField and builder.setName) unchanged
and only update the v.comments.forEach callback to a block body calling
builder.addComment.
In `@packages/cli/src/actions/pull/provider/mysql.ts`:
- Around line 22-93: The getBuiltinType function currently only detects enum/set
when the type string includes values (startsWith('enum(')/'set(')), but the
introspection passes bare 'enum'/'set'; update getBuiltinType to add explicit
switch cases for 'enum' and 'set' (alongside the existing default checks) and
return { type: 'String', isArray } for both so they no longer fall through to
Unsupported.
In `@packages/cli/src/actions/pull/provider/postgresql.ts`:
- Around line 9-15: The isSupportedFeature function currently only recognizes
'Schema', causing syncEnums to skip DB enums; update isSupportedFeature (in
packages/cli/src/actions/pull/provider/postgresql.ts) to also return true for
the 'NativeEnum' feature (e.g., add a case 'NativeEnum': return true) so
PostgreSQL enums are detected and syncEnums will process them; keep the existing
default false behavior for other features.
In `@packages/cli/src/actions/pull/provider/sqlite.ts`:
- Around line 23-49: The getBuiltinType method currently matches the raw type
string exactly and misses parameterized types like VARCHAR(255) and JSON; update
getBuiltinType to first normalize by lowercasing and trimming then strip any
parenthesized constraint suffix (e.g., remove "(...)" from the type) before the
switch, and extend the switch in getBuiltinType to treat 'json' (and 'jsonb') as
Json and map 'varchar', 'char', and their variants to 'String' so parameterized
and json variants are recognized instead of falling through to 'Unsupported'.
🧹 Nitpick comments (2)
packages/language/src/factory/attribute.ts (2)
81-103: Avoid default empty string for optional arg names.Using
''for an optional identifier blurs “unset” vs “set,” and can leak empty identifiers into downstream consumers that treat non-undefined as meaningful.🔧 Proposed tweak
export class AttributeArgFactory extends AstFactory<AttributeArg> { - name?: RegularID = ''; + name?: RegularID; value?: AstFactory<Expression>;
114-123: AlignsetDeclnull-guard with sibling factories.DataField/DataModel factories already guard nullish
decl. Adding the same guard here keeps behavior consistent and yields clearer errors.🔧 Proposed tweak
setDecl(decl: Attribute) { + if (!decl) { + throw new Error('Attribute declaration is required'); + } this.decl = { $refText: decl.name, ref: decl,
| if (deletedModels.length > 0) { | ||
| console.log(colors.bold('\nDeleted Models:')); | ||
| deletedModels.forEach((msg) => console.log(msg)); | ||
| } | ||
|
|
||
| if (deletedEnums.length > 0) { | ||
| console.log(colors.bold('\nDeleted Enums:')); | ||
| deletedEnums.forEach((msg) => console.log(msg)); | ||
| } | ||
|
|
||
| if (addedFields.length > 0) { | ||
| console.log(colors.bold('\nAdded Fields:')); | ||
| addedFields.forEach((msg) => console.log(msg)); | ||
| } | ||
|
|
||
| if (deletedAttributes.length > 0) { | ||
| console.log(colors.bold('\nDeleted Attributes:')); | ||
| deletedAttributes.forEach((msg) => console.log(msg)); | ||
| } | ||
|
|
||
| if (deletedFields.length > 0) { | ||
| console.log(colors.bold('\nDeleted Fields:')); | ||
| deletedFields.forEach((msg) => console.log(msg)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -n 395 packages/cli/src/actions/db.ts | tail -n 30Repository: zenstackhq/zenstack-v3
Length of output: 1135
🏁 Script executed:
wc -l packages/cli/src/actions/db.tsRepository: zenstackhq/zenstack-v3
Length of output: 100
🌐 Web query:
Biome useIterableCallbackReturn rule forEach expression body
💡 Result:
Biome’s lint/suspicious/useIterableCallbackReturn enforces that forEach callbacks must not return a value. With an expression-bodied arrow function, JavaScript implicitly returns the expression’s value, so Biome flags code like:
arr.forEach(x => doSomething(x)); // implicitly returns doSomething(...) -> lint errorFix it by making the callback return void (or by using a block body):
arr.forEach(x => { doSomething(x); }); // OK (no return)
arr.forEach(x => void doSomething(x)); // OK (explicitly void)Biome explicitly documents that return values are disallowed for forEach, and that () => void … is considered valid. (biomejs.dev)
Citations:
🏁 Script executed:
find . -maxdepth 2 -type f \( -name "biome.json" -o -name "biome.jsonc" -o -name ".biomeignore" \) 2>/dev/nullRepository: zenstackhq/zenstack-v3
Length of output: 78
🏁 Script executed:
cat biome.json 2>/dev/null || cat biome.jsonc 2>/dev/null || echo "No biome config found in root"Repository: zenstackhq/zenstack-v3
Length of output: 1841
Convert forEach expression bodies to block bodies to comply with Biome's useIterableCallbackReturn rule.
Lines 370, 374, 378, 382, 386: Expression-bodied forEach callbacks implicitly return values, which violates the rule. Use block bodies instead.
Suggested fix
if (deletedModels.length > 0) {
console.log(colors.bold('\nDeleted Models:'));
- deletedModels.forEach((msg) => console.log(msg));
+ deletedModels.forEach((msg) => {
+ console.log(msg);
+ });
}
@@
if (deletedEnums.length > 0) {
console.log(colors.bold('\nDeleted Enums:'));
- deletedEnums.forEach((msg) => console.log(msg));
+ deletedEnums.forEach((msg) => {
+ console.log(msg);
+ });
}
@@
if (addedFields.length > 0) {
console.log(colors.bold('\nAdded Fields:'));
- addedFields.forEach((msg) => console.log(msg));
+ addedFields.forEach((msg) => {
+ console.log(msg);
+ });
}
@@
if (deletedAttributes.length > 0) {
console.log(colors.bold('\nDeleted Attributes:'));
- deletedAttributes.forEach((msg) => console.log(msg));
+ deletedAttributes.forEach((msg) => {
+ console.log(msg);
+ });
}
@@
if (deletedFields.length > 0) {
console.log(colors.bold('\nDeleted Fields:'));
- deletedFields.forEach((msg) => console.log(msg));
+ deletedFields.forEach((msg) => {
+ console.log(msg);
+ });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (deletedModels.length > 0) { | |
| console.log(colors.bold('\nDeleted Models:')); | |
| deletedModels.forEach((msg) => console.log(msg)); | |
| } | |
| if (deletedEnums.length > 0) { | |
| console.log(colors.bold('\nDeleted Enums:')); | |
| deletedEnums.forEach((msg) => console.log(msg)); | |
| } | |
| if (addedFields.length > 0) { | |
| console.log(colors.bold('\nAdded Fields:')); | |
| addedFields.forEach((msg) => console.log(msg)); | |
| } | |
| if (deletedAttributes.length > 0) { | |
| console.log(colors.bold('\nDeleted Attributes:')); | |
| deletedAttributes.forEach((msg) => console.log(msg)); | |
| } | |
| if (deletedFields.length > 0) { | |
| console.log(colors.bold('\nDeleted Fields:')); | |
| deletedFields.forEach((msg) => console.log(msg)); | |
| if (deletedModels.length > 0) { | |
| console.log(colors.bold('\nDeleted Models:')); | |
| deletedModels.forEach((msg) => { | |
| console.log(msg); | |
| }); | |
| } | |
| if (deletedEnums.length > 0) { | |
| console.log(colors.bold('\nDeleted Enums:')); | |
| deletedEnums.forEach((msg) => { | |
| console.log(msg); | |
| }); | |
| } | |
| if (addedFields.length > 0) { | |
| console.log(colors.bold('\nAdded Fields:')); | |
| addedFields.forEach((msg) => { | |
| console.log(msg); | |
| }); | |
| } | |
| if (deletedAttributes.length > 0) { | |
| console.log(colors.bold('\nDeleted Attributes:')); | |
| deletedAttributes.forEach((msg) => { | |
| console.log(msg); | |
| }); | |
| } | |
| if (deletedFields.length > 0) { | |
| console.log(colors.bold('\nDeleted Fields:')); | |
| deletedFields.forEach((msg) => { | |
| console.log(msg); | |
| }); |
🧰 Tools
🪛 Biome (2.3.13)
[error] 370-370: This callback passed to forEach() iterable method should not return a value.
Either remove this return or remove the returned value.
(lint/suspicious/useIterableCallbackReturn)
[error] 375-375: This callback passed to forEach() iterable method should not return a value.
Either remove this return or remove the returned value.
(lint/suspicious/useIterableCallbackReturn)
[error] 380-380: This callback passed to forEach() iterable method should not return a value.
Either remove this return or remove the returned value.
(lint/suspicious/useIterableCallbackReturn)
[error] 385-385: This callback passed to forEach() iterable method should not return a value.
Either remove this return or remove the returned value.
(lint/suspicious/useIterableCallbackReturn)
[error] 390-390: This callback passed to forEach() iterable method should not return a value.
Either remove this return or remove the returned value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
In `@packages/cli/src/actions/db.ts` around lines 368 - 390, The forEach callbacks
using expression-bodied arrow functions (e.g., deletedModels.forEach((msg) =>
console.log(msg))) violate Biome's useIterableCallbackReturn rule; update each
forEach callback for deletedModels, deletedEnums, addedFields,
deletedAttributes, and deletedFields to use block-bodied functions (for example
(msg) => { console.log(msg); }) so they no longer implicitly return a value.
Ensure all five occurrences are changed consistently.
| d.fields.forEach((v) => { | ||
| factory.addField((builder) => { | ||
| builder.setName(v.name); | ||
| // Copy field-level comments | ||
| if (v.comments?.length) { | ||
| v.comments.forEach((c) => builder.addComment(c)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the file exists and view lines 88-94
cat -n packages/cli/src/actions/pull/index.ts | sed -n '80,100p'Repository: zenstackhq/zenstack-v3
Length of output: 1247
🏁 Script executed:
# Check if Biome is configured in the project
fd -t f "biome\.json|biome\.jsonc|\.biomerc" . | head -5Repository: zenstackhq/zenstack-v3
Length of output: 78
🏁 Script executed:
# Search for forEach patterns that might match the issue
rg "\.forEach\(\(.*\)\s*=>" packages/cli/src/actions/pull/index.ts -A 2 -B 2Repository: zenstackhq/zenstack-v3
Length of output: 1955
🏁 Script executed:
# Check the Biome configuration
cat biome.jsonRepository: zenstackhq/zenstack-v3
Length of output: 1841
🏁 Script executed:
# Let's also verify what the actual line 93 contains more precisely
sed -n '93p' packages/cli/src/actions/pull/index.tsRepository: zenstackhq/zenstack-v3
Length of output: 143
Avoid returning a value from forEach callbacks (Biome lint).
Line 93: forEach with an expression body returns a value and triggers useIterableCallbackReturn. Convert to a block body to avoid the implicit return.
✅ Suggested fix
- v.comments.forEach((c) => builder.addComment(c));
+ v.comments.forEach((c) => {
+ builder.addComment(c);
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| d.fields.forEach((v) => { | |
| factory.addField((builder) => { | |
| builder.setName(v.name); | |
| // Copy field-level comments | |
| if (v.comments?.length) { | |
| v.comments.forEach((c) => builder.addComment(c)); | |
| } | |
| d.fields.forEach((v) => { | |
| factory.addField((builder) => { | |
| builder.setName(v.name); | |
| // Copy field-level comments | |
| if (v.comments?.length) { | |
| v.comments.forEach((c) => { | |
| builder.addComment(c); | |
| }); | |
| } |
🧰 Tools
🪛 Biome (2.3.13)
[error] 93-93: This callback passed to forEach() iterable method should not return a value.
Either remove this return or remove the returned value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
In `@packages/cli/src/actions/pull/index.ts` around lines 88 - 94, The inner arrow
used in d.fields.forEach → v.comments.forEach((c) => builder.addComment(c))
currently uses an expression body which implicitly returns a value and triggers
the useIterableCallbackReturn lint; change that callback to an explicit block
body so it doesn't return anything (e.g., v.comments.forEach((c) => {
builder.addComment(c); })); keep the surrounding structure (within
factory.addField and builder.setName) unchanged and only update the
v.comments.forEach callback to a block body calling builder.addComment.
| getBuiltinType(type) { | ||
| const t = (type || '').toLowerCase().trim(); | ||
|
|
||
| // MySQL doesn't have native array types | ||
| const isArray = false; | ||
|
|
||
| switch (t) { | ||
| // integers | ||
| case 'tinyint': | ||
| case 'smallint': | ||
| case 'mediumint': | ||
| case 'int': | ||
| case 'integer': | ||
| return { type: 'Int', isArray }; | ||
| case 'bigint': | ||
| return { type: 'BigInt', isArray }; | ||
|
|
||
| // decimals and floats | ||
| case 'decimal': | ||
| case 'numeric': | ||
| return { type: 'Decimal', isArray }; | ||
| case 'float': | ||
| case 'double': | ||
| case 'real': | ||
| return { type: 'Float', isArray }; | ||
|
|
||
| // boolean (MySQL uses TINYINT(1) for boolean) | ||
| case 'boolean': | ||
| case 'bool': | ||
| return { type: 'Boolean', isArray }; | ||
|
|
||
| // strings | ||
| case 'char': | ||
| case 'varchar': | ||
| case 'tinytext': | ||
| case 'text': | ||
| case 'mediumtext': | ||
| case 'longtext': | ||
| return { type: 'String', isArray }; | ||
|
|
||
| // dates/times | ||
| case 'date': | ||
| case 'time': | ||
| case 'datetime': | ||
| case 'timestamp': | ||
| case 'year': | ||
| return { type: 'DateTime', isArray }; | ||
|
|
||
| // binary | ||
| case 'binary': | ||
| case 'varbinary': | ||
| case 'tinyblob': | ||
| case 'blob': | ||
| case 'mediumblob': | ||
| case 'longblob': | ||
| return { type: 'Bytes', isArray }; | ||
|
|
||
| // json | ||
| case 'json': | ||
| return { type: 'Json', isArray }; | ||
|
|
||
| default: | ||
| // Handle ENUM type - MySQL returns enum values like "enum('val1','val2')" | ||
| if (t.startsWith('enum(')) { | ||
| return { type: 'String', isArray }; | ||
| } | ||
| // Handle SET type | ||
| if (t.startsWith('set(')) { | ||
| return { type: 'String', isArray }; | ||
| } | ||
| return { type: 'Unsupported' as const, isArray }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and read the MySQL provider file
find . -type f -name "mysql.ts" | grep -E "actions/pull/provider"Repository: zenstackhq/zenstack-v3
Length of output: 115
🏁 Script executed:
# Check the file exists and read the relevant section
cat -n packages/cli/src/actions/pull/provider/mysql.ts | head -100Repository: zenstackhq/zenstack-v3
Length of output: 4209
🏁 Script executed:
# Search for DATA_TYPE usage in the MySQL provider file
rg "DATA_TYPE" packages/cli/src/actions/pull/provider/mysql.ts -A 2 -B 2Repository: zenstackhq/zenstack-v3
Length of output: 480
🏁 Script executed:
# Check if COLUMN_TYPE is used anywhere in the query or introspection
rg "COLUMN_TYPE" packages/cli/src/actions/pull/provider/mysql.ts -B 3 -A 3Repository: zenstackhq/zenstack-v3
Length of output: 800
🏁 Script executed:
# Search for how the datatype value is retrieved and passed to getBuiltinType
rg "getBuiltinType" packages/cli/src/actions/pull/provider/mysql.ts -B 5 -A 5Repository: zenstackhq/zenstack-v3
Length of output: 322
🏁 Script executed:
# Check the full introspection query to understand the complete flow
sed -n '140,250p' packages/cli/src/actions/pull/provider/mysql.tsRepository: zenstackhq/zenstack-v3
Length of output: 5114
Add cases for MySQL enum and set types without values.
Lines 28–93: The table introspection query passes c.DATA_TYPE to getBuiltinType, which returns bare enum or set strings (not enum(...) or set(...) with values). These fall through to the default handler and are not matched by the existing pattern checks, resulting in Unsupported type mapping. Add explicit switch cases for enum and set to map them to String.
Suggested fix
// json
case 'json':
return { type: 'Json', isArray };
+ // enum/set (from DATA_TYPE without values)
+ case 'enum':
+ case 'set':
+ return { type: 'String', isArray };
default:
// Handle ENUM type - MySQL returns enum values like "enum('val1','val2')"🤖 Prompt for AI Agents
In `@packages/cli/src/actions/pull/provider/mysql.ts` around lines 22 - 93, The
getBuiltinType function currently only detects enum/set when the type string
includes values (startsWith('enum(')/'set(')), but the introspection passes bare
'enum'/'set'; update getBuiltinType to add explicit switch cases for 'enum' and
'set' (alongside the existing default checks) and return { type: 'String',
isArray } for both so they no longer fall through to Unsupported.
| isSupportedFeature(feature) { | ||
| switch (feature) { | ||
| case 'Schema': | ||
| return true; | ||
| default: | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Enable NativeEnum support for PostgreSQL enums.
Line [9]-[15]: isSupportedFeature only returns true for Schema, so syncEnums will skip DB enums and keep stale enum definitions. Add NativeEnum support.
✅ Suggested fix
isSupportedFeature(feature) {
switch (feature) {
case 'Schema':
+ case 'NativeEnum':
return true;
default:
return false;
}
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| isSupportedFeature(feature) { | |
| switch (feature) { | |
| case 'Schema': | |
| return true; | |
| default: | |
| return false; | |
| } | |
| isSupportedFeature(feature) { | |
| switch (feature) { | |
| case 'Schema': | |
| case 'NativeEnum': | |
| return true; | |
| default: | |
| return false; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@packages/cli/src/actions/pull/provider/postgresql.ts` around lines 9 - 15,
The isSupportedFeature function currently only recognizes 'Schema', causing
syncEnums to skip DB enums; update isSupportedFeature (in
packages/cli/src/actions/pull/provider/postgresql.ts) to also return true for
the 'NativeEnum' feature (e.g., add a case 'NativeEnum': return true) so
PostgreSQL enums are detected and syncEnums will process them; keep the existing
default false behavior for other features.
| getBuiltinType(type) { | ||
| const t = (type || '').toLowerCase().trim(); | ||
| // SQLite has no array types | ||
| const isArray = false; | ||
| switch (t) { | ||
| case 'integer': | ||
| return { type: 'Int', isArray }; | ||
| case 'text': | ||
| return { type: 'String', isArray }; | ||
| case 'bigint': | ||
| return { type: 'BigInt', isArray }; | ||
| case 'blob': | ||
| return { type: 'Bytes', isArray }; | ||
| case 'real': | ||
| return { type: 'Float', isArray }; | ||
| case 'numeric': | ||
| case 'decimal': | ||
| return { type: 'Decimal', isArray }; | ||
| case 'datetime': | ||
| return { type: 'DateTime', isArray }; | ||
| case 'jsonb': | ||
| return { type: 'Json', isArray }; | ||
| case 'boolean': | ||
| return { type: 'Boolean', isArray }; | ||
| default: { | ||
| return { type: 'Unsupported' as const, isArray }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/cli/src/actions/pull/provider/sqlite.ts | head -80Repository: zenstackhq/zenstack-v3
Length of output: 4109
🏁 Script executed:
cat -n packages/cli/src/actions/pull/provider/sqlite.ts | sed -n '80,200p'Repository: zenstackhq/zenstack-v3
Length of output: 7050
🏁 Script executed:
cat -n packages/cli/src/actions/pull/provider/sqlite.ts | sed -n '200,250p'Repository: zenstackhq/zenstack-v3
Length of output: 2631
🏁 Script executed:
# Search for tests related to SQLite type mapping
fd -e test -e spec | xargs rg -l "getBuiltinType|sqlite" -i | head -10Repository: zenstackhq/zenstack-v3
Length of output: 372
🏁 Script executed:
# Search for SQLite tests and introspection tests
fd -type f -name "*.ts" -o -name "*.js" | xargs rg "getBuiltinType|VARCHAR|JSON.*sqlite" -i -l 2>/dev/null | head -20Repository: zenstackhq/zenstack-v3
Length of output: 629
🏁 Script executed:
# Check PostgreSQL provider for comparison
cat -n packages/cli/src/actions/pull/provider/postgresql.ts | head -100Repository: zenstackhq/zenstack-v3
Length of output: 3980
🏁 Script executed:
# Check MySQL provider for comparison
cat -n packages/cli/src/actions/pull/provider/mysql.ts | head -150Repository: zenstackhq/zenstack-v3
Length of output: 6772
🏁 Script executed:
# Check if there are any tests specifically for SQLite type handling
rg "sqlite.*getBuiltinType|getBuiltinType.*sqlite" -i -A 5 -B 5Repository: zenstackhq/zenstack-v3
Length of output: 48
🏁 Script executed:
# Search for any pull/introspection tests
find . -type f -name "*.test.ts" -o -name "*.spec.ts" | xargs rg -l "introspect|pull" | head -5Repository: zenstackhq/zenstack-v3
Length of output: 101
🏁 Script executed:
cat -n ./packages/cli/test/db/pull.test.ts | head -200Repository: zenstackhq/zenstack-v3
Length of output: 8727
🏁 Script executed:
cat -n ./packages/cli/test/db/pull.test.ts | sed -n '200,400p'Repository: zenstackhq/zenstack-v3
Length of output: 7651
🏁 Script executed:
cat -n ./packages/cli/test/db/pull.test.ts | sed -n '400,600p'Repository: zenstackhq/zenstack-v3
Length of output: 1785
🏁 Script executed:
cat -n ./packages/cli/test/db/pull.test.ts | sed -n '450,700p'Repository: zenstackhq/zenstack-v3
Length of output: 76
🏁 Script executed:
# Search for SQLite-specific tests
rg -n "sqlite" -i ./packages/cli/test/db/pull.test.tsRepository: zenstackhq/zenstack-v3
Length of output: 48
🏁 Script executed:
# Search for actual SQLite database schema examples or tests
rg -r "CREATE TABLE" --type sql | head -20Repository: zenstackhq/zenstack-v3
Length of output: 462
🏁 Script executed:
# Search for SQLite documentation or actual type examples in tests
rg "VARCHAR|JSONB" --type ts --type js | head -30Repository: zenstackhq/zenstack-v3
Length of output: 262
Normalize and expand SQLite type matching to handle parameterized types and json variant.
The getBuiltinType method uses exact string matching which fails for types with length/precision constraints. PRAGMA table_xinfo returns types like VARCHAR(255) or JSON, which fall through to Unsupported. Strip parenthesized constraints before switching and add support for json, varchar, and char variants to align with other providers.
Suggested fix
getBuiltinType(type) {
const t = (type || '').toLowerCase().trim();
+ const baseType = t.replace(/\(.*\)/, '').trim();
// SQLite has no array types
const isArray = false;
- switch (t) {
+ switch (baseType) {
case 'integer':
return { type: 'Int', isArray };
+ case 'varchar':
+ case 'char':
case 'text':
return { type: 'String', isArray };
case 'bigint':
@@ -41,6 +45,7 @@
case 'datetime':
return { type: 'DateTime', isArray };
+ case 'json':
case 'jsonb':
return { type: 'Json', isArray };
case 'boolean':🤖 Prompt for AI Agents
In `@packages/cli/src/actions/pull/provider/sqlite.ts` around lines 23 - 49, The
getBuiltinType method currently matches the raw type string exactly and misses
parameterized types like VARCHAR(255) and JSON; update getBuiltinType to first
normalize by lowercasing and trimming then strip any parenthesized constraint
suffix (e.g., remove "(...)" from the type) before the switch, and extend the
switch in getBuiltinType to treat 'json' (and 'jsonb') as Json and map
'varchar', 'char', and their variants to 'String' so parameterized and json
variants are recognized instead of falling through to 'Unsupported'.
3 participants
Summary
This PR introduces
db pullsupport tozenstack_v3, similar to Prisma’sdb pull.The command enables introspection of the database schema and generates corresponding models within the project.
Current Status
db pullcommand@mapfor fields/models@db.\<Raw databse type\>, ie:@db.Real)Notes
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.