refactor(sql-orm-client): build PN AST directly instead of using Kysely#234
Merged
refactor(sql-orm-client): build PN AST directly instead of using Kysely#234
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughLarge refactor replacing Kysely SQL compilation with a query-plan pipeline and class-based, fluent SQL AST. Adds row-based batched INSERTs with DEFAULT/ON CONFLICT, DerivedTable lateral joins, BoundWhereExpr/param-offset utilities, SqlQueryPlan propagation and adapter lowering, plus widespread test and fixture migration. Changes
Sequence Diagram(s)sequenceDiagram
participant ORM as ORM Client
participant Planner as Query Planner
participant Runtime as RuntimeScope
participant Adapter as PostgresAdapter
ORM->>Planner: compileSelect(contract, table, state)\n-> SqlQueryPlan(ast, params, meta)
Planner-->>ORM: SqlQueryPlan
ORM->>Runtime: executeQueryPlan(runtimeScope, SqlQueryPlan)
alt plan needs lowering
Runtime->>Adapter: adapter.lower(SqlQueryPlan) -> ExecutionPlan(sql, params)
Adapter-->>Runtime: ExecutionPlan
Runtime->>Runtime: execute(ExecutionPlan) -> AsyncIterableResult
else already-lowered
Runtime->>Runtime: execute(SqlQueryPlan as ExecutionPlan) -> AsyncIterableResult
end
Runtime-->>ORM: rows (AsyncIterableResult)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
@prisma-next/runtime-executor
@prisma-next/sql-runtime
@prisma-next/extension-paradedb
@prisma-next/extension-pgvector
@prisma-next/postgres
@prisma-next/sql-orm-client
@prisma-next/contract-authoring
@prisma-next/contract-ts
@prisma-next/ids
@prisma-next/psl-parser
@prisma-next/cli
@prisma-next/emitter
@prisma-next/eslint-plugin
@prisma-next/migration-tools
@prisma-next/vite-plugin-contract-emit
@prisma-next/sql-contract
@prisma-next/sql-errors
@prisma-next/sql-operations
@prisma-next/sql-schema-ir
@prisma-next/sql-contract-psl
@prisma-next/sql-contract-ts
@prisma-next/sql-contract-emitter
@prisma-next/family-sql
@prisma-next/sql-kysely-lane
@prisma-next/sql-lane-query-builder
@prisma-next/sql-relational-core
@prisma-next/sql-lane
@prisma-next/target-postgres
@prisma-next/adapter-postgres
@prisma-next/driver-postgres
@prisma-next/core-control-plane
@prisma-next/core-execution-plane
@prisma-next/config
@prisma-next/contract
@prisma-next/operations
@prisma-next/plan
@prisma-next/utils
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/3-extensions/sql-orm-client/test/integration/include.test.ts (1)
135-135:⚠️ Potential issue | 🟡 MinorSuspicious
as nevercast for numeric field.Using
as neverto bypass type checking for theviewsfield suggests a type system gap. Ifsum()doesn't accept string field names, the API types may need adjustment rather than using a cast that silences all type errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/test/integration/include.test.ts` at line 135, The test is silencing types by casting the string literal 'views' to never; instead, remove the `as never` and use a properly typed field key (e.g., the model/table's numeric column type or a keyof type) so the call to sum() accepts it without bypassing the type system — locate the `const numericField = 'views' as never;` and change it to a correctly typed identifier (or update the sum() API/type declarations to accept string keys for numeric columns) so the test uses a valid typed field name rather than a never cast.packages/3-extensions/sql-orm-client/src/model-accessor.ts (1)
165-182:⚠️ Potential issue | 🟠 Major
every({})currently means “no related rows”.When
toRelationWhereExpr()returnsundefined—for example onevery({})—theeverybranch leavessubqueryWhere = joinWhereand emitsNOT EXISTS(joinWhere). That excludes parents that do have related rows, even though an empty predicate should be vacuously true forevery.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/src/model-accessor.ts` around lines 165 - 182, The bug is that in the options.mode === 'every' branch we set existsNot = true even when childWhere is undefined (e.g. every({})), which makes the code emit NOT EXISTS(joinWhere) and thus excludes parents that have related rows; to fix, only set existsNot and build subqueryWhere = and(joinWhere, not(childWhere)) when childWhere is present—if childWhere is undefined leave subqueryWhere as joinWhere (or don't invert) so every({}) is treated vacuously true; update the block around toRelationWhereExpr(...), options.mode === 'every', childWhere, existsNot and subqueryWhere accordingly.
🧹 Nitpick comments (21)
packages/3-extensions/sql-orm-client/src/query-plan-meta.ts (1)
6-15: UseifDefined()for conditional object spread.The inline conditional spread for
profileHashshould use theifDefined()utility per coding guidelines.♻️ Suggested refactor
+import { ifDefined } from '@prisma-next/utils/defined'; import type { PlanMeta } from '@prisma-next/contract/types'; import type { SqlContract, SqlStorage } from '@prisma-next/sql-contract/types'; import type { QueryAst } from '@prisma-next/sql-relational-core/ast'; import type { SqlQueryPlan } from '@prisma-next/sql-relational-core/plan'; export function buildOrmPlanMeta(contract: SqlContract<SqlStorage>): PlanMeta { return { target: contract.target, targetFamily: contract.targetFamily, storageHash: contract.storageHash, - ...(contract.profileHash !== undefined ? { profileHash: contract.profileHash } : {}), + ...ifDefined('profileHash', contract.profileHash), lane: 'orm-client', paramDescriptors: [], }; }As per coding guidelines: "Use
ifDefined()from@prisma-next/utils/definedfor conditional object spreads instead of inline conditional spread patterns".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/src/query-plan-meta.ts` around lines 6 - 15, The buildOrmPlanMeta function uses an inline conditional spread for profileHash; import and use the ifDefined utility from '@prisma-next/utils/defined' to conditionally add profileHash to the returned object instead of the ternary spread. Update the return object in buildOrmPlanMeta to spread ifDefined(contract.profileHash, { profileHash: contract.profileHash }) and add the import for ifDefined at the top of the file.packages/3-extensions/sql-orm-client/src/where-utils.ts (1)
9-14: Simplify the implementation.The
firstFilter !== undefinedcheck is defensive but technically unnecessary sincefilters[0]is guaranteed to exist whenfilters.length === 1(the type isWhereExpr[], not(WhereExpr | undefined)[]). Also, spreading[...filters]is unnecessary sincecreateAndExpralready acceptsReadonlyArray<WhereExpr>.♻️ Suggested simplification
export function combineWhereFilters(filters: readonly WhereExpr[]): WhereExpr | undefined { if (filters.length === 0) { return undefined; } - const firstFilter = filters[0]; - if (filters.length === 1 && firstFilter !== undefined) { - return firstFilter; + if (filters.length === 1) { + return filters[0]; } - return createAndExpr([...filters]); + return createAndExpr(filters); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/src/where-utils.ts` around lines 9 - 14, The code in the function that builds the combined WHERE expression can be simplified: remove the redundant defensive check against undefined (the array is typed as WhereExpr[] so filters[0] exists when filters.length === 1) and stop creating a shallow copy before calling createAndExpr; change the branch that currently uses firstFilter and createAndExpr([...filters]) to directly return filters[0] when filters.length === 1, and otherwise call createAndExpr(filters) (referencing the variables filters, firstFilter and the function createAndExpr to locate the change).packages/3-extensions/sql-orm-client/src/execute-query-plan.ts (1)
5-9: Consider exportingQueryPlanExecutorinterface.The
QueryPlanExecutorinterface is currently not exported but mirrorsRuntimeScope.execute. If this interface is intentionally separate for abstraction purposes (e.g., to allow different executor implementations), consider exporting it. Otherwise, theexecuteQueryPlanfunction could acceptRuntimeScopedirectly to avoid interface duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/src/execute-query-plan.ts` around lines 5 - 9, The QueryPlanExecutor interface is currently internal but duplicates RuntimeScope.execute; either export QueryPlanExecutor for reuse or simplify by changing executeQueryPlan's parameter to accept RuntimeScope directly. Update the interface declaration (QueryPlanExecutor) to be exported if you want an abstraction surface, or modify the executeQueryPlan signature to take scope: RuntimeScope and remove the local interface; ensure any call sites constructing executors are updated to match the exported interface or the RuntimeScope parameter change.packages/3-extensions/sql-orm-client/test/where-interop.test.ts (1)
149-150: UsecreateTableRef()in these join-source expectations.These inline
{ kind: 'table', name: ... }objects repeat AST structure in a test that is otherwise validating behavior, not constructor details. Swapping them to the table-ref factory will make this expectation less brittle.As per coding guidelines, "Use factory functions (createColumnRef, createParamRef, createLiteralExpr, createTableRef, createBinaryExpr) from
@prisma-next/sql-relational-core/astinstead of manual object creation when creating AST nodes in tests".Also applies to: 160-161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/test/where-interop.test.ts` around lines 149 - 150, Replace the inline join-source table objects (e.g. the { kind: 'table', name: 'posts' } used as the source in join expectations) with the createTableRef factory from `@prisma-next/sql-relational-core/ast`; import createTableRef if not present and use it to construct the same table reference for both occurrences (the one shown and the similar one at 160-161) so the tests use the endorsed factory instead of manual AST objects.packages/3-extensions/sql-orm-client/test/query-plan-mutations.test.ts (1)
35-52: Use AST factory helpers in these expectations.These inline node literals duplicate the AST shape, so this test will need churn every time
ParamRef/ColumnRefchanges. PrefercreateParamRef()andcreateColumnRef()here, and keep only thedefaultcell manual if there is no factory for it yet.As per coding guidelines, "Use factory functions (createColumnRef, createParamRef, createLiteralExpr, createTableRef, createBinaryExpr) from
@prisma-next/sql-relational-core/astinstead of manual object creation when creating AST nodes in tests".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/test/query-plan-mutations.test.ts` around lines 35 - 52, Replace the inline AST object literals in the assertions with the AST factory helpers: use createParamRef(...) for each param cell and createColumnRef(...) for each returning column; keep the { kind: 'default' } cell as-is if no factory exists. Update expectations for plan.ast.rows[0], plan.ast.rows[1] to call createParamRef with the appropriate index and name (e.g., matching ids 1..7) instead of manual { kind: 'param', index: ..., name: ... } objects, and replace entries in plan.ast.returning with createColumnRef('users', 'id'|'name'|'email'|'invited_by_id') calls. Ensure imports for createParamRef and createColumnRef from `@prisma-next/sql-relational-core/ast` are added/updated.packages/2-sql/4-lanes/relational-core/src/ast/builders.ts (1)
25-33: Reuse the sharedifDefined()utility here.This helper duplicates the repo-standard conditional-spread utility and adds another local pattern in core code. Import the canonical helper instead of carrying a private copy in each builder module.
As per coding guidelines, "Use ifDefined() from
@prisma-next/utils/definedfor conditional object spreads instead of inline conditional spread patterns".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/relational-core/src/ast/builders.ts` around lines 25 - 33, Replace the local duplicate ifDefined function in this module with the canonical helper: remove the local function declaration named ifDefined and import the shared ifDefined from `@prisma-next/utils/defined`, then update any local usages to call the imported ifDefined (keeping the same generic key/value usage) so types remain compatible; ensure you only export/import the helper and delete the duplicate implementation in builders.ts.packages/2-sql/4-lanes/sql-lane/test/sql-joins.builder.test.ts (1)
184-185: UsecreateTableRef()for the join sources in these expectations.The new
sourceassertions are still hand-written table AST nodes. Using the factory keeps these tests aligned with the public AST helpers and reduces churn ifTableSourcepicks up more fields later.As per coding guidelines, "Use factory functions (createColumnRef, createParamRef, createLiteralExpr, createTableRef, createBinaryExpr) from
@prisma-next/sql-relational-core/astinstead of manual object creation when creating AST nodes in tests".Also applies to: 219-220, 230-231
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-lane/test/sql-joins.builder.test.ts` around lines 184 - 185, Replace the hand-crafted table AST objects used as join sources (e.g., source: { kind: 'table', name: 'post' }) with the factory helper createTableRef(...) from `@prisma-next/sql-relational-core/ast` so the tests use createTableRef('post') for those expectations; update each occurrence noted (the one at the shown diff and the additional occurrences around the other mentioned expectations) so the join source assertions call createTableRef instead of manual objects, keeping other surrounding expectation structure intact.packages/2-sql/4-lanes/sql-lane/test/mutation-builder.test.ts (1)
330-338: Assert the generated row more strictly.This only proves that row 0 has some
idobject. A regression that emits the wrong cell kind, adds extra columns, or changes row ordering would still pass. It's worth comparingrowsexactly here, since this test is specifically about generated-default insert shaping.As per coding guidelines, "Compare entire plan structures using single
toEqualassertions rather than piece-by-piece checks. Verify complete AST structures to ensure all fields are present and correct, not just a subset."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-lane/test/mutation-builder.test.ts` around lines 330 - 338, The test currently only checks that plan.ast.rows[0] contains some id object and that plan.params has length 2; replace these piecemeal checks with a single strict equality assertion that compares the entire generated plan (including plan.ast and plan.params) using toEqual so the rows array, each cell kind and order, and params are verified exactly; specifically, construct the expected plan object mirroring the full AST structure (with the insert kind, the exact rows array and cell entries for id and any other columns) and use expect(plan).toEqual(expectedPlan) (use expect.any(...) only where a dynamic value is unavoidable) so the test fails on wrong cell kinds, extra/missing columns or row reordering.packages/2-sql/4-lanes/sql-lane/test/sql-dml.test.ts (1)
73-82: Assert the full insert AST here.These are the main regression tests for the new
rowsshape, buttoMatchObject()will still pass if extra fields appear or required fields disappear. Please switch these expectations to a singletoEqual()on the whole AST.🧪 Example tightening for these assertions
- expect(plan.ast).toMatchObject({ + expect(plan.ast).toEqual({ kind: 'insert', - table: { name: 'user' }, + table: { kind: 'table', name: 'user' }, rows: [ { email: { kind: 'param', name: 'email', index: 1 }, createdAt: { kind: 'param', name: 'createdAt', index: 2 }, }, ], });Apply the same pattern to the
returningcase below.As per coding guidelines, "Compare entire plan structures using single
toEqualassertions rather than piece-by-piece checks. Verify complete AST structures to ensure all fields are present and correct, not just a subset."Also applies to: 106-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-lane/test/sql-dml.test.ts` around lines 73 - 82, The test uses toMatchObject on plan.ast which permits missing or extra fields; update the assertion(s) to assert the entire AST exactly using toEqual against the full expected AST object (replace the current toMatchObject block for plan.ast with a single expect(plan.ast).toEqual({...}) containing every field the AST should have, and do the same for the later `returning` case so both insert and returning assertions verify the complete structure rather than partial matches.packages/3-extensions/sql-orm-client/test/integration/group-by.test.ts (1)
64-69: Tighten thehavingAST assertion.
expect(ast.having?.kind).toBe('bin')only proves that some binary HAVING node exists. A regression to the wrong operator or wrong aggregate operand would still pass. Since this test now validates AST output instead of SQL text, please assert the fullhavingshape here.As per coding guidelines, "Compare entire plan structures using single
toEqualassertions rather than piece-by-piece checks. Verify complete AST structures to ensure all fields are present and correct, not just a subset."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/test/integration/group-by.test.ts` around lines 64 - 69, Replace the partial assertion expect(ast.having?.kind).toBe('bin') with a full-structure equality check against the exact expected HAVING AST node: build an expectedHaving object that mirrors the complete shape (kind, op, left/right subnodes and any aggregate/column nodes) and assert expect(ast.having).toEqual(expectedHaving); keep the existing isSelectAst check and use runtime.executions[0]?.ast to locate the AST used in the assertion.packages/3-extensions/sql-orm-client/test/collection-mutation-dispatch.test.ts (1)
11-27: Consider using AST factory functions instead of manual object creation.The
makeCompiledhelper manually constructs AST nodes. Per coding guidelines, prefer factory functions from@prisma-next/sql-relational-core/astfor AST node creation in tests.♻️ Suggested refactor using factories
+import { + createLiteralExpr, + createProjectionItem, + createSelectAstBuilder, + createTableSource, +} from '@prisma-next/sql-relational-core/ast'; import type { SqlQueryPlan } from '@prisma-next/sql-relational-core/plan'; function makeCompiled(sqlText = 'select 1'): SqlQueryPlan<Record<string, unknown>> { + const ast = createSelectAstBuilder(createTableSource('users')) + .project([createProjectionItem('_sql', createLiteralExpr(sqlText))]) + .build(); return { - ast: { - kind: 'select', - from: { kind: 'table', name: 'users' }, - project: [{ alias: '_sql', expr: { kind: 'literal', value: sqlText } }], - }, + ast, params: [], meta: { target: 'postgres',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/test/collection-mutation-dispatch.test.ts` around lines 11 - 27, Replace the manual AST object construction inside the helper function makeCompiled with the AST factory functions exported from `@prisma-next/sql-relational-core/ast`: use the factory for a select node, a table/from node, a literal expression node, and project item nodes, and assemble them into the SqlQueryPlan return (preserving params and meta). Update makeCompiled to import and call those factories (rather than creating plain objects) so the test builds ASTs consistently with the library's types and helpers.packages/2-sql/4-lanes/sql-lane/src/sql/join-builder.ts (1)
17-17: Consider extracting the source creation for readability.The line is dense with nested calls. A local variable would improve readability.
♻️ Suggested refactor
const onExpr = createJoinOnExpr(leftCol, rightCol); - return createJoin(join.joinType, createTableSource(join.table.name, join.table.alias), onExpr, false); + const source = createTableSource(join.table.name, join.table.alias); + return createJoin(join.joinType, source, onExpr, false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-lane/src/sql/join-builder.ts` at line 17, The return statement is dense with nested calls; extract the table source creation into a local variable to improve readability and maintainability. For example, call createTableSource(join.table.name, join.table.alias) once into a const (e.g., tableSource) and then pass that variable into createJoin(join.joinType, tableSource, onExpr, false) instead of nesting createTableSource directly inside createJoin; update the code around createJoin, createTableSource, join.joinType, join.table.name, join.table.alias, and onExpr accordingly.packages/3-extensions/sql-orm-client/test/integration/include.test.ts (1)
1-598: Test file exceeds 500 line limit.At ~598 lines, this file exceeds the 500-line guideline. Consider splitting by functionality (e.g.,
include.basic.test.tsfor multi-query includes,include.lateral.test.tsfor single-query lateral tests,include.correlated.test.tsfor correlated subquery tests). As per coding guidelines: "Keep test files under 500 lines to maintain readability and navigability."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/test/integration/include.test.ts` around lines 1 - 598, The test file describe('integration/include') is over 500 lines; split it into smaller test files by functionality to meet the guideline. Create separate test files (e.g., include.basic.test.ts, include.lateral.test.ts, include.correlated.test.ts) and move related tests into them: multi-query/basic include tests (those that use createUsersCollection, createPostsCollection, seedUsers/seedPosts/seedProfiles/seedComments and assertions about rows and runtime.executions), lateral-specific tests (those that call createUsersCollectionWithCapabilities with lateral and jsonAgg enabled and assert AST/join structure), and correlated/jsonAgg tests (those that enable only jsonAgg and assert subquery projections and SQL correlation). Ensure each new file imports the same helpers (withCollectionRuntime, createUsersCollection, createUsersCollectionWithCapabilities, seed* helpers) and preserves test timeouts and isSelectAst/type checks so each suite remains self-contained.packages/2-sql/5-runtime/test/lints.test.ts (1)
166-175: UsecreateColumnRef()for the derived-table projection.Line 174 reintroduces a manual AST node literal. Keeping test fixtures on the AST factories makes this coverage less brittle as the AST surface evolves.
♻️ Suggested cleanup
const selectAst = createSelectAst({ from: createDerivedTableSource( 'user_ids', createSelectAst({ from: userTable, project: [{ alias: 'id', expr: idCol }], }), ), - project: [{ alias: 'id', expr: { kind: 'col', table: 'user_ids', column: 'id' } }], + project: [{ alias: 'id', expr: createColumnRef('user_ids', 'id') }], });As per coding guidelines, "Use factory functions (createColumnRef, createParamRef, createLiteralExpr, createTableRef, createBinaryExpr) from
@prisma-next/sql-relational-core/astinstead of manual object creation when creating AST nodes in tests".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/5-runtime/test/lints.test.ts` around lines 166 - 175, Replace the manual column AST literal used in the derived-table projection with the AST factory: when building the inner projection passed to createDerivedTableSource (the object currently using { alias: 'id', expr: idCol } or the outer project using { kind: 'col', table: 'user_ids', column: 'id' }), use createColumnRef(...) from `@prisma-next/sql-relational-core/ast` instead of a raw object; update the inner projection entry so the expr is produced by createColumnRef(idCol.column or appropriate identifier) and similarly replace any manual { kind: 'col', table: 'user_ids', column: 'id' } with createColumnRef('id', 'user_ids') (or the factory signature used in the codebase) so tests consistently use createSelectAst, createDerivedTableSource and createColumnRef factories.packages/2-sql/4-lanes/relational-core/test/ast/builders.test.ts (1)
20-47: Assert the full builder output, not just selected fields.These tests mostly check fragments of the generated AST, so changes in unasserted fields can slip through unnoticed. In this file, a single
expect(ast).toEqual(...)per case will pin the builder surface much more effectively.As per coding guidelines, "Compare entire plan structures using single
toEqualassertions rather than piece-by-piece checks. Verify complete AST structures to ensure all fields are present and correct, not just a subset."Also applies to: 49-68, 70-86, 88-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/relational-core/test/ast/builders.test.ts` around lines 20 - 47, The test only asserts selected fields of the AST causing unnoticed regressions; replace the fragmented asserts in the 'builds select ast with fluent low-level builder' test with a single deep equality check against the full expected AST object produced by createSelectAstBuilder(userTable)...build(); construct an expected AST object that includes all properties used in the builder (kind, from, project, where, orderBy, distinct, distinctOn, groupBy, having, limit, offset, etc.) using the same symbols (userTable, idExpr, emailExpr, where, createParamRef calls) and then call expect(ast).toEqual(expectedAst); apply the same single toEqual style for the other test cases referenced (those around lines 49-68, 70-86, 88-131) for functions/builders under test.packages/2-sql/4-lanes/relational-core/test/ast/select.test.ts (1)
136-158: Extend the omission coverage for the new Select fields.This adds the happy path, but the existing “removes undefined/empty” tests were not updated for
distinct,distinctOn,groupBy,having, oroffset. IfcreateSelectAst()starts retainingdistinctOn: []orgroupBy: [], this suite will miss it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/relational-core/test/ast/select.test.ts` around lines 136 - 158, The existing test validates the positive case for distinct/distinctOn/groupBy/having/offset but does not update the "removes undefined/empty" coverage to ensure createSelectAst cleans up empty/undefined values; update the test suite (the test that previously asserted removal of undefined/empty fields) to include assertions for createSelectAst when passed distinct: false/undefined, distinctOn: [], groupBy: [], having: undefined, and offset: undefined — ensure that the resulting Select AST produced by createSelectAst does not contain keys or contains them as undefined/null as per project convention (reference createSelectAst, and the existing "removes undefined/empty" test name) so regressions that retain empty arrays or falsy flags are caught.packages/3-extensions/sql-orm-client/test/where-interop.select-includes.test.ts (1)
29-89: Prefer AST factories over inline node literals in these new cases.These tests hand-assemble
select,derivedTable,join,subquery, andbinnodes. Switching them to the canonical AST factories/builders will keep the assertions focused on behavior instead of raw shape churn as the AST surface keeps moving.As per coding guidelines, "Use factory functions (createColumnRef, createParamRef, createLiteralExpr, createTableRef, createBinaryExpr) from
@prisma-next/sql-relational-core/astinstead of manual object creation when creating AST nodes in tests".Also applies to: 129-210
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/test/where-interop.select-includes.test.ts` around lines 29 - 89, Replace inline AST node literals in the test (the select/derivedTable/join/subquery/bin nodes and their children) with the canonical AST factory functions: use createTableRef for table nodes, createColumnRef for col(...), createParamRef for param(...), createBinaryExpr for bin expressions, and createLiteralExpr or the appropriate factory for literals/subquery projections; update the nested select/derivedTable/join/subquery constructions to call those factories so the test builds nodes via `@prisma-next/sql-relational-core/ast` helpers instead of raw object literals (apply same changes to the other cases around lines 129-210).packages/3-extensions/sql-orm-client/src/where-interop.ts (1)
326-337: UseifDefined()for the optionalorderByspread.This file already has the helper in scope, but this branch reintroduces the inline conditional spread pattern the repo explicitly avoids. As per coding guidelines, "Use
ifDefined()from@prisma-next/utils/definedfor conditional object spreads instead of inline conditional spread patterns."♻️ Suggested cleanup
case 'jsonArrayAgg': return { ...expr, expr: replaceParamsInExpression(expr.expr, params), - ...(expr.orderBy - ? { - orderBy: expr.orderBy.map((order) => ({ - ...order, - expr: replaceParamsInExpression(order.expr, params), - })), - } - : {}), + ...ifDefined( + 'orderBy', + expr.orderBy?.map((order) => ({ + ...order, + expr: replaceParamsInExpression(order.expr, params), + })), + ), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/src/where-interop.ts` around lines 326 - 337, The 'jsonArrayAgg' case currently uses an inline conditional spread for the optional orderBy; replace that pattern by importing/using ifDefined from '@prisma-next/utils/defined' to conditionally add the orderBy key. Specifically, keep the top-level spread of expr and the expr: replaceParamsInExpression(expr.expr, params), then wrap the mapped orderBy block with ifDefined(() => ({ orderBy: expr.orderBy.map(order => ({ ...order, expr: replaceParamsInExpression(order.expr, params) })) })) so the orderBy property is only added when present, referencing the jsonArrayAgg branch and replaceParamsInExpression helper.packages/2-sql/4-lanes/sql-lane/test/sql-includes.ast.test.ts (1)
35-38: Assert the full join shape after the existence check.After
expect(join).toBeDefined(), continuing withjoin?.weakens the failure mode, and these assertions still only cover a tiny slice of the produced AST. A singletoEqualonjoin!would make this much harder to regress. As per coding guidelines, "Prefer assertions over defensive checks when data is guaranteed to be valid - use non-null assertions" and "Compare entire plan structures using singletoEqualassertions rather than piece-by-piece checks."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-lane/test/sql-includes.ast.test.ts` around lines 35 - 38, The test weakly checks the join after verifying existence; replace the defensive chained checks with a single strict assertion using a non-null assertion on join (from selectAst.joins) and compare the entire join object with toEqual, e.g. assert expect(join!).toEqual({...}) where the expected shape includes lateral: true and source: { kind: 'derivedTable', ... } plus any other expected fields so the assertion verifies the full join AST rather than piecewise optional checks.packages/2-sql/4-lanes/relational-core/src/ast/types.ts (1)
16-16: Drop theTableRefmigration alias from the public AST.
TableSource/FromSourceis the new vocabulary. KeepingTableRef = TableSourcehere, pluscreateTableRef()inpackages/2-sql/4-lanes/relational-core/src/ast/common.tsLines 30-32, leaves two public names for the same concept and extends the legacy surface this refactor is supposed to remove.As per coding guidelines, "Do not add backward-compatibility shims, migration scaffolding, deprecation warnings, or comments explaining legacy vs. new approaches" and "Remove old code entirely instead of preserving it with backward-compatibility shims".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/relational-core/src/ast/types.ts` at line 16, Remove the legacy alias and constructor: delete the exported type alias TableRef (export type TableRef = TableSource) and remove the createTableRef() function and its export so only TableSource/FromSource remain; then update any code that imports or uses TableRef or createTableRef to use TableSource (or the existing FromSource constructor/factory) instead, ensuring there are no remaining references to TableRef or createTableRef in the codebase.packages/2-sql/4-lanes/relational-core/src/ast/common.ts (1)
22-27: Avoid cast-based AST factories.
compact(...) as ...turns these helpers into unchecked constructors, so the compiler stops enforcing the shape of the new AST right where it is created. Prefer explicit object construction, or makecompact()preserve the exact return type.As per coding guidelines, "Replace blind casts with type predicates instead of
ascasts".Also applies to: 101-106, 133-138
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/relational-core/src/ast/common.ts` around lines 22 - 27, The factory createTableSource uses a blind cast ("compact(...) as TableSource") which disables compiler shape checks; replace the cast by constructing a properly-typed object or by making compact preserve the exact generic type and/or return a type-guard: e.g. declare a local variable typed as TableSource (const src: TableSource = { kind: 'table', name, alias }) and then call compact(src) or change compact's signature to compact<T>(obj: T): T so you no longer need "as"; apply the same pattern to the other AST factory helpers that currently use compact(...) as ... to remove unchecked casts and satisfy the "replace blind casts with type predicates" guideline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/2-sql/4-lanes/kysely-lane/src/transform/transform-dml.ts`:
- Around line 141-167: The code currently defers calling validateColumn until
after handling undefined or DefaultInsertValueNode, letting misspelled/invalid
insert columns pass when the value is omitted or DEFAULT; move the
validateColumn(ctx.contract, tableRef.name, columnName) call (and the
ctx.refsTables.add(tableRef.name) and ctx.refsColumns.set(...) bookkeeping) to
immediately after columnName is resolved (after getColumnName succeeds) and
before inspecting valueNode/DefaultInsertValueNode/undefined so that every
declared INSERT column is validated regardless of whether its value is omitted
or DEFAULT; keep the existing error throw for missing columnName and preserve
createDefaultValueExpr usage for the undefined/DefaultInsertValueNode branches.
- Around line 94-105: The insert transform currently ignores
InsertQueryNode.onConflict, causing upserts to be dropped; update
transformInsert (the branch handling node.defaultValues === true and the general
insert mapping) to detect node.onConflict, transform it and set
InsertAst.onConflict accordingly (e.g. call a helper like
transformOnConflict(node.onConflict, ctx, tableRef.name) or the existing
conflict transformer in this module) and include the resulting value in the
returned object alongside table, rows and returning so the generated AST
preserves conflict-resolution clauses.
In `@packages/2-sql/4-lanes/relational-core/README.md`:
- Around line 96-100: The README currently lists DefaultValueExpr under
"Expressions" which implies it's a general expression node; update the
documentation to remove `DefaultValueExpr` from the Expressions bullet and
instead mention it only under the "Inserts" section (e.g., document that
`InsertAst.rows` supports `InsertValue` cells and may contain `DEFAULT` via the
`DefaultValueExpr`/insert-only sentinel). Ensure references to
`DefaultValueExpr` in the surrounding bullets (Expressions, Predicates,
Lane-agnostic filter interop, Joins) are removed so readers only see `DEFAULT`
described alongside `InsertAst.rows`/`InsertValue`.
In `@packages/2-sql/4-lanes/relational-core/src/ast/builders.ts`:
- Around line 198-199: The rows(...) method in the builder currently converts an
explicit empty array into a single default-row ([{}]) which causes an unexpected
INSERT DEFAULT VALUES; change the assignment in rows to preserve an empty array:
set this.rowValues = rows.map(row => ({ ...row })) (and if rows is empty keep it
as []), rather than coercing to [{}]; update any downstream expectations that
relied on the implicit default-row (e.g., code reading this.rowValues in the
same builder) to handle an empty array as “no rows” or fail fast as intended.
In `@packages/2-sql/4-lanes/relational-core/src/ast/join.ts`:
- Around line 6-18: createJoin currently allows lateral=true for any FromSource;
add a runtime validation in createJoin that rejects/throws when lateral is true
but the provided source is a plain table (i.e., a TableSource) and only allow
lateral when source is a DerivedTableSource (subquery). Locate createJoin and
use the unique symbols FromSource, TableSource (or check source.kind ===
'table') and DerivedTableSource to implement the guard, throwing a clear error
like "LATERAL is only valid for derived tables" if the check fails so invalid
ASTs cannot be constructed.
In `@packages/2-sql/4-lanes/relational-core/src/ast/types.ts`:
- Around line 184-190: The AggregateExpr type currently allows expr to be
optional for all AggregateFn values; change the AST so only the 'count' variant
permits a missing expr by splitting AggregateExpr into two discriminated
interfaces (e.g., AggregateCountExpr with fn: 'count' and optional expr, and
AggregateOpExpr with fn: 'sum'|'avg'|'min'|'max' and a required expr) and update
the AggregateFn union/type accordingly; then update createAggregateExpr() to
enforce these shapes at construction time (allow no expr only when fn ===
'count' and require an expr for the other fns).
In `@packages/2-sql/4-lanes/sql-lane/src/sql/include-builder.ts`:
- Around line 217-227: The current logic in include.childOrderBy handling
(involving orderBy, OrderBuilder, isOperationExpr, extractBaseColumnRef,
createColumnRef, createOrderByItem) reduces computed sort expressions to their
base column and thus changes query semantics; instead, if orderBy.expr is an
operation expression (isOperationExpr(orderExpr)), pass the operation expression
through as the order expression (use orderExpr directly) rather than calling
extractBaseColumnRef/createColumnRef, and only fall back to createColumnRef when
the expr is a plain column reference; ensure the value passed into
createOrderByItem remains the correct expression type (cast if needed) so
computed expressions like lower(...) or coalesce(...) are preserved.
- Around line 301-306: The inner SELECT built by childRowsBuilder uses
createTableSource(include.table.name) which drops include.table.alias and can
break ON predicates for aliased/self-relations; change the table source to
preserve the alias by using createTableSource with both include.table.name and
include.table.alias (or otherwise set the source alias) so the inner FROM binds
include.table.alias (refer to include.table.alias, include.table.name,
rowsAlias, childRowsBuilder, createTableSource, createSelectAstBuilder).
In `@packages/2-sql/4-lanes/sql-lane/test/sql-includes.ast.test.ts`:
- Around line 41-54: The test's subquery never references the outer "user" alias
so it isn't correlated; update the childQuery built with
createSelectAstBuilder(createTableSource('post')) to include a reference to the
outer alias (for example add a WHERE predicate like post.user_id = user.id or
add a projection that uses createColumnRef('user','id')) so the subquery becomes
correlated, ensuring the posts projection (created with
createProjectionItem('posts', createSubqueryExpr(childQuery))) actually models a
correlated subquery; alternatively, if a plain subquery is intended, rename the
test to reflect that behavior.
In `@packages/3-extensions/sql-orm-client/package.json`:
- Line 26: Remove the unused dependency entry for "@prisma-next/utils":
"workspace:*" from this package's package.json dependencies; locate the
dependencies object in packages/3-extensions/sql-orm-client/package.json and
delete the "@prisma-next/utils" key so it is no longer listed among the
package's dependencies.
In `@packages/3-extensions/sql-orm-client/src/where-interop.ts`:
- Around line 350-363: In the 'operation' branch of replaceParamsInExpression,
don't convert arg.kind === 'param' into a LiteralExpr via paramRefToLiteral
because that strips codec-aware casts; instead preserve the param node (or pass
it through replaceParamsInExpression) so adapter rendering in adapter.ts can see
a param-kind and emit ::vector/::jsonb casts. Update replaceParamsInExpression's
case 'operation' to stop calling paramRefToLiteral for operation args and leave
param nodes intact.
In `@packages/3-extensions/sql-orm-client/test/grouped-collection.test.ts`:
- Around line 100-117: The test currently only extracts aggregate function names
into havingFns (from runtime.executions -> entry.plan.ast.having) and thus loses
the comparison operators; update the mapping to return the full
aggregate/operator pair (e.g., {fn: having.left.fn, op: having.kind === 'bin' ?
having.op : undefined}) or a string like "sum:gt" for each execution where
isSelectAst(entry.plan.ast) and entry.plan.ast.having is a binary aggregate,
then filter undefined and assert that you have six entries and that the set
equals the six expected aggregate/operator pairs (including eq/neq/gt/lt/gte/lte
and both count cases) instead of only checking function names in havingFns.
- Around line 35-45: The test currently only asserts that firstAst.having?.kind
is 'bin' — update the assertions to ensure the HAVING's left operand is an
aggregate COUNT: after confirming isSelectAst(firstAst) and that
firstAst.having?.kind === 'bin', assert that (firstAst.having.left).kind ===
'aggregate' and that (firstAst.having.left).fn === 'count' (or equivalent
property used for function name), so the HAVING is compiled from count().gte(1)
rather than a plain column comparison; also keep the existing checks for
totalViewsProjection and its aggregate fn === 'sum'.
In `@packages/3-extensions/sql-orm-client/test/integration/create.test.ts`:
- Around line 141-159: The test incorrectly uses a synchronous throw check for
the async API users.createAll; change the assertion to await the promise and use
rejects: replace the expect(() => users.createAll([...])).toThrow(...) pattern
with await expect(users.createAll([...])).rejects.toThrow(/requires contract
capability "returning"/); keep the other create() assertion as-is and ensure the
test function is async so awaiting works.
- Around line 95-121: The test passes invitedById: null for the second row but
asserts the AST/sql uses DEFAULT, so update the fixture passed to
users.createCount to exercise the DEFAULT path: make the second object omit
invitedById (or set invitedById: undefined) instead of null so the insert
planner will emit invited_by_id: { kind: 'default' } and the VALUES clause
contains "default" for the second row; adjust the second element in the array
given to users.createCount accordingly.
In `@packages/3-extensions/sql-orm-client/test/integration/upsert.test.ts`:
- Around line 43-62: The test currently uses conflictOn: { id: 2 } which mirrors
the primary key and can hide a bug where upsert() ignores conflictOn; change the
test to drive the conflict via a different unique column (for example use email
as the conflict target) by seeding a row with a different id but the same unique
email, call users.upsert({ create: {...}, update: {...}, conflictOn: { email:
'bob@example.com' } }) and assert the row with that email (not the PK) was
updated; update references in this test (seedUsers,
createReturningUsersCollection, the upsert() call and the expect result) so the
assertion verifies the row matched by conflictOn was updated.
- Around line 89-91: The test imports createTestContract from helpers but
helpers.ts only exports getTestContract, so the import fails; fix by either
exporting createTestContract as an alias to getTestContract in helpers.ts or
update the test to import getTestContract instead and use that when building the
contract (the failing symbols are createTestContract → replace with
getTestContract, and ensure withReturningCapability(createTestContract())
becomes withReturningCapability(getTestContract()) or add export const
createTestContract = getTestContract in helpers).
In `@packages/3-targets/6-adapters/postgres/src/core/adapter.ts`:
- Around line 149-177: renderSelect currently appends ORDER BY/LIMIT/OFFSET at
the outer query level which breaks LATERAL aggregates; update
renderJsonArrayAggExpr and renderSelect so that when building a LATERAL subquery
used in json_agg and both orderBy and (limit or offset) are present you wrap the
child rows in an inner SELECT that applies ORDER BY/LIMIT/OFFSET and projects
each selected column with stable aliases, then change the aggregate to
json_agg(row_to_json(sub.*)) over that inner-select alias. Concretely: in
renderJsonArrayAggExpr detect expr.orderBy && (expr.limit !== undefined ||
expr.offset !== undefined) and instead of aggregating the outer query result,
build innerSql = renderSelect(childAst without outer-level ORDER/LIMIT/OFFSET)
but include ORDER/LIMIT/OFFSET on an inner projection SELECT that aliases every
column (e.g., SELECT col1 AS c1, col2 AS c2 FROM (...child...) AS t ORDER BY ...
LIMIT ... OFFSET ...), then emit json_agg(row_to_json(sub_alias.*)); adjust
renderSelect (used for lateral subqueries) to support returning the inner
projection SQL when requested. Use function names renderSelect and
renderJsonArrayAggExpr and the json_agg(row_to_json(sub.*)) pattern to locate
and implement the change.
---
Outside diff comments:
In `@packages/3-extensions/sql-orm-client/src/model-accessor.ts`:
- Around line 165-182: The bug is that in the options.mode === 'every' branch we
set existsNot = true even when childWhere is undefined (e.g. every({})), which
makes the code emit NOT EXISTS(joinWhere) and thus excludes parents that have
related rows; to fix, only set existsNot and build subqueryWhere =
and(joinWhere, not(childWhere)) when childWhere is present—if childWhere is
undefined leave subqueryWhere as joinWhere (or don't invert) so every({}) is
treated vacuously true; update the block around toRelationWhereExpr(...),
options.mode === 'every', childWhere, existsNot and subqueryWhere accordingly.
In `@packages/3-extensions/sql-orm-client/test/integration/include.test.ts`:
- Line 135: The test is silencing types by casting the string literal 'views' to
never; instead, remove the `as never` and use a properly typed field key (e.g.,
the model/table's numeric column type or a keyof type) so the call to sum()
accepts it without bypassing the type system — locate the `const numericField =
'views' as never;` and change it to a correctly typed identifier (or update the
sum() API/type declarations to accept string keys for numeric columns) so the
test uses a valid typed field name rather than a never cast.
---
Nitpick comments:
In `@packages/2-sql/4-lanes/relational-core/src/ast/builders.ts`:
- Around line 25-33: Replace the local duplicate ifDefined function in this
module with the canonical helper: remove the local function declaration named
ifDefined and import the shared ifDefined from `@prisma-next/utils/defined`, then
update any local usages to call the imported ifDefined (keeping the same generic
key/value usage) so types remain compatible; ensure you only export/import the
helper and delete the duplicate implementation in builders.ts.
In `@packages/2-sql/4-lanes/relational-core/src/ast/common.ts`:
- Around line 22-27: The factory createTableSource uses a blind cast
("compact(...) as TableSource") which disables compiler shape checks; replace
the cast by constructing a properly-typed object or by making compact preserve
the exact generic type and/or return a type-guard: e.g. declare a local variable
typed as TableSource (const src: TableSource = { kind: 'table', name, alias })
and then call compact(src) or change compact's signature to compact<T>(obj: T):
T so you no longer need "as"; apply the same pattern to the other AST factory
helpers that currently use compact(...) as ... to remove unchecked casts and
satisfy the "replace blind casts with type predicates" guideline.
In `@packages/2-sql/4-lanes/relational-core/src/ast/types.ts`:
- Line 16: Remove the legacy alias and constructor: delete the exported type
alias TableRef (export type TableRef = TableSource) and remove the
createTableRef() function and its export so only TableSource/FromSource remain;
then update any code that imports or uses TableRef or createTableRef to use
TableSource (or the existing FromSource constructor/factory) instead, ensuring
there are no remaining references to TableRef or createTableRef in the codebase.
In `@packages/2-sql/4-lanes/relational-core/test/ast/builders.test.ts`:
- Around line 20-47: The test only asserts selected fields of the AST causing
unnoticed regressions; replace the fragmented asserts in the 'builds select ast
with fluent low-level builder' test with a single deep equality check against
the full expected AST object produced by
createSelectAstBuilder(userTable)...build(); construct an expected AST object
that includes all properties used in the builder (kind, from, project, where,
orderBy, distinct, distinctOn, groupBy, having, limit, offset, etc.) using the
same symbols (userTable, idExpr, emailExpr, where, createParamRef calls) and
then call expect(ast).toEqual(expectedAst); apply the same single toEqual style
for the other test cases referenced (those around lines 49-68, 70-86, 88-131)
for functions/builders under test.
In `@packages/2-sql/4-lanes/relational-core/test/ast/select.test.ts`:
- Around line 136-158: The existing test validates the positive case for
distinct/distinctOn/groupBy/having/offset but does not update the "removes
undefined/empty" coverage to ensure createSelectAst cleans up empty/undefined
values; update the test suite (the test that previously asserted removal of
undefined/empty fields) to include assertions for createSelectAst when passed
distinct: false/undefined, distinctOn: [], groupBy: [], having: undefined, and
offset: undefined — ensure that the resulting Select AST produced by
createSelectAst does not contain keys or contains them as undefined/null as per
project convention (reference createSelectAst, and the existing "removes
undefined/empty" test name) so regressions that retain empty arrays or falsy
flags are caught.
In `@packages/2-sql/4-lanes/sql-lane/src/sql/join-builder.ts`:
- Line 17: The return statement is dense with nested calls; extract the table
source creation into a local variable to improve readability and
maintainability. For example, call createTableSource(join.table.name,
join.table.alias) once into a const (e.g., tableSource) and then pass that
variable into createJoin(join.joinType, tableSource, onExpr, false) instead of
nesting createTableSource directly inside createJoin; update the code around
createJoin, createTableSource, join.joinType, join.table.name, join.table.alias,
and onExpr accordingly.
In `@packages/2-sql/4-lanes/sql-lane/test/mutation-builder.test.ts`:
- Around line 330-338: The test currently only checks that plan.ast.rows[0]
contains some id object and that plan.params has length 2; replace these
piecemeal checks with a single strict equality assertion that compares the
entire generated plan (including plan.ast and plan.params) using toEqual so the
rows array, each cell kind and order, and params are verified exactly;
specifically, construct the expected plan object mirroring the full AST
structure (with the insert kind, the exact rows array and cell entries for id
and any other columns) and use expect(plan).toEqual(expectedPlan) (use
expect.any(...) only where a dynamic value is unavoidable) so the test fails on
wrong cell kinds, extra/missing columns or row reordering.
In `@packages/2-sql/4-lanes/sql-lane/test/sql-dml.test.ts`:
- Around line 73-82: The test uses toMatchObject on plan.ast which permits
missing or extra fields; update the assertion(s) to assert the entire AST
exactly using toEqual against the full expected AST object (replace the current
toMatchObject block for plan.ast with a single expect(plan.ast).toEqual({...})
containing every field the AST should have, and do the same for the later
`returning` case so both insert and returning assertions verify the complete
structure rather than partial matches.
In `@packages/2-sql/4-lanes/sql-lane/test/sql-includes.ast.test.ts`:
- Around line 35-38: The test weakly checks the join after verifying existence;
replace the defensive chained checks with a single strict assertion using a
non-null assertion on join (from selectAst.joins) and compare the entire join
object with toEqual, e.g. assert expect(join!).toEqual({...}) where the expected
shape includes lateral: true and source: { kind: 'derivedTable', ... } plus any
other expected fields so the assertion verifies the full join AST rather than
piecewise optional checks.
In `@packages/2-sql/4-lanes/sql-lane/test/sql-joins.builder.test.ts`:
- Around line 184-185: Replace the hand-crafted table AST objects used as join
sources (e.g., source: { kind: 'table', name: 'post' }) with the factory helper
createTableRef(...) from `@prisma-next/sql-relational-core/ast` so the tests use
createTableRef('post') for those expectations; update each occurrence noted (the
one at the shown diff and the additional occurrences around the other mentioned
expectations) so the join source assertions call createTableRef instead of
manual objects, keeping other surrounding expectation structure intact.
In `@packages/2-sql/5-runtime/test/lints.test.ts`:
- Around line 166-175: Replace the manual column AST literal used in the
derived-table projection with the AST factory: when building the inner
projection passed to createDerivedTableSource (the object currently using {
alias: 'id', expr: idCol } or the outer project using { kind: 'col', table:
'user_ids', column: 'id' }), use createColumnRef(...) from
`@prisma-next/sql-relational-core/ast` instead of a raw object; update the inner
projection entry so the expr is produced by createColumnRef(idCol.column or
appropriate identifier) and similarly replace any manual { kind: 'col', table:
'user_ids', column: 'id' } with createColumnRef('id', 'user_ids') (or the
factory signature used in the codebase) so tests consistently use
createSelectAst, createDerivedTableSource and createColumnRef factories.
In `@packages/3-extensions/sql-orm-client/src/execute-query-plan.ts`:
- Around line 5-9: The QueryPlanExecutor interface is currently internal but
duplicates RuntimeScope.execute; either export QueryPlanExecutor for reuse or
simplify by changing executeQueryPlan's parameter to accept RuntimeScope
directly. Update the interface declaration (QueryPlanExecutor) to be exported if
you want an abstraction surface, or modify the executeQueryPlan signature to
take scope: RuntimeScope and remove the local interface; ensure any call sites
constructing executors are updated to match the exported interface or the
RuntimeScope parameter change.
In `@packages/3-extensions/sql-orm-client/src/query-plan-meta.ts`:
- Around line 6-15: The buildOrmPlanMeta function uses an inline conditional
spread for profileHash; import and use the ifDefined utility from
'@prisma-next/utils/defined' to conditionally add profileHash to the returned
object instead of the ternary spread. Update the return object in
buildOrmPlanMeta to spread ifDefined(contract.profileHash, { profileHash:
contract.profileHash }) and add the import for ifDefined at the top of the file.
In `@packages/3-extensions/sql-orm-client/src/where-interop.ts`:
- Around line 326-337: The 'jsonArrayAgg' case currently uses an inline
conditional spread for the optional orderBy; replace that pattern by
importing/using ifDefined from '@prisma-next/utils/defined' to conditionally add
the orderBy key. Specifically, keep the top-level spread of expr and the expr:
replaceParamsInExpression(expr.expr, params), then wrap the mapped orderBy block
with ifDefined(() => ({ orderBy: expr.orderBy.map(order => ({ ...order, expr:
replaceParamsInExpression(order.expr, params) })) })) so the orderBy property is
only added when present, referencing the jsonArrayAgg branch and
replaceParamsInExpression helper.
In `@packages/3-extensions/sql-orm-client/src/where-utils.ts`:
- Around line 9-14: The code in the function that builds the combined WHERE
expression can be simplified: remove the redundant defensive check against
undefined (the array is typed as WhereExpr[] so filters[0] exists when
filters.length === 1) and stop creating a shallow copy before calling
createAndExpr; change the branch that currently uses firstFilter and
createAndExpr([...filters]) to directly return filters[0] when filters.length
=== 1, and otherwise call createAndExpr(filters) (referencing the variables
filters, firstFilter and the function createAndExpr to locate the change).
In
`@packages/3-extensions/sql-orm-client/test/collection-mutation-dispatch.test.ts`:
- Around line 11-27: Replace the manual AST object construction inside the
helper function makeCompiled with the AST factory functions exported from
`@prisma-next/sql-relational-core/ast`: use the factory for a select node, a
table/from node, a literal expression node, and project item nodes, and assemble
them into the SqlQueryPlan return (preserving params and meta). Update
makeCompiled to import and call those factories (rather than creating plain
objects) so the test builds ASTs consistently with the library's types and
helpers.
In `@packages/3-extensions/sql-orm-client/test/integration/group-by.test.ts`:
- Around line 64-69: Replace the partial assertion
expect(ast.having?.kind).toBe('bin') with a full-structure equality check
against the exact expected HAVING AST node: build an expectedHaving object that
mirrors the complete shape (kind, op, left/right subnodes and any
aggregate/column nodes) and assert expect(ast.having).toEqual(expectedHaving);
keep the existing isSelectAst check and use runtime.executions[0]?.ast to locate
the AST used in the assertion.
In `@packages/3-extensions/sql-orm-client/test/integration/include.test.ts`:
- Around line 1-598: The test file describe('integration/include') is over 500
lines; split it into smaller test files by functionality to meet the guideline.
Create separate test files (e.g., include.basic.test.ts,
include.lateral.test.ts, include.correlated.test.ts) and move related tests into
them: multi-query/basic include tests (those that use createUsersCollection,
createPostsCollection, seedUsers/seedPosts/seedProfiles/seedComments and
assertions about rows and runtime.executions), lateral-specific tests (those
that call createUsersCollectionWithCapabilities with lateral and jsonAgg enabled
and assert AST/join structure), and correlated/jsonAgg tests (those that enable
only jsonAgg and assert subquery projections and SQL correlation). Ensure each
new file imports the same helpers (withCollectionRuntime, createUsersCollection,
createUsersCollectionWithCapabilities, seed* helpers) and preserves test
timeouts and isSelectAst/type checks so each suite remains self-contained.
In `@packages/3-extensions/sql-orm-client/test/query-plan-mutations.test.ts`:
- Around line 35-52: Replace the inline AST object literals in the assertions
with the AST factory helpers: use createParamRef(...) for each param cell and
createColumnRef(...) for each returning column; keep the { kind: 'default' }
cell as-is if no factory exists. Update expectations for plan.ast.rows[0],
plan.ast.rows[1] to call createParamRef with the appropriate index and name
(e.g., matching ids 1..7) instead of manual { kind: 'param', index: ..., name:
... } objects, and replace entries in plan.ast.returning with
createColumnRef('users', 'id'|'name'|'email'|'invited_by_id') calls. Ensure
imports for createParamRef and createColumnRef from
`@prisma-next/sql-relational-core/ast` are added/updated.
In
`@packages/3-extensions/sql-orm-client/test/where-interop.select-includes.test.ts`:
- Around line 29-89: Replace inline AST node literals in the test (the
select/derivedTable/join/subquery/bin nodes and their children) with the
canonical AST factory functions: use createTableRef for table nodes,
createColumnRef for col(...), createParamRef for param(...), createBinaryExpr
for bin expressions, and createLiteralExpr or the appropriate factory for
literals/subquery projections; update the nested
select/derivedTable/join/subquery constructions to call those factories so the
test builds nodes via `@prisma-next/sql-relational-core/ast` helpers instead of
raw object literals (apply same changes to the other cases around lines
129-210).
In `@packages/3-extensions/sql-orm-client/test/where-interop.test.ts`:
- Around line 149-150: Replace the inline join-source table objects (e.g. the {
kind: 'table', name: 'posts' } used as the source in join expectations) with the
createTableRef factory from `@prisma-next/sql-relational-core/ast`; import
createTableRef if not present and use it to construct the same table reference
for both occurrences (the one shown and the similar one at 160-161) so the tests
use the endorsed factory instead of manual AST objects.
aqrln
force-pushed
the
aqrln-mntyvllwtpwx
branch
22 times, most recently
from
4b74f4b to
05ccc3c
Compare
aqrln
force-pushed
the
aqrln-mntyvllwtpwx
branch
from
3ff136e to
a4d5300
Compare
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/reference/modular-refactoring-patterns.md (1)
245-246:⚠️ Potential issue | 🟠 MajorDocument still references deprecated factory-style AST patterns.
This file now mixes class-based guidance with legacy
createXxx/“AST factories” examples, which conflicts with the current refactor direction and can misguide implementation choices. Please update these examples and “Key Learnings” language to class-based construction/re-exports consistently.As per coding guidelines, “Keep docs current (READMEs, rules, links) - see
.cursor/rules/doc-maintenance.mdc.”Also applies to: 325-326
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/modular-refactoring-patterns.md` around lines 245 - 246, The docs still show legacy factory-style AST usage (e.g., createDeleteAst and other createXxx factories) and mismatched guidance (errorModelNotFound examples); update all examples and the "Key Learnings" wording to use the current class-based construction and re-export patterns instead of createXxx factories, replacing any create* symbols with their class-based counterparts and adjusting import/usage examples and narrative to reflect class constructors and named re-exports consistently; also apply the same edits to the other instances flagged (the other createXxx references around the file) so the entire document uniformly describes class-based AST construction and re-exports.
♻️ Duplicate comments (2)
packages/3-extensions/sql-orm-client/test/integration/create.test.ts (1)
135-144:⚠️ Potential issue | 🟠 MajorUse async rejection assertion for
createAll()capability gating.This path is still using a sync
toThrowassertion for an async API and can miss promise rejections.✅ Suggested fix
- expect(() => - users.createAll([ - { - id: 31, - name: 'NoReturn2', - email: 'noreturn2@example.com', - invitedById: null, - }, - ]), - ).toThrow(/requires contract capability "returning"/); + await expect( + users.createAll([ + { + id: 31, + name: 'NoReturn2', + email: 'noreturn2@example.com', + invitedById: null, + }, + ]), + ).rejects.toThrow(/requires contract capability "returning"/);#!/bin/bash # Verify async shape/usage around createAll and current assertion pattern. rg -n --type=ts -C2 '\bcreateAll\s*\(' packages/3-extensions/sql-orm-client/src rg -n --type=ts -C2 'createAll\(' packages/3-extensions/sql-orm-client/test/integration/create.test.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/test/integration/create.test.ts` around lines 135 - 144, The test incorrectly uses a synchronous toThrow for the async users.createAll call; change the assertion to await the promise and assert rejection (e.g., use await expect(users.createAll([...])).rejects.toThrow(/requires contract capability "returning"/)) so the promise rejection is properly handled; update the test around the createAll invocation (in the create.test.ts test that calls users.createAll) to use the async/await + expect(...).rejects pattern and mark the test function as async if needed.packages/2-sql/4-lanes/sql-lane/test/sql-includes.ast.test.ts (1)
36-39:⚠️ Potential issue | 🟡 MinorCorrelate the lateral join to the outer row.
The join condition is
user.id = user.id, so this test still passes if the include loses its parent/child correlation. Tie the lateral source to the outer row through a child key so the AST actually models an include.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-lane/test/sql-includes.ast.test.ts` around lines 36 - 39, The lateral join condition currently compares ColumnRef.of('user','id') to itself (BinaryExpr.eq(ColumnRef.of('user','id'), ColumnRef.of('user','id'))), so the test won't catch lost parent/child correlation; update the right-hand column reference to point to the lateral/child table (the DerivedTableSource.as('posts_lateral', includeAggregate) alias) so the join ties the lateral rows to the outer row — e.g. replace the second ColumnRef.of('user','id') with ColumnRef.of('posts_lateral','user_id') (or the actual child key name) in the BinaryExpr used by JoinAst.left.
🧹 Nitpick comments (20)
docs/reference/modular-refactoring-patterns.md (2)
31-33: Consider renaming the helper description to “normalization” instead of “builder.”
expressionFromSourceconverts a source into an expression viatoExpr(), so “builder helper” is slightly misleading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/modular-refactoring-patterns.md` around lines 31 - 33, Update the helper description wording from “builder helpers” to “normalization helpers” to accurately reflect purpose: change the heading/description that references the utils/guards.ts helper expressionFromSource (and any sibling helpers) so it describes these as normalization helpers that convert AnyExpressionSource to Expression via toExpr(), rather than calling them builder helpers.
23-24: Use “re-exports” instead of “thin wrappers” here.This snippet directly re-exports AST classes; no wrapper is shown. Updating the comment avoids confusion for readers learning the new class-based API.
Suggested doc tweak
-// utils/ast.ts - Thin wrappers around rich AST classes +// utils/ast.ts - Re-exports of rich AST classes export { BinaryExpr, ColumnRef, OperationExpr, ParamRef } from '@prisma-next/sql-relational-core/ast';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/modular-refactoring-patterns.md` around lines 23 - 24, Update the leading comment to accurately describe what's happening: replace "Thin wrappers around rich AST classes" with something like "Re-exports of AST classes" to avoid implying wrapper code; reference the exported symbols BinaryExpr, ColumnRef, OperationExpr, and ParamRef and the source module '@prisma-next/sql-relational-core/ast' so readers understand these are direct re-exports rather than wrapper implementations.packages/3-extensions/sql-orm-client/README.md (1)
34-34: Shorten Mermaid node label and move detail outside diagram.The node label is too verbose for Mermaid; keep labels short and put details in nearby text.
Suggested doc edit
- C --> D[SqlQueryPlan (AST + params + meta)] + C --> D[SqlQueryPlan]As per coding guidelines, “Keep Mermaid diagram labels short and move detail to surrounding Markdown”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/README.md` at line 34, The Mermaid node label "SqlQueryPlan (AST + params + meta)" is too verbose; shorten the node text to "SqlQueryPlan" in the diagram and relocate the details "(AST + params + meta)" into nearby Markdown (e.g., a caption or paragraph under the diagram) so the diagram stays concise while preserving the explanatory detail for the SqlQueryPlan node.packages/2-sql/4-lanes/relational-core/test/ast/order.test.ts (1)
14-23: Make immutability explicit with identity assertion.You validate expression rewrite behavior already; add a reference check to guarantee
rewrite()returns a new instance.Suggested diff
const rewritten = item.rewrite({ columnRef: (expr) => (expr.table === 'post' ? col('article', expr.column) : expr), }); + expect(rewritten).not.toBe(item); expect(item.expr).toEqual(col('post', 'title')); expect(rewritten.expr).toEqual(col('article', 'title')); expect(rewritten.dir).toBe('asc');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/relational-core/test/ast/order.test.ts` around lines 14 - 23, The test should also assert that rewrite returns a new instance to make immutability explicit: after calling OrderByItem.asc(...) and const rewritten = item.rewrite(...), add an identity check (e.g., ensure item !== rewritten) so the test verifies rewrite produced a different object while still checking item.expr stayed unchanged and rewritten.expr changed; reference OrderByItem.asc and the rewrite method in the assertion.packages/2-sql/4-lanes/sql-lane/test/rich-operators-and-projection.test.ts (1)
1-2: Usepatheinstead ofnode:path.As per coding guidelines,
patheshould be used instead ofnode:pathfor path manipulation to ensure consistent cross-platform behavior.Proposed fix
import { readFileSync } from 'node:fs'; -import { dirname, join } from 'node:path'; +import { dirname, join } from 'pathe';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-lane/test/rich-operators-and-projection.test.ts` around lines 1 - 2, Replace imports from 'node:path' with 'pathe': update the import statement that currently brings in dirname and join from 'node:path' to import them from 'pathe' instead (the symbols dirname and join are the ones to change). Ensure any other references in this test file that rely on dirname/join continue to work with the pathe-provided functions.packages/2-sql/4-lanes/kysely-lane/test/build-plan.collect-params.test.ts (1)
235-256: Consolidate redundant error assertions.
buildKyselyPlanis called twice — once intoThrow()(line 244) and again in the try/catch (lines 247-248). Use a single try/catch with all assertions:♻️ Proposed consolidation
it('runs guardrails on compile-free build path', () => { const contract = createTestContract(); const opNode = db .selectFrom('user as u') .innerJoin('user as p', 'u.id', 'p.id') .selectAll('u') .where('id', '=', 'u_1') .toOperationNode(); - expect(() => buildKyselyPlan(contract, opNode)).toThrow(KyselyTransformError); - let caughtError: unknown; try { buildKyselyPlan(contract, opNode); } catch (error) { caughtError = error; } + + expect(caughtError).toBeInstanceOf(KyselyTransformError); expect(KyselyTransformError.is(caughtError)).toBe(true); expect((caughtError as KyselyTransformError).code).toBe( KYSELY_TRANSFORM_ERROR_CODES.UNQUALIFIED_REF_IN_MULTI_TABLE, ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/kysely-lane/test/build-plan.collect-params.test.ts` around lines 235 - 256, The test calls buildKyselyPlan twice; remove the redundant expect(...toThrow()) call and consolidate into a single try/catch: call buildKyselyPlan(contract, opNode) once inside a try block, capture the thrown error in caughtError, then assert KyselyTransformError.is(caughtError) and that (caughtError as KyselyTransformError).code equals KYSELY_TRANSFORM_ERROR_CODES.UNQUALIFIED_REF_IN_MULTI_TABLE; reference the existing symbols buildKyselyPlan, KyselyTransformError, and KYSELY_TRANSFORM_ERROR_CODES.UNQUALIFIED_REF_IN_MULTI_TABLE when making the change.packages/3-extensions/sql-orm-client/test/integration/include.test.ts (1)
1-589: File exceeds 500-line limit; consider splitting by functionality.The file is 589 lines, which exceeds the 500-line guideline. The tests group naturally into distinct concerns:
- Basic include behavior (one-to-many, one-to-one)
- Scalar aggregations (count, sum, avg, min, max)
- Combine branches
- Lateral strategy tests
- Correlated strategy tests
Consider splitting into files like
include.basic.test.ts,include.aggregates.test.ts, andinclude.strategies.test.ts.As per coding guidelines: "Keep test files under 500 lines to maintain readability and navigability. If a test file exceeds this limit, it should be split into multiple files."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/test/integration/include.test.ts` around lines 1 - 589, The test file describe('integration/include') is 589 lines and exceeds the 500-line guideline; split it into multiple focused test files (e.g. include.basic.test.ts, include.aggregates.test.ts, include.strategies.test.ts) by moving related it(...) blocks: basic includes (the test using createUsersCollection and createPostsCollection that asserts one-to-many/one-to-one), scalar aggregation tests (the tests that call posts.count(), posts.sum(), posts.avg()/min()/max() and use NumericPostField), combine branch tests (the test using posts.combine({...})), and strategy tests (all tests creating collections with createUsersCollectionWithCapabilities and asserting lateral/correlated behavior, inspecting runtime.executions, ast, JsonArrayAggExpr/SubqueryExpr, and SQL). Keep shared helpers (withCollectionRuntime, seedUsers, seedPosts, seedProfiles, createUsersCollection, createPostsCollection, getTestContract) imported at top of each new file and preserve timeouts and type aliases (NumericPostField) so each split file remains self-contained.packages/3-extensions/sql-orm-client/test/integration/runtime-helpers.ts (1)
5-5: Import from the public package instead of the relative path.The import should use
@prisma-next/adapter-postgres/adapterinstead of the relative path../../../../3-targets/6-adapters/postgres/src/core/adapter. ThecreatePostgresAdapterfunction is exported from the package's public API, and package imports are clearer and more robust to directory restructuring.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/test/integration/runtime-helpers.ts` at line 5, The import currently pulls createPostgresAdapter via a relative path; replace that relative import with the package's public entry by importing createPostgresAdapter from "@prisma-next/adapter-postgres/adapter" (update the import statement that references createPostgresAdapter accordingly) so the module uses the published public API instead of an internal file path.packages/2-sql/4-lanes/sql-lane/test/rich-select-and-include.test.ts (2)
2-2: Usepatheinstead ofnode:pathin this TypeScript test file.This file currently imports path helpers from
node:path; repository rules requirepathefor TS path operations.♻️ Proposed change
-import { dirname, join } from 'node:path'; +import { dirname, join } from 'pathe';As per coding guidelines, "
**/*.{ts,tsx,mts,cts}: Usepatheinstead ofnode:pathfor path manipulation in TypeScript files."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-lane/test/rich-select-and-include.test.ts` at line 2, Replace the import of path helpers from Node's built-in module to the pathe package: change the import statement that currently reads "import { dirname, join } from 'node:path';" to import the same symbols from "pathe" instead; ensure any usages of dirname and join in rich-select-and-include.test.ts continue to work (no other code changes needed) and that TypeScript resolves the pathe types in the test file.
58-68: Prefer full-structure assertions for AST/plan verification.These checks are currently split across many field-level expectations with optional chaining. For these plan-shape tests, use a single structured
toEqual/toMatchObjectassertion per logical block and assert required nodes directly.As per coding guidelines, "Compare entire plan structures using single
toEqualassertions rather than piece-by-piece checks" and "Prefer object matchers over multiple individualexpect().toBe()calls when checking 2 or more related values in tests."Also applies to: 97-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-lane/test/rich-select-and-include.test.ts` around lines 58 - 68, Replace the fragmented, field-by-field assertions with single structured assertions that compare the relevant AST/plan blocks as whole objects: keep the type check (expect(plan.ast).toBeInstanceOf(SelectAst)) then use one toMatchObject/toEqual for the AST projection, orderBy and where together (e.g., an object containing project: [{ alias: 'user_id' }, { alias: 'user_email' }], orderBy: [{ expr: ColumnRef.of('user','email') }], where: BinaryExpr with right: ParamRef.of(1,'userId')) and another single toEqual/toMatchObject for plan.meta (projection and annotations). Do the same for the other block noted (lines 97-108): combine related expectations into one structured assertion each, referencing SelectAst, ColumnRef.of, ParamRef.of, plan.meta.projection and plan.meta.annotations to locate the fields.packages/2-sql/4-lanes/sql-lane/test/sql.test.ts (1)
83-101: Avoid directas Contractcast at the mock boundary.Prefer the explicit unsafe boundary form used in tests (
as unknown as Contract) instead of direct casting.As per coding guidelines, "Use double casts (`as unknown as X`) for mocks and dynamic proxies in tests instead of direct casts."♻️ Suggested cast adjustment
- } as Contract; + } as unknown as Contract;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-lane/test/sql.test.ts` around lines 83 - 101, The test currently uses a direct cast "as Contract" on the mock object `decorated`; change this to the safer mock boundary pattern by casting via unknown (i.e., `as unknown as Contract`) to follow the project's testing guideline—locate the `decorated` variable in the test (`const decorated = { ... } as Contract;`) and replace the direct cast with the double cast `as unknown as Contract`.packages/3-extensions/sql-orm-client/test/integration/create.test.ts (1)
79-79: Prefer non-null assertions after length invariants in these AST checks.After asserting
runtime.executionshas length1, optional chaining here hides broken invariants.♻️ Suggested cleanup
- expectInsertBatchAst(runtime.executions[0]?.ast); + expectInsertBatchAst(runtime.executions[0]!.ast); ... - expectInsertBatchAst(runtime.executions[0]?.ast); + expectInsertBatchAst(runtime.executions[0]!.ast);As per coding guidelines:
Prefer assertions over defensive checks when data is guaranteed to be valid ... use non-null assertions ... Remove optional chaining ... for properties guaranteed to exist after validation.Also applies to: 106-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/test/integration/create.test.ts` at line 79, The test uses optional chaining on runtime.executions after an earlier length assertion which hides violated invariants; update the calls (e.g., the expectInsertBatchAst(runtime.executions[0]?.ast) at the call site and the similar occurrence at line 106) to use non-null assertions instead of optional chaining so the compiler/test fails loudly when the invariant is broken — e.g., replace runtime.executions[0]?.ast with runtime.executions![0].ast (or runtime.executions![0]!.ast if element can be nullable) at each referenced call such as expectInsertBatchAst and the other AST expectation.packages/2-sql/4-lanes/sql-lane/test/plan.test.ts (1)
98-113: Use real child-side refs in this include fixture.
createColumnBuilder('post', '')pluson: { left: userColumns.id, right: userColumns.id }means this case never proves thatbuildMeta()traverses related-table column refs. The test will still pass if the include path only keeps the table name. Prefer actualpostcolumns in the projection and join predicate so the assertion exercises the child side end to end.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-lane/test/plan.test.ts` around lines 98 - 113, The include fixture uses placeholder child refs (createColumnBuilder('post', '') and on: { left: userColumns.id, right: userColumns.id }) which prevents the test from exercising traversal into related-table column refs; update the fixture to use real post-side column references (e.g., replace createColumnBuilder('post', '') with the actual postColumns (such as postColumns.id, postColumns.<other>) and change the join/include predicates (on) to use postColumns.id on the right side instead of userColumns.id), ensuring the include with alias 'posts', childProjection, and joins all reference postColumns so buildMeta() is validated end-to-end.packages/2-sql/4-lanes/relational-core/src/ast/common.ts (1)
1-14: Avoid pass-through reexports insrc/modules.This file is now just a barrel over
./types. The repo rules reserve that pattern forexports/entrypoints, so consumers should either import from the defining module directly or this surface should move to anexports/file.As per coding guidelines, "Do not reexport things from one file in another, except in the exports/ folders."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/relational-core/src/ast/common.ts` around lines 1 - 14, This file is a pass-through reexport of symbols (AggregateExpr, ColumnRef, DefaultValueExpr, DerivedTableSource, JsonArrayAggExpr, JsonObjectExpr, LiteralExpr, OperationExpr, ParamRef, ProjectionItem, SubqueryExpr, TableSource) from ./types which violates the rule against reexporting in src; fix by removing this barrel and updating all consumers to import the listed symbols directly from ./types, or alternatively move this exact reexport into the package's exports/ entrypoint (creating the same export there and deleting this src wrapper) and update package exports as needed.packages/3-extensions/sql-orm-client/src/collection.ts (1)
107-114: Consider using theWhereExprbase class check instead of a subtype allowlist for better maintainability.The current guard correctly covers all five
WhereExprsubclasses (BinaryExpr,AndExpr,OrExpr,ExistsExpr,NullCheckExpr). However, hardcoding instanceof checks for concrete classes becomes brittle as the AST grows. Using a base class check or shared AST methods would keepCollection.where()automatically aligned with newWhereExprsubclasses without requiring updates to this guard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/src/collection.ts` around lines 107 - 114, The isWhereExprInput guard currently enumerates concrete subclasses (BinaryExpr, ExistsExpr, NullCheckExpr, AndExpr, OrExpr); change it to test against the shared base type instead so new AST node types are handled automatically—replace the subtype allowlist with a single base-class check (value instanceof WhereExpr) or, if WhereExpr is an interface/abstract without runtime presence, use a stable duck-typed check shared by all WhereExpr implementations (e.g., a common symbol or method) so Collection.where() will accept future WhereExpr subclasses without updating this guard.packages/2-sql/4-lanes/relational-core/test/ast/test-helpers.ts (1)
9-9: Avoid/exports/import path in test helpersLine 9 imports from
../../src/exports/ast; prefer importing from concrete source modules to keep dependency direction explicit and avoid export-surface coupling.As per coding guidelines: "Import from shared plane packages using package name and export identifier ... not
/exports/paths."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/relational-core/test/ast/test-helpers.ts` at line 9, Replace the import in test-helpers.ts that currently targets '../../src/exports/ast' with a direct import from the concrete AST source module (e.g., the module that actually declares the AST types/functions rather than the aggregated exports surface) or the package-level export name; update the import to reference the module that defines the symbols you use in this file (the AST types/functions referenced in test-helpers.ts) so you avoid importing via the /exports/ forwarder and keep dependency direction explicit.packages/2-sql/4-lanes/sql-lane/test/sql-includes.builder.basic.test.ts (1)
53-86: Consider extracting the nested AST traversal for readability.Lines 77-78 perform deep nesting to access the rows subquery. While functionally correct, this could benefit from intermediate variables for clarity.
The
as nevercast on Line 83 for theParamRefcomparison is acceptable in tests for shape matching.♻️ Optional: Extract intermediate variables
- const rowsQuery = ((includeJoin?.source as DerivedTableSource).query.from as DerivedTableSource) - .query; + const aggregateSource = includeJoin?.source as DerivedTableSource; + const rowsSource = aggregateSource.query.from as DerivedTableSource; + const rowsQuery = rowsSource.query;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-lane/test/sql-includes.builder.basic.test.ts` around lines 53 - 86, The test digs three levels into the built AST to reach the rows subquery which hurts readability; refactor the nested traversal in the 'builds includeMany with custom aliases and child where clauses' test by extracting intermediate consts (e.g., capture the found join into includeJoin, then its source into derivedSource, then the inner DerivedTableSource into lateralDerived and finally rowsQuery from lateralDerived.query.from.query) so the final expect reads clearly against rowsQuery.where; keep the existing comparisons (including the as never cast) intact.packages/2-sql/4-lanes/sql-lane/src/sql/mutation-builder.ts (1)
374-377: Consider extracting conditional returning application.The pattern for conditionally applying returning is repeated across insert, update, and delete builders:
let ast = ...; if (returning.length > 0) { ast = ast.withReturning(returning); }This could be simplified with a helper or inline ternary, though the current approach is readable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-lane/src/sql/mutation-builder.ts` around lines 374 - 377, The repeated pattern of conditionally applying .withReturning(...) should be extracted into a small helper to avoid duplication; add a function (e.g. applyReturning(ast, returning)) that returns returning.length > 0 ? ast.withReturning(returning) : ast, then replace the inline blocks around UpdateAst.table(...).withSet(...).withWhere(...), the insert builder AST creation, and the delete builder AST creation to call applyReturning(...) instead of repeating the if-check; reference UpdateAst.table, TableSource.named, withSet, withWhere, withReturning and the insert/delete builders when making the replacements.packages/3-extensions/sql-orm-client/src/query-plan-mutations.ts (1)
31-51: Missing table existence validation increateColumnParamDescriptor.If
tableNamedoesn't exist incontract.storage.tables, the lookup on Line 37 silently returnsundefined, and the function produces a ParamDescriptor without codec metadata. This might mask configuration errors. Consider validating the table exists or documenting that callers must ensure valid table names.🛡️ Proposed validation
function createColumnParamDescriptor( contract: SqlContract<SqlStorage>, tableName: string, columnName: string, index: number, ): ParamDescriptor { + const table = contract.storage.tables[tableName]; + if (!table) { + throw new Error(`Unknown table "${tableName}" in createColumnParamDescriptor`); + } - const columnMeta = contract.storage.tables[tableName]?.columns[columnName]; + const columnMeta = table.columns[columnName]; return {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/src/query-plan-mutations.ts` around lines 31 - 51, createColumnParamDescriptor currently assumes tableName exists and will silently return a ParamDescriptor missing codec metadata if it does not; update the function to explicitly validate that contract.storage.tables[tableName] exists (e.g., retrieve const tableMeta = contract.storage.tables[tableName]) and if missing throw or return a clear error mentioning the invalid tableName and the contract context, so callers get immediate feedback instead of a silent, incomplete ParamDescriptor (keep the rest of the columnMeta handling for when the table exists).packages/3-extensions/sql-orm-client/src/query-plan-meta.ts (1)
17-29: Consider usingifDefined()for conditional spread.Line 25 uses an inline conditional spread pattern. As per coding guidelines, prefer
ifDefined()from@prisma-next/utils/definedfor conditional object spreads.Suggested refactor
+import { ifDefined } from '@prisma-next/utils/defined'; + export function buildOrmPlanMeta( contract: SqlContract<SqlStorage>, paramDescriptors: readonly ParamDescriptor[] = [], ): PlanMeta { return { target: contract.target, targetFamily: contract.targetFamily, storageHash: contract.storageHash, - ...(contract.profileHash !== undefined ? { profileHash: contract.profileHash } : {}), + ...ifDefined({ profileHash: contract.profileHash }), lane: 'orm-client', paramDescriptors: [...paramDescriptors], }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/src/query-plan-meta.ts` around lines 17 - 29, Replace the inline conditional spread in buildOrmPlanMeta with the ifDefined helper: import ifDefined from '@prisma-next/utils/defined' and use it to conditionally include profileHash (e.g. ...ifDefined(contract.profileHash !== undefined, { profileHash: contract.profileHash })) so the returned PlanMeta uses the standardized conditional spread pattern while leaving target, targetFamily, storageHash, lane and paramDescriptors unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/Testing` Guide.md:
- Around line 493-495: Update the outdated exception text that references
OperationExpr.function(...) to use the current API/class names introduced by the
AST refactor (e.g., FunctionOperationExpr) and any new factory usage patterns;
find the mention of OperationExpr.function(...) and replace it with the
canonical constructor/factory name (FunctionOperationExpr or the appropriate
factory method from the use-ast-factories guidance) and update the surrounding
wording to match the current AST construction guidance in
.cursor/rules/use-ast-factories.mdc.
In `@packages/1-framework/4-runtime-executor/src/plugins/budgets.ts`:
- Around line 128-137: The current code reads annotations and uses
Math.min(limit, tableEstimate) without validating the limit; update the check
around the computed limit (the variable named limit derived from annotations) so
you only apply Math.min when limit is a finite non-negative number (e.g.,
Number.isFinite(limit) && limit >= 0) to guard against NaN/Infinity/negative
values, or coerce it to a safe value first (e.g., clamp with Math.max(0,
Math.min(limit, tableEstimate))); ensure this change is applied where limit is
compared to tableEstimate so tableEstimate is never reduced by an invalid limit.
In `@packages/2-sql/4-lanes/kysely-lane/src/transform/transform-expr.ts`:
- Line 223: The code constructs AST nodes directly in transform-expr.ts: keep
the direct construction for BinaryExpr when the operator is dynamic (leave the
existing new BinaryExpr(operator, left, right) usage), but replace any direct
construction of ordering nodes with the direction-specific factories — use
OrderByItem.asc(...) or OrderByItem.desc(...) based on the parsed direction
variable (e.g., `direction`/`dir`) instead of new OrderByItem(...); update the
branch that creates ordering items to call OrderByItem.asc() when direction is
ascending and OrderByItem.desc() when descending to match the codebase
convention.
In `@packages/2-sql/4-lanes/relational-core/test/ast/builders.test.ts`:
- Around line 17-37: Add an assertion that verifies the having clause was set:
after building the AST with
SelectAst.from(...).withHaving(BinaryExpr.gt(col('user', 'id'), param(2,
'minId'))), add expect(ast.having).toEqual(BinaryExpr.gt(col('user', 'id'),
param(2, 'minId'))); so the test asserts the having clause matches the
BinaryExpr created by withHaving.
In `@packages/2-sql/4-lanes/sql-lane/src/sql/include-builder.ts`:
- Around line 218-226: The check in buildChildProjectionItems wrongly treats an
empty string alias as missing; change the condition to test for undefined (and
null if needed) explicitly: replace the `if (!column || !alias)` with something
like `if (column === undefined || alias === undefined)` (or `column == null ||
alias == null` to include null), so empty-string aliases are allowed, then call
errorMissingColumnForAlias(alias ?? 'unknown', idx) and proceed to return
ProjectionItem.of(alias, column.toExpr()).
In `@packages/2-sql/4-lanes/sql-lane/test/include-builder.test.ts`:
- Around line 99-104: Replace the plain object cast used as a ParamRef in the
assertion with the ParamRef factory method and import ParamRef from
`@prisma-next/sql-relational-core/ast`; specifically change the second operand in
BinaryExpr.eq (currently { index: 1, name: 'userId' } as never) to use
ParamRef.of(...) (or the library's ParamRef factory) so the assertion on
rowsQuery.where (which uses AndExpr.of and BinaryExpr.eq with
ColumnRef.of('user','id')) uses a proper ParamRef instance.
In `@packages/2-sql/4-lanes/sql-lane/test/rich-mutation.test.ts`:
- Line 2: Replace the Node builtin path import with pathe: change the import
that currently pulls dirname and join from 'node:path' to import them from
'pathe' so all path manipulations use the pathe utilities (update the import
that references dirname and join accordingly and keep usages of dirname(...) and
join(...) unchanged).
In `@packages/2-sql/4-lanes/sql-lane/test/rich-operators-and-projection.test.ts`:
- Around line 51-64: The test references a non-existent column deletedAt which
causes failures; update the fixture contract or the test: either add a nullable
column (e.g., deleted_at with nullable: true) to the contract returned by
loadContract() so schema<Contract>(context).tables.user includes deletedAt (or
mapped name), or change the test to use an existing nullable column such as
invited_by_id throughout the test (adjust ColumnRef.of('user','deletedAt') and
the .where call to use the chosen column), ensuring
sql(...).delete(...).where(...) still asserts DeleteAst and that where equals
NullCheckExpr.isNotNull(ColumnRef.of(...)).
In `@packages/2-sql/4-lanes/sql-lane/test/select-builder.test.ts`:
- Around line 93-97: The test currently spreads the class instance
joinTables.user.columns.id which strips its prototype methods; instead create a
mock that preserves the original prototype and then assign the test-only fields.
For example, create a new object with the same prototype via
Object.create(Object.getPrototypeOf(joinTables.user.columns.id)) or
Object.create(joinTables.user.columns.id) and then use Object.assign(...) to add
table: 'post', column: 'userId', toExpr: () => ColumnRef.of('post','userId');
ensure you reference joinTables.user.columns.id and ColumnRef.of when
constructing the mock so prototype methods remain available.
In `@packages/2-sql/4-lanes/sql-lane/test/sql.test.ts`:
- Around line 16-35: The test omits asserting the ordering even though it calls
orderBy(createdAt.desc()), so add an assertion that the built SelectAst's order
(from plan.ast as SelectAst) matches a descending order on
ColumnRef.of('user','createdAt'); locate the test's plan building block that
uses orderBy and add an expect against the SelectAst's order/orderBy property to
verify it contains the createdAt column with descending direction so sort
regressions are caught.
In `@packages/2-sql/4-lanes/sql-lane/test/test-helpers.ts`:
- Line 2: The import of path utilities currently uses "node:path" with named
imports dirname and join; change the import source to "pathe" so it reads import
{ dirname, join } from 'pathe' and keep usages of dirname(...) and join(...) in
test helpers (e.g., in test-helpers.ts) unchanged; this ensures TypeScript path
utility guidelines are followed.
In `@packages/3-extensions/sql-orm-client/README.md`:
- Around line 22-24: The README currently references ExecutionPlan but the repo
uses SqlQueryPlan terminology; update the dependency list to use SqlQueryPlan
consistently (replace "ExecutionPlan" with "SqlQueryPlan"), ensure the package
reference that exports SqlQueryPlan (the one listed among `@prisma-next/`* deps)
is correct, and run a quick grep for other README occurrences to remove any
remaining "ExecutionPlan" mentions so the doc consistently uses SqlQueryPlan.
In `@packages/3-extensions/sql-orm-client/src/grouped-collection.ts`:
- Around line 140-149: The aggregate methods sum/avg/min/max are needlessly
casting the typed parameter to string; update the implementations in the
grouped-collection methods (sum, avg, min, max) to remove the blind "as string"
casts and pass the field argument directly into createMetricExpr, e.g.
createMetricExpr('sum', field), so createHavingComparisonMethods receives the
properly-typed metric expression and type precision is preserved.
In `@packages/3-extensions/sql-orm-client/src/where-interop.ts`:
- Around line 24-31: The current normalization rebuilds paramDescriptors by
array position and drops each descriptor.index, which can misbind params; in the
function that returns { expr, params, paramDescriptors } (using the local bound
variable), preserve descriptor-to-param mapping by rebuilding paramDescriptors
ordered by each descriptor.index (or validate that they are already 1..n) before
returning—i.e., create a new array of length params.length and place each
descriptor at descriptor.index-1 (or throw on missing/duplicate/index out of
range) so codecs/metadata stay bound to the correct parameter.
In `@packages/3-extensions/sql-orm-client/test/grouped-collection.test.ts`:
- Around line 216-232: Replace the weak single-containment check with an
assertion that all four HAVING metrics fell back to the raw field by verifying
metricColumns (the result of mapping runtime.executions -> having.left.expr)
contains exactly four entries and each entry equals 'posts:userId'; update the
assertion that currently uses metricColumns and expect(...).toContain to instead
assert the entire metricColumns array length is 4 and every element ===
'posts:userId' so failures in any of the AggregateExpr cases (sum/avg/min/max)
are detected.
---
Outside diff comments:
In `@docs/reference/modular-refactoring-patterns.md`:
- Around line 245-246: The docs still show legacy factory-style AST usage (e.g.,
createDeleteAst and other createXxx factories) and mismatched guidance
(errorModelNotFound examples); update all examples and the "Key Learnings"
wording to use the current class-based construction and re-export patterns
instead of createXxx factories, replacing any create* symbols with their
class-based counterparts and adjusting import/usage examples and narrative to
reflect class constructors and named re-exports consistently; also apply the
same edits to the other instances flagged (the other createXxx references around
the file) so the entire document uniformly describes class-based AST
construction and re-exports.
---
Duplicate comments:
In `@packages/2-sql/4-lanes/sql-lane/test/sql-includes.ast.test.ts`:
- Around line 36-39: The lateral join condition currently compares
ColumnRef.of('user','id') to itself (BinaryExpr.eq(ColumnRef.of('user','id'),
ColumnRef.of('user','id'))), so the test won't catch lost parent/child
correlation; update the right-hand column reference to point to the
lateral/child table (the DerivedTableSource.as('posts_lateral',
includeAggregate) alias) so the join ties the lateral rows to the outer row —
e.g. replace the second ColumnRef.of('user','id') with
ColumnRef.of('posts_lateral','user_id') (or the actual child key name) in the
BinaryExpr used by JoinAst.left.
In `@packages/3-extensions/sql-orm-client/test/integration/create.test.ts`:
- Around line 135-144: The test incorrectly uses a synchronous toThrow for the
async users.createAll call; change the assertion to await the promise and assert
rejection (e.g., use await
expect(users.createAll([...])).rejects.toThrow(/requires contract capability
"returning"/)) so the promise rejection is properly handled; update the test
around the createAll invocation (in the create.test.ts test that calls
users.createAll) to use the async/await + expect(...).rejects pattern and mark
the test function as async if needed.
---
Nitpick comments:
In `@docs/reference/modular-refactoring-patterns.md`:
- Around line 31-33: Update the helper description wording from “builder
helpers” to “normalization helpers” to accurately reflect purpose: change the
heading/description that references the utils/guards.ts helper
expressionFromSource (and any sibling helpers) so it describes these as
normalization helpers that convert AnyExpressionSource to Expression via
toExpr(), rather than calling them builder helpers.
- Around line 23-24: Update the leading comment to accurately describe what's
happening: replace "Thin wrappers around rich AST classes" with something like
"Re-exports of AST classes" to avoid implying wrapper code; reference the
exported symbols BinaryExpr, ColumnRef, OperationExpr, and ParamRef and the
source module '@prisma-next/sql-relational-core/ast' so readers understand these
are direct re-exports rather than wrapper implementations.
In `@packages/2-sql/4-lanes/kysely-lane/test/build-plan.collect-params.test.ts`:
- Around line 235-256: The test calls buildKyselyPlan twice; remove the
redundant expect(...toThrow()) call and consolidate into a single try/catch:
call buildKyselyPlan(contract, opNode) once inside a try block, capture the
thrown error in caughtError, then assert KyselyTransformError.is(caughtError)
and that (caughtError as KyselyTransformError).code equals
KYSELY_TRANSFORM_ERROR_CODES.UNQUALIFIED_REF_IN_MULTI_TABLE; reference the
existing symbols buildKyselyPlan, KyselyTransformError, and
KYSELY_TRANSFORM_ERROR_CODES.UNQUALIFIED_REF_IN_MULTI_TABLE when making the
change.
In `@packages/2-sql/4-lanes/relational-core/src/ast/common.ts`:
- Around line 1-14: This file is a pass-through reexport of symbols
(AggregateExpr, ColumnRef, DefaultValueExpr, DerivedTableSource,
JsonArrayAggExpr, JsonObjectExpr, LiteralExpr, OperationExpr, ParamRef,
ProjectionItem, SubqueryExpr, TableSource) from ./types which violates the rule
against reexporting in src; fix by removing this barrel and updating all
consumers to import the listed symbols directly from ./types, or alternatively
move this exact reexport into the package's exports/ entrypoint (creating the
same export there and deleting this src wrapper) and update package exports as
needed.
In `@packages/2-sql/4-lanes/relational-core/test/ast/order.test.ts`:
- Around line 14-23: The test should also assert that rewrite returns a new
instance to make immutability explicit: after calling OrderByItem.asc(...) and
const rewritten = item.rewrite(...), add an identity check (e.g., ensure item
!== rewritten) so the test verifies rewrite produced a different object while
still checking item.expr stayed unchanged and rewritten.expr changed; reference
OrderByItem.asc and the rewrite method in the assertion.
In `@packages/2-sql/4-lanes/relational-core/test/ast/test-helpers.ts`:
- Line 9: Replace the import in test-helpers.ts that currently targets
'../../src/exports/ast' with a direct import from the concrete AST source module
(e.g., the module that actually declares the AST types/functions rather than the
aggregated exports surface) or the package-level export name; update the import
to reference the module that defines the symbols you use in this file (the AST
types/functions referenced in test-helpers.ts) so you avoid importing via the
/exports/ forwarder and keep dependency direction explicit.
In `@packages/2-sql/4-lanes/sql-lane/src/sql/mutation-builder.ts`:
- Around line 374-377: The repeated pattern of conditionally applying
.withReturning(...) should be extracted into a small helper to avoid
duplication; add a function (e.g. applyReturning(ast, returning)) that returns
returning.length > 0 ? ast.withReturning(returning) : ast, then replace the
inline blocks around UpdateAst.table(...).withSet(...).withWhere(...), the
insert builder AST creation, and the delete builder AST creation to call
applyReturning(...) instead of repeating the if-check; reference
UpdateAst.table, TableSource.named, withSet, withWhere, withReturning and the
insert/delete builders when making the replacements.
In `@packages/2-sql/4-lanes/sql-lane/test/plan.test.ts`:
- Around line 98-113: The include fixture uses placeholder child refs
(createColumnBuilder('post', '') and on: { left: userColumns.id, right:
userColumns.id }) which prevents the test from exercising traversal into
related-table column refs; update the fixture to use real post-side column
references (e.g., replace createColumnBuilder('post', '') with the actual
postColumns (such as postColumns.id, postColumns.<other>) and change the
join/include predicates (on) to use postColumns.id on the right side instead of
userColumns.id), ensuring the include with alias 'posts', childProjection, and
joins all reference postColumns so buildMeta() is validated end-to-end.
In `@packages/2-sql/4-lanes/sql-lane/test/rich-operators-and-projection.test.ts`:
- Around line 1-2: Replace imports from 'node:path' with 'pathe': update the
import statement that currently brings in dirname and join from 'node:path' to
import them from 'pathe' instead (the symbols dirname and join are the ones to
change). Ensure any other references in this test file that rely on dirname/join
continue to work with the pathe-provided functions.
In `@packages/2-sql/4-lanes/sql-lane/test/rich-select-and-include.test.ts`:
- Line 2: Replace the import of path helpers from Node's built-in module to the
pathe package: change the import statement that currently reads "import {
dirname, join } from 'node:path';" to import the same symbols from "pathe"
instead; ensure any usages of dirname and join in
rich-select-and-include.test.ts continue to work (no other code changes needed)
and that TypeScript resolves the pathe types in the test file.
- Around line 58-68: Replace the fragmented, field-by-field assertions with
single structured assertions that compare the relevant AST/plan blocks as whole
objects: keep the type check (expect(plan.ast).toBeInstanceOf(SelectAst)) then
use one toMatchObject/toEqual for the AST projection, orderBy and where together
(e.g., an object containing project: [{ alias: 'user_id' }, { alias:
'user_email' }], orderBy: [{ expr: ColumnRef.of('user','email') }], where:
BinaryExpr with right: ParamRef.of(1,'userId')) and another single
toEqual/toMatchObject for plan.meta (projection and annotations). Do the same
for the other block noted (lines 97-108): combine related expectations into one
structured assertion each, referencing SelectAst, ColumnRef.of, ParamRef.of,
plan.meta.projection and plan.meta.annotations to locate the fields.
In `@packages/2-sql/4-lanes/sql-lane/test/sql-includes.builder.basic.test.ts`:
- Around line 53-86: The test digs three levels into the built AST to reach the
rows subquery which hurts readability; refactor the nested traversal in the
'builds includeMany with custom aliases and child where clauses' test by
extracting intermediate consts (e.g., capture the found join into includeJoin,
then its source into derivedSource, then the inner DerivedTableSource into
lateralDerived and finally rowsQuery from lateralDerived.query.from.query) so
the final expect reads clearly against rowsQuery.where; keep the existing
comparisons (including the as never cast) intact.
In `@packages/2-sql/4-lanes/sql-lane/test/sql.test.ts`:
- Around line 83-101: The test currently uses a direct cast "as Contract" on the
mock object `decorated`; change this to the safer mock boundary pattern by
casting via unknown (i.e., `as unknown as Contract`) to follow the project's
testing guideline—locate the `decorated` variable in the test (`const decorated
= { ... } as Contract;`) and replace the direct cast with the double cast `as
unknown as Contract`.
In `@packages/3-extensions/sql-orm-client/README.md`:
- Line 34: The Mermaid node label "SqlQueryPlan (AST + params + meta)" is too
verbose; shorten the node text to "SqlQueryPlan" in the diagram and relocate the
details "(AST + params + meta)" into nearby Markdown (e.g., a caption or
paragraph under the diagram) so the diagram stays concise while preserving the
explanatory detail for the SqlQueryPlan node.
In `@packages/3-extensions/sql-orm-client/src/collection.ts`:
- Around line 107-114: The isWhereExprInput guard currently enumerates concrete
subclasses (BinaryExpr, ExistsExpr, NullCheckExpr, AndExpr, OrExpr); change it
to test against the shared base type instead so new AST node types are handled
automatically—replace the subtype allowlist with a single base-class check
(value instanceof WhereExpr) or, if WhereExpr is an interface/abstract without
runtime presence, use a stable duck-typed check shared by all WhereExpr
implementations (e.g., a common symbol or method) so Collection.where() will
accept future WhereExpr subclasses without updating this guard.
In `@packages/3-extensions/sql-orm-client/src/query-plan-meta.ts`:
- Around line 17-29: Replace the inline conditional spread in buildOrmPlanMeta
with the ifDefined helper: import ifDefined from '@prisma-next/utils/defined'
and use it to conditionally include profileHash (e.g.
...ifDefined(contract.profileHash !== undefined, { profileHash:
contract.profileHash })) so the returned PlanMeta uses the standardized
conditional spread pattern while leaving target, targetFamily, storageHash, lane
and paramDescriptors unchanged.
In `@packages/3-extensions/sql-orm-client/src/query-plan-mutations.ts`:
- Around line 31-51: createColumnParamDescriptor currently assumes tableName
exists and will silently return a ParamDescriptor missing codec metadata if it
does not; update the function to explicitly validate that
contract.storage.tables[tableName] exists (e.g., retrieve const tableMeta =
contract.storage.tables[tableName]) and if missing throw or return a clear error
mentioning the invalid tableName and the contract context, so callers get
immediate feedback instead of a silent, incomplete ParamDescriptor (keep the
rest of the columnMeta handling for when the table exists).
In `@packages/3-extensions/sql-orm-client/test/integration/create.test.ts`:
- Line 79: The test uses optional chaining on runtime.executions after an
earlier length assertion which hides violated invariants; update the calls
(e.g., the expectInsertBatchAst(runtime.executions[0]?.ast) at the call site and
the similar occurrence at line 106) to use non-null assertions instead of
optional chaining so the compiler/test fails loudly when the invariant is broken
— e.g., replace runtime.executions[0]?.ast with runtime.executions![0].ast (or
runtime.executions![0]!.ast if element can be nullable) at each referenced call
such as expectInsertBatchAst and the other AST expectation.
In `@packages/3-extensions/sql-orm-client/test/integration/include.test.ts`:
- Around line 1-589: The test file describe('integration/include') is 589 lines
and exceeds the 500-line guideline; split it into multiple focused test files
(e.g. include.basic.test.ts, include.aggregates.test.ts,
include.strategies.test.ts) by moving related it(...) blocks: basic includes
(the test using createUsersCollection and createPostsCollection that asserts
one-to-many/one-to-one), scalar aggregation tests (the tests that call
posts.count(), posts.sum(), posts.avg()/min()/max() and use NumericPostField),
combine branch tests (the test using posts.combine({...})), and strategy tests
(all tests creating collections with createUsersCollectionWithCapabilities and
asserting lateral/correlated behavior, inspecting runtime.executions, ast,
JsonArrayAggExpr/SubqueryExpr, and SQL). Keep shared helpers
(withCollectionRuntime, seedUsers, seedPosts, seedProfiles,
createUsersCollection, createPostsCollection, getTestContract) imported at top
of each new file and preserve timeouts and type aliases (NumericPostField) so
each split file remains self-contained.
In `@packages/3-extensions/sql-orm-client/test/integration/runtime-helpers.ts`:
- Line 5: The import currently pulls createPostgresAdapter via a relative path;
replace that relative import with the package's public entry by importing
createPostgresAdapter from "@prisma-next/adapter-postgres/adapter" (update the
import statement that references createPostgresAdapter accordingly) so the
module uses the published public API instead of an internal file path.
a4d5300 to
105a7cd
Compare
…assertions Replace verbose property-by-property expect() chains with toMatchObject to make test assertions resemble the AST structure being tested and improve diff readability on failure.
…d add DoUpdateSet test Add early throw when normalizeInsertRows receives an empty rows array, preventing silent generation of a single-row INSERT with no columns. Also add test coverage for compileUpsertReturning with non-empty update data to verify DoUpdateSetConflictAction is produced with correct column assignments and param references.
Replace the adapter's local quoteIdentifier (which only escapes double quotes) with the shared sql-utils implementation that also validates empty identifiers, null bytes, and warns on >63-char names.
Verify that a SelectAst with a SubqueryExpr in its projection lowers to the expected (SELECT ...) AS "alias" SQL, closing the gap in adapter-level test coverage for subquery expressions.
Verify that users -> posts -> comments nested includes produce correct deeply-nested result shapes, validating param offset logic for multi-level lateral/correlated subqueries.
…r base class Replace the closed list of instanceof checks (BinaryExpr, ExistsExpr, NullCheckExpr, AndExpr, OrExpr) with a single WhereExpr base class check, so future WhereExpr subclasses are automatically recognized without updating this function.
…e assertions Remove redundant deleteAst/updateAst/selectAst aliases since TypeScript already narrows ast inside instanceof blocks. Replace unsafe 'as' casts for selectAllIntent annotation with index-access + narrowing to avoid brittle type assertions that would break if PlanMeta.annotations changes.
…isk, param indexing, rewrite intent, and HAVING restriction Add targeted comments addressing code review findings: - AstNode: note that instanceof is the sole dispatch mechanism and a structural brand should be considered if duplicate packages arise - ParamRef.index: document 1-based convention and params[index - 1] - DerivedTableSource.rewrite: explain why it bypasses tableSource callback - validateGroupedComparable: explain why HAVING rejects ParamRef
- Add missing ParamDescriptor import in query-plan-mutations.ts - Widen bindFromSource parameter from concrete union to FromSource base to match SelectAst.from's type - Fix test helpers to use non-null assertions for known contract table/column lookups instead of optional chaining that widens to undefined
wmadden
force-pushed
the
aqrln-mntyvllwtpwx
branch
from
063ae15 to
6efddfa
Compare
…erts helpers The include integration tests used if-instanceof guards for TypeScript narrowing, which silently skipped assertions when the type didn't match. Replace with asserts-style helpers (expectSelectAst, expectSubqueryExpr, etc.) that both narrow the type and fail the test on mismatch, removing all conditional nesting. Also add doc comment to normalizeInsertRows explaining its sparse-row-to-uniform-shape normalization strategy.
… from lints plugin The annotation path existed for the Kysely lane, which is being removed. The AST's selectAllIntent property is the single source of truth, so remove the redundant annotations lookup, the unused PlanMeta import, and the now-unnecessary meta parameter from evaluateAstLints.
…eet coverage thresholds adapter-postgres branch coverage dropped below 87% due to new rendering functions (DISTINCT, GROUP BY, HAVING, table aliases, OR expressions). sql-orm-client branch coverage dropped below 95% because where-binding.ts had no dedicated tests. Add tests for both packages to restore coverage above thresholds.
The field held ReadonlyArray<ProjectionItem> but was named as a verb, making it ambiguous with method names. Rename to `projection` to match the element type and read as a noun. Renames withProject() -> withProjection(), addProject() -> addProjection() and updates all call sites across relational-core, sql-lane, kysely-lane, sql-orm-client, adapter-postgres, sql-runtime, and the demo example.
These files (select.ts, delete.ts, insert.ts, update.ts, predicate.ts, order.ts, common.ts) only re-exported from types.ts and violated the rule against re-exporting outside exports/ folders. The barrel exports/ast.ts already re-exports types.ts directly, so they were pure indirection.
…ntics freezeArray -> frozenArrayCopy, freezeOptionalObject -> frozenOptionalRecordCopy, freezeRecord -> frozenRecordCopy. Addresses SevInf review: names now communicate that these functions shallow-copy before freezing, and use consistent 'record' terminology instead of mixing 'object' and 'record'.
RowType was changed from `declare const` to `Symbol()` but it is only used in type positions. Reverting to `declare const ... unique symbol` as SevInf pointed out — it was never meant to exist at runtime.
Replace casted object literal with ParamRef.of() in include builder test; add join.source assertion to catch regressions in JoinAst source field.
The test exercised .orderBy() in the builder but never asserted the resulting ast.orderBy field, so sort regressions would go undetected.
Missed type annotation in transform-select.ts and assertion key in the integration test from the SelectAst.project rename.
The first test in the suite takes ~223ms due to cold Commander instantiation, exceeding the 100ms default timeout in CI (200ms with 2x multiplier). Give it a larger individual timeout.
wmadden
approved these changes
Mar 23, 2026
This was referenced Mar 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes TML-2040
Intent
The ORM client (
sql-orm-client) previously used Kysely as an internal SQL compiler — it built Kysely query builders, compiled them to SQL strings, and shipped those strings to the runtime for execution. This meant the ORM bypassed the PN AST entirely: queries wentCollectionState → Kysely → SQL string, skipping the structured representation that every other lane (sql-lane, kysely-lane) produces.This PR removes the Kysely dependency from
sql-orm-clientand rewrites every query-building path to emit PN AST nodes directly. The ORM now producesSqlQueryPlan(AST + params + meta) — the same plan shape that sql-lane emits — and the adapter lowers AST to SQL at execution time. This unifies the execution model and lets the runtime, plugins, and adapters work with a single structured representation regardless of which surface produced the query.On top of that, the AST itself is overhauled from plain interfaces + factory functions into an immutable class hierarchy with built-in
rewrite()/fold()/collectRefs()operations, eliminating the external visitor machinery and mutable-builder layer.The story
1. Rewrite the AST from interfaces to immutable classes
The existing PN AST was a collection of plain TypeScript interfaces (
SelectAst,InsertAst,WhereExpr, etc.) constructed viacreateXxxAst()factory functions. Tree transformations and traversals lived in a separatevisitors.tsmodule (mapExpression,foldExpression) that had to be manually kept in sync with every new node type — a growing maintenance burden.This commit replaces all of that with a class hierarchy rooted in three abstract bases:
Expression— scalar value nodes (column refs, params, literals, binary ops, function calls, JSON expressions, aggregates, subqueries)WhereExpr— boolean predicate nodes (and, or, not, binary comparisons, is-null, exists, true/false)QueryAst— statement-level nodes (select, insert, update, delete)Every concrete node is a frozen (
Object.freeze) class instance with:rewrite(rewriter: ExpressionRewriter): Self— structural recursion that rebuilds the tree with targeted leaf-node replacements (param→literal substitution, column remapping, etc.)fold<T>(folder: ExpressionFolder<T>): T— catamorphism for collecting param refs, column refs, or any other derived value in a single passcollectColumnRefs()/collectParamRefs()/collectRefs()— convenience methods built onfoldKey deletions:
visitors.ts— the externalmapExpression/foldExpression/mapComparablemachinery (~420 lines)builders.ts— the mutableSelectAstBuilder/InsertAstBuilder/UpdateAstBuilder/DeleteAstBuilderfluent builders (~300 lines)createXxxAst()factory function bodies inselect.ts,insert.ts,update.ts,delete.ts— replaced by class constructorsThe
ExpressionRewriter/ExpressionFolder<T>interfaces define the callback shapes.AstRewriterextendsExpressionRewriterwithtableSource?andeqColJoinOn?callbacks for full-AST rewrites. Everyswitch/dispatch onkindtags throughout the codebase is replaced byinstanceofchecks or method dispatch on the class itself.Supporting type changes:
BinaryOpunion type replaces string-typedopfieldsAggregateFn='count' | 'sum' | 'avg' | 'min' | 'max'replaces ad-hoc stringsJsonObjectEntry,ProjectionExpr,SqlComparable,InsertValue,JoinOnExpr,WhereArgtype aliases consolidate recurring patternsFromSourceis now an abstract class (base ofTableSourceandDerivedTableSource), not a union typePlanRefscollection (sorted tables + columns) is built viacollectRefs()on every query AST node2. Enrich the AST vocabulary for ORM-level queries
The existing AST was designed for the sql-lane's typed DSL — basic selects, joins, inserts, updates, and deletes. The ORM needs richer constructs. New node classes added:
DerivedTableSource— subqueries in FROM positionSubqueryExpr— scalar subqueries in projectionsJsonObjectExpr/JsonArrayAggExpr— JSON aggregation (for includes)AggregateExpr—count(*),sum(col), etc.FunctionOperationExpr— generic SQL function calls (coalesce, etc.)DefaultValueExpr—DEFAULTplaceholder for multi-row insertsInsertOnConflictAst—ON CONFLICT DO NOTHING / DO UPDATE SETJoinAstgainslateral: booleanandsource: FromSource(widened fromTableRef)SelectAstgainsdistinct,distinctOn,groupBy,having,offsetInsertAstrestructured from single-rowvaluestorows: ReadonlyArray<Record<string, InsertValue>>withnormalizeInsertRowsfor heterogeneous column setsIncludeAst/IncludeRefremoved — these were special-case AST nodes that only the adapter knew how to handle. Includes are now expressed using standard SQL constructs (lateral joins + JSON aggregation), making the AST target-agnostic.3. Replace Kysely compilation in sql-orm-client with direct AST construction
Six
kysely-compiler-*.tsfiles are deleted and replaced by threequery-plan-*.tsfiles:query-plan-select.ts— buildsSelectAstfor selects, relation selects, and single-query includes (both lateral and correlated strategies)query-plan-mutations.ts— buildsInsertAst(with multi-row batching andDefaultValueExprfor heterogeneous column sets),UpdateAst,DeleteAstwith parameterized valuesquery-plan-aggregate.ts— builds aggregate and grouped-aggregate queries, rewriting__orm_havingpseudo-table references into realAggregateExprnodesAll compilation functions now accept the contract (to resolve table columns for
RETURNING */SELECT *) and returnSqlQueryPlaninstead of Kysely'sCompiledQuery.executeCompiledQuery(which wrapped Kysely'sCompiledQueryinto anExecutionPlanwith asqlstring) is replaced byexecuteQueryPlan, which simply forwards theSqlQueryPlanto the runtime — the adapter handles lowering.4. Rewrite sql-lane's include builder with the enriched AST
The sql-lane's
buildIncludeAstis replaced withbuildIncludeJoinArtifact, which returns a lateralJoinAst(with a derived-table source containing the child rows query wrapped injson_agg(json_build_object(...))) plus aProjectionItemthat references the aggregated column. The select builder merges these artifacts into the main query's joins and projection.5. Update the postgres adapter to lower the new AST nodes
The adapter's
lower()function gains handling for: derived-table sources (rendering subqueries in FROM with aliases), lateral joins,json_agg/json_build_object/coalescefor JSON aggregation, aggregate function expressions, subquery expressions in projections,ON CONFLICT DO NOTHING/UPDATE SET,HAVING,DISTINCT ON,GROUP BY,OFFSET, multi-rowINSERT INTO ... VALUES (...), (...)with per-cellDEFAULT, and improved literal rendering for dates and bigints.6. Update all traversal sites to use the new class methods
Every function that walked the AST via external visitor functions or
kind-tag switches — param index collection/remapping inwhere-expr.ts, param-to-literal replacement inwhere-interop.ts, column ref collection inguards.ts— is updated to usenode.rewrite(...)/node.fold(...)/node.collectColumnRefs()method calls directly. ThecollectColumnRefsutility inguards.tsis simplified to delegate toast.collectRefs().Change map
relational-core/ast/types.ts— Complete rewrite: ~200-line interface file → ~1500-line class hierarchy. All AST nodes are now frozen classes withrewrite(),fold(),collectRefs()methods. Abstract bases:AstNode,QueryAst,FromSource,Expression,WhereExpr. Rewriter/folder interfaces:ExpressionRewriter,AstRewriter,ExpressionFolder<T>.relational-core/ast/visitors.ts— Deleted.mapExpression/foldExpressionreplaced bynode.rewrite()/node.fold().relational-core/ast/builders.ts— Deleted.SelectAstBuilderetc. replaced by class constructors (new SelectAst(...),new InsertAst(...)).relational-core/ast/{select,insert,update,delete,join,order,common,predicate}.ts— Reduced to re-exports fromtypes.ts; factory function bodies moved into class constructors.relational-core/utils/guards.ts—collectColumnRefssimplified toast.collectRefs(); standalone guard functions updated.sql-lane— Include builder emits lateral join artifacts (buildIncludeJoinArtifact) instead ofIncludeAstnodes; select/mutation/predicate builders use new class constructors; join builder usesTableSource.named().kysely-lane— Alltransform-*.tsfiles updated from kind-tag dispatch toinstanceofchecks and class method calls;where-expr.tsrewritten to userewrite()/fold().sql-orm-client(core change) — Deleted:kysely-compiler-*.ts(6 files),raw-compiled-query.ts. Added:query-plan-select.ts,query-plan-mutations.ts,query-plan-aggregate.ts,query-plan-meta.ts,execute-query-plan.ts,where-utils.ts. Barrel re-export renamedkysely-compiler.ts→query-plan.ts.sql-orm-client/where-interop.ts— Rewritten to usenode.rewrite(...)instead ofmapExpression(...).postgres adapter—lower()updated to handle all new AST class types viainstanceofdispatch: derived-table sources, lateral joins, JSON aggregation, aggregates, subqueries,ON CONFLICT,HAVING,DISTINCT ON,GROUP BY,OFFSET, multi-rowINSERT,DefaultValueExpr.sql-runtime/lints—LINT.NO_LIMITupdated forDerivedTableSourceclass.runtime-executor/budgets— Updated for class-based AST.instanceofchecks. Newrich-*.test.tsfiles added for class-based construction patterns. Oldsql-compilation/test directory in sql-orm-client deleted and replaced byquery-plan-*.test.ts+rich-*.test.ts.Behavior changes & evidence
Object.freeze(this);instanceofchecks replace kind-tag dispatch everywherevisitors.tsdeleted; all call sites usenode.rewrite()/node.fold()builders.tsdeleted;new SelectAst({...})replacesSelectAstBuilderSqlQueryPlan(AST) instead of pre-rendered SQLexecuteCompiledQuerycall sites replaced withexecuteQueryPlan; integration tests verify AST structurekyselyremoved fromsql-orm-clientdependenciespackage.jsondiff removes"kysely": "catalog:"sql-includes.ast.test.ts, integrationinclude.test.tsverify lateral join structureDEFAULTfor missing columnsquery-plan-mutations.test.tsverifiesDefaultValueExprcells; integrationcreate.test.tsconfirms batched VALUESInsertAstsupportsonConflict(upsert)insert.test.ts, integrationupsert.test.tsverifydoNothing/doUpdateSetSelectAstsupportsdistinct,distinctOn,groupBy,having,offsetselect.test.ts, integrationgroup-by.test.tsinclude.test.tslateral/correlated self-relation testsNotable API changes (all in-repo consumers updated)
new XxxAst({...})ornew XxxExpr(...)instead ofcreateXxxAst({...})factory functionsExpression,WhereExpr,FromSource,QueryAstare abstract base classes — useinstanceofinstead ofkindtag checksExpressionRewriter/ExpressionFolder<T>replacemapExpression()/foldExpression()external visitorsJoinAst.table→JoinAst.source(type widened fromTableReftoFromSource), plus newlateral: booleanfieldInsertAst.values→InsertAst.rows(single-value record replaced by array of rows withInsertValuecells)IncludeAst/IncludeReftypes removed — includes are now standard lateral joins + JSON aggregationastinstead ofsql; the adapter lowers to SQL at execution timeFollow-ups / open questions
@ts-ignore— a cleaner test utility would be preferable.Non-goals / intentionally out of scope
Summary by CodeRabbit
New Features
Improvements
Tests
Documentation