fix: orderBy issue when used with groupBy, add zod cache and $connect API #317
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRefactors query ordering and group-by construction, replaces per-instance validator cache with a global WeakMap, adds an eager Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Multiple structural refactors (query construction, global caching, executor constructor changes) plus API additions and cascading call-site updates require careful cross-file review. Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
Claude finished @ymc9's task —— View job Code Review for PR #317✅ OverviewThis PR addresses three main improvements:
📋 Code Quality and Best Practices✅ Strengths
|
| Category | Rating | Notes |
|---|---|---|
| Code Quality | ⭐⭐⭐⭐⭐ | Clean, well-structured implementation |
| Security | ⭐⭐⭐⭐⭐ | No security concerns identified |
| Performance | ⭐⭐⭐⭐⭐ | Significant improvements with caching |
| Test Coverage | ⭐⭐⭐⭐☆ | Good coverage, minor additions suggested |
| Documentation | ⭐⭐⭐⭐☆ | Code is self-documenting, some comments could help |
✅ Recommendation: APPROVE
This is a well-implemented PR that addresses real performance and functionality issues. The orderBy fix resolves a legitimate bug, the caching implementation provides meaningful performance improvements, and the $connect API adds useful functionality. The code quality is high and follows established patterns.
Key Strengths:
- Fixes actual groupBy orderBy issues
- Implements efficient validation caching
- Maintains type safety and existing patterns
- Comprehensive test coverage for critical paths
Minor suggestions for future consideration:
- Monitor cache performance metrics
- Consider adding inline documentation for complex logic
- Benchmark cache efficiency in production scenarios
There was a problem hiding this comment.
Pull Request Overview
Fixes groupBy orderBy pagination behavior, introduces shared Zod schema cache, and adds a $connect API to proactively initialize the DB connection.
- Corrects groupBy to respect provided orderBy and removes implicit default ordering when paginating
- Adds a WeakMap-based Zod schema cache keyed by schema to reduce validator rebuilds
- Introduces client.$connect() and makes name mapping conditional to reduce overhead
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e/orm/client-api/group-by.test.ts | Updates expectations to reflect new groupBy ordering behavior. |
| tests/e2e/orm/client-api/find.test.ts | Adds explicit orderBy for deterministic pagination. |
| packages/runtime/src/client/executor/zenstack-query-executor.ts | Makes name mapping optional and adds a helper to safely transform query nodes. |
| packages/runtime/src/client/crud/validator/index.ts | Adds shared Zod schema cache across clients keyed by SchemaDef, updates cache keys. |
| packages/runtime/src/client/crud/operations/group-by.ts | Refactors groupBy pipeline to apply grouping before pagination and fixes orderBy/aliasing. |
| packages/runtime/src/client/crud/operations/aggregate.ts | Aligns buildOrderBy calls with updated signature. |
| packages/runtime/src/client/crud/dialects/base-dialect.ts | Removes default ordering fallback and simplifies buildOrderBy signature and behavior. |
| packages/runtime/src/client/contract.ts | Adds $connect contract and updates disconnect doc. |
| packages/runtime/src/client/client-impl.ts | Implements $connect by acquiring and releasing a connection. |
| packages/language/src/utils.ts | Minor cleanup in getAuthDecl. |
| packages/language/res/stdlib.zmodel | Corrects function docs and comments out the search() stdlib function. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
packages/runtime/src/client/executor/zenstack-query-executor.ts
Outdated
Show resolved
Hide resolved
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/runtime/src/client/crud/validator/index.ts (1)
1417-1429: groupBy.orderBy validation breaks whenorderByis an array.
Object.keys(value.orderBy)assumes an object; arrays yield indices ('0','1',...) and cause false failures. Normalize to an array and validate each element.-// fields used in `orderBy` must be either in the `by` list, or aggregations -schema = schema.refine((value) => { - const bys = typeof value.by === 'string' ? [value.by] : value.by; - if ( - value.orderBy && - Object.keys(value.orderBy) - .filter((f) => !AGGREGATE_OPERATORS.includes(f as AGGREGATE_OPERATORS)) - .some((key) => !bys.includes(key)) - ) { - return false; - } else { - return true; - } -}, 'fields in "orderBy" must be in "by"'); +// fields used in `orderBy` must be either in the `by` list, or aggregations +schema = schema.refine((value) => { + const bys = typeof value.by === 'string' ? [value.by] : value.by; + if (!value.orderBy) return true; + const orderBys = Array.isArray(value.orderBy) ? value.orderBy : [value.orderBy]; + const invalid = orderBys.some((ob: Record<string, unknown>) => + Object.keys(ob) + .filter((k) => !AGGREGATE_OPERATORS.includes(k as AGGREGATE_OPERATORS)) + .some((k) => !bys.includes(k)) + ); + return !invalid; +}, 'fields in "orderBy" must be in "by"');packages/runtime/src/client/executor/zenstack-query-executor.ts (1)
185-193: Critical: hooks and entity loading receive physical table names instead of logical model names.You're computing
mutationInfofromfinalQuery(after name mapping), andcallAfterMutationHooksrecomputes the model from the mapped AST. This leaks mapped table names into:
- plugin hooks (
modelarg),loadEntities(selectFrom(model)), risking mismatch with typed model names and breaking hook contracts.Fix by deriving
mutationInfofrom the unmapped query and passing that through, and by not recomputing the model incallAfterMutationHooks. Minimal diffs:@@ - if ( - (InsertQueryNode.is(query) || UpdateQueryNode.is(query)) && - this.hasEntityMutationPluginsWithAfterMutationHooks - ) { + if ( + (InsertQueryNode.is(query) || UpdateQueryNode.is(query)) && + this.hasEntityMutationPluginsWithAfterMutationHooks + ) { // ensure returning all fields ... query = { ...query, returning: ReturningNode.create([SelectionNode.createSelectAll()]), }; } - const finalQuery = this.processNameMapping(query); + // derive mutation info from the UNMAPPED query to preserve logical model names + const mutationInfo = this.getMutationInfo(query as MutationQueryNode); + + const finalQuery = this.processNameMapping(query); compiled = this.compileQuery(finalQuery); @@ - const mutationInfo = this.getMutationInfo(finalQuery); + // keep using previously derived mutationInfo (logical model names)And in
callAfterMutationHooks:@@ - const mutationModel = this.getMutationModel(queryNode); + // use logical model name determined before mapping + const mutationModel = mutationInfo.model; @@ - await hook({ - model: mutationModel, - action: mutationInfo.action, - queryNode, - loadAfterMutationEntities, - client, - queryId, - }); + await hook({ + model: mutationModel, + action: mutationInfo.action, + queryNode, + loadAfterMutationEntities, + client, + queryId, + });This keeps hooks and pre/post-entity loading aligned with schema model names while still compiling/executing the mapped SQL.
Also applies to: 206-211, 218-227, 241-251
packages/runtime/src/client/crud/dialects/base-dialect.ts (2)
91-106: Cursor pagination bug: ORDER BY must match cursor filter (default vs none).When args.orderBy is undefined, buildCursorFilter (Line 167) computes a default order, but buildOrderBy (Line 92) now returns the query unchanged. Cursor predicates without a matching ORDER BY are incorrect and can yield unstable pages and duplicates across page boundaries; negative take reversal also becomes a no-op without an ORDER BY.
Proposed fix: use the same effective order used by the cursor when present.
- // orderBy - result = this.buildOrderBy(result, model, modelAlias, args.orderBy, negateOrderBy); + // orderBy: must align with cursor filter for correctness + const effectiveOrderBy = + args.orderBy ?? (args.cursor ? makeDefaultOrderBy(this.schema, model) : undefined); + result = this.buildOrderBy(result, model, modelAlias, effectiveOrderBy, negateOrderBy);
845-854: Alias collision for to-one relation ordering.leftJoin uses the model name as alias (e.g., "User"). With multiple relations to the same target model (author, editor -> User), joins collide and produce incorrect predicates and ambiguous references. Use a field-scoped alias and propagate it.
- result = result.leftJoin(relationModel, (join) => { - const joinPairs = buildJoinPairs(this.schema, model, modelAlias, field, relationModel); + const relAlias = `${modelAlias}$${field}$order`; + result = result.leftJoin(`${relationModel} as ${relAlias}`, (join) => { + const joinPairs = buildJoinPairs(this.schema, model, modelAlias, field, relAlias); return join.on((eb) => this.and( ...joinPairs.map(([left, right]) => eb(this.eb.ref(left), '=', this.eb.ref(right))), ), ); }); - result = this.buildOrderBy(result, fieldDef.type, relationModel, value, negated); + result = this.buildOrderBy(result, fieldDef.type, relAlias, value, negated);
🧹 Nitpick comments (10)
packages/language/src/utils.ts (1)
446-450: Consider consistent defensive checks across both lookups.Line 448 adds a runtime type guard
(isDataModel(d) || isTypeDef(d))before accessingd.name, while line 446 directly callshasAttribute(d, '@@auth')without such guards. Sincedeclsis already statically typed as(DataModel | TypeDef)[], the type guard on line 448 is defensive but creates an inconsistency.For uniformity, either:
- Add the same guard to line 446, or
- Remove it from line 448 if the static type guarantees are sufficient
Apply this diff for consistency (option 1 - add guard to line 446):
- let authModel = decls.find((d) => hasAttribute(d, '@@auth')); + let authModel = decls.find((d) => (isDataModel(d) || isTypeDef(d)) && hasAttribute(d, '@@auth'));Or apply this diff (option 2 - remove guard from line 448):
- authModel = decls.find((d) => (isDataModel(d) || isTypeDef(d)) && d.name === 'User'); + authModel = decls.find((d) => d.name === 'User');packages/language/res/stdlib.zmodel (1)
554-557: Good parameter name improvement.Renaming the parameter from
regextopatternimproves clarity and avoids the naming conflict where the parameter had the same name as the function.packages/runtime/src/client/crud/validator/index.ts (4)
195-203: DRY the cache map creation.
getSchemaCacherepeats the “get-or-create Map” logic thatsetSchemaCachealso has. Factor it out to avoid divergence.+private getOrCreateSchemaCache(): Map<string, ZodType> { + let map = schemaCache.get(this.schema); + if (!map) { + map = new Map<string, ZodType>(); + schemaCache.set(this.schema, map); + } + return map; +} private getSchemaCache(cacheKey: string) { - let thisCache = schemaCache.get(this.schema); - if (!thisCache) { - thisCache = new Map<string, ZodType>(); - schemaCache.set(this.schema, thisCache); - } - return thisCache.get(cacheKey); + return this.getOrCreateSchemaCache().get(cacheKey); }
204-212:setSchemaCacheshould returnvoid(avoid returningMap).
Map#setreturns the map, but callers don’t use it. Make intent explicit and consistent.-private setSchemaCache(cacheKey: string, schema: ZodType) { - let thisCache = schemaCache.get(this.schema); - if (!thisCache) { - thisCache = new Map<string, ZodType>(); - schemaCache.set(this.schema, thisCache); - } - return thisCache.set(cacheKey, schema); -} +private setSchemaCache(cacheKey: string, schema: ZodType): void { + const cache = this.getOrCreateSchemaCache(); + cache.set(cacheKey, schema); +}
220-231: Cache key looks good; drop non‑null assertions and type the key.
json-stable-stringifyreturnsstring. The!is unnecessary and obscures intent.-const cacheKey = stableStringify({ +const cacheKey: string = stableStringify({ type: 'model', model, operation, options, extraValidationsEnabled: this.extraValidationsEnabled, }); -let schema = this.getSchemaCache(cacheKey!); +let schema = this.getSchemaCache(cacheKey); if (!schema) { schema = getSchema(model, options); - this.setSchemaCache(cacheKey!, schema); + this.setSchemaCache(cacheKey, schema); }
316-321: Typedef cache key is solid; also remove unnecessary!.Keying by
{ type: 'typedef', name, extraValidationsEnabled }under per‑schema cache is correct. Minor cleanup: avoid!and explicitly type the key.-const key = stableStringify({ +const key: string = stableStringify({ type: 'typedef', name: type, extraValidationsEnabled: this.extraValidationsEnabled, }); -let schema = this.getSchemaCache(key!); +let schema = this.getSchemaCache(key); ... -this.setSchemaCache(key!, schema); +this.setSchemaCache(key, schema);Also applies to: 343-344
packages/runtime/src/client/executor/zenstack-query-executor.ts (2)
58-61: Tighten mapped-name detection and simplify typing.
- Minor: method-level generic on
schemaHasMappedNames<Schema extends SchemaDef>shadows the class generic and adds noise.- Detection currently returns true on any
@@mapattr (even without anameargument) and includestypeDefsalthoughQueryNameMapperonly maps models/fields.Suggested cleanup:
- private schemaHasMappedNames<Schema extends SchemaDef>(schema: Schema) { - const hasMapAttr = (decl: ModelDef | TypeDefDef) => { - if (decl.attributes?.some((attr) => attr.name === '@@map')) { - return true; - } - return Object.values(decl.fields).some((field) => field.attributes?.some((attr) => attr.name === '@map')); - }; - return Object.values(schema.models).some(hasMapAttr) || Object.values(schema.typeDefs ?? []).some(hasMapAttr); - } + private schemaHasMappedNames(schema: SchemaDef) { + const hasMapAttr = (decl: ModelDef) => + !!decl.attributes?.some((a) => a.name === '@@map') || + Object.values(decl.fields).some((f) => f.attributes?.some((a) => a.name === '@map')); + return Object.values(schema.models).some(hasMapAttr); + }If you plan to support TypeDef mapping at the SQL layer, extend
QueryNameMapperaccordingly and keep the broader detection.Also applies to: 63-72
107-117: Improve error message to include the actually executed SQL.
executeQuerywraps errors usingcompiledQuery?.sqlfrom the outer call, which may differ from the SQL compiled fromfinalQuery. Consider capturing and loggingcompiled?.sqlfrom the actual compile step for accuracy.packages/runtime/src/client/crud/dialects/base-dialect.ts (2)
739-758: Avoid shadowing imported buildFieldRef; minor cleanup.The local const buildFieldRef masks the imported symbol and the class method name, hurting readability. Rename the local helper and update call sites.
- const buildFieldRef = (model: string, field: string, modelAlias: string) => { + const resolveFieldRef = (model: string, field: string, modelAlias: string) => { const fieldDef = requireField(this.schema, model, field); return fieldDef.originModel ? this.fieldRef(fieldDef.originModel, field, fieldDef.originModel) : this.fieldRef(model, field, modelAlias); }; @@ - result = result.orderBy( - (eb) => aggregate(eb, buildFieldRef(model, k, modelAlias), field as AGGREGATE_OPERATORS), + result = result.orderBy( + (eb) => aggregate(eb, resolveFieldRef(model, k, modelAlias), field as AGGREGATE_OPERATORS), sql.raw(this.negateSort(v, negated)), ); @@ - result = result.orderBy( - (eb) => eb.fn.count(buildFieldRef(model, k, modelAlias)), + result = result.orderBy( + (eb) => eb.fn.count(resolveFieldRef(model, k, modelAlias)), sql.raw(this.negateSort(v, negated)), ); @@ - const fieldRef = buildFieldRef(model, field, modelAlias); + const fieldRef = resolveFieldRef(model, field, modelAlias);Also applies to: 770-801
766-789: Remove unreachable duplicate _count handling._orderBy aggregations are already handled in the earlier branch (including _count). The subsequent switch case for '_count' is dead code. Delete it to reduce confusion.
- switch (field) { - case '_count': { - invariant(value && typeof value === 'object', 'invalid orderBy value for field "_count"'); - for (const [k, v] of Object.entries<string>(value)) { - invariant(v === 'asc' || v === 'desc', `invalid orderBy value for field "${field}"`); - result = result.orderBy( - (eb) => eb.fn.count(buildFieldRef(model, k, modelAlias)), - sql.raw(this.negateSort(v, negated)), - ); - } - continue; - } - default: - break; - } + // no-op: aggregations already handled above
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/language/res/stdlib.zmodel(3 hunks)packages/language/src/utils.ts(1 hunks)packages/runtime/src/client/client-impl.ts(1 hunks)packages/runtime/src/client/contract.ts(1 hunks)packages/runtime/src/client/crud/dialects/base-dialect.ts(3 hunks)packages/runtime/src/client/crud/operations/aggregate.ts(1 hunks)packages/runtime/src/client/crud/operations/group-by.ts(1 hunks)packages/runtime/src/client/crud/validator/index.ts(5 hunks)packages/runtime/src/client/executor/zenstack-query-executor.ts(6 hunks)tests/e2e/orm/client-api/find.test.ts(5 hunks)tests/e2e/orm/client-api/group-by.test.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/language/src/utils.tspackages/runtime/src/client/client-impl.tspackages/runtime/src/client/crud/validator/index.tstests/e2e/orm/client-api/group-by.test.tstests/e2e/orm/client-api/find.test.tspackages/language/res/stdlib.zmodelpackages/runtime/src/client/crud/operations/group-by.tspackages/runtime/src/client/contract.tspackages/runtime/src/client/crud/operations/aggregate.tspackages/runtime/src/client/crud/dialects/base-dialect.tspackages/runtime/src/client/executor/zenstack-query-executor.ts
tests/e2e/**
📄 CodeRabbit inference engine (CLAUDE.md)
End-to-end tests must live under
tests/e2e/
Files:
tests/e2e/orm/client-api/group-by.test.tstests/e2e/orm/client-api/find.test.ts
🧬 Code graph analysis (4)
packages/language/src/utils.ts (2)
packages/sdk/src/model-utils.ts (1)
hasAttribute(59-64)packages/language/src/generated/ast.ts (2)
isDataModel(391-393)isTypeDef(737-739)
packages/runtime/src/client/crud/validator/index.ts (3)
packages/sdk/src/schema/schema.ts (1)
SchemaDef(10-18)packages/runtime/src/client/crud/operations/base.ts (2)
schema(90-92)options(94-96)packages/runtime/src/client/executor/zenstack-query-executor.ts (1)
options(78-80)
packages/runtime/src/client/crud/operations/group-by.ts (1)
packages/runtime/src/client/crud/dialects/base-dialect.ts (1)
fieldRef(1073-1075)
packages/runtime/src/client/executor/zenstack-query-executor.ts (2)
packages/runtime/src/client/executor/name-mapper.ts (1)
QueryNameMapper(28-458)packages/sdk/src/schema/schema.ts (3)
SchemaDef(10-18)ModelDef(20-37)TypeDefDef(101-105)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: claude-review
- GitHub Check: build-test (20.x, sqlite)
- GitHub Check: build-test (20.x, postgresql)
🔇 Additional comments (9)
packages/runtime/src/client/contract.ts (1)
154-161: LGTM! Clean API addition for eager connection.The new
$connect()method provides a useful way to eagerly establish database connections, and the updated$disconnect()documentation maintains good symmetry.packages/runtime/src/client/client-impl.ts (1)
236-238: LGTM! Correct implementation of eager connect.The implementation correctly uses Kysely's
.connection().execute()pattern with an empty callback to force connection acquisition. This is the standard approach for eagerly establishing database connections.packages/language/res/stdlib.zmodel (3)
154-175: Documentation improvements look good.The function documentation has been updated for consistency and clarity. The comments now correctly describe each function's behavior, which is especially important given the semantic swap of
hasSomeandhasEvery.
162-169: The review comment is incorrect—no issues found.Based on comprehensive verification across the codebase:
Function semantics are correct:
hasSome(at least one match) andhasEvery(all elements match) are properly named and implemented consistently across all components.Database operators align: PostgreSQL
@>(contains all) forhasEveryand&&(overlap) forhasSomeare correct.Validator logic is sound: Both functions implement the expected logic per their names (lines 417–423 in
crud/validator/utils.ts).Tests verify correctness: All test cases (custom-validation.test.ts, scalar-list.test.ts) use these functions with semantics matching their names.
No breaking changes, no semantic swap, and no additional verification needed.
129-133: ****The
searchfunction was never implemented in the codebase. The runtime export atpackages/runtime/src/client/functions.ts(lines 10–11) throws an error:"search function is not implemented yet". Additionally, the validator has no handler for the search function, and no code in the repository calls it. Commenting out the function definition instdlib.zmodelsimply formalizes that it is not available—this is not a breaking change and requires no verification of dependent code.Likely an incorrect or invalid review comment.
packages/runtime/src/client/crud/operations/aggregate.ts (1)
55-56: LGTM: updated to new buildOrderBy signature with negation.Negated ordering for negative
takeis handled correctly; removing the old pagination flag aligns with the dialect change. Please ensure all other call sites were similarly updated.tests/e2e/orm/client-api/find.test.ts (1)
190-191: Deterministic ordering for paginated/cursor tests — good move.Explicit
orderByensures stable results across backends now that no default ordering is injected.Also applies to: 199-200, 227-227, 245-245, 350-351
tests/e2e/orm/client-api/group-by.test.ts (1)
60-60: Tests adapted for new groupBy semantics — looks right.Using
arrayContainingand explicitorderBymatches the updated query behavior and avoids brittle ordering assertions.Also applies to: 69-69, 91-96
packages/runtime/src/client/crud/operations/group-by.ts (1)
14-19: Main-query groupBy refactor is coherent; negation and filters look correct.Field refs, where/having, and
skip/takewith negatedorderByare applied consistently in the main query. Consider adding a test that combines negativetakewith bothhavingand aggregatedorderByto guard against regressions.Also applies to: 24-33, 36-37, 40-41, 78-79, 86-88
|
Claude finished @ymc9's task —— View job Code Review for PR #317✅ OverviewThis PR addresses three main improvements:
📋 Todo List:
📋 Code Quality and Best Practices✅ Strengths
|
| Category | Rating | Notes |
|---|---|---|
| Code Quality | ⭐⭐⭐⭐☆ | Clean implementation with some architectural concerns |
| Security | ⭐⭐⭐⭐⭐ | No security concerns identified |
| Performance | ⭐⭐⭐⭐⭐ | Significant improvements with caching |
| Test Coverage | ⭐⭐⭐⭐☆ | Good coverage, some edge cases could be added |
| Bug Risk | ⭐⭐⭐☆☆ | Few issues that should be addressed |
✅ Recommendation: APPROVE with Minor Fixes
This PR provides valuable improvements to groupBy ordering, performance through caching, and useful functionality with $connect. The core architecture is sound and the implementation follows good patterns.
Key Strengths:
- Fixes real orderBy/groupBy compatibility issues
- Implements efficient validation caching that will significantly improve performance
- Maintains type safety and existing patterns
- Comprehensive test coverage for the main functionality
Minor issues to address:
- OrderBy array validation bug
- Cursor pagination without orderBy edge case
- Name mapping in mutation hooks
- Consider cache instance retention implications
The issues identified are not blocking but should be addressed to ensure robustness in edge cases.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/e2e/orm/client-api/connect-disconnect.test.ts (1)
27-27: Strengthen the error assertion for better test reliability.The test uses a generic
.rejects.toThrow()assertion, which could pass if the query throws for unexpected reasons (e.g., schema issues, memory errors). Consider asserting a specific error pattern or type that indicates the connection was actually closed.Apply this diff to make the assertion more specific:
- await expect(db.user.findFirst()).rejects.toThrow(); + await expect(db.user.findFirst()).rejects.toThrow(/connection|closed|destroyed/i);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/language/src/utils.ts(1 hunks)packages/runtime/src/client/client-impl.ts(1 hunks)packages/runtime/src/client/executor/zenstack-query-executor.ts(6 hunks)tests/e2e/orm/client-api/connect-disconnect.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/language/src/utils.ts
🧰 Additional context used
📓 Path-based instructions (2)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/runtime/src/client/client-impl.tstests/e2e/orm/client-api/connect-disconnect.test.tspackages/runtime/src/client/executor/zenstack-query-executor.ts
tests/e2e/**
📄 CodeRabbit inference engine (CLAUDE.md)
End-to-end tests must live under
tests/e2e/
Files:
tests/e2e/orm/client-api/connect-disconnect.test.ts
🧬 Code graph analysis (2)
tests/e2e/orm/client-api/connect-disconnect.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(52-169)
packages/runtime/src/client/executor/zenstack-query-executor.ts (2)
packages/runtime/src/client/executor/name-mapper.ts (1)
QueryNameMapper(28-458)packages/sdk/src/schema/schema.ts (2)
ModelDef(20-37)TypeDefDef(101-105)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: build-test (20.x, sqlite)
🔇 Additional comments (4)
packages/runtime/src/client/client-impl.ts (1)
236-240: LGTM! Connection establishment follows best practices.The implementation correctly executes a lightweight ping query (
select 1) to establish the database connection, which addresses the previous review feedback and follows standard connection validation patterns.packages/runtime/src/client/executor/zenstack-query-executor.ts (3)
45-45: Good optimization: conditional name mapper initialization.Making
nameMapperoptional and only initializing it when the schema contains@@mapor@mapattributes reduces overhead for schemas that don't require name mapping. This is a sensible performance improvement.Also applies to: 58-61
63-72: LGTM! Schema mapping detection is comprehensive.The helper correctly checks for both model/typeDef-level
@@mapattributes and field-level@mapattributes across models and typeDefs. The implementation properly supports the conditional initialization strategy.
256-258: LGTM! Safe abstraction for optional name mapping.The method cleanly encapsulates the conditional name mapping logic using optional chaining, ensuring the query is returned unchanged when no mapper is present.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests