fix(stack-drizzle): preserve per-column types in extractEncryptionSchema - #845
Conversation
`extractEncryptionSchema` returned the widened `AnyV3Table`, discarding
every column's concrete EQL v3 domain. `InferPlaintext` over an extracted
schema therefore collapsed to an index signature, so along the documented
flow — `extractEncryptionSchema(table)` -> `Encryption({ schemas })` ->
`bulkEncryptModels` — a `string` written to an `IntegerOrd` column
type-checked, and plain helper columns (`id`, a plain `text()`) were typed
as encrypted. Type-safety/DX only: the runtime already recovered each
builder correctly via `getEqlV3Column`.
Three linked pieces, all in `@cipherstash/stack-drizzle` — the v3 core
needed no change:
- `makeEqlV3Column` carries its builder `C` on a phantom brand. The brand
rides on the column's `_` config, not the builder object: `pgTable()`
rebuilds each builder through drizzle's `BuildColumn`, which keeps only
`Omit<TBuilder['_'], keyof MakeColumnConfig<…>>` — drizzle's own
extension point (`NotNull<T> = T & { _: { notNull: true } }`). A brand on
the object would be dropped and extraction would have nothing to read.
- `extractEncryptionSchema` is generic over the table and returns
`EncryptedTable<Cols> & Cols`, the shape `encryptedTable()` itself
returns. `EncryptedColumnsOf<T>` recovers each brand and keeps only the
branded columns — the type-level mirror of the existing runtime loop,
which is unchanged.
- `V3BuilderOf` short-circuits `any` to `AnyEncryptedV3Column`, so an
untyped table keeps the widened-but-usable typing it had before rather
than branding every column `any`.
`__tests__/schema-extraction.test-d.ts` locks both paths: extracted and
hand-authored must infer the same plaintext map, reject a field typed
against the wrong domain, and pin `encrypt` to the addressed column. Six of
its eight assertions fail without this fix.
Two integration files relied on the old looseness and are corrected, not
worked around: the dynamically-built matrix table names its model type
explicitly (its rows are a pure index signature, so inference dropped the
`| null` that `nullableTextEq` needs), and the bench comment claiming
extraction returns `AnyV3Table` is now false and says so.
Closes #589
🦋 Changeset detectedLatest commit: 9a4af72 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthrough
ChangesTyped schema extraction
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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: 2
🧹 Nitpick comments (1)
packages/stack-drizzle/src/column.ts (1)
254-261: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid the unsuppressed type-erasing cast.
column as unknown as EqlV3ColumnCarrierbypasses source typing without a scoped suppression. Replace it with a typed runtime-carrier helper, or add a narrow Biome suppression documenting why this attachment is safe. As per coding guidelines, “Avoid type-erasing assertions such asas any,as never, andas unknownin source unless deliberately suppressed with a reason.”🤖 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/stack-drizzle/src/column.ts` around lines 254 - 261, Replace the unsuppressed unknown-based cast in the column construction flow with a typed runtime-carrier helper, or add a narrowly scoped Biome suppression with a reason documenting why the attachment is safe. Keep the existing writeBuilder calls and getEqlV3Column runtime behavior unchanged.Source: Coding guidelines
🤖 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 @.changeset/typed-extract-encryption-schema.md:
- Line 18: Update the changeset release note to include the required follow-up
for the separately maintained documentation site, alongside the existing
skills/stash-drizzle documentation entry. Keep the note focused on the public
typing change and explicitly identify the documentation-site update as required.
In `@packages/stack-drizzle/src/schema-extraction.ts`:
- Around line 49-51: Update ExtractedEncryptionSchema and its schema-extraction
typing so mixed tables with branded and SQL-type-recovered columns retain the
recovered columns’ encrypted runtime behavior instead of selecting the narrow
precise branch. Widen the mixed-schema result or represent recovered columns as
widened encrypted columns, while preserving precise typing for fully branded
tables and AnyV3Table for tables with no encrypted columns; add a type test
covering one branded and one ordinary EQL customType column.
---
Nitpick comments:
In `@packages/stack-drizzle/src/column.ts`:
- Around line 254-261: Replace the unsuppressed unknown-based cast in the column
construction flow with a typed runtime-carrier helper, or add a narrowly scoped
Biome suppression with a reason documenting why the attachment is safe. Keep the
existing writeBuilder calls and getEqlV3Column runtime behavior unchanged.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ead2364e-fc06-4a7a-9e66-086e8d69b5c0
📒 Files selected for processing (7)
.changeset/typed-extract-encryption-schema.mdpackages/bench/src/drizzle/setup.tspackages/stack-drizzle/__tests__/schema-extraction.test-d.tspackages/stack-drizzle/integration/relational.integration.test.tspackages/stack-drizzle/src/column.tspackages/stack-drizzle/src/schema-extraction.tsskills/stash-drizzle/SKILL.md
|
|
||
| **Runtime behaviour is unchanged** — the runtime already recovered each column's builder correctly, so this is a type-level fix only. It is `minor` rather than `patch` because code that previously compiled against the widened types can now fail to compile: a model field typed against the wrong domain, or a schema-derived type that relied on the old index signature. Rows whose shape is only known at runtime (a dynamically built table) should name their model type explicitly — `client.bulkEncryptModels<typeof schema, MyRow>(rows, schema)` — rather than being cast back to `AnyV3Table`. | ||
|
|
||
| `skills/stash-drizzle` documents the preserved typing and warns against casting an extracted schema to `AnyV3Table` to make an insert compile. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Record the documentation-site follow-up.
This is a public type-surface change, but the release note only mentions the in-repository skill. Add the required note for the separately maintained documentation site. As per coding guidelines, “When changing public APIs, preserve ESM/CJS compatibility, update in-repository examples, add or extend tests, and note required updates to the separately maintained documentation site.”
🤖 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 @.changeset/typed-extract-encryption-schema.md at line 18, Update the
changeset release note to include the required follow-up for the separately
maintained documentation site, alongside the existing skills/stash-drizzle
documentation entry. Keep the note focused on the public typing change and
explicitly identify the documentation-site update as required.
Source: Coding guidelines
Two gaps in the #589 type-level suite, both of which would have shipped green. Drizzle column modifiers. Every existing fixture declares bare columns, but real schemas almost always use `.notNull()`. The brand survives `pgTable()` only because drizzle's `BuildColumn` keeps `Omit<TBuilder['_'], keyof MakeColumnConfig<…> | 'brand' | 'dialect'>`, and modifiers then intersect further into `_` (`NotNull<T> = T & { _: { notNull: true } }`). That is an upstream compatibility contract, not one we control: a drizzle-orm change to either step could drop the brand for modified columns while bare ones kept working. The runtime would not notice — `getEqlV3Column` recovers builders from the runtime carrier and the SQL domain, never from the type — so extraction would silently type a `.notNull()` column as a plain pass-through field while still encrypting it. Verified the guard isolates that class: narrowing `V3BuilderOf` to match only `notNull: false` fails exactly the three new modifier assertions and leaves all fourteen pre-existing ones green. The json domain. `types.Json` is the only domain whose plaintext is a union (`JsonDocument`) rather than a scalar, which makes it the easiest to lose to a widened index signature without another assertion noticing. Covered on both the extracted and hand-authored paths, including a `@ts-expect-error` on a scalar written to a json column. Both blocks fail against the pre-fix source, so they cover the fix rather than passing incidentally. Test-only; the existing changeset already covers the behaviour.
There was a problem hiding this comment.
Pull request overview
This PR fixes a long-standing Drizzle adapter type-safety gap by making extractEncryptionSchema() preserve each encrypted column’s concrete EQL v3 domain when the table’s branded column types are available, while retaining the existing safe AnyV3Table fallback for widened PgTable inputs and SQL-domain-detected customType columns. This improves InferPlaintext, column addressing (schema.email), and model encryption (encryptModel/bulkEncryptModels) type precision without changing runtime extraction behavior.
Changes:
- Add a private, type-level phantom brand to Drizzle carrier columns (
makeEqlV3Column) and a mapped-type extractor to recover the concrete v3 builders from apgTable()definition. - Make
extractEncryptionSchemageneric and conditionally precise, widening back toAnyV3Tablewhen brands are missing or incomplete (e.g., widenedPgTable, SQL-domain fallback columns, mixed branded + SQL-derived). - Add compile-time
.test-d.tscoverage for precise inference and fallback behavior; update the shippedstash-drizzleskill and the changeset accordingly.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
packages/stack-drizzle/src/column.ts |
Introduces a type-only builder brand (EqlV3Column<C>) and V3BuilderOf recovery to carry concrete v3 domains through Drizzle’s pgTable() typing. |
packages/stack-drizzle/src/schema-extraction.ts |
Makes extractEncryptionSchema generic and conditionally precise via EncryptedColumnsOf<T> / fallback logic, while keeping runtime extraction unchanged. |
packages/stack-drizzle/__tests__/schema-extraction.test-d.ts |
Adds type-level regression tests proving precise inference matches hand-authored schemas and that widened/SQL-derived/mixed cases fall back safely. |
packages/stack-drizzle/integration/relational.integration.test.ts |
Adjusts a runtime-shaped table encryption call to provide an explicit model type parameter where inference is intentionally impossible. |
skills/stash-drizzle/SKILL.md |
Documents the new “precise when branded, widened when not” boundary and warns against casting away the type safety. |
.changeset/typed-extract-encryption-schema.md |
Ships release notes for the typing change (@cipherstash/stack-drizzle minor) and the skill update (stash patch). |
packages/bench/src/drizzle/setup.ts |
Updates bench commentary to reflect the new precise InferPlaintext behavior while explaining why the bench row type remains hand-written. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Closes #589.
Summary
extractEncryptionSchemanow preserves each branded Drizzle column concrete EQL v3 domain instead of widening every extracted schema. This keepsInferPlaintext, model operations, and addressed-column encryption precise through the documented Drizzle flow.The return type also preserves the safe pre-existing widened behavior when no private brands can be recovered. Tables annotated as
PgTableand ordinarycustomTypecolumns detected through their EQL SQL domain now returnAnyV3Table, rather than the incorrect empty schema type, while runtime extraction continues to discover their encrypted columns.The implementation remains isolated to
@cipherstash/stack-drizzle; no v3 core or runtime behavior changes are required.Tests
PgTableinputs and SQL-type fallback columnsEncrypted, not unchanged plaintextcode:checkpassedRelease notes
The existing changeset updates
@cipherstash/stack-drizzleand the shippedstash-drizzleskill documents both precise branded inference and the widened fallback boundary.Summary by CodeRabbit
Bug Fixes
extractEncryptionSchematyping to retain each encrypted column’s concrete Postgres domain (instead of falling back to a widened schema).Documentation
Tests