Upgrade @cipherstash/protect-ffi to 0.20.1 and new encryptQuery() API#276
Upgrade @cipherstash/protect-ffi to 0.20.1 and new encryptQuery() API#276
Conversation
Updates FFI dependency to support new encryptQuery/encryptQueryBulk functions.
Adds new encryptQuery() method to ProtectClient supporting both single
value and bulk encryption with explicit or inferred query types.
Key changes:
- Add QueryTypeName and FfiIndexTypeName types with queryTypeToFfi mapping
- Add inferIndexType() helper with priority: unique > match > ore
- Add validateIndexType() for explicit queryType validation
- Create EncryptQueryOperation for single value encryption
- Create BatchEncryptQueryOperation for bulk encryption
- Support LockContext and audit metadata
- Handle edge cases: null, NaN, Infinity values
API usage:
// Single value with explicit type
client.encryptQuery('test@example.com', {
column: users.email, table: users, queryType: 'equality'
})
// Bulk with auto-inference
client.encryptQuery([
{ value: 'test', column: users.email, table: users }
])
Moves SearchTermsOperation to deprecated/ directory and updates it to
use encryptQueryBulk internally. The createSearchTerms() method remains
functional for backward compatibility but is marked deprecated.
Migration path:
// Before (deprecated)
client.createSearchTerms([{ value: 'test', column, table }])
// After
client.encryptQuery([{ value: 'test', column, table, queryType: 'equality' }])
🦋 Changeset detectedLatest commit: 203f6a2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 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 |
- Add NaN/Infinity validation to EncryptQueryOperationWithLockContext (was missing in lock-context variant but present in base class) - Fix isScalarQueryTermArray type guard to not treat empty arrays as batch terms (empty arrays now require element inspection) - Update dispatch logic to maintain backward compatibility: empty arrays without opts continue to be handled as empty batch operations
3a4c05c to
b4b31c5
Compare
Add comprehensive test coverage for encryptQuery/encryptQueryBulk: - Complete auto-inference tests for freeTextSearch and orderAndRange - Validation error scenarios (queryType mismatch, no indexes) - Numeric edge cases (MAX/MIN_SAFE_INTEGER, negative zero, -Infinity) - String edge cases (empty string, unicode/emoji, SQL injection chars) - Bulk index preservation (interspersed nulls, single-item, all-null) - LockContext execution tests with actual .execute() calls - LockContext failure handling and null value handling
…tion Migrate from deprecated createSearchTerms() to encryptQuery() API, passing the appropriate queryType parameter based on operator type: - freeTextSearch for like/ilike/notIlike (generates bf field) - orderAndRange for gt/gte/lt/lte/between/notBetween (generates ob field) - equality for eq/ne/inArray/notInArray (generates hm field) This fixes text search operations which were incorrectly generating the hm (hash match) field instead of the required bf (bloom filter) field, causing eql_v2.like() and eql_v2.ilike() queries to fail.
Add platform doc links to QueryTypeName describing each query type. Mark operation classes as @internal to direct users to ProtectClient.
📝 WalkthroughWalkthroughAdds a new FFI-backed encryptQuery API (single + batch) with per-value queryType metadata threaded from Drizzle operators through encryption, index-type inference/validation, LockContext/audit support, deprecated createSearchTerms path, new helpers/types/operations, and test/example updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App as App
participant PC as ProtectClient
participant OP as Operation (EncryptQuery / Batch)
participant FFI as ProtectFFI
participant LC as LockService
App->>PC: encryptQuery(plaintext | terms)
PC->>OP: construct operation (single or batch)
alt operation needs LockContext
OP->>LC: getLockContext()
LC-->>OP: serviceToken + context
end
OP->>FFI: ffiEncryptQuery / ffiEncryptQueryBulk(payload with indexType, lock/audit)
FFI-->>OP: Encrypted result(s)
OP-->>PC: Result<Encrypted...>
PC-->>App: return encrypted payload(s)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Documents version bumps for the encryptQuery feature: - @cipherstash/protect: minor (new API with deprecation) - @cipherstash/drizzle: patch (internal fix for index selection)
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@packages/protect/src/ffi/operations/batch-encrypt-query.ts`:
- Around line 171-189: The execute() method currently fetches lockContext before
checking for all-null terms; move the call to filterNullTerms(this.terms) to run
before lockContext.getLockContext(), and if nonNullTerms.length === 0 return
this.terms.map(() => null) immediately to avoid calling
lockContext.getLockContext(); specifically update the logic in the execute()
function so filterNullTerms runs first, then only call
lockContext.getLockContext() (and use ctsToken/context) when nonNullTerms.length
> 0.
In `@packages/protect/src/ffi/operations/deprecated/search-terms.ts`:
- Line 28: The debug log in the deprecated search-terms code is exposing
plaintext PII by logging this.terms; modify the logger.debug call in
search-terms.ts (the line issuing "Creating search terms (deprecated API)") to
avoid printing raw terms and instead log only safe metadata such as the number
of terms (this.terms.length) or a non-reversible summary (e.g., counts or
boolean flags), and remove or redact any direct contents of this.terms from the
log payload.
- Around line 84-100: The LockContext path is missing the same
client-initialization guard used elsewhere, so add a check before using
op.client in the block that builds QueryPayloads and calls encryptQueryBulk; if
op.client is not initialized call the existing noClientError() (or throw its
result) instead of proceeding. Locate the code around withResult /
this.getAuditData and the queries/encryptedTerms logic (functions/symbols:
withResult, getAuditData, QueryPayload, SearchTerm, inferIndexType,
encryptQueryBulk, op.client, ctsToken, context) and insert the client guard
early in this path to return the same actionable error when the client is
missing.
🧹 Nitpick comments (5)
packages/drizzle/src/pg/operators.ts (1)
677-682: Remove unusedsqlFnMapobject.The
sqlFnMapobject is defined but never used. The actual SQL generation at line 696 usessql.raw(operator)instead.🧹 Proposed fix to remove dead code
- // Create SQL using eql_v2 functions for encrypted columns - const sqlFnMap = { - gt: () => sql`eql_v2.gt(${left}, ${bindIfParam(right, left)})`, - gte: () => sql`eql_v2.gte(${left}, ${bindIfParam(right, left)})`, - lt: () => sql`eql_v2.lt(${left}, ${bindIfParam(right, left)})`, - lte: () => sql`eql_v2.lte(${left}, ${bindIfParam(right, left)})`, - } - // This will be replaced with encrypted value in executeLazyOperatorpackages/protect/src/types.ts (1)
160-177: Consider extracting shared properties to reduce duplication.
EncryptQueryOptionsandScalarQueryTermsharecolumn,table, andqueryTypeproperties. You could extract a base type to reduce duplication, though this is optional given the small surface area.♻️ Optional refactor to reduce duplication
+/** + * Base options for query encryption operations. + */ +type QueryTermBase = { + column: ProtectColumn + table: ProtectTable<ProtectTableColumn> + queryType?: QueryTypeName +} + /** * Options for encrypting a single query term. */ -export type EncryptQueryOptions = { - column: ProtectColumn - table: ProtectTable<ProtectTableColumn> - queryType?: QueryTypeName // Optional - auto-infers if omitted -} +export type EncryptQueryOptions = QueryTermBase /** * Individual query term for bulk operations. */ -export type ScalarQueryTerm = { - value: JsPlaintext | null - column: ProtectColumn - table: ProtectTable<ProtectTableColumn> - queryType?: QueryTypeName // Optional - auto-infers if omitted -} +export type ScalarQueryTerm = QueryTermBase & { + value: JsPlaintext | null +}packages/protect/src/ffi/operations/encrypt-query.ts (1)
97-168: Consider extracting shared validation logic to reduce duplication.The
EncryptQueryOperationWithLockContextclass duplicates the null/NaN/Infinity validation (lines 110-130) and indexType inference logic (lines 145-151) fromEncryptQueryOperation. Consider extracting these into shared helper methods.♻️ Example helper extraction
// In a shared location or base class: function validatePlaintext(plaintext: JsPlaintext | null): Result<void, ProtectError> | null { if (plaintext === null) { return { data: null } as unknown as Result<void, ProtectError> } if (typeof plaintext === 'number' && Number.isNaN(plaintext)) { return { failure: { type: ProtectErrorTypes.EncryptionError, message: '[protect]: Cannot encrypt NaN value', }, } } if (typeof plaintext === 'number' && !Number.isFinite(plaintext)) { return { failure: { type: ProtectErrorTypes.EncryptionError, message: '[protect]: Cannot encrypt Infinity value', }, } } return null // Continue with encryption }packages/protect/__tests__/infer-index-type.test.ts (1)
5-13: Nested describe block has confusing naming.There's an outer
describe('inferIndexType')that contains bothdescribe('inferIndexType')anddescribe('validateIndexType'). The outer describe should have a more general name since it encompasses both functions.♻️ Suggested fix
-describe('inferIndexType', () => { +describe('infer-index-type helpers', () => { const users = csTable('users', { email: csColumn('email').equality(), bio: csColumn('bio').freeTextSearch(), age: csColumn('age').orderAndRange(), name: csColumn('name').equality().freeTextSearch(), }) describe('inferIndexType', () => {packages/protect/src/ffi/helpers/type-guards.ts (1)
7-17: Type guard could be stricter to avoid false positives.The current check only verifies
'column' in value[0], which could match objects that aren't actuallyScalarQueryTerm(e.g., any object with acolumnproperty). Consider adding a check for thetableproperty as well for more robust discrimination.♻️ Proposed stricter type guard
export function isScalarQueryTermArray( value: unknown ): value is readonly ScalarQueryTerm[] { return ( Array.isArray(value) && value.length > 0 && typeof value[0] === 'object' && value[0] !== null && - 'column' in value[0] + 'column' in value[0] && + 'table' in value[0] ) }
- Skip lockContext fetch when all query terms are null (optimization) - Remove PII from deprecated search-terms debug logs (security) - Add client initialization guard in SearchTermsOperationWithLockContext - Stricter type guard for ScalarQueryTerm detection - Remove unused sqlFnMap dead code in drizzle operators - Fix test describe block naming for infer-index-type helpers
…ptQuery - Create validation.ts with validateNumericValue() and assertValidNumericValue() to eliminate NaN/Infinity validation duplication across operations - Add resolveIndexType() to infer-index-type.ts combining index inference and validation logic - Create test fixtures module with shared schemas, mock factories (createMockLockContext, createFailingMockLockContext), and test helpers (unwrapResult, expectFailure) - Consolidate EncryptQueryOptions and ScalarQueryTerm via QueryTermBase type - Refactor encrypt-query.test.ts to use fixtures, reducing ~150 lines Addresses PR #276 feedback on code reuse.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.changeset/encrypt-query-api.md:
- Line 9: Replace the internal FFI names in the changeset text with the public
API names exported as QueryTypeName: change `match` → `equality`, `ore` →
`orderAndRange`, and `unique` → the appropriate public name (e.g., `...` if
different), and use `freeTextSearch` for full-text cases instead of any FFI
label; also check whether `ste_vec` exists in the public API (inspect
packages/protect/src/types.ts and the exported QueryTypeName) and if it is not a
planned public feature remove it from the list, or if it is planned rename it to
the documented public name.
🧹 Nitpick comments (2)
packages/drizzle/src/pg/operators.ts (1)
1438-1593: Consider extracting shared batching logic.The
queryTypepropagation in bothprotectAndandprotectOris correct. However, these two functions share ~90% identical code for collecting lazy operators, batching encryption, and mapping results. The only differences are the final combinator (andvsor) and default return values.♻️ Suggested approach to reduce duplication
Extract a shared helper that accepts the combinator function as a parameter:
async function batchEncryptAndCombine( conditions: (SQL | SQLWrapper | Promise<SQL> | undefined)[], combinator: typeof and | typeof or, defaultValue: SQL, protectClient: ProtectClient, defaultProtectTable: ProtectTable<ProtectTableColumn> | undefined, protectTableCache: Map<string, ProtectTable<ProtectTableColumn>>, ): Promise<SQL> { // ... shared batching logic ... return combinator(...allConditions) ?? defaultValue } const protectAnd = async (...conditions) => batchEncryptAndCombine(conditions, and, sql`true`, ...) const protectOr = async (...conditions) => batchEncryptAndCombine(conditions, or, sql`false`, ...)Also applies to: 1598-1744
.changeset/encrypt-query-api.md (1)
6-11: Enhance changeset description with missing features and clearer wording.The description could be improved in two ways:
Wording clarity: Line 8 says the new method "replaces" the old one, but line 10 correctly states it "deprecates" it. Consider using consistent terminology - deprecation means the old API remains for backward compatibility, not a full replacement.
Missing features: The PR objectives mention several notable features not documented here:
- Auto-inference of query type from column configuration when omitted
- Support for both single and bulk operations
- Upgrade of
@cipherstash/protect-ffidependency to 0.20.1Including these details would give users a more complete picture of the changes.
📝 Suggested enhancement to the description
Add `encryptQuery` API for encrypting query terms with explicit query type selection. -- New `encryptQuery()` method replaces `createSearchTerms()` with improved query type handling -- Supports `match`, `ste_vec`, `ore`, and `unique` query types +- New `encryptQuery()` method with improved query type handling (supports single and bulk operations) +- Automatically infers query type from column configuration when not specified +- Upgrades `@cipherstash/protect-ffi` to v0.20.1 - Deprecates `createSearchTerms()` - use `encryptQuery()` instead - Updates drizzle operators to use correct index selection via `queryType` parameterNote: The query type names in line 9 should be updated once verified per the previous comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/protect/src/ffi/operations/encrypt-query.ts`:
- Around line 42-45: The execute() method in encrypt-query.ts currently returns
a raw ProtectError from validateNumericValue, causing a Result<undefined,
ProtectError> instead of the expected Result<Encrypted, ProtectError>; update
both validation branches (around validateNumericValue usage at the first check
and the similar block at lines ~95–98) to wrap the validation failure in the
Result failure type used across this module (e.g., return the module's
Result.fail/err<Encrypted, ProtectError>(validationError) or equivalent) so the
returned type is Result<Encrypted, ProtectError>.
🧹 Nitpick comments (1)
packages/protect/__tests__/fixtures/index.ts (1)
78-83: Consider handling the edge case where neitherdatanorfailureis present.If a malformed result (e.g.,
{}) is passed, the function silently returnsundefinedcast asT, which could cause confusing downstream test failures.🛡️ Optional: Add a guard for malformed results
export function unwrapResult<T>(result: { data?: T; failure?: { message: string } }): T { if (result.failure) { throw new Error(result.failure.message) } + if (result.data === undefined) { + throw new Error('Result has neither data nor failure') + } return result.data as T }
92e68cf to
afb448a
Compare
- Export LockContext class as value export, not just as type, allowing users to instantiate the class with `new LockContext()` - Fix typo: ctsEndoint → ctsEndpoint in identify module
Migrate all usage of deprecated createSearchTerms to the new encryptQuery API while preserving backward compatibility tests. Changes: - Update number-protect tests to use encryptQuery with queryType - Update supabase test to use encryptQuery + encryptedToPgComposite - Add number returnType tests to deprecated/search-terms.test.ts - Migrate DynamoDB examples to use protectClient.encryptQuery - Migrate TypeORM helper to use encryptQuery + encryptedToPgComposite - Add @deprecated JSDoc to protect-dynamodb createSearchTerms API
…ries
The encryptedToPgComposite helper produces { data: encrypted } which works
for inserts but not for .eq() queries where Supabase serializes it as
[object Object].
Add encryptedToCompositeLiteral helper that produces the PostgreSQL
composite literal string format ("json_string") required for equality
queries.
Add optional returnType field to ScalarQueryTerm enabling direct PostgreSQL composite literal output during encryption, eliminating the need for separate helper functions. - Add returnType to QueryTermBase type - Add EncryptedQueryResult union type - Implement formatting in assembleResults based on returnType - Update Supabase test to use returnType instead of helper - Deprecate encryptedToCompositeLiteral helper function - Add comprehensive tests for returnType functionality
- Add JSDoc documentation to returnType field in QueryTermBase - Extract encryptedToEscapedCompositeLiteral helper to eliminate duplication - Remove unused nullIndices parameter from assembleResults()
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/protect/src/helpers/index.ts`:
- Around line 39-57: The composite-literal helpers currently accept Encrypted
which can be null and end up producing a `"null"` literal; change both
encryptedToCompositeLiteral and encryptedToEscapedCompositeLiteral to explicitly
guard against null (throw a clear error when obj === null) and tighten their
parameter types to exclude null, then have encryptedToEscapedCompositeLiteral
reuse encryptedToCompositeLiteral (i.e., call encryptedToCompositeLiteral(obj)
and JSON.stringify the returned string) so escaping is consistent.
Update encryptedToCompositeLiteral and encryptedToEscapedCompositeLiteral to reject null values with clear errors and use the non-nullable CipherStashEncrypted type. Have encryptedToEscapedCompositeLiteral reuse encryptedToCompositeLiteral for DRY implementation.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/protect/src/helpers/index.ts`:
- Around line 46-63: Mark the function encryptedToEscapedCompositeLiteral as
deprecated by adding a JSDoc `@deprecated` tag and brief guidance mirroring
encryptedToCompositeLiteral's deprecation: instruct callers to use
encryptQuery() with returnType: 'escaped-composite-literal' instead; keep a
reference to encryptedToCompositeLiteral for consistency in the comment so
readers can see parallel deprecation guidance.
🧹 Nitpick comments (1)
packages/protect/src/ffi/operations/batch-encrypt-query.ts (1)
22-40:nullIndicesis computed but never used.The
filterNullTermsfunction returnsnullIndices, but neitherBatchEncryptQueryOperation.execute()norBatchEncryptQueryOperationWithLockContext.execute()uses it. TheassembleResultsfunction initializes the array withnulland overwrites only the non-null positions, makingnullIndicesredundant.♻️ Simplified return type
function filterNullTerms( terms: readonly ScalarQueryTerm[], ): { - nullIndices: Set<number> nonNullTerms: { term: ScalarQueryTerm; originalIndex: number }[] } { - const nullIndices = new Set<number>() const nonNullTerms: { term: ScalarQueryTerm; originalIndex: number }[] = [] terms.forEach((term, index) => { - if (term.value === null) { - nullIndices.add(index) - } else { + if (term.value !== null) { nonNullTerms.push({ term, originalIndex: index }) } }) - return { nullIndices, nonNullTerms } + return { nonNullTerms } }Then update the destructuring at call sites:
- const { nullIndices, nonNullTerms } = filterNullTerms(this.terms) + const { nonNullTerms } = filterNullTerms(this.terms)
Add @deprecated JSDoc tag directing users to use encryptQuery() with returnType: 'escaped-composite-literal' instead. Includes before/after code examples showing the migration path.
Replace flexible `expect.any(Number)` assertions with explicit `v: 2` to verify the payload contains the correct EQL version.
coderdan
left a comment
There was a problem hiding this comment.
A couple of minor suggestions but nothing blocking. LGTM!
CDThomas
left a comment
There was a problem hiding this comment.
Left a few comments but nothing blocking. I appreciated all of the examples of migrating to the new APIs.
I think it'd be worth getting a look from @calvinbrewer , too though.
…ptQuery Add validation that errors when encrypting numeric values with a 'match' index (freeTextSearch). Numbers cannot be meaningfully tokenized for bloom filter indexing, so this prevents semantically incorrect behavior. - Add validateValueIndexCompatibility and assertValueIndexCompatibility functions to validation.ts - Integrate validation into EncryptQueryOperation (single and with lock context) - Integrate validation into BatchEncryptQueryOperation - Add comprehensive tests for value/index type compatibility
Add test that verifies encryptQuery handles explicit null context from getLockContext gracefully. This tests the defensive null check that prevents null from being passed to the FFI layer. - Add createMockLockContextWithNullContext helper to fixtures - Add test case in LockContext support describe block
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/protect/__tests__/encrypt-query.test.ts`:
- Line 2: The test file uses vi.fn() mocks but does not import vi, causing a
ReferenceError; add an import for vi from Vitest (e.g., add "import { describe,
expect, it, beforeAll, vi } from 'vitest'") at the top of
packages/protect/__tests__/encrypt-query.test.ts so all occurrences of vi.fn()
(around the mocks used between lines ~667-679) resolve correctly.
🧹 Nitpick comments (1)
packages/protect/src/ffi/operations/batch-encrypt-query.ts (1)
126-126: Remove unusednullIndicesvariable.The
nullIndicesset is destructured in bothexecute()methods but never used. Consider removing it to avoid dead code.♻️ Proposed fix
- const { nullIndices, nonNullTerms } = filterNullTerms(this.terms) + const { nonNullTerms } = filterNullTerms(this.terms)Apply to both line 126 and line 177.
Also applies to: 177-177
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/protect/__tests__/encrypt-query.test.ts`:
- Around line 1-15: The test imports the internal type ProtectClient from
../src/ffi; change that import to use the public API's exported type instead by
importing ProtectClient from the package's main export (the same module used for
protect and ProtectErrorTypes) so the test references import type {
ProtectClient } from the public API; update the import statement accordingly to
remove the internal module dependency.
🧹 Nitpick comments (1)
packages/protect/__tests__/fixtures/index.ts (1)
95-100: Consider stricter type handling inunwrapResult.The function returns
result.data as Twithout checking ifdatais defined (only checks forfailure). While the API contract likely ensures one ofdata/failureis always set, a stricter implementation could improve clarity.🔧 Optional: Add explicit undefined check
export function unwrapResult<T>(result: { data?: T; failure?: { message: string } }): T { if (result.failure) { throw new Error(result.failure.message) } + if (result.data === undefined) { + throw new Error('Result has neither data nor failure') + } - return result.data as T + return result.data }
…turn Extract failure property when returning validation error to satisfy Result<Encrypted, ProtectError> return type. The validateNumericValue function returns Result<never, ProtectError> | undefined, which was incompatible when returned directly. Applied to both EncryptQueryOperation and EncryptQueryOperationWithLockContext.
Summary
Migrates
@cipherstash/protectfrom FFI v0.19.0 to v0.20.1, replacing the internal use ofencryptBulk()for search terms with the new dedicatedencryptQuery()/encryptQueryBulk()functions.@cipherstash/protect-ffito 0.20.1encryptQuery()method supporting single and bulk operationsequality,freeTextSearch,orderAndRangecreateSearchTerms()(remains functional for backward compatibility)Review
createSearchTermscapabilitiescreateSearchTermsis deprecated across codebaseCode Review Agents
Feedback from original prototype PR has been incorporated to (hopefully) avoid regression: #257
Summary by CodeRabbit
New Features
Tests
Documentation
Chores