chore(orm): use "json_each" instead of "jsonb_each" for better compatibility by ymc9 · Pull Request #484 · zenstackhq/zenstack-v3

ymc9 · 2025-12-13T06:22:37Z

Summary by CodeRabbit

Bug Fixes
- Refined SQLite JSON array filtering and existence checking operations for improved accuracy.
Tests
- Enhanced test suite type safety and removed unnecessary debug logging configurations.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

…ibility

coderabbitai · 2025-12-13T06:22:46Z

Walkthrough

Changes include switching JSON functions from jsonb_each to json_each in SQLite dialect operations for array filtering and existence predicates, and removing unnecessary type assertions from test client initialization across multiple test files. One test removes a debug flag configuration.

Changes

Cohort / File(s)	Change Summary
SQLite Dialect JSON Processing `packages/orm/src/client/crud/dialects/sqlite.ts`	In `buildJsonArrayFilter` and `buildJsonArrayExistsPredicate`, replaced `jsonb_each` with `json_each` when iterating over JSON arrays for comparison and existence evaluation operations.
Test Client Type Assertions Removal `tests/e2e/orm/client-api/aggregate.test.ts`, `count.test.ts`, `create-many-and-return.test.ts`, `create-many.test.ts`, `create.test.ts`, `delete-many.test.ts`, `delete.test.ts`, `find.test.ts`, `group-by.test.ts`, `raw-query.test.ts`, `transaction.test.ts`, `undefined-values.test.ts`, `update-many.test.ts`, `update.test.ts` `tests/e2e/orm/plugin-infra/on-kysely-query.test.ts`, `on-query-hooks.test.ts` `tests/e2e/orm/policy/basic-schema-read.test.ts`	Removed explicit `as any` type casts from test client initialization in `beforeEach` hooks. Changed from `(await createTestClient(schema)) as any` to `await createTestClient(schema)`, enabling direct type inference without runtime casting.
Test Configuration Updates `tests/e2e/orm/policy/policy-functions.test.ts`	Removed `debug: true` option from `createPolicyTestClient` call in the "escapes input for contains" test.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

SQLite JSON function switch: The change from jsonb_each to json_each requires verification that this is compatible with SQLite's JSON handling and doesn't alter query semantics or behavior.
Test type assertion removals: While repetitive across many files, each removal is straightforward and low-risk; the pattern is consistent across all test files.
Debug flag removal: Single-line change with minimal impact, but should confirm the test behavior remains correct without the debug flag.

Possibly related PRs

feat(orm): implement json filters #472: Directly modifies buildJsonArrayFilter and buildJsonArrayExistsPredicate in the same SQLite dialect file.
fix(orm): "array_contains" filter fixes #480: Also modifies buildJsonArrayFilter and buildJsonArrayExistsPredicate in the same SQLite dialect module.

Poem

🐰 From jsonb_each to json_each we hop,
Type casts removed, no more "as any" crop,
Tests now run cleaner, more truthfully typed,
Debug flags silent, the way we're all wiped.
Simpler SQLite, simpler test file—
Code hops along with a whimsical smile! 🌱

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the primary change: switching from jsonb_each to json_each in SQLite dialect for better compatibility.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ 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 chore/sqlite-jsonb

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 · 2025-12-13T06:22:49Z

Claude finished @ymc9's task —— View job

Pull Request Review

Review Checklist

Read repository guidelines
Examine core SQLite dialect changes
Review test file modifications
Analyze compatibility implications
Provide comprehensive feedback on code quality, bugs, performance, security, and test coverage

Code Review Summary

This PR makes two main types of changes:

Core functionality: Replaces jsonb_each with json_each in SQLite dialect for better compatibility
Test cleanup: Removes unnecessary type assertions and debug flags

Detailed Feedback

✅ Code Quality & Best Practices

Excellent: The core change is well-targeted and follows SQLite best practices:

json_each is the standard SQLite JSON table-valued function
jsonb_each is PostgreSQL-specific and not supported in SQLite
Changes are isolated to SQLite-specific code paths (packages/orm/src/client/crud/dialects/sqlite.ts:383,401)

Good: Test cleanup improvements:

Removing unnecessary as any type assertions improves type safety
Removing { debug: true } from tests is appropriate for production code

✅ Compatibility & Performance

Critical Fix: This change resolves a serious compatibility issue:

jsonb_each would cause runtime errors in SQLite environments
json_each is supported in SQLite 3.38.0+ (released 2022-02-22)
No performance difference between the functions - both are efficient table-valued functions

✅ Security Considerations

No security concerns identified
JSON operations are properly parameterized using Kysely's SQL builder
No raw SQL injection vectors introduced

✅ Test Coverage

Comprehensive: Changes span 17 test files across:

Client API operations (CRUD, aggregation, transactions)
Plugin infrastructure
Policy enforcement

Appropriate scope: Only test setup changes, no test logic modifications needed since the API remains identical.

⚠️ Potential Considerations

SQLite Version Dependency:
- json_each requires SQLite 3.38.0+
- Consider documenting minimum SQLite version requirements
- Most environments (Node.js with better-sqlite3) should support this
Backward Compatibility:
- This fixes broken functionality rather than changing working behavior
- No breaking API changes for end users

Recommendation: ✅ APPROVE

This is a necessary bug fix that resolves SQLite runtime errors. The changes are:

Targeted: Only affects the broken SQLite paths
Safe: No API changes or logic modifications
Well-tested: Comprehensive test coverage across all affected areas
Standards-compliant: Uses correct SQLite JSON functions

The PR title accurately describes the change as a compatibility improvement rather than a feature addition.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

packages/orm/src/client/crud/dialects/sqlite.ts (1)

371-393: Prefer building array_contains with Kysely exists (avoid raw SQL + unaliased value).

This keeps the JSON array paths consistent with buildJsonArrayExistsPredicate, and avoids relying on the bare value column name from json_each(...).

-                    return sql<any>`EXISTS (SELECT 1 FROM json_each(${lhs}) WHERE value = ${value})`;
+                    return this.eb.exists(
+                        this.eb
+                            .selectFrom(this.eb.fn('json_each', [lhs]).as('$items'))
+                            .select(this.eb.lit(1).as('$t'))
+                            .whereRef('$items.value', '=', this.eb.val(value)),
+                    );

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cce7555 and a34bcc1.

📒 Files selected for processing (19)

packages/orm/src/client/crud/dialects/sqlite.ts (2 hunks)
tests/e2e/orm/client-api/aggregate.test.ts (1 hunks)
tests/e2e/orm/client-api/count.test.ts (1 hunks)
tests/e2e/orm/client-api/create-many-and-return.test.ts (1 hunks)
tests/e2e/orm/client-api/create-many.test.ts (1 hunks)
tests/e2e/orm/client-api/create.test.ts (1 hunks)
tests/e2e/orm/client-api/delete-many.test.ts (1 hunks)
tests/e2e/orm/client-api/delete.test.ts (1 hunks)
tests/e2e/orm/client-api/find.test.ts (1 hunks)
tests/e2e/orm/client-api/group-by.test.ts (1 hunks)
tests/e2e/orm/client-api/raw-query.test.ts (1 hunks)
tests/e2e/orm/client-api/transaction.test.ts (1 hunks)
tests/e2e/orm/client-api/undefined-values.test.ts (1 hunks)
tests/e2e/orm/client-api/update-many.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/plugin-infra/on-query-hooks.test.ts (1 hunks)
tests/e2e/orm/policy/basic-schema-read.test.ts (1 hunks)
tests/e2e/orm/policy/policy-functions.test.ts (0 hunks)

💤 Files with no reviewable changes (1)

tests/e2e/orm/policy/policy-functions.test.ts

🧰 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/undefined-values.test.ts
tests/e2e/orm/client-api/update-many.test.ts
tests/e2e/orm/client-api/create.test.ts
tests/e2e/orm/policy/basic-schema-read.test.ts
tests/e2e/orm/client-api/transaction.test.ts
tests/e2e/orm/client-api/raw-query.test.ts
tests/e2e/orm/client-api/delete.test.ts
tests/e2e/orm/plugin-infra/on-query-hooks.test.ts
tests/e2e/orm/client-api/count.test.ts
tests/e2e/orm/plugin-infra/on-kysely-query.test.ts
tests/e2e/orm/client-api/delete-many.test.ts
tests/e2e/orm/client-api/group-by.test.ts
tests/e2e/orm/client-api/find.test.ts
tests/e2e/orm/client-api/aggregate.test.ts
tests/e2e/orm/client-api/create-many.test.ts
tests/e2e/orm/client-api/create-many-and-return.test.ts
tests/e2e/orm/client-api/update.test.ts

🧠 Learnings (6)

📚 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/sqlite.ts