feat(orm): add result plugin extension point#2442
Open
genu wants to merge 19 commits intozenstackhq:devfrom
Open
feat(orm): add result plugin extension point#2442genu wants to merge 19 commits intozenstackhq:devfrom
genu wants to merge 19 commits intozenstackhq:devfrom
Conversation
Implement a new `result` extension point that allows plugins to declare computed fields on query results with automatic type safety and select/omit awareness. Changes: - Add ExtResultFieldDef and ExtResultBase types to plugin.ts - Add ExtResult generic parameter to RuntimePlugin, AnyPlugin, definePlugin - Thread ExtResult through ClientContract and all CRUD return types - Add ExtractExtResult, ExtResultSelectOmitFields, SelectAwareExtResult type helpers - Implement recursive runtime computation in client-impl.ts with needs injection/stripping - Add ext result fields to Zod validation for select/omit schemas - Support nested relation ext result computation (both runtime and types) - Add 26 comprehensive tests covering single-model and nested relation scenarios Features: - Type-safe computed fields on query results - Select/omit-aware types (only include selected fields in result type) - Automatic field dependency (needs) injection and stripping - Multi-plugin composition support - Full support for nested relations (include/select with ext results) - Works with $transaction, $setAuth, $setOptions, etc. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAdds plugin-driven "ext result" computed fields: types and plugin APIs for declaring ext fields, runtime arg preparation/injection for their dependencies, integration into CRUD execution, post-processing to compute ext values (including nested relations), Zod schema exposure, and comprehensive e2e tests and fixtures. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
When a user omits a field that is a `needs` dependency of an active ext result field, the computed field was silently missing from results. Ensure needs dependencies are un-omitted for the DB query and stripped from the final result, mirroring the existing `select` path behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/e2e/orm/plugin-infra/ext-result.test.ts (1)
601-603: Avoid hard-coded foreign keys in relation setup.These cases use
authorId: 1directly. Prefer the ID from the created user to keep tests independent from sequence/reset behavior.Suggested patch pattern
-await extDb.user.create({ data: { name: 'Alice' } }); -await extDb.post.create({ data: { title: 'Hello', authorId: 1 } }); +const user = await extDb.user.create({ data: { name: 'Alice' } }); +await extDb.post.create({ data: { title: 'Hello', authorId: user.id } });Also applies to: 624-626, 655-657, 680-682, 705-707, 734-736
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/plugin-infra/ext-result.test.ts` around lines 601 - 603, The test uses hard-coded foreign keys (authorId: 1) when creating posts; change each extDb.user.create call to capture the returned user (e.g., const user = await extDb.user.create(...)) and use that user's id in subsequent extDb.post.create calls (authorId: user.id) so tests don't rely on sequence/reset behavior; update all occurrences that currently pass authorId: 1 (the blocks around extDb.user.create / extDb.post.create) 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/orm/src/client/crud-types.ts`:
- Around line 2458-2469: SelectAwareExtResult currently treats any key present
in select/omit as active regardless of its boolean value; update the key
extraction to only include keys whose value is truthy (true). Replace the
Extract<keyof S & string, ...> expression with a conditional-key expression like
({ [K in keyof S & string]: S[K] extends true ? K : never }[keyof S & string]) &
keyof ExtractExtResult<ExtResult, Model>, and similarly for omit use ({ [K in
keyof O & string]: O[K] extends true ? K : never }[keyof O & string]) & keyof
ExtractExtResult<ExtResult, Model> so SelectAwareExtResult (and the omit branch)
matches the runtime truthiness checks; refer to SelectAwareExtResult and
ExtractExtResult to locate and update the two key-extraction spots.
In `@tests/e2e/orm/plugin-infra/ext-result.test.ts`:
- Around line 36-40: The test relies on array index ordering from
extDb.user.findMany() which is non-deterministic; update the call in the test to
request a deterministic order (e.g., add an orderBy clause such as orderBy: {
id: 'asc' } or createdAt: 'asc') so the assertions expecting users[0].greeting
and users[1].greeting are stable, or alternatively fetch and sort the returned
array by a stable key before asserting; locate the call to extDb.user.findMany
in the test and apply the orderBy or explicit sort to produce deterministic
ordering.
---
Nitpick comments:
In `@tests/e2e/orm/plugin-infra/ext-result.test.ts`:
- Around line 601-603: The test uses hard-coded foreign keys (authorId: 1) when
creating posts; change each extDb.user.create call to capture the returned user
(e.g., const user = await extDb.user.create(...)) and use that user's id in
subsequent extDb.post.create calls (authorId: user.id) so tests don't rely on
sequence/reset behavior; update all occurrences that currently pass authorId: 1
(the blocks around extDb.user.create / extDb.post.create) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 340e2411-a2f8-4e9b-9c37-c0c7f30db3c4
📒 Files selected for processing (8)
packages/orm/src/client/client-impl.tspackages/orm/src/client/contract.tspackages/orm/src/client/crud-types.tspackages/orm/src/client/plugin.tspackages/orm/src/client/zod/factory.tspackages/plugins/policy/src/plugin.tstests/e2e/orm/plugin-infra/ext-result.test.tstests/e2e/orm/plugin-infra/ext-result/schema.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/e2e/orm/plugin-infra/ext-result.test.ts (1)
36-39:⚠️ Potential issue | 🟡 MinorAdd deterministic ordering for index-based assertions.
At Line 36,
findMany()is used withoutorderBy, but Lines 38-39 assertusers[0]andusers[1]. This can be flaky depending on DB return order (same pattern appears elsewhere in this file).Suggested patch pattern
-const users = await extDb.user.findMany(); +const users = await extDb.user.findMany({ orderBy: { id: 'asc' } });#!/bin/bash # Verify index-based assertions that may need explicit ordering in this file rg -n -C2 'findMany\(|createManyAndReturn\(|\[[0-9]+\]!\.' tests/e2e/orm/plugin-infra/ext-result.test.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/plugin-infra/ext-result.test.ts` around lines 36 - 39, The test uses extDb.user.findMany() and then asserts users[0] and users[1] which is flaky because DB return order is not guaranteed; modify the test to call extDb.user.findMany({ orderBy: { id: 'asc' } }) (or another stable column like createdAt/name) before making index-based assertions, and apply the same explicit orderBy change to any other occurrences in this file that assert by array index (e.g., places referencing users[N] or similar results from createManyAndReturn/findMany).packages/orm/src/client/client-impl.ts (1)
569-571:⚠️ Potential issue | 🟠 MajorGate ext-result processing to row-returning operations only.
At Line 569 and Line 586, ext-result logic is enabled for all
postProcessoperations;groupBystill passespostProcess = trueat Line 799. This can apply model ext fields to aggregate rows whose runtime/type shape doesn’t model ext results.Suggested fix
+ const shouldApplyExtResult = + hasAnyExtResult && + [ + 'findUnique', + 'findFirst', + 'findMany', + 'create', + 'createManyAndReturn', + 'update', + 'updateManyAndReturn', + 'upsert', + 'delete', + ].includes(operation); const processedArgs = - postProcess && hasAnyExtResult + shouldApplyExtResult ? prepareArgsForExtResult(_args, model, schema, plugins) : _args; ... - if (result && postProcess && hasAnyExtResult) { + if (result && shouldApplyExtResult) { result = applyExtResult(result, model, _args, schema, plugins); }Also applies to: 586-588, 793-800
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/client-impl.ts` around lines 569 - 571, postProcess currently gates ext-result handling for all operations, which incorrectly applies model extension fields to non-row-returning ops like groupBy; update the logic around prepareArgsForExtResult (used where postProcess && hasAnyExtResult) to additionally verify the operation returns rows (e.g., check the resolver/method type or a flag indicating row-returning operations) before calling prepareArgsForExtResult, and apply this same guard wherever similar code appears (references: prepareArgsForExtResult, hasAnyExtResult, postProcess, and the groupBy call site) so ext-result processing runs only for row-returning operations.
🧹 Nitpick comments (2)
tests/e2e/orm/plugin-infra/ext-result.test.ts (1)
626-627: Avoid hard-coded relation IDs in fixtures.At Line 627, using
authorId: 1assumes a fixed PK value. Prefer using the created user’s id from Line 626 to keep tests resilient. This same pattern appears again (e.g., Lines 650, 681, 706, 731, 760).Suggested patch pattern
-await extDb.user.create({ data: { name: 'Alice' } }); -await extDb.post.create({ data: { title: 'Hello', authorId: 1 } }); +const user = await extDb.user.create({ data: { name: 'Alice' } }); +await extDb.post.create({ data: { title: 'Hello', authorId: user.id } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/plugin-infra/ext-result.test.ts` around lines 626 - 627, The test uses a hard-coded foreign key (authorId: 1) when creating posts; capture the created user's id from extDb.user.create (e.g., const user = await extDb.user.create(...); const userId = user.id) and pass that variable to extDb.post.create (authorId: userId) everywhere it appears (references around extDb.user.create and extDb.post.create) to avoid assuming a fixed primary key; update all occurrences noted (lines near the subsequent post creations) to use the created user's id instead of 1.packages/orm/src/client/client-impl.ts (1)
1002-1005: Cache ext-result defs perapplyExtResultcall to reduce row-wise overhead.
collectExtResultDefsis invoked again for every row inapplyExtResultToRow, which is avoidable work for large result sets.Refactor sketch
function applyExtResult( result: unknown, model: string, originalArgs: unknown, schema: SchemaDef, plugins: AnyPlugin[], ): unknown { + const extResultDefs = collectExtResultDefs(model, plugins); if (Array.isArray(result)) { for (let i = 0; i < result.length; i++) { - result[i] = applyExtResultToRow(result[i], model, originalArgs, schema, plugins); + result[i] = applyExtResultToRow(result[i], model, originalArgs, schema, plugins, extResultDefs); } return result; } else { - return applyExtResultToRow(result, model, originalArgs, schema, plugins); + return applyExtResultToRow(result, model, originalArgs, schema, plugins, extResultDefs); } } function applyExtResultToRow( row: unknown, model: string, originalArgs: unknown, schema: SchemaDef, plugins: AnyPlugin[], + extResultDefs: Map<string, ExtResultDef>, ): unknown { ... - const extResultDefs = collectExtResultDefs(model, plugins);Also applies to: 1024-1025
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/client-impl.ts` around lines 1002 - 1005, The loop calls applyExtResultToRow for each row which internally calls collectExtResultDefs repeatedly causing per-row overhead; change applyExtResult (or the block where the Array.isArray(result) is handled) to call collectExtResultDefs once, store the returned ext-result definitions in a local variable, and pass that cached defs into applyExtResultToRow (or modify applyExtResultToRow to accept defs as an extra parameter) so the per-row iteration reuses the same defs instead of recomputing them for every row; apply the same change to the analogous code at the 1024-1025 site.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/orm/plugin-infra/ext-result.test.ts`:
- Around line 167-205: The test name mentions "aggregate" but never calls
extDb.user.aggregate; add an aggregate call and assertion to match the test
intent: invoke extDb.user.aggregate with a simple aggregate (e.g., _count or
_avg) via the extDb.user.aggregate(...) API, assert the expected aggregate value
shape (e.g., presence of _count) and then assert that the returned aggregate
object does not have the virtual field (e.g., (aggregateResult as any).upperName
is undefined); alternatively, if you prefer to change the test name, update the
it(...) description to remove "aggregate" to reflect the current assertions.
---
Duplicate comments:
In `@packages/orm/src/client/client-impl.ts`:
- Around line 569-571: postProcess currently gates ext-result handling for all
operations, which incorrectly applies model extension fields to
non-row-returning ops like groupBy; update the logic around
prepareArgsForExtResult (used where postProcess && hasAnyExtResult) to
additionally verify the operation returns rows (e.g., check the resolver/method
type or a flag indicating row-returning operations) before calling
prepareArgsForExtResult, and apply this same guard wherever similar code appears
(references: prepareArgsForExtResult, hasAnyExtResult, postProcess, and the
groupBy call site) so ext-result processing runs only for row-returning
operations.
In `@tests/e2e/orm/plugin-infra/ext-result.test.ts`:
- Around line 36-39: The test uses extDb.user.findMany() and then asserts
users[0] and users[1] which is flaky because DB return order is not guaranteed;
modify the test to call extDb.user.findMany({ orderBy: { id: 'asc' } }) (or
another stable column like createdAt/name) before making index-based assertions,
and apply the same explicit orderBy change to any other occurrences in this file
that assert by array index (e.g., places referencing users[N] or similar results
from createManyAndReturn/findMany).
---
Nitpick comments:
In `@packages/orm/src/client/client-impl.ts`:
- Around line 1002-1005: The loop calls applyExtResultToRow for each row which
internally calls collectExtResultDefs repeatedly causing per-row overhead;
change applyExtResult (or the block where the Array.isArray(result) is handled)
to call collectExtResultDefs once, store the returned ext-result definitions in
a local variable, and pass that cached defs into applyExtResultToRow (or modify
applyExtResultToRow to accept defs as an extra parameter) so the per-row
iteration reuses the same defs instead of recomputing them for every row; apply
the same change to the analogous code at the 1024-1025 site.
In `@tests/e2e/orm/plugin-infra/ext-result.test.ts`:
- Around line 626-627: The test uses a hard-coded foreign key (authorId: 1) when
creating posts; capture the created user's id from extDb.user.create (e.g.,
const user = await extDb.user.create(...); const userId = user.id) and pass that
variable to extDb.post.create (authorId: userId) everywhere it appears
(references around extDb.user.create and extDb.post.create) to avoid assuming a
fixed primary key; update all occurrences noted (lines near the subsequent post
creations) to use the created user's id instead of 1.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 646e0c67-c9c1-483e-8217-faea28a8e0bd
📒 Files selected for processing (2)
packages/orm/src/client/client-impl.tstests/e2e/orm/plugin-infra/ext-result.test.ts
- Gate ext-result computation to model-row operations only, excluding
groupBy which uses postProcess but returns aggregated rows
- Fix SelectAwareExtResult type to respect boolean values in select/omit
(e.g. `{ select: { field: false } }` no longer type-includes the field)
- Add orderBy to findMany test to ensure deterministic ordering
- Remove "aggregate" from test name that didn't test aggregate
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/orm/src/client/client-impl.ts (1)
551-554: Consider renaming shadowedpluginsvariable for clarity.The outer
plugins(line 552) is captured by theproceedclosure for ext result processing, while the innerplugins(line 607) is a spread copy used for plugin iteration. Both work correctly, but the shadowing can be confusing when reading the code.Suggested rename
// apply plugins - const plugins = [...(client.$options.plugins ?? [])]; - for (const plugin of plugins) { + const pluginList = [...(client.$options.plugins ?? [])]; + for (const plugin of pluginList) {Also applies to: 606-607
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/client-impl.ts` around lines 551 - 554, The outer variable plugins (from client.$options.plugins) is shadowed by a later local spread copy, which is confusing; rename the outer binding (e.g., clientPlugins or outerPlugins) and update all uses (including the hasExtResultDefs(plugins) call and the proceed closure that captures it) so the closure references the renamed clientPlugins, and rename the inner spread copy (e.g., pluginsCopy or pluginList) where iteration happens (the local spread near the plugin iteration block) to avoid shadowing and clarify intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/orm/plugin-infra/ext-result.test.ts`:
- Around line 626-627: The test hardcodes authorId: 1 when creating posts which
can break if auto-increment IDs are not 1; change the pattern so you capture the
created user's id from extDb.user.create and use that id when creating the post
(e.g., assign const user = await extDb.user.create(...); then use user.id for
extDb.post.create). Apply the same fix for the other occurrences of
extDb.post.create that pass authorId (the repeated pattern around the other test
cases).
---
Nitpick comments:
In `@packages/orm/src/client/client-impl.ts`:
- Around line 551-554: The outer variable plugins (from client.$options.plugins)
is shadowed by a later local spread copy, which is confusing; rename the outer
binding (e.g., clientPlugins or outerPlugins) and update all uses (including the
hasExtResultDefs(plugins) call and the proceed closure that captures it) so the
closure references the renamed clientPlugins, and rename the inner spread copy
(e.g., pluginsCopy or pluginList) where iteration happens (the local spread near
the plugin iteration block) to avoid shadowing and clarify intent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 368a023a-086b-4802-aad7-dde0743a87bf
📒 Files selected for processing (3)
packages/orm/src/client/client-impl.tspackages/orm/src/client/crud-types.tstests/e2e/orm/plugin-infra/ext-result.test.ts
Ext result fields in nested relation select/omit are not yet reflected in the type system. Use `as any` casts to fix TypeScript compilation errors in the nested relation test cases while still testing runtime behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
MySQL does not support createManyAndReturn. Add an early return guard matching the pattern used in other test files. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/e2e/orm/plugin-infra/ext-result.test.ts (1)
631-632:⚠️ Potential issue | 🟡 MinorHardcoded
authorId: 1causes flaky tests across database providers.This pattern appears in multiple tests (lines 632, 655, 686, 713, 738, 767). Auto-increment counters don't reset on
deleteMany(), so the first user created might haveid > 1. Use the created user's actual ID instead.Suggested fix pattern
-await extDb.user.create({ data: { name: 'Alice' } }); -await extDb.post.create({ data: { title: 'Hello', authorId: 1 } }); +const user = await extDb.user.create({ data: { name: 'Alice' } }); +await extDb.post.create({ data: { title: 'Hello', authorId: user.id } });,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/plugin-infra/ext-result.test.ts` around lines 631 - 632, The test uses a hardcoded authorId (authorId: 1) when creating posts which causes flakes because auto-increment IDs may not start at 1 after deleteMany(); update the test to capture the created user result from extDb.user.create (e.g., const user = await extDb.user.create(...)) and use user.id for extDb.post.create authorId (and similarly replace other hardcoded authorId occurrences in this file at the locations noted) so the post references the actual created user's ID.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/orm/plugin-infra/ext-result.test.ts`:
- Around line 164-169: The test assumes a deterministic order from
extDb.user.createManyAndReturn; instead make assertions order-independent by
verifying the returned array contains both expected records regardless of
position—e.g., extract names or upperName from the users array (from the users
variable returned by extDb.user.createManyAndReturn) and assert that the
set/array includes 'ALICE' and 'BOB' (or compare sorted arrays) rather than
checking users[0] and users[1].
- Around line 193-194: The test uses a hardcoded id (1) in extDb.user.exists
which makes it flaky; capture the created user's id from the create call (e.g.,
save the result of extDb.user.create into a variable like createdUser or user)
and use createdUser.id in the exists check (extDb.user.exists({ where: { id:
createdUser.id } })). Update the assertion to use that dynamic id so it reliably
checks existence regardless of auto-increment state.
---
Duplicate comments:
In `@tests/e2e/orm/plugin-infra/ext-result.test.ts`:
- Around line 631-632: The test uses a hardcoded authorId (authorId: 1) when
creating posts which causes flakes because auto-increment IDs may not start at 1
after deleteMany(); update the test to capture the created user result from
extDb.user.create (e.g., const user = await extDb.user.create(...)) and use
user.id for extDb.post.create authorId (and similarly replace other hardcoded
authorId occurrences in this file at the locations noted) so the post references
the actual created user's ID.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4514f3a1-875d-419e-9b2f-908ef03d9668
📒 Files selected for processing (1)
tests/e2e/orm/plugin-infra/ext-result.test.ts
…ultField helper - Add ExtResult parameter to IncludeInput and SelectInput, and pass it through SelectIncludeOmit, so nested relation select/omit inputs recognize ext result field names without requiring `as any` casts - Add resultField() helper function that provides typed compute parameters based on needs keys via correlated type inference - Remove as-any casts from nested relation tests now that types work - Add test for resultField helper Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Import ExtResultFieldDef from plugin.ts instead of redeclaring a local duplicate type, and extract duplicated Zod factory loop into a shared addExtResultFields helper method. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add the source .zmodel file so the generated schema.ts can be regenerated, matching the convention used by sibling test directories. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add input.ts and models.ts generated alongside schema.ts, matching the convention used by sibling test directories. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move EXT_RESULT_OPERATIONS to a module-level constant to avoid reallocating on every Proxy get trap, and compute collectExtResultFieldDefs once per query instead of once per row. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/e2e/orm/plugin-infra/ext-result.test.ts (2)
193-193:⚠️ Potential issue | 🟡 MinorRemove hardcoded PK assumptions in setup and assertions.
Several cases assume IDs start at
1(or thatidDoubled === 2), which makes this suite flaky across DB providers and run order.Suggested fix pattern
- await extDb.user.create({ data: { name: 'Alice' } }); - await extDb.post.create({ data: { title: 'Hello', authorId: 1 } }); + const user = await extDb.user.create({ data: { name: 'Alice' } }); + await extDb.post.create({ data: { title: 'Hello', authorId: user.id } }); - const exists = await extDb.user.exists({ where: { id: 1 } }); + const exists = await extDb.user.exists({ where: { id: user.id } }); - expect(users[0]!.idDoubled).toBe(2); + expect(users[0]!.idDoubled).toBe(users[0]!.id * 2);Also applies to: 343-343, 404-404, 570-570, 581-581, 632-632, 655-655, 684-684, 709-709, 734-734, 763-763, 808-808
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/plugin-infra/ext-result.test.ts` at line 193, Tests assume primary keys equal specific values (e.g. id === 1 or idDoubled === 2) which is flaky; update the setup and assertions in ext-result.test.ts to capture and use the actual IDs (or other stable identifiers) returned by create/insert operations instead of hardcoding numeric PKs: replace direct checks like extDb.user.exists({ where: { id: 1 } }) and comparisons against literal IDs with checks using the inserted record's id variable (or query by unique fields), and update any related assertions (e.g. idDoubled comparisons) to compute using those captured values so the tests no longer rely on DB-specific starting IDs.
164-169:⚠️ Potential issue | 🟡 MinorMake
createManyAndReturnassertions order-independent.This still depends on returned row order, which is not guaranteed across providers.
Suggested patch
const users = await extDb.user.createManyAndReturn({ data: [{ name: 'Alice' }, { name: 'Bob' }], }); expect(users).toHaveLength(2); -expect(users[0]!.upperName).toBe('ALICE'); -expect(users[1]!.upperName).toBe('BOB'); +const upperNames = users.map((u) => u.upperName).sort(); +expect(upperNames).toEqual(['ALICE', 'BOB']);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/plugin-infra/ext-result.test.ts` around lines 164 - 169, The test assumes a specific row order from extDb.user.createManyAndReturn; make it order-independent by extracting the returned users' upperName values (from the createManyAndReturn result) and asserting the set/array contains both 'ALICE' and 'BOB' (e.g., compare sorted arrays or use an unordered containment assertion such as expect(...).toEqual(expect.arrayContaining([...]))), rather than checking users[0] and users[1] directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/orm/plugin-infra/ext-result.test.ts`:
- Around line 603-608: The test currently asserts posts by position after
calling extDb.user.findMany({ include: { posts: true } }) causing flakiness;
update the assertion to be content-based or deterministic by either sorting the
returned posts array (e.g., by id or title) before asserting or finding each
post by its upperTitle/value and asserting their contents, targeting the posts
collection on users[0] (users[0].posts) and the upperTitle property (upperTitle)
rather than relying on posts[0]/posts[1] positions.
---
Duplicate comments:
In `@tests/e2e/orm/plugin-infra/ext-result.test.ts`:
- Line 193: Tests assume primary keys equal specific values (e.g. id === 1 or
idDoubled === 2) which is flaky; update the setup and assertions in
ext-result.test.ts to capture and use the actual IDs (or other stable
identifiers) returned by create/insert operations instead of hardcoding numeric
PKs: replace direct checks like extDb.user.exists({ where: { id: 1 } }) and
comparisons against literal IDs with checks using the inserted record's id
variable (or query by unique fields), and update any related assertions (e.g.
idDoubled comparisons) to compute using those captured values so the tests no
longer rely on DB-specific starting IDs.
- Around line 164-169: The test assumes a specific row order from
extDb.user.createManyAndReturn; make it order-independent by extracting the
returned users' upperName values (from the createManyAndReturn result) and
asserting the set/array contains both 'ALICE' and 'BOB' (e.g., compare sorted
arrays or use an unordered containment assertion such as
expect(...).toEqual(expect.arrayContaining([...]))), rather than checking
users[0] and users[1] directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2fb575b0-4834-494a-997a-374be229765f
📒 Files selected for processing (8)
packages/orm/src/client/client-impl.tspackages/orm/src/client/crud-types.tspackages/orm/src/client/plugin.tspackages/orm/src/client/zod/factory.tstests/e2e/orm/plugin-infra/ext-result.test.tstests/e2e/orm/plugin-infra/ext-result/input.tstests/e2e/orm/plugin-infra/ext-result/models.tstests/e2e/orm/plugin-infra/ext-result/schema.zmodel
✅ Files skipped from review due to trivial changes (1)
- packages/orm/src/client/crud-types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/orm/src/client/zod/factory.ts
ymc9
reviewed
Mar 9, 2026
Member
ymc9
left a comment
ymc9
left a comment
There was a problem hiding this comment.
Hi @genu ,
I've finished reviewing this PR, and it looks amazing! Both the typing parts and runtime parts are well thought through, with many edge cases covered.
Please find my review comments attached. Looking forward to having the feature in v3.5!
packages/orm/src/client/plugin.ts
Outdated
| /** | ||
| * Definition for a single extended result field. | ||
| */ | ||
| export type ExtResultFieldDef = { |
Member
There was a problem hiding this comment.
Same here, we should be able to add a Schema and Model parameters so as to type the needs field and data parameter.
Contributor
Author
There was a problem hiding this comment.
I updated this to export type ExtResultFieldDef<Needs extends Record<string, true> = Record<string, true>>.
Did you mean we should pass Schema/Model through to it? We can add those params to ExtResultFieldDef itself, but we can't use them in ExtResultBase — doing so forces TypeScript to resolve NonRelationFields<Schema, M> while it's still inferring ExtResult in definePlugin, which creates a circular inference that collapses ExtResult to {}.
The needs validation already works today via the infer N + keyof N extends NonRelationFields check in RuntimePlugin.result.
For the compute data values (which were typed as any), I’ve already updated RuntimePlugin.result to provide a typed contextual type using MapModelFieldType, so compute’s data parameter now gets proper field types (e.g., user.name is string, user.id is number) without needing to change ExtResultFieldDef or ExtResultBase.
Unless you mean something else?
| * }); | ||
| * ``` | ||
| */ | ||
| export function resultField<const N extends Record<string, true>, R>(def: { |
Member
There was a problem hiding this comment.
I think this helper is related to my previous comment. Just wondering if it's more straightforward to express the typing directly on the RuntimePlugin interface side.
Contributor
Author
There was a problem hiding this comment.
since RuntimePlugin.result now contextually provides strict typing for both needs keys and compute data values (via MapModelFieldType), this helper may no longer be needed — it actually gives weaker types (any values) than what the interface provides.
The only case where it could still be useful is if someone defines result fields in a separate variable outside of definePlugin, where the contextual typing wouldn’t apply. But since that’s an uncommon pattern, maybe we can remove it?
| definePlugin({ | ||
| id: 'greeting', | ||
| result: { | ||
| User: { |
Member
There was a problem hiding this comment.
I'm thinking if we should consistently use "lower-case-first" model name ("User" -> "user") in ZenStackClient and plugin settings, which looks more natural.
Currently the "omit" settings in the ORM client use original model names. I regretted it and I'm planning to make a change there.
Contributor
Author
There was a problem hiding this comment.
Made the change, however, to make user: work as a key instead of User:, the types had to change too. If I only changed the tests to use user: without updating ExtResultBase and the related types, TypeScript would reject user as an invalid key
Contributor
|
Mostly copying my suggested changes from the other PR: All fields should maintain their typing as specified in the schema, but non-dependency fields should be optional, and the computed field in question should be absent. Consider the following: model User {
id String @id @default(uuid())
age Int
birthDate DateTime
role Role
firstName String
lastName String
fullName String
}
enum Role {
USER
ADMIN
}const db = client.$use(definePlugin({
id: 'display',
result: {
User: {
fullName: {
needs: { firstName: true, lastName: true },
compute: (user) => `${user.firstName} ${user.lastName}`,
},
},
},
}));The {
id?: string
age?: number
role?: 'USER' | 'ADMIN'
birthDate?: Date
firstName: string
lastName: string
}Notice how everything that isn't a dependency is allowed to be undefined, dependencies themselves are not, everything still maintains its proper type, and fullName is absent because it doesn't make sense to be able to access a field you are defining how to access. @ymc9 @genu thoughts? This is the most accurate typing as far as I can tell. Could be reduced if it's too complex to implement. |
Replace injected-needs tracking with declarative cleanup: after computing ext result fields, strip fields not in the original select/omit instead of surgically tracking which needs were injected.
Parameterize ExtResultBase with Schema across all type definitions for stricter type checking. Add NonRelationFields constraint on plugin result needs fields. Add type-level tests for invalid model names and invalid needs field names. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
tests/e2e/orm/plugin-infra/ext-result.test.ts (4)
603-608:⚠️ Potential issue | 🟡 MinorPositional assertions for included
postswithout ordering may be flaky.Relation order isn't guaranteed. Sort the posts array before asserting or use content-based checks.
Suggested fix
const users = await extDb.user.findMany({ include: { posts: true } }); expect(users).toHaveLength(1); expect(users[0]!.posts).toHaveLength(2); -expect(users[0]!.posts[0]!.upperTitle).toBe('HELLO WORLD'); -expect(users[0]!.posts[1]!.upperTitle).toBe('SECOND POST'); +const titles = users[0]!.posts.map((p) => p.upperTitle).sort(); +expect(titles).toEqual(['HELLO WORLD', 'SECOND POST']);,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/plugin-infra/ext-result.test.ts` around lines 603 - 608, The test uses extDb.user.findMany(... include: { posts: true }) and then asserts posts by position (users[0]!.posts[0]!, posts[1]!), which is flaky because relation order is not guaranteed; modify the test to either sort users[0]!.posts by a stable key (e.g., id or title) before making positional assertions or replace positional checks with content-based assertions that locate posts by value (e.g., find post where upperTitle === 'HELLO WORLD' and assert its existence and properties) so the expectations no longer depend on insertion order.
193-193:⚠️ Potential issue | 🟡 MinorHardcoded
id: 1inexistscheck may cause flaky tests.Auto-increment counters don't reset on
deleteMany(). Capture the created user's ID and use it dynamically.Suggested fix
-await extDb.user.create({ data: { name: 'Alice' } }); +const user = await extDb.user.create({ data: { name: 'Alice' } }); const count = await extDb.user.count(); expect(count).toBe(1); expect((count as any).upperName).toBeUndefined(); -const exists = await extDb.user.exists({ where: { id: 1 } }); +const exists = await extDb.user.exists({ where: { id: user.id } });,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/plugin-infra/ext-result.test.ts` at line 193, The exists check currently uses a hardcoded id (id: 1) which can cause flaky tests; modify the test to capture the created user's ID (e.g., from the result of extDb.user.create or whichever variable holds the created user) and pass that dynamic id to extDb.user.exists({ where: { id: createdUser.id } }) instead of using 1 so the existence check targets the actual created record.
631-632:⚠️ Potential issue | 🟡 MinorHardcoded
authorId: 1causes flaky tests across database providers.Auto-increment counters don't reset on
deleteMany(). This pattern appears in multiple tests. Capture the created user's ID and use it when creating posts.Suggested fix pattern (apply to all occurrences)
-await extDb.user.create({ data: { name: 'Alice' } }); -await extDb.post.create({ data: { title: 'Hello', authorId: 1 } }); +const user = await extDb.user.create({ data: { name: 'Alice' } }); +await extDb.post.create({ data: { title: 'Hello', authorId: user.id } });,
Also applies to: 654-655, 683-684, 708-709, 733-734, 762-763, 807-808
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/plugin-infra/ext-result.test.ts` around lines 631 - 632, The test is using a hardcoded authorId (authorId: 1) which leads to flaky behavior because auto-increment IDs aren't guaranteed after deleteMany(); update the test to capture the created user's id from extDb.user.create(...) (e.g., const user = await extDb.user.create(...); const authorId = user.id) and then pass that variable into extDb.post.create({ data: { title: '...', authorId } }) for each occurrence (references: extDb.user.create and extDb.post.create) so the post always links to the actual user record; apply this pattern to all listed occurrences.
164-169:⚠️ Potential issue | 🟡 Minor
createManyAndReturnresult order is not deterministic.The test assumes
users[0]is Alice andusers[1]is Bob, but return order isn't guaranteed by SQL standard. Sort before asserting or use content-based assertions.Suggested fix
const users = await extDb.user.createManyAndReturn({ data: [{ name: 'Alice' }, { name: 'Bob' }], }); expect(users).toHaveLength(2); -expect(users[0]!.upperName).toBe('ALICE'); -expect(users[1]!.upperName).toBe('BOB'); +const sorted = users.sort((a, b) => a.name.localeCompare(b.name)); +expect(sorted[0]!.upperName).toBe('ALICE'); +expect(sorted[1]!.upperName).toBe('BOB');,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/plugin-infra/ext-result.test.ts` around lines 164 - 169, The test assumes deterministic order from extDb.user.createManyAndReturn (creating Alice then Bob) but SQL does not guarantee order; update the test that uses createManyAndReturn to either sort the returned array by a stable key (e.g., sort users by user.name or upperName) before asserting lengths and values, or use content-based assertions by locating entries via find/map (e.g., find user with name 'Alice' and assert its upperName is 'ALICE', and similarly for 'Bob') so the assertions no longer depend on insertion order.
🧹 Nitpick comments (1)
packages/orm/src/client/plugin.ts (1)
35-35: Consider documenting theanytyping in compute parameter.The
computecallback receives{ [K in keyof N]: any }, which loses type safety for actual field values. This appears intentional based on PR discussion (implementation complexity trade-off), but a brief JSDoc note explaining this design choice would help future maintainers understand why stricter typing wasn't used.Also applies to: 163-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/plugin.ts` at line 35, Add a brief JSDoc above the compute function signature in plugin.ts explaining why the parameter uses a loose any type (e.g., "compute: (data: { [K in keyof Needs]: any }) => unknown;") — note that stricter typing was intentionally avoided due to implementation complexity and runtime variability of field values, and mention locations where the same pattern is used (the same compute signature around lines 163-166) so future maintainers understand the design trade-off and where to update if stricter typing becomes feasible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/e2e/orm/plugin-infra/ext-result.test.ts`:
- Around line 603-608: The test uses extDb.user.findMany(... include: { posts:
true }) and then asserts posts by position (users[0]!.posts[0]!, posts[1]!),
which is flaky because relation order is not guaranteed; modify the test to
either sort users[0]!.posts by a stable key (e.g., id or title) before making
positional assertions or replace positional checks with content-based assertions
that locate posts by value (e.g., find post where upperTitle === 'HELLO WORLD'
and assert its existence and properties) so the expectations no longer depend on
insertion order.
- Line 193: The exists check currently uses a hardcoded id (id: 1) which can
cause flaky tests; modify the test to capture the created user's ID (e.g., from
the result of extDb.user.create or whichever variable holds the created user)
and pass that dynamic id to extDb.user.exists({ where: { id: createdUser.id } })
instead of using 1 so the existence check targets the actual created record.
- Around line 631-632: The test is using a hardcoded authorId (authorId: 1)
which leads to flaky behavior because auto-increment IDs aren't guaranteed after
deleteMany(); update the test to capture the created user's id from
extDb.user.create(...) (e.g., const user = await extDb.user.create(...); const
authorId = user.id) and then pass that variable into extDb.post.create({ data: {
title: '...', authorId } }) for each occurrence (references: extDb.user.create
and extDb.post.create) so the post always links to the actual user record; apply
this pattern to all listed occurrences.
- Around line 164-169: The test assumes deterministic order from
extDb.user.createManyAndReturn (creating Alice then Bob) but SQL does not
guarantee order; update the test that uses createManyAndReturn to either sort
the returned array by a stable key (e.g., sort users by user.name or upperName)
before asserting lengths and values, or use content-based assertions by locating
entries via find/map (e.g., find user with name 'Alice' and assert its upperName
is 'ALICE', and similarly for 'Bob') so the assertions no longer depend on
insertion order.
---
Nitpick comments:
In `@packages/orm/src/client/plugin.ts`:
- Line 35: Add a brief JSDoc above the compute function signature in plugin.ts
explaining why the parameter uses a loose any type (e.g., "compute: (data: { [K
in keyof Needs]: any }) => unknown;") — note that stricter typing was
intentionally avoided due to implementation complexity and runtime variability
of field values, and mention locations where the same pattern is used (the same
compute signature around lines 163-166) so future maintainers understand the
design trade-off and where to update if stricter typing becomes feasible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0903d5ec-7a23-48f3-9d10-8714860f80c8
📒 Files selected for processing (4)
packages/orm/src/client/contract.tspackages/orm/src/client/crud-types.tspackages/orm/src/client/plugin.tstests/e2e/orm/plugin-infra/ext-result.test.ts
Use Uncapitalize<GetModels<Schema>> for plugin result keys to match the lowercase-first convention used in ZenStackClient (e.g., db.user). Also export MapModelFieldType and strictly type compute data values via contextual typing in RuntimePlugin.result.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
tests/e2e/orm/plugin-infra/ext-result.test.ts (1)
193-194:⚠️ Potential issue | 🟡 MinorStop hardcoding primary key
1in these cases.These inserts/assertions assume the PK sequence resets between tests. If it does not,
exists({ id: 1 })andauthorId: 1point at the wrong row and make the suite flaky. Capture the created user's id and reuse it instead.Patch pattern
- await extDb.user.create({ data: { name: 'Alice' } }); - const exists = await extDb.user.exists({ where: { id: 1 } }); + const user = await extDb.user.create({ data: { name: 'Alice' } }); + const exists = await extDb.user.exists({ where: { id: user.id } }); - await extDb.user.create({ data: { name: 'Alice' } }); - await extDb.post.create({ data: { title: 'Hello', authorId: 1 } }); + const user = await extDb.user.create({ data: { name: 'Alice' } }); + await extDb.post.create({ data: { title: 'Hello', authorId: user.id } });Also applies to: 631-632, 654-655, 683-684, 708-709, 733-734, 762-763, 807-808
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/plugin-infra/ext-result.test.ts` around lines 193 - 194, The test is hardcoding primary key 1 for existence checks and foreign keys; instead capture and reuse the created user's actual id. Update places that call extDb.user.exists({ where: { id: 1 } }) and any authorId: 1 in subsequent inserts to first create the user (via extDb.user.create(...)), save const user = await extDb.user.create(...); const userId = user.id, then use userId in extDb.user.exists({ where: { id: userId } }) and in records that set authorId (e.g., when creating posts/comments). Replace all occurrences referenced in this file (the extDb.user.exists and authorId usages) to use the captured userId variable.packages/orm/src/client/plugin.ts (1)
187-190:⚠️ Potential issue | 🟠 Major
resultField()still erases thecomputeinput types.This helper contextually types
computeas{ [K in keyof N]: any }, so callers lose the schema-aware field types they get from inlineresultdefinitions. A quick sanity check likeresultField({ needs: { name: true }, compute: (u) => u.name.toFixed() })will still compile today. If this helper stays, it needs to stop spellinganyhere or it undercuts the typed-compute API.
🤖 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/orm/src/client/client-impl.ts`:
- Around line 871-884: collectExtResultFieldDefs currently accepts plugin result
keys without checking they collide with real model fields; update it to validate
each fieldName against the model's schema and throw an error if a plugin
attempts to register a key that already exists on the model (e.g., "name", "id",
"posts"). Locate collectExtResultFieldDefs and, before defs.set(...), look up
the model's defined fields (using the existing model/schema registry available
in this module/class) and if fieldName is present in the model schema, throw a
descriptive Error mentioning the model and offending key; otherwise continue to
add the virtual field. This prevents prepareArgsForExtResult and
applyExtResultToRow from overwriting real fields.
In `@packages/orm/src/client/plugin.ts`:
- Around line 176-183: The example uses an uppercase model key "User" which will
fail type-checking because result keys are lowercase model names; update the
example to use the lowercase model identifier (e.g., change "User" to "user") so
the mapping under result (including the resultField for fullName with needs and
compute) matches the expected model key names used by the ORM plugin and types.
---
Duplicate comments:
In `@tests/e2e/orm/plugin-infra/ext-result.test.ts`:
- Around line 193-194: The test is hardcoding primary key 1 for existence checks
and foreign keys; instead capture and reuse the created user's actual id. Update
places that call extDb.user.exists({ where: { id: 1 } }) and any authorId: 1 in
subsequent inserts to first create the user (via extDb.user.create(...)), save
const user = await extDb.user.create(...); const userId = user.id, then use
userId in extDb.user.exists({ where: { id: userId } }) and in records that set
authorId (e.g., when creating posts/comments). Replace all occurrences
referenced in this file (the extDb.user.exists and authorId usages) to use the
captured userId variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a77142ed-fb76-42c2-ac5b-9917f19e9e32
📒 Files selected for processing (5)
packages/orm/src/client/client-impl.tspackages/orm/src/client/crud-types.tspackages/orm/src/client/plugin.tstests/e2e/orm/plugin-infra/ext-result.test.tstests/e2e/orm/plugin-infra/ext-result/schema.ts
Simplify the `result` property type in RuntimePlugin from a complex
mapped/conditional type to plain `ExtResult`. The complex type prevented
TypeScript from reverse-inferring ExtResult, causing it to fall back to
{} and making all ext result fields invisible in return types.
Also fix addExtResultFields in ZodSchemaFactory to use lowerCaseFirst(model)
matching the convention used in the runtime helpers.
Convert compile-time validation tests to runtime graceful-handling tests
since model name and needs field validation requires an explicit Schema
type parameter which isn't available through definePlugin().
Add a check in collectExtResultFieldDefs that throws if a plugin registers an ext result field name that already exists on the model. Without this, prepareArgsForExtResult would strip the real field from select/omit and applyExtResultToRow would overwrite the actual DB value with the computed one.
Contributor
Author
|
I think this is reasonable since at runtime the full row is passed to compute anyway — those fields may or may not be present depending on the query’s select. Making them optional accurately reflects that. The exclusion of the field being defined also makes sense to prevent circular references. One consideration: this adds type-level complexity since we’d need to thread the field name being defined into the compute parameter type and map all model fields. But the DX improvement seems worth it. I'm not exactly sure on the level of effort here as it’s a TypeScript inference puzzle. Could take a few hours of experimentation, or could be a rabbit hole. |
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
tests/e2e/orm/plugin-infra/ext-result.test.ts (4)
189-194:⚠️ Potential issue | 🟡 MinorUse the created user's id in the
existscheck.This test still assumes the inserted row has
id = 1. Capture thecreate()result and reuse itsidso the assertion doesn't depend on auto-increment state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/plugin-infra/ext-result.test.ts` around lines 189 - 194, The test assumes the created user has id=1; change it to capture the result of extDb.user.create(...) (e.g., const created = await extDb.user.create({...})) and then use created.id in the exists check (extDb.user.exists({ where: { id: created.id } })) so the assertion doesn't rely on auto-increment state; keep the existing count and upperName assertions intact.
164-169:⚠️ Potential issue | 🟡 MinorMake
createManyAndReturnassertions order-independent.This still assumes the returned rows come back as Alice then Bob. That order isn't guaranteed, so sort/assert by content before indexing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/plugin-infra/ext-result.test.ts` around lines 164 - 169, The test assumes deterministic ordering from extDb.user.createManyAndReturn — change the assertions to be order-independent by mapping or sorting the returned users before checking values: e.g., extract users.map(u => u.upperName) and assert it contains both 'ALICE' and 'BOB' or sort that array and compare to an expected sorted array; update the assertions referencing users, users[0]!, and users[1]! accordingly so the test no longer depends on insertion order.
603-607:⚠️ Potential issue | 🟡 MinorDon't assert nested post order without ordering or sorting.
include: { posts: true }doesn't guarantee the relation array order, soposts[0]/posts[1]is still flaky. Sort the titles first or request a deterministic nestedorderBy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/plugin-infra/ext-result.test.ts` around lines 603 - 607, The test currently assumes a deterministic order for nested posts returned by extDb.user.findMany({ include: { posts: true } }) by asserting posts[0] and posts[1]; change this to a deterministic check either by requesting a nested order (e.g., include posts with an orderBy on title or createdAt) in the extDb.user.findMany call or by extracting posts' titles/upperTitle into an array and sorting it before assertions; update references to posts[0], posts[1], and upperTitle accordingly so the test asserts against a sorted/deterministic list rather than positional indices.
631-632:⚠️ Potential issue | 🟡 MinorAvoid hardcoding
authorId: 1in these relation fixtures.This pattern still makes the tests depend on the auto-increment counter resetting. Reuse the
user.idfrom the preceding create call instead. The same issue repeats on Lines 655, 684, 709, 734, 763, and 808.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/plugin-infra/ext-result.test.ts` around lines 631 - 632, The test currently hardcodes authorId: 1 when creating posts; instead capture the created user object returned from extDb.user.create (e.g., const user = await extDb.user.create(...)) and use user.id when calling extDb.post.create({ data: { title: 'Hello', authorId: user.id } }); apply the same change for the other occurrences referenced (lines where extDb.post.create is used after extDb.user.create) so tests no longer depend on an auto-increment reset.
🤖 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/orm/src/client/client-impl.ts`:
- Around line 871-889: collectExtResultFieldDefs currently only rejects direct
field-name conflicts but doesn't validate fieldDef.needs keys; update
collectExtResultFieldDefs to iterate over each key in (fieldDef as
ExtResultFieldDef).needs (if present) and for each key call getField(schema,
model, needKey) to ensure the field exists and is not a relation field, throwing
an Error that names plugin.id, model, fieldName and the invalid needKey when a
need is missing or is a relation; this ensures prepareArgsForExtResult won't
later inject invalid or relation fields into select.
---
Duplicate comments:
In `@tests/e2e/orm/plugin-infra/ext-result.test.ts`:
- Around line 189-194: The test assumes the created user has id=1; change it to
capture the result of extDb.user.create(...) (e.g., const created = await
extDb.user.create({...})) and then use created.id in the exists check
(extDb.user.exists({ where: { id: created.id } })) so the assertion doesn't rely
on auto-increment state; keep the existing count and upperName assertions
intact.
- Around line 164-169: The test assumes deterministic ordering from
extDb.user.createManyAndReturn — change the assertions to be order-independent
by mapping or sorting the returned users before checking values: e.g., extract
users.map(u => u.upperName) and assert it contains both 'ALICE' and 'BOB' or
sort that array and compare to an expected sorted array; update the assertions
referencing users, users[0]!, and users[1]! accordingly so the test no longer
depends on insertion order.
- Around line 603-607: The test currently assumes a deterministic order for
nested posts returned by extDb.user.findMany({ include: { posts: true } }) by
asserting posts[0] and posts[1]; change this to a deterministic check either by
requesting a nested order (e.g., include posts with an orderBy on title or
createdAt) in the extDb.user.findMany call or by extracting posts'
titles/upperTitle into an array and sorting it before assertions; update
references to posts[0], posts[1], and upperTitle accordingly so the test asserts
against a sorted/deterministic list rather than positional indices.
- Around line 631-632: The test currently hardcodes authorId: 1 when creating
posts; instead capture the created user object returned from extDb.user.create
(e.g., const user = await extDb.user.create(...)) and use user.id when calling
extDb.post.create({ data: { title: 'Hello', authorId: user.id } }); apply the
same change for the other occurrences referenced (lines where extDb.post.create
is used after extDb.user.create) so tests no longer depend on an auto-increment
reset.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 969acd25-b8a5-4db4-86a8-35494a3492d7
📒 Files selected for processing (6)
packages/orm/src/client/client-impl.tspackages/orm/src/client/crud-types.tspackages/orm/src/client/plugin.tspackages/orm/src/client/zod/factory.tstests/e2e/orm/plugin-infra/ext-result.test.tstests/e2e/orm/plugin-infra/ext-result/schema.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/orm/src/client/zod/factory.ts
- tests/e2e/orm/plugin-infra/ext-result/schema.ts
- Add runtime validation in collectExtResultFieldDefs to reject needs keys that don't exist on the model or reference relation fields - Improve ExtResultBase type to constrain needs keys to NonRelationFields for better autocomplete and type checking via `satisfies` - Update existing test from silent-skip to fail-fast behavior - Add test for relation fields in needs
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
tests/e2e/orm/plugin-infra/ext-result.test.ts (4)
187-194:⚠️ Potential issue | 🟡 MinorHardcoded
id: 1inexistscheck may cause flaky tests.Auto-increment counters don't reset on
deleteMany(). If tests run in different orders, the user created on line 187 may haveid > 1.Suggested fix
-await extDb.user.create({ data: { name: 'Alice' } }); +const user = await extDb.user.create({ data: { name: 'Alice' } }); const count = await extDb.user.count(); expect(count).toBe(1); expect((count as any).upperName).toBeUndefined(); -const exists = await extDb.user.exists({ where: { id: 1 } }); +const exists = await extDb.user.exists({ where: { id: user.id } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/plugin-infra/ext-result.test.ts` around lines 187 - 194, The exists check is using a hardcoded id (id: 1) which is flaky because auto-increment IDs can differ; change the test to use the actual id of the created user (capture the result of extDb.user.create and pass its id to extDb.user.exists) or use a stable unique field (e.g., name) in the where clause so extDb.user.exists(...) queries the record just created rather than assuming id 1.
631-632:⚠️ Potential issue | 🟡 MinorHardcoded
authorId: 1may cause flaky tests across database providers.Auto-increment counters typically don't reset on
deleteMany(), so the first user created might haveid > 1if previous tests ran. This pattern is repeated at multiple locations (lines 632, 655, 684, 709, 734, 763).Suggested fix pattern
-await extDb.user.create({ data: { name: 'Alice' } }); -await extDb.post.create({ data: { title: 'Hello', authorId: 1 } }); +const user = await extDb.user.create({ data: { name: 'Alice' } }); +await extDb.post.create({ data: { title: 'Hello', authorId: user.id } });Also applies to: 654-655, 683-684, 708-709, 733-734, 762-763
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/plugin-infra/ext-result.test.ts` around lines 631 - 632, The test hardcodes authorId: 1 when creating posts which is flaky because auto-increment IDs may not start at 1; change the flow in the ext-result.test.ts tests to capture the created user’s id from the result of extDb.user.create (e.g., const user = await extDb.user.create(...); const userId = user.id) and pass that userId into extDb.post.create instead of 1; apply the same change to all repeated locations that call extDb.user.create followed by extDb.post.create (the occurrences around the extDb.user.create / extDb.post.create pairs at lines noted in the review).
164-170:⚠️ Potential issue | 🟡 Minor
createManyAndReturnresult order may not be deterministic.The test assumes
users[0]is Alice andusers[1]is Bob, but return order isn't guaranteed by SQL standard. Consider asserting based on content rather than position.Suggested fix
const users = await extDb.user.createManyAndReturn({ data: [{ name: 'Alice' }, { name: 'Bob' }], }); expect(users).toHaveLength(2); -expect(users[0]!.upperName).toBe('ALICE'); -expect(users[1]!.upperName).toBe('BOB'); +const sorted = users.sort((a, b) => a.name.localeCompare(b.name)); +expect(sorted[0]!.upperName).toBe('ALICE'); +expect(sorted[1]!.upperName).toBe('BOB');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/plugin-infra/ext-result.test.ts` around lines 164 - 170, The test assumes a deterministic order from extDb.user.createManyAndReturn; instead update the assertions to be order-independent: call extDb.user.createManyAndReturn, extract the returned upperName values (or names) from the result of createManyAndReturn, and assert the array contains 'ALICE' and 'BOB' regardless of position (e.g., using a contains/arrayContaining style assertion or by sorting the extracted values before equality checks) so the test no longer relies on users[0]/users[1] positions.
603-608:⚠️ Potential issue | 🟡 MinorAvoid positional assertions for included
postswithout ordering.This test assumes relation order. Make the assertion content-based (or sort first) to keep it stable.
Suggested patch
const users = await extDb.user.findMany({ include: { posts: true } }); expect(users).toHaveLength(1); expect(users[0]!.posts).toHaveLength(2); -expect(users[0]!.posts[0]!.upperTitle).toBe('HELLO WORLD'); -expect(users[0]!.posts[1]!.upperTitle).toBe('SECOND POST'); +const titles = users[0]!.posts.map((p) => p.upperTitle).sort(); +expect(titles).toEqual(['HELLO WORLD', 'SECOND POST']);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/plugin-infra/ext-result.test.ts` around lines 603 - 608, The test is asserting positions of included posts from extDb.user.findMany which is unstable because relation order is not guaranteed; fix it by making the assertion content-based: after fetching users (extDb.user.findMany with include: { posts: true }), either sort users[0].posts by a stable key (e.g., post.id or post.title) before asserting the indexed upperTitle values, or replace the positional checks with an order-independent assertion such as expecting the array of posts.map(p => p.upperTitle) toEqual(expect.arrayContaining(['HELLO WORLD', 'SECOND POST'])) so the test does not rely on relation ordering.
🧹 Nitpick comments (1)
packages/orm/src/client/plugin.ts (1)
139-165:resultFieldhelper provides weaker typing than contextual type inference.As discussed in past comments, when defining result fields directly inside
definePlugin, theRuntimePlugin.resultinterface already provides contextual typing with proper field types viaMapModelFieldType. This helper returns{ [K in keyof N]: any }which loses those types.The helper is still useful for defining result fields in separate variables outside
definePluginwhere contextual typing wouldn't apply, but this is an uncommon pattern.Consider documenting this trade-off in the JSDoc, or removing the helper if the team decides the contextual typing is sufficient for all practical use cases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/plugin.ts` around lines 139 - 165, The resultField helper returns compute data typed as { [K in keyof N]: any } which loses the contextual field types provided by RuntimePlugin.result/MapModelFieldType when used directly inside definePlugin; update the JSDoc for resultField (the exported function resultField) to document this trade-off explicitly (explain that contextual typing in definePlugin is preferred and resultField is mainly for standalone variable declarations), or, if the team prefers, remove the resultField helper entirely and rely on contextual typing via RuntimePlugin.result/MapModelFieldType instead; reference the function name resultField, the callsite pattern definePlugin, and the types RuntimePlugin.result and MapModelFieldType in the doc/update decision.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/e2e/orm/plugin-infra/ext-result.test.ts`:
- Around line 187-194: The exists check is using a hardcoded id (id: 1) which is
flaky because auto-increment IDs can differ; change the test to use the actual
id of the created user (capture the result of extDb.user.create and pass its id
to extDb.user.exists) or use a stable unique field (e.g., name) in the where
clause so extDb.user.exists(...) queries the record just created rather than
assuming id 1.
- Around line 631-632: The test hardcodes authorId: 1 when creating posts which
is flaky because auto-increment IDs may not start at 1; change the flow in the
ext-result.test.ts tests to capture the created user’s id from the result of
extDb.user.create (e.g., const user = await extDb.user.create(...); const userId
= user.id) and pass that userId into extDb.post.create instead of 1; apply the
same change to all repeated locations that call extDb.user.create followed by
extDb.post.create (the occurrences around the extDb.user.create /
extDb.post.create pairs at lines noted in the review).
- Around line 164-170: The test assumes a deterministic order from
extDb.user.createManyAndReturn; instead update the assertions to be
order-independent: call extDb.user.createManyAndReturn, extract the returned
upperName values (or names) from the result of createManyAndReturn, and assert
the array contains 'ALICE' and 'BOB' regardless of position (e.g., using a
contains/arrayContaining style assertion or by sorting the extracted values
before equality checks) so the test no longer relies on users[0]/users[1]
positions.
- Around line 603-608: The test is asserting positions of included posts from
extDb.user.findMany which is unstable because relation order is not guaranteed;
fix it by making the assertion content-based: after fetching users
(extDb.user.findMany with include: { posts: true }), either sort users[0].posts
by a stable key (e.g., post.id or post.title) before asserting the indexed
upperTitle values, or replace the positional checks with an order-independent
assertion such as expecting the array of posts.map(p => p.upperTitle)
toEqual(expect.arrayContaining(['HELLO WORLD', 'SECOND POST'])) so the test does
not rely on relation ordering.
---
Nitpick comments:
In `@packages/orm/src/client/plugin.ts`:
- Around line 139-165: The resultField helper returns compute data typed as { [K
in keyof N]: any } which loses the contextual field types provided by
RuntimePlugin.result/MapModelFieldType when used directly inside definePlugin;
update the JSDoc for resultField (the exported function resultField) to document
this trade-off explicitly (explain that contextual typing in definePlugin is
preferred and resultField is mainly for standalone variable declarations), or,
if the team prefers, remove the resultField helper entirely and rely on
contextual typing via RuntimePlugin.result/MapModelFieldType instead; reference
the function name resultField, the callsite pattern definePlugin, and the types
RuntimePlugin.result and MapModelFieldType in the doc/update decision.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 04add8d3-1256-4cfb-b353-9bcce9afbeca
📒 Files selected for processing (3)
packages/orm/src/client/client-impl.tspackages/orm/src/client/plugin.tstests/e2e/orm/plugin-infra/ext-result.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implement a new
resultextension point for plugins to declare computed fields on query results with full type safety and select/omit awareness. The feature includes recursive runtime computation, automatic dependency (needs) injection/stripping, and complete support for nested relations.Implementation
ExtResultFieldDefandExtResultBasetypes to plugin definitionsExtResultgeneric throughClientContract,ModelResult, and all CRUD return typesSelectAwareExtResulttype that respects select/omit clausesprepareArgsForExtResultandapplyExtResultruntime functions for nested relations$use,$unuse,$transaction, etc.)Testing
Usage Example
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Tests