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:
📝 WalkthroughWalkthroughAdds pgvector-backed embedding support and expression-based ordering/filters to the SQL ORM client, integrates pgvector into tests and demo CLI, and updates AST/ParamRef/order-by code paths plus tests and demo commands for embedding search. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (main.ts)
participant ORM as ORM helper
participant Runtime as SQL Runtime
participant DB as Prisma / Query Engine
participant PG as PostgreSQL (pgvector)
CLI->>ORM: invoke find/search with args + runtime
ORM->>Runtime: createOrmClient(runtime) / build query (ParamRef embedding)
Runtime->>DB: execute bound query (params include vectors)
DB->>PG: evaluate vector ops (cosineDistance/cosineSimilarity)
PG-->>DB: filtered/sorted rows
DB-->>Runtime: rows (+ included relations)
Runtime-->>ORM: result rows
ORM-->>CLI: pretty-printed JSON
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
39381cd to
e2d7990
Compare
@prisma-next/mongo-runtime
@prisma-next/family-mongo
@prisma-next/sql-runtime
@prisma-next/family-sql
@prisma-next/extension-paradedb
@prisma-next/extension-pgvector
@prisma-next/postgres
@prisma-next/sql-orm-client
@prisma-next/target-mongo
@prisma-next/adapter-mongo
@prisma-next/driver-mongo
@prisma-next/contract
@prisma-next/utils
@prisma-next/config
@prisma-next/errors
@prisma-next/framework-components
@prisma-next/operations
@prisma-next/contract-authoring
@prisma-next/ids
@prisma-next/psl-parser
@prisma-next/psl-printer
@prisma-next/cli
@prisma-next/emitter
@prisma-next/migration-tools
@prisma-next/vite-plugin-contract-emit
@prisma-next/runtime-executor
@prisma-next/mongo-codec
@prisma-next/mongo-contract
@prisma-next/mongo-value
@prisma-next/mongo-contract-psl
@prisma-next/mongo-emitter
@prisma-next/mongo-query-ast
@prisma-next/mongo-orm
@prisma-next/mongo-pipeline-builder
@prisma-next/mongo-lowering
@prisma-next/mongo-wire
@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/sql-lane-query-builder
@prisma-next/sql-relational-core
@prisma-next/sql-builder
@prisma-next/target-postgres
@prisma-next/adapter-postgres
@prisma-next/driver-postgres
commit: |
e2d7990 to
e12abbb
Compare
e8a47b2 to
5e4d739
Compare
2fe21ce to
712095b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/3-extensions/sql-orm-client/test/integration/nested-mutations.test.ts (1)
36-37: Keep non-extension nested-mutation tests decoupled fromembedding.These assertions hard-code an extension-dependent field in tests whose purpose is nested mutation behavior. Prefer partial assertions that validate relational/mutation outcomes and omit
embeddinghere.♻️ Suggested pattern
- expect(created).toEqual({ + expect(created).toMatchObject({ id: 20, title: 'Connected Post', userId: 5, views: 7, - embedding: null, });As per coding guidelines
**/*.test.ts: "Avoid selecting or using PostgreSQL extension-dependent columns (e.g., vector types) in tests that are not specifically testing extension functionality".Also applies to: 73-73, 130-130, 164-164
🤖 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/nested-mutations.test.ts` around lines 36 - 37, The test is asserting an extension-dependent field `embedding` in nested-mutations.test.ts; remove `embedding` from the hard-coded expected post objects (e.g., the objects with id 10/11 and similar assertions at the other mentioned locations) or switch to partial assertions (e.g., use expect.objectContaining or compare only id/title/userId/views) so the tests only validate relational/mutation outcomes and do not reference PostgreSQL extension-dependent columns; update the assertions at the occurrences flagged (the entries around id 10/11 and the other occurrences) to omit `embedding` or use partial matching.packages/3-extensions/sql-orm-client/test/query-plan-select.test.ts (1)
201-204: Inconsistent OrderByItem construction in assertion.Lines 201-204 use
new OrderByItem(...)constructor directly while lines 171-172 use the static factory methodsOrderByItem.asc(...)andOrderByItem.desc(...). For consistency and clarity, prefer the static factory methods throughout.♻️ Suggested refactor for consistency
expect(plan.ast.orderBy).toEqual([ - new OrderByItem(ColumnRef.of('posts', 'id'), 'asc'), - new OrderByItem(opExpr, 'desc'), + OrderByItem.asc(ColumnRef.of('posts', 'id')), + OrderByItem.desc(opExpr), ]);🤖 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-select.test.ts` around lines 201 - 204, The assertion constructs OrderByItem instances directly; change them to use the static factories for consistency: replace new OrderByItem(ColumnRef.of('posts','id'), 'asc') and new OrderByItem(opExpr, 'desc') with OrderByItem.asc(ColumnRef.of('posts','id')) and OrderByItem.desc(opExpr') respectively so the check against plan.ast.orderBy uses the same OrderByItem.asc(...) and OrderByItem.desc(...) patterns used earlier.packages/3-extensions/sql-orm-client/test/model-accessor.test.ts (1)
458-473: Consider using object matcher for method existence checks.The test checks multiple method existence properties individually. Per coding guidelines, prefer object matchers over multiple individual
expect().toBe()calls when checking 2+ related values.♻️ Suggested refactor using object matcher
- expect(typeof result['lt']).toBe('function'); - expect(typeof result['gt']).toBe('function'); - expect(typeof result['eq']).toBe('function'); - expect(typeof result['asc']).toBe('function'); - expect(typeof result['desc']).toBe('function'); - expect(typeof result['isNull']).toBe('function'); - expect(result['like']).toBeUndefined(); + expect(result).toMatchObject({ + lt: expect.any(Function), + gt: expect.any(Function), + eq: expect.any(Function), + asc: expect.any(Function), + desc: expect.any(Function), + isNull: expect.any(Function), + }); + expect(result).not.toHaveProperty('like');🤖 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/model-accessor.test.ts` around lines 458 - 473, Replace the multiple individual existence assertions for the embedding expression with a single object matcher assertion: after calling createModelAccessor(context, 'Post') and getting embedding.cosineDistance([1,2,3]) as result, assert that result contains functions for 'lt', 'gt', 'eq', 'asc', 'desc', and 'isNull' and that 'like' is undefined using one expect(result).toMatchObject(...) (or an equivalent object matcher) rather than multiple expect(typeof ...).toBe('function') calls; update the test around the variables accessor, embedding, and result in model-accessor.test.ts accordingly.
🤖 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/3-extensions/sql-orm-client/test/collection.state.test.ts`:
- Around line 131-138: The test fixtures define an OperationExpr.function with
method 'cosineDistance' (created via OperationExpr.function) but the template is
inverted as "1 - ({{self}} <=> {{arg0}})"; update the template to return the
actual cosine distance by replacing the template with "{{self}} <=> {{arg0}}"
for both instances of the 'cosineDistance' OperationExpr.function (the one using
ColumnRef.of('posts','embedding') and the other fixture) so the method name
matches the computation.
---
Nitpick comments:
In
`@packages/3-extensions/sql-orm-client/test/integration/nested-mutations.test.ts`:
- Around line 36-37: The test is asserting an extension-dependent field
`embedding` in nested-mutations.test.ts; remove `embedding` from the hard-coded
expected post objects (e.g., the objects with id 10/11 and similar assertions at
the other mentioned locations) or switch to partial assertions (e.g., use
expect.objectContaining or compare only id/title/userId/views) so the tests only
validate relational/mutation outcomes and do not reference PostgreSQL
extension-dependent columns; update the assertions at the occurrences flagged
(the entries around id 10/11 and the other occurrences) to omit `embedding` or
use partial matching.
In `@packages/3-extensions/sql-orm-client/test/model-accessor.test.ts`:
- Around line 458-473: Replace the multiple individual existence assertions for
the embedding expression with a single object matcher assertion: after calling
createModelAccessor(context, 'Post') and getting
embedding.cosineDistance([1,2,3]) as result, assert that result contains
functions for 'lt', 'gt', 'eq', 'asc', 'desc', and 'isNull' and that 'like' is
undefined using one expect(result).toMatchObject(...) (or an equivalent object
matcher) rather than multiple expect(typeof ...).toBe('function') calls; update
the test around the variables accessor, embedding, and result in
model-accessor.test.ts accordingly.
In `@packages/3-extensions/sql-orm-client/test/query-plan-select.test.ts`:
- Around line 201-204: The assertion constructs OrderByItem instances directly;
change them to use the static factories for consistency: replace new
OrderByItem(ColumnRef.of('posts','id'), 'asc') and new OrderByItem(opExpr,
'desc') with OrderByItem.asc(ColumnRef.of('posts','id')) and
OrderByItem.desc(opExpr') respectively so the check against plan.ast.orderBy
uses the same OrderByItem.asc(...) and OrderByItem.desc(...) patterns used
earlier.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 4a02a593-31ff-4a4b-a390-bc59b76be7e2
⛔ Files ignored due to path filters (8)
packages/2-sql/4-lanes/sql-builder/test/fixtures/generated/contract.d.tsis excluded by!**/generated/**packages/3-extensions/sql-orm-client/test/fixtures/generated/contract.d.tsis excluded by!**/generated/**packages/3-extensions/sql-orm-client/test/fixtures/generated/contract.jsonis excluded by!**/generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlprojects/orm-client-extensions/plan.mdis excluded by!projects/**projects/orm-client-extensions/spec.mdis excluded by!projects/**test/e2e/framework/test/fixtures/generated/contract.d.tsis excluded by!**/generated/**test/integration/test/mongo/fixtures/generated/contract.d.tsis excluded by!**/generated/**
📒 Files selected for processing (29)
examples/prisma-next-demo/src/main.tsexamples/prisma-next-demo/src/orm-client/find-similar-posts.tsexamples/prisma-next-demo/src/orm-client/search-posts-by-embedding.tsexamples/prisma-next-demo/src/prisma/contract.d.tsexamples/prisma-next-demo/test/repositories.integration.test.tspackages/3-extensions/sql-orm-client/package.jsonpackages/3-extensions/sql-orm-client/src/collection.tspackages/3-extensions/sql-orm-client/src/exports/index.tspackages/3-extensions/sql-orm-client/src/model-accessor.tspackages/3-extensions/sql-orm-client/src/query-plan-select.tspackages/3-extensions/sql-orm-client/src/types.tspackages/3-extensions/sql-orm-client/src/where-binding.tspackages/3-extensions/sql-orm-client/test/collection.state.test.tspackages/3-extensions/sql-orm-client/test/filters.test.tspackages/3-extensions/sql-orm-client/test/fixtures/contract.tspackages/3-extensions/sql-orm-client/test/fixtures/prisma-next.config.tspackages/3-extensions/sql-orm-client/test/helpers.tspackages/3-extensions/sql-orm-client/test/integration/extension-operations.test.tspackages/3-extensions/sql-orm-client/test/integration/include.test.tspackages/3-extensions/sql-orm-client/test/integration/nested-mutations.test.tspackages/3-extensions/sql-orm-client/test/integration/orm.test.tspackages/3-extensions/sql-orm-client/test/integration/runtime-helpers.tspackages/3-extensions/sql-orm-client/test/integration/update.test.tspackages/3-extensions/sql-orm-client/test/model-accessor.test.tspackages/3-extensions/sql-orm-client/test/query-plan-aggregate.test.tspackages/3-extensions/sql-orm-client/test/query-plan-select.test.tspackages/3-extensions/sql-orm-client/test/repository.test.tspackages/3-extensions/sql-orm-client/test/rich-filters-and-where.test.tstest/integration/test/fixtures/contract.d.ts
💤 Files with no reviewable changes (2)
- packages/3-extensions/sql-orm-client/test/query-plan-aggregate.test.ts
- packages/3-extensions/sql-orm-client/src/exports/index.ts
712095b to
15d8319
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/3-extensions/sql-orm-client/src/query-plan-select.ts (2)
90-119:⚠️ Potential issue | 🟠 MajorReject non-column
orderByitems in cursor mode.
Collection.orderBy()can now carry extensionOperationExprorders, but this loop just drops them. That makes the cursor predicate no longer match the emittedORDER BY, so pagination can skip/duplicate rows; if every item is expression-based, this also falls through with an emptyentriesset. Failing fast here is safer than compiling a partial boundary.🛑 Safer fallback
function buildCursorWhere( tableName: string, orderBy: readonly OrderByItem[] | undefined, cursor: Readonly<Record<string, unknown>> | undefined, ): AnyExpression | undefined { if (!cursor || !orderBy || orderBy.length === 0) { return undefined; } const entries: CursorOrderEntry[] = []; for (const order of orderBy) { - if (order.expr.kind !== 'column-ref') continue; + if (order.expr.kind !== 'column-ref') { + throw new Error('Cursor pagination only supports column-based orderBy items'); + } const column = order.expr.column; const value = cursor[column]; if (value === undefined) { throw new Error(`Missing cursor value for orderBy column "${column}"`); }🤖 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-select.ts` around lines 90 - 119, buildCursorWhere currently silently skips non-column orderBy items which makes the generated cursor predicate mismatch the actual ORDER BY; change the loop in buildCursorWhere to reject any order where order.expr.kind !== 'column-ref' by throwing a clear Error (e.g. "Non-column orderBy item in cursor mode not supported") so callers fail fast, and also throw if after processing there are zero entries (instead of falling through) before calling createBoundaryExpr or buildLexicographicCursorWhere; reference buildCursorWhere, createBoundaryExpr and buildLexicographicCursorWhere when locating the change.
165-191:⚠️ Potential issue | 🟠 MajorRemap include order expressions to the child alias.
buildIncludeChildRowsSelect()still aliases self-relations, butbuildIncludeOrderArtifacts()now forwardsorderByunchanged. For self-relations, the childORDER BYand hidden projections keep pointing atinclude.relatedTableNameinstead ofchildTableRef, which can bind to the wrong table inside the subquery or produce invalid SQL.Also applies to: 222-225
🤖 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-select.ts` around lines 165 - 191, buildIncludeOrderArtifacts currently keeps the original orderBy expressions (orderItem.expr) which still reference the parent relationName; for self-relations those expressions must be remapped to the child alias (rowAlias). Update buildIncludeOrderArtifacts so that when creating the hiddenOrderProjection (ProjectionItem.of) and the aggregateOrderBy (new OrderByItem(..., orderItem.dir)) you first transform orderItem.expr by replacing any ColumnRef that targets relationName with a ColumnRef that targets rowAlias (i.e., remap table alias inside OrderByItem.expr to rowAlias), and apply the same remapping in the analogous code path in buildIncludeChildRowsSelect (the block around lines 222-225) so hidden projections and generated OrderByItems bind to the child subquery alias instead of include.relatedTableName.packages/3-extensions/sql-orm-client/src/types.ts (1)
276-349:⚠️ Potential issue | 🔴 CriticalRestore null-aware
eq/neqgeneration.
scalarComparisonMethod()now backseqandneq, sofield.eq(null)/field.neq(null)emitBinaryExprwithParamRef(null). That regresses nullable filters to SQL= NULL/<> NULLsemantics instead ofIS NULL/IS NOT NULL.🩹 Restore the null special case
+function equalityComparisonMethod(op: 'eq' | 'neq') { + return ((left, codecId) => (value: unknown) => { + if (value === null) { + return op === 'eq' ? NullCheckExpr.isNull(left) : NullCheckExpr.isNotNull(left); + } + return new BinaryExpr(op, left, param(codecId, value)); + }) satisfies MethodFactory; +} + export const COMPARISON_METHODS_META = { eq: { traits: ['equality'], - create: scalarComparisonMethod('eq'), + create: equalityComparisonMethod('eq'), }, neq: { traits: ['equality'], - create: scalarComparisonMethod('neq'), + create: equalityComparisonMethod('neq'), },Based on learnings: When generating SQL for comparison operators in the SQL ORM client, do not emit
col = NULLorcol <> NULL; ifvalue === nulland the operator iseq, emitNullCheckExpr.isNull(left), and ifneq, emitNullCheckExpr.isNotNull(left).🤖 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/types.ts` around lines 276 - 349, The eq/neq methods were wired to scalarComparisonMethod which always emits a BinaryExpr(param(...)) even for nulls, producing "col = NULL"/"col <> NULL"; modify the factory so eq/neq treat null specially: update scalarComparisonMethod (or override create for the eq and neq entries in COMPARISON_METHODS_META) to check if value === null and if so return NullCheckExpr.isNull(left) for 'eq' and NullCheckExpr.isNotNull(left) for 'neq', otherwise continue to return new BinaryExpr(op, left, param(codecId, value)); keep references to BinaryExpr, param, paramList, NullCheckExpr.isNull/isNotNull, scalarComparisonMethod and COMPARISON_METHODS_META so the change is easy to locate.
🧹 Nitpick comments (2)
examples/prisma-next-demo/test/repositories.integration.test.ts (1)
670-726: Split this test file; it now exceeds the repository size limit for test files.These new cases are valuable, but this file is already beyond the 500-line cap. Please move embedding scenarios into a dedicated file (for example,
repositories.embedding.integration.test.ts) to keep tests navigable.As per coding guidelines: "
**/*.test.ts: Keep test files under 500 lines ... split by functionality."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/prisma-next-demo/test/repositories.integration.test.ts` around lines 670 - 726, The embedding-related test cases (the it blocks referencing ormClientFindSimilarPosts, ormClientSearchPostsByEmbedding, seedEmbeddingPosts, and embeddingPostIds) should be moved out of this oversized file into a new test file (e.g., repositories.embedding.integration.test.ts); extract both "ormClientFindSimilarPosts returns posts ordered by cosine distance with user include" and "ormClientSearchPostsByEmbedding returns posts within max cosine distance ordered by similarity" into the new file, copy over any required imports, helper functions (withDevDatabase, initTestDatabase, getRuntime, makeVector, timeouts), and ensure the same teardown (runtime.close()) and timeout (timeouts.spinUpPpgDev) are preserved so the tests run unchanged.packages/3-extensions/sql-orm-client/test/integration/runtime-helpers.ts (1)
5-5: MoveencodeParamsto public API or create dedicated test export.The import directly from the internal
src/codecs/encodingmodule couples the test to the package's internal structure. Either exportencodeParamsfrom@prisma-next/sql-runtimepublic API or create a dedicated test export to keep the boundary stable and follow the guideline: "Imports must follow unidirectional layer dependencies within a domain."🤖 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 test imports encodeParams from an internal path, coupling tests to implementation; fix by exposing encodeParams via the package public API or a dedicated test export and update the test import: add a named re-export of encodeParams in the package entry (e.g., export { encodeParams } from './src/codecs/encoding' in the public index) or create a test-only module (e.g., src/test-exports.ts) that exports encodeParams and ensure the package exports it for tests, then change the import in runtime-helpers.ts to import { encodeParams } from '@prisma-next/sql-runtime' (or the new test-export module) so tests use the stable public surface.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/prisma-next-demo/test/repositories.integration.test.ts`:
- Around line 84-90: The makeVector function can silently overflow the intended
1536-dimension when leadingValues.length > 1536; add a bounds check at the start
of makeVector that throws a clear Error (or assertion) if leadingValues.length >
1536 so invalid fixtures fail fast, then proceed to allocate the 1536-length
array and copy values as before; reference the function name makeVector and the
fixed dimension value 1536 when implementing the guard.
---
Outside diff comments:
In `@packages/3-extensions/sql-orm-client/src/query-plan-select.ts`:
- Around line 90-119: buildCursorWhere currently silently skips non-column
orderBy items which makes the generated cursor predicate mismatch the actual
ORDER BY; change the loop in buildCursorWhere to reject any order where
order.expr.kind !== 'column-ref' by throwing a clear Error (e.g. "Non-column
orderBy item in cursor mode not supported") so callers fail fast, and also throw
if after processing there are zero entries (instead of falling through) before
calling createBoundaryExpr or buildLexicographicCursorWhere; reference
buildCursorWhere, createBoundaryExpr and buildLexicographicCursorWhere when
locating the change.
- Around line 165-191: buildIncludeOrderArtifacts currently keeps the original
orderBy expressions (orderItem.expr) which still reference the parent
relationName; for self-relations those expressions must be remapped to the child
alias (rowAlias). Update buildIncludeOrderArtifacts so that when creating the
hiddenOrderProjection (ProjectionItem.of) and the aggregateOrderBy (new
OrderByItem(..., orderItem.dir)) you first transform orderItem.expr by replacing
any ColumnRef that targets relationName with a ColumnRef that targets rowAlias
(i.e., remap table alias inside OrderByItem.expr to rowAlias), and apply the
same remapping in the analogous code path in buildIncludeChildRowsSelect (the
block around lines 222-225) so hidden projections and generated OrderByItems
bind to the child subquery alias instead of include.relatedTableName.
In `@packages/3-extensions/sql-orm-client/src/types.ts`:
- Around line 276-349: The eq/neq methods were wired to scalarComparisonMethod
which always emits a BinaryExpr(param(...)) even for nulls, producing "col =
NULL"/"col <> NULL"; modify the factory so eq/neq treat null specially: update
scalarComparisonMethod (or override create for the eq and neq entries in
COMPARISON_METHODS_META) to check if value === null and if so return
NullCheckExpr.isNull(left) for 'eq' and NullCheckExpr.isNotNull(left) for 'neq',
otherwise continue to return new BinaryExpr(op, left, param(codecId, value));
keep references to BinaryExpr, param, paramList, NullCheckExpr.isNull/isNotNull,
scalarComparisonMethod and COMPARISON_METHODS_META so the change is easy to
locate.
---
Nitpick comments:
In `@examples/prisma-next-demo/test/repositories.integration.test.ts`:
- Around line 670-726: The embedding-related test cases (the it blocks
referencing ormClientFindSimilarPosts, ormClientSearchPostsByEmbedding,
seedEmbeddingPosts, and embeddingPostIds) should be moved out of this oversized
file into a new test file (e.g., repositories.embedding.integration.test.ts);
extract both "ormClientFindSimilarPosts returns posts ordered by cosine distance
with user include" and "ormClientSearchPostsByEmbedding returns posts within max
cosine distance ordered by similarity" into the new file, copy over any required
imports, helper functions (withDevDatabase, initTestDatabase, getRuntime,
makeVector, timeouts), and ensure the same teardown (runtime.close()) and
timeout (timeouts.spinUpPpgDev) are preserved so the tests run unchanged.
In `@packages/3-extensions/sql-orm-client/test/integration/runtime-helpers.ts`:
- Line 5: The test imports encodeParams from an internal path, coupling tests to
implementation; fix by exposing encodeParams via the package public API or a
dedicated test export and update the test import: add a named re-export of
encodeParams in the package entry (e.g., export { encodeParams } from
'./src/codecs/encoding' in the public index) or create a test-only module (e.g.,
src/test-exports.ts) that exports encodeParams and ensure the package exports it
for tests, then change the import in runtime-helpers.ts to import { encodeParams
} from '@prisma-next/sql-runtime' (or the new test-export module) so tests use
the stable public surface.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 4af4c745-60d6-4c54-b6f1-3cdc47b81e28
⛔ Files ignored due to path filters (5)
packages/3-extensions/sql-orm-client/test/fixtures/generated/contract.d.tsis excluded by!**/generated/**packages/3-extensions/sql-orm-client/test/fixtures/generated/contract.jsonis excluded by!**/generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlprojects/orm-client-extensions/plan.mdis excluded by!projects/**projects/orm-client-extensions/spec.mdis excluded by!projects/**
📒 Files selected for processing (27)
examples/prisma-next-demo/src/main.tsexamples/prisma-next-demo/src/orm-client/find-similar-posts.tsexamples/prisma-next-demo/src/orm-client/search-posts-by-embedding.tsexamples/prisma-next-demo/test/repositories.integration.test.tspackages/3-extensions/sql-orm-client/package.jsonpackages/3-extensions/sql-orm-client/src/collection.tspackages/3-extensions/sql-orm-client/src/exports/index.tspackages/3-extensions/sql-orm-client/src/model-accessor.tspackages/3-extensions/sql-orm-client/src/query-plan-select.tspackages/3-extensions/sql-orm-client/src/types.tspackages/3-extensions/sql-orm-client/src/where-binding.tspackages/3-extensions/sql-orm-client/test/collection.state.test.tspackages/3-extensions/sql-orm-client/test/filters.test.tspackages/3-extensions/sql-orm-client/test/fixtures/contract.tspackages/3-extensions/sql-orm-client/test/fixtures/prisma-next.config.tspackages/3-extensions/sql-orm-client/test/helpers.tspackages/3-extensions/sql-orm-client/test/integration/extension-operations.test.tspackages/3-extensions/sql-orm-client/test/integration/include.test.tspackages/3-extensions/sql-orm-client/test/integration/nested-mutations.test.tspackages/3-extensions/sql-orm-client/test/integration/orm.test.tspackages/3-extensions/sql-orm-client/test/integration/runtime-helpers.tspackages/3-extensions/sql-orm-client/test/integration/update.test.tspackages/3-extensions/sql-orm-client/test/model-accessor.test.tspackages/3-extensions/sql-orm-client/test/query-plan-aggregate.test.tspackages/3-extensions/sql-orm-client/test/query-plan-select.test.tspackages/3-extensions/sql-orm-client/test/repository.test.tspackages/3-extensions/sql-orm-client/test/rich-filters-and-where.test.ts
💤 Files with no reviewable changes (2)
- packages/3-extensions/sql-orm-client/test/query-plan-aggregate.test.ts
- packages/3-extensions/sql-orm-client/src/exports/index.ts
✅ Files skipped from review due to trivial changes (9)
- packages/3-extensions/sql-orm-client/test/helpers.ts
- packages/3-extensions/sql-orm-client/test/integration/orm.test.ts
- packages/3-extensions/sql-orm-client/package.json
- packages/3-extensions/sql-orm-client/src/where-binding.ts
- packages/3-extensions/sql-orm-client/test/repository.test.ts
- packages/3-extensions/sql-orm-client/test/integration/nested-mutations.test.ts
- packages/3-extensions/sql-orm-client/test/fixtures/contract.ts
- packages/3-extensions/sql-orm-client/test/integration/include.test.ts
- packages/3-extensions/sql-orm-client/test/integration/extension-operations.test.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/3-extensions/sql-orm-client/test/rich-filters-and-where.test.ts
- packages/3-extensions/sql-orm-client/test/fixtures/prisma-next.config.ts
- packages/3-extensions/sql-orm-client/test/integration/update.test.ts
- examples/prisma-next-demo/src/main.ts
- packages/3-extensions/sql-orm-client/test/collection.state.test.ts
- examples/prisma-next-demo/src/orm-client/find-similar-posts.ts
- examples/prisma-next-demo/src/orm-client/search-posts-by-embedding.ts
- packages/3-extensions/sql-orm-client/src/collection.ts
- packages/3-extensions/sql-orm-client/test/model-accessor.test.ts
- packages/3-extensions/sql-orm-client/src/model-accessor.ts
e5f8d2c to
6e23ab1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/3-extensions/sql-orm-client/test/query-plan-select.test.ts (1)
152-159: Extract repeated cosineDistanceOperationExprfixture into a helper.The same operation fixture is duplicated multiple times; a local helper would reduce drift risk and simplify maintenance.
🧩 Suggested helper extraction
+function cosineDistanceExpr(paramName: string, vec: number[]) { + return OperationExpr.function({ + method: 'cosineDistance', + forTypeId: 'pg/vector@1', + self: ColumnRef.of('posts', 'embedding'), + args: [ParamRef.of(vec, { name: paramName, codecId: 'pg/vector@1' })], + returns: { kind: 'builtin', type: 'number' }, + template: '{{self}} <=> {{arg0}}', + }); +} -const opExpr = OperationExpr.function({ ...searchVec...[1,2,3]... }); +const opExpr = cosineDistanceExpr('searchVec', [1, 2, 3]); -const orderOpExpr = OperationExpr.function({ ...orderVec...[4,5,6]... }); +const orderOpExpr = cosineDistanceExpr('orderVec', [4, 5, 6]);Also applies to: 182-189, 212-219, 241-248, 252-259
🤖 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-select.test.ts` around lines 152 - 159, Multiple tests repeat the same cosineDistance OperationExpr construction (using OperationExpr.function with method 'cosineDistance', forTypeId 'pg/vector@1', self ColumnRef.of('posts','embedding'), ParamRef.of([1,2,3], { name: 'searchVec', codecId: 'pg/vector@1' }), returns builtin number, template '{{self}} <=> {{arg0}}'); extract that block into a shared helper (e.g., makeCosineDistanceOp or cosineDistanceOpFixture) that returns the OperationExpr instance and replace the duplicated inline constructions (currently named/opExpr usages) in tests at the noted locations with calls to the helper to reduce duplication and maintenance risk.
🤖 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/3-extensions/sql-orm-client/test/integration/runtime-helpers.ts`:
- Line 5: The test imports the internal encodeParams function directly from src,
which violates the public API rule; either export encodeParams from the
package's public exports (add encodeParams to src/exports/index.ts and re-export
the implementation so runtime-helpers.ts can import it via
`@prisma-next/sql-runtime`) or change
packages/3-extensions/sql-orm-client/test/integration/runtime-helpers.ts to
remove the internal dependency by reimplementing the minimal encoding logic
needed for the tests or using an existing public helper; reference the
encodeParams symbol in your change and update the test import to use the package
export instead of the internal path.
---
Nitpick comments:
In `@packages/3-extensions/sql-orm-client/test/query-plan-select.test.ts`:
- Around line 152-159: Multiple tests repeat the same cosineDistance
OperationExpr construction (using OperationExpr.function with method
'cosineDistance', forTypeId 'pg/vector@1', self
ColumnRef.of('posts','embedding'), ParamRef.of([1,2,3], { name: 'searchVec',
codecId: 'pg/vector@1' }), returns builtin number, template '{{self}} <=>
{{arg0}}'); extract that block into a shared helper (e.g., makeCosineDistanceOp
or cosineDistanceOpFixture) that returns the OperationExpr instance and replace
the duplicated inline constructions (currently named/opExpr usages) in tests at
the noted locations with calls to the helper to reduce duplication and
maintenance risk.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: a0ab80ce-0ab0-4528-8db0-56489cf20b71
⛔ Files ignored due to path filters (3)
packages/3-extensions/sql-orm-client/test/fixtures/generated/contract.d.tsis excluded by!**/generated/**packages/3-extensions/sql-orm-client/test/fixtures/generated/contract.jsonis excluded by!**/generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (27)
examples/prisma-next-demo/src/main.tsexamples/prisma-next-demo/src/orm-client/find-similar-posts.tsexamples/prisma-next-demo/src/orm-client/search-posts-by-embedding.tsexamples/prisma-next-demo/test/repositories.integration.test.tspackages/3-extensions/sql-orm-client/package.jsonpackages/3-extensions/sql-orm-client/src/collection.tspackages/3-extensions/sql-orm-client/src/exports/index.tspackages/3-extensions/sql-orm-client/src/model-accessor.tspackages/3-extensions/sql-orm-client/src/query-plan-select.tspackages/3-extensions/sql-orm-client/src/types.tspackages/3-extensions/sql-orm-client/src/where-binding.tspackages/3-extensions/sql-orm-client/test/collection.state.test.tspackages/3-extensions/sql-orm-client/test/filters.test.tspackages/3-extensions/sql-orm-client/test/fixtures/contract.tspackages/3-extensions/sql-orm-client/test/fixtures/prisma-next.config.tspackages/3-extensions/sql-orm-client/test/helpers.tspackages/3-extensions/sql-orm-client/test/integration/extension-operations.test.tspackages/3-extensions/sql-orm-client/test/integration/include.test.tspackages/3-extensions/sql-orm-client/test/integration/nested-mutations.test.tspackages/3-extensions/sql-orm-client/test/integration/orm.test.tspackages/3-extensions/sql-orm-client/test/integration/runtime-helpers.tspackages/3-extensions/sql-orm-client/test/integration/update.test.tspackages/3-extensions/sql-orm-client/test/model-accessor.test.tspackages/3-extensions/sql-orm-client/test/query-plan-aggregate.test.tspackages/3-extensions/sql-orm-client/test/query-plan-select.test.tspackages/3-extensions/sql-orm-client/test/repository.test.tspackages/3-extensions/sql-orm-client/test/rich-filters-and-where.test.ts
💤 Files with no reviewable changes (2)
- packages/3-extensions/sql-orm-client/src/exports/index.ts
- packages/3-extensions/sql-orm-client/test/query-plan-aggregate.test.ts
✅ Files skipped from review due to trivial changes (7)
- packages/3-extensions/sql-orm-client/test/fixtures/prisma-next.config.ts
- packages/3-extensions/sql-orm-client/src/where-binding.ts
- packages/3-extensions/sql-orm-client/package.json
- packages/3-extensions/sql-orm-client/test/integration/update.test.ts
- packages/3-extensions/sql-orm-client/test/integration/nested-mutations.test.ts
- packages/3-extensions/sql-orm-client/test/integration/orm.test.ts
- packages/3-extensions/sql-orm-client/test/integration/extension-operations.test.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/3-extensions/sql-orm-client/test/helpers.ts
- packages/3-extensions/sql-orm-client/test/rich-filters-and-where.test.ts
- packages/3-extensions/sql-orm-client/test/repository.test.ts
- examples/prisma-next-demo/src/orm-client/search-posts-by-embedding.ts
- packages/3-extensions/sql-orm-client/test/integration/include.test.ts
- packages/3-extensions/sql-orm-client/test/collection.state.test.ts
- examples/prisma-next-demo/src/orm-client/find-similar-posts.ts
- packages/3-extensions/sql-orm-client/src/types.ts
- packages/3-extensions/sql-orm-client/src/model-accessor.ts
- Throw on ILIKE instead of silently mapping to LIKE; waiting for #277 to move ilike into an extension with proper capability gating - Document groupRowsByColumnSignature order-preservation as deliberate - Add unit tests for compileInsertReturningSplit/compileInsertCountSplit - Document sql.defaultInInsert capability in capabilities reference
6e23ab1 to
9a32e48
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/3-extensions/sql-orm-client/src/query-plan-select.ts (1)
90-119:⚠️ Potential issue | 🟠 MajorReject
.cursor()when anyorderByitem is expression-based.Line 101 silently drops non-
column-refsort keys. With the new extension operators,.orderBy((post) => post.embedding.cosineDistance(v).asc()).cursor(...)now builds a boundary from only the remaining column keys—or from none at all—so pagination can skip/duplicate rows instead of following the actual sort order.💡 Suggested fix
const entries: CursorOrderEntry[] = []; for (const order of orderBy) { - if (order.expr.kind !== 'column-ref') continue; + if (order.expr.kind !== 'column-ref') { + throw new Error('cursor() only supports column-based orderBy items'); + } const column = order.expr.column; const value = cursor[column]; if (value === undefined) { throw new Error(`Missing cursor value for orderBy column "${column}"`); }🤖 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-select.ts` around lines 90 - 119, The buildCursorWhere function currently ignores non-column order keys causing incorrect pagination; modify the for-loop in buildCursorWhere to reject/throw when any order.expr.kind !== 'column-ref' (referencing OrderByItem and buildCursorWhere) instead of continuing silently, with a clear error message like "cursor pagination unsupported for expression-based orderBy item" (so callers know to remove .cursor() or change orderBy); keep the existing behavior for column-ref entries and continue to call createBoundaryExpr and buildLexicographicCursorWhere for valid column-based entries.
🧹 Nitpick comments (2)
packages/3-extensions/sql-orm-client/test/integration/include.test.ts (1)
1-668: Consider splitting this test file (668 lines) to improve maintainability.This file exceeds the 500-line guideline. While the current PR only adds a few lines, consider splitting the file in a follow-up by logical groupings such as:
include.basic.test.ts— one-to-many/one-to-one relation testsinclude.aggregates.test.ts— count/sum/avg/min/max testsinclude.strategies.test.ts— lateral/correlated strategy testsinclude.nested.test.ts— multi-level include testsAs 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 - 668, The test file describe('integration/include') is over 500 lines and should be split by logical groups; create separate test files (e.g., include.basic.test.ts, include.aggregates.test.ts, include.strategies.test.ts, include.nested.test.ts) and move the corresponding it(...) blocks into them, keeping shared helpers (expectSelectAst, expectDerivedTableSource, expectSubqueryExpr, expectJsonArrayAggExpr, createUsersCollectionWithCapabilities, NumericPostField type, and imports like withCollectionRuntime/seed*/* helpers) in a common test helper module or re-export them from a small top-level test-utils file so each split test file imports only what it needs, update imports at the top of each new file accordingly, and run tests to ensure no breaking import/name changes (refer to unique symbols: describe('integration/include'), createUsersCollectionWithCapabilities, withCollectionRuntime, seedUsers/seedPosts/seedComments/seedProfiles).examples/prisma-next-demo/test/repositories.integration.test.ts (1)
202-727: Split this test file into focused suites to stay under the 500-line cap.This file is now 727 lines and exceeds the repository test-size limit. Please split by feature area (for example, keep core repository tests here and move embedding/vector scenarios to
repositories.embedding.integration.test.ts).As per coding guidelines: "
**/*.test.ts: Keep test files under 500 lines... Split test files when they exceed 500 lines... Use descriptive file names with the pattern{base}.{category}.test.ts."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/prisma-next-demo/test/repositories.integration.test.ts` around lines 202 - 727, The test file exceeds the 500-line limit; split the long integration suite into focused files by feature — keep core repository tests (those referencing ormClientGetUsers, ormClientGetAdminUsers, ormClientFindUserByEmail, ormClientCreateUser, ormClientAggregateUsers, ormClientGetUserPosts, ormClientGetDashboardUsers, ormClientGetPostFeed, pagination tests like ormClientGetUsersByIdCursor and ormClientGetUsersBackwardCursor, upsert/delete tests, and user-insight/grouping tests) in the current suite and move embedding/vector-related scenarios (those that call seedEmbeddingPosts, ormClientFindSimilarPosts, ormClientSearchPostsByEmbedding, makeVector, and use embeddingPostIds) into a new file named repositories.embedding.integration.test.ts; ensure each new test file preserves the original setup/teardown helpers (withDevDatabase, initTestDatabase, getRuntime, seedOrmClientData) and imports, and update test exports/describe blocks so each file stays under 500 lines.
🤖 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/3-extensions/sql-orm-client/src/query-plan-select.ts`:
- Around line 165-175: When building include order artifacts in
buildIncludeOrderArtifacts, remap any nested orderBy/hidden projection column
references that point at the base relation to the child row alias so
self-relations order/project the correct alias: walk childOrderBy (and
aggregateOrderBy) and replace table references that equal the relationName/base
table with rowAlias, and do the same for entries in hiddenOrderProjection, then
pass the remapped childOrderBy into withOrderBy (and use remapped projections)
so all order/projection expressions target the child alias for self-referential
includes.
In `@packages/3-extensions/sql-orm-client/src/types.ts`:
- Around line 293-300: The eq/neq factories created via scalarComparisonMethod
currently wrap all values (including null) into BinaryExpr(op, left,
param(codecId, value)), producing invalid SQL; update the factories (or
scalarComparisonMethod) so that when value === null they return
NullCheckExpr.isNull(left) for eq and NullCheckExpr.isNotNull(left) for neq
instead of BinaryExpr; keep using the existing param(codecId, value) path for
non-null values and preserve codecId handling and traits.
---
Outside diff comments:
In `@packages/3-extensions/sql-orm-client/src/query-plan-select.ts`:
- Around line 90-119: The buildCursorWhere function currently ignores non-column
order keys causing incorrect pagination; modify the for-loop in buildCursorWhere
to reject/throw when any order.expr.kind !== 'column-ref' (referencing
OrderByItem and buildCursorWhere) instead of continuing silently, with a clear
error message like "cursor pagination unsupported for expression-based orderBy
item" (so callers know to remove .cursor() or change orderBy); keep the existing
behavior for column-ref entries and continue to call createBoundaryExpr and
buildLexicographicCursorWhere for valid column-based entries.
---
Nitpick comments:
In `@examples/prisma-next-demo/test/repositories.integration.test.ts`:
- Around line 202-727: The test file exceeds the 500-line limit; split the long
integration suite into focused files by feature — keep core repository tests
(those referencing ormClientGetUsers, ormClientGetAdminUsers,
ormClientFindUserByEmail, ormClientCreateUser, ormClientAggregateUsers,
ormClientGetUserPosts, ormClientGetDashboardUsers, ormClientGetPostFeed,
pagination tests like ormClientGetUsersByIdCursor and
ormClientGetUsersBackwardCursor, upsert/delete tests, and user-insight/grouping
tests) in the current suite and move embedding/vector-related scenarios (those
that call seedEmbeddingPosts, ormClientFindSimilarPosts,
ormClientSearchPostsByEmbedding, makeVector, and use embeddingPostIds) into a
new file named repositories.embedding.integration.test.ts; ensure each new test
file preserves the original setup/teardown helpers (withDevDatabase,
initTestDatabase, getRuntime, seedOrmClientData) and imports, and update test
exports/describe blocks so each file stays under 500 lines.
In `@packages/3-extensions/sql-orm-client/test/integration/include.test.ts`:
- Around line 1-668: The test file describe('integration/include') is over 500
lines and should be split by logical groups; create separate test files (e.g.,
include.basic.test.ts, include.aggregates.test.ts, include.strategies.test.ts,
include.nested.test.ts) and move the corresponding it(...) blocks into them,
keeping shared helpers (expectSelectAst, expectDerivedTableSource,
expectSubqueryExpr, expectJsonArrayAggExpr,
createUsersCollectionWithCapabilities, NumericPostField type, and imports like
withCollectionRuntime/seed*/* helpers) in a common test helper module or
re-export them from a small top-level test-utils file so each split test file
imports only what it needs, update imports at the top of each new file
accordingly, and run tests to ensure no breaking import/name changes (refer to
unique symbols: describe('integration/include'),
createUsersCollectionWithCapabilities, withCollectionRuntime,
seedUsers/seedPosts/seedComments/seedProfiles).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 3c53ad11-3ad7-41f8-bfed-af87c66a84a4
⛔ Files ignored due to path filters (3)
packages/3-extensions/sql-orm-client/test/fixtures/generated/contract.d.tsis excluded by!**/generated/**packages/3-extensions/sql-orm-client/test/fixtures/generated/contract.jsonis excluded by!**/generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (27)
examples/prisma-next-demo/src/main.tsexamples/prisma-next-demo/src/orm-client/find-similar-posts.tsexamples/prisma-next-demo/src/orm-client/search-posts-by-embedding.tsexamples/prisma-next-demo/test/repositories.integration.test.tspackages/3-extensions/sql-orm-client/package.jsonpackages/3-extensions/sql-orm-client/src/collection.tspackages/3-extensions/sql-orm-client/src/exports/index.tspackages/3-extensions/sql-orm-client/src/model-accessor.tspackages/3-extensions/sql-orm-client/src/query-plan-select.tspackages/3-extensions/sql-orm-client/src/types.tspackages/3-extensions/sql-orm-client/src/where-binding.tspackages/3-extensions/sql-orm-client/test/collection.state.test.tspackages/3-extensions/sql-orm-client/test/filters.test.tspackages/3-extensions/sql-orm-client/test/fixtures/contract.tspackages/3-extensions/sql-orm-client/test/fixtures/prisma-next.config.tspackages/3-extensions/sql-orm-client/test/helpers.tspackages/3-extensions/sql-orm-client/test/integration/extension-operations.test.tspackages/3-extensions/sql-orm-client/test/integration/include.test.tspackages/3-extensions/sql-orm-client/test/integration/nested-mutations.test.tspackages/3-extensions/sql-orm-client/test/integration/orm.test.tspackages/3-extensions/sql-orm-client/test/integration/runtime-helpers.tspackages/3-extensions/sql-orm-client/test/integration/update.test.tspackages/3-extensions/sql-orm-client/test/model-accessor.test.tspackages/3-extensions/sql-orm-client/test/query-plan-aggregate.test.tspackages/3-extensions/sql-orm-client/test/query-plan-select.test.tspackages/3-extensions/sql-orm-client/test/repository.test.tspackages/3-extensions/sql-orm-client/test/rich-filters-and-where.test.ts
💤 Files with no reviewable changes (2)
- packages/3-extensions/sql-orm-client/test/query-plan-aggregate.test.ts
- packages/3-extensions/sql-orm-client/src/exports/index.ts
✅ Files skipped from review due to trivial changes (10)
- packages/3-extensions/sql-orm-client/test/integration/update.test.ts
- packages/3-extensions/sql-orm-client/package.json
- packages/3-extensions/sql-orm-client/test/helpers.ts
- packages/3-extensions/sql-orm-client/src/where-binding.ts
- packages/3-extensions/sql-orm-client/test/integration/orm.test.ts
- packages/3-extensions/sql-orm-client/test/collection.state.test.ts
- packages/3-extensions/sql-orm-client/test/rich-filters-and-where.test.ts
- examples/prisma-next-demo/src/main.ts
- examples/prisma-next-demo/src/orm-client/find-similar-posts.ts
- packages/3-extensions/sql-orm-client/test/integration/extension-operations.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/3-extensions/sql-orm-client/test/fixtures/contract.ts
- packages/3-extensions/sql-orm-client/test/fixtures/prisma-next.config.ts
- packages/3-extensions/sql-orm-client/test/repository.test.ts
- examples/prisma-next-demo/src/orm-client/search-posts-by-embedding.ts
- packages/3-extensions/sql-orm-client/test/filters.test.ts
- packages/3-extensions/sql-orm-client/test/query-plan-select.test.ts
- packages/3-extensions/sql-orm-client/test/integration/runtime-helpers.ts
9a32e48 to
2511c0e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/3-extensions/sql-orm-client/src/query-plan-select.ts (1)
90-119:⚠️ Potential issue | 🟠 MajorHandle the empty post-filter cursor case explicitly.
Now that
orderBy()can carry arbitraryOrderByItems, Lines 101-111 can legally skip every entry. Line 119 then callsbuildLexicographicCursorWhere(...)with[], which yieldsOrExpr.of([])instead of a usable cursor boundary for expression-only ordering.🤖 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-select.ts` around lines 90 - 119, The bug is that buildCursorWhere can call buildLexicographicCursorWhere with an empty entries array when all OrderByItem entries are non-column expressions; update buildCursorWhere so after collecting CursorOrderEntry items (in the loop over orderBy) you explicitly handle the empty-post-filter case by returning undefined when entries.length === 0, keep the existing single-entry branch using createBoundaryExpr for entries.length === 1, and only call buildLexicographicCursorWhere when entries.length > 1; reference buildCursorWhere, OrderByItem, createBoundaryExpr, and buildLexicographicCursorWhere to locate the change.packages/3-extensions/sql-orm-client/test/integration/runtime-helpers.ts (1)
90-105:⚠️ Potential issue | 🟠 MajorRecord
executionsthrough the runtime-backed lowerer, not a bare adapter.
toLoweredPlan()still uses a standalonecreatePostgresAdapter().lower(...), so the captured plan bypasses theextensionPacksyou attached to the execution context. That meansruntime.executionscan now diverge from the SQL/params thatrealRuntime.execute()actually uses, and extension-defined operations can fail here even when the runtime path succeeds. Please derive the recorded plan from the same context/runtime-backed lowering path thatrealRuntime.execute()uses. Cross-file context:packages/2-sql/5-runtime/src/sql-context.ts:446-465foldsstack.extensionPacksinto the execution context contributors, but this helper never uses that context when lowering.Also applies to: 123-125
🤖 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` around lines 90 - 105, toLoweredPlan currently calls the standalone adapter.lower (bypassing extension packs), so recorded executions can diverge from what realRuntime.execute uses; change to derive the lowered plan via the runtime-backed lowering path used by realRuntime.execute (i.e., call the runtime/context lowerer that folds stack.extensionPacks into contributors instead of createPostgresAdapter().lower or adapter.lower), so runtime.executions reflects the same SQL/params and extensions as realRuntime.execute; update toLoweredPlan to invoke that runtime lowerer with the same inputs (plan.ast, { contract, params }) and return lowered.body.sql/params while keeping ast and meta.
♻️ Duplicate comments (2)
packages/3-extensions/sql-orm-client/src/types.ts (1)
276-279:⚠️ Potential issue | 🟠 MajorKeep
eq(null)/neq(null)on the null-check path.The shared scalar factory now parameterizes every value, so direct null comparisons compile to
BinaryExpr(..., ParamRef.of(null)). In SQL that becomes= NULL/<> NULL, which breakseq(null)andneq(null).💡 Suggested fix
export const COMPARISON_METHODS_META = { eq: { traits: ['equality'], - create: scalarComparisonMethod('eq'), + create: (left, codecId) => (value: unknown) => + value === null + ? NullCheckExpr.isNull(left) + : new BinaryExpr('eq', left, param(codecId, value)), }, neq: { traits: ['equality'], - create: scalarComparisonMethod('neq'), + create: (left, codecId) => (value: unknown) => + value === null + ? NullCheckExpr.isNotNull(left) + : new BinaryExpr('neq', left, param(codecId, value)), },Based on learnings, when
value === nulland the operator iseq, emitNullCheckExpr.isNull(left); ifneq, emitNullCheckExpr.isNotNull(left).Also applies to: 293-299
🤖 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/types.ts` around lines 276 - 279, The scalarComparisonMethod currently wraps every value into a Param (via param(codecId, value)) causing eq(null)/neq(null) to compile to BinaryExpr(..., ParamRef.of(null)) which yields invalid SQL; update scalarComparisonMethod to check if value === null and if so return NullCheckExpr.isNull(left) for op === "eq" and NullCheckExpr.isNotNull(left) for op === "neq" instead of creating a Param/ BinaryExpr; otherwise keep the existing new BinaryExpr(op, left, param(codecId, value)) behavior. Ensure the same null-branch change is applied to the analogous factory at lines 293-299 referenced in the comment.packages/3-extensions/sql-orm-client/src/query-plan-select.ts (1)
165-191:⚠️ Potential issue | 🟠 MajorRemap nested
orderByexpressions to the child alias on self-relations.When the include self-joins,
buildIncludeChildRowsSelect()emitsFROM <table> AS <relation>__child, butchildOrderByandhiddenOrderProjectionstill reuse the original expressions. NestedorderBy()on self-relations will therefore project/order against the base table name instead of the child alias.💡 Suggested fix
function buildIncludeOrderArtifacts( relationName: string, rowAlias: string, + fromTable: string, + toTable: string, orderBy: readonly OrderByItem[] | undefined, ): { readonly childOrderBy: ReadonlyArray<OrderByItem> | undefined; readonly hiddenOrderProjection: ReadonlyArray<ProjectionItem>; readonly aggregateOrderBy: ReadonlyArray<OrderByItem> | undefined; } { - const childOrderBy = orderBy; + const childOrderBy = + fromTable === toTable + ? orderBy + : orderBy?.map( + (item) => + new OrderByItem( + item.expr.rewrite(createTableRefRemapper(fromTable, toTable)), + item.dir, + ), + ); if (!childOrderBy || childOrderBy.length === 0) { return { childOrderBy: undefined, @@ const { childOrderBy, hiddenOrderProjection, aggregateOrderBy } = buildIncludeOrderArtifacts( include.relationName, rowsAlias, + include.relatedTableName, + childTableRef, childState.orderBy, );Also applies to: 222-226
🤖 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-select.ts` around lines 165 - 191, buildIncludeOrderArtifacts is creating ProjectionItem.of using orderItem.expr from the original table, so self-join include orderBy expressions still reference the base table instead of the child alias; update buildIncludeOrderArtifacts (and the similar logic around lines 222-226) to first remap the orderItem.expr to use the child alias (rowAlias) before creating ProjectionItem.of and before computing aggregateOrderBy (e.g., via a small helper that walks/rewrites ColumnRef/table qualifiers in the expression to ColumnRef.of(rowAlias, ...)), then use that rewritten expression when constructing hiddenOrderProjection and aggregateOrderBy so nested orderBy uses the child alias.
🧹 Nitpick comments (1)
packages/3-extensions/sql-orm-client/test/integration/update.test.ts (1)
95-96: Keep this assertion focused on update/include behavior, not the pgvector column.Hard-coding
embedding: nullmakes this generic update-path test depend on an extension-backed field. Prefer partial matchers for the relatedpostsrows so the test still validates the selected scalars and relation data without coupling itself to pgvector-specific shape.Suggested fix
expect(updated).toEqual({ name: 'Alice Updated', posts: [ - { id: 10, title: 'Post A', userId: 1, views: 100, embedding: null }, - { id: 11, title: 'Post B', userId: 1, views: 200, embedding: null }, + expect.objectContaining({ + id: 10, + title: 'Post A', + userId: 1, + views: 100, + }), + expect.objectContaining({ + id: 11, + title: 'Post B', + userId: 1, + views: 200, + }), ], });As per coding guidelines, "
**/*.test.ts: Avoid selecting or using PostgreSQL extension-dependent columns (e.g., vector types) in tests that are not specifically testing extension functionality`."🤖 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/update.test.ts` around lines 95 - 96, The test is asserting full post objects including the extension-dependent `embedding` field; change the expectation to use partial matchers so the assertion focuses on update/include behavior (scalars and relations) and ignores `embedding`. Locate the assertion that expects posts like `{ id: 10, title: 'Post A', userId: 1, views: 100, embedding: null }` and replace it with an assertion using partial matchers (e.g., expect.arrayContaining with expect.objectContaining) that verifies only `id`, `title`, `userId`, `views` (and relation fields if needed) and does not reference `embedding`.
🤖 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/3-extensions/sql-orm-client/test/integration/runtime-helpers.ts`:
- Around line 62-86: The runtime setup can throw after the Postgres Pool has
been opened (the pool variable), leaking connections; wrap the initialization
sequence starting from await driver.connect({ kind: 'pgPool', pool }) through
createRuntime(...) in a try/catch, and in the catch call await pool.end() before
rethrowing the error so the Pool is always closed on setup failure; keep the
rest of the logic that returns the normal close handler unchanged (use the same
pool variable and driver/stackInstance/createRuntime identifiers to locate the
code).
---
Outside diff comments:
In `@packages/3-extensions/sql-orm-client/src/query-plan-select.ts`:
- Around line 90-119: The bug is that buildCursorWhere can call
buildLexicographicCursorWhere with an empty entries array when all OrderByItem
entries are non-column expressions; update buildCursorWhere so after collecting
CursorOrderEntry items (in the loop over orderBy) you explicitly handle the
empty-post-filter case by returning undefined when entries.length === 0, keep
the existing single-entry branch using createBoundaryExpr for entries.length ===
1, and only call buildLexicographicCursorWhere when entries.length > 1;
reference buildCursorWhere, OrderByItem, createBoundaryExpr, and
buildLexicographicCursorWhere to locate the change.
In `@packages/3-extensions/sql-orm-client/test/integration/runtime-helpers.ts`:
- Around line 90-105: toLoweredPlan currently calls the standalone adapter.lower
(bypassing extension packs), so recorded executions can diverge from what
realRuntime.execute uses; change to derive the lowered plan via the
runtime-backed lowering path used by realRuntime.execute (i.e., call the
runtime/context lowerer that folds stack.extensionPacks into contributors
instead of createPostgresAdapter().lower or adapter.lower), so
runtime.executions reflects the same SQL/params and extensions as
realRuntime.execute; update toLoweredPlan to invoke that runtime lowerer with
the same inputs (plan.ast, { contract, params }) and return
lowered.body.sql/params while keeping ast and meta.
---
Duplicate comments:
In `@packages/3-extensions/sql-orm-client/src/query-plan-select.ts`:
- Around line 165-191: buildIncludeOrderArtifacts is creating ProjectionItem.of
using orderItem.expr from the original table, so self-join include orderBy
expressions still reference the base table instead of the child alias; update
buildIncludeOrderArtifacts (and the similar logic around lines 222-226) to first
remap the orderItem.expr to use the child alias (rowAlias) before creating
ProjectionItem.of and before computing aggregateOrderBy (e.g., via a small
helper that walks/rewrites ColumnRef/table qualifiers in the expression to
ColumnRef.of(rowAlias, ...)), then use that rewritten expression when
constructing hiddenOrderProjection and aggregateOrderBy so nested orderBy uses
the child alias.
In `@packages/3-extensions/sql-orm-client/src/types.ts`:
- Around line 276-279: The scalarComparisonMethod currently wraps every value
into a Param (via param(codecId, value)) causing eq(null)/neq(null) to compile
to BinaryExpr(..., ParamRef.of(null)) which yields invalid SQL; update
scalarComparisonMethod to check if value === null and if so return
NullCheckExpr.isNull(left) for op === "eq" and NullCheckExpr.isNotNull(left) for
op === "neq" instead of creating a Param/ BinaryExpr; otherwise keep the
existing new BinaryExpr(op, left, param(codecId, value)) behavior. Ensure the
same null-branch change is applied to the analogous factory at lines 293-299
referenced in the comment.
---
Nitpick comments:
In `@packages/3-extensions/sql-orm-client/test/integration/update.test.ts`:
- Around line 95-96: The test is asserting full post objects including the
extension-dependent `embedding` field; change the expectation to use partial
matchers so the assertion focuses on update/include behavior (scalars and
relations) and ignores `embedding`. Locate the assertion that expects posts like
`{ id: 10, title: 'Post A', userId: 1, views: 100, embedding: null }` and
replace it with an assertion using partial matchers (e.g.,
expect.arrayContaining with expect.objectContaining) that verifies only `id`,
`title`, `userId`, `views` (and relation fields if needed) and does not
reference `embedding`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 242fcffc-d47a-41b2-a141-e2fcbb3b57e5
⛔ Files ignored due to path filters (5)
packages/3-extensions/sql-orm-client/test/fixtures/generated/contract.d.tsis excluded by!**/generated/**packages/3-extensions/sql-orm-client/test/fixtures/generated/contract.jsonis excluded by!**/generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlprojects/orm-client-extensions/plan.mdis excluded by!projects/**projects/orm-client-extensions/spec.mdis excluded by!projects/**
📒 Files selected for processing (27)
examples/prisma-next-demo/src/main.tsexamples/prisma-next-demo/src/orm-client/find-similar-posts.tsexamples/prisma-next-demo/src/orm-client/search-posts-by-embedding.tsexamples/prisma-next-demo/test/repositories.integration.test.tspackages/3-extensions/sql-orm-client/package.jsonpackages/3-extensions/sql-orm-client/src/collection.tspackages/3-extensions/sql-orm-client/src/exports/index.tspackages/3-extensions/sql-orm-client/src/model-accessor.tspackages/3-extensions/sql-orm-client/src/query-plan-select.tspackages/3-extensions/sql-orm-client/src/types.tspackages/3-extensions/sql-orm-client/src/where-binding.tspackages/3-extensions/sql-orm-client/test/collection.state.test.tspackages/3-extensions/sql-orm-client/test/filters.test.tspackages/3-extensions/sql-orm-client/test/fixtures/contract.tspackages/3-extensions/sql-orm-client/test/fixtures/prisma-next.config.tspackages/3-extensions/sql-orm-client/test/helpers.tspackages/3-extensions/sql-orm-client/test/integration/extension-operations.test.tspackages/3-extensions/sql-orm-client/test/integration/include.test.tspackages/3-extensions/sql-orm-client/test/integration/nested-mutations.test.tspackages/3-extensions/sql-orm-client/test/integration/orm.test.tspackages/3-extensions/sql-orm-client/test/integration/runtime-helpers.tspackages/3-extensions/sql-orm-client/test/integration/update.test.tspackages/3-extensions/sql-orm-client/test/model-accessor.test.tspackages/3-extensions/sql-orm-client/test/query-plan-aggregate.test.tspackages/3-extensions/sql-orm-client/test/query-plan-select.test.tspackages/3-extensions/sql-orm-client/test/repository.test.tspackages/3-extensions/sql-orm-client/test/rich-filters-and-where.test.ts
💤 Files with no reviewable changes (2)
- packages/3-extensions/sql-orm-client/test/query-plan-aggregate.test.ts
- packages/3-extensions/sql-orm-client/src/exports/index.ts
✅ Files skipped from review due to trivial changes (10)
- packages/3-extensions/sql-orm-client/src/where-binding.ts
- packages/3-extensions/sql-orm-client/test/fixtures/prisma-next.config.ts
- packages/3-extensions/sql-orm-client/test/integration/orm.test.ts
- packages/3-extensions/sql-orm-client/test/helpers.ts
- packages/3-extensions/sql-orm-client/package.json
- packages/3-extensions/sql-orm-client/test/rich-filters-and-where.test.ts
- packages/3-extensions/sql-orm-client/test/integration/nested-mutations.test.ts
- examples/prisma-next-demo/src/main.ts
- packages/3-extensions/sql-orm-client/test/integration/extension-operations.test.ts
- packages/3-extensions/sql-orm-client/test/integration/include.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/3-extensions/sql-orm-client/test/fixtures/contract.ts
- packages/3-extensions/sql-orm-client/test/repository.test.ts
- examples/prisma-next-demo/src/orm-client/search-posts-by-embedding.ts
- packages/3-extensions/sql-orm-client/test/query-plan-select.test.ts
- packages/3-extensions/sql-orm-client/src/model-accessor.ts
- packages/3-extensions/sql-orm-client/test/collection.state.test.ts
- packages/3-extensions/sql-orm-client/test/filters.test.ts
- Throw on ILIKE instead of silently mapping to LIKE; waiting for #277 to move ilike into an extension with proper capability gating - Document groupRowsByColumnSignature order-preservation as deliberate - Add unit tests for compileInsertReturningSplit/compileInsertCountSplit - Document sql.defaultInInsert capability in capabilities reference
wmadden
left a comment
There was a problem hiding this comment.
Approving preemptively — the implementation is well-structured, the type machinery is solid, and the integration tests with real pgvector demonstrate end-to-end correctness. Please address or respond to the inline comments before merging; no re-review needed.
One non-code item: the PR description still lists "Remaining work to do" items that appear to have been completed. Worth updating the description to avoid confusion.
| throw new Error(`Unknown column "${columnRef.column}" in table "${columnRef.table}"`); | ||
| } | ||
| return ParamRef.of(value, { name: columnRef.column, codecId }); | ||
| return ParamRef.of(value, { codecId }); |
There was a problem hiding this comment.
F04 — name removed from ParamRef: Previously ParamRef.of(value, { name: columnRef.column, codecId }) — the name provided a way to trace parameters back to their source column. Was the removal intentional? If so, a brief note on why would help. If not, it's a one-line fix to keep it.
There was a problem hiding this comment.
Using column names as param names is useless at best and a correctness issue at worst, param names must be unique if used.
- Throw on ILIKE instead of silently mapping to LIKE; waiting for #277 to move ilike into an extension with proper capability gating - Document groupRowsByColumnSignature order-preservation as deliberate - Add unit tests for compileInsertReturningSplit/compileInsertCountSplit - Document sql.defaultInInsert capability in capabilities reference
4bb3ede to
b68124d
Compare
b68124d to
fdaa5b6
Compare
- Throw on ILIKE instead of silently mapping to LIKE; waiting for #277 to move ilike into an extension with proper capability gating - Document groupRowsByColumnSignature order-preservation as deliberate - Add unit tests for compileInsertReturningSplit/compileInsertCountSplit - Document sql.defaultInInsert capability in capabilities reference
56cb4d2 to
a87d7cc
Compare
a87d7cc to
83227a9
Compare
- Throw on ILIKE instead of silently mapping to LIKE; waiting for #277 to move ilike into an extension with proper capability gating - Document groupRowsByColumnSignature order-preservation as deliberate - Add unit tests for compileInsertReturningSplit/compileInsertCountSplit - Document sql.defaultInInsert capability in capabilities reference
Summary
Add support for extension-defined operations (such as pgvector's
cosineDistanceandcosineSimilarity) in the ORM client. Extension operations are automatically surfaced onModelAccessorfields whose codec matches the operation's first argument, with full type safety — vector fields expose vector operations, while text/numeric fields do not.Key changes
ModelAccessorfields now expose methods from registered extension packs when the field's codec matches the operation'sselfargument. Calling an extension method (e.g.,p.embedding.cosineDistance(vec)) returns aComparisonMethodsobject whose available methods are gated by the return codec's traits — socosineDistance(returningfloat8) exposesgt,lt,asc,desc, etc.OrderExpr/OrderByDirectivewithOrderByItem: The ad-hoc ordering types incollection.tsandtypes.tshave been replaced with the AST'sOrderByItem, which supports arbitrary expressions (not just column refs) inORDER BYclauses. This is required for ordering by extension operation results likeembedding <=> $1.ParamRefinstead ofLiteralExprin comparison methods: Comparison method factories (eq,gt,in, etc.) now produceParamRefnodes with codec metadata instead of bareLiteralExprnodes. This ensures values flow through the parameterized query path with proper codec encoding.FieldOperations,QueryOperationMethod, etc.) map extension operations to the correct field types at compile time using codec ID matching, ensuring only valid operations appear in autocomplete/type checks.cosineSimilarityinwhere(),orderBy(), and combined usage against a real Postgres + pgvector instance.repo-similar-postsandrepo-search-postsCLI commands to the demo app showcasing the ORM client's vector search capabilities.Summary by CodeRabbit
New Features
Tests
Chores