Skip to content

Conversation

@ymc9
Copy link
Member

@ymc9 ymc9 commented Oct 24, 2025

No description provided.

github-actions bot and others added 2 commits October 23, 2025 18:26
Co-authored-by: ymc9 <104139426+ymc9@users.noreply.github.com>
* feat(server): migrate rpc api handler and express adapter

* fix package.json

* misc fixes

* update

* update ot express5 only

* update

* update

* fix test
Copilot AI review requested due to automatic review settings October 24, 2025 02:05
@coderabbitai
Copy link

coderabbitai bot commented Oct 24, 2025

Warning

Rate limit exceeded

@ymc9 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 18 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between ba3bbe8 and 56b68ab.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (45)
  • BREAKINGCHANGES.md (1 hunks)
  • TODO.md (1 hunks)
  • package.json (1 hunks)
  • packages/cli/package.json (1 hunks)
  • packages/common-helpers/package.json (1 hunks)
  • packages/common-helpers/src/index.ts (1 hunks)
  • packages/common-helpers/src/safe-json-stringify.ts (1 hunks)
  • packages/config/eslint-config/package.json (1 hunks)
  • packages/config/typescript-config/package.json (1 hunks)
  • packages/config/vitest-config/package.json (1 hunks)
  • packages/create-zenstack/package.json (1 hunks)
  • packages/dialects/sql.js/package.json (1 hunks)
  • packages/language/package.json (2 hunks)
  • packages/language/src/constants.ts (1 hunks)
  • packages/plugins/policy/package.json (1 hunks)
  • packages/plugins/policy/src/policy-handler.ts (2 hunks)
  • packages/runtime/package.json (2 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 (2 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)
  • packages/tanstack-query/package.json (1 hunks)
  • packages/testtools/package.json (1 hunks)
  • packages/testtools/src/client.ts (1 hunks)
  • packages/zod/package.json (1 hunks)
  • pnpm-workspace.yaml (1 hunks)
  • samples/blog/package.json (1 hunks)
  • tests/e2e/package.json (2 hunks)
  • tests/regression/package.json (2 hunks)
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch dev

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@claude
Copy link

claude bot commented Oct 24, 2025

Claude finished @ymc9's task —— View job


Pull Request Review: v3.0.0-beta.13 Release

Reviewing this release merge from dev → main with 2115 additions across 46 files.

Review Checklist

  • Read repository guidelines and setup
  • Review new @zenstackhq/server package implementation
  • Analyze RPC API security and design
  • Review Express middleware and adapter
  • Check error handling improvements
  • Assess performance considerations
  • Evaluate test coverage
  • Provide comprehensive feedback

🔍 Review Summary

This is a substantial release that introduces the new @zenstackhq/server package with RPC API handlers and Express adapters. Overall code quality is high with good test coverage, but there are some security and performance considerations.

Positive Highlights

1. Architecture & Design

  • Clean separation between framework-agnostic API handlers and Express adapters
  • Well-designed type system using generics throughout ApiHandler<Schema> and RequestContext<Schema>
  • Proper abstraction with the ApiHandler interface allowing for future framework adapters
  • Good use of SuperJSON for handling complex data serialization

2. Error Handling

  • Comprehensive error hierarchy in packages/runtime/src/client/errors.ts:68-76
  • Proper HTTP status code mapping for different error types (404 for NotFoundError, 403 for RejectedByPolicyError, etc.)
  • Structured error responses with detailed error information

3. Test Coverage

  • Extensive test coverage for RPC API functionality (packages/server/test/api/rpc.test.ts)
  • Integration tests for Express middleware (packages/server/test/adapter/express.test.ts)
  • Tests cover CRUD operations, query parameters, and error scenarios

⚠️ Security Concerns

1. Input Validation

  • packages/server/src/api/rpc/index.ts:131 - Direct dynamic method invocation: (client as any)[model][op](processedArgs)
  • Recommendation: Add whitelist validation for allowed operations to prevent potential method injection
  • Model validation exists (isValidModel()) but operation validation could be stronger

2. Query Parameter Handling

  • packages/server/src/api/rpc/index.ts:74-76 - Query parameter deserialization with basic try/catch
  • packages/server/src/express/middleware.ts:50-52 - URL reconstruction for query parsing
  • Recommendation: Add stricter validation for query parameter structure and size limits

3. JSON Deserialization

  • Multiple places use JSON.parse() and SuperJSON.deserialize() on user input
  • Current error handling catches exceptions but could benefit from input size limits

🚀 Performance Considerations

1. PostgreSQL Optimizations

  • Excellent PostgreSQL-specific optimizations in packages/runtime/src/client/crud/dialects/postgresql.ts:102-107
  • Proper timezone handling for PostgreSQL Date objects
  • Efficient JSONB operations for relation building (jsonb_build_object, jsonb_agg)

2. Query Builder Efficiency

  • Smart relation handling with lateral joins to avoid N+1 queries
  • Conditional nested select logic based on complexity (canJoinWithoutNestedSelect())

3. Serialization Performance

  • Safe JSON stringify helper for BigInt values in packages/common-helpers/src/safe-json-stringify.ts:4-12
  • SuperJSON integration for complex data types

🐛 Potential Issues

1. Memory Usage

  • No size limits on request body or query parameters
  • Recommendation: Add configurable limits for request size

2. Error Information Leakage

  • packages/server/src/api/rpc/index.ts:176 - Generic error responses expose full error messages
  • Recommendation: Consider sanitizing error messages in production

3. Express Middleware Edge Cases

  • packages/server/src/express/middleware.ts:39-45 - Error handling when sendResponse=false could be improved
  • Potential memory leak with res.locals if responses aren't consumed

📋 Code Quality Observations

1. Type Safety

  • Excellent use of TypeScript generics and type constraints
  • Proper type definitions in packages/server/src/types.ts
  • Good separation of concerns between types and implementation

2. Logging & Debugging

  • Comprehensive debug logging with configurable log levels
  • Safe JSON stringification for logs avoiding circular references

3. Package Structure

  • Clean package.json with proper exports and peer dependencies
  • Good separation between API core and framework adapters

🔧 Recommendations

  1. Add input validation whitelist for allowed operations in RPC handler
  2. Implement request size limits to prevent DoS attacks
  3. Add rate limiting considerations to the middleware documentation
  4. Consider adding request timeout handling for long-running operations
  5. Add integration tests for error scenarios and edge cases

📊 Overall Assessment

This is a well-engineered release that significantly enhances ZenStack's server capabilities. The code demonstrates solid architectural principles, comprehensive error handling, and good performance optimizations. The main areas for improvement are around input validation hardening and adding configurable security limits.

Recommendation: ✅ Approve with minor security enhancements suggested above.


Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR merges the development branch to main, releasing version 3.0.0-beta.13. The changes primarily focus on introducing a new @zenstackhq/server package that provides framework-agnostic API handlers and Express middleware support, along with dependency management improvements and bug fixes.

  • Introduced @zenstackhq/server package with RPC API handler and Express adapter
  • Centralized decimal.js dependency management using pnpm catalog
  • Enhanced error handling with model information and improved foreign key constraints

Reviewed Changes

Copilot reviewed 45 out of 46 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
packages/server/* New server package with API handlers, Express middleware, and comprehensive tests
packages/runtime/src/client/errors.ts Added model field to InputValidationError and NotFoundError constructors
packages/runtime/src/client/crud/validator/index.ts Updated to pass model name when throwing InputValidationError
packages/runtime/src/client/helpers/schema-db-pusher.ts Added default foreign key cascade actions
packages/runtime/src/client/crud/dialects/postgresql.ts Fixed bytea deserialization for hex-encoded strings in JSON
packages/common-helpers/src/safe-json-stringify.ts Moved safeJSONStringify to common-helpers package
packages/testtools/src/client.ts Replaced invariant with fallback value for test name
pnpm-workspace.yaml Added decimal.js to catalog
*/package.json Version bumps to 3.0.0-beta.13 and decimal.js catalog references
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.

@ymc9 ymc9 added this pull request to the merge queue Oct 24, 2025
Merged via the queue into main with commit 1b95b16 Oct 24, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants