Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 7 minutes and 58 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughIntroduces a codec trait system (equality, order, boolean, numeric, textual) and threads a typed ExecutionContext through the ORM; codec traits gate available comparison operators at runtime and in generated types via registry lookups and conditional typing. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant ORM as ORM (uses ExecutionContext)
participant CR as CodecRegistry
participant DB as Database/Runtime
Dev->>ORM: build client with ExecutionContext(context, codecs, contract)
ORM->>CR: traitsOf(codecId) / hasTrait(codecId, trait)
CR-->>ORM: trait list / boolean
ORM->>ORM: enable/disable comparison methods (type + runtime gating)
ORM->>DB: execute compiled query (uses contract from context)
DB-->>ORM: results
ORM-->>Dev: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
@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-sql-builder-new
@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: |
SevInf
changed the title
SevInf
force-pushed
the
worktree/codec-traits
branch
from
5dcaca6 to
d6f93e7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/3-extensions/sql-orm-client/src/model-accessor.ts (1)
222-235: Silently skippingnullshorthand whenisNullis unavailable may cause confusion.When a shorthand predicate contains
{ field: null }but the field's accessor lacksisNull(e.g., codec has no traits), the filter is silently skipped. This differs from theeqcase on line 229-233, which throws an error.Consider throwing a similar error for consistency:
♻️ Proposed fix for consistent error handling
if (value === null) { - if (fieldAccessor.isNull) { - exprs.push(fieldAccessor.isNull()); + if (!fieldAccessor.isNull) { + throw new Error( + `Shorthand filter on "${relatedModelName}.${fieldName}": field does not support null checks`, + ); } + exprs.push(fieldAccessor.isNull()); continue; }🤖 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 222 - 235, The current shorthand handling in model-accessor.ts silently skips null filters when a field's accessor lacks isNull; change this to throw a clear error like the eq case: inside the loop where value === null is checked, if fieldAccessor.isNull is falsy throw an Error referencing the relatedModelName and fieldName (similar wording to the eq branch) instead of silently continuing; otherwise keep pushing fieldAccessor.isNull() into exprs as before.packages/3-extensions/sql-orm-client/test/integration/helpers.ts (1)
22-32: Shallow context merge may cause inconsistency between contract and registries.When overriding
context.contractvia spread, theoperations,codecs, andtypesregistries remain bound to the originalbaseTestContractfromgetTestContext(). IfwithReturningCapability()modifies the contract in ways that affect codec lookups (e.g., different tables or codec IDs), the registries won't reflect those changes.For test purposes this may be acceptable if the returning-capable contract only adds capabilities without changing codec structure. If that's the intent, consider adding a brief comment to clarify.
🤖 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/helpers.ts` around lines 22 - 32, createReturningUsersCollection and createReturningPostsCollection currently do a shallow spread of getTestContext() which leaves registries (operations, codecs, types) bound to the original base contract; instead, obtain the base context via getTestContext(), set context.contract = withReturningCapability(getTestContract()), and then update the registries on that context to match the new contract (e.g., context.operations/context.codecs/context.types = contract.operations/... or replace the registries object if the contract exposes one) so codec lookups stay consistent with the modified contract; alternatively, if withReturningCapability only adds capabilities and you intend no registry change, add a clarifying comment to both functions (createReturningUsersCollection, createReturningPostsCollection) explaining the shallow merge is intentional.packages/2-sql/4-lanes/relational-core/src/ast/codec-types.ts (1)
367-375: RuntimeCodecTypesobject omitstraitsfield.The runtime object constructed here includes only
inputandoutput, butExtractCodecTypesnow defines atraitsfield per entry. SinceCodecTypesis used as a phantom type for compile-time type extraction and the actual runtime value is never accessed, this works correctly. However, if future code attempts to accesstraitsat runtime throughthis.CodecTypes, it would fail silently.Consider adding
traitsto the runtime object for consistency:for (const [, codecImpl] of Object.entries(this._codecs)) { const codecImplTyped = codecImpl as Codec<string>; codecTypes[codecImplTyped.id] = { input: undefined as unknown as CodecInput<typeof codecImplTyped>, output: undefined as unknown as CodecOutput<typeof codecImplTyped>, + traits: undefined as unknown as CodecTraits<typeof codecImplTyped>, }; }🤖 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/codec-types.ts` around lines 367 - 375, The runtime CodecTypes object currently builds entries with only input and output, but ExtractCodecTypes expects a traits field too; update the loop that builds codecTypes (iterating this._codecs and using codecImplTyped.id) to include a traits property alongside input and output (e.g., set traits to undefined casted to the appropriate type) so the runtime shape matches the compile-time ExtractCodecTypes for CodecTypes and avoids runtime missing-field surprises.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/2-sql/4-lanes/relational-core/src/ast/codec-types.ts`:
- Around line 367-375: The runtime CodecTypes object currently builds entries
with only input and output, but ExtractCodecTypes expects a traits field too;
update the loop that builds codecTypes (iterating this._codecs and using
codecImplTyped.id) to include a traits property alongside input and output
(e.g., set traits to undefined casted to the appropriate type) so the runtime
shape matches the compile-time ExtractCodecTypes for CodecTypes and avoids
runtime missing-field surprises.
In `@packages/3-extensions/sql-orm-client/src/model-accessor.ts`:
- Around line 222-235: The current shorthand handling in model-accessor.ts
silently skips null filters when a field's accessor lacks isNull; change this to
throw a clear error like the eq case: inside the loop where value === null is
checked, if fieldAccessor.isNull is falsy throw an Error referencing the
relatedModelName and fieldName (similar wording to the eq branch) instead of
silently continuing; otherwise keep pushing fieldAccessor.isNull() into exprs as
before.
In `@packages/3-extensions/sql-orm-client/test/integration/helpers.ts`:
- Around line 22-32: createReturningUsersCollection and
createReturningPostsCollection currently do a shallow spread of getTestContext()
which leaves registries (operations, codecs, types) bound to the original base
contract; instead, obtain the base context via getTestContext(), set
context.contract = withReturningCapability(getTestContract()), and then update
the registries on that context to match the new contract (e.g.,
context.operations/context.codecs/context.types = contract.operations/... or
replace the registries object if the contract exposes one) so codec lookups stay
consistent with the modified contract; alternatively, if withReturningCapability
only adds capabilities and you intend no registry change, add a clarifying
comment to both functions (createReturningUsersCollection,
createReturningPostsCollection) explaining the shallow merge is intentional.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 7fa8829d-17a2-4eab-9bb5-36df1d00aa3e
⛔ Files ignored due to path filters (1)
projects/codec-traits/plan.mdis excluded by!projects/**
📒 Files selected for processing (29)
.claude/settings.json.cursor/rules/use-correct-tools.mdcdocs/architecture docs/adrs/ADR 170 - Codec trait system.mdexamples/prisma-next-demo/src/orm-client/client.tspackages/2-sql/4-lanes/relational-core/src/ast/codec-types.tspackages/2-sql/4-lanes/relational-core/src/ast/sql-codecs.tspackages/2-sql/4-lanes/relational-core/test/ast/codec-types.test.tspackages/3-extensions/pgvector/src/core/codecs.tspackages/3-extensions/postgres/src/runtime/postgres.tspackages/3-extensions/sql-orm-client/src/collection.tspackages/3-extensions/sql-orm-client/src/grouped-collection.tspackages/3-extensions/sql-orm-client/src/model-accessor.tspackages/3-extensions/sql-orm-client/src/orm.tspackages/3-extensions/sql-orm-client/src/types.tspackages/3-extensions/sql-orm-client/test/collection-dispatch.test.tspackages/3-extensions/sql-orm-client/test/collection-fixtures.tspackages/3-extensions/sql-orm-client/test/filters.test.tspackages/3-extensions/sql-orm-client/test/generated-contract-types.test-d.tspackages/3-extensions/sql-orm-client/test/helpers.tspackages/3-extensions/sql-orm-client/test/include-cardinality.test-d.tspackages/3-extensions/sql-orm-client/test/integration/helpers.tspackages/3-extensions/sql-orm-client/test/integration/orm.test.tspackages/3-extensions/sql-orm-client/test/model-accessor.test.tspackages/3-extensions/sql-orm-client/test/orm.test.tspackages/3-extensions/sql-orm-client/test/orm.types.test-d.tspackages/3-extensions/sql-orm-client/test/repository.test.tspackages/3-extensions/sql-orm-client/test/sql-compilation/include.test.tspackages/3-extensions/sql-orm-client/test/sql-compilation/upsert.test.tspackages/3-targets/6-adapters/postgres/src/core/codecs.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/3-extensions/sql-orm-client/test/generated-contract-types.test-d.ts (1)
165-170: Make the mockExecutionContextcast explicit.
{} as ExecutionContext<...>hides the whole unsafe boundary. In test files, preferas unknown as ...or a minimal mock shape so context changes fail where the mock is created.Small cleanup
-const context = {} as ExecutionContext<GeneratedLikeContract>; +const context = {} as unknown as ExecutionContext<GeneratedLikeContract>;As per coding guidelines, "Use double casts (
as unknown as X) for mocks and dynamic proxies in tests instead of direct casts."🤖 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/generated-contract-types.test-d.ts` around lines 165 - 170, The test currently uses an unsafe direct cast for context (const context = {} as ExecutionContext<GeneratedLikeContract>); change this to an explicit double-cast or a minimal mock shape so the unsafe boundary is visible: replace the direct cast with either "{} as unknown as ExecutionContext<GeneratedLikeContract>" or construct a small object matching the ExecutionContext interface and cast that, updating the context variable used when creating PostCollection and Collection to use the new, explicit mock.docs/architecture docs/adrs/ADR 170 - Codec trait system.md (1)
301-307: Use a stable reference here instead of the Linear ticket.This ADR will outlive the rollout, so the Linear reference is likely to rot faster than a repo-stable PR or architecture doc. I'd replace it with a durable reference or drop it from the list.
Based on learnings, durable system documentation under
docs/should not reference transient project artifacts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/architecture` docs/adrs/ADR 170 - Codec trait system.md around lines 301 - 307, Replace the transient Linear ticket reference in the References section: locate the line "TML-2084 — Trait system for codecs (Linear)" in ADR 170 - Codec trait system.md and either replace it with a durable reference (e.g., a repo-stable PR number, a formal architecture/doc link under docs/, or an RFC/archival doc) or remove the entry entirely; ensure the new reference uses a persistent URL or doc identifier consistent with other entries like "ADR 030" and "ADR 114" so the ADR remains stable over time.
🤖 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/collection.ts`:
- Around line 171-176: The shorthand path builds whereArg via
shorthandToWhereExpr(this.ctx.context.contract, this.modelName, input) which
only passes the contract and thus lacks context.codecs; update the call so the
full ExecutionContext is passed (e.g., shorthandToWhereExpr(this.ctx.context,
this.modelName, input)) or instead route shorthand construction through
createModelAccessor(this.ctx.context, this.modelName) so both callback filters
and shorthand filters use the same context-aware logic; change usages in the
whereArg assignment and the reload-by-criterion code path to accept an
ExecutionContext parameter and use this.ctx.context rather than only contract so
the runtime equality-trait gating works consistently.
In `@packages/3-extensions/sql-orm-client/src/model-accessor.ts`:
- Around line 64-75: The function resolveFieldTraits currently returns undefined
when codecId is missing which lets the caller's truthiness guard treat it as a
hit; change resolveFieldTraits to return an empty array ([]) instead of
undefined when codecId is not found so unmapped fields remain locked down, and
apply the same fix to the similar trait-resolving function found in the nearby
block (the other resolve... function around lines 78-93) so both use [] for
missing codec metadata rather than undefined; update code paths that call
context.codecs.traitsOf(codecId) accordingly to still return the codec traits
when present.
---
Nitpick comments:
In `@docs/architecture` docs/adrs/ADR 170 - Codec trait system.md:
- Around line 301-307: Replace the transient Linear ticket reference in the
References section: locate the line "TML-2084 — Trait system for codecs
(Linear)" in ADR 170 - Codec trait system.md and either replace it with a
durable reference (e.g., a repo-stable PR number, a formal architecture/doc link
under docs/, or an RFC/archival doc) or remove the entry entirely; ensure the
new reference uses a persistent URL or doc identifier consistent with other
entries like "ADR 030" and "ADR 114" so the ADR remains stable over time.
In
`@packages/3-extensions/sql-orm-client/test/generated-contract-types.test-d.ts`:
- Around line 165-170: The test currently uses an unsafe direct cast for context
(const context = {} as ExecutionContext<GeneratedLikeContract>); change this to
an explicit double-cast or a minimal mock shape so the unsafe boundary is
visible: replace the direct cast with either "{} as unknown as
ExecutionContext<GeneratedLikeContract>" or construct a small object matching
the ExecutionContext interface and cast that, updating the context variable used
when creating PostCollection and Collection to use the new, explicit mock.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 2674d599-263c-4a38-866b-e783bf37d26b
⛔ Files ignored due to path filters (1)
projects/codec-traits/plan.mdis excluded by!projects/**
📒 Files selected for processing (24)
.claude/settings.json.cursor/rules/use-correct-tools.mdcdocs/architecture docs/adrs/ADR 170 - Codec trait system.mdexamples/prisma-next-demo/src/orm-client/client.tspackages/3-extensions/postgres/src/runtime/postgres.tspackages/3-extensions/sql-orm-client/src/collection.tspackages/3-extensions/sql-orm-client/src/grouped-collection.tspackages/3-extensions/sql-orm-client/src/model-accessor.tspackages/3-extensions/sql-orm-client/src/orm.tspackages/3-extensions/sql-orm-client/src/types.tspackages/3-extensions/sql-orm-client/test/collection-dispatch.test.tspackages/3-extensions/sql-orm-client/test/collection-fixtures.tspackages/3-extensions/sql-orm-client/test/filters.test.tspackages/3-extensions/sql-orm-client/test/generated-contract-types.test-d.tspackages/3-extensions/sql-orm-client/test/helpers.tspackages/3-extensions/sql-orm-client/test/include-cardinality.test-d.tspackages/3-extensions/sql-orm-client/test/integration/helpers.tspackages/3-extensions/sql-orm-client/test/integration/orm.test.tspackages/3-extensions/sql-orm-client/test/model-accessor.test.tspackages/3-extensions/sql-orm-client/test/orm.test.tspackages/3-extensions/sql-orm-client/test/orm.types.test-d.tspackages/3-extensions/sql-orm-client/test/repository.test.tspackages/3-extensions/sql-orm-client/test/sql-compilation/include.test.tspackages/3-extensions/sql-orm-client/test/sql-compilation/upsert.test.ts
✅ Files skipped from review due to trivial changes (3)
- packages/3-extensions/sql-orm-client/test/repository.test.ts
- .claude/settings.json
- packages/3-extensions/sql-orm-client/test/collection-dispatch.test.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- .cursor/rules/use-correct-tools.mdc
- examples/prisma-next-demo/src/orm-client/client.ts
- packages/3-extensions/postgres/src/runtime/postgres.ts
- packages/3-extensions/sql-orm-client/test/sql-compilation/include.test.ts
- packages/3-extensions/sql-orm-client/test/helpers.ts
- packages/3-extensions/sql-orm-client/test/orm.types.test-d.ts
- packages/3-extensions/sql-orm-client/test/orm.test.ts
- packages/3-extensions/sql-orm-client/test/collection-fixtures.ts
- packages/3-extensions/sql-orm-client/src/orm.ts
- packages/3-extensions/sql-orm-client/test/include-cardinality.test-d.ts
- packages/3-extensions/sql-orm-client/test/sql-compilation/upsert.test.ts
- packages/3-extensions/sql-orm-client/test/model-accessor.test.ts
- packages/3-extensions/sql-orm-client/test/integration/helpers.ts
- packages/3-extensions/sql-orm-client/src/types.ts
- packages/3-extensions/sql-orm-client/test/filters.test.ts
| function resolveFieldTraits( | ||
| contract: SqlContract<SqlStorage>, | ||
| tableName: string, | ||
| columnName: string, | ||
| context: ExecutionContext, | ||
| ): readonly string[] | undefined { | ||
| const tables = contract.storage?.tables as | ||
| | Record<string, { columns?: Record<string, { codecId?: string }> }> | ||
| | undefined; | ||
| const codecId = tables?.[tableName]?.columns?.[columnName]?.codecId; | ||
| if (!codecId) return undefined; // Column not in storage — can't resolve traits | ||
| return context.codecs.traitsOf(codecId); |
There was a problem hiding this comment.
Fail closed when trait lookup misses.
If codecId can't be resolved, resolveFieldTraits() returns undefined, and the traits && ... guard ends up attaching every method. That re-exposes gt/like/asc/etc. on unmapped fields instead of only the zero-trait methods. Return [] here, or remove the truthiness check, so missing metadata stays locked down.
Suggested fix
function resolveFieldTraits(
contract: SqlContract<SqlStorage>,
tableName: string,
columnName: string,
context: ExecutionContext,
-): readonly string[] | undefined {
+): readonly string[] {
const tables = contract.storage?.tables as
| Record<string, { columns?: Record<string, { codecId?: string }> }>
| undefined;
const codecId = tables?.[tableName]?.columns?.[columnName]?.codecId;
- if (!codecId) return undefined; // Column not in storage — can't resolve traits
+ if (!codecId) return [];
return context.codecs.traitsOf(codecId);
}
function createScalarFieldAccessor(
tableName: string,
columnName: string,
- traits: readonly string[] | undefined,
+ traits: readonly string[],
): Partial<ComparisonMethodFns<unknown>> {
const column: ColumnRef = { kind: 'col', table: tableName, column: columnName };
const methods: Record<string, unknown> = {};
for (const [name, meta] of Object.entries(COMPARISON_METHODS_META)) {
- if (traits && meta.traits.length > 0 && !meta.traits.every((t) => traits.includes(t))) {
+ if (meta.traits.length > 0 && !meta.traits.every((t) => traits.includes(t))) {
continue;
}
methods[name] = meta.create(column);
}Also applies to: 78-93
🤖 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 64 -
75, The function resolveFieldTraits currently returns undefined when codecId is
missing which lets the caller's truthiness guard treat it as a hit; change
resolveFieldTraits to return an empty array ([]) instead of undefined when
codecId is not found so unmapped fields remain locked down, and apply the same
fix to the similar trait-resolving function found in the nearby block (the other
resolve... function around lines 78-93) so both use [] for missing codec
metadata rather than undefined; update code paths that call
context.codecs.traitsOf(codecId) accordingly to still return the codec traits
when present.
wmadden
left a comment
wmadden
left a comment
There was a problem hiding this comment.
@SevInf good work on the traits system 👏:skin-tone-2:
I’m eagerly approving but you have open comments from me and CodeRabbit, please address them before merging. my only major concern is the COMPARISON_METHODS_META is defined in the sql-orm-client package but it’s metadata which should be inherited by all query surfaces
wmadden
left a comment
wmadden
left a comment
There was a problem hiding this comment.
Code Review — Codec Trait System
Introduces a codec trait system with 5 traits (equality, order, numeric, textual, boolean), wires traits through the Codec interface, CodecRegistry, CodecDefBuilder, and ExtractCodecTypes, declares traits on all Postgres and core SQL codecs, and gates ORM comparison methods at both type level and runtime. Replaces NumericNativeType with trait-based NumericFieldNames. The core trait infrastructure and ORM gating are well-implemented, but the knowledge of which operators require which traits is defined inside the ORM package rather than in the shared core — meaning other query surfaces can't reuse it.
What Looks Solid
-
ComparisonMethods<T, Traits>conditional mapped type: The[ComparisonMethodsMeta[K]['traits'][number]] extends [Traits]check is a clean, non-distributing conditional that correctly handles empty traits, single trait requirements, and multi-trait requirements. -
ExecutionContextconsolidation: Moving from separatecontract+ implicit codec access to a singleExecutionContextparameter acrossOrmOptions,CollectionContext, andcreateModelAccessoris a clean simplification. -
Codec declarations are thorough: All Postgres codecs have appropriate trait assignments. The
json(traits: []) vsjsonb(traits: ['equality']) distinction correctly models Postgres semantics. -
Test coverage is strong: Type-level tests cover positive and negative cases for bool, jsonb, text, int4. Runtime tests verify method presence/absence based on trait registry.
-
NumericNativeTypecleanly deleted: The 15-entry Postgres-specific union and its helper types are replaced by a singleNumericFieldNamestype using'numeric' extends FieldTraits<...>.
Findings
| ID | Severity | Summary |
|---|---|---|
| F01 | Blocking | Operator trait requirements are defined in the ORM, not in the shared core |
| F02 | Blocking | traits: [] is indistinguishable from "no traits" at the type level |
| F03 | Non-blocking | Permissive runtime fallback when traits are unknown |
| F04 | Non-blocking | No test for shorthand-filter error on equality-less codecs |
| F05 | Non-blocking | isNull guard in shorthand toRelationWhereExpr is asymmetric |
| F06 | Nit | ctx.context.contract double-nesting reads awkwardly |
| F07 | Nit | Explicit type annotations on Postgres codec encode/decode are inconsistent |
| F08 | Nit | MethodFactory type uses never[] for rest args |
See inline comments for details.
Acceptance-Criteria Traceability
| AC | Criterion | Status |
|---|---|---|
| AC1 | CodecTrait type with 5 traits |
✅ |
| AC2 | Codec interface has optional traits |
✅ |
| AC3 | codec() factory preserves traits |
✅ |
| AC4 | aliasCodec() forwards traits |
✅ |
| AC5 | hasTrait() works |
✅ |
| AC6 | traitsOf() works |
✅ |
| AC7 | ExtractCodecTypes carries traits |
✅ |
| AC8 | All Postgres codecs have traits | ✅ |
| AC9 | pgvector has ['equality'] |
✅ |
| AC10 | NumericNativeType deleted |
✅ |
| AC11 | sum/avg gated by numeric |
✅ |
| AC12 | gt/lt gated by order |
✅ |
| AC13 | like/ilike gated by textual |
✅ |
| AC14 | bool rejects gt, like, asc |
✅ |
| AC15 | jsonb rejects gt, like, asc |
✅ |
| AC16 | Runtime method gating | ✅ |
| AC17 | Shorthand throws without equality | |
| AC18 | All existing tests pass | ✅ Assumed (CI) |
| AC19 | Import layering valid | ✅ lint:deps excludes test files; source layering valid |
| // --------------------------------------------------------------------------- | ||
| // COMPARISON_METHODS_META — single source of truth for traits + factories | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| function literal(value: unknown): LiteralExpr { | ||
| return { kind: 'literal', value }; | ||
| } | ||
|
|
||
| function listLiteral(values: readonly unknown[]) { | ||
| return { kind: 'listLiteral' as const, values: values.map(literal) }; | ||
| } | ||
|
|
||
| function bin(op: BinaryExpr['op'], column: ColumnRef, right: BinaryExpr['right']): BinaryExpr { | ||
| return { kind: 'bin', op, left: column, right }; | ||
| } | ||
|
|
||
| type MethodFactory = (column: ColumnRef) => (...args: never[]) => unknown; | ||
|
|
||
| type ComparisonMethodMeta = { | ||
| readonly traits: readonly CodecTrait[]; | ||
| readonly create: MethodFactory; | ||
| }; | ||
|
|
||
| /** | ||
| * Declares trait requirements and runtime factory for each comparison method. | ||
| * | ||
| * - `traits: []` means "no trait required" — always available | ||
| * - Multi-trait: `traits: ['equality', 'order']` means BOTH traits are required | ||
| */ | ||
| export const COMPARISON_METHODS_META = { | ||
| eq: { | ||
| traits: ['equality'], | ||
| create: (column) => (value: unknown) => bin('eq', column, literal(value)), | ||
| }, | ||
| neq: { | ||
| traits: ['equality'], | ||
| create: (column) => (value: unknown) => bin('neq', column, literal(value)), | ||
| }, | ||
| in: { | ||
| traits: ['equality'], | ||
| create: (column) => (values: readonly unknown[]) => bin('in', column, listLiteral(values)), | ||
| }, | ||
| notIn: { | ||
| traits: ['equality'], | ||
| create: (column) => (values: readonly unknown[]) => bin('notIn', column, listLiteral(values)), | ||
| }, | ||
| gt: { | ||
| traits: ['order'], | ||
| create: (column) => (value: unknown) => bin('gt', column, literal(value)), | ||
| }, | ||
| lt: { | ||
| traits: ['order'], | ||
| create: (column) => (value: unknown) => bin('lt', column, literal(value)), | ||
| }, | ||
| gte: { | ||
| traits: ['order'], | ||
| create: (column) => (value: unknown) => bin('gte', column, literal(value)), | ||
| }, | ||
| lte: { | ||
| traits: ['order'], | ||
| create: (column) => (value: unknown) => bin('lte', column, literal(value)), | ||
| }, | ||
| like: { | ||
| traits: ['textual'], | ||
| create: (column) => (pattern: string) => bin('like', column, literal(pattern)), | ||
| }, | ||
| ilike: { | ||
| traits: ['textual'], | ||
| create: (column) => (pattern: string) => bin('ilike', column, literal(pattern)), | ||
| }, | ||
| asc: { | ||
| traits: ['order'], | ||
| create: (column) => () => ({ column: column.column, direction: 'asc' as const }), | ||
| }, | ||
| desc: { | ||
| traits: ['order'], | ||
| create: (column) => () => ({ column: column.column, direction: 'desc' as const }), | ||
| }, | ||
| isNull: { | ||
| traits: [], | ||
| create: (column) => () => ({ kind: 'nullCheck' as const, expr: column, isNull: true }), | ||
| }, | ||
| isNotNull: { | ||
| traits: [], | ||
| create: (column) => () => ({ kind: 'nullCheck' as const, expr: column, isNull: false }), | ||
| }, | ||
| } as const satisfies Record<keyof ComparisonMethodFns<unknown>, ComparisonMethodMeta>; | ||
|
|
||
| type ComparisonMethodsMeta = typeof COMPARISON_METHODS_META; |
There was a problem hiding this comment.
F01 (Blocking): Operator trait requirements are defined in the ORM, not in the shared core
The mapping from operators to their required traits (e.g., gt requires order, like requires textual) is defined inside COMPARISON_METHODS_META in the sql-orm-client package. This is the wrong layer. This mapping is universal SQL knowledge — it applies to every query surface, not just the ORM.
Because the mapping lives in a leaf extension package, other query surfaces cannot use it:
- The SQL lane (
relational-core/src/schema.ts) hardcodeseq,neq,gt,lt,gte,lte,asc,descon every column unconditionally — no trait gating, andlike/ilikearen't even present. - A new query surface (there is one under active development) would have to either import from
sql-orm-client(wrong dependency direction), duplicate the mapping (will drift), or skip trait gating entirely.
The problem is compounded because COMPARISON_METHODS_META bundles the trait requirements together with ORM-specific AST factory functions into a single object. This makes it impossible to import just the trait requirements without also pulling in the ORM's runtime code.
Suggestion: Move the operator-to-trait mapping to relational-core as a simple data structure (e.g., Record<OperatorName, readonly CodecTrait[]>), next to CodecTrait and BinaryOp. Each query surface imports the mapping and combines it with its own method construction. The ORM can still have its COMPARISON_METHODS_META, but it should compose the shared mapping with its factories rather than being the sole owner of both.
There was a problem hiding this comment.
I get the point and I agree in principle, however, COMPARISON_METHODS_META has very ORM-oriented format in practice — the values it is returning are pre-existing ORM-internal intermediate representations, not SQL AST.
relational-core would need something more generic (structure similar to the one sql-builder's Functions<QC> is what I am thinking of).
However, I'd argue this should be done in a separate follow up task: hardcoded list of comparison operation existed in ORM before this PR, I've just adapted it to use traits rather than accept everything.
There was a problem hiding this comment.
Agreed - create a Linear ticket to share this operation metadata between both ORM Client and SQL Query Builder, and defer it til later
| if (!fieldAccessor.eq) { | ||
| throw new Error( | ||
| `Shorthand filter on "${relatedModelName}.${fieldName}": field does not support equality comparisons`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
F04 (Non-blocking): No test for shorthand-filter error on equality-less codecs
toRelationWhereExpr throws "field does not support equality comparisons" when a shorthand filter uses a field whose codec lacks the equality trait. This runtime error path has no unit test exercising it. Since it's a user-facing error message, it should have test coverage to prevent regressions.
Suggestion: Add a test in model-accessor.test.ts that creates a registry with a codec without equality (e.g., traits: ['order']), builds a model accessor, and verifies the shorthand filter throws with the expected message.
| if (value === null) { | ||
| exprs.push(fieldAccessor.isNull()); | ||
| if (fieldAccessor.isNull) { | ||
| exprs.push(fieldAccessor.isNull()); | ||
| } | ||
| continue; | ||
| } |
There was a problem hiding this comment.
F05 (Non-blocking): isNull guard in shorthand toRelationWhereExpr is asymmetric
For null values, the code checks if (fieldAccessor.isNull) and silently skips if missing. For non-null values, it throws if fieldAccessor.eq is missing. The asymmetry is reasonable (isNull has traits: [] so it should always be present), but the silent skip means a bug in trait gating that accidentally removes isNull would cause silent predicate loss rather than an error.
Suggestion: Consider adding a defensive throw (throw new Error(...)) for the isNull case too, since isNull should never be absent.
wmadden
left a comment
wmadden
left a comment
There was a problem hiding this comment.
Preemptively approving - please address the existing comments, then merge. Don't wait for a re-review 👍🏻
- Fail-closed when traits are unknown: resolveFieldTraits returns [] instead of undefined so unmapped columns only get isNull/isNotNull - Defensive throw when isNull is unexpectedly missing in shorthand filters - Add MethodFactory type comment explaining never[] intent - Reduce ctx.context.contract nesting via private contract field - Add traits to runtime CodecTypes object for consistency - Add test for shorthand filter error on equality-less codecs - Double-cast mock ExecutionContext in type test - Add clarifying comment for shallow context merge in integration helpers - Replace Linear ticket reference with PR #247 in ADR 170 - Fix rebase artifacts: update remaining test call sites Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
SevInf
force-pushed
the
worktree/codec-traits
branch
from
d6f93e7 to
457d2b1
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/model-accessor.ts (1)
50-63:⚠️ Potential issue | 🟠 MajorUnknown shorthand keys still slip through the trait gate.
accessor[fieldName]is effectively never falsy here because the Proxy manufactures either a relation accessor or a scalar accessor for any string key. That means a typo like{ titlle: null }still generatesIS NULLagainst a non-existent column via the zero-trait methods, while{ titlle: 1 }throws the misleading equality-trait error instead of an unknown-field error. Please validatefieldNameagainst the model/column mapping before building SQL.Also applies to: 83-98, 197-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/model-accessor.ts` around lines 50 - 63, The Proxy get in ModelAccessor currently returns a relation or scalar accessor for any string key, allowing typos to generate misleading SQL; change the logic in the Proxy get (and the analogous proxy blocks at the other affected spots) to first validate that prop exists in either tableRelations or fieldToColumn (or its canonicalized column via fieldToColumn[prop] ?? prop) and if it does not, throw an explicit unknown-field error that mentions the modelName/tableName and the invalid field; only when the field is validated should you call createRelationFilterAccessor, resolveFieldTraits, or createScalarFieldAccessor to build the accessor.
🤖 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/architecture` docs/adrs/ADR 170 - Codec trait system.md:
- Around line 41-57: Update the ADR examples to match the current shipped types:
change the Codec generic signature shown (the one labeled Codec<Id, TWire, TJs,
TParams, THelper>) to the current signature used in code, and update the
CodecTypes example referenced near CodecTypes / ExtractCodecTypes to include the
emitted input field that ExtractCodecTypes now produces; specifically locate the
example usages of the Codec type and the CodecTypes example and add the missing
input property and any other current generic parameters so the snippets are
copy/pasteable against the real API.
In `@packages/2-sql/4-lanes/relational-core/src/ast/codec-types.ts`:
- Around line 220-221: The current implementation returns and stores the
caller's codec traits array by reference (via this._byId), allowing external
mutation; change registration to store an immutable copy (e.g.,
Array.from(traits) and Object.freeze) when inserting into this._byId and change
traitsOf(codecId: string) to return a defensive copy (e.g., Array.from(...) )
instead of the stored array so consumers cannot mutate internal metadata; update
the codec registration function that writes into this._byId and the traitsOf
method to perform these defensive copy/freeze operations referencing _byId and
traitsOf by name.
---
Outside diff comments:
In `@packages/3-extensions/sql-orm-client/src/model-accessor.ts`:
- Around line 50-63: The Proxy get in ModelAccessor currently returns a relation
or scalar accessor for any string key, allowing typos to generate misleading
SQL; change the logic in the Proxy get (and the analogous proxy blocks at the
other affected spots) to first validate that prop exists in either
tableRelations or fieldToColumn (or its canonicalized column via
fieldToColumn[prop] ?? prop) and if it does not, throw an explicit unknown-field
error that mentions the modelName/tableName and the invalid field; only when the
field is validated should you call createRelationFilterAccessor,
resolveFieldTraits, or createScalarFieldAccessor to build the accessor.
🪄 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: 537866d2-e936-41a4-a4b3-6b284b3c48cd
📒 Files selected for processing (20)
.claude/settings.json.cursor/rules/use-correct-tools.mdcdocs/architecture docs/adrs/ADR 170 - Codec trait system.mdexamples/prisma-next-demo/src/orm-client/client.tspackages/2-sql/4-lanes/relational-core/src/ast/codec-types.tspackages/2-sql/4-lanes/relational-core/src/ast/sql-codecs.tspackages/2-sql/4-lanes/relational-core/test/ast/codec-types.test.tspackages/3-extensions/pgvector/src/core/codecs.tspackages/3-extensions/postgres/src/runtime/postgres.tspackages/3-extensions/sql-orm-client/src/collection.tspackages/3-extensions/sql-orm-client/src/grouped-collection.tspackages/3-extensions/sql-orm-client/src/model-accessor.tspackages/3-extensions/sql-orm-client/src/orm.tspackages/3-extensions/sql-orm-client/src/types.tspackages/3-extensions/sql-orm-client/test/collection-dispatch.test.tspackages/3-extensions/sql-orm-client/test/collection-fixtures.tspackages/3-extensions/sql-orm-client/test/filters.test.tspackages/3-extensions/sql-orm-client/test/generated-contract-types.test-d.tspackages/3-extensions/sql-orm-client/test/helpers.tspackages/3-extensions/sql-orm-client/test/include-cardinality.test-d.ts
✅ Files skipped from review due to trivial changes (2)
- .claude/settings.json
- .cursor/rules/use-correct-tools.mdc
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/3-extensions/sql-orm-client/test/filters.test.ts
- packages/3-extensions/sql-orm-client/test/include-cardinality.test-d.ts
- examples/prisma-next-demo/src/orm-client/client.ts
- packages/2-sql/4-lanes/relational-core/src/ast/sql-codecs.ts
- packages/3-extensions/postgres/src/runtime/postgres.ts
- packages/3-extensions/sql-orm-client/src/orm.ts
- packages/3-extensions/sql-orm-client/src/collection.ts
- packages/2-sql/4-lanes/relational-core/test/ast/codec-types.test.ts
- packages/3-extensions/sql-orm-client/src/grouped-collection.ts
- packages/3-extensions/sql-orm-client/test/generated-contract-types.test-d.ts
- packages/3-extensions/sql-orm-client/test/collection-fixtures.ts
- packages/3-extensions/pgvector/src/core/codecs.ts
- packages/3-extensions/sql-orm-client/src/types.ts
| traitsOf(codecId: string): readonly CodecTrait[] { | ||
| return this._byId.get(codecId)?.traits ?? []; |
There was a problem hiding this comment.
Keep trait metadata immutable after registration.
Line 268 stores the caller's traits array by reference, and Line 221 returns that same array back out of the registry. In plain JS, mutating either reference after registration changes which operators the ORM exposes at runtime. Freeze/copy the array on write, or return a defensive copy here.
Also applies to: 260-268
🤖 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/codec-types.ts` around lines
220 - 221, The current implementation returns and stores the caller's codec
traits array by reference (via this._byId), allowing external mutation; change
registration to store an immutable copy (e.g., Array.from(traits) and
Object.freeze) when inserting into this._byId and change traitsOf(codecId:
string) to return a defensive copy (e.g., Array.from(...) ) instead of the
stored array so consumers cannot mutate internal metadata; update the codec
registration function that writes into this._byId and the traitsOf method to
perform these defensive copy/freeze operations referencing _byId and traitsOf by
name.
- Fail-closed when traits are unknown: resolveFieldTraits returns [] instead of undefined so unmapped columns only get isNull/isNotNull - Defensive throw when isNull is unexpectedly missing in shorthand filters - Add MethodFactory type comment explaining never[] intent - Reduce ctx.context.contract nesting via private contract field - Add traits to runtime CodecTypes object for consistency - Add test for shorthand filter error on equality-less codecs - Double-cast mock ExecutionContext in type test - Add clarifying comment for shallow context merge in integration helpers - Replace Linear ticket reference with PR #247 in ADR 170 - Fix rebase artifacts: update remaining test call sites Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
SevInf
force-pushed
the
worktree/codec-traits
branch
from
095c04f to
a1fafd4
Compare
Introduce CodecTrait type ('equality' | 'order' | 'boolean' | 'numeric' | 'textual')
as a generic parameter on the Codec interface. Codecs declare their traits at
registration time, enabling DSL surfaces to gate operators by type semantics
rather than hardcoded codec IDs or native type names.
- Add TTraits generic (2nd param) to Codec interface, codec() factory, aliasCodec()
- Add hasTrait()/traitsOf() to CodecRegistry
- Declare traits on all core SQL, Postgres adapter, and pgvector codecs
- ExtractCodecTypes propagates traits into CodecTypes for contract.d.ts emission
- ORM trait-gating deferred to follow-up
- Fail-closed when traits are unknown: resolveFieldTraits returns [] instead of undefined so unmapped columns only get isNull/isNotNull - Defensive throw when isNull is unexpectedly missing in shorthand filters - Add MethodFactory type comment explaining never[] intent - Reduce ctx.context.contract nesting via private contract field - Add traits to runtime CodecTypes object for consistency - Add test for shorthand filter error on equality-less codecs - Double-cast mock ExecutionContext in type test - Add clarifying comment for shallow context merge in integration helpers - Replace Linear ticket reference with PR #247 in ADR 170 - Fix rebase artifacts: update remaining test call sites Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
shorthandToWhereExpr now accepts ExecutionContext and checks the
codec equality trait before building eq() filters. This ensures
shorthand .where({ field: val }) and callback .where(m => m.field.eq(val))
share the same fail-closed trait gating.
Threads ExecutionContext through mutation-executor functions that
call shorthandToWhereExpr (createGraph, updateFirstGraph,
applyParentOwnedMutation, applyChildOwnedMutation, findRowByCriterion).
…rray - Update ADR 170 Codec signature to include TTraits type parameter - Add missing input field to CodecTypes example in ADR - Freeze traits array in codec() factory to prevent mutation after registration
SevInf
force-pushed
the
worktree/codec-traits
branch
from
a1fafd4 to
f8423e8
Compare
2 participants
Walkthrough: Codec Trait System + ORM Trait-Based Gating
Key snippet
Before
After
Sources
origin/main...HEAD(2 commits: codec trait system + ORM trait gating)Intent
Wire semantic codec traits (
equality,order,numeric,textual,boolean) into the ORM's type system and runtime so that comparison methods and aggregate functions are gated by what a type actually supports, rather than hardcoded native type lists. Aboolfield getseq/neqbut notgt/like; ajsonbfield getseqbut notgt/asc;isNull/isNotNullare always available.Change map
Implementation:
ComparisonMethodFns,ComparisonMethods,COMPARISON_METHODS_META,FieldTraits,NumericFieldNamescreateModelAccessor+createScalarFieldAccessorruntime gatingOrmOptionsnow takesExecutionContextCodecTraittypeExecutionContextto ORMTests (evidence):
views.like('%')rejected,views.gt(5)accepted,agg.sum('title')rejectedgt,like,asctraitsOf/hasTraitThe story
Codecs declare semantic traits. Each codec gains a
traitsarray (['equality', 'order', 'numeric']for int4,['equality']for jsonb,[]for json). TheCodecinterface,CodecRegistry, andCodecDefBuilderare extended to carry and propagate trait information.COMPARISON_METHODS_METAbecomes the single source of truth. Each of the 14 comparison methods declares its required traits and its runtime AST factory in one place. Adding a method or changing its trait requirement is a single edit.Type-level gating via
ComparisonMethods<T, Traits>. A mapped type iteratesCOMPARISON_METHODS_METAkeys, filtering to only those whose required traits are all present in the field'sTraitsunion (resolved from contractCodecTypesviaFieldTraits).isNull/isNotNull(traits:[]) are always present.Runtime gating via codec registry.
createModelAccessorreceives theExecutionContext, looks up each field's codec traits viacontext.codecs.traitsOf(codecId), and only creates methods that pass the trait check. Shorthand filters throw if a field lacksequality.NumericFieldNamesuses thenumerictrait instead of a hardcodedNumericNativeTypeunion of 15 Postgres-specific strings.ExecutionContextreplaces separatecontract+codecsparameters.CollectionContextandOrmOptionsnow takeExecutionContext<TContract>which carries contract, codecs, operations, and types. The redundantcontractparameter is removed.Behavior changes & evidence
Comparison methods are type-gated by codec traits:
post.views.like('%')is a compile error (int4 has notextualtrait);user.active.gt(true)is a compile error (bool has noordertrait);isNull()/isNotNull()always available.Comparison methods are runtime-gated by codec traits: accessor proxy only creates methods whose codec traits match. Fields without
equalitytrait throw on shorthand filter use.Numeric aggregate gating uses
numerictrait:sum('title')is a compile error becausetexthas nonumerictrait. Replaces 15-entryNumericNativeTypeunion.All Postgres codecs declare traits: text →
equality|order|textual, int4 →equality|order|numeric, bool →equality|boolean, json → (none), jsonb →equality, timestamps →equality|order, etc.ExecutionContextflows through the ORM:OrmOptionsandCollectionContextacceptExecutionContext<TContract>instead of separatecontract+codecs. The postgres runtime passescontextdirectly.Compatibility / migration / risk
ComparisonMethods<T>now requires a second type argumentTraits. All direct usages must provide it.OrmOptionsandCollectionContextno longer accept a separatecontractparameter; passExecutionContextviacontextinstead.createModelAccessorsignature changed from(contract, modelName)to(context, modelName).isNull/isNotNullat both type and runtime level. Contracts must provideCodecTypeswithtraitsto get comparison methods.NumericNativeType,IsNumericStorageColumn,StrictNumericFieldNames,NumericFieldNamesFromRowType— replaced by trait-basedNumericFieldNames.Follow-ups / open questions
HavingComparisonMethodshardcodes'equality' | 'order'since aggregate results are always numbers — may need revisiting if non-numeric aggregates are added.Non-goals / intentionally out of scope
equality,order,boolean,numeric,textual).contract.d.tswas done in M3.Summary by CodeRabbit
New Features
Documentation
Chores