fix: workaround node-pg's issue with passing array to non-array JSON fields #74
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update introduces a new boolean parameter to the Changes
Poem
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 adds a workaround for node-pg’s mishandling of JSON arrays when passing to non-array JSON fields by introducing a forArrayField flag in transformPrimitive, and it extends type coverage tests to cover default values and array behavior for plain JSON fields.
- Introduces a third
forArrayFieldparameter ontransformPrimitiveand updates all its invocations. - Adds new tests for default values across supported types and for JSON arrays on non-array JSON fields.
- Implements the workaround in the Postgres dialect to JSON-stringify arrays for non-array JSON columns.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/runtime/test/client-api/type-coverage.test.ts | Added tests covering default values for all scalar types and plain JSON |
| packages/runtime/src/plugins/policy/utils.ts | Updated boolean ValueNode creation to pass forArrayField = false |
| packages/runtime/src/plugins/policy/policy-handler.ts | Passed forArrayField to transformPrimitive in policy handler |
| packages/runtime/src/plugins/policy/expression-transformer.ts | Updated transformValue to supply forArrayField = false |
| packages/runtime/src/client/crud/operations/base.ts | Expanded calls to transformPrimitive with forArrayField where needed |
| packages/runtime/src/client/crud/dialects/sqlite.ts | Updated SQLite dialect signature and ensured nested calls propagate flag |
| packages/runtime/src/client/crud/dialects/postgresql.ts | Workaround for JSON arrays on non-array JSON fields in Postgres dialect |
| packages/runtime/src/client/crud/dialects/base.ts | Changed base signature of transformPrimitive to accept forArrayField |
Comments suppressed due to low confidence (4)
packages/runtime/test/client-api/type-coverage.test.ts:55
- [nitpick] The default-values test covers all scalar types except
Bytes. To ensure full coverage, consider adding a default for theBytesfield and verifying it in the expectation.
model Foo {
packages/runtime/src/client/crud/dialects/base.ts:40
- The new
forArrayFieldparameter is not documented. Please update the JSDoc or add a comment explaining its purpose and default behavior.
transformPrimitive(value: unknown, _type: BuiltinType, _forArrayField: boolean) {
packages/runtime/src/client/crud/dialects/base.ts:40
- Requiring the third
forArrayFieldargument may break callers oftransformPrimitive. Consider making this parameter optional (with a default offalse) to maintain backward compatibility.
transformPrimitive(value: unknown, _type: BuiltinType, _forArrayField: boolean) {
packages/runtime/test/client-api/type-coverage.test.ts:71
- This test assumes the generated
idwill be '1', but the model uses@default(cuid()). Instead, capture the created record'sidfrom thecreatecall (e.g.,const created = await db.foo.create(...)) or provide a fixedidin the data.
await expect(db.foo.findUnique({ where: { id: '1' } })).resolves.toMatchObject({
Summary by CodeRabbit