feat: provide the client in the computed field context - #2789
Conversation
Computed field implementations receive (eb, { modelAlias }, args) and
cannot read per-client state such as the auth context set via $setAuth.
Pass the executing client in the context, mirroring what custom function
implementations already get through ZModelFunctionContext.
Also replaces the scattered 'as unknown as ClientContract' casts with a
single ClientImpl.$contract accessor.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe ORM exposes a typed ChangesClient-aware computed fields
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/orm/src/client/client-impl.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. packages/orm/src/client/executor/zenstack-query-executor.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. 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 |
…t test A bare binary comparison embedded by the boolean where-filter renders as a chained '=' — a syntax error on postgres (sqlite tolerates it). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
CI fix in 771e216: the new e2e test failed on the postgres matrix only. The computed field was written as a bare binary comparison ( Side note for maintainers: this is reproducible on dev with any boolean computed field written as a bare comparison plus a boolean where filter — the dialect could parenthesize computed expressions when embedding them. Happy to file it as a separate issue. |
| @@ -42,6 +43,9 @@ export abstract class BaseCrudDialect<Schema extends SchemaDef> { | |||
| constructor( | |||
There was a problem hiding this comment.
Hi @evgenovalov , I'm wondering if it's cleaner if we make it two overloaded constructor signature: one with schema/options, the other with client (from which schema and options can be derived).
There was a problem hiding this comment.
Done. The dialect now takes either the client or a schema/options pair. Schema and options come from the client when you pass one.
I used a tuple type instead of two constructor overloads, because super(...args) does not compile against an overloaded base and PostgresCrudDialect has to forward to super.
I also pass the client in the policy handler, the policy expression transformer and the soft-delete plugin — they all had one already. ResultProcessor keeps the schema/options form, it has no client and only transforms output values.
| // `computedArgs` is the query-time args object for a parameterized computed | ||
| // field (undefined otherwise); forwarded as the implementation's 3rd argument. | ||
| return computer(this.eb, { modelAlias }, computedArgs); | ||
| return computer(this.eb, { modelAlias, client: this.client }, computedArgs); |
There was a problem hiding this comment.
Should we assert this.client here? I believe it's always available in this path. If so, we can make the client field non-optional in ComputedFieldContext, right? @evgenovalov
There was a problem hiding this comment.
Yes. I added the assert and made client required in ComputedFieldContext.
The dialect field itself stays optional, because getCrudDialect is public and can still be called with schema/options. In our own code no such path reaches a computed field — neither the policy plugin nor the soft-delete plugin calls fieldRef, and ResultProcessor only transforms output values.
Thanks for finding this extra issue. Filing one would be great! |
…ontext - `BaseCrudDialect` / `getCrudDialect` now take either the client executing the queries (schema and options derived from it) or a standalone schema/options pair, instead of an extra optional `client` parameter - pass the client at the remaining construction sites (policy handler, policy expression transformer, soft-delete plugin); `ResultProcessor` stays on the standalone form as it has no client and only transforms output values - assert the client when evaluating a computed field and make `ComputedFieldContext.client` non-optional Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/orm/src/client/crud/dialects/base-dialect.ts`:
- Around line 1689-1694: The computed-field branch in fieldRef must wrap every
computer result with this.eb.parens(...) before returning it, while preserving
the existing client invariant and computedArgs forwarding. Add a PostgreSQL
regression test covering a computed binary comparison used in a where clause.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8100d281-5070-4e2f-bf76-75dec26c7536
📒 Files selected for processing (12)
packages/orm/src/client/crud/dialects/base-dialect.tspackages/orm/src/client/crud/dialects/index.tspackages/orm/src/client/crud/dialects/mysql.tspackages/orm/src/client/crud/dialects/postgresql.tspackages/orm/src/client/crud/operations/base.tspackages/orm/src/client/executor/name-mapper.tspackages/orm/src/client/executor/zenstack-query-executor.tspackages/orm/src/client/options.tspackages/plugins/policy/src/expression-transformer.tspackages/plugins/policy/src/policy-handler.tspackages/plugins/soft-delete/src/plugin.tstests/e2e/orm/client-api/computed-fields.test.ts
💤 Files with no reviewable changes (1)
- packages/orm/src/client/crud/dialects/mysql.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/orm/src/client/executor/name-mapper.ts
- packages/orm/src/client/crud/operations/base.ts
- packages/orm/src/client/options.ts
- tests/e2e/orm/client-api/computed-fields.test.ts
- packages/orm/src/client/executor/zenstack-query-executor.ts
| // every query issued through the ORM builds the dialect from a client, and a dialect | ||
| // built from a standalone schema/options pair never inlines computed fields | ||
| invariant(this.client, `computed field "${field}" of model "${model}" needs a client to be evaluated`); | ||
| // `computedArgs` is the query-time args object for a parameterized computed | ||
| // field (undefined otherwise); forwarded as the implementation's 3rd argument. | ||
| return computer(this.eb, { modelAlias }, computedArgs); | ||
| return computer(this.eb, { modelAlias, client: this.client }, computedArgs); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'fieldRef\(|computedFields|\.parens\(' packages/orm/src tests/e2e
rg -n -C 6 'authorId|computed.*where|where.*computed|\$setAuth' tests/e2eRepository: zenstackhq/zenstack
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="packages/orm/src/client/crud/dialects/base-dialect.ts"
printf '%s\n' '--- target implementation ---'
sed -n '1640,1710p' "$file"
printf '%s\n' '--- fieldRef declarations and call sites ---'
rg -n -C 5 'fieldRef' packages/orm/src --glob '*.ts' --glob '*.tsx' | head -n 240
printf '%s\n' '--- computed-field declarations and handlers ---'
rg -n -C 5 'computed|compute' packages/orm/src --glob '*.ts' --glob '*.tsx' | head -n 320
printf '%s\n' '--- parens API usage ---'
rg -n -C 3 '\.parens\(' packages/orm/src --glob '*.ts' --glob '*.tsx' | head -n 200Repository: zenstackhq/zenstack
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- relevant test files ---'
rg -l -i 'computed|`@computed`|computedField' tests/e2e packages --glob '*.test.ts' --glob '*.ts' --glob '*.zmodel' | head -n 120
printf '%s\n' '--- PostgreSQL SQL generation and dialect tests ---'
rg -l -i 'postgres|toSql|compile|query.sql|sql' packages/orm/src tests/e2e/orm --glob '*test.ts' | head -n 120
printf '%s\n' '--- package metadata and available dependencies ---'
find packages/orm -maxdepth 2 -type f \( -name 'package.json' -o -name 'pnpm-lock.yaml' \) -print
rg -n '"kysely"|"pg"' package.json packages/*/package.json packages/orm/package.json 2>/dev/null | head -n 80Repository: zenstackhq/zenstack
Length of output: 5817
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("packages/orm/src/client/crud/dialects/base-dialect.ts")
text = p.read_text()
for needle in ("fieldRef", "computedFields", "parens"):
print(f"--- {needle} ---")
for i, line in enumerate(text.splitlines(), 1):
if needle in line:
print(i, line.strip())
PY
printf '%s\n' '--- exact target context ---'
nl -ba packages/orm/src/client/crud/dialects/base-dialect.ts | sed -n '1650,1705p'Repository: zenstackhq/zenstack
Length of output: 4724
🏁 Script executed:
#!/bin/bash
set -euo pipefail
set +e
node - <<'JS'
try {
const kysely = require('kysely');
console.log('kysely available');
const { PostgresAdapter, PostgresQueryCompiler, PostgresIntrospector, Kysely, sql } = kysely;
const db = new Kysely({
dialect: {
createAdapter: () => new PostgresAdapter(),
createDriver: () => { throw new Error('not used'); },
createIntrospector: (db) => new PostgresIntrospector(db),
createQueryCompiler: () => new PostgresQueryCompiler(),
},
});
const expression = sql`"authorId" = ${sql.val(2)}`;
const ungrouped = db.selectFrom('Post').selectAll().where(expression, '=', 3).compile();
const grouped = db.selectFrom('Post').selectAll().where(sql`(${expression})`, '=', 3).compile();
console.log(JSON.stringify({ ungrouped, grouped }, null, 2));
} catch (e) {
console.log(`${e.name}: ${e.message}`);
}
JS
set -eRepository: zenstackhq/zenstack
Length of output: 236
🌐 Web query:
Kysely PostgresQueryCompiler nested binary operation expression parentheses where expression left operand
💡 Result:
In Kysely, nested binary operation expressions and the use of parentheses are handled primarily through the ExpressionBuilder's parens method [1][2]. When building queries, the PostgresQueryCompiler uses a visitor pattern, including a visitBinaryOperation method, to compile operation nodes into SQL [3]. However, Kysely does not automatically wrap all nested binary operations in parentheses by default to avoid unnecessary SQL syntax [4]. To control operator precedence or force grouping in your generated SQL, you must explicitly use the eb.parens method provided by the ExpressionBuilder [1][5]. Key behaviors regarding parentheses in Kysely include: 1. Explicit Wrapping: You can use eb.parens to wrap binary expressions or other expressions [1][2]. For example: const result = await db.selectFrom('person').where((eb) => eb(eb.parens('age', '+', 1), '/', 100), '<', 0.1).execute; This generates: where ("age" + $1) / $2 < $3 [1][5]. 2. Expression Composition: Since Kysely expressions are composable, you can pass nested expressions directly into eb.parens to manage complex logic [1][6]. For logical operations (like.and or.or), Kysely often handles the necessary parentheses automatically when combining filter lists [7][5]. 3. Compiler Logic: The PostgresQueryCompiler (in default-query-compiler.ts) manages parent-node awareness to decide if specific query structures (like subqueries or CTEs) require wrapping, but for general binary expression trees, the structure is determined by how you build the expression tree using the builder [8][9]. If you find that your generated SQL is missing required parentheses for a specific complex operation, the recommended approach is to wrap the sub-expression explicitly using the.parens builder method rather than relying on automatic compiler behavior [1][4].
Citations:
- 1: https://jsr.io/@kysely/kysely/doc/~/ExpressionBuilder.parens
- 2: https://kysely-org-kysely.mintlify.app/api/expression-builder
- 3: https://kysely-org.github.io/kysely-apidoc/classes/PostgresQueryCompiler.html
- 4: add support for nested
with...selectwhen using set operations (e.g.union). kysely-org/kysely#285 - 5: https://github.com/kysely-org/kysely/blob/master/src/expression/expression-builder.ts
- 6: https://kysely.dev/docs/recipes/reusable-helpers
- 7: https://github.com/kysely-org/kysely/blob/master/src/parser/binary-operation-parser.ts
- 8: https://github.com/kysely-org/kysely/blob/master/src/query-compiler/default-query-compiler.ts
- 9: fix: insert/update not being wrapped in parens when in CTE of a merge query. kysely-org/kysely#1611
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- computed-field tests ---'
sed -n '1,280p' tests/e2e/orm/client-api/computed-fields.test.ts
printf '%s\n' '--- PostgreSQL computed multischema tests ---'
sed -n '1,260p' tests/e2e/orm/client-api/pg-computed-multischema.test.ts
printf '%s\n' '--- computed-field type definitions ---'
sed -n '280,340p' packages/orm/src/client/options.ts
printf '%s\n' '--- package lock Kysely version ---'
rg -n -C 2 'kysely@|kysely:' pnpm-lock.yaml package.json packages/orm/package.json | head -n 100Repository: zenstackhq/zenstack
Length of output: 19375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- all direct computed handler definitions in tests and examples ---'
rg -n -C 8 'computedFields\s*[:=]|computedFields:' tests packages --glob '*.ts' --glob '*.tsx' --glob '*.js' | head -n 360
printf '%s\n' '--- expression-builder binary-expression implementation references ---'
rg -n 'eb\([^;]*[=<>!]|this\.eb\([^;]*[=<>!]|sql\.raw.*=' tests/e2e/orm/client-api/computed-fields.test.ts tests/e2e/orm packages/orm/test --glob '*.ts' | head -n 240Repository: zenstackhq/zenstack
Length of output: 29985
🌐 Web query:
site:github.com/kysely-org/kysely BinaryOperationNode compiler parentheses operator precedence
💡 Result:
In the context of the Kysely query builder, BinaryOperationNode is a specific node in the library's internal abstract syntax tree (AST) used to represent binary expressions (e.g., leftOperand operator rightOperand) [1][2]. Because these internal nodes do not automatically enforce operator precedence or inject parentheses based on mathematical rules, the library relies on an explicit ParensNode to handle operator precedence [3]. Key technical details include: 1. Handling Precedence: Parentheses are not managed implicitly by the BinaryOperationNode itself. Instead, developers must explicitly wrap expressions using an expression builder method (e.g., eb.parens), which creates a ParensNode in the AST [3]. 2. AST Representation: When the query is compiled into SQL, the query compiler visits the nodes recursively [4][5]. If a ParensNode is encountered, the compiler explicitly appends opening and closing parentheses before and after visiting the nested node [4][5]. 3. Compilation Process: The visitBinaryOperation method in the query compiler performs a straightforward concatenation of the left operand, the operator, and the right operand [4][6]. It does not perform any logic to determine if parentheses are required for correct SQL precedence; that responsibility is shifted to the user or the query construction logic [3]. This design prioritizes explicit control over expression structure, ensuring that the generated SQL follows the intended precedence defined by the user during the construction of the query [3].
Citations:
- 1: https://github.com/kysely-org/kysely/blob/e58d31b3/src/operation-node/binary-operation-node.ts
- 2: add
filterclause support atAggregateFunctionBuilder. kysely-org/kysely#208 - 3: https://github.com/kysely-org/kysely/blob/master/src/expression/expression-builder.ts
- 4: https://github.com/kysely-org/kysely/blob/master/src/query-compiler/default-query-compiler.ts
- 5: https://github.com/kysely-org/kysely/blob/33e2b398/src/query-compiler/default-query-compiler.ts
- 6: https://github.com/kysely-org/kysely/blob/e58d31b3/src/query-compiler/default-query-compiler.ts
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- existing boolean computed-field test ---'
sed -n '940,1045p' tests/e2e/orm/client-api/computed-fields.test.ts
printf '%s\n' '--- primitive boolean filter construction ---'
sed -n '640,710p' packages/orm/src/client/crud/dialects/base-dialect.ts
sed -n '1080,1130p' packages/orm/src/client/crud/dialects/base-dialect.ts
printf '%s\n' '--- current diff/stat and target-file history summary ---'
git diff --stat
git status --short
git log --oneline -8 -- packages/orm/src/client/crud/dialects/base-dialect.tsRepository: zenstackhq/zenstack
Length of output: 8951
Parenthesize computed expressions in fieldRef.
Kysely does not add parentheses around nested binary expressions. The existing boolean computed-field test avoids invalid SQL because its handler calls eb.parens directly. fieldRef should enforce this for all computed handlers by returning this.eb.parens(computer(...)). Add a PostgreSQL regression test with a computed binary comparison in where.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/orm/src/client/crud/dialects/base-dialect.ts` around lines 1689 -
1694, The computed-field branch in fieldRef must wrap every computer result with
this.eb.parens(...) before returning it, while preserving the existing client
invariant and computedArgs forwarding. Add a PostgreSQL regression test covering
a computed binary comparison used in a where clause.
Conflict in `zenstack-query-executor.ts`: dev's name mapper reuse (zenstackhq#2777) and this branch's `$contract` accessor touched the same constructor block. Kept dev's reuse logic and passed `client.$contract` instead of the cast. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
LGTM! I'm merging it and will publish a 3.9.1 release shortly. |
2 participants
Motivation
Computed field implementations receive
(eb, { modelAlias }, args). There is no way to read per-client state — most notably the auth context set with$setAuth. A field like "is this row mine" currently needs a runtime plugin to inject the user id into the query. Custom function implementations already get the client throughZModelFunctionContext; this PR gives computed fields the same access.Example
Changes
ComputedFieldContext<Schema>type ({ modelAlias, client? });ComputedFieldsOptionstypes the context parameter with it. Generated schemas are untouched, so existing implementations keep compiling.BaseCrudDialect/getCrudDialecttake an optionalclient, threaded from the CRUD operations, the query executor, and the name mapper.fieldRefpasses it into the context.clientis optional in the context because a dialect can be constructed without one (e.g.ResultProcessor); every query issued through the client API has it.as unknown as ClientContract<SchemaDef>casts inclient-impl.ts/zenstack-query-executor.tswith a singleClientImpl.$contractaccessor — same object at runtime, one assertion at one documented boundary.$setAuth; the base client stays unaffected.No breaking changes: the new parameters are optional and the context object only gains a property.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
$contractaccessor for safer client interactions.Bug Fixes
Tests