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. WalkthroughPostgres JSON array containment now preserves input arrays instead of always wrapping values; SQLite rejects array-valued inputs for array_contains and switches json_each→jsonb_each; tests add a PostgreSQL-specific assertion for array_contains with an array argument. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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)
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 Complete ✅I've analyzed the array_contains filter fixes across PostgreSQL/SQLite dialects. Here's my comprehensive review: Todo List:
SummaryThis PR fixes the Code Quality & Best Practices ✅PostgreSQL Changes (packages/orm/src/client/crud/dialects/postgresql.ts:471-474)
SQLite Changes (packages/orm/src/client/crud/dialects/sqlite.ts:377-382, 399)
Bug Fixes & Correctness ✅
Security Concerns ✅
Performance Considerations ✅PostgreSQL:
SQLite:
Test Coverage ✅New Test (tests/e2e/orm/client-api/json-filter.test.ts:762-773):
Recommendations
if ((db.$schema.provider.type as any) === 'sqlite') {
await expect(
db.foo.findFirst({
where: {
data: {
path: '$.tags',
array_contains: ['react'],
},
},
})
).toBeRejected();
}
Overall Assessment ⭐⭐⭐⭐⭐Excellent PR - Addresses real bugs with appropriate database-specific solutions. The PostgreSQL fix enables proper array containment checking, while the SQLite fix prevents runtime errors with clear messaging. The Changes are minimal, focused, and well-tested. No breaking changes, proper error handling, and maintains backward compatibility for existing single-value usage. |
![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
♻️ Duplicate comments (1)
packages/orm/src/client/crud/dialects/sqlite.ts (1)
399-399: Duplicate issue: Usejson_eachinstead ofjsonb_each.Same issue as flagged in lines 377-383. SQLite does not support
jsonb_each.Apply this diff:
- .selectFrom(this.eb.fn('jsonb_each', [receiver]).as('$items')) + .selectFrom(this.eb.fn('json_each', [receiver]).as('$items'))
🧹 Nitpick comments (1)
tests/e2e/orm/client-api/json-filter.test.ts (1)
762-774: Avoidas any+ align provider detection with the rest of the test file.This PG-only assertion is great coverage, but
if ((db.$schema.provider.type as any) === 'postgresql')is brittle and inconsistent with the existinggetTestDbProvider()usage (Line 534). Consider:- if ((db.$schema.provider.type as any) === 'postgresql') { + if (getTestDbProvider() === 'postgresql') { await expect( db.foo.findFirst({ where: { data: { path: '$.tags', array_contains: ['react'], }, }, }), ).resolves.toMatchObject({ data: { tags: ['typescript', 'react', 'node'] } }); }Based on learnings, tests/e2e/**/* should stay harness-driven and portable across providers.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/orm/src/client/crud/dialects/postgresql.ts(1 hunks)packages/orm/src/client/crud/dialects/sqlite.ts(3 hunks)tests/e2e/orm/client-api/json-filter.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
tests/e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
E2E tests should validate real-world schema compatibility with established projects
Files:
tests/e2e/orm/client-api/json-filter.test.ts
🧠 Learnings (3)
📚 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/postgresql.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests
Applied to files:
tests/e2e/orm/client-api/json-filter.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
Applied to files:
tests/e2e/orm/client-api/json-filter.test.ts
🧬 Code graph analysis (2)
packages/orm/src/client/crud/dialects/postgresql.ts (1)
packages/orm/src/helpers.ts (1)
sql(1-1)
packages/orm/src/client/crud/dialects/sqlite.ts (1)
packages/orm/src/client/errors.ts (1)
createNotSupportedError(143-145)
⏰ 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)
- GitHub Check: Agent
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: build-test (20.x, sqlite)
🔇 Additional comments (3)
packages/orm/src/client/crud/dialects/postgresql.ts (1)
471-474: LGTM! Normalization logic correctly implements JSONB containment semantics.The array normalization ensures the PostgreSQL
@>operator receives a properly formatted JSONB array for containment checks. Whenvalueis already an array, it's used directly; when it's a scalar, wrapping it in an array enables the containment check to work correctly.packages/orm/src/client/crud/dialects/sqlite.ts (2)
17-17: LGTM!The added imports support the new error handling for unsupported array operations in SQLite.
378-380: No action needed—behavior is intentional and properly tested.The array rejection for
array_containsin SQLite is a deliberate design difference from PostgreSQL. The test suite already accounts for this: the test at line 768 ofjson-filter.test.tsthat usesarray_contains: ['react']is wrapped in a conditional (if ((db.$schema.provider.type as any) === 'postgresql')) and only runs for PostgreSQL, which natively supports array containment checks. SQLite's implementation restricts the input to single values and provides a clear error message when arrays are passed, which is the expected behavior for this database.
There was a problem hiding this comment.
Pull request overview
This PR fixes issues with the array_contains filter operation for JSON fields, addressing provider-specific behavior differences between PostgreSQL and SQLite. The main fix involves allowing PostgreSQL to accept both single values and arrays (leveraging the @> containment operator), while SQLite explicitly rejects arrays since it can only check for single values using jsonb_each. Additionally, a bug in SQLite's buildArrayItemFilter was corrected to use jsonb_each instead of json_each.
Key changes:
- PostgreSQL's
array_containsnow conditionally wraps single values in an array, allowing both single values and arrays as input - SQLite's
array_containsnow throws aNOT_SUPPORTEDerror when an array is passed, with added validation - Fixed SQLite's
buildArrayItemFilterto usejsonb_eachinstead ofjson_eachfor consistency
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/orm/src/client/crud/dialects/postgresql.ts | Modified array_contains to conditionally wrap values, supporting both single values and arrays |
| packages/orm/src/client/crud/dialects/sqlite.ts | Added array validation to reject arrays in array_contains, fixed json_each to jsonb_each in buildArrayItemFilter, imported createNotSupportedError |
| tests/e2e/orm/client-api/json-filter.test.ts | Added PostgreSQL-specific test for array_contains with array value ['react'] |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Claude finished @ymc9's task —— View job Code Review Complete ✅I've analyzed the array_contains filter fixes across PostgreSQL/SQLite dialects. Here's my comprehensive review: Todo List:
SummaryThis PR fixes the Code Quality & Best Practices ✅PostgreSQL Changes (packages/orm/src/client/crud/dialects/postgresql.ts:471-474)
SQLite Changes (packages/orm/src/client/crud/dialects/sqlite.ts:377-382, 399)
Bug Fixes & Correctness ✅
Security Concerns ✅
Performance Considerations ✅PostgreSQL:
SQLite:
Test Coverage ✅New Test (tests/e2e/orm/client-api/json-filter.test.ts:762-773):
Recommendations
if ((db.$schema.provider.type as any) === 'sqlite') {
await expect(
db.foo.findFirst({
where: {
data: {
path: '$.tags',
array_contains: ['react'],
},
},
})
).toBeRejected();
}
Overall Assessment ⭐⭐⭐⭐⭐Excellent PR - Addresses real bugs with appropriate database-specific solutions. The PostgreSQL fix enables proper array containment checking, while the SQLite fix prevents runtime errors with clear messaging. The Changes are minimal, focused, and well-tested. No breaking changes, proper error handling, and maintains backward compatibility for existing single-value usage. |
1 participant
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.