feat(server): migrate rpc api handler and express adapter #328
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. WalkthroughNew @zenstackhq/server package added (RPC handler, Express middleware, types, utilities, tests). Runtime and tooling changes: safe JSON stringify utility added/removed in places, Postgres bytea hex handling, FK default actions, error constructors now include model, policy error construction updated, decimal.js moved to catalog, and BREAKINGCHANGES updated (check() removed). Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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: RPC API Handler and Express Adapter MigrationI've completed a comprehensive review of this PR that introduces the new 🟢 StrengthsArchitecture & Design:
Code Quality:
🟡 Security ConsiderationsInput Validation:
Potential Security Issues:
🔴 Code Issues & BugsRPC Handler (
return Object.keys(client.$schema.models).some((m) => m.toLowerCase() === model.toLowerCase());Model comparison is case-insensitive, but the actual client call uses the exact path segment. This could cause runtime errors.
} else {
return this.makeGenericErrorResponse(err);
}All non-ZenStack errors return 400 status, but some might warrant 500 (internal server errors). Express Middleware (
if (sendResponse === false && !client) {
throw new Error('unable to get ZenStackClient from request context');
}
if (!client) {
return response.status(500).json({ message: 'unable to get ZenStackClient from request context' });
}Same error condition handled differently based on
const url = request.protocol + '://' + request.get('host') + request.originalUrl;Uses user-supplied 🔍 Performance ConsiderationsPositive:
Potential Improvements:
📋 Test Coverage AssessmentExcellent coverage includes:
Missing test cases:
📝 Recommendations
🎯 Overall AssessmentThis is a well-architected and thoroughly tested implementation that successfully migrates RPC functionality to a dedicated server package. The code demonstrates strong TypeScript practices, comprehensive error handling, and good separation of concerns. The main concerns are around security hardening and some edge case handling that should be addressed before production use. Recommendation: Approve with minor fixes for the identified security and bug issues. --- • Branch: feat/rpc-adapter |
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the RPC API handler and Express adapter to a new @zenstackhq/server package. The migration standardizes dependency management by moving decimal.js to the workspace catalog, implements a framework-agnostic API handler with Express middleware support, and consolidates helper utilities.
Key Changes:
- Created new
@zenstackhq/serverpackage with RPC API handler and Express middleware - Centralized
decimal.jsdependency to workspace catalog - Moved
safeJSONStringifyutility to@zenstackhq/common-helperspackage - Enhanced error handling with model context in validation and policy errors
Reviewed Changes
Copilot reviewed 29 out of 30 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
packages/server/package.json |
New package configuration with peer dependencies for Express |
packages/server/src/api/rpc/index.ts |
RPC-style API handler implementing framework-agnostic request handling |
packages/server/src/express/middleware.ts |
Express adapter middleware for the API handler |
packages/server/test/api/rpc.test.ts |
Comprehensive test suite for RPC handler functionality |
packages/server/test/adapter/express.test.ts |
Integration tests for Express middleware |
pnpm-workspace.yaml |
Added decimal.js to catalog for centralized version management |
packages/runtime/src/client/errors.ts |
Enhanced error classes with model context |
packages/common-helpers/src/safe-json-stringify.ts |
Extracted JSON stringify utility for reuse |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Claude finished @ymc9's task —— View job Code Review: RPC API Handler and Express Adapter MigrationTodo List:
🟢 StrengthsArchitecture & Design:
Code Quality:
🟡 Security ConsiderationsInput Validation (✅ Good):
Security Concerns (
🔴 Critical Issues1. Case Sensitivity Bug ( return Object.keys(client.$schema.models).some((m) => m.toLowerCase() === model.toLowerCase());Model validation is case-insensitive, but the actual client call uses the exact path segment. This could cause runtime errors. 2. Inconsistent Error Handling ( if (sendResponse === false && !client) {
throw new Error('unable to get ZenStackClient from request context');
}
if (!client) {
return response.status(500).json({ message: 'unable to get ZenStackClient from request context' });
}Same error condition handled differently based on 3. Host Header Vulnerability ( const url = request.protocol + '://' + request.get('host') + request.originalUrl;Uses user-supplied 🟢 Test Coverage AssessmentExcellent Coverage:
Minor Gaps:
📝 Recommendations
🎯 Overall AssessmentThis is a well-architected and thoroughly tested implementation that successfully migrates RPC functionality to a dedicated server package. The code demonstrates strong TypeScript practices and comprehensive error handling. The main concerns are around security hardening that should be addressed before production use. Recommendation: Approve with minor fixes for the identified security and bug issues. --- • Branch: feat/rpc-adapter |
|
Claude finished @ymc9's task —— View job Code Review: RPC API Handler and Express Adapter MigrationTodo List:
🟢 Architecture & Design StrengthsClean Framework-Agnostic Design:
Package Structure:
🔴 Critical Issues Found1. Package Export Configuration Bug (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (12)
TODO.md (1)
89-89: Address markdown linting issue: adjust list indentation.The static analysis tool flagged an indentation inconsistency (MD007). The file uses 4-space indentation for nested list items, but the linter expects 2-space indentation. Consider aligning with the linter's expectations for consistency across the codebase.
- [ ] ORM - - [x] generate + - [x] generate(Apply this pattern throughout nested lists in the file.)
packages/server/src/api/utils.ts (1)
6-24: LGTM! Consider enhancing error formatting.The log function implementation is solid with good support for lazy message evaluation and multiple logger types.
One minor suggestion: Line 22 converts the error to a string using template literal interpolation (
${error}), which may not provide detailed stack traces. Consider usingerror instanceof Error ? error.stack : String(error)for more informative error logging.Apply this diff if you want to include stack traces:
- logFn(`@zenstackhq/server: [${level}] ${getMessage()}${error ? `\n${error}` : ''}`); + logFn(`@zenstackhq/server: [${level}] ${getMessage()}${error ? `\n${error instanceof Error ? error.stack : String(error)}` : ''}`);packages/common-helpers/src/safe-json-stringify.ts (1)
1-12: LGTM! Function correctly handles bigint serialization.The implementation properly converts bigint values to strings during JSON stringification. The JSDoc comment accurately describes the specific handling of bigint values.
Optional: If you plan to add more safety features in the future (e.g., circular reference detection, other special type handling), consider renaming to
bigIntSafeJSONStringifyto be more explicit about its current scope, or keep the current name if you intend to expand its capabilities.packages/server/vitest.config.ts (1)
1-4: Merge is redundant; export base directly.No local overrides. Simplify to reduce indirection.
-import base from '@zenstackhq/vitest-config/base'; -import { defineConfig, mergeConfig } from 'vitest/config'; - -export default mergeConfig(base, defineConfig({})); +import base from '@zenstackhq/vitest-config/base'; +export default base;packages/server/package.json (1)
1-64: Minor package polish.Optional tweaks for DX and safety.
- Add engines: { "node": ">=18" }.
- Consider "sideEffects": false for better tree-shaking.
- Add top-level "types": "./dist/index.d.ts" for editor tooling.
packages/server/src/types.ts (2)
49-62: Avoid naming collision with global Response.Rename to reduce confusion with DOM/Fetch Response.
-export type Response = { +export type ApiResponse = { status: number; body: unknown; };And update usages accordingly (e.g., ApiHandler.handleRequest return type).
28-41: Narrow method and harden query typing.Small ergonomics improvement.
- method: string; + method: 'GET' | 'POST' | 'PUT' | 'PATCH' | 'DELETE'; - query?: Record<string, string | string[]>; + query?: Readonly<Record<string, string | string[]>>;packages/server/test/adapter/express.test.ts (1)
10-92: Add tests for SuperJSON GET path and error cases.Increase confidence in RPC handler.
- SuperJSON: use makeUrl with useSuperJson=true and include a Date in filter to verify meta round-trip.
- Error handling: assert 400 for method mismatch (e.g., POST /findMany) and unknown model path.
Example snippet:// superjson GET with Date const date = new Date(); let r = await request(app).get(makeUrl('/api/post/findMany', { where: { createdAt: { gte: date } } }, true)); expect(r.status).toBe(200); // method mismatch r = await request(app).post('/api/post/findMany'); expect(r.status).toBe(400); // unknown model r = await request(app).get(makeUrl('/api/unknown/findMany')); expect(r.status).toBe(400);packages/server/src/api/rpc/index.ts (4)
84-86: Tiny grammar fixMessage reads “only PUT AND PATCH are supported”. Prefer “PUT or PATCH”.
- return this.makeBadInputErrorResponse('invalid request method, only PUT AND PATCH are supported'); + return this.makeBadInputErrorResponse('invalid request method, only PUT or PATCH are supported');
170-177: Map unexpected errors to 500 Internal Server ErrorGeneric, non-ZenStack errors should be server faults (500), not client faults (400).
- const resp = { - status: 400, + const resp = { + status: 500, body: { error: { message: (err as Error).message || 'unknown error' } }, };
205-216: Optional: drop unnecessary async
processRequestPayloaddoesn’t await anything; removeasyncto simplify.- private async processRequestPayload(args: any) { + private processRequestPayload(args: any) {
131-139: Note on serialization of falsy resultsWhen
clientResultis falsy (e.g., 0 for count), serialization is skipped. That’s fine for current needs, but if you ever return falsy structured values, consider serializing unconditionally.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (29)
BREAKINGCHANGES.md(1 hunks)TODO.md(1 hunks)packages/common-helpers/src/index.ts(1 hunks)packages/common-helpers/src/safe-json-stringify.ts(1 hunks)packages/plugins/policy/src/policy-handler.ts(2 hunks)packages/runtime/package.json(1 hunks)packages/runtime/src/client/crud/dialects/postgresql.ts(1 hunks)packages/runtime/src/client/crud/validator/index.ts(1 hunks)packages/runtime/src/client/errors.ts(2 hunks)packages/runtime/src/client/helpers/schema-db-pusher.ts(1 hunks)packages/runtime/src/client/query-utils.ts(0 hunks)packages/sdk/package.json(1 hunks)packages/server/eslint.config.js(1 hunks)packages/server/package.json(1 hunks)packages/server/src/api/index.ts(1 hunks)packages/server/src/api/rpc/index.ts(1 hunks)packages/server/src/api/utils.ts(1 hunks)packages/server/src/express/index.ts(1 hunks)packages/server/src/express/middleware.ts(1 hunks)packages/server/src/types.ts(1 hunks)packages/server/test/adapter/express.test.ts(1 hunks)packages/server/test/api/rpc.test.ts(1 hunks)packages/server/test/utils.ts(1 hunks)packages/server/tsconfig.json(1 hunks)packages/server/tsup.config.ts(1 hunks)packages/server/vitest.config.ts(1 hunks)pnpm-workspace.yaml(1 hunks)tests/e2e/package.json(1 hunks)tests/regression/package.json(1 hunks)
💤 Files with no reviewable changes (1)
- packages/runtime/src/client/query-utils.ts
🧰 Additional context used
📓 Path-based instructions (3)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/server/vitest.config.tspackages/sdk/package.jsonpackages/runtime/src/client/crud/dialects/postgresql.tstests/e2e/package.jsonpackages/server/src/api/utils.tspackages/server/test/api/rpc.test.tspackages/common-helpers/src/index.tspackages/runtime/package.jsonpackages/server/src/api/index.tspackages/server/src/express/index.tspackages/server/test/utils.tspackages/server/tsconfig.jsonpackages/server/src/express/middleware.tspackages/common-helpers/src/safe-json-stringify.tspackages/server/tsup.config.tspackages/runtime/src/client/helpers/schema-db-pusher.tspackages/runtime/src/client/crud/validator/index.tspackages/server/test/adapter/express.test.tspackages/plugins/policy/src/policy-handler.tspackages/server/src/types.tspackages/server/package.jsonpackages/runtime/src/client/errors.tstests/regression/package.jsonpackages/server/eslint.config.jspackages/server/src/api/rpc/index.ts
tests/e2e/**
📄 CodeRabbit inference engine (CLAUDE.md)
End-to-end tests must live under
tests/e2e/
Files:
tests/e2e/package.json
pnpm-workspace.yaml
📄 CodeRabbit inference engine (CLAUDE.md)
Use pnpm workspaces for the monorepo
Files:
pnpm-workspace.yaml
🧬 Code graph analysis (8)
packages/server/src/api/utils.ts (2)
packages/server/src/types.ts (2)
LogConfig(17-17)LogLevel(7-7)packages/runtime/src/client/executor/zenstack-driver.ts (1)
error(172-179)
packages/server/test/api/rpc.test.ts (4)
packages/runtime/src/client/contract.ts (1)
ClientContract(50-170)packages/testtools/src/client.ts (2)
createPolicyTestClient(179-190)createTestClient(52-169)packages/server/src/api/rpc/index.ts (3)
schema(30-32)handleRequest(34-155)RPCApiHandler(27-241)packages/server/test/utils.ts (1)
schema(3-29)
packages/server/test/utils.ts (2)
packages/runtime/src/client/crud/validator/index.ts (1)
schema(60-62)packages/server/src/api/rpc/index.ts (1)
schema(30-32)
packages/server/src/express/middleware.ts (3)
packages/sdk/src/schema/schema.ts (1)
SchemaDef(10-18)packages/server/src/types.ts (2)
ApiHandler(67-77)Response(52-62)packages/runtime/src/client/contract.ts (1)
ClientContract(50-170)
packages/server/test/adapter/express.test.ts (3)
packages/testtools/src/client.ts (1)
createPolicyTestClient(179-190)packages/server/src/api/rpc/index.ts (2)
schema(30-32)RPCApiHandler(27-241)packages/server/test/utils.ts (2)
schema(3-29)makeUrl(31-33)
packages/plugins/policy/src/policy-handler.ts (1)
packages/runtime/src/client/errors.ts (1)
RejectedByPolicyError(68-76)
packages/server/src/types.ts (2)
packages/sdk/src/schema/schema.ts (1)
SchemaDef(10-18)packages/runtime/src/client/contract.ts (1)
ClientContract(50-170)
packages/server/src/api/rpc/index.ts (6)
packages/server/src/api/utils.ts (2)
registerCustomSerializers(29-50)log(6-24)packages/sdk/src/schema/schema.ts (1)
SchemaDef(10-18)packages/server/src/types.ts (4)
LogConfig(17-17)ApiHandler(67-77)RequestContext(22-47)Response(52-62)packages/common-helpers/src/safe-json-stringify.ts (1)
safeJSONStringify(4-12)packages/runtime/src/client/errors.ts (4)
ZenStackError(4-4)NotFoundError(36-43)InputValidationError(9-17)RejectedByPolicyError(68-76)packages/runtime/src/client/contract.ts (1)
ClientContract(50-170)
🪛 markdownlint-cli2 (0.18.1)
TODO.md
89-89: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🔇 Additional comments (24)
packages/runtime/src/client/helpers/schema-db-pusher.ts (1)
295-307: LGTM! Sensible foreign key constraint defaults.The default behaviors are well-chosen and align with database best practices:
onDelete: 'set null'for optional relations allows parent deletion without orphaning childrenonDelete: 'restrict'for required relations prevents deletion that would violate referential integrityonUpdate: 'cascade'maintains consistency when primary keys are updatedThese defaults will improve the developer experience by reducing boilerplate while maintaining data integrity.
packages/runtime/src/client/crud/dialects/postgresql.ts (1)
113-120: Code implementation is correct; verify test coverage for hex-encoded bytea handling.The
transformOutputBytesmethod correctly handles the three cases: Buffer objects, hex-encoded bytea strings prefixed with\x, and other values. The implementation properly parses the hex string by removing the prefix and converting to Uint8Array.Existing tests in
packages/server/test/api/rpc.test.tsexercise Bytes fields with PostgreSQL, but the specific hex-encoded bytea case (when node-pg embeds bytea in JSON responses) cannot be confirmed as explicitly tested. Verify that your test suite exercises this code path, particularly when bytea fields are returned in JSON contexts from the database.tests/regression/package.json (1)
12-12: Catalog-based decimal.js resolution is properly aligned.The migration to
catalog:is consistent with the catalog definition inpnpm-workspace.yamlline 19, which specifies^10.4.3. No issues detected.packages/server/tsconfig.json (1)
1-4: TypeScript configuration is correctly structured.The configuration properly extends the workspace base config and includes the appropriate source path pattern. No issues detected.
packages/runtime/package.json (1)
67-67: Catalog-based decimal.js resolution is properly configured.Consistent with the catalog definition in
pnpm-workspace.yaml. No issues detected.tests/e2e/package.json (1)
23-23: Catalog-based decimal.js resolution is correctly applied.Consistent with the workspace catalog definition. No issues detected.
packages/sdk/package.json (1)
50-50: Catalog-based decimal.js resolution in devDependencies is correct.Properly configured and consistent with the workspace catalog. No issues detected.
pnpm-workspace.yaml (1)
19-19: Decimal.js catalog entry is properly configured.The catalog entry establishes
^10.4.3as the centralized version referenced by all packages viacatalog:syntax. Consistent and correct.BREAKINGCHANGES.md (1)
1-3: Breaking changes documentation is clear and properly ordered.The restructured list correctly prioritizes the update/delete policy rejection behavior, documents the new check() API removal, and maintains the to-one relation context. Clear and appropriate.
packages/server/src/api/utils.ts (1)
29-50: Verify idempotency of serializer registration.The implementation looks correct with proper edge runtime support. However, if
registerCustomSerializersis called multiple times (e.g., in different parts of the application), the same serializers will be re-registered.Please verify that SuperJSON handles duplicate registrations gracefully, or consider adding a guard to ensure this function is only called once:
let registered = false; export function registerCustomSerializers() { if (registered) return; registered = true; // ... rest of the implementation }packages/server/src/express/index.ts (1)
1-1: LGTM!Clean re-export of the Express middleware.
packages/runtime/src/client/crud/validator/index.ts (1)
234-239: LGTM!The updated error construction correctly passes the model as the first parameter, aligning with the new
InputValidationErrorconstructor signature.packages/common-helpers/src/index.ts (1)
4-4: LGTM!Clean addition to the barrel exports.
packages/server/src/api/index.ts (1)
1-1: LGTM!Clean re-export of the RPC API handler.
packages/plugins/policy/src/policy-handler.ts (1)
107-107: LGTM!The updated error construction correctly passes
RejectedByPolicyReason.NO_ACCESSas the second parameter, aligning with the newRejectedByPolicyErrorconstructor signature. The reason code accurately represents the policy rejection scenario in both cases.Also applies to: 624-624
packages/runtime/src/client/errors.ts (3)
10-16: LGTM! Enhanced error context with model information.The updated constructor signature adds the model as a public readonly field, enabling programmatic access to the model context when handling validation errors. This is a breaking change but significantly improves error handling capabilities.
37-42: LGTM! Enhanced error context with model information.The updated constructor adds model as a public readonly field, improving error handling by providing programmatic access to the affected model.
48-75: LGTM! Well-structured policy rejection handling.The new
RejectedByPolicyReasonenum provides clear, structured reason codes for different policy rejection scenarios. The updatedRejectedByPolicyErrorconstructor with a default reason ofNO_ACCESSis well-designed and aligns with the usage patterns in the codebase.packages/server/tsup.config.ts (1)
12-13: Confirm .d.cts generation for CJS “types” export.With dts: true and dual formats, tsup may only emit .d.ts. Your package.json requires ./dist/index.d.cts for require.
Please build locally and confirm dist contains index.d.cts. If not, either:
- Configure tsup to emit .d.cts, or
- Simplify exports to use index.d.ts for both import/require.
packages/server/package.json (1)
45-55: body-parser v2 with v1 types; verify compatibility.body-parser is ^2.2.0 while @types/body-parser is ^1.19.6.
- If v2 ships its own types, remove @types/body-parser.
- Otherwise, bump @types/body-parser to match v2 (if available) or pin body-parser to v1.x.
packages/server/eslint.config.js (1)
1-4: The suggested typeLinter.FlatConfig[]is deprecated; useLinter.Config[]instead.The review correctly identifies a type mismatch:
packages/server/eslint.config.jsre-exports an array frombase.jsbut types it as singularConfig. However, ESLint v9 replaced the deprecatedLinter.FlatConfigwithLinter.Config, so the correct fix is:-/** @type {import("eslint").Linter.Config} */ +/** @type {import("eslint").Linter.Config[]} */ export default config;Likely an incorrect or invalid review comment.
packages/server/test/api/rpc.test.ts (3)
14-17: Ensure clients are disconnected to avoid leaked connectionsAdd an afterAll to close both clients. Also disconnect the Postgres test client created in “field types”.
[ suggest_recommended_refactor ]beforeAll(async () => { client = await createPolicyTestClient(schema); rawClient = client.$unuseAll(); }); + + afterAll(async () => { + try { + await rawClient?.$disconnect(); + await client?.$disconnect(); + } catch {} + });And near the end of the “field types” test (before the closing brace):
expect(r.status).toBe(200); expect(r.data).toBeNull(); - }); + await client.$disconnect(); + });
286-291: Remove redundant handler re-initialization
handleRequest = makeHandler();is called twice back-to-back; the second is unnecessary.
[ suggest_optional_refactor ]- let handleRequest = makeHandler(); - - handleRequest = makeHandler(); + const handleRequest = makeHandler();
359-381: Avoid variable shadowing for readabilityLocal
schemashadows the importedschema. Rename to a distinct identifier to reduce confusion.
[ suggest_optional_refactor ]- const schema = ` + const fieldTypesSchema = ` ... - const client = await createTestClient(schema, { provider: 'postgresql' }); + const client = await createTestClient(fieldTypesSchema, { provider: 'postgresql' });
|
Claude finished @ymc9's task —— View job Code Review: RPC API Handler and Express Adapter MigrationI've completed a comprehensive review of this PR that introduces the new 🟢 StrengthsArchitecture & Design:
Code Quality:
🟡 Security ConsiderationsInput Validation (✅ Good):
Security Concerns (
🔴 Critical Issues1. Case Sensitivity Bug ( return Object.keys(client.$schema.models).some((m) => lowerCaseFirst(m) === lowerCaseFirst(model));Model validation is case-insensitive, but the actual client call uses the exact path segment. This could cause runtime errors. 2. Inconsistent Error Handling ( if (sendResponse === false && !client) {
throw new Error('unable to get ZenStackClient from request context');
}
if (!client) {
return response.status(500).json({ message: 'unable to get ZenStackClient from request context' });
}Same error condition handled differently based on 3. Host Header Vulnerability ( const url = request.protocol + '://' + request.get('host') + request.originalUrl;Uses user-supplied 🟢 Test Coverage AssessmentExcellent Coverage:
Minor Gaps:
📝 Recommendations
🎯 Overall AssessmentThis is a well-architected and thoroughly tested implementation that successfully migrates RPC functionality to a dedicated server package. The code demonstrates strong TypeScript practices and comprehensive error handling. The main concerns are around security hardening that should be addressed before production use. Recommendation: Approve with minor fixes for the identified security and bug issues. --- • Branch: feat/rpc-adapter |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/server/src/api/rpc/index.ts (1)
73-80: Preserve unmarshalQ error message and handle duplicate q/meta parametersReturn the specific error from unmarshalQ and reject duplicate array-style query params. This keeps “invalid meta” vs “invalid q” distinct and avoids passing arrays into JSON.parse.
Apply to both GET and DELETE branches:
- try { - args = query?.['q'] - ? this.unmarshalQ(query['q'] as string, query['meta'] as string | undefined) - : {}; - } catch { - return this.makeBadInputErrorResponse('invalid "q" query parameter'); - } + try { + const qParam = query?.['q']; + const metaParam = query?.['meta']; + if (Array.isArray(qParam) || Array.isArray(metaParam)) { + return this.makeBadInputErrorResponse('duplicate "q" or "meta" query parameter'); + } + args = qParam ? this.unmarshalQ(qParam as string, metaParam as string | undefined) : {}; + } catch (err) { + return this.makeBadInputErrorResponse((err as Error).message); + }Also applies to: 100-106
🧹 Nitpick comments (7)
packages/server/src/api/rpc/index.ts (7)
132-139: Always SuperJSON.serialize to preserve meta for falsy values (0/false/null)Conditional serialization skips meta when result is falsy (e.g., count = 0). Serialize unconditionally for consistency.
- // superjson serialize response - if (clientResult) { - const { json, meta } = SuperJSON.serialize(clientResult); - responseBody = { data: json }; - if (meta) { - responseBody.meta = { serialization: meta }; - } - } + // superjson serialize response (always) + const { json, meta } = SuperJSON.serialize(clientResult); + responseBody = { data: json }; + if (meta) { + responseBody.meta = { serialization: meta }; + }
123-127: Avoid logging full request args/response bodies at debug; redact or summarize to reduce PII riskCurrent logs may include sensitive data. Prefer summarizing (op/model and arg keys) or add redaction.
Example tweak (minimal):
- () => `handling "${model}.${op}" request with args: ${safeJSONStringify(processedArgs)}`, + () => `handling "${model}.${op}" request (arg keys: ${processedArgs && typeof processedArgs === 'object' ? Object.keys(processedArgs as any).join(',') : 'n/a'})`,- () => `sending response for "${model}.${op}" request: ${safeJSONStringify(response)}`, + () => `sending response for "${model}.${op}" request (status: ${resCode})`,Also applies to: 141-147
60-62: Return 200 for upsert; reserve 201 for createsUpsert may update an existing entity; 201 can mislead clients.
- resCode = 201; + resCode = op === 'upsert' ? 200 : 201;
162-169: Use appropriate log levels for error responses
- 400/403/404/422: warn
- 500: error
- log(this.options.log, 'debug', () => `sending error response: ${safeJSONStringify(resp)}`); + log(this.options.log, 'warn', () => `sending error response: ${safeJSONStringify(resp)}`);- log(this.options.log, 'debug', () => `sending error response: ${safeJSONStringify(resp)}`); + log(this.options.log, 'error', () => `sending error response: ${safeJSONStringify(resp)}`);- log(this.options.log, 'debug', () => `sending error response: ${safeJSONStringify(resp)}`); + log(this.options.log, 'warn', () => `sending error response: ${safeJSONStringify(resp)}`);Also applies to: 171-178, 201-204
118-131: Fail fast if operation is not implemented on the clientProtects against version skew (e.g., createManyAndReturn not present) and yields a clear 400 instead of 500.
log( this.options.log, 'debug', () => `handling "${model}.${op}" request with args: ${safeJSONStringify(processedArgs)}`, ); - const clientResult = await (client as any)[model][op](processedArgs); + const modelApi = (client as any)[model]; + if (!modelApi || typeof modelApi[op] !== 'function') { + return this.makeBadInputErrorResponse(`unsupported operation "${op}" for model "${model}"`); + } + const clientResult = await modelApi[op](processedArgs);If you prefer, I can run a repo scan to see where these ops are relied on and confirm runtime compatibility.
14-14: Consider registering SuperJSON serializers once to avoid duplicate registration across importsIf other modules also call registerCustomSerializers, SuperJSON may throw on duplicate type IDs. Use a module-level once flag.
Example (in utils.ts):
let __serializersRegistered = false; export function registerCustomSerializersOnce() { if (__serializersRegistered) return; __serializersRegistered = true; registerCustomSerializers(); }Then here:
// replace -registerCustomSerializers(); // with +registerCustomSerializersOnce();
113-121: Optional: validate model before payload processing to fail fastMinor efficiency and clearer errors for unknown models.
- const { result: processedArgs, error } = await this.processRequestPayload(args); - if (error) { - return this.makeBadInputErrorResponse(error); - } - - try { - if (!this.isValidModel(client, model)) { - return this.makeBadInputErrorResponse(`unknown model name: ${model}`); - } + if (!this.isValidModel(client, model)) { + return this.makeBadInputErrorResponse(`unknown model name: ${model}`); + } + const { result: processedArgs, error } = await this.processRequestPayload(args); + if (error) { + return this.makeBadInputErrorResponse(error); + } + try {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/language/package.json(1 hunks)packages/server/package.json(1 hunks)packages/server/src/api/rpc/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/server/package.json
🧰 Additional context used
📓 Path-based instructions (1)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/language/package.jsonpackages/server/src/api/rpc/index.ts
🧬 Code graph analysis (1)
packages/server/src/api/rpc/index.ts (6)
packages/server/src/api/utils.ts (2)
registerCustomSerializers(29-50)log(6-24)packages/sdk/src/schema/schema.ts (1)
SchemaDef(10-18)packages/server/src/types.ts (4)
LogConfig(17-17)ApiHandler(67-77)RequestContext(22-47)Response(52-62)packages/common-helpers/src/safe-json-stringify.ts (1)
safeJSONStringify(4-12)packages/runtime/src/client/errors.ts (4)
ZenStackError(4-4)NotFoundError(36-43)InputValidationError(9-17)RejectedByPolicyError(68-76)packages/runtime/src/client/contract.ts (1)
ClientContract(50-170)
⏰ 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 (1)
packages/language/package.json (1)
16-16: Good alignment with repo-wide testing infrastructure.The addition of the
"test": "vitest run"script aligns the language package with the Vitest-based testing approach being introduced across the monorepo, as evidenced by the existing@zenstackhq/vitest-configdev dependency.
|
Claude finished @ymc9's task —— View job Code Review: RPC API Handler and Express Adapter MigrationTodo List:
🟢 Architecture & Design StrengthsClean Framework-Agnostic Design:
Package Structure:
🔴 Critical Issues Found1. Case Sensitivity Security Issue (
|
|
Claude finished @ymc9's task —— View job Code Review: RPC API Handler and Express Adapter MigrationI've completed a comprehensive review of this PR that introduces the new 🟢 StrengthsArchitecture & Design:
Code Quality:
🟡 Security ConsiderationsInput Validation (✅ Good):
Security Concerns (
🔴 Critical Issues1. Case Sensitivity Bug ( return Object.keys(client.$schema.models).some((m) => lowerCaseFirst(m) === lowerCaseFirst(model));Model validation is case-insensitive, but the actual client call uses the exact path segment. This could cause runtime errors. 2. Inconsistent Error Handling ( if (sendResponse === false && !client) {
throw new Error('unable to get ZenStackClient from request context');
}
if (!client) {
return response.status(500).json({ message: 'unable to get ZenStackClient from request context' });
}Same error condition handled differently based on 3. Host Header Vulnerability ( const url = request.protocol + '://' + request.get('host') + request.originalUrl;Uses user-supplied 🟢 Test Coverage AssessmentExcellent Coverage:
Minor Gaps:
📝 Recommendations
🎯 Overall AssessmentThis is a well-architected and thoroughly tested implementation that successfully migrates RPC functionality to a dedicated server package. The code demonstrates strong TypeScript practices and comprehensive error handling. The main concerns are around security hardening that should be addressed before production use. Recommendation: Approve with minor fixes for the identified security and bug issues. --- • Branch: feat/rpc-adapter |
|
Claude finished @ymc9's task —— View job Code Review: RPC API Handler and Express Adapter MigrationTodo List:
🟢 Architecture & Design StrengthsClean Framework-Agnostic Design:
Package Structure:
🔴 Critical Issues Found1. Case Sensitivity Security Issue (
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
packages/server/src/api/rpc/index.ts (4)
100-108: Nice: error propagation now preserves q/meta specificityDELETE branch now returns the specific
unmarshalQerror message. LGTM.
85-87: Wording nit resolved“PUT or PATCH” wording looks good now. Thanks for fixing.
73-79: Preserve specific unmarshalQ error (don’t always blame "q")Propagate the actual error from
unmarshalQso “invalid meta” is not masked.- try { - args = query?.['q'] - ? this.unmarshalQ(query['q'] as string, query['meta'] as string | undefined) - : {}; - } catch { - return this.makeBadInputErrorResponse('invalid "q" query parameter'); - } + try { + args = query?.['q'] + ? this.unmarshalQ(query['q'] as string, query['meta'] as string | undefined) + : {}; + } catch (err) { + return this.makeBadInputErrorResponse( + err instanceof Error ? err.message : 'invalid "q" query parameter', + ); + }
208-219: Guard processRequestPayload against null/non-object/array to avoid TypeError and array-shape corruptionDestructuring
argscan throw for null/primitives and converts arrays to objects. Add shape checks before destructuring.- private async processRequestPayload(args: any) { - const { meta, ...rest } = args; - if (meta?.serialization) { - try { - // superjson deserialization - args = SuperJSON.deserialize({ json: rest, meta: meta.serialization }); - } catch (err) { - return { result: undefined, error: `failed to deserialize request payload: ${(err as Error).message}` }; - } - } - return { result: args, error: undefined }; - } + private async processRequestPayload(args: any) { + // Non-object or null: nothing to deserialize + if (args === null || typeof args !== 'object') { + return { result: args, error: undefined }; + } + // Avoid converting arrays to objects; top-level arrays are not expected to carry meta + if (Array.isArray(args)) { + return { result: args, error: undefined }; + } + + const { meta, ...rest } = args as Record<string, unknown>; + if (meta && typeof meta === 'object' && (meta as any).serialization) { + try { + // superjson deserialization + args = SuperJSON.deserialize({ json: rest, meta: (meta as any).serialization }); + } catch (err) { + return { result: undefined, error: `failed to deserialize request payload: ${(err as Error).message}` }; + } + } + return { result: args, error: undefined }; + }
🧹 Nitpick comments (5)
packages/server/src/api/rpc/index.ts (5)
73-76: Handle string[] query params for q/meta (avoid ambiguous inputs)
RequestContext.queryallowsstring | string[]; normalize to a single string. This prevents accidental array-to-string casts.- args = query?.['q'] - ? this.unmarshalQ(query['q'] as string, query['meta'] as string | undefined) + args = this.getSingleQueryParam(query, 'q') + ? this.unmarshalQ( + this.getSingleQueryParam(query, 'q')!, + this.getSingleQueryParam(query, 'meta'), + ) : {};// Add this helper in the class (e.g., near other private helpers) private getSingleQueryParam( query: Record<string, string | string[]> | undefined, key: string, ): string | undefined { const v = query?.[key]; return Array.isArray(v) ? v[0] : (v as string | undefined); }Also applies to: 100-103
131-133: Guard for unsupported operations to return 400 instead of 500Validate
client[model][op]exists and is callable before invoking.- const clientResult = await (client as any)[model][op](processedArgs); + const modelApi = (client as any)[model]; + if (!modelApi || typeof modelApi[op] !== 'function') { + return this.makeBadInputErrorResponse(`unsupported operation: ${model}.${op}`); + } + const clientResult = await modelApi[op](processedArgs);
49-62: Consider 200 for upsert responses (201 may be misleading when updating)RPC style doesn’t need REST creation semantics; if kept, return 200 for
upsertto avoid implying resource creation.- case 'create': - case 'createMany': - case 'createManyAndReturn': - case 'upsert': + case 'create': + case 'createMany': + case 'createManyAndReturn': if (method !== 'POST') { return this.makeBadInputErrorResponse('invalid request method, only POST is supported'); } if (!requestBody) { return this.makeBadInputErrorResponse('missing request body'); } args = requestBody; resCode = 201; break; + + case 'upsert': + if (method !== 'POST') { + return this.makeBadInputErrorResponse('invalid request method, only POST is supported'); + } + if (!requestBody) { + return this.makeBadInputErrorResponse('missing request body'); + } + args = requestBody; + resCode = 200; + break;
14-14: Make serializer registration idempotent (avoid duplicate SuperJSON.registerCustom)Top‑level
registerCustomSerializers()can be invoked from multiple entry points; guard it to run once or initialize at app bootstrap.
125-130: PII‑aware loggingDebug logs currently include full args and response. Consider masking sensitive fields or gating by env to avoid accidental PII leakage in prod.
Also applies to: 143-149
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
packages/language/src/constants.ts(1 hunks)packages/server/package.json(1 hunks)packages/server/src/api/rpc/index.ts(1 hunks)packages/server/src/express/middleware.ts(1 hunks)packages/testtools/src/client.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/language/src/constants.ts
- packages/server/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/server/src/express/middleware.ts
🧰 Additional context used
📓 Path-based instructions (1)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/testtools/src/client.tspackages/server/src/api/rpc/index.ts
🧬 Code graph analysis (1)
packages/server/src/api/rpc/index.ts (7)
packages/server/src/api/utils.ts (2)
registerCustomSerializers(29-50)log(6-24)packages/sdk/src/schema/schema.ts (1)
SchemaDef(10-18)packages/server/src/types.ts (4)
LogConfig(17-17)ApiHandler(67-77)RequestContext(22-47)Response(52-62)packages/common-helpers/src/lower-case-first.ts (1)
lowerCaseFirst(1-3)packages/common-helpers/src/safe-json-stringify.ts (1)
safeJSONStringify(4-12)packages/runtime/src/client/errors.ts (4)
ZenStackError(4-4)NotFoundError(36-43)InputValidationError(9-17)RejectedByPolicyError(68-76)packages/runtime/src/client/contract.ts (1)
ClientContract(50-170)
🔇 Additional comments (1)
packages/testtools/src/client.ts (1)
200-200: Good defensive improvement; verify edge case handling.The fallback to
'unnamed'makes the function more robust by preventing failures when test state is unavailable. However, if bothcurrentTestNameandtestPath(line 201) are unavailable in multiple tests, they would generate identical database names, potentially causing conflicts during parallel test execution.Consider verifying that this edge case is either:
- Unlikely to occur in practice (test state is consistently available), or
- Acceptable for your test isolation strategy (tests with unavailable state run sequentially)
You can also add an incrementing counter or timestamp to the digest computation to further reduce collision risk if needed.
Summary by CodeRabbit
Breaking Changes
New Features
Bug Fixes
Chores