-
-
Notifications
You must be signed in to change notification settings - Fork 12
chore: cache validation schemas #18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe updates introduce schema caching for input validation in the ORM runtime, refactoring the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ClientProxy
participant InputValidator
participant ResultProcessor
participant ModelCrudHandler
Client->>ClientProxy: createClientProxy()
ClientProxy->>InputValidator: instantiate (once)
ClientProxy->>ResultProcessor: instantiate (once)
ClientProxy->>ModelCrudHandler: createModelCrudHandler(..., InputValidator, ResultProcessor)
Client->>ModelCrudHandler: Perform operation (e.g., find, create)
ModelCrudHandler->>InputValidator: validate(args)
InputValidator->>InputValidator: fetch/create schema from cache
InputValidator-->>ModelCrudHandler: validated args
ModelCrudHandler->>ResultProcessor: process result (if needed)
ResultProcessor-->>ModelCrudHandler: processed result
ModelCrudHandler-->>Client: result
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/runtime/package.json (1)
86-99: 💡 Verification agent🧩 Analysis chain
Add accompanying
@types/json-stable-stringifyto avoid TS compilation errors
json-stable-stringifyis published without bundled typings.
Unless the project globally enablesskipLibCheck, importing it (seevalidator.ts) will break the build."devDependencies": { + "@types/json-stable-stringify": "^1.0.0",Please run
pnpm tsc -bafter adding the types – the build should succeed withoutskipLibCheck.
🏁 Script executed:
#!/bin/bash echo "== Checking skipLibCheck settings ==" rg -n skipLibCheck . echo echo "== Checking for @types/json-stable-stringify in devDependencies ==" rg -n "@types/json-stable-stringify" packages/runtime/package.json || echo "Not found" echo echo "== Checking for json-stable-stringify in dependencies ==" rg -n "json-stable-stringify" packages/runtime/package.json echo echo "== Searching for imports of json-stable-stringify ==" rg -n "json-stable-stringify" -t tsLength of output: 972
Add missing
@types/json-stable-stringifydev dependencyAlthough
skipLibCheckis enabled in your root tsconfig, TypeScript still reports a “Cannot find module 'json-stable-stringify'” error (TS7016) when no declaration file exists. To eliminate this, add the official types package:• File: packages/runtime/package.json
• Section:devDependencies"devDependencies": { + "@types/json-stable-stringify": "^1.0.0",After adding, run
pnpm tsc -bto confirm the build passes without missing‐module errors.
🧹 Nitpick comments (3)
TODO.md (1)
56-60: Markdown list indentation violates MD007Indentation for the newly-checked item is 8 spaces instead of 4, tripping
markdownlint.- - [x] Cache validation schemas + - [x] Cache validation schemas🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
56-56: Unordered list indentation
Expected: 4; Actual: 8(MD007, ul-indent)
57-57: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
58-58: Unordered list indentation
Expected: 4; Actual: 8(MD007, ul-indent)
59-59: Unordered list indentation
Expected: 4; Actual: 8(MD007, ul-indent)
60-60: Unordered list indentation
Expected: 4; Actual: 8(MD007, ul-indent)
packages/runtime/src/client/client-impl.ts (1)
306-309: Potential memory growth via sharedResultProcessor
ResultProcessor.processResultkeeps internal caches (see implementation).
With the same instance now shared by every query, its cache could grow unbounded during a long-lived process (e.g. server).
Consider adding an eviction policy or exposing aclear()API that users can hook into between requests.packages/runtime/src/client/crud/validator.ts (1)
183-205: Schema cache lacks eviction – risk of unbounded memory usage
schemaCacheis a plainMapthat only ever grows.
Long-running services executing a large variety of models/operations/options will keep every generatedZodSchemaforever.Consider an LRU cache with a sane upper bound (e.g. 100-500 entries):
- private schemaCache = new Map<string, ZodSchema>(); + private readonly schemaCache = new Map<string, ZodSchema>(); + private static readonly MAX_CACHE_SIZE = 256; ... - if (!schema) { + if (!schema) { schema = getSchema(model, options); - this.schemaCache.set(cacheKey!, schema); + // simple LRU eviction + if (this.schemaCache.size >= InputValidator.MAX_CACHE_SIZE) { + const oldestKey = this.schemaCache.keys().next().value; + this.schemaCache.delete(oldestKey); + } + this.schemaCache.set(cacheKey!, schema); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
TODO.md(1 hunks)packages/runtime/package.json(1 hunks)packages/runtime/src/client/client-impl.ts(3 hunks)packages/runtime/src/client/crud/validator.ts(5 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
TODO.md
58-58: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-test (20.x)
🔇 Additional comments (2)
packages/runtime/src/client/client-impl.ts (1)
251-270: Nice centralisation of validator / processor instancesCreating the
InputValidator&ResultProcessoronce per client avoids thousands of schema rebuilds – good call.
No functional issues spotted here.packages/runtime/src/client/crud/validator.ts (1)
1-4: Missing type definitions forjson-stable-stringifyThe default export is CommonJS; without
@types/json-stable-stringifythe import will produceTS7016.
See package-level comment for the proposed dev-dependency addition.
| validateFindArgs(model: GetModels<Schema>, unique: boolean, args: unknown) { | ||
| return this.validate<FindArgs<Schema, GetModels<Schema>, true>>( | ||
| this.makeFindSchema(model, unique, true), | ||
| return this.validate< | ||
| FindArgs<Schema, GetModels<Schema>, true>, | ||
| Parameters<typeof this.makeFindSchema>[1] | ||
| >( | ||
| model, | ||
| 'find', | ||
| { unique, collection: true }, | ||
| (model, options) => this.makeFindSchema(model, options), | ||
| args | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
collection flag hard-coded to true
validateFindArgs always sets collection: true, enabling skip/take/orderBy even for findUnique and findFirst.
If this is unintended and parity with Prisma is desired, derive the flag from the concrete operation:
- { unique, collection: true },
+ { unique, collection: !unique }, // findMany → true, findUnique/First → falseDouble-check all call-sites (e.g. FindOperationHandler) after the signature change.
I’d like to inspect how makeFindSchema and the other validateFind* helpers use the collection flag. Let me run a few searches in validator.ts:
🏁 Script executed:
#!/bin/bash
# Locate validateFind* methods and makeFindSchema in the validator
rg -n "validateFindArgs" packages/runtime/src/client/crud/validator.ts
rg -n "validateFindUniqueArgs" packages/runtime/src/client/crud/validator.ts || true
rg -n "validateFindFirstArgs" packages/runtime/src/client/crud/validator.ts || true
rg -n "makeFindSchema" -n -A5 -B5 packages/runtime/src/client/crud/validator.tsLength of output: 1325
I’ll inspect the full makeFindSchema implementation and how options.collection is used:
🏁 Script executed:
#!/bin/bash
# Show the body of makeFindSchema
sed -n '207,350p' packages/runtime/src/client/crud/validator.ts
# Highlight where options.collection is referenced
rg -n "options.collection" -A5 -B5 packages/runtime/src/client/crud/validator.tsLength of output: 6229
Derive the collection flag from unique
In packages/runtime/src/client/crud/validator.ts (around lines 52–58), the call to makeFindSchema always passes collection: true, which allows pagination and ordering on unique queries (findUnique). To align with Prisma semantics, update it to:
- { unique, collection: true },
+ { unique, collection: !unique }, // collections only for non‐unique queriesThis change will disable skip/take/orderBy for findUnique while still permitting them for findMany and findFirst. After applying, review any call-sites of validateFindArgs to ensure your higher-level handlers continue to work correctly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| validateFindArgs(model: GetModels<Schema>, unique: boolean, args: unknown) { | |
| return this.validate<FindArgs<Schema, GetModels<Schema>, true>>( | |
| this.makeFindSchema(model, unique, true), | |
| return this.validate< | |
| FindArgs<Schema, GetModels<Schema>, true>, | |
| Parameters<typeof this.makeFindSchema>[1] | |
| >( | |
| model, | |
| 'find', | |
| { unique, collection: true }, | |
| (model, options) => this.makeFindSchema(model, options), | |
| args | |
| ); | |
| } | |
| validateFindArgs(model: GetModels<Schema>, unique: boolean, args: unknown) { | |
| return this.validate< | |
| FindArgs<Schema, GetModels<Schema>, true>, | |
| Parameters<typeof this.makeFindSchema>[1] | |
| >( | |
| model, | |
| 'find', | |
| - { unique, collection: true }, | |
| + { unique, collection: !unique }, // collections only for non‐unique queries | |
| (model, options) => this.makeFindSchema(model, options), | |
| args | |
| ); | |
| } |
🤖 Prompt for AI Agents
In packages/runtime/src/client/crud/validator.ts around lines 47 to 58, the
validateFindArgs method currently hard-codes the collection flag to true when
calling makeFindSchema, which incorrectly enables pagination and ordering
options for unique queries like findUnique. To fix this, derive the collection
flag from the unique parameter by setting collection to !unique, so that
collection is false for unique queries and true otherwise. After making this
change, verify all call-sites of validateFindArgs to ensure they handle the
updated signature correctly.
Summary by CodeRabbit
Refactor
Chores