Conversation
📝 WalkthroughWalkthroughAdds a DB "pull" workflow with introspection providers (MySQL, PostgreSQL, SQLite), provider registry, sync helpers to convert introspected schemas to ZModel, a new CLI 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
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.
Enhances database introspection to correctly handle composite foreign keys by mapping columns by position rather than name alone. Improves MySQL introspection by checking statistics tables for single-column unique indexes, ensuring accurate model generation even when column keys are ambiguous. Ensures MySQL synthetic enum names respect requested model casing to prevent unnecessary schema mapping. Adds comprehensive tests for composite relations and database-specific uniqueness detection.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@packages/cli/src/actions/db.ts`:
- Around line 626-663: The log in the docs loop uses uri.path but the file is
written to uri.fsPath, causing odd output on Windows; update the console message
inside the else branch (the for...of over docs where
parseResult.value/documentModel is processed and zmodelSchema is written using
fs.writeFileSync(uri.fsPath, ...)) to log the actual OS path (use uri.fsPath or
a resolved path) instead of uri.path so logged paths match the written paths.
In `@packages/cli/src/actions/pull/utils.ts`:
- Around line 75-84: The code casts the result of getStringLiteral(...) for the
datasource provider to DataSourceProviderType without checking for undefined,
which can mask missing provider fields; update the logic around
datasource.fields.find((f) => f.name === 'provider')?.value and getStringLiteral
to explicitly validate the provider (in the function building the returned
object where name/provider/url/defaultSchema/schemas/allSchemas are assembled),
and if getStringLiteral returns undefined either throw a clear error indicating
the datasource is missing a provider or supply a safe default; ensure the thrown
error message names the datasource (datasource.name) and the provider field so
failures are clear and traceable.
In `@packages/cli/test/db/pull.test.ts`:
- Around line 736-768: The test skips when getTestDbProvider() isn't mysql or
postgresql but calls createProject without specifying the provider and calls
getDefaultPrelude() with no args, which can produce a mismatched datasource;
modify the test to pass the active provider into createProject (e.g.,
createProject(..., { provider })) and pass the provider into getDefaultPrelude
(e.g., getDefaultPrelude(provider)) so the created project and the zeroed
prelude use the same SQL provider; update the calls to createProject and
getDefaultPrelude referenced in this test to include the provider variable.
🧹 Nitpick comments (14)
packages/cli/src/actions/pull/provider/sqlite.ts (1)
370-379: Convention-based@updatedAtheuristic is reasonable but worth documenting.This silently adds
@updatedAtto any DateTime field namedupdatedAt/updated_at, which may surprise users who happen to use those names for non-update-tracking columns. Consider adding a log message (like the existing casing mappings do) so users are aware the attribute was inferred.packages/cli/src/actions/db.ts (2)
82-84:runPullcould benefit from early spinner cleanup on non-introspection errors.The spinner is created at line 83 and started at line 111. If an error occurs between lines 85–110 (schema loading, provider resolution),
spinner.fail()in the catch block (line 667) will display a failure message even though the spinner was never started, resulting in a confusing "Pull failed" message with no preceding "Introspecting database..." line.Consider moving
spinner.start()earlier or guardingspinner.fail()with a check on whether the spinner is actually spinning.
403-438:@defaultattribute diffing via serialization is pragmatic but fragile.The
serializeArgshelper at line 408 handles known AST node types but falls back toarg.value?.$type ?? 'unknown'for unrecognized types (line 422). If two different default expressions both serialize to'unknown', the diff won't detect a change. This is a low-risk edge case for now, but consider logging when the fallback is hit so it doesn't silently swallow mismatches in the future.packages/cli/test/utils.ts (2)
9-21:TEST_PG_CONFIGandTEST_MYSQL_CONFIGare duplicated from@zenstackhq/testtools.These constants are identical to those in
packages/testtools/src/client.ts(which is already a devDependency). Importing them would avoid drift between the two copies.#!/bin/bash # Verify the testtools exports match rg -n "TEST_PG_CONFIG|TEST_MYSQL_CONFIG" --type ts -g '*/testtools/*'
23-43: SQLite test DB name is static — parallel test runs would collide.
getTestDbName('sqlite')always returns'./test.db'(line 25). If multiple SQLite test suites run in parallel (e.g., via Vitest's default worker pool), they'd share the same file in eachworkDir. SincecreateProjectcreates a freshworkDirper test, this is likely safe in practice — just noting it for awareness.packages/cli/src/actions/pull/provider/mysql.ts (2)
143-155: Inconsistent indentation in column sorting/mapping block.Lines 143–155 use 2-space indentation for the
const sortedColumnsblock while the surrounding code uses 4-space (or 8-space within the method body). This looks like an accidental formatting inconsistency.
263-294:getFieldAttributeshandlesUnsupportedfieldType unsafely.
this.getDefaultDatabaseType(fieldType as BuiltinType)on line 276 castsfieldTypewhich could be'Unsupported'. WhilegetDefaultDatabaseTypereturnsundefinedfor unrecognized types (due to the exhaustive switch with no default), the cast itself is misleading. An early return forUnsupportedwould be cleaner and safer.Suggested improvement
getFieldAttributes({ fieldName, fieldType, datatype, length, precision, services }) { const factories: DataFieldAttributeFactory[] = []; + + if (fieldType === 'Unsupported') { + return factories; + } // Add `@updatedAt` for DateTime fields named updatedAt or updated_atpackages/language/src/factory/attribute.ts (1)
17-79:DataFieldAttributeFactoryandDataModelAttributeFactoryare nearly identical — consider extracting a shared base.Both classes have identical
setDeclandaddArgimplementations. Extracting a parameterized base class (e.g.,BaseAttributeFactory<T extends DataFieldAttribute | DataModelAttribute>) would eliminate the duplication and reduce the maintenance surface.packages/cli/test/db/pull.test.ts (1)
296-307: Comprehensive "all features" test with informative TODO.The schema covers a wide variety of features (enums, relations, mappings, indexes, defaults, multiple types). The TODO on line 302 about MySQL enum handling is a known limitation — good to track.
Would you like me to open an issue to track the MySQL enum detection improvement mentioned in the TODO?
packages/cli/src/actions/pull/index.ts (2)
397-401:tableMapAttributeis unused insyncRelation.The variable on line 401 is fetched but never referenced in the function body. The null-check on line 405 also validates it unnecessarily.
Suggested fix
const idAttribute = getAttributeRef('@id', services); const uniqueAttribute = getAttributeRef('@unique', services); const relationAttribute = getAttributeRef('@relation', services); const fieldMapAttribute = getAttributeRef('@map', services); - const tableMapAttribute = getAttributeRef('@@map', services); - const includeRelationName = selfRelation || similarRelations > 0; - - if (!idAttribute || !uniqueAttribute || !relationAttribute || !fieldMapAttribute || !tableMapAttribute) { + if (!idAttribute || !uniqueAttribute || !relationAttribute || !fieldMapAttribute) { throw new CliError('Cannot find required attributes in the model.'); }
440-463: Complex naming logic is difficult to follow in single expressions.Lines 446 and 454–458 pack multiple conditional naming rules into very long ternary/template expressions. Consider extracting helper functions (e.g.,
deriveSourceFieldName) to improve readability and make the naming heuristics easier to reason about and test independently.packages/cli/src/actions/pull/provider/provider.ts (1)
7-48: Optional: consider extracting inline column/index types for reuse.The
columnsandindexesinline types withinIntrospectedTableare quite large. Extracting them as named types (e.g.,IntrospectedColumn,IntrospectedIndex) would improve readability and enable reuse in provider implementations and tests.packages/cli/src/actions/pull/utils.ts (2)
138-146: Minor duplication withgetDbName's schema extraction logic.Both
getDbSchemaNameand the schema extraction block ingetDbName(lines 90–97) perform the same@@schemalookup with a'public'default. Consider extracting a shared helper to avoid divergence if the default or lookup logic changes.
181-189: The decimal branch (lines 185–186) is identical to the fallback (line 188).Both return
ab.NumberLiteral.setValue(val)unchanged. The explicit decimal match is arguably intentional for readability/symmetry withnormalizeDecimalDefault, so this is just a nit.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/cli/src/actions/pull/provider/sqlite.ts`:
- Around line 237-254: The current extraction only captures the first column
from the FOREIGN KEY(...) clause; update the logic around fkRegex /
fkConstraintNames in sqlite.ts to capture the full parenthesized list (e.g.,
change the regex to capture the whole group inside FOREIGN KEY\s*\(([^)]+)\) or
add a second regex) and then split that captured group on commas, normalizing
quotes/backticks and whitespace, and for each column add
fkConstraintNames.set(columnName, constraintName); alternatively you can map
constraint names by the FK identifier from PRAGMA foreign_key_list (use the FK
id to associate all seq rows) — target the fkRegex / fkConstraintNames handling
and t.definition parsing to implement one of these fixes.
- Around line 97-113: In the default branch of the SQLite type mapping (the
block that inspects variable t and returns { type: ..., isArray }), handle empty
or null declarations by treating an empty t as BLOB affinity: if t is falsy or
an empty string, return { type: 'Bytes', isArray } before the other includes()
checks; also ensure t is normalized (lowercased) earlier where t is derived so
the includes() checks remain correct. This will ensure columns declared without
a type (e.g., CREATE TABLE t(x)) map to Bytes instead of Unsupported.
🧹 Nitpick comments (4)
packages/cli/src/actions/pull/provider/sqlite.ts (3)
8-21:isSupportedFeaturereturnsfalsefor all features, including thedefaultcase.This is a safe, conservative approach for SQLite. However, if new features are added to the provider interface in the future that SQLite does support, this default will silently mark them as unsupported. Consider whether the
defaultbranch should returntrueto opt in to new features by default (matching the typical pattern where only known-unsupported features are excluded).
268-277:as anycasts onfk.on_updateandfk.on_deletebypass type safety.PRAGMA
foreign_key_listreturns string values like'CASCADE','SET NULL','NO ACTION', etc. TheCascadetype inIntrospectedTablelikely expects specific string literals. Usingas anysilently allows mismatches. Consider mapping these to the expectedCascadeunion type or at least validating the values.
370-379: Heuristic@updatedAtinference is reasonable but worth documenting the assumption.The convention of inferring
@updatedAtfrom field names likeupdated_at/updatedAtis pragmatic fordb pull, but users with differently-purposed fields matching these names could get unexpected behavior. A small inline note or doc comment about this heuristic would help future maintainers.packages/cli/src/actions/pull/utils.ts (1)
22-29:getAttributecastsmodel.$document.referenceswithout type narrowing.Line 25 casts
model.$document.referencestoReference<AbstractDeclaration>[]. If the Langiumreferencesproperty has a different runtime shape, this could silently produce incorrect results. Consider adding a guard or documenting the assumption.
…ite FKs Ensures columns with no declared type are correctly mapped to Bytes following SQLite affinity rules, preventing them from being marked as Unsupported. Updates the DDL parser to correctly identify and map constraint names for composite foreign keys. This ensures that multi-column relations are properly restored during the pull process. Adds regression tests for both untyped columns and composite foreign key restoration.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/cli/src/actions/pull/provider/sqlite.ts`:
- Around line 292-326: The computed flag only checks for hidden === 2 (virtual
generated columns) so STORED generated columns (hidden === 3) are treated as
regular fields; update the logic in the columnsInfo loop (where columns are
built and computed is set) to mark computed when hidden === 2 OR hidden === 3
(i.e., computed: hidden === 2 || hidden === 3) so both virtual and stored
generated columns are flagged as computed and excluded from writable schema
operations.
🧹 Nitpick comments (1)
packages/cli/test/db/pull.test.ts (1)
810-842: Consider testing enum restoration independently per provider.This test runs for both MySQL and PostgreSQL but the comment at line 301–303 in the comprehensive test notes that MySQL enum handling is different (overrides vs. detects). If the enum restoration behavior diverges between providers, a single shared test with
toEqualcould mask provider-specific regressions. Consider splitting or adding provider-specific assertions if MySQL enum handling evolves.
Improves database introspection by identifying generated columns in MySQL, PostgreSQL, and SQLite. These columns are now pulled as `Unsupported` types containing their full DDL definition, preventing issues where read-only database fields were incorrectly treated as writable application-level fields. Includes normalization for expression formatting and a fix for string literal escaping in the code generator to ensure stable schema output. Relates to ZModel introspection consistency.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@packages/cli/src/actions/pull/index.ts`:
- Around line 466-470: The collision fallback currently uses
relation.references.columns[0] which can be null per the Relation type, so
update the logic around the block that sets sourceFieldName (the check using
sourceModel.fields.find and the assignment to
`${sourceFieldName}To${lowerCaseFirst(targetModel.name)}_${relation.references.columns[0]}`)
to guard against a null column: either assert non-null with a comment (e.g.,
ensure relation.references.columns[0]!) if you know it cannot be null here, or
compute a safe fallback string (like using relation.references.columns[0] ??
'ref') before interpolating so the generated name never contains "null". Ensure
the change is applied where sourceFieldName is reassigned.
In `@packages/cli/src/actions/pull/provider/postgresql.ts`:
- Around line 225-229: The enum default extraction currently strips all
apostrophes from val which corrupts escaped quotes; in the enum-handling block
(where datatype === 'enum' and enumDef is found, affecting variable enumValue)
change the logic to remove only the surrounding single quotes (if present)
rather than globally replacing all apostrophes, then split on '::' and trim; use
a regex or string-slice approach that captures the inner value between the outer
quotes so escaped apostrophes remain intact.
- Around line 334-339: The conditional and argument building in
DataFieldAttributeFactory currently use truthy checks and logical OR which treat
0 as falsy; in the block around dbAttrFactory (DataFieldAttributeFactory,
addArg, NumberLiteral) replace the truthy checks and the length! || precision!
usage with nullish-coalescing semantics so a valid 0 is preserved (e.g., check
for (length ?? precision) being defined instead of (length || precision), and
pass the selected value using length ?? precision when calling
NumberLiteral.setValue).
🧹 Nitpick comments (3)
packages/cli/src/actions/pull/provider/postgresql.ts (1)
318-318: UnsafefieldType as BuiltinTypecast when fieldType is'Unsupported'.
getDefaultDatabaseTypehas noUnsupportedcase and returnsundefined, so the subsequentdefaultDatabaseType &&check prevents runtime errors. But the cast is a type-safety gap. Consider guarding explicitly.Proposed fix
+ if (fieldType === 'Unsupported') { + return factories; + } const defaultDatabaseType = this.getDefaultDatabaseType(fieldType as BuiltinType);Based on learnings, production code in this repo should avoid type-safety workarounds; use explicit type annotations or guards instead.
packages/cli/src/actions/pull/provider/mysql.ts (1)
296-296: SamefieldType as BuiltinTypecast issue as in the PostgreSQL provider.When
fieldTypeis'Unsupported', this cast is incorrect. An early return guard would be safer and consistent.Proposed fix
+ if (fieldType === 'Unsupported') { + return factories; + } const defaultDatabaseType = this.getDefaultDatabaseType(fieldType as BuiltinType);packages/cli/test/utils.ts (1)
58-75:getDefaultPreludeduplicates URL construction already ingetTestDbUrl.The switch statement at lines 63–75 rebuilds the same URL that
getTestDbUrlproduces. This can be simplified.Proposed refactor
export function getDefaultPrelude(options?: { provider?: 'sqlite' | 'postgresql' | 'mysql', datasourceFields?: Record<string, string | string[]> }) { const provider = (options?.provider || getTestDbProvider()) ?? 'sqlite'; const dbName = getTestDbName(provider); - let dbUrl: string; - - switch (provider) { - case 'sqlite': - dbUrl = `file:${dbName}`; - break; - case 'postgresql': - dbUrl = `postgres://${TEST_PG_CONFIG.user}:${TEST_PG_CONFIG.password}@${TEST_PG_CONFIG.host}:${TEST_PG_CONFIG.port}/${dbName}`; - break; - case 'mysql': - dbUrl = `mysql://${TEST_MYSQL_CONFIG.user}:${TEST_MYSQL_CONFIG.password}@${TEST_MYSQL_CONFIG.host}:${TEST_MYSQL_CONFIG.port}/${dbName}`; - break; - default: - throw new Error(`Unsupported provider: ${provider}`); - } + const dbUrl = getTestDbUrl(provider, dbName);
ymc9
left a comment
ymc9
left a comment
There was a problem hiding this comment.
Adding some final review comments 😄
Switches from template literal interpolation to parameterized queries in MySQL introspection functions. This improves security by preventing potential SQL injection and ensures better handling of database names containing special characters.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/cli/src/actions/pull/provider/mysql.ts`:
- Around line 298-310: The precision comparison uses the wrong nullish check:
inside the conditional that compares defaultDatabaseType.precision to (length ||
precision) replace the falsy-coalescing `||` with the nullish-coalescing `??` so
that a legitimate 0 length isn't discarded; update the conditional around
dbAttr/defaultDatabaseType (the block that constructs a new
DataFieldAttributeFactory and pushes to factories) to use `(length ??
precision)` consistently (note the later use already uses `length ??
precision`), leaving the DataFieldAttributeFactory and addArg logic unchanged.
🧹 Nitpick comments (2)
packages/cli/src/actions/pull/provider/mysql.ts (2)
286-289:@updatedAtheuristic is name-based and could produce false positives.Any
DateTimecolumn namedupdated_atorupdatedat(case-insensitive) will get@updatedAtapplied, even if the column is manually managed and not auto-updated by the application. This is a reasonable convention, but consider documenting this behavior or making it opt-in via a CLI flag so users aren't surprised.
156-174: Inconsistent indentation.Lines 158–162 use 14-space indentation while surrounding code uses 16 spaces (4-level nesting at 4 spaces each). Minor, but worth aligning for consistency.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/cli/src/actions/pull/provider/postgresql.ts`:
- Line 617: The WHERE clause currently uses a substring regex (!~
'_prisma_migrations') on "cls"."relname" which can wrongly exclude tables
containing that substring; update the condition to match the exact table name by
replacing it with either an equality check ("cls"."relname" !=
'_prisma_migrations') or an anchored regex ("cls"."relname" !~
'^_prisma_migrations$') in the SQL that builds the query (look for the clause
referencing "cls"."relname" and '_prisma_migrations' in postgresql.ts) so only
the exact _prisma_migrations table is excluded.
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
db pullto introspect MySQL/Postgres/SQLite and generate ZenStack models with output, casing, mapping, quoting, and indent options.New API
Bug Fixes
Tests
Chores