TML-1662: Store column native type in contract#30
Conversation
WalkthroughReplaces per-column Changes
Sequence Diagram(s)Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
packages/sql/sql-runtime/src/codecs/validation.ts (2)
37-44: Rename variable for terminology consistency.The variable
typeIdshould be renamed tocodecIdto align with the new contract schema terminology introduced in this PR.Apply this diff:
- const invalidCodecs: Array<{ table: string; column: string; typeId: string }> = []; + const invalidCodecs: Array<{ table: string; column: string; codecId: string }> = []; - for (const [key, typeId] of typeIds.entries()) { - if (!registry.has(typeId)) { + for (const [key, codecId] of typeIds.entries()) { + if (!registry.has(codecId)) { const parts = key.split('.'); const table = parts[0] ?? ''; const column = parts[1] ?? ''; - invalidCodecs.push({ table, column, typeId }); + invalidCodecs.push({ table, column, codecId }); } }
56-56: Update error message terminology.The error message refers to "typeIds" but should use "codecIds" for consistency with the new schema terminology.
Apply this diff:
throw runtimeError( 'RUNTIME.CODEC_MISSING', - `Missing codec implementations for column typeIds: ${invalidCodecs.map((c) => `${c.table}.${c.column} (${c.typeId})`).join(', ')}`, + `Missing codec implementations for column codecIds: ${invalidCodecs.map((c) => `${c.table}.${c.column} (${c.codecId})`).join(', ')}`, details, );Note: The interpolation
c.typeIdshould also change toc.codecIdif the earlier refactor is applied.packages/sql/lanes/orm-lane/src/mutations/insert-builder.ts (1)
87-107: Relying oncolumnMeta.codecIdinvariant – consider guarding for missing codec IDsSwitching from
columnMeta.typetocolumnMeta.codecIdand always writingparamCodecs[paramName] = codecIdassumes every column now has a non‑emptycodecId. That matches the new StorageColumn shape, but it also means:
- If any contract/column lacks
codecId(e.g. legacy or malformed contracts),paramCodecswill containundefineddespite itsRecord<string, string>type.- Downstream consumers of
meta.annotations.codecswill now see entries for every param instead of just truthy ones.If the invariant “all SQL columns have a codecId” is guaranteed by validation, this is fine; otherwise, you may want to add an explicit guard (throw or omit) when
columnMeta.codecIdis missing to fail fast with a clearer error.packages/sql/lanes/sql-lane/test/sql-includes.builder.advanced.test.ts (1)
17-54: Consider introducing type aliases for column descriptors.The expanded column type definitions are verbose. Consider introducing reusable type aliases for common column types to reduce duplication:
type Int4Column = { readonly nativeType: 'int4'; readonly codecId: 'pg/int4@1'; nullable: false }; type TextColumn = { readonly nativeType: 'text'; readonly codecId: 'pg/text@1'; nullable: false };This would make the type definitions more concise and maintainable.
packages/sql/lanes/orm-lane/test/mutations/delete-builder.test.ts (1)
2-14:int4Columnreuse looks good, but “without codecId” test no longer matches its nameCentralizing
{ nativeType, codecId, nullable }intoint4Columnand reusing it in both contracts keeps the test aligned with the newStorageColumnshape. However,contractandcontractWithCodecnow have identicalidcolumn metadata (both withcodecId), so the tests “without codecId” and “with codecId” exercise the same scenario. Consider either:
- Removing/renaming the “without codecId” test, or
- Using a different descriptor there if a truly codec-less path is still meant to be covered.
Also applies to: 31-35, 136-144
packages/targets/postgres-adapter/src/core/adapter.ts (1)
171-182: Vector handling now correctly keyed off codecId; consider small cleanupBasing vector casts in
renderParam,renderInsert, andrenderUpdateoncolumnMeta.codecId === 'pg/vector@1'is consistent with the new column shape and withexpr.forTypeIdinrenderOperation. As a minor cleanup, you might want to (a) extract'pg/vector@1'into a shared constant to avoid typos across these sites, and (b) either wire callers to passtableName/columnNameintorenderParamor drop that unused vector branch there to reduce dead code.Also applies to: 349-383
packages/framework/tooling/cli/test/emit-cli-process.e2e.test.ts (1)
19-54: Update round‑trip assertions to comparenativeType/codecIdinstead of legacytypeTyping
EmittedContract.user.columns.id/emailas{ nativeType; codecId; nullable }is aligned with the newStorageColumnshape and withvalidateContract<EmittedContract>.However, in the round‑trip test you still treat columns as
{ type?: string }and comparecolumns['id']?.typeandcolumns['email']?.typeto the original contract. With the new normalization logic, thetypefield is removed, so these assertions effectively becomeundefined === undefinedand no longer validate that column metadata survives the TS → JSON → validate round‑trip.Consider asserting on
codecId(and optionallynativeType) instead, e.g.:- if (userTable && originalUserTable) { - const columns = userTable['columns'] as Record<string, { type?: string }> | undefined; - const originalColumns = originalUserTable['columns'] as - | Record<string, { type?: string }> - | undefined; - if (columns && originalColumns) { - expect(columns['id']?.type).toBe(originalColumns['id']?.type); - expect(columns['email']?.type).toBe(originalColumns['email']?.type); - } - } + if (userTable && originalUserTable) { + const columns = userTable['columns'] as + | Record<string, { nativeType?: string; codecId?: string }> + | undefined; + const originalColumns = originalUserTable['columns'] as + | Record<string, { nativeType?: string; codecId?: string }> + | undefined; + if (columns && originalColumns) { + expect(columns['id']?.codecId).toBe(originalColumns['id']?.codecId); + expect(columns['email']?.codecId).toBe(originalColumns['email']?.codecId); + expect(columns['id']?.nativeType).toBe(originalColumns['id']?.nativeType); + expect(columns['email']?.nativeType).toBe(originalColumns['email']?.nativeType); + } + }This keeps the intent of the test (emitted contract matches the TS contract) while reflecting the new column representation.
Also applies to: 166-180
packages/sql/lanes/sql-lane/test/sql-joins.on.test.ts (1)
14-46: Minor inconsistency:nullablemissingreadonlymodifier.The
nativeTypeandcodecIdfields are markedreadonly, butnullableis not. Per theStorageColumntype definition inpackages/sql/contract/src/types.ts, all three fields should bereadonly.readonly id: { readonly nativeType: 'int4'; readonly codecId: 'pg/int4@1'; - nullable: false; + readonly nullable: false; }; readonly email: { readonly nativeType: 'text'; readonly codecId: 'pg/text@1'; - nullable: false; + readonly nullable: false; };Apply the same fix to all column definitions in the
posttable (lines 35, 40, 45).
| With this refactor, the SQL family’s contract IR becomes a **complete description of both database structure and codec configuration**. Tools that only have: | ||
|
|
||
| - The contract (including the DB-stored marker) and | ||
| - A live schema snapshot | ||
|
|
||
| can perform meaningful structural verification and migration planning purely from native types, while query builders and runtime remain codec-aware using the same contract. This aligns the contract more closely with the actual database while still serving as a single source of truth for application-level configuration.*** | ||
|
|
There was a problem hiding this comment.
Remove trailing artifact.
Line 435 contains a stray *** that appears to be an editing artifact. Remove it for cleaner documentation.
-can perform meaningful structural verification and migration planning purely from native types, while query builders and runtime remain codec-aware using the same contract. This aligns the contract more closely with the actual database while still serving as a single source of truth for application-level configuration.***
+can perform meaningful structural verification and migration planning purely from native types, while query builders and runtime remain codec-aware using the same contract. This aligns the contract more closely with the actual database while still serving as a single source of truth for application-level configuration.📝 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.
| With this refactor, the SQL family’s contract IR becomes a **complete description of both database structure and codec configuration**. Tools that only have: | |
| - The contract (including the DB-stored marker) and | |
| - A live schema snapshot | |
| can perform meaningful structural verification and migration planning purely from native types, while query builders and runtime remain codec-aware using the same contract. This aligns the contract more closely with the actual database while still serving as a single source of truth for application-level configuration.*** | |
| With this refactor, the SQL family's contract IR becomes a **complete description of both database structure and codec configuration**. Tools that only have: | |
| - The contract (including the DB-stored marker) and | |
| - A live schema snapshot | |
| can perform meaningful structural verification and migration planning purely from native types, while query builders and runtime remain codec-aware using the same contract. This aligns the contract more closely with the actual database while still serving as a single source of truth for application-level configuration. |
🤖 Prompt for AI Agents
In docs/briefs/Sql-Contract-Native-and-Codec-Types.md around lines 429 to 435,
remove the stray trailing artifact "***" at the end of the paragraph so the
sentence ends cleanly; update the line to end with the period and no extra
asterisks, and ensure no other unintended markup remains on that line.
| /** | ||
| * Column type descriptors for pgvector extension. | ||
| * | ||
| * These descriptors provide both codecId and nativeType for use in contract authoring. | ||
| * They are derived from the same source of truth as codec definitions and manifests. | ||
| */ | ||
|
|
||
| import type { ColumnTypeDescriptor } from '@prisma-next/contract-authoring'; | ||
|
|
||
| export const vectorColumn: ColumnTypeDescriptor = { | ||
| codecId: 'pg/vector@1', | ||
| nativeType: 'vector', | ||
| } as const; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
vectorColumn descriptor is well-typed and matches pgvector semantics
Exporting vectorColumn as a ColumnTypeDescriptor with codecId: 'pg/vector@1' and nativeType: 'vector' cleanly exposes the vector column type for contract authoring. The extra as const is harmless but not strictly necessary. Otherwise this looks solid.
🤖 Prompt for AI Agents
In packages/extensions/pgvector/src/exports/column-types.ts lines 1 to 13,
remove the unnecessary "as const" assertion on the exported vectorColumn object;
keep the explicit ColumnTypeDescriptor typing and the existing properties
(codecId and nativeType) unchanged so the descriptor remains well-typed without
the redundant const assertion.
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.ts
Outdated
Show resolved
Hide resolved
| readonly id: { | ||
| readonly nativeType: 'int4'; | ||
| readonly codecId: 'pg/int4@1'; | ||
| nullable: false; | ||
| }; | ||
| readonly title: { | ||
| readonly nativeType: 'text'; | ||
| readonly codecId: 'pg/text@1'; | ||
| nullable: false; | ||
| }; | ||
| readonly userId: { | ||
| readonly nativeType: 'int4'; | ||
| readonly codecId: 'pg/int4@1'; | ||
| nullable: false; | ||
| }; | ||
| readonly createdAt: { | ||
| readonly nativeType: 'timestamptz'; | ||
| readonly codecId: 'pg/timestamptz@1'; | ||
| nullable: false; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Same readonly inconsistency in post table columns.
Apply the same readonly modifier fix to nullable properties in the post table columns for consistency.
🤖 Prompt for AI Agents
In packages/sql/lanes/sql-lane/test/fixtures/contract-with-relations.d.ts around
lines 35 to 54, the post table column definitions currently use readonly on keys
like id/title/userId/createdAt but their nullable properties lack the readonly
modifier; update each column's nullable line to be "readonly nullable:
<boolean>;" to match the other readonly modifiers and maintain consistency
across the contract definitions.
packages/sql/tooling/emitter/test/emitter-hook.structure.test.ts
Outdated
Show resolved
Hide resolved
| export const textColumn: ColumnTypeDescriptor = { | ||
| codecId: 'pg/text@1', | ||
| nativeType: 'text', | ||
| } as const; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant as const with explicit type annotation.
The explicit : ColumnTypeDescriptor type annotation widens the literal types back to string, making the as const assertion ineffective. If you want literal types for better type inference (e.g., 'pg/text@1' instead of string), remove the type annotation. If string types are sufficient, remove as const.
Option 1 - Keep literal types (preferred for type inference):
-export const textColumn: ColumnTypeDescriptor = {
+export const textColumn = {
codecId: 'pg/text@1',
nativeType: 'text',
-} as const;
+} as const satisfies ColumnTypeDescriptor;Option 2 - Use string types:
export const textColumn: ColumnTypeDescriptor = {
codecId: 'pg/text@1',
nativeType: 'text',
-} as const;
+};🤖 Prompt for AI Agents
In packages/targets/postgres-adapter/src/exports/column-types.ts around lines 10
to 13, the object uses both an explicit type annotation (: ColumnTypeDescriptor)
and an as const assertion which is redundant because the annotation widens the
literals back to string; to keep literal types (preferred) remove the explicit
type annotation so the as const preserves literal types (i.e., leave the object
with as const and no : ColumnTypeDescriptor), or if you prefer string types
remove the as const and keep the : ColumnTypeDescriptor annotation—pick one of
these two changes and apply it consistently.
| import { Client } from 'pg'; | ||
| import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| const textColumn: StorageColumn = { nativeType: 'text', codecId: 'pg/text@1', nullable: false }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider importing the text column descriptor from the adapter to reduce duplication.
The postgres adapter exports a textColumn descriptor with the same nativeType and codecId values. You could import and extend it to avoid hardcoding these values:
+import { textColumn as textColumnDescriptor } from '@prisma-next/adapter-postgres/column-types';
+
-const textColumn: StorageColumn = { nativeType: 'text', codecId: 'pg/text@1', nullable: false };
+const textColumn: StorageColumn = { ...textColumnDescriptor, nullable: false };This approach reuses the canonical type definition and reduces maintenance burden if type identifiers change.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In test/integration/test/budgets.integration.test.ts around line 19, you're
duplicating the text column descriptor; instead import the canonical textColumn
from the Postgres adapter and use or extend it (e.g., spread it and set
nullable: false) so you reuse the adapter's definition and avoid hardcoding
nativeType/codecId values.
wmadden
force-pushed
the
column-native-type-in-contract
branch
from
232b2ac to
a9bdbac
Compare
wmadden
changed the title
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
.cursor/rules/prefer-assertions-over-defensive-checks.mdc (1)
93-100: Verify that the "before" pattern accurately represents the problematic code style.The "before" pattern at line 96 shows
const codecId = columnMeta?.codecId;with optional chaining. However, this is shown without the validation check that precedes it in the earlier "wrong" example. To maintain consistency, consider explicitly showing that this optional chaining occurs after validation has already checkedcontractTable.columns[columnName], making it unnecessary.This clarification would strengthen the refactoring pattern and make it clearer why optional chaining is problematic in this context.
Add validation context to the "before" pattern:
**Before:** ```typescript +// Validate first +if (!contractTable.columns[columnName]) { + errorUnknownColumn(columnName, tableName); +} + const columnMeta = contractTable.columns[columnName]; const codecId = columnMeta?.codecId; if (codecId && paramName) { paramCodecs[paramName] = codecId; }</blockquote></details> <details> <summary>packages/framework/core-control-plane/src/schema-view.ts (1)</summary><blockquote> `35-40`: **Clarify example meta: `nativeType` value looks like a codec ID** The example now shows `meta: { nativeType: 'pg/int4@1', nullable: false, ... }`, but across the SQL stack `nativeType` represents the DB type (e.g. `'int4'`/`'integer'`), while `'pg/int4@1'` is a codec/type ID. Consider updating this snippet to something like: ```ts // label: 'id: int4 (not null)', // meta: { nativeType: 'int4', codecId: 'pg/int4@1', nullable: false, ... }or otherwise clearly distinguishing codec vs native type to avoid confusion for implementers projecting SqlSchemaIR into this view.
[scratchpad_end] -->
packages/framework/tooling/cli/test/fixtures/valid-contract.ts (1)
1-14: Fixture now correctly uses column-type descriptorsSwitching the
column()calls to use{ type: int4Column | textColumn, nullable: false }matches the new ColumnTypeDescriptor-based builder API and ensurescodecId/nativeTypeare present in the built storage.Given the testing guidelines about avoiding tight coupling between fixtures and specific adapters, you may eventually want to mirror these descriptors inline (or via a tiny local helper) to keep this CLI fixture adapter-agnostic—but that’s non-blocking for this migration.
[scratchpad_end] -->
packages/sql/family/test/verify-database.test.ts (1)
4-48: Integration test contract now aligned with column descriptorsUsing
int4Column/textColumnincreateTestContract()keeps the test’s contract in sync with the ColumnTypeDescriptor-based builder API and ensures emitted contracts include bothnativeTypeandcodecId.This is consistent with the rest of the Postgres-specific wiring in this test (driver, adapter, real DB). If you want to tidy up later, you could also move the
CodecTypesimport to the adapter’s public exports for consistency, but that’s orthogonal to this change.[scratchpad_end] -->
packages/sql/lanes/sql-lane/test/sql-includes.builder.advanced.test.ts (1)
215-223: Avoid passingCodecTypesas a separate generic tosql()Calls like:
sql<ContractWithCapabilities, CodecTypes>({ context })and
sql<ContractWithoutCapabilities, CodecTypes>({ context: contextWithoutCaps })conflict with the guideline to never pass
CodecTypes(orOperationTypes) as separate type parameters toschema(),sql(), ororm(). The codec types should be extracted from the contract type instead.You can simplify to rely on the contract type alone, for example:
const tables = schema<ContractWithCapabilities>(context).tables; const plan = sql<ContractWithCapabilities>({ context }) .from(tables.user) // ... .build();and similarly for
ContractWithoutCapabilities. If you need codec types elsewhere, prefer the helper typeExtractCodecTypes<ContractWithCapabilities>rather than a second generic argument.Based on learnings from
.cursor/rules/type-extraction-from-contract.mdc.Also applies to: 243-245, 268-269, 292-293, 315-316, 340-341, 368-369, 395-396
test/integration/test/contract-builder.types.test-d.ts (1)
144-155: Replace deprecatedtoMatchTypeOf()withtoExtend<Row>()The
toMatchTypeOf()matcher is deprecated in Vitest'sexpectTypeOfAPI. UsetoExtend<Row>()instead to verify that the inferred type conforms to theRowtype:expectTypeOf(_testRow).toExtend<Row>();Note: This pattern appears in multiple test files across the codebase and should be updated consistently.
packages/sql/lanes/sql-lane/test/fixtures/contract-with-relations.json (1)
51-96: Remove adapter-specific CodecTypes import from fixture type definitions.The fixture type files (
contract.d.tsandcontract-with-relations.d.ts) importCodecTypesfrom@prisma-next/adapter-postgres/codec-types, creating an adapter dependency that violates the testing guide requirement: "Test fixtures should not depend on specific adapters" and "DefineCodecTypesinline in test fixtures to remain adapter-agnostic."Define
CodecTypesinline within each fixture's.d.tsfile instead of importing from the adapter package.packages/sql/lanes/orm-lane/test/selection/predicates.test.ts (2)
205-228: Consider removing test for impossible state.Per coding guidelines, tests should not use type assertions to bypass type checking and test impossible states. This test uses
{ nullable: false } as unknown as StorageColumnto create an invalid state wherecodecIdis missing. IfStorageColumnrequirescodecId, this scenario cannot occur in production.If this tests defensive handling of legacy data, add a comment explaining the scenario; otherwise, consider removing this test.
230-253: Consider removing test for impossible state.Similar to the previous test, this uses
{ codecId: 'pg/int4@1' } as unknown as StorageColumnto bypass type checking. IfnativeTypeandnullableare required fields, this scenario cannot occur with valid contracts.packages/framework/tooling/cli/test/db-verify.e2e.test.ts (1)
313-407: loadConfig mocking is effective; consider restoring spy for future testsThe PN‑CLI‑4010 test’s dynamic
import('../src/config-loader')plusvi.spyOn(..., 'loadConfig').mockResolvedValue(...)is a good way to bypass validation and simulate a missing driver.If more tests are ever added in this file, you may want to:
- Move
vi.restoreAllMocks()intoafterEach(or restore this specific spy), soloadConfigisn’t left mocked beyond this test.Not urgent with the current test layout but worth keeping in mind for maintainability.
packages/sql/lanes/relational-core/test/schema.test.ts (1)
219-235: columnMeta assertions align with new codecId-based metadataThe
column builder has columnMeta propertytest now checksidColumn.columnMeta.codecId === 'pg/int4@1'andnullable === false, which matches the new metadata model. If you ever need stronger guarantees here, you could also assertcolumnMeta.nativeType === 'int4', but the current checks are already sufficient for this suite.packages/sql/lanes/relational-core/src/types.ts (1)
69-99: AnyColumnBuilderBase + union keep column builders assignable despite conditional opsIntroducing
AnyColumnBuilderBaseand definingAnyColumnBuilderas a union of the genericColumnBuilder<..., any>and this base shape is a good way to sidestep conditional‑type incompatibilities across differentOperationsmaps while still preserving the key column properties and methods. The comment explaining theanyonOperationssatisfies the documented pattern.
♻️ Duplicate comments (11)
packages/sql/lanes/orm-lane/test/selection/ordering.test.ts (1)
22-22: OrderBuilder generics correctly useStorageColumn; explicit parameters are slightly redundantCasting to
OrderBuilder<string, StorageColumn, unknown>keeps these tests aligned with the updatedOrderBuilderdefinition that treatsStorageColumnas the column meta type, which is what you want under the new contract shape.Since
OrderBuilderalready defaultsColumnNametostringandColumnMetatoStorageColumn, you could optionally simplify these to justOrderBuilder(orOrderBuilder<string>if you want to keep the column-name type explicit) to reduce repetition, but this is purely stylistic and not required.Also applies to: 55-55, 77-77, 102-102, 135-135, 182-182
packages/sql/lanes/relational-core/test/column-builder-operations.test.ts (1)
20-20: Inconsistent column format - update vector column to match the new structure.The vector column still uses the old
typeformat while the id and email columns on lines 18-19 have been updated to use the newnativeTypeandcodecIdstructure. This inconsistency will cause validation or runtime failures.Apply this diff to align with the new format:
- vector: { type: 'pgvector/vector@1', nullable: false }, + vector: { nativeType: 'vector', codecId: 'pgvector/vector@1', nullable: false },packages/sql/family/src/core/instance.ts (1)
1285-1315: Guard against undefined contractNativeType in consistency check.When
contractNativeTypeis undefined (handled as a fail at lines 1217-1238), the consistency check at line 1301 can still execute and produce a misleading warning comparing againstundefined.Apply this diff to skip the consistency warning when contractNativeType is missing:
- } else if (typeMetadata.nativeType && typeMetadata.nativeType !== contractNativeType) { + } else if (contractNativeType && typeMetadata.nativeType && typeMetadata.nativeType !== contractNativeType) {packages/sql/tooling/emitter/test/emitter-hook.structure.test.ts (1)
556-606: Missing negative test coverage for nativeType and codecId validation.The tests cover negative cases for
nullable(missing field at line 556, invalid type at line 582), but there are no corresponding tests for missing or invalidnativeTypeandcodecIdfields. Per the past review comment, adding these tests would provide symmetrical coverage for all three required column fields.packages/sql/lanes/relational-core/test/fixtures/contract.d.ts (1)
21-23: Inconsistentreadonlymodifier onnullablefield.The
nativeTypeandcodecIdfields usereadonlymodifier butnullabledoes not. For consistency with the immutable contract pattern, consider addingreadonlytonullableas well.- readonly id: { readonly nativeType: 'int4'; readonly codecId: 'pg/int4@1'; nullable: false }; - readonly email: { readonly nativeType: 'text'; readonly codecId: 'pg/text@1'; nullable: false }; - readonly createdAt: { readonly nativeType: 'timestamptz'; readonly codecId: 'pg/timestamptz@1'; nullable: false }; + readonly id: { readonly nativeType: 'int4'; readonly codecId: 'pg/int4@1'; readonly nullable: false }; + readonly email: { readonly nativeType: 'text'; readonly codecId: 'pg/text@1'; readonly nullable: false }; + readonly createdAt: { readonly nativeType: 'timestamptz'; readonly codecId: 'pg/timestamptz@1'; readonly nullable: false };packages/sql/lanes/sql-lane/src/sql/mutation-builder.ts (1)
134-146: Simplify codec assignment;codecIdis guaranteed by the contract typeReading
columnMeta.codecIdis correct, but the extracodecIdchecks:const codecId = columnMeta?.codecId; if (paramName && codecId) { paramCodecs[paramName] = codecId; } paramDescriptors.push({ // ... ...(codecId ? { type: codecId } : {}), });are redundant because
StorageColumn.codecIdis a required string. For consistency with the ORM lane and to trust validated contracts, you can:
- Drop
&& codecIdfrom the guard and assignparamCodecs[paramName] = codecIdoncecolumnMetais known.- Set
type: codecIdunconditionally inparamDescriptors.This keeps the logic simpler and avoids implying that
codecIdmight be missing.Also applies to: 283-295
packages/extensions/pgvector/src/exports/column-types.ts (1)
1-13:vectorColumndescriptor matches the new contract-authoring patternThe exported
vectorColumnhas the expected{ codecId: 'pg/vector@1', nativeType: 'vector' }shape and is correctly checked againstColumnTypeDescriptor. The extraas constis redundant but harmless.test/integration/test/fixtures/contract.d.ts (1)
20-34: Column definitions correctly updated to new shape withnativeTypeandcodecId.The fixture now uses the enriched column descriptor format. Note: The inconsistent
readonlymodifier onnullablewas already flagged in a previous review comment.packages/sql/lanes/orm-lane/test/fixtures/contract-with-relations.d.ts (1)
12-54: Addreadonlymodifier tonullableproperties for consistency.The
nativeTypeandcodecIdproperties are marked asreadonly, butnullableis not. For consistency across the type definitions, addreadonlyto allnullableproperties.Apply this pattern to all affected columns:
readonly id: { readonly nativeType: 'int4'; readonly codecId: 'pg/int4@1'; - nullable: false; + readonly nullable: false; };packages/sql/lanes/sql-lane/test/fixtures/contract-with-relations.d.ts (1)
12-54: Addreadonlymodifier tonullableproperties for consistency.The
nativeTypeandcodecIdproperties are marked asreadonly, butnullableis not. For consistency across the type definitions, addreadonlyto allnullableproperties.Apply this pattern to all affected columns:
readonly id: { readonly nativeType: 'int4'; readonly codecId: 'pg/int4@1'; - nullable: false; + readonly nullable: false; };packages/targets/postgres-adapter/src/exports/column-types.ts (1)
8-53: Simplify column descriptor typing (as constvs: ColumnTypeDescriptor)Each descriptor is declared as
const X: ColumnTypeDescriptor = { ... } as const;. The explicit type annotation already widens the literals tostring, so theas constmainly adds redundancy and can be misleading if you expect literal types for better inference.To keep literal codec/native type values and still enforce the descriptor shape, prefer
satisfiesand drop the explicit annotation; for example:-import type { ColumnTypeDescriptor } from '@prisma-next/contract-authoring'; - -export const textColumn: ColumnTypeDescriptor = { - codecId: 'pg/text@1', - nativeType: 'text', -} as const; +import type { ColumnTypeDescriptor } from '@prisma-next/contract-authoring'; + +export const textColumn = { + codecId: 'pg/text@1', + nativeType: 'text', +} as const satisfies ColumnTypeDescriptor;Apply the same pattern to
int4Column,int2Column,int8Column,float4Column,float8Column,timestampColumn,timestamptzColumn, andboolColumnfor consistency.
packages/framework/tooling/cli/test/cli-integration-test-app/fixtures/emit-contract/contract.ts
Outdated
Show resolved
Hide resolved
packages/sql/lanes/sql-lane/test/sql-includes.builder.basic.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
packages/sql/lanes/relational-core/test/column-builder-operations.test.ts (1)
15-37: Use fully-typed contract type for proper type inference.The
validateContract<SqlContract<SqlStorage>>()call uses a generic type parameter, which prevents proper type inference throughout the test. Define the contract type inline usingtypeofor as a specific type to enable accurate type checking.Apply this diff:
- const contract = validateContract<SqlContract<SqlStorage>>({ + const contract = validateContract({ target: 'postgres', targetFamily: 'sql', coreHash: 'test-hash', storage: { tables: { user: { columns: { id: { ...int4ColumnType, nullable: false }, email: { ...textColumnType, nullable: false }, vector: { ...vectorColumnType, nullable: false }, }, primaryKey: { columns: ['id'] }, uniques: [], indexes: [], foreignKeys: [], }, }, }, models: {}, relations: {}, mappings: {}, - }); + } as const);Based on learnings, test fixtures should use fully-typed contracts for proper type inference rather than generic
SqlContract<SqlStorage>type parameters.packages/sql/lanes/orm-lane/test/selection/predicates.test.ts (1)
207-232: Avoid double-casting toStorageColumnfor impossible states in testsThe tests that do:
columnMeta: { nullable: false } as unknown as StorageColumn; columnMeta: { codecId: 'pg/int4@1' } as unknown as StorageColumn;manufacture
StorageColumnvalues that violate the type’s required fields just to exercise fallback behavior. This goes against the guidance to avoid using assertions to bypass type checking and test impossible states.Consider instead:
- Representing these as a dedicated partial meta type consumed by
buildWhereExpr, or- Using
// @ts-expect-errorwith a brief comment explaining the intentional invalid shape,so the intent is explicit and aligned with the test guidelines.
Also applies to: 234-257
packages/sql/authoring/sql-contract-ts/README.md (1)
35-37: Align documented source ofSqlContracttypes with current packageThe Architecture section still says SQL-specific contract types come from
@prisma-next/sql-target, while the Dependencies section (and code) use@prisma-next/sql-contract. For consistency with current imports (and the guideline to import SQL contract types from@prisma-next/sql-contract/types), consider updating that bullet to reference@prisma-next/sql-contract.Based on learnings, SQL contract types should come from
@prisma-next/sql-contract/types, not@prisma-next/sql-target.Also applies to: 91-95
packages/sql/authoring/sql-contract-ts/test/contract.integration.test.ts (1)
84-84: PrefertoEqualTypeOf/toExtendover deprecatedtoMatchTypeOfin type testsHere you’re asserting that
resulthas exactly the typeSqlContract<SqlStorage>. Vitest’sexpectTypeOf().toMatchTypeOf()is deprecated; usingtoEqualTypeOfis clearer and matches current guidance.- expectTypeOf(result).toMatchTypeOf<SqlContract<SqlStorage>>(); + expectTypeOf(result).toEqualTypeOf<SqlContract<SqlStorage>>();As per coding guidelines, Vitest type assertions should use
toExtend/toEqualTypeOfinstead oftoMatchTypeOf; please confirm against the version of Vitest in this repo.packages/sql/lanes/sql-lane/test/sql.test.ts (1)
84-93: ParamDescriptor expectation now matches codecId/nativeType-based metadataAsserting
{ name: 'userId', codecId: 'pg/int4@1', nativeType: 'int4', nullable: false, source: 'dsl', refs: { table: 'user', column: 'id' }, }keeps this test in sync with the new
ParamDescriptorshape and the column metadata coming from the contract.Based on learnings, a later cleanup could derive
CodecTypesfromContractviaExtractCodecTypes<Contract>instead of passingCodecTypesas a separate generic argument tosql(), but that’s orthogonal to this change.packages/sql/lanes/orm-lane/src/mutations/update-builder.ts (1)
70-83: Update plan now propagates codecId/nativeType into param metadataUsing
columnMeta.codecIdforparamCodecs[paramName]and passing bothcodecIdandnativeTypeintocreateParamDescriptoraligns update mutations with the new StorageColumn shape and with how select/insert paths annotate parameters.The only minor asymmetry is that
buildInsertPlannow throws if a column is missingcodecId, whilebuildUpdatePlanwould just propagate whatever is oncolumnMeta.codecId. If contracts are always validated/normalized first, this shouldn’t matter at runtime; otherwise, consider either mirroring the guard here or removing it in insert to rely solely on contract validation.packages/sql/lanes/orm-lane/src/mutations/insert-builder.ts (1)
94-109: Insert plan correctly annotates params with codecId/nativeType (with an extra guard)Reading
columnMeta.codecId, erroring when it’s missing, and then wiringcodecId/nativeTypeintocreateParamDescriptorplusparamCodecsensures insert plans have complete codec metadata consistent with the contract.If
validateContractguarantees that every SQLStorageColumnhas a non-emptycodecId/nativeType, this runtimeif (!codecId)check is mainly defensive and slightly diverges frombuildUpdatePlan, which doesn’t guard. You might either keep this guard and add a similar one in update, or rely on contract validation and simplify by trusting the types.packages/framework/tooling/emitter/test/emitter.test.ts (1)
146-146: Update test data to use new column format.This test uses the old
typefield instead ofcodecIdandnativeType. With the validation now checking forcodecId(line 37), this test will fail with "missing codecId" error instead of the intended namespace validation error.Apply this diff to fix the test data:
columns: { - id: { type: 'unknown/type@1', nullable: false }, + id: { codecId: 'unknown/type@1', nativeType: 'unknown_type', nullable: false }, },
♻️ Duplicate comments (4)
packages/sql/lanes/orm-lane/test/selection/projection.test.ts (1)
1-40: StorageColumn normalization and helper usage look consistent and correctImporting
StorageColumnfrom@prisma-next/sql-contract/types, centralizing conversion inconvertColumnMeta, and wiring it throughcreateMockColumnBuilderkeep all test column builders aligned with the new{ nativeType, codecId, nullable }shape. The updated projections and includes still only depend on aliases/columns, so this refactor is safe and improves consistency across tests.Also applies to: 98-195, 203-460
packages/sql/lanes/sql-lane/src/sql/mutation-builder.ts (1)
134-138: RedundantcodecIdcheck in guard condition.Since
StorageColumn.codecIdis a required string property (per the type definition), and the column existence is already validated at line 122-124,codecIdwill always be defined here. The&& codecIdcheck is unnecessarily defensive.This was previously flagged and marked as addressed, but the pattern remains. For consistency with ORM insert-builder (which only checks
paramName), consider:- if (paramName && codecId) { + if (paramName) { paramCodecs[paramName] = codecId; }The same applies to UpdateBuilderImpl at lines 286-288.
packages/sql/lanes/sql-lane/src/sql/predicate-builder.ts (2)
46-48: Type inconsistency between columnMeta declaration and fallback cast.Same issue as flagged in past review: Line 46 declares
codecId: stringas required, but line 48 casts the fallback to{ codecId?: string }with optionalcodecId. Consider aligning these types or importing a sharedColumnMetatype.
61-69: Descriptor update logic duplicated with orm-lane/predicates.ts.This exact pattern for updating descriptors with
codecIdandnativeTypeis duplicated inpackages/sql/lanes/orm-lane/src/selection/predicates.ts(lines 54-62). Consider extracting to a shared helper to reduce maintenance burden.
packages/sql/lanes/relational-core/test/operation-capabilities.test.ts
Outdated
Show resolved
Hide resolved
packages/sql/lanes/relational-core/test/operations-registry.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/sql/lanes/sql-lane/test/sql-includes.builder.basic.test.ts (2)
150-150: Remove explicitCodecTypestype parameter fromsql()builder calls.The
CodecTypesshould be extracted automatically from the contract rather than passed explicitly as a type parameter.As per coding guidelines, "Never pass
CodecTypesorOperationTypesas separate type parameters toschema(),sql(), ororm()functions."Apply this pattern to all
sql()calls:- const plan = sql<ContractWithCapabilities, CodecTypes>({ context }) + const plan = sql<ContractWithCapabilities>({ context })This applies to lines 150, 180, 208, 237, and 267.
Also applies to: 180-180, 208-208, 237-237, 267-267
165-170: Prefer object matcher for related assertions.Consider consolidating multiple individual assertions into a single object matcher for improved readability.
- const ast = plan.ast as SelectAst; - expect(ast?.includes).toBeDefined(); - expect(ast?.includes?.length).toBe(1); - expect(ast?.includes?.[0]?.kind).toBe('includeMany'); - expect(ast?.includes?.[0]?.alias).toBe('post'); - expect(ast?.includes?.[0]?.child.table.name).toBe('post'); - expect(ast?.includes?.[0]?.child.project.length).toBe(2); + const ast = plan.ast as SelectAst; + expect(ast.includes).toMatchObject([ + { + kind: 'includeMany', + alias: 'post', + child: { + table: { name: 'post' }, + project: expect.any(Array), + }, + }, + ]); + expect(ast.includes?.[0]?.child.project).toHaveLength(2);This pattern can also be applied to similar assertion groups at lines 196-198, 226-227, and 255-257.
♻️ Duplicate comments (3)
packages/framework/tooling/cli/test/db-introspect.test.ts (1)
117-157: Remove unnecessary block scopes inside remaining test functions.The first test correctly removed its block scope, but subsequent tests still have unnecessary nested
{ }blocks. These add indentation without providing variable isolation or other benefits.Apply this pattern consistently across all tests:
it('outputs summary when toSchemaView is not available', async () => { const testSetup = setupTestDirectoryFromFixtures( createTempDir, fixtureSubdir, 'prisma-next.config.with-db.ts', { '{{DB_URL}}': 'postgresql://user:pass@localhost/test' }, ); const configPath = testSetup.configPath; - { - const mockSchemaIR = { tables: { user: { columns: {} } } }; + const mockSchemaIR = { tables: { user: { columns: {} } } }; // ... rest of test without extra indentation - } });Also applies to: 169-216, 228-249, 261-277, 289-321, 333-373, 385-420, 432-470, 482-516, 528-561
packages/framework/tooling/cli/test/emit-command.e2e.test.ts (1)
213-222: Remove unnecessary CLI routing args and correct misleading comment.The second error test passes unnecessary routing args
['node', 'cli.js', 'emit', ...]toexecuteCommand, inconsistent with the corrected first error test (line 172) and all success tests. The comment on line 213 claiming this matches "emit-command.test.ts" is misleading—unit tests and E2E tests use different patterns. Commander.jsparseAsync()only needs actual CLI options.Apply this diff to fix the argument pattern and remove the misleading comment:
- // Commands don't throw - they call process.exit() with non-zero exit code - // executeCommand will catch the process.exit error and re-throw for non-zero codes - // Match the pattern from emit-command.test.ts: include command name in args await expect( - executeCommand(command, [ - 'node', - 'cli.js', - 'emit', - '--config', - 'prisma-next.config.ts', - '--json', - ]), + executeCommand(command, ['--config', 'prisma-next.config.ts', '--json']), ).rejects.toThrow('process.exit called');packages/sql/lanes/sql-lane/src/sql/predicate-builder.ts (1)
43-48: AligncolumnMetatyping with its actual usage and remove dead codecId field
columnMetais declared as{ codecId: string; nullable?: boolean }(Line 46), but immediately normalized viacolBuilder.columnMeta ?? {}and cast to{ codecId?: string; nullable?: boolean }(Line 48). That still leaves a mismatch between “required” vs “optional”codecId, and in practice onlynullableis consumed downstream.Since
codecIdfor the parameter now comes exclusively from the contract’sStorageColumn, you can simplify and make the types honest:
- Drop
codecIdfrom the localcolumnMetashape.- Avoid the awkward cast by pulling just
nullabledirectly.For example:
- const colBuilder = where.left as unknown as { - table: string; - column: string; - columnMeta?: { codecId: string; nullable?: boolean }; - }; - const meta = (colBuilder.columnMeta ?? {}) as { codecId?: string; nullable?: boolean }; + const colBuilder = where.left as unknown as { + table: string; + column: string; + columnMeta?: { nullable?: boolean }; + }; + const nullable = colBuilder.columnMeta?.nullable; @@ - refs: { table: colBuilder.table, column: colBuilder.column }, - ...(typeof meta.nullable === 'boolean' ? { nullable: meta.nullable } : {}), + refs: { table: colBuilder.table, column: colBuilder.column }, + ...(typeof nullable === 'boolean' ? { nullable } : {}),This removes the stale
codecIdfield from the builder-side meta and keeps the declared type in sync with how it is actually used.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
packages/framework/tooling/emitter/test/emitter.test.ts (1)
1-646: Split test file to comply with 500-line limit.This test file contains 603 lines and exceeds the 500-line maximum specified in the coding guidelines. Consider splitting it into multiple files organized by functionality:
emitter.basic.test.ts- Basic emission test (lines 78-138)emitter.validation.test.ts- CodecId and type validation tests (lines 140-342)emitter.structure.test.ts- Missing field validation tests (lines 344-645)Each file should include the necessary imports and the
mockSqlHooksetup.Based on learnings, test files must not exceed 500 lines.
packages/sql/lanes/relational-core/test/ast/join.test.ts (1)
104-113: Remove redundant assertions aftertoEqual.The
toEqualcheck already verifies all properties. The individualtoBeassertions on lines 110-113 are redundant and can be removed.expect(join).toEqual({ kind: 'join', joinType: 'inner', table, on, }); - expect(join.kind).toBe('join'); - expect(join.joinType).toBe('inner'); - expect(join.table).toBe(table); - expect(join.on).toBe(on);The same pattern appears at lines 160-167 in
createJoinOnExprtests.packages/sql/lanes/relational-core/test/operations-registry.test.ts (1)
1-824: Consider splitting this test file for better maintainability.At 824 lines, this file significantly exceeds the 500-line guideline. The 14 test cases could be logically organized into separate files by functionality: basic attachment tests, capability filtering tests, argument validation tests, and advanced operation tests. This would improve readability and maintainability without affecting the current column format migration.
packages/sql/authoring/sql-contract-ts/test/contract.integration.test.ts (2)
6-26: Consider defining a fully-typed contract inline for better type inference.The tests use
SqlContract<SqlStorage>, which is a generic type. Based on learnings, test files should define a fully-typed contract type inline to ensure proper type inference and better demonstrate the intended usage pattern.Consider defining a specific contract type:
type TestContract = SqlContract<{ tables: { User: { columns: { id: { codecId: 'pg/text@1'; nativeType: 'text'; nullable: false }; email: { codecId: 'pg/text@1'; nativeType: 'text'; nullable: false }; }; primaryKey: { columns: ['id'] }; uniques: []; indexes: []; foreignKeys: []; }; }; }>; const validContract = validateContract<TestContract>({ // ... rest of contract });This provides better type inference throughout the tests and matches the pattern used in production code where contracts are imported from
contract.d.ts.Based on learnings, test files should define fully-typed contracts inline rather than using generic types.
59-88: Test description contradicts implementation; approvetoEqualTypeOfchange.The test is named "accepts type parameter for strict contract type" and the comment states "The type parameter provides the strict type from contract.d.ts", but it uses the generic
SqlContract<SqlStorage>type instead of a fully-typed contract with literal types.To properly demonstrate the intended usage pattern where a strict type from
contract.d.tsis used, define a fully-typed contract type inline as suggested in the previous comment.However, the change from
toMatchTypeOftotoEqualTypeOfon line 84 is correct per coding guidelines.Based on learnings, tests should use fully-typed contracts to demonstrate the production usage pattern.
packages/sql/lanes/relational-core/test/schema.test.ts (1)
223-239: Column meta assertion now targetscodecId; consider also checkingnativeTypeUpdating the assertion from
columnMeta.typetocolumnMeta.codecIdwith expected'pg/int4@1'matches the new contract model and verifies the codec identifier is wired through correctly.You might optionally extend this to assert
idColumn.columnMeta.nativeType === 'int4'to cover the newnativeTypefield as well, but the current check is already valid.packages/sql/lanes/sql-lane/test/sql-includes.builder.basic.test.ts (1)
152-158: Remove explicitCodecTypesparameter fromsql()callsThe
sql()function has a default type parameter that automatically extracts codec types from the contract usingExtractCodecTypes<TContract>. PassingCodecTypesexplicitly as a second type parameter is redundant and violates the pattern. Change all instances from:-const plan = sql<ContractWithCapabilities, CodecTypes>({ context }) +const plan = sql<ContractWithCapabilities>({ context })Apply to lines 152, 187, 217, 249, and 284.
packages/sql/lanes/orm-lane/test/plan/plan-assembly.test.ts (1)
20-87: Consider splitting this test file to respect the 500-line guideline.This file now contains close to 900 lines covering
buildMeta,createPlan, andcreatePlanWithExists. Per the testing guidelines, test files should stay under ~500 lines and be split by logical area (e.g.,plan-assembly.meta.test.ts,plan-assembly.create-plan.test.ts,plan-assembly.exists.test.ts). Doing so would make these scenarios easier to scan and maintain.Also applies to: 88-768
♻️ Duplicate comments (5)
test/utils/README.md (2)
17-21: Update Responsibilities section to include Column Descriptors.The new Column Descriptors section is well-documented (lines 69-98), but the Responsibilities list doesn't mention this capability. Add "Column Descriptors" as a responsibility alongside Database Management and Async Iterable Utilities to keep the overview consistent with the detailed documentation.
Based on past review comments.
30-51: Add Column Descriptors node to architecture diagram.The mermaid diagram shows DB and ASYNC helpers but omits the newly added Column Descriptors capability. Add a COLUMN_DESCRIPTORS node (or similar) alongside the existing helper nodes to reflect the complete package architecture.
Based on past review comments.
packages/framework/tooling/cli/test/db-verify.e2e.test.ts (2)
169-171: Unused contract variable.
loadContractFromDiskis called but its result is not used. Either capture the result and add assertions, or remove the call if the emission success is validated elsewhere.// Load precomputed contract from disk const contractJsonPath = join(testDir, 'output', 'contract.json'); - loadContractFromDisk<SqlContract<SqlStorage>>(contractJsonPath); + const contract = loadContractFromDisk<SqlContract<SqlStorage>>(contractJsonPath); + expect(contract).toBeDefined();Or remove the lines if not needed for the test.
151-199: Remove unnecessary block scopes.Similar to the db-introspect tests, several e2e tests contain unnecessary nested
{ }blocks (lines 151-199, 219-290, 310-360, 380-479) that add indentation without purpose.Example for the second test:
it( 'reports error when marker is missing via driver', async () => { await withDevDatabase(async ({ connectionString }) => { const testSetup = setupTestDirectoryFromFixtures(...); const testDir = testSetup.testDir; const configPath = testSetup.configPath; - { - // Emit contract first - const emitCommand = createContractEmitCommand(); + // Emit contract first + const emitCommand = createContractEmitCommand(); // ... rest of test without extra indentation ... - } }); }, timeouts.spinUpPpgDev, );Also applies to: 219-290, 310-360, 380-479
packages/framework/tooling/cli/test/db-introspect.test.ts (1)
117-157: Remove remaining unnecessary block scopes.While the first test had its block scope removed (addressing the earlier review), tests 2-11 still contain unnecessary nested
{ }blocks that add indentation without serving a scoping purpose. For example, lines 117-157, 169-216, 228-249, etc.Example fix for the second test:
it('outputs summary when toSchemaView is not available', async () => { const testSetup = setupTestDirectoryFromFixtures( createTempDir, fixtureSubdir, 'prisma-next.config.with-db.ts', { '{{DB_URL}}': 'postgresql://user:pass@localhost/test' }, ); const configPath = testSetup.configPath; - { - const mockSchemaIR = { tables: { user: { columns: {} } } }; + const mockSchemaIR = { tables: { user: { columns: {} } } }; // ... rest of test logic without extra indentation ... - } });Apply the same pattern to the other affected tests.
Also applies to: 169-216, 228-249, 261-277, 289-321, 333-373, 385-420, 432-470, 482-516, 528-561
| ## Forbidden: Blind Type Casts | ||
|
|
||
| **CRITICAL RULE**: Blind type casts (`as unknown as X`) are **forbidden** in production code. Use type predicates instead. | ||
|
|
||
| ### The Rule | ||
|
|
||
| **❌ FORBIDDEN: Blind casts in production code** | ||
|
|
||
| ```typescript | ||
| // ❌ FORBIDDEN: Blind cast bypasses type checking | ||
| const colBuilder = where.left as unknown as { | ||
| table: string; | ||
| column: string; | ||
| }; | ||
|
|
||
| // ❌ FORBIDDEN: Double cast to force type assertion | ||
| const builder = (o as unknown as { user: () => unknown }).user(); | ||
| ``` | ||
|
|
||
| **✅ CORRECT: Use type predicates for proper type narrowing** | ||
|
|
||
| ```typescript | ||
| // ✅ CORRECT: Use type predicate to narrow the type | ||
| if (isColumnBuilder(where.left)) { | ||
| const { table, column } = getColumnInfo(where.left); | ||
| // TypeScript knows where.left is ColumnBuilder here | ||
| } | ||
|
|
||
| // ✅ CORRECT: Use type predicate to check and extract | ||
| const operationExpr = getOperationExpr(where.left); | ||
| if (operationExpr) { | ||
| // TypeScript knows operationExpr is OperationExpr here | ||
| } | ||
| ``` | ||
|
|
||
| ### Why Blind Casts Are Forbidden | ||
|
|
||
| 1. **Bypasses type safety**: Blind casts tell TypeScript to trust you without verification | ||
| 2. **Hides runtime errors**: Type mismatches won't be caught until runtime | ||
| 3. **Makes code brittle**: Changes to types won't be caught by the compiler | ||
| 4. **Violates type safety principles**: This codebase prioritizes type safety | ||
|
|
||
| ### Exception: Test Code Only | ||
|
|
||
| Blind casts are **only acceptable** in test files when: | ||
| - Creating test mocks that intentionally don't match the full interface | ||
| - Testing invalid inputs or error cases | ||
| - Working with dynamic proxy objects in test contexts | ||
|
|
||
| **✅ ACCEPTABLE: Blind casts in test files only** | ||
|
|
||
| ```typescript | ||
| // ✅ ACCEPTABLE: In test file, creating simplified mock | ||
| function createMockColumnBuilder(): AnyColumnBuilder { | ||
| return { | ||
| kind: 'column', | ||
| eq: () => ({ kind: 'binary', op: 'eq', left: {} as unknown, right: {} as unknown }), | ||
| } as unknown as AnyColumnBuilder; | ||
| } | ||
|
|
||
| // ✅ ACCEPTABLE: In test file, working with dynamic proxy | ||
| const builder = (o as unknown as { user: () => unknown }).user(); | ||
| ``` | ||
|
|
||
| **❌ FORBIDDEN: Blind casts in production code** | ||
|
|
||
| ```typescript | ||
| // ❌ FORBIDDEN: Production code must use type predicates | ||
| const colBuilder = where.left as unknown as { table: string; column: string }; | ||
| ``` | ||
|
|
||
| ### How to Replace Blind Casts | ||
|
|
||
| 1. **Create or use a type predicate**: Check if a value matches a type using a type guard function | ||
| 2. **Use helper functions**: Extract common type-checking logic into reusable helpers | ||
| 3. **Throw errors for invalid states**: If a value doesn't match expected types, throw an error | ||
|
|
||
| **Example: Replacing blind cast with type predicate** | ||
|
|
||
| ```typescript | ||
| // ❌ BEFORE: Blind cast | ||
| const colBuilder = where.left as unknown as { | ||
| table: string; | ||
| column: string; | ||
| }; | ||
|
|
||
| // ✅ AFTER: Type predicate with error handling | ||
| if (isColumnBuilder(where.left)) { | ||
| const { table, column } = getColumnInfo(where.left); | ||
| // Use table and column safely | ||
| } else { | ||
| errorFailedToBuildWhereClause(); // Throw error for invalid state | ||
| } | ||
| ``` | ||
|
|
||
| ### Creating Type Predicates | ||
|
|
||
| When you need to check if a value matches a type, create a type predicate: | ||
|
|
||
| ```typescript | ||
| /** | ||
| * Type predicate to check if a value is a ColumnBuilder. | ||
| */ | ||
| export function isColumnBuilder(value: unknown): value is AnyColumnBuilder { | ||
| return ( | ||
| typeof value === 'object' && | ||
| value !== null && | ||
| 'kind' in value && | ||
| (value as { kind: unknown }).kind === 'column' | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Helper to extract operation expression from builder. | ||
| * Returns OperationExpr if present, undefined otherwise. | ||
| */ | ||
| export function getOperationExpr( | ||
| builder: AnyColumnBuilder | OperationExpr, | ||
| ): OperationExpr | undefined { | ||
| if (isOperationExpr(builder)) { | ||
| return builder; | ||
| } | ||
| const builderWithExpr = builder as unknown as { _operationExpr?: OperationExpr }; | ||
| return builderWithExpr._operationExpr; | ||
| } | ||
| ``` | ||
|
|
||
| **Note**: The `getOperationExpr` helper still uses a cast internally, but it's: | ||
| - Centralized in one place | ||
| - Documented and tested | ||
| - Used consistently across the codebase | ||
| - Better than having blind casts everywhere | ||
|
|
||
| ### Related Patterns | ||
|
|
||
| - See "Type Predicates" section below for more examples | ||
| - See "Type Safety is Non-Negotiable" section for why type safety matters | ||
| - See "Test Mock Type Assertions" section for acceptable test patterns | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consolidate duplicated “Forbidden: Blind Type Casts” content and repeated notes.
The “Forbidden: Blind Type Casts” guidance appears twice with near-identical wording, and the NOTE in “Avoiding Unnecessary Type Casts” is duplicated verbatim. Consider keeping a single authoritative “Forbidden: Blind Type Casts” section and having other sections link to it, and drop the repeated NOTE line. This will make future edits less error‑prone and the rules easier to follow.
Also applies to: 619-764, 769-779
🤖 Prompt for AI Agents
In .cursor/rules/typescript-patterns.mdc around lines 480-618 (and also
duplicate occurrences at 619-764 and 769-779), the “Forbidden: Blind Type Casts”
section and a repeated NOTE are duplicated; consolidate into a single
authoritative section: remove the duplicated blocks at the additional ranges,
keep one canonical section (merge any unique content into it), replace the
removed duplicates with short cross-reference links or anchors pointing to the
canonical section, and delete the duplicated NOTE line so it appears only once
in the canonical location; ensure links/anchors are valid and update any
internal references accordingly.
packages/sql/lanes/relational-core/test/column-builder-operations.test.ts
Outdated
Show resolved
Hide resolved
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/framework/tooling/cli/test/emit-command.e2e.test.ts (1)
211-220: Inconsistent CLI argument pattern.This test passes extra routing args
['node', 'cli.js', 'emit', ...]while other tests in this file use the cleaner['--config', ...]format directly. TheexecuteCommandhelper uses{ from: 'user' }which expects user args only, not process.argv format.Apply this diff to match the pattern used in other tests (lines 52, 121, 169, 257, 288):
await expect( - executeCommand(command, [ - 'node', - 'cli.js', - 'emit', - '--config', - 'prisma-next.config.ts', - '--json', - ]), + executeCommand(command, ['--config', 'prisma-next.config.ts', '--json']), ).rejects.toThrow('process.exit called');
♻️ Duplicate comments (1)
packages/framework/tooling/cli/test/db-verify.e2e.test.ts (1)
168-170: Unused contract variable from disk load.The
loadContractFromDiskcall validates that the contract was emitted correctly, but the result is discarded. Either assign and use the result (like in lines 323-327), or add a comment explaining the intent is just validation.// Load precomputed contract from disk const contractJsonPath = join(testDir, 'output', 'contract.json'); - loadContractFromDisk<SqlContract<SqlStorage>>(contractJsonPath); + // Validate contract was emitted (result not needed for this test) + loadContractFromDisk<SqlContract<SqlStorage>>(contractJsonPath);Or remove if the validation isn't needed since the emit command already succeeded.
so that both codec id and native type are available at authoring time
wmadden
force-pushed
the
column-native-type-in-contract
branch
from
3fe63b9 to
23eeccf
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
packages/sql/contract/README.md (2)
70-84: Fix inconsistentcol()factory usage in examples.Line 71 correctly shows the three-argument form
col('int4', 'pg/int4@1', false), but lines 76–77 show only two arguments. Per the PR objectives,nullableis required for all columns. The table creation example must include the nullable parameter for consistency and correctness.Apply this diff to align the table example with the documented API:
const userTable = table( { - id: col('int4', 'pg/int4@1'), - email: col('text', 'pg/text@1'), + id: col('int4', 'pg/int4@1', false), + email: col('text', 'pg/text@1', false), },If some columns should be nullable, update accordingly—e.g.,
col('text', 'pg/text@1', true).
104-109: Add explicit note about public API change to Exports section.The Exports section should highlight that the
col(...)factory signature has changed to require three parameters(nativeType: string, codecId: string, nullable: boolean). This makes the API boundary change explicit for users reviewing the package exports.Add a note before the "## Exports" header or within that section. For example:
+### API Changes + +The `col(...)` factory function in `./exports/factories` now requires three parameters: + +```typescript +col(nativeType: string, codecId: string, nullable: boolean) +``` + +Both `nativeType` (e.g., `'int4'`) and `codecId` (e.g., `'pg/int4@1'`) are required to ensure contracts are consumable by both application and database layers. + ## Exportspackages/sql/schema-ir/README.md (1)
94-105: Add context on how nativeType is populated during introspection.The Schema Introspection section references adapters producing
SqlSchemaIR, but doesn't clarify hownativeTypeis extracted from database catalogs. Given the PR's emphasis onnativeTypeas the authoritative database-native type, a brief note explaining that adapters query information_schema (or equivalent) to populatenativeTypewould improve clarity for readers implementing new adapters.Consider adding a note like:
Adapters query database catalogs (e.g., PostgreSQL's `information_schema`) to extract native type strings (e.g., `'integer'`, `'text'`) and populate each column's `nativeType` field.packages/sql/lanes/orm-lane/test/mutations/update-builder.test.ts (1)
307-309: Consider using explicit assertion for descriptor existence.The optional chaining
emailDescriptor?.nullableis unnecessarily defensive in a test context. IfemailDescriptoris undefined, the test should fail explicitly.Option 1: Assert existence explicitly
const paramDescriptors = plan.meta.paramDescriptors; const emailDescriptor = paramDescriptors.find((d) => d.name === 'email'); -expect(emailDescriptor?.nullable).toBe(true); +expect(emailDescriptor).toBeDefined(); +expect(emailDescriptor!.nullable).toBe(true);Option 2: Use object matcher (preferred per coding guidelines)
-const paramDescriptors = plan.meta.paramDescriptors; -const emailDescriptor = paramDescriptors.find((d) => d.name === 'email'); -expect(emailDescriptor?.nullable).toBe(true); +const emailDescriptor = plan.meta.paramDescriptors.find((d) => d.name === 'email'); +expect(emailDescriptor).toMatchObject({ name: 'email', nullable: true });packages/framework/core-contract/test/ir.test.ts (1)
220-231: Consider consolidating assertions into a single object matcher.This test uses 10 individual
expect()calls to verify properties of the sameirobject. Per coding guidelines, prefer a single.toEqual()call when checking multiple related values.Apply this diff to consolidate the assertions:
- expect(ir.schemaVersion).toBe('1'); - expect(ir.target).toBe('postgres'); - expect(ir.targetFamily).toBe('sql'); - expect(ir.storage).toEqual(storage); - expect(ir.models).toEqual(models); - expect(ir.relations).toEqual(relations); - expect(ir.capabilities).toEqual({ - postgres: { returning: true }, - }); - expect(ir.extensions).toEqual({}); - expect(ir.meta).toEqual({}); - expect(ir.sources).toEqual({}); + expect(ir).toEqual({ + schemaVersion: '1', + target: 'postgres', + targetFamily: 'sql', + coreHash: 'sha256:abc123', + storage, + models, + relations, + capabilities: { + postgres: { returning: true }, + }, + extensions: {}, + meta: {}, + sources: {}, + });packages/sql/tooling/emitter/test/emitter-hook.types.test.ts (1)
115-133: Add test coverage for missing nativeType validation.The test fixture at line 121 has a column missing both
nativeTypeandcodecId, but only verifies the codecId error. According to the PR objectives, bothnativeTypeandcodecIdare required fields that must be validated by the emitter.Consider adding dedicated test cases for:
- Missing
nativeTypespecifically (column with onlycodecIdandnullable)- Invalid
nativeTypeformat or value (if applicable)Example test for missing nativeType:
it('throws error for missing nativeType', () => { const ir = createContractIR({ storage: { tables: { user: { columns: { id: { codecId: 'pg/int4@1', nullable: false }, }, }, }, }, }); const ctx: ValidationContext = { extensionIds: ['pg'], }; expect(() => { sqlTargetFamilyHook.validateTypes(ir, ctx); }).toThrow('is missing nativeType'); });packages/sql/lanes/orm-lane/test/mutations/delete-builder.test.ts (1)
174-191: Clarify or remove this test.The test name claims "throws error when where expr is missing", but the implementation verifies that the plan is successfully built. Additionally,
contextWithoutWhereis identical tocontext, so no special case is being tested. The comment acknowledges thatbuildWhereExpralways returns an expression, making this test redundant with the first test.Consider removing this test or rewriting it to test an actual edge case.
packages/sql/lanes/sql-lane/test/sql-includes.types.test-d.ts (1)
1-1: Consider defining CodecTypes inline per testing guidelines.While this type-only import is low-impact, the testing guidelines recommend defining
CodecTypesinline to keep test fixtures adapter-agnostic. For a type test file that's already PostgreSQL-specific (given the codec IDs used), this is a minor concern, but following the pattern would maintain consistency with other test files.Based on learnings, test fixtures should define CodecTypes inline rather than importing from specific adapters.
Example inline definition:
type CodecTypes = { 'pg/int4@1': number; 'pg/text@1': string; 'pg/timestamptz@1': string; // ... other types as needed };test/integration/test/emitter-lanes.integration.test.ts (1)
62-98: Use factory function for ContractIR construction.The test manually constructs a
ContractIRobject instead of using a factory function. Per coding guidelines, tests should use factory functions likecreateContractIR()to ensure all required fields are present.As per coding guidelines, use factory functions for ContractIR objects in tests.
packages/sql/lanes/sql-lane/src/sql/projection.ts (1)
94-99: Placeholder columnMeta shape looks correct; consider avoiding hard‑codedjsonbfor future targetsThe updated placeholder columnMeta aligns with the new
{ nativeType, codecId, nullable }shape and should let runtime codec collection pick upcore/json@1for includes, withnullable: truematching JSON aggregates that can beNULL. Functionally this looks fine.If you expect additional SQL targets later, you might want to derive
nativeTypefrom the JSON codec’s registry entry or the active adapter instead of hard‑coding'jsonb'here, so that include projections don’t implicitly assume a Postgres‑style type name.packages/sql/lanes/relational-core/src/operations-registry.ts (1)
191-200: Swapping operation lookup tocolumnMeta.codecIdis correct; the null-check is now mostly defensiveUsing
columnMeta.codecIdas the key forregistry.byType()aligns with howColumnBuilderand the SQL contract now model column types (operations are keyed by codec ID, not the oldtypefield). Given StorageColumn is Arktype‑validated and requires acodecId, theif (!typeId)early return should be unreachable in normal flows; it’s harmless, but you could drop it to better reflect the “trust validated data” guideline and simplify control flow:- // Use codecId if present - const typeId = columnMeta.codecId; - if (!typeId) { - return columnBuilder as ColumnBuilder<ColumnName, ColumnMeta, JsType, Operations>; - } - - const operations = registry.byType(typeId) as SqlOperationSignature[]; + // Use codecId as the operation type identifier + const typeId = columnMeta.codecId; + const operations = registry.byType(typeId) as SqlOperationSignature[];packages/sql/lanes/sql-lane/test/sql-includes.builder.basic.test.ts (1)
144-300: includeMany builder tests align with contract typing; tighten nullability assumptionsThe includeMany tests now:
- Use
sql<ContractWithCapabilities>({ context })without a separateCodecTypesgeneric, which matches the “single generic contract” pattern.- Verify default and custom aliases, as well as child
where,orderBy, andlimitclauses viatoMatchObjectand length/limit checks, making the assertions more robust to AST evolution.One small improvement: since
includeManymust always produce at least one entry inast.includes, you can replace optional chaining onincludeswith a non-null assertion so failures are more direct if the include is missing:- expect(ast.includes?.[0]?.child.project).toHaveLength(2); + expect(ast.includes![0]!.child.project).toHaveLength(2); - expect(ast.includes?.[0]?.child).toMatchObject({ + expect(ast.includes![0]!.child).toMatchObject({ where: { kind: 'bin', }, }); - expect(ast.includes?.[0]?.child).toMatchObject({ + expect(ast.includes![0]!.child).toMatchObject({ orderBy: [ { dir: 'desc', }, ], }); - expect(ast.includes?.[0]?.child.orderBy).toHaveLength(1); + expect(ast.includes![0]!.child.orderBy).toHaveLength(1); - expect(ast?.includes?.[0]?.child.limit).toBe(10); + expect(ast.includes![0]!.child.limit).toBe(10);This keeps the intent (“includes must be present for these tests”) explicit and avoids defensive optional access where the data is expected to exist. Please double-check against the current
SelectAst/includeManytypings and behavior.
♻️ Duplicate comments (13)
packages/framework/tooling/cli/test/emit-cli-process.e2e.test.ts (1)
180-183: Consider using object matcher for related assertions.Per coding guidelines, prefer
toMatchObject()when checking 2+ related values. The suggested refactor from the past review applies here.test/integration/test/emitter-lanes.integration.test.ts (2)
164-202: Use factory function for ContractIR construction.Same issue as in the first test: the
ContractIRobject is manually constructed instead of using a factory function.As per coding guidelines, use factory functions for ContractIR objects in tests.
249-285: Use factory function for ContractIR construction.Same issue as in previous tests: the
ContractIRobject is manually constructed instead of using a factory function.As per coding guidelines, use factory functions for ContractIR objects in tests.
examples/prisma-orm-demo/test/budgets.integration.test.ts (1)
22-38: Tighten helper typing and drop casts by constraining column keysThe helper extraction is the right direction, but the current generic/implementation still throws away some type information and relies on casts. You can keep the same runtime behavior while making the typing crisper and fully avoiding
as:
- Constrain the columns map to always have
idand- Use those keys directly instead of generic
Record<string, unknown>+ bracket access.-function getUserIdAndEmailColumns<T extends Record<string, unknown>>(userTable: { - columns: T; -}): { - idColumn: T['id']; - emailColumn: T['email']; -} { - const userColumns = userTable.columns; - const idColumn = userColumns['id']; - const emailColumn = userColumns['email']; - if (!idColumn || !emailColumn) { - throw new Error('Columns id or email not found'); - } - return { idColumn: idColumn as T['id'], emailColumn: emailColumn as T['email'] }; -} +function getUserIdAndEmailColumns<TColumns extends Record<'id' | 'email', unknown>>(userTable: { + columns: TColumns; +}): { + idColumn: TColumns['id']; + emailColumn: TColumns['email']; +} { + const { id, email } = userTable.columns; + if (!id || !email) { + throw new Error('Columns id or email not found'); + } + return { idColumn: id, emailColumn: email }; +}This keeps the helper generic while preserving concrete column descriptor types from
schema(context).tableswithout needing type assertions. Based on learnings about TypeScript patterns and type inference in helpers.packages/sql/tooling/emitter/test/emitter-hook.structure.test.ts (1)
556-567: Extend negative coverage tonativeType/codecIdvalidation, not justnullableThe structure tests exercise missing and invalid
nullablecases, but there are still no parallel cases for missing/invalidnativeTypeorcodecId, even though those are now required fields in the column schema. Consider adding tests that construct columns withoutnativeType/codecId, and with wrong types for those fields, to mirror the nullable coverage and ensure the validator’s error paths are tested for all three required properties.Also applies to: 582-605
.cursor/rules/typescript-patterns.mdc (1)
480-618: Consolidate the duplicated “Forbidden: Blind Type Casts” sectionsThe “Forbidden: Blind Type Casts” guidance appears twice with near-identical wording and examples; keeping a single canonical section (and linking to it from elsewhere if needed) would reduce maintenance overhead and avoid future inconsistencies between the duplicates.
Also applies to: 619-764
packages/sql/authoring/sql-contract-ts/test/helpers/column-descriptor.ts (1)
1-5: Helper is correct for current codecId formats; consider hardening for multi-segment IDsThe
columnDescriptorhelper correctly derivesnativeTypefrom codecIds likepg/int4@1and respects an explicitnativeTypeoverride, but for future codecIds with multiple/segments the regex will produce values likevector/foo. If you later introduce such ids, consider splitting on/and@(taking the last path segment before@) to keepnativeTypeclean.test/integration/test/fixtures/contract.d.ts (1)
20-34: Addreadonlymodifier tonullablefield for consistency.The
nativeTypeandcodecIdfields havereadonlymodifiers, butnullabledoes not. This inconsistency should be fixed by addingreadonlyto all threenullablefields.packages/extensions/pgvector/src/exports/column-types.ts (1)
10-13: Nitpick: Redundantas constassertion (duplicate).The
as constassertion on line 13 is redundant given the explicitColumnTypeDescriptortype annotation. This was already noted in a previous review comment.packages/sql/lanes/relational-core/test/fixtures/contract.d.ts (1)
21-23: Missingreadonlymodifier onnullablefield for consistency.The
nativeTypeandcodecIdfields use thereadonlymodifier, butnullabledoes not. For consistency with the immutable contract pattern, addreadonlyto thenullablefield as well.Apply this diff:
- readonly id: { readonly nativeType: 'int4'; readonly codecId: 'pg/int4@1'; nullable: false }; - readonly email: { readonly nativeType: 'text'; readonly codecId: 'pg/text@1'; nullable: false }; - readonly createdAt: { readonly nativeType: 'timestamptz'; readonly codecId: 'pg/timestamptz@1'; nullable: false }; + readonly id: { readonly nativeType: 'int4'; readonly codecId: 'pg/int4@1'; readonly nullable: false }; + readonly email: { readonly nativeType: 'text'; readonly codecId: 'pg/text@1'; readonly nullable: false }; + readonly createdAt: { readonly nativeType: 'timestamptz'; readonly codecId: 'pg/timestamptz@1'; readonly nullable: false };packages/sql/lanes/orm-lane/test/fixtures/contract-with-relations.d.ts (1)
15-15: Minor:nullablefield lacksreadonlymodifier for consistency.The
nativeTypeandcodecIdfields are markedreadonly, butnullableis not. This inconsistency appears throughout both the user and post table column definitions. Consider addingreadonlyfor consistency with the other test fixtures in this PR.Also applies to: 20-20, 25-25, 38-38, 43-43, 48-48, 53-53
packages/sql/lanes/sql-lane/test/fixtures/contract-with-relations.d.ts (1)
15-15: Minor:nullablefield lacksreadonlymodifier for consistency.Same as the issue noted in
orm-lane/test/fixtures/contract-with-relations.d.ts- thenullablefields should havereadonlymodifiers to matchnativeTypeandcodecId.Also applies to: 20-20, 25-25, 38-38, 43-43, 48-48, 53-53
packages/targets/postgres-adapter/src/exports/column-types.ts (1)
10-53: Redundantas constwith explicit type annotations.All column type descriptors combine explicit
: ColumnTypeDescriptortype annotations withas constassertions. The type annotation widens the literal types back tostring, making theas constineffective for preserving literal types like'pg/text@1'and'text'.For better type inference, use the
satisfiesoperator pattern:-export const textColumn: ColumnTypeDescriptor = { +export const textColumn = { codecId: 'pg/text@1', nativeType: 'text', -} as const; +} as const satisfies ColumnTypeDescriptor;Apply this pattern consistently to all descriptors (textColumn, int4Column, int2Column, int8Column, float4Column, float8Column, timestampColumn, timestamptzColumn, boolColumn).
| function createMockColumnBuilder( | ||
| table: string, | ||
| column: string, | ||
| columnMeta: { type: string; nullable: boolean }, | ||
| columnMeta: { codecId: string; nativeType: string; nullable: boolean }, | ||
| ): AnyColumnBuilder { | ||
| return { | ||
| kind: 'column', | ||
| table, | ||
| column, | ||
| columnMeta, | ||
| columnMeta: { | ||
| nativeType: columnMeta.nativeType, | ||
| codecId: columnMeta.codecId, | ||
| nullable: columnMeta.nullable, | ||
| }, | ||
| eq: () => ({ kind: 'binary', op: 'eq', left: {} as unknown, right: {} as unknown }), |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider typing columnMeta as StorageColumn in the mock helper
createMockColumnBuilder currently takes a { codecId; nativeType; nullable } literal type and reassembles it into the builder’s columnMeta. Importing StorageColumn and using it as the parameter type would avoid duplicating the shape and automatically track future changes to StorageColumn.
🤖 Prompt for AI Agents
In packages/sql/lanes/orm-lane/test/plan/plan-assembly.test.ts around lines 67
to 81, the createMockColumnBuilder helper types its columnMeta as an inline
literal ({ codecId; nativeType; nullable }) which duplicates the StorageColumn
shape; import the StorageColumn type from its module and change the columnMeta
parameter to use StorageColumn so the mock tracks future changes automatically,
adjust any property usage if names differ, and update imports accordingly.
| const columnMeta = contractTable.columns[columnName]; | ||
| const codecId = columnMeta?.type; | ||
| if (codecId && paramName) { | ||
| const codecId = columnMeta?.codecId; | ||
| if (paramName && codecId) { | ||
| paramCodecs[paramName] = codecId; | ||
| } | ||
|
|
||
| paramDescriptors.push({ | ||
| name: paramName, | ||
| source: 'dsl', | ||
| refs: { table: this.table.name, column: columnName }, | ||
| ...(codecId ? { type: codecId } : {}), | ||
| ...(codecId ? { codecId } : {}), | ||
| ...(columnMeta?.nativeType ? { nativeType: columnMeta.nativeType } : {}), | ||
| ...(columnMeta?.nullable !== undefined ? { nullable: columnMeta.nullable } : {}), | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Param metadata now correctly carries codecId/nativeType/nullable from StorageColumn
For both insert and update, pulling codecId and nativeType from contract.storage.tables[table].columns[columnName] and wiring them into paramCodecs and each ParamDescriptor aligns the mutation metadata with the new { nativeType, codecId, nullable } column shape. That gives downstream consumers enough information to choose codecs and reason about nullability.
If you ever want to tighten this further, you could rely on the normalized StorageColumn contract (post‑validation) and drop some of the defensive optional chaining around columnMeta/codecId in favor of direct destructuring, but the current implementation is functionally correct.
Also applies to: 284-297
|
|
||
| // Define CodecTypes type for contract mappings | ||
| type CodecTypes = Record<string, { readonly output: unknown }>; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
CodecTypes helper is fine; consider dropping the redundant comment
The CodecTypes alias is self-explanatory and matches the contract mappings shape. To follow the “code over comments” guideline, you can remove the preceding comment without losing clarity.
-// Define CodecTypes type for contract mappings
-type CodecTypes = Record<string, { readonly output: unknown }>;
+type CodecTypes = Record<string, { readonly output: unknown }>;📝 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.
| // Define CodecTypes type for contract mappings | |
| type CodecTypes = Record<string, { readonly output: unknown }>; | |
| type CodecTypes = Record<string, { readonly output: unknown }>; |
🤖 Prompt for AI Agents
In packages/sql/lanes/sql-lane/test/sql-includes.builder.basic.test.ts around
lines 10 to 12, the leading comment above the CodecTypes type alias is redundant
because the alias is self-explanatory; remove the comment line(s) so only the
type alias remains, leaving the code clear and concise.
| id: { nativeType: 'int4', codecId: 'pg/int4@1', nullable: false }, | ||
| email: { nativeType: 'text', codecId: 'pg/text@1', nullable: false }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract repeated column descriptors into shared helpers/constants
The pg/int4@1 and pg/text@1 column objects (and their nullable variant) are repeated many times across these tests. This is a good candidate for small helpers or shared constants to reduce duplication and keep future shape changes localized, for example:
const pgInt4NonNull = { nativeType: 'int4', codecId: 'pg/int4@1', nullable: false } as const;
const pgTextNonNull = { nativeType: 'text', codecId: 'pg/text@1', nullable: false } as const;
const pgTextNullable = { nativeType: 'text', codecId: 'pg/text@1', nullable: true } as const;
// ...
columns: {
id: pgInt4NonNull,
email: pgTextNonNull,
// ...
}This follows the testing guideline to introduce a helper once the same pattern appears 3+ times, while keeping the tests readable.
Also applies to: 215-216, 240-241, 265-266, 290-290, 299-300, 331-331, 340-341, 371-371, 403-405, 437-438, 468-468, 490-490
🤖 Prompt for AI Agents
In packages/sql/tooling/emitter/test/emitter-hook.generation.basic.test.ts
around lines 190-191 (also touching 215-216, 240-241, 265-266, 290, 299-300,
331, 340-341, 371, 403-405, 437-438, 468, 490), repeated column descriptor
objects for pg/int4@1 and pg/text@1 (nullable and non-nullable) should be
extracted into shared constants/helpers at the top of the test file; create
concise constants (e.g., pgInt4NonNull, pgTextNonNull, pgTextNullable)
typed/locked with as const, replace all duplicated inline objects with those
constants, and ensure any type assertions or imports still satisfy the test
types.
| id: { codecId: 'pg/int4@1', nativeType: 'int4', nullable: false }, | ||
| vector: { codecId: 'pg/vector@1', nativeType: 'vector', nullable: false }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Column descriptors correctly use { codecId, nativeType, nullable }, consider completing the contract
The new id/vector column descriptors match the updated storage contract shape and use fully qualified codecIds, which is good. The tests later reference additional columns on "user" (otherVector, email, active, data) that are not present in this contract; if lower() ever starts consulting column metadata for those (e.g. for casts), this fixture could become a hidden source of mismatch.
Consider adding descriptors for all referenced columns to keep the contract aligned with the AST used in the tests, even if they’re not strictly required today.
🤖 Prompt for AI Agents
In packages/targets/postgres-adapter/test/operation-lowering.test.ts around
lines 15 to 16, the column descriptor fixture only includes id and vector but
the tests later reference other columns on the "user" table (otherVector, email,
active, data); add descriptors for those columns to keep the storage contract
aligned with the AST used in tests. Add entries for otherVector, email, active,
and data using the same object shape { codecId, nativeType, nullable } with
fully qualified codecIds (e.g. pg/vector@1, pg/text@1, pg/bool@1, pg/jsonb@1 or
the correct codecs used elsewhere) and correct nullable flags to match the test
AST so future lowering that reads column metadata won’t mismatch the fixture.
| const makeColumn = (nativeType: string, codecId: string, nullable: boolean): StorageColumn => ({ | ||
| nativeType, | ||
| codecId, | ||
| nullable, | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Helper makeColumn keeps the fixture aligned with StorageColumn shape
This small factory cleanly enforces { nativeType, codecId, nullable } and makes the fixture easier to update as the column shape evolves.
If most columns stay non-nullable, you could optionally default nullable to false to reduce call-site noise:
const makeColumn = (
nativeType: string,
codecId: string,
nullable: boolean = false,
): StorageColumn => ({
nativeType,
codecId,
nullable,
});🤖 Prompt for AI Agents
In test/integration/test/codecs.integration.test.ts around lines 18 to 22, the
test fixture manually constructs column objects and should use a helper factory
to enforce the StorageColumn shape; add a makeColumn helper that returns {
nativeType, codecId, nullable } and replace inline objects with calls to it;
optionally give the nullable parameter a default of false (nullable: boolean =
false) so most call sites can omit the third argument.
| .column('id', { type: int4Column, nullable: false } as const) | ||
| .column('email', { type: textColumn, nullable: false } as const) | ||
| .column('createdAt', { type: timestamptzColumn, nullable: false } as const) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unnecessary as const assertions on column options.
The as const assertions appear unnecessary here since int4Column, textColumn, and timestamptzColumn are already typed as readonly descriptors with literal types. If these descriptors are defined with as const at their source, the assertions here are redundant.
t
- .column('id', { type: int4Column, nullable: false } as const)
- .column('email', { type: textColumn, nullable: false } as const)
- .column('createdAt', { type: timestamptzColumn, nullable: false } as const)
+ .column('id', { type: int4Column, nullable: false })
+ .column('email', { type: textColumn, nullable: false })
+ .column('createdAt', { type: timestamptzColumn, nullable: false })🤖 Prompt for AI Agents
In test/integration/test/contract-builder.integration.test.ts around lines 23 to
25, the column definitions use redundant "as const" assertions on the column
option objects; remove the trailing "as const" from each .column call so the
already-readonly column descriptors (int4Column, textColumn, timestamptzColumn)
are passed directly without the unnecessary assertion.
| id: { codecId: 'pg/int4@1', nativeType: 'int4', nullable: false }, | ||
| email: { codecId: 'pg/text@1', nativeType: 'text', nullable: false }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Column definitions correctly migrated to new shape.
The column definitions properly include both codecId and nativeType alongside nullable, matching the PR's objective to store column native types in contracts.
Consider adding explicit assertions to verify the emitted contract contains the expected nativeType and codecId fields:
const contract = validateContract(contractJson);
expect(contract.targetFamily).toBe('sql');
expect(contract.target).toBe('postgres');
expect(contract.storage).toBeDefined();
+
+// Verify new column shape is preserved in emitted contract
+expect(contract.storage.tables.user?.columns.id).toMatchObject({
+ codecId: 'pg/int4@1',
+ nativeType: 'int4',
+ nullable: false,
+});
+expect(contract.storage.tables.user?.columns.email).toMatchObject({
+ codecId: 'pg/text@1',
+ nativeType: 'text',
+ nullable: false,
+});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In test/integration/test/emitter-lanes.integration.test.ts around lines 85 to
86, the test currently shows the migrated column shape but lacks explicit
assertions for the new fields; add assertions that the emitted contract's
columns include for id: codecId === 'pg/int4@1', nativeType === 'int4', nullable
=== false, and for email: codecId === 'pg/text@1', nativeType === 'text',
nullable === false so the test verifies both codecId and nativeType are present
and correct.
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (17)
packages/sql/tooling/emitter/test/emitter-hook.generation.advanced.test.ts (2)
258-258: Duplicate test description.This test has the same description as the test on line 209:
"generates mappings type when models is undefined with codecTypes". Duplicate names cause confusion in test reports and make failures harder to identify.- it('generates mappings type when models is undefined with codecTypes', () => { + it('generates mappings type with codecTypes when packs defined after IR', () => {
1-637: Test file exceeds 500-line limit.Per coding guidelines, test files should not exceed 500 lines. This file is 637 lines. Consider splitting by logical groupings:
emitter-hook.generation.relations.test.ts(relation type generation tests)emitter-hook.generation.mappings.test.ts(mappings type generation tests)emitter-hook.generation.imports.test.ts(type imports extraction tests)packages/sql/authoring/sql-contract-ts/test/contract.logic.test.ts (1)
46-46: Consider defining a fully-typed contract for better type inference.All validateContract calls use the generic
SqlContract<SqlStorage>type parameter. Per learnings, defining a fully-typed contract (either imported from contract.d.ts or defined inline) enables better type inference. For tests, an inline fully-typed contract definition is acceptable and would align with established patterns.If refactoring, you could define the contract type once and reuse it across tests, or extract the validContractInput to use a const-asserted type:
type TestContract = SqlContract< SqlStorage & { tables: { User: { columns: { id: StorageColumn; email: StorageColumn; name: StorageColumn }; /* ... */ }; Post: { columns: { id: StorageColumn; userId: StorageColumn; title: StorageColumn }; /* ... */ }; }; } >;Also applies to: 70-70, 95-95, 120-120, 151-151, 182-182, 213-213, 244-244, 283-283, 322-322, 362-362, 385-385, 422-422, 489-489, 502-502, 513-513, 545-545, 562-562, 594-594
packages/sql/authoring/sql-contract-ts/test/contract.structure.test.ts (1)
28-31: Test implicitly verifies field requirements; consider explicit missing-field tests.The
validContractInputtemplate requiresnativeType,codecId, andnullableon all columns—you cannot construct a valid column without them. This implicitly proves these are required fields per the PR objectives. However, explicit tests for missingnativeTypeor missingcodecIdwould make field requirements more transparent to future maintainers, even though such tests are outside the scope of this PR's data migration.If desirable in a follow-up, consider adding tests like:
it('throws on missing nativeType', () => { const invalid = { ...validContractInput, storage: { tables: { User: { columns: { id: { codecId: 'pg/text@1', nullable: false } }, }, }, }, } as any; expect(() => validateContract<SqlContract<SqlStorage>>(invalid)).toThrow( /nativeType|Column.*validation failed/, ); });This would make the requirement explicit without adding production complexity.
Also applies to: 113-124
packages/sql/lanes/sql-lane/test/sql-includes.builder.advanced.test.ts (1)
220-220: RemoveCodecTypestype parameter fromsql()calls.The
sql()function should not receiveCodecTypesas a separate type parameter. According to coding guidelines,CodecTypesshould be extracted from the contract type internally using theExtractCodecTypes<Contract>helper.Apply this pattern to all
sql()calls:-sql<ContractWithCapabilities, CodecTypes>({ context }) +sql<ContractWithCapabilities>({ context })-sql<ContractWithoutCapabilities, CodecTypes>({ context }) +sql<ContractWithoutCapabilities>({ context })As per coding guidelines, the
sql()builder should extractCodecTypesfrom the contract type parameter rather than accepting it separately.Also applies to: 244-244, 268-268, 292-292, 315-315, 340-340, 368-368, 395-395
packages/sql/lanes/orm-lane/test/mutations/delete-builder.test.ts (1)
104-148: Optional: Consider consolidating annotation tests.The "builds delete plan with annotations" and "builds delete plan with codecId" tests use identical setup and both verify different parts of
plan.meta.annotations. You could combine them into a single test that checks all annotation fields together, reducing duplication while maintaining coverage.However, the current separation does provide clearer test isolation, making it easier to identify which specific annotation aspect fails. Given the file is well under the 500-line limit, either approach is acceptable.
packages/sql/lanes/sql-lane/test/sql-dml.integration.test.ts (1)
127-129: Type assertions suggest potential type system improvement.The repeated need to cast columns to
ColumnBuildertype indicates a type mismatch between schema column types and what.returning()expects. Consider whether the column type definitions can be improved to avoid these assertions.Also applies to: 233-234, 342-343
packages/sql/family/test/sign-database.test.ts (1)
24-24: Consider using a fully-typed contract type instead of genericSqlContract<SqlStorage>.The return type
SqlContract<SqlStorage>and its use invalidateContract<SqlContract<SqlStorage>>()may limit type inference benefits. For better type safety and inference, consider defining a specific contract type.Apply this pattern to improve type inference:
function createTestContract() { const contractObj = defineContract<CodecTypes>() .target('postgres') .table('user', (t) => t .column('id', { type: int4Column, nullable: false }) .column('email', { type: textColumn, nullable: false }) .primaryKey(['id']), ) .model('User', 'user', (m) => m.field('id', 'id').field('email', 'email')) .build(); - return { + const contract = { ...contractObj, extensions: { postgres: { version: '15.0.0', }, pg: {}, }, - }; + } as const; + + return contract; } + +type TestContract = ReturnType<typeof createTestContract>;Then use the specific type in validation:
-const validatedContract = validateContract<SqlContract<SqlStorage>>(contract); +const validatedContract = validateContract<TestContract>(contract);Based on learnings, using fully-typed contracts ensures proper type inference throughout the codebase.
Also applies to: 92-92
packages/sql/lanes/sql-lane/test/sql-includes.types.test-d.ts (1)
251-252: Avoid passingCodecTypesas a separate generic tosql()Across these tests,
sql<TestContractWithCapabilities, CodecTypes>(…)andsql<ContractWithComments, CodecTypes>(…)still passCodecTypesexplicitly as a second type parameter; per the type-extraction rules,sql()should derive codec types from the contract via helper types (e.g.,ExtractCodecTypes<Contract>) and only take the contract type as a generic. Consider simplifying these calls tosql<TestContractWithCapabilities>(…)/sql<ContractWithComments>(…)and letting the implementation pull codec types from the contract.Based on learnings, this will better align with
.cursor/rules/type-extraction-from-contract.mdc.Also applies to: 288-289, 323-324, 355-356, 391-392, 566-567, 618-619
packages/framework/authoring/contract-authoring/src/builder-state.ts (1)
1-19: ColumnTypeDescriptor and ColumnBuilderState runtime shape are consistent; consider typing nativeType generically laterThe new
ColumnTypeDescriptorand addednativeTypefield onColumnBuilderStatealign with the contract’s{ nativeType, codecId }model and give builder code the right runtime shape. If you later need type‑level tracking of native types (e.g.,'int4' | 'text'), consider extendingColumnBuilderState/ColumnBuilderwith a separateNativeType extends stringgeneric instead of plainstringhere.packages/sql/authoring/sql-contract-ts/test/contract.normalization.test.ts (1)
492-808: Consider switching tests away fromSqlContract<SqlStorage>as the validateContract type parameterThese tests still call
validateContract<SqlContract<SqlStorage>>(...), whereas the new validate-contract usage rule prefers a fully-typed contract type (inline or fromcontract.d.ts) so downstream tools can rely on precise table/column literals. For this file it’s mostly a type-level improvement, but future refactors would be easier if the tests defined a concrete contract type matching the fixture shapes instead of the genericSqlContract<SqlStorage>.Based on learnings, validateContract usage should rely on fully-typed contract types rather than
SqlContract<SqlStorage>.packages/sql/lanes/relational-core/src/types.ts (1)
32-56: codecId-based operation wiring and AnyColumnBuilder refactor look correctSwitching the ColumnBuilder operation-extension to key off
ColumnMeta['codecId']and look upOperationsForTypeIdaligns operation methods with codec IDs, which is how OperationTypes are keyed elsewhere. The introduction ofAnyColumnBuilderBaseand theAnyColumnBuilderunion cleanly sidestep variance/intersection incompatibilities caused by the conditionalOperationMethodsmixin, while preserving the structural fields (kind,table,column, etc.) used throughout the builders.packages/framework/tooling/emitter/README.md (1)
172-183: Example column shape correctly updated to{ nativeType, codecId, nullable }.The README example now matches the new
StorageColumnshape used by validators and contracts, which keeps the emitter docs consistent with the SQL contract schema.As per coding guidelines, keeping READMEs current with contract shapes is recommended.
packages/framework/tooling/cli/src/config-loader.ts (1)
29-40: Fix c12 configFile usage to pass filename without extension instead of absolute path.The equality check on line 31 (
result.configFile !== resolvedConfigPath) is unreliable because the code misuses c12's API. TheconfigFileoption should receive a filename without extension (e.g.,basename(resolvedConfigPath, extname(resolvedConfigPath))), not the full absolute path. This causesresult.configFileto potentially contain a different format thanresolvedConfigPath, making the strict equality comparison fail to detect when a different file was actually loaded.Correct the c12 call to:
const result = await loadConfigC12<PrismaNextConfig>({ name: 'prisma-next', ...(resolvedConfigPath ? { configFile: basename(resolvedConfigPath, extname(resolvedConfigPath)) } : {}), cwd: configCwd, });This ensures
result.configFilewill be comparable to the filename portion ofresolvedConfigPath, allowing the fallback-prevention check to work as intended.test/integration/test/contract-builder.types.test-d.ts (1)
154-154: UsetoExtend()instead of deprecatedtoMatchTypeOf().Per coding guidelines,
toMatchTypeOf()is deprecated in Vitest'sexpectTypeOfAPI.- expectTypeOf(_testRow).toMatchTypeOf<Row>(); + expectTypeOf(_testRow).toExtend<Row>();test/integration/test/contract-builder.integration.test.ts (1)
21-32: Inconsistentas constusage across tests.Lines 23-25 use
as conston column options, but similar definitions elsewhere (e.g., lines 126-128, 146-148) don't. Consider removingas consthere for consistency with other tests, or adding it everywhere if it serves a specific purpose for type inference.t - .column('id', { type: int4Column, nullable: false } as const) - .column('email', { type: textColumn, nullable: false } as const) - .column('createdAt', { type: timestamptzColumn, nullable: false } as const) + .column('id', { type: int4Column, nullable: false }) + .column('email', { type: textColumn, nullable: false }) + .column('createdAt', { type: timestamptzColumn, nullable: false })packages/sql/sql-runtime/src/codecs/validation.ts (1)
62-67: Consider adding JSDoc explaining the wrapper's purpose.
validateCodecRegistryCompletenessis a thin wrapper that currently just delegates. If it's intended as a stable public API entry point (allowing internal refactoring), a brief doc comment would clarify this intent.
♻️ Duplicate comments (20)
.cursor/rules/typescript-patterns.mdc (2)
769-769: Remove redundant duplicate NOTE lines.Lines 769 and 778 repeat the same note verbatim. Keep one instance (recommend line 769 before the section body begins, remove line 778).
Also applies to: 778-778
480-618: Consolidate duplicated "Forbidden: Blind Type Casts" sections.Lines 480-618 and 619-764 duplicate this critical rule with largely overlapping content. The second version (619-764) is more comprehensive with better examples (lines 712–719 expand on the AFTER case, and lines 753–757 clarify helper function patterns). Keep the improved version and remove the first.
After consolidation, update cross-references at lines 615–617 to point to the single canonical section location.
Also applies to: 619-764
examples/prisma-orm-demo/test/budgets.integration.test.ts (1)
22-38: Strengthen the generic constraint to eliminate type assertions.The current constraint
T extends Record<string, unknown>doesn't guarantee that'id'and'email'keys exist, forcing bracket access and type assertions. The assertions don't actually improve type safety since both sides areunknown.Use a more specific constraint to ensure the required keys exist at the type level, which allows dot notation and eliminates the need for assertions:
/** * Extracts id and email columns from user table, throwing if either is missing. */ -function getUserIdAndEmailColumns<T extends Record<string, unknown>>(userTable: { +function getUserIdAndEmailColumns<T extends Record<'id' | 'email', unknown>>(userTable: { columns: T; }): { idColumn: T['id']; emailColumn: T['email']; } { const userColumns = userTable.columns; - const idColumn = userColumns['id']; - const emailColumn = userColumns['email']; + const idColumn = userColumns.id; + const emailColumn = userColumns.email; if (!idColumn || !emailColumn) { throw new Error('Columns id or email not found'); } - return { idColumn: idColumn as T['id'], emailColumn: emailColumn as T['email'] }; + return { idColumn, emailColumn }; }This follows the coding guideline to remove unnecessary type casts when values are guaranteed by proper constraints.
packages/sql/lanes/orm-lane/test/utils/guards.test.ts (1)
19-23: Shared int4ColumnMeta fixture and columnMeta wiring are consistent.
int4ColumnMetacorrectly matches theStorageColumnshape (nativeType,codecId,nullable), and reusing it viacolumnMeta: int4ColumnMetain the column-builder-like objects keeps the guard tests exercising the new metadata contract without duplication.Based on learnings, this aligns with the migration to
{ nativeType, codecId, nullable }for storage columns.Also applies to: 195-203, 248-256, 260-267
packages/sql/family/src/core/instance.ts (1)
1212-1315: Tighten registry consistency check and avoidundefinedin type display.Behavior is functionally correct, but there are a couple of small polish points:
Skip registry consistency warnings when
nativeTypeis already missing.
WhencontractNativeTypeis falsy, we already push atype_mismatchfailure and mark the column asfail. The registry block can still run and emit atype_consistency_warningthat mentions"undefined", which is noisy and redundant.Avoid
undefinedleaking into display/actual values.
IfnativeTypeis missing on either side (a defensive “should not happen” path), the column label and extra-columnactualcan end up asundefined. A simple fallback keeps the tree readable while preserving the existing failure semantics.You can address both with a small refactor like:
- // Optionally validate that codecId (if present) and nativeType agree with registry - if (contractColumn.codecId) { + // Optionally validate that codecId (if present) and nativeType agree with registry + // Only run this when the contract has a nativeType; missing-nativeType is already a hard failure above. + if (contractColumn.codecId && contractNativeType) { const typeMetadata = typeMetadataRegistry.get(contractColumn.codecId); if (!typeMetadata) { @@ - } else if (typeMetadata.nativeType && typeMetadata.nativeType !== contractNativeType) { + } else if (typeMetadata.nativeType && typeMetadata.nativeType !== contractNativeType) { // Warning: codecId and nativeType don't agree with registry @@ - const nullableText = contractColumn.nullable ? 'nullable' : 'not nullable'; - // Format: columnName: nativeType (codecId) (nullability) - // Reuse contractNativeType from above scope - const columnTypeDisplay = contractColumn.codecId - ? `${contractNativeType} (${contractColumn.codecId})` - : contractNativeType; + const nullableText = contractColumn.nullable ? 'nullable' : 'not nullable'; + // Format: columnName: nativeType (codecId) (nullability) + // Prefer contract nativeType, fall back to schema/native "unknown" for defensive paths. + const displayNativeType = contractNativeType ?? schemaNativeType ?? 'unknown'; + const columnTypeDisplay = contractColumn.codecId + ? `${displayNativeType} (${contractColumn.codecId})` + : displayNativeType; @@ - expected: undefined, - actual: schemaColumn.nativeType, + expected: undefined, + actual: schemaColumn.nativeType ?? 'unknown',This keeps the main verification behavior the same, but avoids redundant warnings and improves the readability of failure output when invariants are violated.
Also applies to: 1352-1356, 1423-1424
packages/sql/lanes/sql-lane/src/sql/mutation-builder.ts (1)
135-147: Remove redundantcodecIdcheck when populatingparamCodecsAfter the
errorUnknownColumnguard,contractTable.columns[columnName]is guaranteed to exist, andStorageColumn.codecIdis a required string, socodecIdcannot be missing here. The extra&& codecIdmakes this path more defensive than needed and diverges from the ORM lane pattern.You can simplify and make the two builders consistent by only checking
paramName:- const columnMeta = contractTable.columns[columnName]; - const codecId = columnMeta?.codecId; - if (paramName && codecId) { - paramCodecs[paramName] = codecId; - } + const columnMeta = contractTable.columns[columnName]; + const codecId = columnMeta.codecId; + if (paramName) { + paramCodecs[paramName] = codecId; + }Apply the same change in
UpdateBuilderImpl.build()for thesetloop.Also applies to: 284-297
test/integration/test/fixtures/contract.d.ts (1)
20-34: Makenullablereadonly for consistency in the fixture type
nativeTypeandcodecIdare markedreadonly, butnullableis not. For an immutable contract fixture, it’s cleaner and more consistent to makenullablereadonly as well (as in other fixtures in this repo).- readonly id: { - readonly nativeType: 'int4'; - readonly codecId: 'pg/int4@1'; - nullable: false; - }; - readonly email: { - readonly nativeType: 'text'; - readonly codecId: 'pg/text@1'; - nullable: false; - }; - readonly createdAt: { - readonly nativeType: 'timestamptz'; - readonly codecId: 'pg/timestamptz@1'; - nullable: false; - }; + readonly id: { + readonly nativeType: 'int4'; + readonly codecId: 'pg/int4@1'; + readonly nullable: false; + }; + readonly email: { + readonly nativeType: 'text'; + readonly codecId: 'pg/text@1'; + readonly nullable: false; + }; + readonly createdAt: { + readonly nativeType: 'timestamptz'; + readonly codecId: 'pg/timestamptz@1'; + readonly nullable: false; + };packages/framework/tooling/cli/test/fixtures/exact-prefix-import.ts (1)
2-2: Test fixture violates adapter-agnostic principle.This fixture imports column-type constants from
@prisma-next/adapter-postgres/column-types, creating a direct dependency on a specific adapter package.Based on learnings, test fixtures should remain adapter-agnostic and not depend on specific adapters. Define column descriptors inline instead:
-import { int4Column, textColumn } from '@prisma-next/adapter-postgres/column-types'; + +const int4Column = { codecId: 'pg/int4@1', nativeType: 'int4' } as const; +const textColumn = { codecId: 'pg/text@1', nativeType: 'text' } as const;packages/framework/tooling/cli/test/fixtures/valid-contract.ts (1)
2-2: Test fixture violates adapter-agnostic principle.This fixture imports column-type constants from
@prisma-next/adapter-postgres/column-types, creating a direct dependency on a specific adapter package.Based on learnings, test fixtures should remain adapter-agnostic and not depend on specific adapters. Define column descriptors inline instead:
-import { int4Column, textColumn } from '@prisma-next/adapter-postgres/column-types'; + +const int4Column = { codecId: 'pg/int4@1', nativeType: 'int4' } as const; +const textColumn = { codecId: 'pg/text@1', nativeType: 'text' } as const;packages/sql/authoring/sql-contract-ts/src/contract-builder.ts (1)
130-145: EnforcenativeTypepresence inbuild()to match normalization contractThe doc block states that
nativeTypeis a required, normalized field and the PR description says.build()should require it, but the implementation currently just forwardscolumnState.nativeTypeintoBuildStorageColumnunder a type assertion. If a column is defined without a descriptor (or if builder state forgets to setnativeType), this will silently emitnativeType: undefinedand only fail later during validation or runtime use.Given the builder is the single normalization boundary, it should either guarantee
columnState.nativeTypeis always a string or throw a clear error here. A small guard in the column loop would make this robust and developer-friendly:for (const columnName in tableState.columns) { const columnState = tableState.columns[columnName]; if (!columnState) continue; - const codecId = columnState.type; - const nativeType = columnState.nativeType; - - columns[columnName as keyof ColumnDefs] = { - nativeType, - codecId, + const codecId = columnState.type; + const nativeType = columnState.nativeType; + if (!nativeType) { + throw new Error( + `Cannot determine nativeType for codecId "${codecId}" in column "${columnName}" of table "${tableName}". ` + + 'Use a column type descriptor (for example int4Column or textColumn) from your adapter package.', + ); + } + + columns[columnName as keyof ColumnDefs] = { + nativeType, + codecId, nullable: (columnState.nullable ?? false) as ColumnDefs[keyof ColumnDefs]['nullable'] & boolean, } as BuildStorageColumn< ColumnDefs[keyof ColumnDefs]['nullable'] & boolean, ColumnDefs[keyof ColumnDefs]['type'] >; }This keeps the normalization responsibility local, prevents malformed
StorageColumnobjects, and aligns the implementation with the documented contract.Also applies to: 200-221
packages/sql/authoring/sql-contract-ts/test/helpers/column-descriptor.ts (1)
3-5: columnDescriptor helper is correct; multi-segment codecId caveat from earlier review still appliesImplementation matches
ColumnTypeDescriptorand works for codec IDs likepg/int4@1andpg/text@1. If you later introduce codec IDs with multiple/segments before@(e.g.pg/vector/someType@1), this regex will include slashes innativeType; at that point, consider switching to a split-based derivation (codecId.split('/')then strip the@suffix) while still honoring the explicitnativeTypeargument when provided.packages/sql/tooling/emitter/test/emitter-hook.structure.test.ts (1)
37-38: Column shapes updated correctly; still missing negative tests for nativeType/codecIdThe move to
{ nativeType, codecId, nullable }across all table definitions keeps these structure tests aligned with the new column contract. However, the validator now treatsnativeTypeandcodecIdas required, yet the negative coverage still only exercisesnullable(missing/invalid) whilenativeType/codecIdare never tested for absence or wrong type. Adding mirrored tests for:
- missing
nativeType- non-string
nativeType- missing
codecId- non-string
codecIdwould close the gap and ensure all three required fields are enforced symmetrically.
Also applies to: 94-95, 233-234, 262-263, 284-285, 306-307, 329-331, 352-363, 389-400, 426-437, 463-474, 489-491, 509-510, 540-541, 562-566, 588-592, 614-615, 636-637, 658-659, 689-690, 745-746, 754-756, 817-818, 840-841, 880-882, 890-893
test/utils/README.md (1)
69-97: New descriptor docs and exports align with utilities; consider updating the architecture diagramThe Column/Operation Descriptor sections and the updated exports list accurately describe and expose the new test helpers while keeping test-utils dependency-free on other
@prisma-next/*packages. To fully reflect current responsibilities, consider extending the mermaid diagram to add nodes for “Column Descriptors” and “Operation Descriptors” alongside DB/Async helpers so the high-level architecture stays in sync with the documented components.Also applies to: 99-127, 204-208
packages/framework/tooling/cli/test/emit-command.e2e.test.ts (1)
151-193: Error path for missing config uses consistent argument shape and assertionsThe missing-config test now uses the same
setupTestDirectoryFromFixturessetup and passes only CLI options toexecuteCommand, while still asserting on non-zero exit code 2 and the PN-CLI-4001 JSON error object; this resolves previous concerns about extra args while keeping coverage intact.packages/sql/lanes/relational-core/test/fixtures/contract.d.ts (1)
21-23: Consider makingnullablereadonly for consistency
nativeTypeandcodecIdare readonly butnullableis not. For a fully immutable fixture type, you may wantreadonly nullable: falseon these three column descriptors as well.packages/targets/postgres-adapter/src/exports/column-types.ts (1)
10-53: Redundantas constwith explicit type annotation.The explicit
: ColumnTypeDescriptortype annotation widens the literal types back tostring, making theas constassertion ineffective. To preserve literal types for better type inference while still validating the structure, usesatisfies:-export const textColumn: ColumnTypeDescriptor = { +export const textColumn = { codecId: 'pg/text@1', nativeType: 'text', -} as const; +} as const satisfies ColumnTypeDescriptor;Apply this pattern to all column type descriptors in the file.
packages/sql/lanes/orm-lane/test/fixtures/contract-with-relations.d.ts (1)
12-26: Inconsistentreadonlymodifier onnullableproperty.The
nativeTypeandcodecIdproperties are marked asreadonly, butnullableis not. This inconsistency appears in all column definitions. Consider addingreadonlyfor consistency with other test files in this PR (e.g.,join.test.ts).readonly id: { readonly nativeType: 'int4'; readonly codecId: 'pg/int4@1'; - nullable: false; + readonly nullable: false; };Also applies to: 35-54
packages/sql/lanes/sql-lane/test/fixtures/contract-with-relations.d.ts (1)
12-26: Inconsistentreadonlymodifier onnullableproperty.The
nativeTypeandcodecIdproperties are marked asreadonly, butnullableis not. This was flagged in a previous review and marked as addressed, but the code still shows the inconsistency. Apply the samereadonlymodifier fix to allnullableproperties for consistency with other test files.readonly id: { readonly nativeType: 'int4'; readonly codecId: 'pg/int4@1'; - nullable: false; + readonly nullable: false; };Also applies to: 35-54
packages/framework/tooling/cli/test/db-verify.e2e.test.ts (1)
168-170: Unused return value from loadContractFromDisk.The contract is loaded but not used. If this is intentional to verify emission succeeded, consider adding a comment. Otherwise, either remove the call or add a minimal assertion.
// Load precomputed contract from disk const contractJsonPath = join(testDir, 'output', 'contract.json'); - loadContractFromDisk<SqlContract<SqlStorage>>(contractJsonPath); + // Verify contract was emitted successfully (value not needed for this test) + loadContractFromDisk<SqlContract<SqlStorage>>(contractJsonPath);packages/framework/tooling/cli/test/utils/test-helpers.ts (1)
250-261: RedundantmkdirSynccalls removed - addressed.The past review comment about redundant
mkdirSynccalls has been addressed with a clarifying comment on line 251. The code now correctly reuses theoutputDirthat was already created on line 235.
| With this refactor, the SQL family’s contract IR becomes a **complete description of both database structure and codec configuration**. Tools that only have: | ||
|
|
||
| - The contract (including the DB-stored marker) and | ||
| - A live schema snapshot | ||
|
|
||
| can perform meaningful structural verification and migration planning purely from native types, while query builders and runtime remain codec-aware using the same contract. This aligns the contract more closely with the actual database while still serving as a single source of truth for application-level configuration. | ||
|
|
||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove trailing blank line.
Line 435 appears to contain only the change marker with no content. The document should end cleanly after the concluding paragraph.
can perform meaningful structural verification and migration planning purely from native types, while query builders and runtime remain codec-aware using the same contract. This aligns the contract more closely with the actual database while still serving as a single source of truth for application-level configuration.
-📝 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.
| With this refactor, the SQL family’s contract IR becomes a **complete description of both database structure and codec configuration**. Tools that only have: | |
| - The contract (including the DB-stored marker) and | |
| - A live schema snapshot | |
| can perform meaningful structural verification and migration planning purely from native types, while query builders and runtime remain codec-aware using the same contract. This aligns the contract more closely with the actual database while still serving as a single source of truth for application-level configuration. | |
| With this refactor, the SQL family's contract IR becomes a **complete description of both database structure and codec configuration**. Tools that only have: | |
| - The contract (including the DB-stored marker) and | |
| - A live schema snapshot | |
| can perform meaningful structural verification and migration planning purely from native types, while query builders and runtime remain codec-aware using the same contract. This aligns the contract more closely with the actual database while still serving as a single source of truth for application-level configuration. |
🤖 Prompt for AI Agents
In docs/briefs/Sql-Contract-Native-and-Codec-Types.md around lines 428 to 435,
remove the trailing blank line/change marker at line 435 so the file ends
immediately after the concluding paragraph; ensure there is no extra empty line
or stray diff marker at EOF and save the file with a single newline at end if
your editor enforces it.
packages/framework/tooling/cli/test/fixtures/custom-allowlist.ts
Outdated
Show resolved
Hide resolved
packages/framework/tooling/cli/test/fixtures/valid-contract-default.ts
Outdated
Show resolved
Hide resolved
| }; | ||
|
|
||
| const plan = buildDeletePlan(contextWithCodec, 'User', where, getModelAccessor, { | ||
| const plan = buildDeletePlan(context, 'User', where, getModelAccessor, { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent type parameter usage across tests.
Line 137 omits explicit type parameters on buildDeletePlan, relying on inference, while the first test (lines 79-80) still includes them. For consistency, either use explicit type parameters in all test cases or remove them from all cases where inference works.
🤖 Prompt for AI Agents
In packages/sql/lanes/orm-lane/test/mutations/delete-builder.test.ts around line
137, the call to buildDeletePlan omits the explicit generic type parameter while
the earlier test includes it; make the usage consistent by adding the same
explicit type parameter (e.g., <User>) to the buildDeletePlan invocation on line
137 so both tests use explicit generics, or alternatively remove the explicit
generic from the earlier test — prefer adding <User> at line 137 for minimal
change.
| // Use codecId if present | ||
| const typeId = columnMeta.codecId; | ||
| if (!typeId) { | ||
| return columnBuilder as ColumnBuilder<ColumnName, ColumnMeta, JsType, Operations>; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider failing fast (or at least surfacing) when codecId is missing
The switch to columnMeta.codecId is correct, but the if (!typeId) { return columnBuilder; } path silently drops all operations for that column. Given StorageColumn now comes from validated SQL contracts and should always carry a codecId, a missing value likely indicates a contract/metadata bug.
You may want to:
- Either treat a missing
codecIdas aplanInvaliderror, or - At minimum, surface a warning/diagnostic instead of quietly returning a bare builder.
This would make misconfigured columns easier to diagnose than “operations just don’t appear.”
As per coding guidelines, once Arktype has validated the contract, we generally trust the shape and prefer assertions/fail‑fast over silent fallbacks.
🤖 Prompt for AI Agents
In packages/sql/lanes/relational-core/src/operations-registry.ts around lines
196 to 200, the current code silently returns the bare columnBuilder when
columnMeta.codecId is missing; instead fail fast: replace the early return with
an assertion or throw a planInvalid-style error (or at minimum emit a diagnostic
warning) indicating that codecId is required on StorageColumn — include the
column name/context in the error message so contract/metadata bugs are surfaced
immediately.
| for (const [key, codecId] of codecIds.entries()) { | ||
| if (!registry.has(codecId)) { | ||
| const parts = key.split('.'); | ||
| const table = parts[0] ?? ''; | ||
| const column = parts[1] ?? ''; | ||
| invalidCodecs.push({ table, column, typeId }); | ||
| invalidCodecs.push({ table, column, codecId }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Edge case: table/column names containing dots.
The key format ${tableName}.${columnName} and subsequent key.split('.') assumes table/column names don't contain dots. While PostgreSQL identifiers rarely contain dots, this could produce incorrect results if they do.
Consider using a different delimiter or storing a tuple instead:
-const key = `${tableName}.${columnName}`;
-codecIds.set(key, codecId);
+codecIds.set(`${tableName}\0${columnName}`, codecId);
...
-const parts = key.split('.');
-const table = parts[0] ?? '';
-const column = parts[1] ?? '';
+const [table, column] = key.split('\0') as [string, string];Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/sql/sql-runtime/src/codecs/validation.ts around lines 39 to 44, the
code assumes keys are in the form "table.column" and uses key.split('.') which
breaks when either identifier contains dots; change the representation or
parsing: ideally change whoever builds codecIds to store [tableName, columnName]
tuples (so this loop can destructure into table and column without splitting),
and if changing the upstream structure is not feasible, parse the key by
locating the first '.' (indexOf) and using substring/slice to take the left side
as table and the remainder as column (or use split('.', 2)) to avoid splitting
on dots inside names; update this loop to extract table and column using one of
these approaches and push the correct values into invalidCodecs.
| const VECTOR_CODEC_ID = 'pg/vector@1' as const; | ||
|
|
There was a problem hiding this comment.
renderParam’s vector-specific branch is currently unreachable
Introducing VECTOR_CODEC_ID and using codecId in renderInsert/renderUpdate and renderOperation is solid. However, the new logic in renderParam:
function renderParam(ref, contract?, tableName?, columnName?) {
if (contract && tableName && columnName) {
const tableMeta = contract.storage.tables[tableName];
const columnMeta = tableMeta?.columns[columnName];
if (columnMeta?.codecId === VECTOR_CODEC_ID) {
return `$${ref.index}::vector`;
}
}
return `$${ref.index}`;
}is never exercised, because all call sites in this file only pass (ref, contract), not tableName/columnName. That means simple predicates like embedding = $1 still won’t cast the param to vector, despite the new code.
Either:
- Wire the left-hand column into the call in
renderBinary, e.g. whenrightExpr.kind === 'param'andleftExpr.kind === 'col', callrenderParam(rightExpr, contract, leftExpr.table, leftExpr.column), or - Drop the extra parameters and columnMeta lookup if you don’t intend to support column‑driven casting here, relying solely on operation/insert/update paths.
As written, it’s dead code and may mislead future readers about behavior.
Also applies to: 167-182, 200-211, 351-385
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
packages/sql/authoring/sql-contract-ts/test/contract.normalization.test.ts (2)
768-773: Consider using object matchers for related assertions.Multiple individual property checks could be consolidated using
.toMatchObject()for clearer intent.Apply this diff to improve readability:
- expect(mappings.modelToTable?.['User']).toBe('user'); - expect(mappings.tableToModel?.['user']).toBe('User'); - expect(mappings.fieldToColumn?.['User']?.['id']).toBe('id'); - expect(mappings.fieldToColumn?.['User']?.['email']).toBe('email'); - expect(mappings.columnToField?.['user']?.['id']).toBe('id'); - expect(mappings.columnToField?.['user']?.['email']).toBe('email'); + expect(mappings).toMatchObject({ + modelToTable: { User: 'user' }, + tableToModel: { user: 'User' }, + fieldToColumn: { + User: { id: 'id', email: 'email' } + }, + columnToField: { + user: { id: 'id', email: 'email' } + } + });
1-859: Split this 859-line test file to comply with the 500-line maximum.This file significantly exceeds the 500-line guideline for test files. Consider splitting into:
contract.normalization.validation.test.ts- validateContract normalization tests (lines 10-738)contract.normalization.mappings.test.ts- computeMappings tests (lines 741-859)Alternatively, split the normalization tests by category (nullable/arrays, relations, top-level fields, edge cases).
Based on coding guidelines: "Test files must not exceed 500 lines; split files that exceed this limit into multiple files organized by logical functionality"
packages/sql/lanes/orm-lane/test/mutations/delete-builder.test.ts (2)
13-34: Good reuse of a typedStorageColumnconstantDefining
int4Column: StorageColumnwith{ nativeType: 'int4', codecId: 'pg/int4@1', nullable: false }and reusing it forstorage.tables.user.columns.idkeeps the contract aligned with the new{ nativeType, codecId, nullable }shape and makes the codec used in later assertions explicit and easy to update. One follow-up you may consider is validating this contract once in a setup helper withvalidateContractto ensure tests stay in lockstep with production contract expectations.Based on learnings about contract validation in tests.
104-129: Annotations test name and assertions now clearly target write intent/mutationRenaming the test to
'includes write intent and mutation flag in annotations'and assertingintent/isMutationvia a singletoMatchObjectkeeps the intent clear and concise. If you want to tighten the pattern further, you could first bindconst annotations = plan.meta.annotations!;and assert onannotations.intent/annotations.isMutationdirectly, avoiding optional chaining after confirming existence.As per coding guidelines on preferring assertions over defensive optional chaining once validity is established.
packages/sql/tooling/emitter/README.md (1)
11-17: README responsibilities now correctly scopevalidateStructureto logical checksThe updated description that
validateStructure()handles only SQL logical consistency (FKs, model/table wiring, constraint consistency) while Arktype enforces structural fields and types is aligned with the emitter’s intended responsibilities.packages/sql/tooling/emitter/test/emitter-hook.generation.basic.test.ts (1)
41-52: New nullable-undefined test conflicts with contract-normalization responsibilitiesThe test
"generates contract types with column nullable undefined defaults to false"(lines 506–540) constructs an IR wherenullableis explicitlyundefinedvia type assertion, then asserts that the emitter treats it as non-nullable. This violates the contract-normalization layer boundaries:
- Contracts passed to the emitter hook must already be fully normalized (all required fields present with correct types).
- Emitter hook tests must verify behavior on normalized contracts only, not test missing-field defaults or fallback logic.
- Normalization (setting defaults like
nullable: false) is the contract builder's responsibility; validation is the validator's responsibility; the emitter should assume normalized input.The defensive
column.nullable ?? falseingenerateModelsTypeenables this test to pass, but it masks a contract quality problem rather than solving it. Either:
- Remove this test and add coverage for "missing
nullable" scenarios to the SQL contract validator tests instead, or- Rewrite the test to use a normalized IR (with
nullable: falseexplicitly set) to verify the type generation itself.Then simplify the emitter to rely on validated
booleanfields without defensive fallbacks.
♻️ Duplicate comments (2)
packages/sql/lanes/sql-lane/test/fixtures/contract.d.ts (1)
15-29: Inconsistentreadonlymodifier onnullableproperty.The
nativeTypeandcodecIdproperties are declared asreadonly, butnullableis not. For type consistency and immutability, all three properties should bereadonly.Apply this diff to make
nullablereadonly:readonly id: { readonly nativeType: 'int4'; readonly codecId: 'pg/int4@1'; - nullable: false; + readonly nullable: false; }; readonly email: { readonly nativeType: 'text'; readonly codecId: 'pg/text@1'; - nullable: false; + readonly nullable: false; }; readonly createdAt: { readonly nativeType: 'timestamptz'; readonly codecId: 'pg/timestamptz@1'; - nullable: false; + readonly nullable: false; };packages/sql/lanes/relational-core/src/operations-registry.ts (1)
196-202: Fail fast instead of silently dropping operations whencodecIdis missingThis still returns the bare
columnBuilderwhencolumnMeta.codecIdis falsy, which silently disables all operations on that column. GivenStorageColumncomes from an Arktype‑validated SQL contract and must carry acodecId, hitting this branch likely indicates a contract/metadata bug; failing fast would be safer and easier to debug than “methods just don’t exist.”Consider throwing
planInvalidwith column context instead of returning early, for example:- // Use codecId to look up operations registered for this column's type - const codecId = columnMeta.codecId; - if (!codecId) { - return columnBuilder as ColumnBuilder<ColumnName, ColumnMeta, JsType, Operations>; - } - - const operations = registry.byType(codecId) as SqlOperationSignature[]; + // Use codecId to look up operations registered for this column's type + const codecId = columnMeta.codecId; + if (!codecId) { + throw planInvalid( + `Missing codecId for column "${columnBuilder.table}.${columnBuilder.column}" when attaching operations`, + ); + } + + const operations = registry.byType(codecId) as SqlOperationSignature[];This keeps behavior aligned with the assumption that contracts are validated and enforces a clear invariant at the boundary where it matters.
As per coding guidelines, once Arktype has validated the contract we should trust the shape and prefer assertions/fail‑fast over silent fallbacks.
| ## Schema Validation Redundancy | ||
|
|
||
| **CRITICAL**: Don't re-validate structural properties that are already validated by schema validators (e.g., Arktype). | ||
|
|
||
| **❌ WRONG: Re-validating properties already validated by Arktype** | ||
|
|
||
| ```typescript | ||
| // validateStructure() re-checking properties that Arktype already validates | ||
| for (const [colName, col] of Object.entries(table.columns)) { | ||
| if (typeof col.nullable !== 'boolean') { | ||
| throw new Error(`Column "${colName}" is missing required field "nullable"`); | ||
| } | ||
| if (!col.nativeType || typeof col.nativeType !== 'string') { | ||
| throw new Error(`Column "${colName}" is missing required field "nativeType"`); | ||
| } | ||
| if (!col.codecId || typeof col.codecId !== 'string') { | ||
| throw new Error(`Column "${colName}" is missing required field "codecId"`); | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| **Why this is wrong:** | ||
| - Arktype's `StorageColumnSchema` already validates `nullable`, `nativeType`, and `codecId` as required fields | ||
| - These checks create impossible code paths (contracts that pass Arktype validation but fail these checks) | ||
| - Requires awkward tests that bypass type checking to test impossible states | ||
| - Adds maintenance burden when schema changes | ||
|
|
||
| **✅ CORRECT: Focus on logical validation that schema validators can't do** | ||
|
|
||
| ```typescript | ||
| // validateStructure() focuses on logical consistency (references, relationships) | ||
| // Column structure is already validated by Arktype - no need to re-check here | ||
| for (const [tableName, table] of Object.entries(storage.tables)) { | ||
| const columnNames = new Set(Object.keys(table.columns)); | ||
|
|
||
| // Validate foreign key references (logical validation, not structural) | ||
| for (const fk of table.foreignKeys) { | ||
| for (const colName of fk.columns) { | ||
| if (!columnNames.has(colName)) { | ||
| throw new Error(`ForeignKey references non-existent column "${colName}"`); | ||
| } | ||
| } | ||
| // Validate referenced table exists (logical validation) | ||
| if (!tableNames.has(fk.references.table)) { | ||
| throw new Error(`ForeignKey references non-existent table "${fk.references.table}"`); | ||
| } | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| **Validation Responsibility Separation:** | ||
|
|
||
| - **Schema Validators (Arktype)**: Validate structural properties (required fields, types, shapes) | ||
| - **Logical Validators (`validateStructure`)**: Validate logical consistency (references, relationships, constraints) | ||
|
|
||
| **When to add validation checks:** | ||
| - ✅ Validating references (foreign keys, model references) | ||
| - ✅ Validating relationships (model-to-table mappings) | ||
| - ✅ Validating constraint consistency (primary key columns exist) | ||
| - ❌ Re-validating structural properties already validated by schema validators | ||
|
|
||
| **Benefits:** | ||
| - Eliminates redundant code and tests | ||
| - Clear separation of concerns (structural vs logical validation) | ||
| - Easier to maintain (schema changes don't require updating multiple validators) | ||
| - Better error messages (schema validator catches structural issues early) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Schema Validation Redundancy section is excellent architectural guidance.
The new "Schema Validation Redundancy" section (lines 152–217) clearly articulates the critical separation between structural validation (Arktype's responsibility) and logical validation (custom validators). The concrete examples of wrong vs. correct patterns, and the validation responsibility matrix, make the distinction actionable and maintainable. This aligns well with learnings on contract normalization and Arktype usage and directly supports the PR's codecId/nativeType changes.
Consider adding a brief cross-reference to .cursor/rules/arktype-usage.mdc to reinforce that this pattern applies wherever Arktype validators are in place.
🤖 Prompt for AI Agents
In .cursor/rules/prefer-assertions-over-defensive-checks.mdc around lines 152 to
217, add a brief cross-reference to .cursor/rules/arktype-usage.mdc to reinforce
that structural validation should be delegated to Arktype; insert a single
sentence at the end of the "Schema Validation Redundancy" section pointing
readers to the arktype-usage guideline (e.g., "See
.cursor/rules/arktype-usage.mdc for Arktype usage patterns and examples.") so
it's discoverable without duplicating content or changing the section's tone.
| it('adds table when callback returns undefined', () => { | ||
| const builder = defineContract(); | ||
| // Callback returns undefined - should fall back to using the original tableBuilder | ||
| const withTable = builder.table('user', () => undefined); | ||
| // Verify table was added by adding another table - if first wasn't added, this would fail | ||
| const withTwoTables = withTable.table('post', (t) => | ||
| t.column('id', { type: intColumn }).primaryKey(['id']), | ||
| ); | ||
| expect(withTwoTables).toBeInstanceOf(ContractBuilder); | ||
| expect(withTwoTables).not.toBe(withTable); | ||
| }); | ||
|
|
||
| it('adds model when callback returns undefined', () => { | ||
| const builder = defineContract(); | ||
| // First add a table for the model to reference | ||
| const withTable = builder.table('user', (t) => | ||
| t.column('id', { type: intColumn }).primaryKey(['id']), | ||
| ); | ||
| // Callback returns undefined - should fall back to using the original modelBuilder | ||
| const withModel = withTable.model('User', 'user', () => undefined); | ||
| // Verify model was added by adding another model - if first wasn't added, this would fail | ||
| const withTwoModels = withModel.model('Post', 'post', (m) => m.field('id', 'id')); | ||
| expect(withTwoModels).toBeInstanceOf(ContractBuilder); | ||
| expect(withTwoModels).not.toBe(withModel); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Strengthen “callback returns undefined” tests to assert table/model presence
Both new tests currently only verify that chaining (.table(...), .model(...)) still returns a new ContractBuilder instance. They don’t directly assert that the initial 'user' table / User model was actually added when the callback returned undefined, so they could still pass if the fallback-to-original-builder logic regressed while chaining continued to work.
Consider tightening these tests by inspecting the built contract or equivalent internal state (e.g., via build() or another accessor) and asserting that:
- The
'user'table exists instorage.tablesafter the first.table('user', () => undefined), and - The
Usermodel exists in the models map after.model('User', 'user', () => undefined).
That would make these tests more robust against future changes in how callbacks are handled.
| thresholds: { | ||
| lines: 95, | ||
| branches: 92, | ||
| branches: 95, | ||
| functions: 95, | ||
| statements: 95, | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Branches threshold alignment is good; consider centralizing common Vitest config (paths + coverage).
Matching branches: 95 to the other thresholds is consistent and fine assuming coverage already clears it. Since you’re touching this config, it might be worth:
- Adding
include: ['src/**/*.ts']undercoverage, like the other configs. - Wiring
vite-tsconfig-paths(directly or via a shared base Vitest config) so package tests benefit from tsconfig path mappings:
import { defineConfig } from 'vitest/config';
import tsconfigPaths from 'vite-tsconfig-paths';
export default defineConfig({
plugins: [tsconfigPaths()],
test: {
// ...
},
});As per coding guidelines.
🤖 Prompt for AI Agents
In packages/framework/authoring/contract-authoring/vitest.config.ts around lines
20 to 25, add coverage include and tsconfig paths support: under the existing
coverage block add include: ['src/**/*.ts'] so only source files are measured,
and wire up vite-tsconfig-paths by importing the plugin and adding it to the
exported defineConfig plugins array (or extend from a shared base Vitest config
that already registers the plugin) so tsconfig path mappings work in tests;
ensure vite-tsconfig-paths is added to devDependencies and adjust test/config
export accordingly.
| thresholds: { | ||
| lines: 91, | ||
| branches: 86, | ||
| branches: 95, | ||
| functions: 100, | ||
| statements: 95, | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Branches coverage threshold bump looks good; consider adding explicit coverage include.
Raising branches to 95 is fine as long as the package already meets this in CI. I’d also add include: ['src/**/*.ts'] under coverage so source files are consistently tracked, not just excluded files:
coverage: {
provider: 'v8',
reporter: ['text', 'json', 'html'],
+ include: ['src/**/*.ts'],
exclude: [
'dist/**',
'test/**',
'**/*.test.ts',
'**/*.test-d.ts',
'**/*.config.ts',
'**/exports/**',
],As per coding guidelines.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/framework/tooling/emitter/vitest.config.ts around lines 21 to 26,
the coverage config currently sets thresholds but lacks an explicit include; add
an include entry (e.g. include: ['src/**/*.ts']) inside the same coverage object
so Vitest consistently tracks source files for coverage reporting; ensure the
include is an array of glob patterns and keep existing thresholds and excludes
intact.
| it('preserves existing mappings when provided', () => { | ||
| const input = { | ||
| ...validContractInput, | ||
| mappings: { | ||
| modelToTable: { CustomModel: 'User' }, | ||
| }, | ||
| }; | ||
| // biome-ignore lint/suspicious/noExplicitAny: testing input with mappings property | ||
| const result = validateContract<SqlContract<SqlStorage>>(input as any); | ||
| expect(result.mappings.modelToTable).toEqual({ CustomModel: 'User' }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider moving this test to the normalization test file.
This test validates mapping preservation behavior rather than structural validation. Since the file is named contract.structure.test.ts and focuses on validation failures for missing/invalid fields, and a separate contract.normalization.test.ts file exists, this test would be better suited there.
Per coding guidelines: "Validator tests must verify that validation fails when required fields are missing, and must NOT test normalization behavior." While this test doesn't strictly test normalization defaults, it does test the mapping computation behavior which is beyond structural validation.
🤖 Prompt for AI Agents
In packages/sql/authoring/sql-contract-ts/test/contract.structure.test.ts around
lines 126 to 136, the test verifies preservation of provided mappings
(normalization behavior) and should be moved out of the structure/validation
test; cut this test and add it to
packages/sql/authoring/sql-contract-ts/test/contract.normalization.test.ts (or
the existing normalization test file), keeping the test body identical but
updating any local references/imports if needed so it runs in the normalization
suite; ensure the structure test no longer contains normalization assertions so
it only asserts validation failures for missing/invalid fields.
| thresholds: { | ||
| lines: 98, | ||
| branches: 93, | ||
| functions: 61, | ||
| functions: 95, | ||
| statements: 98, | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Function coverage threshold increase is reasonable; mirror explicit include here too.
Setting functions: 95 is aligned with the rest of the thresholds, assuming current tests already exceed that. To keep coverage behavior consistent across packages, consider also adding:
coverage: {
provider: 'v8',
reporter: ['text', 'json', 'html'],
+ include: ['src/**/*.ts'],
exclude: [
'dist/**',
'test/**',
'**/*.test.ts',
'**/*.test-d.ts',
'**/*.config.ts',
'**/exports/**',
'**/types.ts',
],As per coding guidelines.
📝 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.
| thresholds: { | |
| lines: 98, | |
| branches: 93, | |
| functions: 61, | |
| functions: 95, | |
| statements: 98, | |
| }, | |
| coverage: { | |
| provider: 'v8', | |
| reporter: ['text', 'json', 'html'], | |
| include: ['src/**/*.ts'], | |
| exclude: [ | |
| 'dist/**', | |
| 'test/**', | |
| '**/*.test.ts', | |
| '**/*.test-d.ts', | |
| '**/*.config.ts', | |
| '**/exports/**', | |
| '**/types.ts', | |
| ], | |
| }, | |
| thresholds: { | |
| lines: 98, | |
| branches: 93, | |
| functions: 95, | |
| statements: 98, | |
| }, |
🤖 Prompt for AI Agents
In packages/targets/postgres-adapter/vitest.config.ts around lines 22 to 27, the
coverage thresholds set functions: 95 should be kept but the coverage config is
missing the explicit "include" pattern used by other packages; add an "include"
entry to the coverage block (matching the repository convention for source
files, e.g., the same glob used in other targets) so coverage is calculated only
against the intended files and behavior is consistent across packages.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.ts (1)
1-1043: Test file exceeds 500-line limit — split by functionality.At 1043 lines, this file significantly exceeds the 500-line guideline for test files. Consider splitting into multiple files organized by test category:
db-schema-verify.basic.e2e.test.ts— TTY/JSON output, success scenariosdb-schema-verify.modes.e2e.test.ts— strict/permissive mode testsdb-schema-verify.errors.e2e.test.ts— missing DB URL, missing contract, parse errors, missing driverdb-schema-verify.flags.e2e.test.ts— quiet, verbose, exit codes, TTY handling, header outputEach split file should import the shared setup (fixture paths, contract factory) and be independently runnable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Lite
📒 Files selected for processing (2)
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.ts(7 hunks)packages/framework/tooling/cli/test/db-sign.e2e.test.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (18)
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/README.md)
**/*.test.{ts,tsx,js,jsx}: Test descriptions should omit 'should' per.cursor/rules/omit-should-in-tests.mdc
Use correct test import patterns: import from source files using relative paths, don't export for tests per.cursor/rules/test-import-patterns.mdc
Prefer object matchers over multiple individual expect().toBe() calls per.cursor/rules/prefer-object-matcher.mdc
Files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
**/cli/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/README.md)
**/cli/**/*.{ts,tsx}: Follow CLI command description patterns per.cursor/rules/cli-command-descriptions.mdc
Follow CLI command error handling patterns: throw errors, don't call process.exit per.cursor/rules/cli-error-handling.mdcAlways pass a
driverparameter toControlFamilyDescriptor.create(), even for commands that don't use it (e.g.,contract emit). This ensures consistent descriptor patterns across all commands.
Files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/README.md)
**/*.test.{ts,tsx}: Use type test patterns with vitest expect.typeOf() per.cursor/rules/vitest-expect-typeof.mdc
Use factory functions for AST nodes in tests per.cursor/rules/use-ast-factories.mdc
Use factory functions for ContractIR objects in tests per.cursor/rules/use-contract-ir-factories.mdc
Files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/README.md)
**/*.{ts,tsx}: Use factory functions for creating AST nodes instead of manual object creation per.cursor/rules/use-ast-factories.mdc
Follow Query DSL patterns per.cursor/rules/query-patterns.mdc
Follow includeMany type inference patterns per.cursor/rules/include-many-patterns.mdc
Follow TypeScript and architecture patterns per.cursor/rules/typescript-patterns.mdc
Follow Arktype usage guidelines per.cursor/rules/arktype-usage.mdc
Extract types from contracts per.cursor/rules/type-extraction-from-contract.mdc
Prohibit inline type imports in source files per.cursor/rules/no-inline-imports.mdc
Follow config validation and normalization patterns using Arktype per.cursor/rules/config-validation-and-normalization.mdc
**/*.{ts,tsx}: Use the'key?'syntax with the?suffix for optional keys in Arktype schemas instead of using'string | undefined'type unions
Reference schemas directly as object property values in Arktype schemas - this works seamlessly without special syntax
Usetype({ '[string]': Schema })syntax to createRecord<string, Schema>types in Arktype instead of usingRecord<string, Schema>string notation
Use the.array()method on Arktype schemas to create array types instead of wrapping schemas intype([Schema])or using manual validation
Extract TypeScript types from Arktype schemas using the.inferproperty instead of manually duplicating type definitions
Usetype.declare<>()to ensure Arktype schemas match pre-existing TypeScript types for compile-time type safety, especially when working with readonly properties
Use.array().readonly()method chaining to create readonly array types in Arktype instead of using string notation like'string[]'
Don't manually re-validate what Arktype has already checked - once validation passes, trust the structure matches the schema
Useinstanceof type.errorsto check for Arktype validation errors instead of other error checking patterns, for proper type narrowing
Return ...
Files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
**/cli/**/*.e2e.test.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/README.md)
Use CLI e2e test fixture patterns with shared fixture app per
.cursor/rules/cli-e2e-test-patterns.mdc
Files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
**/*.{ts,tsx,sql}
📄 CodeRabbit inference engine (.cursor/rules/README.md)
Follow LATERAL/json_agg patterns for Postgres queries per
.cursor/rules/postgres-lateral-patterns.mdc
Files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/README.md)
Use Object.hasOwn() instead of hasOwnProperty() per
.cursor/rules/object-hasown.mdcImport from shared plane packages using the package name and export identifier (e.g.,
@prisma-next/sql-contract/types), not/exports/paths
**/*.{ts,tsx,js,jsx}: Use arktype instead of zod for schema validation
Don't add comments if avoidable; prefer code which expresses its intent
Don't add exports for backwards compatibility unless explicitly requested
Do not reexport things from one file in another, except in theexports/folders
**/*.{ts,tsx,js,jsx}: Prefer links to canonical docs over long comments
Package layering is enforced; fix violations rather than bypassing via import validation
Files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (.cursor/rules/contract-normalization-responsibilities.mdc)
**/*.test.ts: Contract builder tests must verify that normalization happens correctly with all required fields receiving their default values
Validator tests must verify that validation fails when required fields are missing, and must NOT test normalization behavior
Parser and emitter hook tests must test functionality with normalized contracts and must NOT test normalization behaviorRemove tests that use type assertions to bypass type checking and test impossible states. Tests should not artificially test scenarios where required properties are missing.
**/*.test.ts: In tests usingwithDevDatabase()from@prisma-next/test-utils, avoid selecting or using extension-dependent columns (like pgvector, pg_trgm) when testing features unrelated to extension functionality, as pglite does not support PostgreSQL extensions
When tests must verify extension functionality end-to-end (e.g., pgvector queries), use a real PostgreSQL database instead ofwithDevDatabase()viawithClient()andprocess.env['DATABASE_URL']
When writing ORM tests with includes, relation filters, or other non-extension features, explicitly exclude extension-dependent columns from select statements to ensure compatibility with pglite-based test databases
**/*.test.ts: Within-package tests should import from source files directly (e.g., '../src/...') rather than from package exports (e.g., '@prisma-next/package-name/...')
Cross-package tests should import from package exports rather than relative source paths
Test files should never import from their own package using package aliases (e.g., '@prisma-next/package-name/...'); always use relative imports (e.g., '../src/...')
When importing from sibling packages within the same domain in test files, use relative paths instead of workspace package names
When extracting code between packages, move tests to the new package and update test imports to use relative source paths ('../src/...')
The 'exports/' folder contains the public API surface a...
Files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
packages/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/import-validation.mdc)
packages/**/*.{ts,tsx}: Same-layer imports are allowed: packages in the same layer can import from each other (e.g.,orm-lanecan import fromsql-relational-coreif both are in thelaneslayer)
Cross-layer imports must flow inward (toward core): packages can only import from inner layers (closer to core). Outer layers cannot import from inner layers (e.g., core layer cannot import from lanes layer)Import SQL emitter hook (
sqlTargetFamilyHook) from@prisma-next/sql-contract-emitter
Files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/modular-refactoring-patterns.mdc)
**/*.ts: Start refactoring by extracting pure, side-effect-free utility functions as thin wrappers around AST factories and error constructors
Extract state management into separate modules defining immutable state shapes and interfaces
Replaceanytypes with proper generic type parameters during function extraction, using existing type definitions likeModelColumnAccessor
Use thecompacthelper function to removeundefinedvalues from objects when TypeScript'sexactOptionalPropertyTypesis enabled
Organize imports in the following order: external type imports, external value imports, internal type imports, internal value importsPrefer assertions over defensive checks when data is guaranteed to be valid. After validation, use non-null assertions (!) or explicit checks rather than optional chaining to access properties.
Do not export functions, types, or utilities solely for test use; instead, tests should import from source using relative paths within the same package or refactor to use public APIs
Files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
**/packages/**/+(*.test|*.spec).{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/moving-packages.mdc)
In test files, update paths to other packages, extension pack manifests, fixtures, and import paths based on the new package location's directory depth from root
Files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
**/*.{test,spec}.{ts,js}
📄 CodeRabbit inference engine (.cursor/rules/omit-should-in-tests.mdc)
Omit the word 'should' in test descriptions - write concise, direct descriptions instead (e.g., 'equals 1' instead of 'should equal 1')
Files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (.cursor/rules/prefer-object-matcher.mdc)
Prefer object matchers with
.toMatchObject()over multiple individualexpect().toBe()calls when checking 2+ related values in tests
**/*.{test,spec}.ts: Test files must not exceed 500 lines; split files that exceed this limit into multiple files organized by logical functionality
Split test files by logical groupings: by functionality (basic, errors, structure), by feature area (joins, projections, includes), or by test type (unit, integration, edge-cases)
Use descriptive test file names following the pattern{base}.{category}.test.ts, where base name indicates the subject being tested and category indicates the test type (e.g.,.basic.test.ts,.errors.test.ts,.joins.test.ts)
Ensure each split test file contains all necessary imports, setup/teardown code, and is independently runnable without dependencies on other test files
Prefer object comparison assertions usingtoEqualortoMatchObjectinstead of piece-by-piece property checks withtoHavePropertyor individual type assertions
Useexpect.any()for type-only checks andexpect.not.objectContaining()for absence checks when performing partial object comparisons
Compare entire plan structures using object comparison assertions instead of checking individual AST fields piecemeal to ensure all fields are present and correctUse factory functions (such as
createColumnRef,createParamRef,createLiteralExpr,createTableRef,createBinaryExpr) for creating AST nodes in tests instead of manual object creationUse factory functions (e.g.,
createContractIR()) instead of manual object creation when creatingContractIRobjects in tests. Ensure all required fields (schemaVersion, targetFamily, target, models, relations, storage, extensions, capabilities, meta, sources) are present through the factory function rather than manual construction.
**/*.{test,spec}.ts: Do not use the deprecatedtoMatchTypeOf()matcher in Vitest'sexpectTypeOfAPI; usetoExtend()instead for verifying ...
Files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
packages/**/test/**/*.{ts,test.ts}
📄 CodeRabbit inference engine (.cursor/rules/shared-plane-packages.mdc)
Shared plane packages should include unit tests for validators and factories, type tests for exported types, and no integration tests requiring migration or runtime plane packages
Files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
**/*.test.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/testing-guide.mdc)
**/*.test.{ts,tsx,js}: Test descriptions should omit 'should' - use direct action-oriented phrases like 'returns correct value' instead of 'should return correct value'
Test fixtures should not depend on specific adapters (e.g.,@prisma-next/adapter-postgres). DefineCodecTypesinline in test fixtures to remain adapter-agnostic and usecreateStubAdapter()from@prisma-next/runtime/test/utilsinstead of specific adapter factories.
Import test utilities from specific packages to avoid circular dependencies: Use@prisma-next/test-utilsfor generic database/async utilities,@prisma-next/runtime/test/utilsfor runtime-specific utilities, ande2e-tests/test/utils.tsfor contract-related E2E utilities.
UsewithDevDatabasefor automatic database cleanup and ensure only one connection at a time. Close thewithClientconnection before running CLI commands that need their own database connection.
UsecollectAsync()orexecutePlanAndCollect()to collect all results from async iterables, ordrainAsyncIterable()/drainPlanExecution()to drain without collecting.
Keep test files under 500 lines maximum. Split large files by functionality (basic, errors, structure, generation), feature area (joins, projections, includes), or test type (unit, integration, edge-cases). Use descriptive file names likecodecs.registry.test.ts,runtime.joins.test.ts,driver.errors.test.ts.
Prefer object comparison usingtoEqual()ortoMatchObject()instead of piece-by-piece property checks withtoHaveProperty()and type casts.
UsetoMatchObject()for JSON parsing assertions without type casts. Useexpect.anything()for nested objects when verifying existence.
Use the extension mechanism viacreateTestContextwith anextensionsparameter to register operations and codecs programmatically, rather than manually creating operation registries.
Let TypeScript automatically infer return types from plans usingResultType<typeof plan>rather than manual type param...
Files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/testing-guide.mdc)
Always quote table names in SQL statements to handle PostgreSQL reserved keywords (e.g., use
"user"instead ofuser).
Files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/typescript-patterns.mdc)
**/*.{test,spec}.{ts,tsx}: Always validate contracts in tests using validateContract function. Contracts must contain fully qualified type IDs (e.g., 'pg/text@1'), not bare scalars.
In test files, use @ts-expect-error comments with explanations when working with dynamic proxy objects or complex type scenarios. Never use bare type assertions without comments.
When creating test mocks, use as unknown as Type (double cast) instead of any. Use Object.create() and Object.assign() to preserve prototype chain when mocking class instances.
Extract common patterns in test files into helper functions with JSDoc comments. Use shared helpers from @prisma-next/test-utils and package-specific utilities before creating new helpers.
Files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/use-correct-tools.mdc)
Use local
pnpm testscripts for testing; usepnpm test:coveragefor all packages orpnpm coverage:packagesfor packages onlyKeep tests concise; omit 'should' from test names
Files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
🧠 Learnings (42)
📓 Common learnings
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/sql-types-imports.mdc:0-0
Timestamp: 2025-12-04T12:15:37.900Z
Learning: Applies to packages/sql/**/*.{ts,tsx} : Import SQL-specific contract types (`SqlContract`, `SqlStorage`, `SqlMappings`, `ModelDefinition`, `StorageTable`, `StorageColumn`) from `prisma-next/sql-contract/types` (shared plane package), not from `prisma-next/contract/types`
Learnt from: carlagn
Repo: prisma/website PR: 1272
File: app/mcp/serializers.tsx:62-81
Timestamp: 2025-10-10T07:38:21.599Z
Learning: In PR #1272 for the prisma/website repository, the user carlagn requested not to provide additional review comments on the file `app/mcp/serializers.tsx`.
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/control-plane-descriptors.mdc:0-0
Timestamp: 2025-12-04T12:12:26.821Z
Learning: Applies to **/*.{ts,tsx} : Migrate away from legacy types (`FamilyDescriptor`, `TargetDescriptor`, `AdapterDescriptor`, `DriverDescriptor`, `ExtensionDescriptor`, `ControlPlaneDriver`) which have been completely removed from the codebase
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/control-plane-descriptors.mdc:0-0
Timestamp: 2025-12-04T12:12:26.821Z
Learning: Applies to **/*.{ts,tsx} : Always use `Control*Descriptor` types (ControlFamilyDescriptor, ControlTargetDescriptor, ControlAdapterDescriptor, ControlDriverDescriptor, ControlExtensionDescriptor). Legacy descriptor types have been completely removed.
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/type-extraction-from-contract.mdc:0-0
Timestamp: 2025-12-04T12:16:59.752Z
Learning: Applies to **/*.{ts,tsx} : Never pass `CodecTypes` or `OperationTypes` as separate type parameters to `schema()`, `sql()`, or `orm()` functions
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/type-extraction-from-contract.mdc:0-0
Timestamp: 2025-12-04T12:16:59.752Z
Learning: Applies to **/*.{ts,tsx} : Extract `CodecTypes` from contract using the helper type `ExtractCodecTypes<Contract>` to access `Contract['mappings']['codecTypes']` with fallback to `Record<string, never>`
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/testing-guide.mdc:0-0
Timestamp: 2025-12-04T12:16:51.399Z
Learning: Applies to **/*.test.{ts,tsx,js} : Test fixtures should not depend on specific adapters (e.g., `prisma-next/adapter-postgres`). Define `CodecTypes` inline in test fixtures to remain adapter-agnostic and use `createStubAdapter()` from `prisma-next/runtime/test/utils` instead of specific adapter factories.
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/typescript-patterns.mdc:0-0
Timestamp: 2025-12-04T12:17:20.542Z
Learning: Applies to **/*.{ts,tsx} : When extracting types from ColumnBuilder instances, use infer for all type parameters without constraints to allow correct type inference.
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/modular-refactoring-patterns.mdc:0-0
Timestamp: 2025-12-04T12:13:22.174Z
Learning: Applies to **/*.ts : Replace `any` types with proper generic type parameters during function extraction, using existing type definitions like `ModelColumnAccessor`
📚 Learning: 2025-12-04T12:16:51.399Z
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/testing-guide.mdc:0-0
Timestamp: 2025-12-04T12:16:51.399Z
Learning: Applies to e2e-tests/**/*.test.{ts,tsx,js} : Use `setupTestDirectoryFromFixtures()` helper to create ephemeral test directories for CLI E2E tests
Applied to files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
📚 Learning: 2025-12-04T12:16:51.399Z
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/testing-guide.mdc:0-0
Timestamp: 2025-12-04T12:16:51.399Z
Learning: Applies to e2e-tests/**/*.test.{ts,tsx,js} : Use shared `e2e-tests/test/utils.ts` for E2E contract-related utilities including `loadContractFromDisk()` and `emitAndVerifyContract()` helpers.
Applied to files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
📚 Learning: 2025-12-04T12:11:41.295Z
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/cli-e2e-test-patterns.mdc:0-0
Timestamp: 2025-12-04T12:11:41.295Z
Learning: Applies to **/cli/**/test/**/*.e2e.{ts,tsx} : Use setupTestDirectoryFromFixtures(fixtureSubdir, configFileName?, replacements?) helper to create ephemeral test directories from fixtures with optional config file names and placeholder replacements
Applied to files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
📚 Learning: 2025-12-04T12:11:41.295Z
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/cli-e2e-test-patterns.mdc:0-0
Timestamp: 2025-12-04T12:11:41.295Z
Learning: Applies to packages/framework/tooling/cli/test/cli-e2e-test-app/fixtures/**/*.ts : Organize fixtures by command in subdirectories under packages/framework/tooling/cli/test/cli-e2e-test-app/fixtures/
Applied to files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
📚 Learning: 2025-12-04T12:11:41.295Z
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/cli-e2e-test-patterns.mdc:0-0
Timestamp: 2025-12-04T12:11:41.295Z
Learning: Applies to **/cli/**/test/**/*.e2e.{ts,tsx} : Use setupIntegrationTestDirectoryFromFixtures for tests that need package resolution such as loadContractFromTs
Applied to files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
📚 Learning: 2025-12-04T12:16:51.399Z
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/testing-guide.mdc:0-0
Timestamp: 2025-12-04T12:16:51.399Z
Learning: Applies to e2e-tests/fixtures/** : Use shared `cli-e2e-test-app` directory with static `package.json` at root and organize fixtures by command in subdirectories (e.g., `fixtures/emit/`, `fixtures/db-verify/`)
Applied to files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
📚 Learning: 2025-12-04T12:11:41.295Z
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/cli-e2e-test-patterns.mdc:0-0
Timestamp: 2025-12-04T12:11:41.295Z
Learning: Applies to **/cli/**/test/**/*.e2e.{ts,tsx} : Each test must clean up its own ephemeral directory using cleanupDir() in finally blocks or afterEach hooks - never use afterAll hooks or global cleanup functions that scan and delete directories
Applied to files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
📚 Learning: 2025-12-04T12:16:51.399Z
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/testing-guide.mdc:0-0
Timestamp: 2025-12-04T12:16:51.399Z
Learning: Applies to **/*.test.{ts,tsx,js} : Import test utilities from specific packages to avoid circular dependencies: Use `prisma-next/test-utils` for generic database/async utilities, `prisma-next/runtime/test/utils` for runtime-specific utilities, and `e2e-tests/test/utils.ts` for contract-related E2E utilities.
Applied to files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.ts
📚 Learning: 2025-12-04T12:11:41.295Z
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/cli-e2e-test-patterns.mdc:0-0
Timestamp: 2025-12-04T12:11:41.295Z
Learning: Applies to **/cli/**/test/**/*.e2e.{ts,tsx} : Use shared fixture app pattern with command-specific subdirectories for CLI e2e tests that load config files and need workspace dependency resolution
Applied to files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
📚 Learning: 2025-12-04T17:18:23.669Z
Learnt from: aqrln
Repo: prisma/prisma PR: 28830
File: packages/client/tests/e2e/sqlcommenter-query-insights/_steps.cts:1-20
Timestamp: 2025-12-04T17:18:23.669Z
Learning: E2E tests in packages/client/tests/e2e/** are executed within Docker containers and must contain complete setup steps including `pnpm install`, `pnpm prisma generate`, and database initialization commands, even if those steps appear redundant from a CI perspective.
Applied to files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
📚 Learning: 2025-12-04T12:15:46.557Z
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/test-database-limitations.mdc:0-0
Timestamp: 2025-12-04T12:15:46.557Z
Learning: Applies to **/*.test.ts : In tests using `withDevDatabase()` from `prisma-next/test-utils`, avoid selecting or using extension-dependent columns (like pgvector, pg_trgm) when testing features unrelated to extension functionality, as pglite does not support PostgreSQL extensions
Applied to files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
📚 Learning: 2025-12-04T12:16:51.399Z
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/testing-guide.mdc:0-0
Timestamp: 2025-12-04T12:16:51.399Z
Learning: Applies to **/*.test.{ts,tsx,js} : Test fixtures should not depend on specific adapters (e.g., `prisma-next/adapter-postgres`). Define `CodecTypes` inline in test fixtures to remain adapter-agnostic and use `createStubAdapter()` from `prisma-next/runtime/test/utils` instead of specific adapter factories.
Applied to files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
📚 Learning: 2025-12-02T07:34:13.556Z
Learnt from: CR
Repo: prisma/pdp-cloudflare PR: 0
File: scripts/ppg-tenant-manager-admin/AGENTS.md:0-0
Timestamp: 2025-12-02T07:34:13.556Z
Learning: Applies to scripts/ppg-tenant-manager-admin/prisma/schema.prisma : Prisma schema files must define the test database structure with proper relationships and indexes
Applied to files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
📚 Learning: 2025-08-22T12:12:24.602Z
Learnt from: FGoessler
Repo: prisma/prisma-examples PR: 8260
File: generator-prisma-client/nextjs-starter-webpack-turborepo/packages/database/package.json:13-13
Timestamp: 2025-08-22T12:12:24.602Z
Learning: When analyzing changes from package.json seed scripts to "prisma db seed", always verify the actual content of prisma.config.ts rather than relying solely on regex patterns, as the configuration may be properly defined but in a format that doesn't match overly restrictive search patterns.
Applied to files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.ts
📚 Learning: 2025-12-03T09:46:36.091Z
Learnt from: CR
Repo: prisma/prisma PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T09:46:36.091Z
Learning: Applies to packages/migrate/src/__tests__/fixtures/**/*.config.ts : CLI test fixtures should provide `prisma.config.ts` per schema variant (e.g., `invalid-url.config.ts` next to `prisma/invalid-url.prisma`)
Applied to files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
📚 Learning: 2025-12-04T12:16:51.399Z
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/testing-guide.mdc:0-0
Timestamp: 2025-12-04T12:16:51.399Z
Learning: Applies to **/*.{ts,tsx,js} : Always quote table names in SQL statements to handle PostgreSQL reserved keywords (e.g., use `"user"` instead of `user`).
Applied to files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.ts
📚 Learning: 2025-12-04T12:15:46.557Z
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/test-database-limitations.mdc:0-0
Timestamp: 2025-12-04T12:15:46.557Z
Learning: Applies to **/*.test.ts : When writing ORM tests with includes, relation filters, or other non-extension features, explicitly exclude extension-dependent columns from select statements to ensure compatibility with pglite-based test databases
Applied to files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.ts
📚 Learning: 2025-12-04T12:17:20.542Z
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/typescript-patterns.mdc:0-0
Timestamp: 2025-12-04T12:17:20.542Z
Learning: Applies to **/*.{test,spec}.{ts,tsx} : Always validate contracts in tests using validateContract function. Contracts must contain fully qualified type IDs (e.g., 'pg/text1'), not bare scalars.
Applied to files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.ts
📚 Learning: 2025-12-04T16:42:41.033Z
Learnt from: aqrln
Repo: prisma/prisma PR: 28846
File: packages/client/tests/functional/issues/TML-1664-unknown-enum-value-read-error/test.ts:20-26
Timestamp: 2025-12-04T16:42:41.033Z
Learning: In Prisma Client functional tests (packages/client/tests/functional/**), the test framework guarantees a fresh database for each test suite, so test setup code (like ALTER TYPE ADD VALUE in beforeAll hooks) doesn't need idempotency checks for repeated runs.
Applied to files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
📚 Learning: 2025-12-04T12:14:36.657Z
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/postgres-lateral-patterns.mdc:0-0
Timestamp: 2025-12-04T12:14:36.657Z
Learning: Applies to packages/sql/**/*.{ts,tsx,js} : When using LATERAL joins in PostgreSQL adapters, use different aliases for table and column: table alias as `{alias}_lateral`, column alias as `{alias}`, and select using `table_alias.column_alias`
Applied to files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.ts
📚 Learning: 2025-12-04T12:16:51.399Z
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/testing-guide.mdc:0-0
Timestamp: 2025-12-04T12:16:51.399Z
Learning: Applies to e2e-tests/**/*.test.{ts,tsx,js} : Use `executeCommand()` helper from `test/utils/test-helpers.ts` to execute CLI commands. For error cases, use `await expect(executeCommand(...)).rejects.toThrow()` instead of checking exit codes. For success cases, `executeCommand()` returns `0` (doesn't throw).
Applied to files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
📚 Learning: 2025-12-04T12:11:50.057Z
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/cli-error-handling.mdc:0-0
Timestamp: 2025-12-04T12:11:50.057Z
Learning: Applies to **/*cli**/src/**/*.test.ts : Test CLI commands by spying on `process.exit()` to verify correct exit codes are called
Applied to files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
📚 Learning: 2025-12-04T12:11:41.295Z
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/cli-e2e-test-patterns.mdc:0-0
Timestamp: 2025-12-04T12:11:41.295Z
Learning: Applies to **/cli/**/test/**/*.e2e.{ts,tsx} : For CLI tests that run as separate Node.js processes, use execFileAsync with the cwd option set to testDir so relative paths in config files resolve correctly
Applied to files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.ts
📚 Learning: 2025-12-04T12:15:46.557Z
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/test-database-limitations.mdc:0-0
Timestamp: 2025-12-04T12:15:46.557Z
Learning: Applies to **/*.test.ts : When tests must verify extension functionality end-to-end (e.g., pgvector queries), use a real PostgreSQL database instead of `withDevDatabase()` via `withClient()` and `process.env['DATABASE_URL']`
Applied to files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
📚 Learning: 2025-12-04T12:16:51.399Z
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/testing-guide.mdc:0-0
Timestamp: 2025-12-04T12:16:51.399Z
Learning: Applies to e2e-tests/**/*.test.{ts,tsx,js} : Each CLI E2E test must clean up its own directory. Use `afterEach` for directories created in `beforeEach`, or call `cleanup()` in `finally` blocks. Never use `afterAll` hooks for cleanup - they interfere with running tests and cause race conditions.
Applied to files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
📚 Learning: 2025-12-04T12:16:51.399Z
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/testing-guide.mdc:0-0
Timestamp: 2025-12-04T12:16:51.399Z
Learning: Applies to **/*.test.{ts,tsx,js} : Use `withDevDatabase` for automatic database cleanup and ensure only one connection at a time. Close the `withClient` connection before running CLI commands that need their own database connection.
Applied to files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
📚 Learning: 2025-12-04T12:12:15.848Z
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/contract-normalization-responsibilities.mdc:0-0
Timestamp: 2025-12-04T12:12:15.848Z
Learning: Applies to prisma-next/sql-query/src/contract.ts : The `validateContract()` function must assume the contract passed to it is already normalized (all required fields present) and throw errors if required fields are missing or have incorrect types
Applied to files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.ts
📚 Learning: 2025-12-04T12:12:15.848Z
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/contract-normalization-responsibilities.mdc:0-0
Timestamp: 2025-12-04T12:12:15.848Z
Learning: Applies to prisma-next/sql-query/src/contract.ts : The `validateContract()` function must validate contract structure and types only, using Arktype schemas for structural validation and checking logical consistency (e.g., FK references), but must NOT normalize the contract or set default values for missing fields
Applied to files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.ts
📚 Learning: 2025-12-03T09:46:36.091Z
Learnt from: CR
Repo: prisma/prisma PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T09:46:36.091Z
Learning: Run client e2e suites with `pnpm --filter prisma/client test:e2e --verbose --runInBand` after fresh `pnpm build`
Applied to files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
📚 Learning: 2025-12-03T09:31:15.792Z
Learnt from: CR
Repo: prisma/pdp-cloudflare PR: 0
File: services/tenant-manager/AGENTS.md:0-0
Timestamp: 2025-12-03T09:31:15.792Z
Learning: Applies to services/tenant-manager/tests/**/*.test.ts : Test suite must cover all critical paths including provisioning, cleanup, and error scenarios
Applied to files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
📚 Learning: 2025-12-04T12:16:51.399Z
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/testing-guide.mdc:0-0
Timestamp: 2025-12-04T12:16:51.399Z
Learning: Applies to **/*.test.{ts,tsx,js} : Set environment variables BEFORE importing modules that depend on them (e.g., use dynamic `await import()` after setting DATABASE_URL). Maintain production code structure - don't modify example app query files to work around test requirements.
Applied to files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.tspackages/framework/tooling/cli/test/db-sign.e2e.test.ts
📚 Learning: 2025-12-04T12:11:41.295Z
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/cli-e2e-test-patterns.mdc:0-0
Timestamp: 2025-12-04T12:11:41.295Z
Learning: CLI e2e tests should load config files and verify command behavior with different config variants in isolated environments per test case
Applied to files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.ts
📚 Learning: 2025-11-25T09:20:01.527Z
Learnt from: CR
Repo: prisma/pdp-cloudflare PR: 0
File: scripts/ppg-test-connections/AGENTS.md:0-0
Timestamp: 2025-11-25T09:20:01.527Z
Learning: Applies to scripts/ppg-test-connections/**/*test*.ts : Implement comprehensive error tracking with PostgreSQL error codes, especially 08006 (connection closed by upstream) and 08xxx connection error patterns
Applied to files:
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.ts
📚 Learning: 2025-12-04T12:16:51.399Z
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/testing-guide.mdc:0-0
Timestamp: 2025-12-04T12:16:51.399Z
Learning: Applies to e2e-tests/**/*.test.{ts,tsx,js} : Load contracts from committed fixtures in E2E tests rather than emitting on every test run to improve test performance and consistency.
Applied to files:
packages/framework/tooling/cli/test/db-sign.e2e.test.ts
📚 Learning: 2025-12-04T12:11:41.295Z
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/cli-e2e-test-patterns.mdc:0-0
Timestamp: 2025-12-04T12:11:41.295Z
Learning: Applies to packages/framework/tooling/cli/test/cli-e2e-test-app/fixtures/**/*.ts : Use placeholders like {{PLACEHOLDER}} in fixture config files and replace them at test time to inject dynamic values
Applied to files:
packages/framework/tooling/cli/test/db-sign.e2e.test.ts
📚 Learning: 2025-12-04T12:11:41.295Z
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/cli-e2e-test-patterns.mdc:0-0
Timestamp: 2025-12-04T12:11:41.295Z
Learning: Applies to **/cli/**/test/**/*.e2e.{ts,tsx} : Use execFileAsync (promisified execFile) instead of spawn when running CLI as a separate process, as execFile captures stdout/stderr by default without needing stdio option
Applied to files:
packages/framework/tooling/cli/test/db-sign.e2e.test.ts
📚 Learning: 2025-12-04T12:11:41.295Z
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/cli-e2e-test-patterns.mdc:0-0
Timestamp: 2025-12-04T12:11:41.295Z
Learning: Applies to **/cli/**/test/**/*.e2e.{ts,tsx} : Only log CLI output (stdout/stderr) on errors for cleaner test output when using execFileAsync
Applied to files:
packages/framework/tooling/cli/test/db-sign.e2e.test.ts
📚 Learning: 2025-12-04T12:17:20.542Z
Learnt from: CR
Repo: prisma/prisma-next PR: 0
File: .cursor/rules/typescript-patterns.mdc:0-0
Timestamp: 2025-12-04T12:17:20.542Z
Learning: Applies to **/*.{test,spec}.{ts,tsx} : Extract common patterns in test files into helper functions with JSDoc comments. Use shared helpers from prisma-next/test-utils and package-specific utilities before creating new helpers.
Applied to files:
packages/framework/tooling/cli/test/db-sign.e2e.test.ts
📚 Learning: 2025-11-25T09:20:01.527Z
Learnt from: CR
Repo: prisma/pdp-cloudflare PR: 0
File: scripts/ppg-test-connections/AGENTS.md:0-0
Timestamp: 2025-11-25T09:20:01.527Z
Learning: Applies to scripts/ppg-test-connections/**/*test*.ts : Generate detailed test reports with tabular formatting and comprehensive metrics collection (query counts, connection duration, error frequency)
Applied to files:
packages/framework/tooling/cli/test/db-sign.e2e.test.ts
📚 Learning: 2025-11-25T09:20:01.527Z
Learnt from: CR
Repo: prisma/pdp-cloudflare PR: 0
File: scripts/ppg-test-connections/AGENTS.md:0-0
Timestamp: 2025-11-25T09:20:01.527Z
Learning: Applies to scripts/ppg-test-connections/**/*test*.ts : Implement health check intervals that balance monitoring thoroughness with performance to avoid overwhelming the database
Applied to files:
packages/framework/tooling/cli/test/db-sign.e2e.test.ts
📚 Learning: 2025-11-25T09:20:01.527Z
Learnt from: CR
Repo: prisma/pdp-cloudflare PR: 0
File: scripts/ppg-test-connections/AGENTS.md:0-0
Timestamp: 2025-11-25T09:20:01.527Z
Learning: Applies to scripts/ppg-test-connections/**/*test*.ts : Implement graceful shutdown handlers for long-running connection tests using SIGINT/SIGTERM listeners
Applied to files:
packages/framework/tooling/cli/test/db-sign.e2e.test.ts
🧬 Code graph analysis (1)
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.ts (2)
packages/framework/tooling/cli/test/utils/test-helpers.ts (3)
withTempDir(188-217)executeCommand(47-70)getExitCode(30-39)packages/framework/tooling/cli/src/commands/db-schema-verify.ts (1)
createDbSchemaVerifyCommand(45-219)
🔇 Additional comments (13)
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.ts (3)
239-248: Column format now consistent with PR objectives.The
posttable columns correctly use the new{ codecId, nativeType, nullable }structure, addressing the previous review comment.
18-34: Test structure withwithTempDirwrapper is correct.The wrapper properly manages temp directory lifecycle with automatic cleanup in
afterEach, eliminating the need for manual cleanup in individual tests.
733-803: Exit code verification test is well implemented.The test correctly uses
getExitCode()to verify the exit code after command execution, following the test-helpers pattern.packages/framework/tooling/cli/test/db-sign.e2e.test.ts (10)
1-14: Imports and helpers align with CLI E2E testing patternsUsing
node:specifiers,withDevDatabase/withClient, and the shared helpers (executeCommand,getExitCode,setupCommandMocks,setupTestDirectoryFromFixtures,withTempDir) matches the prescribed CLI e2e patterns; no issues here.Based on learnings, this follows the recommended
cli-e2e-test-apphelper usage.
19-36: withTempDir wrapper and per-test mocks correctly scope side effectsWrapping the suite in
withTempDirand wiringsetupCommandMocks/cleanupMocksinbeforeEach/afterEachcleanly isolates filesystem state and process/console mocks per test; this fits the CLI e2e guidelines.Based on learnings, this is the intended pattern for ephemeral test directories and CLI command mocking.
37-107: Happy‑path sign test robustly validates marker creation and output normalizationThe test sets up the schema via
withDevDatabase/withClient, emits the contract, signs, then (a) asserts the marker row exists inprisma_contract.markerand (b) snapshots normalized, ANSI‑stripped output with paths/ports scrubbed. That gives good coverage of the end‑to‑end “schema matches contract” path under the new contract shape.
109-181: Schema‑mismatch test correctly asserts that no marker is writtenCreating a non‑matching schema, then asserting
db-signthrows and that the marker table (created defensively if needed) contains zero rows forid = 1is a solid check that verification failure prevents marker writes.
183-265: JSON envelope success test covers structure and stabilizes snapshotsClearing prior console output, running with
--json, normalizingmeta.contractPathand timings, then usingtoMatchObjectplus a snapshot on the normalized payload is a good pattern for asserting the JSON envelope without over‑coupling to volatile fields.As per coding guidelines, this uses object matchers appropriately for related fields.
338-383: Quiet mode test correctly suppresses success bannerThe quiet-mode test follows the standard setup, runs
db-signwith--quiet, and then asserts that “Database signed” does not appear inconsoleOutput, which is the key behavioral guarantee. This looks good.
385-449: Schema verification failure test validates new column shape and exit semanticsMutating
contractJson.storage.tables.user.columns.emailto{ codecId: 'pg/text@1', nativeType: 'text', nullable: false }aligns with the newStorageColumnshape, and the test then: (a) expectsexecuteCommandto throw via the mockedprocess.exit, (b) assertsgetExitCode() === 1, and (c) checks that verification output (not sign output) is printed. This gives strong coverage of the failure path under the enriched column representation.Based on learnings, this also correctly avoids adapter-specific test dependencies.
522-571: Formatted sign output test cleanly distinguishes verification vs sign outputBy clearing prior output, running
db-sign, and asserting that the resulting output contains “Database signed” but not “does not satisfy contract”, this test confirms that in the passing case the user sees the sign result rather than verification diagnostics. The flow mirrors other tests and looks correct.
267-298: ENOENT contract test exercises PN-CLI-4 error path; consider also asserting exit codeThe ENOENT scenario is well covered (no
contract.json, expect rejection, and assertPN-CLI-4plus a “file not found” message onconsoleErrors). If PN‑CLI‑4 is meant to correspond to a specific exit code, you might optionally add agetExitCode()assertion here for parity with the schema‑verification failure tests.⛔ Skipped due to learnings
Learnt from: CR Repo: prisma/prisma-next PR: 0 File: .cursor/rules/cli-error-handling.mdc:0-0 Timestamp: 2025-12-04T12:11:50.057Z Learning: Applies to **/*cli**/src/**/*.test.ts : Test CLI commands by spying on `process.exit()` to verify correct exit codes are calledLearnt from: CR Repo: prisma/prisma-next PR: 0 File: .cursor/rules/README.md:0-0 Timestamp: 2025-12-04T12:10:55.650Z Learning: Applies to **/cli/**/*.{ts,tsx} : Follow CLI command error handling patterns: throw errors, don't call process.exit per `.cursor/rules/cli-error-handling.mdc`Learnt from: CR Repo: prisma/prisma-next PR: 0 File: .cursor/rules/testing-guide.mdc:0-0 Timestamp: 2025-12-04T12:16:51.399Z Learning: Applies to e2e-tests/**/*.test.{ts,tsx,js} : Use `executeCommand()` helper from `test/utils/test-helpers.ts` to execute CLI commands. For error cases, use `await expect(executeCommand(...)).rejects.toThrow()` instead of checking exit codes. For success cases, `executeCommand()` returns `0` (doesn't throw).Learnt from: CR Repo: prisma/prisma-next PR: 0 File: .cursor/rules/contract-normalization-responsibilities.mdc:0-0 Timestamp: 2025-12-04T12:12:15.848Z Learning: Applies to **/*.test.ts : Parser and emitter hook tests must test functionality with normalized contracts and must NOT test normalization behaviorLearnt from: CR Repo: prisma/prisma-next PR: 0 File: .cursor/rules/typescript-patterns.mdc:0-0 Timestamp: 2025-12-04T12:17:20.542Z Learning: Applies to **/*.{test,spec}.{ts,tsx} : Always validate contracts in tests using validateContract function. Contracts must contain fully qualified type IDs (e.g., 'pg/text1'), not bare scalars.Learnt from: CR Repo: prisma/prisma-next PR: 0 File: .cursor/rules/cli-e2e-test-patterns.mdc:0-0 Timestamp: 2025-12-04T12:11:41.295Z Learning: Applies to **/cli/**/test/**/*.e2e.{ts,tsx} : Only log CLI output (stdout/stderr) on errors for cleaner test output when using execFileAsyncLearnt from: CR Repo: prisma/prisma-next PR: 0 File: .cursor/rules/cli-e2e-test-patterns.mdc:0-0 Timestamp: 2025-12-04T12:11:41.295Z Learning: Applies to **/cli/**/test/**/*.e2e.{ts,tsx} : For CLI tests that run as separate Node.js processes, use execFileAsync with the cwd option set to testDir so relative paths in config files resolve correctlyLearnt from: aqrln Repo: prisma/prisma PR: 28830 File: packages/client/tests/e2e/sqlcommenter-query-insights/_steps.cts:1-20 Timestamp: 2025-12-04T17:18:23.669Z Learning: E2E tests in packages/client/tests/e2e/** are executed within Docker containers and must contain complete setup steps including `pnpm install`, `pnpm prisma generate`, and database initialization commands, even if those steps appear redundant from a CI perspective.Learnt from: CR Repo: prisma/prisma-next PR: 0 File: .cursor/rules/contract-normalization-responsibilities.mdc:0-0 Timestamp: 2025-12-04T12:12:15.848Z Learning: Applies to prisma-next/cli/src/load-ts-contract.ts : The `loadContractFromTs()` function must only parse and load TypeScript contract files and validate contract purity (JSON-serializability), but must NOT normalize the contract or set default values for missing fieldsLearnt from: CR Repo: prisma/prisma-next PR: 0 File: .cursor/rules/test-database-limitations.mdc:0-0 Timestamp: 2025-12-04T12:15:46.557Z Learning: Applies to **/*.test.ts : In tests using `withDevDatabase()` from `prisma-next/test-utils`, avoid selecting or using extension-dependent columns (like pgvector, pg_trgm) when testing features unrelated to extension functionality, as pglite does not support PostgreSQL extensionsLearnt from: CR Repo: prisma/prisma-next PR: 0 File: .cursor/rules/contract-normalization-responsibilities.mdc:0-0 Timestamp: 2025-12-04T12:12:15.848Z Learning: Applies to prisma-next/sql-query/src/contract.ts : The `validateContract()` function must assume the contract passed to it is already normalized (all required fields present) and throw errors if required fields are missing or have incorrect typesLearnt from: CR Repo: prisma/prisma-next PR: 0 File: .cursor/rules/testing-guide.mdc:0-0 Timestamp: 2025-12-04T12:16:51.399Z Learning: Applies to **/*.test.{ts,tsx,js} : Use `withDevDatabase` for automatic database cleanup and ensure only one connection at a time. Close the `withClient` connection before running CLI commands that need their own database connection.Learnt from: CR Repo: prisma/prisma-next PR: 0 File: .cursor/rules/cli-e2e-test-patterns.mdc:0-0 Timestamp: 2025-12-04T12:11:41.295Z Learning: Applies to **/cli/**/test/**/*.e2e.{ts,tsx} : Use execFileAsync (promisified execFile) instead of spawn when running CLI as a separate process, as execFile captures stdout/stderr by default without needing stdio optionLearnt from: CR Repo: prisma/prisma-next PR: 0 File: .cursor/rules/cli-e2e-test-patterns.mdc:0-0 Timestamp: 2025-12-04T12:11:41.295Z Learning: Applies to **/cli/**/test/**/*.e2e.{ts,tsx} : Use setupIntegrationTestDirectoryFromFixtures for tests that need package resolution such as loadContractFromTsLearnt from: CR Repo: prisma/prisma PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-12-05T12:57:15.977Z Learning: Tests rely on fixtures under `packages/**/src/__tests__/fixtures`. Default Jest/Vitest runner is invoked via `pnpm --filter prisma/<pkg> test <pattern>`. Some packages already use Vitest, `packages/cli` uses both, older packages still use Jest. Inline snapshots can be sensitive to formatting; prefer concise expectations unless the exact message matters.Learnt from: CR Repo: prisma/prisma-next PR: 0 File: .cursor/rules/contract-normalization-responsibilities.mdc:0-0 Timestamp: 2025-12-04T12:12:15.848Z Learning: Applies to prisma-next/cli/src/load-ts-contract.ts : The `loadContractFromTs()` function must assume contracts loaded from manually-written files should be normalized elsewhere (e.g., in the contract builder) and not perform normalization itself
300-336: Non‑ENOENT contract read error path is covered; exit code assertion would tighten itCreating invalid JSON and asserting the command rejects ensures the “file read succeeds but JSON.parse fails” branch is exercised. To fully verify CLI behavior, consider also asserting that
getExitCode()is1(or whatever error code you expect for unexpected failures), mirroring the schema‑verification tests.- // Verify error was handled (command failed) - // The error path is covered even if we don't check the exact error message format - // This tests the branch where file read succeeds but JSON.parse fails + // Verify error was handled (command failed) + // This tests the branch where file read succeeds but JSON.parse fails + expect(getExitCode()).toBe(1);⛔ Skipped due to learnings
Learnt from: CR Repo: prisma/prisma-next PR: 0 File: .cursor/rules/cli-error-handling.mdc:0-0 Timestamp: 2025-12-04T12:11:50.057Z Learning: Applies to **/*cli**/src/**/*.test.ts : Test CLI commands by spying on `process.exit()` to verify correct exit codes are calledLearnt from: CR Repo: prisma/prisma-next PR: 0 File: .cursor/rules/README.md:0-0 Timestamp: 2025-12-04T12:10:55.650Z Learning: Applies to **/cli/**/*.{ts,tsx} : Follow CLI command error handling patterns: throw errors, don't call process.exit per `.cursor/rules/cli-error-handling.mdc`Learnt from: CR Repo: prisma/prisma-next PR: 0 File: .cursor/rules/testing-guide.mdc:0-0 Timestamp: 2025-12-04T12:16:51.399Z Learning: Applies to e2e-tests/**/*.test.{ts,tsx,js} : Use `executeCommand()` helper from `test/utils/test-helpers.ts` to execute CLI commands. For error cases, use `await expect(executeCommand(...)).rejects.toThrow()` instead of checking exit codes. For success cases, `executeCommand()` returns `0` (doesn't throw).Learnt from: CR Repo: prisma/prisma-next PR: 0 File: .cursor/rules/testing-guide.mdc:0-0 Timestamp: 2025-12-04T12:16:51.399Z Learning: Applies to **/*.test.{ts,tsx,js} : Use `toMatchObject()` for JSON parsing assertions without type casts. Use `expect.anything()` for nested objects when verifying existence.Learnt from: CR Repo: prisma/prisma-next PR: 0 File: .cursor/rules/cli-e2e-test-patterns.mdc:0-0 Timestamp: 2025-12-04T12:11:41.295Z Learning: Applies to **/cli/**/test/**/*.e2e.{ts,tsx} : Only log CLI output (stdout/stderr) on errors for cleaner test output when using execFileAsyncLearnt from: CR Repo: prisma/prisma-next PR: 0 File: .cursor/rules/cli-error-handling.mdc:0-0 Timestamp: 2025-12-04T12:11:50.057Z Learning: Applies to **/*cli**/src/**/*command.ts : Call `process.exit(exitCode)` directly in commands for exit code control; never throw errors from commandsLearnt from: CR Repo: prisma/prisma-next PR: 0 File: .cursor/rules/testing-guide.mdc:0-0 Timestamp: 2025-12-04T12:16:51.399Z Learning: Applies to **/*.test.{ts,tsx,js} : Don't test exhaustive validation logic through integration tests with file I/O. Focus integration tests on loading behavior and error handling; unit test validation logic separately by importing validation functions directly.Learnt from: CR Repo: prisma/prisma-next PR: 0 File: .cursor/rules/cli-e2e-test-patterns.mdc:0-0 Timestamp: 2025-12-04T12:11:41.295Z Learning: CLI e2e tests should load config files and verify command behavior with different config variants in isolated environments per test caseLearnt from: CR Repo: prisma/prisma-next PR: 0 File: .cursor/rules/typescript-patterns.mdc:0-0 Timestamp: 2025-12-04T12:17:20.542Z Learning: Applies to **/*.{test,spec}.{ts,tsx} : In test files, use ts-expect-error comments with explanations when working with dynamic proxy objects or complex type scenarios. Never use bare type assertions without comments.Learnt from: CR Repo: prisma/prisma-next PR: 0 File: .cursor/rules/cli-e2e-test-patterns.mdc:0-0 Timestamp: 2025-12-04T12:11:41.295Z Learning: Applies to **/cli/**/test/**/*.e2e.{ts,tsx} : Use execFileAsync (promisified execFile) instead of spawn when running CLI as a separate process, as execFile captures stdout/stderr by default without needing stdio optionLearnt from: CR Repo: prisma/prisma-next PR: 0 File: .cursor/rules/contract-normalization-responsibilities.mdc:0-0 Timestamp: 2025-12-04T12:12:15.848Z Learning: Applies to prisma-next/cli/src/load-ts-contract.ts : The `loadContractFromTs()` function must only parse and load TypeScript contract files and validate contract purity (JSON-serializability), but must NOT normalize the contract or set default values for missing fieldsLearnt from: CR Repo: prisma/prisma-next PR: 0 File: .cursor/rules/contract-normalization-responsibilities.mdc:0-0 Timestamp: 2025-12-04T12:12:15.848Z Learning: Applies to prisma-next/sql-query/src/contract.ts : The `validateContract()` function must assume the contract passed to it is already normalized (all required fields present) and throw errors if required fields are missing or have incorrect typesLearnt from: aqrln Repo: prisma/prisma PR: 28830 File: packages/client/tests/e2e/sqlcommenter-query-insights/_steps.cts:1-20 Timestamp: 2025-12-04T17:18:23.669Z Learning: E2E tests in packages/client/tests/e2e/** are executed within Docker containers and must contain complete setup steps including `pnpm install`, `pnpm prisma generate`, and database initialization commands, even if those steps appear redundant from a CI perspective.Learnt from: CR Repo: prisma/pdp-cloudflare PR: 0 File: scripts/ppg-test-connections/AGENTS.md:0-0 Timestamp: 2025-11-25T09:20:01.527Z Learning: Applies to scripts/ppg-test-connections/**/*test*.ts : Implement comprehensive error tracking with PostgreSQL error codes, especially 08006 (connection closed by upstream) and 08xxx connection error patternsLearnt from: CR Repo: prisma/prisma-next PR: 0 File: .cursor/rules/test-database-limitations.mdc:0-0 Timestamp: 2025-12-04T12:15:46.557Z Learning: Applies to **/*.test.ts : In tests using `withDevDatabase()` from `prisma-next/test-utils`, avoid selecting or using extension-dependent columns (like pgvector, pg_trgm) when testing features unrelated to extension functionality, as pglite does not support PostgreSQL extensionsLearnt from: CR Repo: prisma/prisma-next PR: 0 File: .cursor/rules/typescript-patterns.mdc:0-0 Timestamp: 2025-12-04T12:17:20.542Z Learning: Applies to **/*.{test,spec}.{ts,tsx} : Always validate contracts in tests using validateContract function. Contracts must contain fully qualified type IDs (e.g., 'pg/text1'), not bare scalars.Learnt from: CR Repo: prisma/prisma-next PR: 0 File: .cursor/rules/contract-normalization-responsibilities.mdc:0-0 Timestamp: 2025-12-04T12:12:15.848Z Learning: Applies to prisma-next/cli/src/load-ts-contract.ts : The `loadContractFromTs()` function must assume contracts loaded from manually-written files should be normalized elsewhere (e.g., in the contract builder) and not perform normalization itself
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.ts
Outdated
Show resolved
Hide resolved
| it( | ||
| 'adds blank line after spinners when TTY and not quiet/JSON', | ||
| async () => { | ||
| const originalIsTTY = process.stdout.isTTY; | ||
| process.stdout.isTTY = true; | ||
|
|
||
| try { | ||
| await withDevDatabase(async ({ connectionString }) => { | ||
| await withClient(connectionString, async (client) => { | ||
| await client.query(` | ||
| CREATE TABLE IF NOT EXISTS "user" ( | ||
| id SERIAL PRIMARY KEY, | ||
| email TEXT NOT NULL | ||
| ) | ||
| `); | ||
| }); | ||
|
|
||
| // Snapshot test for JSON output | ||
| expect(normalized).toMatchSnapshot(); | ||
| }); | ||
| }, | ||
| timeouts.spinUpPpgDev, | ||
| ); | ||
| const testSetup = setupTestDirectoryFromFixtures( | ||
| createTempDir, | ||
| fixtureSubdir, | ||
| 'prisma-next.config.with-db.ts', | ||
| { '{{DB_URL}}': connectionString }, | ||
| ); | ||
| const configPath = testSetup.configPath; | ||
|
|
||
| // Emit contract first | ||
| const emitCommand = createContractEmitCommand(); | ||
| const originalCwd = process.cwd(); | ||
| try { | ||
| process.chdir(testSetup.testDir); | ||
| await executeCommand(emitCommand, ['--config', configPath, '--no-color']); | ||
| } finally { | ||
| process.chdir(originalCwd); | ||
| } | ||
|
|
||
| const command = createDbSignCommand(); | ||
| try { | ||
| process.chdir(testSetup.testDir); | ||
| await executeCommand(command, ['--config', configPath, '--no-color']); | ||
| } finally { | ||
| process.chdir(originalCwd); | ||
| } | ||
|
|
||
| // Check that blank line was added after spinner output | ||
| const output = consoleOutput.join('\n'); | ||
| // The blank line appears after spinner messages, so we check for double newline | ||
| // or that output contains the sign result after a blank line | ||
| expect(output).toContain('Database signed'); | ||
| }); | ||
| } finally { | ||
| process.stdout.isTTY = originalIsTTY; | ||
| } | ||
| }, | ||
| timeouts.spinUpPpgDev, | ||
| ); |
There was a problem hiding this comment.
Spinner/blank-line test does not currently assert on blank line behavior
The test toggles process.stdout.isTTY, runs the happy path, and then only checks that the output contains “Database signed”. The comments say it should verify that a blank line is added after spinner output, but there’s no assertion on blank lines, so this is effectively duplicating other happy‑path tests and not guarding the spinner formatting.
Consider asserting that there is a blank line before the final result, for example:
- // Check that blank line was added after spinner output
- const output = consoleOutput.join('\n');
- // The blank line appears after spinner messages, so we check for double newline
- // or that output contains the sign result after a blank line
- expect(output).toContain('Database signed');
+ // Check that blank line was added after spinner output
+ const output = consoleOutput.join('\n');
+ // Look for a blank line (double newline) before the final sign result line
+ expect(output).toMatch(/\n\s*\n[^\n]*Database signed/);This keeps the test aligned with its description while staying tolerant of spinner message content.
🤖 Prompt for AI Agents
In packages/framework/tooling/cli/test/db-sign.e2e.test.ts around lines 573-627,
the test claims to verify a blank line after spinner output but only asserts
that output contains "Database signed"; add an assertion that verifies a blank
line precedes the final result. After collecting output (const output =
consoleOutput.join('\n')), add a check that the output contains a double newline
before the result (for example assert(output.includes('\n\nDatabase signed')) or
use a RegExp to accept CRLF/newline variants), keeping the existing "Database
signed" assertion; ensure this new assertion runs inside the same TTY-enabled
try block and does not alter setup/teardown.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/framework/tooling/cli/test/db-schema-verify.e2e.test.ts (1)
142-202: Consider using an object matcher for the JSON envelope assertionsThe JSON-mode success test asserts several related properties (
ok,summary,schema,counts) separately. For readability and to align with the “prefer object matchers” guideline, consider collapsing these into a singletoMatchObjecton the parsed JSON structure.
♻️ Duplicate comments (5)
packages/framework/tooling/cli/test/db-sign.e2e.test.ts (3)
1-15: Good adoption of shared helpers and temp-dir wrapperUsing
withTempDir,setupDbSignFixture, andrunDbSignto drive the e2e flow (DB setup → contract emit → db-sign) significantly reduces duplication across tests and aligns with the CLI e2e helper patterns.
322-371: Good use of object matcher for JSON failure envelopeThe
--jsonfailure test now parses the JSON slice and usestoMatchObjectto assertok: false, a summary containing'does not satisfy contract', and presence ofschema. This aligns with the object-matcher guideline and keeps the test resilient.
397-421: Spinner/blank-line behavior still not assertedAs before, the TTY spinner test confirms only that
'Database signed'appears. It doesn’t check for the blank line that’s meant to follow spinner output, so regressions in spacing wouldn’t be caught.You can tighten this by matching for a blank line immediately before the final summary line (e.g., via a
\n\s*\n.*Database signedregex).packages/framework/tooling/cli/test/utils/test-helpers.ts (1)
393-437: setupDbSignFixture cleanly encapsulates DB + fixture + emit setupThis helper creates/adjusts the schema, prepares a fixture-backed test directory with DB URL injection, and runs
contract-emitbefore returning. It directly addresses earlier repetition in db-sign tests and keeps those tests focused on behavior instead of setup boilerplate.packages/framework/tooling/cli/test/db-schema-verify.e2e.test.ts (1)
674-727: Spinner/blank-line test still doesn’t assert blank-line behaviorThis test toggles
process.stdout.isTTYand then only checks that the success message appears. It doesn’t actually verify a blank line was printed after spinner output, so it may not catch regressions in the spacing behavior.You could assert that a blank line precedes the final result, e.g. using a regex looking for
\n\s*\nbefore the summary line.
| it( | ||
| 'handles missing contract file (ENOENT error)', | ||
| async () => { | ||
| await withDevDatabase(async ({ connectionString }) => { | ||
| const testSetup = setupTestDirectoryFromFixtures( | ||
| createTempDir, | ||
| fixtureSubdir, | ||
| 'prisma-next.config.with-db.ts', | ||
| { '{{DB_URL}}': connectionString }, | ||
| ); | ||
| const configPath = testSetup.configPath; | ||
|
|
||
| // Don't create contract.json - it should be missing | ||
| const command = createDbSignCommand(); | ||
| const originalCwd = process.cwd(); | ||
| try { | ||
| process.chdir(testSetup.testDir); | ||
| await expect( | ||
| executeCommand(command, ['--config', configPath, '--no-color']), | ||
| ).rejects.toThrow(); | ||
| } finally { | ||
| process.chdir(originalCwd); | ||
| } | ||
|
|
||
| // Verify error output (errors go to stderr/consoleErrors) | ||
| const errorOutput = consoleErrors.join('\n'); | ||
| expect(errorOutput).toContain('PN-CLI-4'); | ||
| expect(errorOutput).toMatch(/file.*not found|not found.*file/i); | ||
| }); | ||
| }, | ||
| timeouts.spinUpPpgDev, | ||
| ); | ||
|
|
||
| it( | ||
| 'handles contract file read errors (non-ENOENT)', | ||
| async () => { | ||
| await withDevDatabase(async ({ connectionString }) => { | ||
| const testSetup = setupTestDirectoryFromFixtures( | ||
| createTempDir, | ||
| fixtureSubdir, | ||
| 'prisma-next.config.with-db.ts', | ||
| { '{{DB_URL}}': connectionString }, | ||
| ); | ||
| const configPath = testSetup.configPath; | ||
|
|
||
| // Create a contract file with invalid JSON (causes parse error, not ENOENT) | ||
| const contractPath = resolve(testSetup.testDir, 'src/prisma/contract.json'); | ||
| mkdirSync(dirname(contractPath), { recursive: true }); | ||
| writeFileSync(contractPath, 'invalid json content', 'utf-8'); | ||
|
|
||
| const command = createDbSignCommand(); | ||
| const originalCwd = process.cwd(); | ||
| try { | ||
| process.chdir(testSetup.testDir); | ||
| // JSON.parse throws SyntaxError, which is caught and wrapped as errorUnexpected | ||
| // The command should exit with non-zero code or throw | ||
| await expect( | ||
| executeCommand(command, ['--config', configPath, '--no-color']), | ||
| ).rejects.toThrow(); | ||
| } finally { | ||
| process.chdir(originalCwd); | ||
| } | ||
|
|
||
| // Verify error was handled (command failed) | ||
| // The error path is covered even if we don't check the exact error message format | ||
| // This tests the branch where file read succeeds but JSON.parse fails | ||
| }); | ||
| }, | ||
| timeouts.spinUpPpgDev, | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Rename test to reflect JSON parse error rather than read error
This test writes invalid JSON to contract.json, so the failure mode is a JSON.parse error, not a file read error. Consider renaming the test description to something like 'handles contract JSON parse errors (invalid JSON content)' so it matches the actual behavior and mirrors the db-schema-verify suite.
🤖 Prompt for AI Agents
In packages/framework/tooling/cli/test/db-sign.e2e.test.ts around lines 187 to
256, the second test's description says "handles contract file read errors
(non-ENOENT)" but the test writes invalid JSON so it actually exercises a
JSON.parse failure; update the it(...) description to accurately reflect that
(e.g., "handles contract JSON parse errors (invalid JSON content)") so the test
name matches the behavior and mirrors the db-schema-verify suite.
1 participant
Currently we store the column
typeas the codec identifier. We need to be able to verifythe database schema using only the contract data stored in the database, so without knowing
the column-codec mapping we can't do that.
This makes it much simpler: the contract columns will store
nativeTypeandcodecId.Codec information will be ignored by the migration system and the database when comparing
contract to schema. Application query DSLs can ignore the native type and use the codec ID
exclusively.
Summary of changes to implement the “native + codec types” brief
SQL contract column shape and validation
StorageColumnin@prisma-next/sql-contractto requirenativeType: string+codecId: string+nullable: booleanfor all SQL columns.sql-contractandsql-contract-tsso contracts and storage structures are only valid when bothnativeTypeandcodecIdare present.col,table,storage,contract) and tests to use the new{ nativeType, codecId, nullable }shape everywhere.Contract authoring and example contracts
@prisma-next/sql-contract-ts/contract-builder) so.build():codecIdfrom the column builder state.nativeTypeon each column (set when the column is defined), and throws if it is missing.storage.tables.*.columns.*as{ nativeType, codecId, nullable }.examples/prisma-next-demo/prisma/contract.ts) to use column-type descriptors likeint4Column,textColumn,timestamptzColumn, andvectorColumn, which carry bothnativeTypeandcodecIdinto the builder.Schema IR and verification
SqlColumnIRin@prisma-next/sql-schema-iras:typeId= codec ID,nativeType?= DB-specific type.SqlColumnIR.nativeTypefor introspected columns based oninformation_schema(handling varchar, numeric, etc.).schemaVerifyto:contractColumn.nativeTypetoschemaColumn.nativeType ?? schemaColumn.typeIdfor type compatibility.codecId’s registrynativeTypedisagrees with the contract’snativeType, without blocking structural validation.Migration planner (planMigration)
@prisma-next/sql-migrationssocolumnTypeCompatible:db initrequires (no narrowing; widening allowed).Lanes, runtime, and emitter
codecIdas the only type ID for codecs and type inference (replacing usages ofStorageColumn.type).codecIds from columns.nativeType: stringandcodecId: string.contract.d.tswith the enriched column type{ nativeType, codecId, nullable }, making the generated contract type reflect both DB structure and codec configuration.Summary by CodeRabbit
New Features
Improvements
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.