Add opt-in deep exact query argument checking#2720
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds opt-in recursive exact query-argument typing through ChangesExact query argument typing
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)
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 |
|
Hi @olup , thanks for making the fix! I'm not sure if it's by design, but I believe the improved EPC check only works for top-level select/omit/include. Adding recursion will probably incur significant type-checking cost, so I think it's a valid trade-off. Looking good to me. |
|
Great ! I'll have a quick look this morning on recursion/performance. |
3816813 to
f5fa064
Compare
|
(message produces by codex under human supervision) I updated the branch to address the recursion/performance concern with a different approach. Instead of using a fixed recursion depth, the new version only applies strict checking along the user-provided selection tree, and only for I also compared this with Drizzle's model. Drizzle's Quick local typechecking perf check:
Average e2e results:
I also checked Validation run locally:
|
|
Checking a couple of details on the where queries before reopening this one |
31aa89e to
371687e
Compare
|
(message produced by codex under human supervision) Small follow-up: I also checked the The selection recursion helper intentionally does not recursively walk arbitrary sub-objects like scalar filter payloads or sort payloads. However, I added explicit type tests for:
Implementation-wise, the patch now applies shallow exactness to Validation rerun:
|
|
Hi @olup , thanks for the update. I've been thinking about the issue and the trade-offs. So ideally, we should have a full recursive EPC for the entire query args. However, this can incur high extra type-checking costs due to the typing complexity of fields like I'm wondering if it's overall better to just have a shallow, strict check for all keys. Implementation is simpler and easier to reason, behavior is consistent, at the cost of losing deep EPC. However, since everything is strictly checked at runtime, and also for IDE experience, the language server's field recommendation is still fully contextual, maybe it's a reasonable trade-off? |
|
Hey - I have been away for a bit. To state back the context of the issue : I really beleive that the interesting thing about a typed ORM is letting you rework your schema with the confidence of your type system catching any mistake. What happens now in ts 6+ is that the argument of the query are not properly typed - meaning a change in your schema can lead to an invalid query failing at run time. To me this feels completely impossible to accpet. Now should the argument typing be only on top level? I underdtand the tradeoff but from a user perspective I would ask why ? If I use narrow selects on relationship, and a property is gone, I would 100% expect zenstack to error at compile time. I think prisma succeed in this way by generating types in codegen, but Drizzle managed it with type only. If there is any way to achieve a light recursive typing of the orm query arguments we should absolutely do it, don't you think ? My present proposal might not be it, but we should explore this - especially since ts7 is so much more efficient. |
|
So here are my explorations: The key observation is that the full schema/query-argument graph does not need to be recursively expanded. Recursion can follow only the keys present in the user-provided input, making it naturally bounded by that input without an arbitrary depth limit. here are some metrics taken from running the test suite before and after my ongiong explorations (not what's in the PR RN)
This is the most complete version I could come up with. I think its necessary in terms of feature but if the cost is too large, I ma told there is also the possibility to make it an option on the client. User could activate this if they want complete typing of queries in ts7. example: |
|
The activation flag is working. I think this could make a interesting design - complicated project can get a higher overhead, but gaining strict checking in the process. The tradeoff could be the user choice. I'll update the PR with this design. Would it be interesting @ymc9 ? |
371687e to
add2903
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/orm/src/utils/type-utils.ts (1)
99-105: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winEmpty
Keyscollapses the whole object tonever, even whenTis also empty.When
Shapehas zero keys,_NoExtraObjectreturns a bareneverfor the whole type instead of a mapped object. This is stricter than necessary: the mapped branch already rejects every key ofTwhenKeysisnever(sinceK extends Keysis false for everyK), but returning bareneveralso rejects the vacuous case whereTitself has no keys (T = {}), becauseneverintersected intoSubset/SelectSubsetcollapses the entire args type tonever. Dropping the special case simplifies the code and fixes this over-rejection.♻️ Proposed simplification
-type _NoExtraObject<T extends object, Shape, Keys extends PropertyKey = _ObjectKeys<Shape>> = [Keys] extends [never] - ? never - : string extends Keys - ? unknown - : { - [K in keyof T]: K extends Keys ? NoExtraProperties<T[K], _ObjectValue<Shape, K>> : never; - }; +type _NoExtraObject<T extends object, Shape, Keys extends PropertyKey = _ObjectKeys<Shape>> = string extends Keys + ? unknown + : { + [K in keyof T]: K extends Keys ? NoExtraProperties<T[K], _ObjectValue<Shape, K>> : never; + };🤖 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/utils/type-utils.ts` around lines 99 - 105, Update the _NoExtraObject type to remove the special-case branch that returns never when Keys extends never. Always use the mapped-object branch so an empty T remains valid while any keys in T are still rejected by the K extends Keys condition; preserve the existing string-key handling.
🤖 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/options.ts`:
- Around line 142-152: Update the JSDoc for typing.exactQueryArgs in the options
definition to describe its current limited coverage: it applies to create,
update, and upsert argument shapes, rather than recursively rejecting unknown
properties across all query arguments. Do not imply enforcement for read,
delete, or aggregate operations until those overloads propagate Options.
---
Nitpick comments:
In `@packages/orm/src/utils/type-utils.ts`:
- Around line 99-105: Update the _NoExtraObject type to remove the special-case
branch that returns never when Keys extends never. Always use the mapped-object
branch so an empty T remains valid while any keys in T are still rejected by the
K extends Keys condition; preserve the existing string-key handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e08d5665-02ff-4bcf-8e4b-d1bdb366b47f
📒 Files selected for processing (5)
packages/orm/src/client/contract.tspackages/orm/src/client/crud-types.tspackages/orm/src/client/options.tspackages/orm/src/utils/type-utils.tstests/e2e/orm/schemas/typing/typecheck.ts
|
Hi @olup , thanks for the follow up. I agree that opt-in is a good idea. Could you check the comment from @coderabbitai? Currently the option is only wired to the write APIs but not read. After including read path for recursive epc check, my observation is about 30% increase in type instantiation and memory usage (TS 6). Not too bad but for sure justifies the default-false opt-in. |
|
Tip For best results, initiate chat on the files or code changes.
Your TS 6 measurement—roughly +30% instantiations and memory once read-path recursive EPC is included—makes the default-false opt-in especially important. That preserves existing performance for most users while allowing projects that prioritize schema-refactor safety to explicitly choose stricter validation. 🐇✅ ✏️ Learnings added
|
|
Of course - I wanted your opinion first before taking more attention on this. Will work a proper pr in the following days |
|
The read path and the remaining delete and analytical operations are now wired to the recursive guard. The empty expected-shape edge case reported by CodeRabbit is fixed as well. A fresh three-run A/B on the identical final corpus gives:
The current input-key-driven implementation therefore does not reproduce the approximate 30% TS6 memory/instantiation increase. TS7 timing rises more in relative terms, but by 0.215 seconds on this corpus. The option remains default-false because workload shape can change the cost. |
Summary
Fixes #2719 by adding an opt-in exactness check for inferred query arguments:
With the option enabled, unknown properties are rejected recursively in inline and hoisted arguments across read, create, update, delete, count, aggregate, groupBy, and exists operations. Covered structures include nested relations, arrays,
where,select,include,orderBy,cursor,connect,connectOrCreate, and nested mutation branches.Design
NoExtraProperties<T, Shape>walks only the keys present in the argument inferred from user code. Expected input unions are consulted property by property, so the checker does not eagerly rebuild every possible schema branch. There is no arbitrary recursion-depth cap.The check preserves property-local diagnostics and treats runtime leaf/open-value types such as
Date,Decimal,Uint8Array, functions, and JSON-style open records appropriately. The empty finite-shape case also preserves a valid{}while continuing to reject supplied unknown keys.The option remains disabled by default because the type-checking cost depends on schema and query complexity. There are no runtime client changes.
Performance
Measured on Node 26.5 using the same final e2e corpus with exactness off and on. Values are medians of three isolated sequential runs per state.
The relative TS7 timing increase is larger than the structural deltas, but the absolute median check-time increase is 0.215 seconds on this corpus. The cost is workload-dependent, which is why the feature remains opt-in.
Validation
pnpm --filter @zenstackhq/orm buildpnpm --filter e2e test:typecheck(TypeScript 6.0.3)pnpm exec tsgo -p tests/e2e/tsconfig.json --noEmit --pretty false(TS7 native preview)pnpm --filter @zenstackhq/orm lintSummary by CodeRabbit
New Features
typing.exactQueryArgs) for stricter TypeScript validation.selectvsinclude/omit).Tests