refactor: revised error system #377
Conversation
|
Claude finished @ymc9's task —— View job PR Review: Error System RefactoringI've completed a comprehensive review of this error system refactoring. This is a well-structured improvement that standardizes error handling across ZenStack. Todo List:
✅ Strengths1. Well-structured Error Hierarchy
2. Comprehensive Error Context
3. Consistent API Integration
4. Good Test Coverage
🔍 Areas for Improvement1. Error Processor Implementation ( export function getDbErrorCode(error: unknown): unknown | undefined {
if (error instanceof Error && 'code' in error) {
return error.code;
} else {
return undefined;
}
}
2. Generic Error Type for dbErrorCode ( public dbErrorCode?: unknown;
3. Security Considerations
4. Error Message Consistency (
🚀 Performance Considerations1. Error Creation Performance
2. Error Processing Overhead
🔒 Security Assessment1. Information Disclosure Risks
2. Error Handling Robustness
📋 Minor Issues1. Type Safety ( export function createDBQueryError(message: string, dbError: unknown, sql: string, parameters: readonly unknown[]) {
2. Documentation
3. Deleted Test File
🎯 Recommendations
Overall AssessmentThis is a solid refactoring that significantly improves error handling consistency across ZenStack. The new error system is well-designed, properly tested, and integrates cleanly with existing APIs. The main concerns are around information disclosure and the incomplete error processor implementation. Recommendation: ✅ Approve with minor follow-ups |
|
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. WalkthroughConsolidates many legacy ORM error classes into a single ORMError/ORMErrorReason surface with factory creators; updates numerous throw sites, imports, server error handlers, policy plugin utilities, tests, and docs to use the new error model and adjust assertions accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code graph analysis (2)packages/server/src/api/rpc/index.ts (1)
packages/orm/src/client/crud/operations/base.ts (1)
⏰ 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)
🔇 Additional comments (4)
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 |
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors the error handling system in the ZenStack ORM to use a unified ORMError class with reason codes instead of multiple specific error classes (like InputValidationError, NotFoundError, QueryError, etc.).
- Introduces a new
ORMErrorclass withORMErrorReasonenum for categorizing errors - Replaces specific error classes with factory functions that create
ORMErrorinstances - Updates all error handling code across the ORM, plugins, server packages, and tests
- Improves error information by adding fields like
dbErrorCode,sql,sqlParams, andrejectedByPolicyReason - Updates documentation to use "ORM Client" terminology instead of "Prisma Client"
Reviewed Changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/orm/src/client/errors.ts | Introduces new ORMError class, ORMErrorReason enum, and factory functions to replace old error class hierarchy |
| packages/orm/src/client/executor/error-processor.ts | Adds utility to extract database error codes from driver errors |
| packages/orm/src/client/index.ts | Updates exports to only expose ORMError, ORMErrorReason, and RejectedByPolicyReason |
| packages/orm/src/client/executor/zenstack-query-executor.ts | Updates query executor to use new error creation functions and wrap database errors properly |
| packages/orm/src/client/query-utils.ts | Replaces error throws with new factory functions |
| packages/orm/src/client/crud/operations/*.ts | Updates create, update, and delete operations to use new error factory functions |
| packages/orm/src/client/crud/validator/index.ts | Updates input validation to use createInvalidInputError |
| packages/orm/src/client/crud/dialects/*.ts | Updates dialect implementations to use new error functions |
| packages/orm/src/client/client-impl.ts | Updates client implementation to use new error types |
| packages/plugins/policy/src/*.ts | Updates policy plugin to use new error system with local helper functions |
| packages/server/src/api/rest/index.ts | Refactors REST API error handling to use pattern matching on ORMErrorReason |
| packages/server/src/api/rpc/index.ts | Refactors RPC API error handling to use pattern matching on ORMErrorReason |
| packages/testtools/src/vitest-ext.ts | Updates test matchers to check for ORMError with specific reasons instead of specific error classes |
| tests/e2e/**/*.test.ts | Updates tests to use new error checking patterns and test matchers |
| packages/language/res/stdlib.zmodel | Updates documentation from "Prisma Client" to "ORM Client" |
| README.md | Improves heading capitalization for consistency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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 (2)
packages/orm/src/client/crud/operations/base.ts (1)
2128-2144: Preserve the model when throwing not-found errorsHere the first argument to
createNotFoundErroris supposed to be the model name, but passing'No rows found'setserr.modelto that string while the message falls back to a generic template. Downstream error responses will therefore report the wrong model and omit the intended detail. Pass the actual model (and the custom message if you need it) instead:- throw createNotFoundError('No rows found'); + throw createNotFoundError(model, 'No rows found');packages/orm/src/client/query-utils.ts (1)
16-177: Restore client-facing error classificationAll of these guard paths used to raise a query-level error when the caller supplied an unknown model/field/relation. After this refactor they bubble up as
ORMErrorReason.INTERNAL_ERROR, which our HTTP and RPC adapters convert into 500s and “unexpected error” messaging. That masks actionable user mistakes (bad model name, missing relation, etc.) and regresses client UX. Please switch these cases tocreateInvalidInputError(and import it) so they remain 4xx-style signals.-import { createInternalError } from './errors'; +import { createInternalError, createInvalidInputError } from './errors'; @@ - throw createInternalError(`Model "${model}" not found in schema`, model); + throw createInvalidInputError(`Model "${model}" not found in schema`, model); @@ - throw createInternalError(`Field "${field}" not found in model "${modelOrType}"`, modelOrType); + throw createInvalidInputError( + `Field "${field}" not found in model "${modelOrType}"`, + modelOrType, + ); @@ - throw createInternalError(`Field "${field}" not found in type "${modelOrType}"`, modelOrType); + throw createInvalidInputError( + `Field "${field}" not found in type "${modelOrType}"`, + modelOrType, + ); @@ - throw createInternalError(`Model or type "${modelOrType}" not found in schema`, modelOrType); + throw createInvalidInputError(`Model or type "${modelOrType}" not found in schema`, modelOrType); @@ - throw createInternalError(`Model "${model}" does not have ID field(s)`, model); + throw createInvalidInputError(`Model "${model}" does not have ID field(s)`, model); @@ - throw createInternalError(`Field "${relationField}" is not a relation`, model); + throw createInvalidInputError(`Field "${relationField}" is not a relation`, model); @@ - throw createInternalError(`Relation references not defined for field "${relationField}"`, model); + throw createInternalError(`Relation references not defined for field "${relationField}"`, model); @@ - throw createInternalError(`Opposite relation not defined for field "${relationField}"`, model); + throw createInvalidInputError(`Opposite relation not defined for field "${relationField}"`, model); @@ - throw createInternalError(`Field "${fieldDef.relation.opposite}" is not a relation`, model); + throw createInvalidInputError(`Field "${fieldDef.relation.opposite}" is not a relation`, model); @@ - throw createInternalError(`Relation fields not defined for field "${relationField}"`, model); + throw createInternalError(`Relation fields not defined for field "${relationField}"`, model); @@ - throw createInternalError(`Relation references not defined for field "${relationField}"`, model); + throw createInternalError(`Relation references not defined for field "${relationField}"`, model); @@ - throw createInternalError(`ID fields not defined for model "${model}"`, model); + throw createInvalidInputError(`ID fields not defined for model "${model}"`, model);Feel free to keep
createInternalErrorfor the true schema corruption paths (missingreferences, etc.), but the user-facing validation failures need to map toINVALID_INPUTagain.
🧹 Nitpick comments (4)
packages/testtools/src/vitest-ext.ts (3)
76-89: Update error message to reference ORMError.Line 86 still mentions "NotFoundError" in the error message, but should reference "ORMError with reason NOT_FOUND" for consistency with the new error system.
Apply this diff:
return { - message: () => `expected NotFoundError, got no error`, + message: () => `expected ORMError with reason NOT_FOUND, got no error`, pass: false, };
91-110: Update error message to reference ORMError.Line 107 still mentions "PolicyError" in the error message, but should reference "ORMError with reason REJECTED_BY_POLICY" for consistency with the new error system.
Apply this diff:
return { - message: () => `expected PolicyError, got no error`, + message: () => `expected ORMError with reason REJECTED_BY_POLICY, got no error`, pass: false, };
112-131: Update error message to reference ORMError.Line 128 still mentions "InputValidationError" in the error message, but should reference "ORMError with reason INVALID_INPUT" for consistency with the new error system.
Apply this diff:
return { - message: () => `expected InputValidationError, got no error`, + message: () => `expected ORMError with reason INVALID_INPUT, got no error`, pass: false, };packages/orm/src/client/errors.ts (1)
86-128: PopulatedbErrorMessagewhen wrapping driver errors
ORMError.dbErrorMessageis documented as “The error message given by the underlying database driver,” butcreateDBQueryErrornever sets it, so consumers always seeundefined. Please capture the driver message alongside the code.export function createDBQueryError(message: string, dbError: unknown, sql: string, parameters: readonly unknown[]) { const error = new ORMError(ORMErrorReason.DB_QUERY_ERROR, message, { cause: dbError }); error.dbErrorCode = getDbErrorCode(dbError); + if (dbError instanceof Error) { + error.dbErrorMessage = dbError.message; + } else if (dbError != null) { + error.dbErrorMessage = String(dbError); + } error.sql = sql; error.sqlParams = parameters; return error; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
README.md(3 hunks)packages/language/res/stdlib.zmodel(1 hunks)packages/orm/src/client/client-impl.ts(3 hunks)packages/orm/src/client/crud/dialects/base-dialect.ts(8 hunks)packages/orm/src/client/crud/dialects/postgresql.ts(2 hunks)packages/orm/src/client/crud/dialects/sqlite.ts(3 hunks)packages/orm/src/client/crud/operations/base.ts(32 hunks)packages/orm/src/client/crud/operations/create.ts(2 hunks)packages/orm/src/client/crud/operations/delete.ts(2 hunks)packages/orm/src/client/crud/operations/update.ts(4 hunks)packages/orm/src/client/crud/validator/index.ts(3 hunks)packages/orm/src/client/crud/validator/utils.ts(2 hunks)packages/orm/src/client/errors.ts(2 hunks)packages/orm/src/client/executor/error-processor.ts(1 hunks)packages/orm/src/client/executor/zenstack-query-executor.ts(3 hunks)packages/orm/src/client/index.ts(1 hunks)packages/orm/src/client/query-utils.ts(9 hunks)packages/plugins/policy/src/expression-transformer.ts(7 hunks)packages/plugins/policy/src/policy-handler.ts(11 hunks)packages/plugins/policy/src/utils.ts(2 hunks)packages/server/src/api/rest/index.ts(4 hunks)packages/server/src/api/rpc/index.ts(3 hunks)packages/server/test/api/rest.test.ts(0 hunks)packages/testtools/src/vitest-ext.ts(4 hunks)tests/e2e/orm/client-api/delegate.test.ts(1 hunks)tests/e2e/orm/client-api/error-handling.test.ts(1 hunks)tests/e2e/orm/client-api/find.test.ts(4 hunks)tests/e2e/orm/client-api/mixin.test.ts(1 hunks)tests/e2e/orm/client-api/update.test.ts(1 hunks)tests/e2e/orm/plugin-infra/on-kysely-query.test.ts(1 hunks)tests/e2e/orm/policy/crud/update.test.ts(2 hunks)tests/e2e/orm/policy/migrated/auth.test.ts(1 hunks)tests/e2e/orm/policy/migrated/deep-nested.test.ts(1 hunks)tests/e2e/orm/policy/migrated/multi-field-unique.test.ts(3 hunks)tests/e2e/orm/policy/migrated/todo-sample.test.ts(0 hunks)
💤 Files with no reviewable changes (2)
- tests/e2e/orm/policy/migrated/todo-sample.test.ts
- packages/server/test/api/rest.test.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-21T16:04:56.292Z
Learnt from: ymc9
Repo: zenstackhq/zenstack-v3 PR: 319
File: packages/runtime/src/client/crud/dialects/base-dialect.ts:745-747
Timestamp: 2025-10-21T16:04:56.292Z
Learning: In packages/runtime/src/client/crud/dialects/base-dialect.ts, it's intentional that buildCursorFilter applies default ordering (via makeDefaultOrderBy fallback) while buildOrderBy does not. This ensures cursor-based pagination always has stable ordering for correctness, while regular queries remain unordered unless explicitly specified. This design is to be consistent with Prisma's pagination requirements.
Applied to files:
packages/orm/src/client/crud/dialects/base-dialect.ts
📚 Learning: 2025-10-21T16:09:31.218Z
Learnt from: ymc9
Repo: zenstackhq/zenstack-v3 PR: 319
File: packages/runtime/src/client/executor/zenstack-query-executor.ts:63-72
Timestamp: 2025-10-21T16:09:31.218Z
Learning: In ZenStack, TypeDefs can be inherited by models. When a TypeDef contains fields with `map` attributes, those mapped field names need to be processed by the QueryNameMapper since they become part of the inheriting model's schema. Therefore, when checking if a schema has mapped names (e.g., in `schemaHasMappedNames`), both `schema.models` and `schema.typeDefs` must be inspected for `@map` and `map` attributes.
Applied to files:
packages/orm/src/client/crud/dialects/sqlite.tspackages/orm/src/client/executor/zenstack-query-executor.tspackages/orm/src/client/query-utils.ts
🔇 Additional comments (46)
packages/language/res/stdlib.zmodel (1)
385-386: LGTM! Documentation terminology updated consistently.The documentation changes align well with the PR's objective to adopt ORM-centric terminology. The additional clarification that ignored fields/models are still recognized by database schema migrations is a helpful detail for users.
Also applies to: 391-392
packages/orm/src/client/crud/validator/utils.ts (2)
16-16: LGTM! Import correctly updated to use error factory.The import statement correctly brings in the
createNotSupportedErrorfactory function, aligning with the PR's error system refactoring.
454-456: Verify consistency with other "unsupported" error cases.The use of
createNotSupportedErroris correct. However, line 314 throws a genericErrorfor unsupported unary operators, which is conceptually similar. Consider updating line 314 for consistency:- throw new Error(`Unsupported unary operator: ${expr.op}`); + throw createNotSupportedError(`Unsupported unary operator: ${expr.op}`);Run the following script to check for other generic Error throws that might need similar updates:
tests/e2e/orm/plugin-infra/on-kysely-query.test.ts (1)
214-214: LGTM: Error assertion updated for new error structure.The test now correctly checks the nested
cause.messageproperty, aligning with the new error wrapping approach where database/plugin errors are wrapped in ORMError instances.packages/orm/src/client/executor/error-processor.ts (1)
1-12: Function looks good, but note the TODO for future consideration.The implementation safely checks for the
codeproperty before accessing it. The TODO on line 4 indicates awareness that different database drivers may structure errors differently. Consider documenting which database drivers are currently supported or add driver-specific code extraction logic if needed in the future.packages/testtools/src/vitest-ext.ts (1)
1-20: LGTM: Error checking refactored to use unified ORMError system.The rename from
expectErrortoexpectErrorReasonand the updated logic correctly validatesORMErrorinstances with the appropriateORMErrorReason.packages/orm/src/client/executor/zenstack-query-executor.ts (2)
107-122: LGTM: Error handling correctly updated to use error factories.The logic appropriately preserves existing
ORMErrorinstances and wraps other errors usingcreateDBQueryErrorwith helpful context (SQL and parameters). This maintains the error chain while providing debugging information.
367-369: LGTM: Internal error creation updated to use factory.The change from
new InternalError(...)tocreateInternalError(...)aligns with the new error factory pattern.packages/orm/src/client/crud/dialects/postgresql.ts (1)
438-442: LGTM: Error creation updated to use factory.The change from throwing
QueryErrorto usingcreateInternalErroris appropriate for this internal error case and aligns with the new error factory pattern.tests/e2e/orm/client-api/delegate.test.ts (1)
185-185: LGTM: Error assertion updated for new error structure.The test now correctly checks the nested
cause.messageproperty with case-insensitive matching, which is robust for different database driver error formats.tests/e2e/orm/client-api/mixin.test.ts (1)
78-78: LGTM: Error assertion updated for new error structure.The test now correctly checks the nested
cause.messageproperty, maintaining the case-insensitive matching from the original regex pattern.tests/e2e/orm/policy/migrated/auth.test.ts (1)
539-541: LGTM: Error assertion updated for new error structure.The test now correctly checks the nested
cause.messageproperty with case-insensitive matching. The multi-line format improves readability.tests/e2e/orm/client-api/update.test.ts (1)
1053-1053: LGTM! Error assertion updated to new error structure.The test now correctly inspects the error's cause chain, checking that
e.cause.messagecontains 'constraint'. This aligns with the broader error system refactoring where database errors are wrapped with cause information.tests/e2e/orm/policy/migrated/deep-nested.test.ts (1)
485-485: LGTM! Consistent error assertion pattern.The test correctly inspects the error cause chain for constraint violations, matching the pattern used across the test suite.
tests/e2e/orm/policy/crud/update.test.ts (1)
761-761: LGTM! Consistent error assertions.Both test assertions correctly check the error cause chain for constraint violations, aligning with the new error structure.
Also applies to: 1127-1127
packages/orm/src/client/crud/dialects/sqlite.ts (1)
15-15: LGTM! Error factory usage is correct.The migration from
QueryErrortocreateInternalErroris properly implemented across all three error sites. The JSON parsing error correctly preserves the cause chain, and the other two errors have appropriate messages.Also applies to: 124-124, 379-379, 382-382
packages/orm/src/client/client-impl.ts (1)
32-32: LGTM! Error factory functions properly implemented.The migration from error classes to factory functions (
createConfigErrorandcreateNotFoundError) is correct. Both error sites use the appropriate factory with proper arguments.Also applies to: 226-226, 392-392
packages/orm/src/client/index.ts (1)
6-6: LGTM! Public API surface appropriately scoped.The change from wildcard export to named exports limits the public API to only
ORMError,ORMErrorReason, andRejectedByPolicyReason. This is appropriate as consumers only need these types for error handling, while factory functions remain internal implementation details.packages/orm/src/client/crud/operations/update.ts (1)
4-4: LGTM! Consistent error factory usage.The migration from
RejectedByPolicyErrorclass tocreateRejectedByPolicyErrorfactory is correctly implemented across all three error sites (update, updateManyAndReturn, and upsert). Each call properly passes the model, reason (CANNOT_READ_BACK), and descriptive message.Also applies to: 64-68, 123-127, 171-175
README.md (1)
38-84: LGTM! Documentation improvements.The heading updates improve consistency with title case formatting, and the added manual setup instructions provide clearer guidance for users.
tests/e2e/orm/client-api/error-handling.test.ts (1)
1-49: LGTM! Comprehensive error handling test coverage.The test suite properly validates the new ORMError-based error handling system across different error scenarios: validation errors, not found errors, policy rejection, and database constraint violations. The provider-specific error code handling (lines 42-43) is appropriate.
packages/orm/src/client/crud/operations/create.ts (2)
4-4: LGTM! Import updated to use error factory.The import correctly switches from the
RejectedByPolicyErrorclass to thecreateRejectedByPolicyErrorfactory function, aligning with the unified error system refactor.
51-55: LGTM! Error construction updated to use factory.The error creation now uses the factory function instead of direct class instantiation, maintaining the same error semantics while aligning with the new error handling approach.
tests/e2e/orm/client-api/find.test.ts (3)
55-56: LGTM! Test assertion updated for new error system.The assertion now uses the
toBeRejectedByValidation()matcher, which is cleaner and aligns with the unified error handling approach.
391-392: LGTM! Test assertion updated for not-found errors.The assertion correctly uses the
toBeRejectedNotFound()matcher for thefindUniqueOrThrowcase.
405-406: LGTM! Test assertion updated consistently.The
findFirstOrThrowtest correctly uses the sametoBeRejectedNotFound()matcher, maintaining consistency across similar test cases.packages/plugins/policy/src/utils.ts (2)
162-171: LGTM! Error factory for policy rejection.The
createRejectedByPolicyErrorfactory function properly constructs anORMErrorwith the appropriate reason and metadata (model, rejectedByPolicyReason), providing a centralized way to create policy-related errors.
173-175: LGTM! Error factory for unsupported operations.The
createUnsupportedErrorfactory provides a clean way to create NOT_SUPPORTED errors, aligning with the unified error handling approach.packages/plugins/policy/src/expression-transformer.ts (6)
43-51: LGTM! Updated imports for error factory.The import statement now includes
createUnsupportedErrorfrom the utils module, enabling the use of the centralized error factory throughout this file.
95-96: LGTM! Appropriate use of invariant.Switching from throwing an error to using
invariant(false, ...)is appropriate here, as this represents a programming invariant violation rather than a user-facing error scenario.
301-303: LGTM! Error factory used for unsupported operators.The error throw now uses
createUnsupportedError, providing consistent error handling for unsupported operators in policy expressions.
321-323: LGTM! Consistent error factory usage.The unsupported
auth()comparison scenario now uses the error factory, maintaining consistency with other error paths.
390-391: LGTM! Factory used for unimplemented functions.Function implementation errors now use the centralized factory, providing consistent error reporting.
447-447: LGTM! Consistent argument error handling.Unsupported argument expressions now use the error factory, completing the consistent error handling refactor in this file.
packages/orm/src/client/crud/dialects/base-dialect.ts (9)
17-17: LGTM! Error factory imports added.The import statement now includes the three error factory functions needed throughout this file, enabling the transition from direct error class instantiation to factory-based error creation.
98-99: LGTM! Factory used for unsupported distinct.The error for unsupported
distinctoperations now usescreateNotSupportedError, providing consistent error handling across the codebase.
485-486: LGTM! Validation error uses factory.Invalid array filter keys now use
createInvalidInputError, maintaining consistent validation error reporting.
513-514: LGTM! Appropriate use of not-supported error.The JSON filter limitation is correctly reported using
createNotSupportedError, clearly indicating this is a feature gap rather than invalid user input.
516-517: LGTM! Validation error for unsupported fields.Attempting to filter on
Unsupportedfield types correctly usescreateInvalidInputError.
592-592: LGTM! Factory used for invalid filter keys.The error factory provides consistent handling for invalid filter keys in the standard filter builder.
645-646: LGTM! String filter validation uses factory.Invalid string filter keys now use the error factory, maintaining consistency with other validation errors.
818-819: LGTM! OrderBy validation error.Invalid orderBy values for to-many relations correctly use
createInvalidInputError.
1087-1088: LGTM! Configuration error factory used.Missing computed field implementations now use
createConfigError, which is the appropriate error type for configuration issues rather than user input or runtime problems.packages/orm/src/client/crud/operations/delete.ts (3)
4-4: LGTM! Imports updated to error factories.The import statement correctly switches from error classes to factory functions, aligning with the unified error handling refactor.
37-38: LGTM! Not-found error uses factory.The error creation for when no record is deleted now uses the
createNotFoundErrorfactory function, maintaining the same error semantics.
43-47: LGTM! Policy rejection error uses factory.The policy rejection error construction now uses the factory function with appropriate reason and message, consistent with the error handling refactor.
|
Claude finished @ymc9's task —— View job PR Review: Error System RefactoringI've completed a comprehensive review of this error system refactoring. This is a well-structured improvement that standardizes error handling across ZenStack. Todo List:
✅ Strengths1. Well-structured Error Hierarchy
2. Comprehensive Error Context
3. Consistent API Integration
4. Good Test Coverage
🔍 Areas for Improvement1. Error Processor Implementation ( export function getDbErrorCode(error: unknown): unknown | undefined {
if (error instanceof Error && 'code' in error) {
return error.code;
} else {
return undefined;
}
}
2. Generic Error Type for dbErrorCode ( public dbErrorCode?: unknown;
3. Security Considerations
4. Error Message Consistency (
🚀 Performance Considerations1. Error Creation Performance
2. Error Processing Overhead
🔒 Security Assessment1. Information Disclosure Risks
2. Error Handling Robustness
📋 Minor Issues1. Type Safety ( export function createDBQueryError(message: string, dbError: unknown, sql: string, parameters: readonly unknown[]) {
2. Documentation
3. Deleted Test File
🎯 Recommendations
Overall AssessmentThis is a solid refactoring that significantly improves error handling consistency across ZenStack. The new error system is well-designed, properly tested, and integrates cleanly with existing APIs. The main concerns are around information disclosure and the incomplete error processor implementation. Recommendation: ✅ Approve with minor follow-ups |
Summary by CodeRabbit
New Features
Documentation
Breaking Changes
Tests