feat(orm): add result plugin extension point#2442
Open
genu wants to merge 13 commits intozenstackhq:devfrom
Open
feat(orm): add result plugin extension point#2442genu wants to merge 13 commits intozenstackhq:devfrom
genu wants to merge 13 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: type-level ExtResult propagation, runtime arg preparation/injection for ext dependencies, post-processing to compute ext fields, Zod schema exposure for ext selections, plugin API extensions, and comprehensive e2e tests and fixtures. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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.
| * }); | ||
| * ``` | ||
| */ | ||
| 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.
| if (selectedExtResultFields !== undefined && !selectedExtResultFields.has(fieldName)) { | ||
| continue; | ||
| } | ||
| const needsSatisfied = Object.keys(fieldDef.needs).every((needField) => needField in data); |
Member
There was a problem hiding this comment.
Just thought of an interesting feature interaction here. If field-level access policy caused a "needed" field to be dropped, the evaluation will be muted here. It's probably a reasonable behavior though.
Contributor
Author
There was a problem hiding this comment.
Good catch — yes, if a policy strips a needed field, needsSatisfied will be false and the virtual field is silently omitted from the result. I think this is the right default: computing with missing data would produce garbage, and throwing would be too disruptive. If we ever want to surface this, a dev-mode warning would be the natural place
| 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
|
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
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