feat(store): add sqlite record resolution - #80
Conversation
|
Warning Review limit reached
Next review available in: 47 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdded SQLite-specific record metadata, direct and custom codecs, validation utilities, immutable runtime resolution assets, adapter exports, documentation, shared SQL contract utilities, and dialect migrations. The synchronous resolver applies overrides and aggregates definition errors without I/O. ChangesSQLite record specialization
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/store/src/sql/sqlite/column-codecs.ts`:
- Around line 210-215: Update the text codec’s encode/decode validation and the
json-blob codec’s payload validation to reject strings containing unpaired
surrogates, while preserving the existing NUL checks. Reuse the established
isValidSqliteName-style validation or String.prototype.isWellFormed when
supported, ensuring invalid values fail before UTF-8 encoding or storage.
In `@packages/store/src/sql/sqlite/record-resolver.ts`:
- Around line 80-113: Extract the shared SQLite name-validation flow from the
table and column resolver blocks into one helper, parameterized by metadata
value, winning name, issue path, and message factory. Have the helper validate
an explicit name candidate, resolve the winning-name comparison, and suppress
the duplicate issue when the winning name is that candidate. Replace both table
and column logic with the helper while preserving their distinct labels, path
tails, and messages.
In `@packages/store/test/sql/sqlite/record.test.ts`:
- Around line 533-638: Add a dedicated record in the test fixture catalog that
uses nullableStringField with a SQLite column configured with notNull: true,
ensuring it reaches the explicit nullability-conflict validation rather than the
ROWID path. Extend the expected issue code and path arrays in
resolveSqliteRecords coverage with the new invalid-database-options entry in
catalog order, pointing to that record’s notNull configuration.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 292cd824-3049-4a4a-a75c-c408fc82a94c
📒 Files selected for processing (14)
.changeset/fair-tables-resolve.mdpackages/store/README.mdpackages/store/package.jsonpackages/store/src/sql/sqlite/adapter.tspackages/store/src/sql/sqlite/column-codecs.tspackages/store/src/sql/sqlite/column-resolution.tspackages/store/src/sql/sqlite/column-type-resolver.tspackages/store/src/sql/sqlite/index.tspackages/store/src/sql/sqlite/metadata.tspackages/store/src/sql/sqlite/record-resolver.tspackages/store/src/sql/sqlite/record.tspackages/store/src/sql/sqlite/resolution-types.tspackages/store/src/sql/sqlite/sqlite-contract.tspackages/store/test/sql/sqlite/record.test.ts
|
Implemented the cross-dialect SQL Record utility refactor in
Verification:
|
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 (1)
packages/store/src/sql/sqlite/record-resolver.ts (1)
338-344: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject generated columns in SQLite primary keys.
resolveSqlPrimaryKeychecks onlynotNull, so a generated column entersprimaryKeywithout an issue. Add SQLite-specific validation and regression coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/store/src/sql/sqlite/record-resolver.ts` around lines 338 - 344, Update resolveSqlPrimaryKey and its SQLite call path to reject generated columns used in primary keys, adding an issue when such a column is encountered rather than including it in primaryKey. Add regression coverage for generated primary-key columns while preserving valid non-generated primary keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/store/src/sql/contract-object.ts`:
- Around line 31-40: Update the snapshot copy loop in
packages/store/src/sql/contract-object.ts#L31-L40 to define each copied key as
an own data property rather than calling Reflect.set, preserving an own
__proto__ key without changing the snapshot prototype. Add a regression test in
packages/store/test/sql/contract-object.test.ts#L43-L75 that snapshots an object
with an own __proto__ property and asserts the snapshot retains it as an own
property.
---
Outside diff comments:
In `@packages/store/src/sql/sqlite/record-resolver.ts`:
- Around line 338-344: Update resolveSqlPrimaryKey and its SQLite call path to
reject generated columns used in primary keys, adding an issue when such a
column is encountered rather than including it in primaryKey. Add regression
coverage for generated primary-key columns while preserving valid non-generated
primary keys.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 27ffe577-8625-4a2c-9d5f-acdc34ce8109
📒 Files selected for processing (33)
docs/specs/sql-store.mdpackages/store/src/sql/base64.tspackages/store/src/sql/contract-object.tspackages/store/src/sql/definition-statement.tspackages/store/src/sql/mysql/column-codecs.tspackages/store/src/sql/mysql/column-resolution.tspackages/store/src/sql/mysql/column-type-resolver.tspackages/store/src/sql/mysql/metadata.tspackages/store/src/sql/mysql/mysql-contract.tspackages/store/src/sql/mysql/record-resolver.tspackages/store/src/sql/mysql/record.tspackages/store/src/sql/opaque-format.tspackages/store/src/sql/postgres/column-codecs.tspackages/store/src/sql/postgres/column-type-resolver.tspackages/store/src/sql/postgres/metadata.tspackages/store/src/sql/postgres/record-resolver.tspackages/store/src/sql/postgres/record.tspackages/store/src/sql/record-catalog-resolver.tspackages/store/src/sql/record.tspackages/store/src/sql/sqlite/column-codecs.tspackages/store/src/sql/sqlite/column-resolution.tspackages/store/src/sql/sqlite/column-type-resolver.tspackages/store/src/sql/sqlite/metadata.tspackages/store/src/sql/sqlite/record-resolver.tspackages/store/src/sql/sqlite/record.tspackages/store/src/sql/sqlite/sqlite-contract.tspackages/store/src/sql/statement.tspackages/store/test/sql/base64.test.tspackages/store/test/sql/contract-object.test.tspackages/store/test/sql/definition-statement.test.tspackages/store/test/sql/opaque-format.test.tspackages/store/test/sql/record-utilities.test.tspackages/store/test/sql/sqlite/record.test.ts
💤 Files with no reviewable changes (1)
- packages/store/src/sql/mysql/mysql-contract.ts
|
Addressed the outside-diff SQLite primary-key finding in
Also fixed the inline |
1 participant
Summary
Verification
pnpm --filter @commissary/store exec vitest run test/sql/sqlite/record.test.tspnpm --filter @commissary/store typecheckpnpm run verifypnpm run pack:checkCloses #62
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests