Skip to content

Upgrade @cipherstash/protect-ffi to 0.20.1 and new encryptQuery() API#276

Merged
tobyhede merged 24 commits intomainfrom
encrypt-query-api
Feb 4, 2026
Merged

Upgrade @cipherstash/protect-ffi to 0.20.1 and new encryptQuery() API#276
tobyhede merged 24 commits intomainfrom
encrypt-query-api

Conversation

@tobyhede
Copy link
Contributor

@tobyhede tobyhede commented Feb 2, 2026

Summary

Migrates @cipherstash/protect from FFI v0.19.0 to v0.20.1, replacing the internal use of encryptBulk() for search terms with the new dedicated encryptQuery()/encryptQueryBulk() functions.

  • Upgrade @cipherstash/protect-ffi to 0.20.1
  • Add new encryptQuery() method supporting single and bulk operations
  • Add user-friendly query types: equality, freeTextSearch, orderAndRange
  • Auto-infer query type from column configuration when omitted
  • Deprecate createSearchTerms() (remains functional for backward compatibility)

Review

  • API usage is correct
  • integration is idiomatic typescript
  • parity with current createSearchTerms capabilities
  • use of createSearchTerms is deprecated across codebase

Code Review Agents

  • Claude
  • Codex
  • Gemini
  • CodeRabbit

Feedback from original prototype PR has been incorporated to (hopefully) avoid regression: #257

Summary by CodeRabbit

  • New Features

    • Adds encryptQuery (single + batch) with per-term queryType, index-type inference/validation, lock-context & audit support; preserves input shape (including nulls). New helpers to format encrypted results (composite/escaped literals) and new query/index type constants.
  • Tests

    • Extensive suites covering encryptQuery, index inference/validation, numeric/string edge cases, batch behavior, audit and lock-context flows, and backward compatibility.
  • Documentation

    • Deprecates createSearchTerms and updates examples/docs to use encryptQuery.
  • Chores

    • Bumped @cipherstash/protect-ffi to 0.20.1

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-bot
Copy link

changeset-bot bot commented Feb 2, 2026

🦋 Changeset detected

Latest commit: 203f6a2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages
Name Type
@cipherstash/protect Minor
@cipherstash/drizzle Major
@cipherstash/protect-dynamodb Major
@cipherstash/basic-example Patch
@cipherstash/dynamo-example Patch
nest Patch
next-drizzle-mysql Patch
@cipherstash/nextjs-clerk-example Patch
@cipherstash/typeorm-example Patch

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
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.
@tobyhede tobyhede changed the title Encrypt query api Upgrade @cipherstash/protect-ffi to 0.20.1 and new encryptQuery() API Feb 3, 2026
Add platform doc links to QueryTypeName describing each query type.
Mark operation classes as @internal to direct users to ProtectClient.
@coderabbitai
Copy link

coderabbitai bot commented Feb 3, 2026

📝 Walkthrough

Walkthrough

Adds 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

Cohort / File(s) Summary
Drizzle operators
packages/drizzle/src/pg/operators.ts
Thread optional queryType through value shapes, ValueToEncrypt, LazyOperator, operator creation paths, and encryption-term construction.
Protect types & constants
packages/protect/src/types.ts
Add QueryTypeName, FfiIndexTypeName, queryTypes, queryTypeToFfi, EncryptedQueryResult, and query-term option types for encryptQuery.
FFI helpers
packages/protect/src/ffi/helpers/infer-index-type.ts, packages/protect/src/ffi/helpers/type-guards.ts, packages/protect/src/ffi/helpers/validation.ts
Add infer/validate/resolve index-type utilities, isScalarQueryTermArray type guard, and value/index compatibility validation/assertion helpers.
FFI operations — single
packages/protect/src/ffi/operations/encrypt-query.ts
Add EncryptQueryOperation (+ lock-context variant): validate plaintext, resolve index type (from queryType or inference), call FFI, include lock/audit handling, map errors.
FFI operations — batch
packages/protect/src/ffi/operations/batch-encrypt-query.ts
Add BatchEncryptQueryOperation (+ lock-context variant): partition nulls, per-term index resolution/validation, bulk FFI call, reassemble results preserving input positions and return formatting.
FFI operations — deprecated path
packages/protect/src/ffi/operations/deprecated/search-terms.ts
Provide deprecated SearchTermsOperation (+ lock variant) implementing legacy createSearchTerms behavior via the new bulk FFI path.
Protect FFI surface
packages/protect/src/ffi/index.ts
Add overloads/unified implementation for ProtectClient.encryptQuery supporting single plaintext+opts and batch ScalarQueryTerm[] (uses type guard and preserves backward-compat empty-array behavior).
Public exports & helpers
packages/protect/src/index.ts, packages/protect/src/helpers/index.ts
Export new operations and infer/validate helpers; add encryptedToCompositeLiteral and encryptedToEscapedCompositeLiteral helpers and docs.
Helpers tests & export
packages/protect/__tests__/helpers.test.ts, packages/protect/src/helpers/index.ts
Expose and test composite-literal helper conversions.
Tests, fixtures & utilities
packages/protect/__tests__/*, packages/protect/__tests__/fixtures/index.ts
Add extensive encryptQuery/infer-index-type tests, fixtures, LockContext mocks, and test helpers (unwrapResult, expectFailure).
Examples
examples/*, examples/dynamo/*, examples/typeorm/*
Replace createSearchTerms usages with encryptQuery, include queryType, update error messages and result extraction.
Protect-Dynamo deprecation docs
packages/protect-dynamodb/src/*
Add deprecation JSDoc recommending protectClient.encryptQuery(terms) and extracting .hm for DynamoDB keys.
Dependency bump
packages/protect/package.json
Bump @cipherstash/protect-ffi from 0.19.0 to 0.20.1.
Misc fix
packages/protect/src/identify/index.ts
Fix local variable name ctsEndointctsEndpoint.
Changeset / docs
.changeset/encrypt-query-api.md
Document new encryptQuery API, deprecation of createSearchTerms, and mapping of queryType → FFI index type.

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)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

Poem

🐇 I nibble queryType on every byte and leaf,
Batch or single, nulls kept free from grief,
Locks and audits snug and bright,
Indices chosen just right,
Encrypted carrots make the garden brief! 🥕🔐

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately summarizes the main changes: upgrading @cipherstash/protect-ffi to 0.20.1 and introducing the new encryptQuery() API. It is concise, clear, and immediately conveys the primary purpose of the changeset to someone reviewing the history.
Docstring Coverage ✅ Passed Docstring coverage is 96.30% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ 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 encrypt-query-api

Comment @coderabbitai help to get the list of available commands and usage tips.

Documents version bumps for the encryptQuery feature:
- @cipherstash/protect: minor (new API with deprecation)
- @cipherstash/drizzle: patch (internal fix for index selection)
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 unused sqlFnMap object.

The sqlFnMap object is defined but never used. The actual SQL generation at line 696 uses sql.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 executeLazyOperator
packages/protect/src/types.ts (1)

160-177: Consider extracting shared properties to reduce duplication.

EncryptQueryOptions and ScalarQueryTerm share column, table, and queryType properties. 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 EncryptQueryOperationWithLockContext class duplicates the null/NaN/Infinity validation (lines 110-130) and indexType inference logic (lines 145-151) from EncryptQueryOperation. 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 both describe('inferIndexType') and describe('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 actually ScalarQueryTerm (e.g., any object with a column property). Consider adding a check for the table property 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.
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 queryType propagation in both protectAnd and protectOr is 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 (and vs or) 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:

  1. 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.

  2. 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-ffi dependency to 0.20.1

Including 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` parameter

Note: The query type names in line 9 should be updated once verified per the previous comment.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 neither data nor failure is present.

If a malformed result (e.g., {}) is passed, the function silently returns undefined cast as T, 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
 }

- 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()
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: nullIndices is computed but never used.

The filterNullTerms function returns nullIndices, but neither BatchEncryptQueryOperation.execute() nor BatchEncryptQueryOperationWithLockContext.execute() uses it. The assembleResults function initializes the array with null and overwrites only the non-null positions, making nullIndices redundant.

♻️ 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.
Copy link
Contributor

@coderdan coderdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of minor suggestions but nothing blocking. LGTM!

Copy link
Contributor

@CDThomas CDThomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 unused nullIndices variable.

The nullIndices set is destructured in both execute() 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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 in unwrapResult.

The function returns result.data as T without checking if data is defined (only checks for failure). While the API contract likely ensures one of data/failure is 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants