fix: several fixes about Json and Unsupported field types #73
Conversation
WalkthroughThe updates introduce support for new built-in types, specifically Changes
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/runtime/src/client/crud/dialects/base.tsOops! Something went wrong! :( ESLint: 9.29.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by packages/runtime/src/client/crud/dialects/postgresql.tsOops! Something went wrong! :( ESLint: 9.29.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by packages/runtime/src/client/crud/dialects/sqlite.tsOops! Something went wrong! :( ESLint: 9.29.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull Request Overview
This PR extends support for JSON and introduces an Unsupported fallback type across schema definitions, code generation, and runtime behaviors, while also cleaning up build configuration and exports.
- Adds
JsonandUnsupportedto the set of built-in types and updates the TS schema generator - Implements JSON transformation in result processing, schema pushing, and SQLite dialects; updates filter validators
- Removes the
cliententry from thetsupbuild config and simplifies runtime exports
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/tanstack-query/src/react.ts | Updated import path to @zenstackhq/runtime |
| packages/sdk/src/ts-schema-generator.ts | Changed fallback literal from 'unknown' to 'Unsupported' |
| packages/sdk/src/schema/schema.ts | Extended BuiltinType with 'Json' and 'Unsupported' |
| packages/runtime/tsup.config.ts | Removed client entry from build configuration |
| packages/runtime/src/index.ts | Replaced named exports with export * from client |
| packages/runtime/src/client/result-processor.ts | Added Json branch and transformJson method |
| packages/runtime/src/client/helpers/schema-db-pusher.ts | Added mapping for Json fields in schema push |
| packages/runtime/src/client/crud/validator.ts | Updated filter schema to handle Json and Unsupported |
| packages/runtime/src/client/crud/dialects/sqlite.ts | Added JSON serialization for SQLite dialect |
| packages/runtime/src/client/crud/dialects/postgresql.ts | Adjusted Decimal handling; JSON transform missing |
| packages/runtime/src/client/crud/dialects/base.ts | Extended filter builder to include Json and Unsupported cases |
| packages/runtime/package.json | Removed client export definitions |
| .coderabbit.yaml | Disabled sequence diagrams |
Comments suppressed due to low confidence (1)
packages/runtime/tsup.config.ts:6
- Removing the
cliententry from tsup config prevents building the client bundle. Verify if this is intentional or re-add the entry to avoid missing artifacts.
schema: 'src/schema/index.ts',
![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
🧹 Nitpick comments (2)
packages/runtime/src/client/crud/dialects/sqlite.ts (1)
42-42: Consider adding error handling for JSON serialization.While the implementation is correct for SQLite's text-based JSON storage, consider adding error handling for cases where
JSON.stringify()might fail (e.g., circular references, BigInt values).-.with('Json', () => JSON.stringify(value)) +.with('Json', () => { + try { + return JSON.stringify(value); + } catch (error) { + throw new Error(`Failed to serialize JSON value: ${error.message}`); + } +})packages/runtime/src/client/result-processor.ts (1)
161-169: Consider adding error handling for JSON parsing failures.The provider-specific JSON transformation logic is sound, but consider adding error handling for JSON parsing failures in SQLite.
private transformJson(value: unknown) { return match(this.schema.provider.type) .with('sqlite', () => { // better-sqlite3 returns JSON as string invariant(typeof value === 'string', 'Expected string, got ' + typeof value); - return JSON.parse(value as string); + try { + return JSON.parse(value as string); + } catch (error) { + throw new Error(`Failed to parse JSON from SQLite: ${error.message}`); + } }) .otherwise(() => value); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.coderabbit.yaml(1 hunks)packages/runtime/package.json(0 hunks)packages/runtime/src/client/crud/dialects/base.ts(1 hunks)packages/runtime/src/client/crud/dialects/postgresql.ts(1 hunks)packages/runtime/src/client/crud/dialects/sqlite.ts(1 hunks)packages/runtime/src/client/crud/validator.ts(1 hunks)packages/runtime/src/client/helpers/schema-db-pusher.ts(1 hunks)packages/runtime/src/client/result-processor.ts(2 hunks)packages/runtime/src/index.ts(1 hunks)packages/runtime/test/client-api/type-coverage.test.ts(2 hunks)packages/runtime/tsconfig.test.json(0 hunks)packages/runtime/tsup.config.ts(0 hunks)packages/sdk/src/schema/schema.ts(1 hunks)packages/sdk/src/ts-schema-generator.ts(1 hunks)packages/tanstack-query/src/react.ts(1 hunks)
💤 Files with no reviewable changes (3)
- packages/runtime/tsconfig.test.json
- packages/runtime/tsup.config.ts
- packages/runtime/package.json
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/runtime/src/client/result-processor.ts (1)
packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)
packages/runtime/src/client/crud/dialects/base.ts (3)
packages/sdk/src/schema/schema.ts (1)
BuiltinType(76-86)packages/language/src/generated/ast.ts (1)
BuiltinType(100-100)packages/runtime/src/client/errors.ts (2)
InternalError(7-11)QueryError(1-5)
packages/runtime/test/client-api/type-coverage.test.ts (1)
packages/runtime/test/utils.ts (1)
createTestClient(75-149)
🔇 Additional comments (14)
.coderabbit.yaml (1)
7-7: Config key is valid and correctly scoped.
sequence_diagramsbelongs directly underreviews, not underauto_review, and the indentation reflects that. No further action needed.packages/tanstack-query/src/react.ts (1)
8-8: LGTM! Import path updated correctly.The import path change aligns with the export consolidation where client exports were moved from a separate subpath to the main runtime package.
packages/runtime/src/index.ts (1)
1-1: LGTM! Export consolidation implemented correctly.The wildcard export properly consolidates all client exports under the main runtime package, eliminating the need for separate export subpaths.
packages/runtime/src/client/crud/dialects/postgresql.ts (1)
40-40: LGTM! Decimal type transformation implemented correctly.The implementation properly handles null values and converts non-null decimal values to string representation, which is appropriate for PostgreSQL decimal handling.
packages/sdk/src/ts-schema-generator.ts (1)
698-699: Approved: Runtime already supports the ‘Unsupported’ typeRuntime components include explicit handling for the new BuiltinType “Unsupported,” so no further changes are needed:
- In packages/runtime/src/client/crud/validator.ts
.with('Unsupported', () => z.never())- In packages/runtime/src/client/crud/dialects/base.ts
.with('Unsupported', () => { throw new QueryError(...) })- In packages/runtime/src/client/helpers/schema-db-pusher.ts fallback throws
Unsupported field typeWith these in place, switching the fallback literal to
"Unsupported"is fully supported at runtime.packages/sdk/src/schema/schema.ts (1)
76-86: LGTM! Foundation for JSON and Unsupported type support.The extension of the
BuiltinTypeunion to include 'Json' and 'Unsupported' provides the necessary type foundation for the broader JSON and unsupported field type support across the codebase.packages/runtime/src/client/crud/validator.ts (1)
382-396: Appropriate schema validation choices for new types.The validation schema choices are well-reasoned:
z.any()for 'Json' appropriately allows any valid JSON valuez.never()for 'Unsupported' correctly prevents validation of unsupported fields- The exhaustive pattern matching ensures all cases are handled
The TODO comment indicates that more sophisticated JSON filtering schemas are planned for future implementation.
packages/runtime/src/client/crud/dialects/base.ts (1)
419-437: Appropriate error handling for unsupported filter operations.The error handling strategy is well-designed:
InternalErrorfor 'Json' correctly indicates a not-yet-implemented featureQueryErrorfor 'Unsupported' appropriately provides user-facing feedback- The exhaustive pattern matching ensures type safety and complete coverage
The parentheses around the match expression improve readability.
packages/runtime/src/client/result-processor.ts (1)
93-93: LGTM! Integration of JSON transformation into scalar processing.The addition of JSON transformation to the scalar processing pipeline is well-integrated and follows the existing pattern.
packages/runtime/test/client-api/type-coverage.test.ts (5)
5-6: LGTM: Database name constant is well-defined.The constant provides a clear, descriptive name for the PostgreSQL test database.
7-7: LGTM: Provider parameterization improves test coverage.The
describe.eachstructure effectively tests both SQLite and PostgreSQL providers, ensuring comprehensive coverage for the new JSON type support.
8-51: LGTM: Comprehensive plain scalar type testing.The test properly validates all scalar types including the newly added
Jsontype. The test data is comprehensive and the resource cleanup is handled correctly in the finally block.
53-100: LGTM: Array type testing with appropriate provider handling.The array test correctly skips SQLite (which doesn't support array types) while testing PostgreSQL. The test data comprehensively covers all array types including
Json[], and proper resource cleanup is ensured.
20-20: LGTM: JSON type integration is consistent and comprehensive.The
Jsontype is properly integrated into both test cases:
- Plain scalar:
Json: { foo: 'bar' }with model fieldJson Json- Array:
Json: [{ foo: 'bar' }]with model fieldJson Json[]This validates the new JSON type support across both scenarios.
Also applies to: 38-38, 69-69, 87-87
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores