Skip to content

fix: align Firestore repository semantics - #37

Open
leoafarias wants to merge 3 commits into
agent/rockets-auth-hardeningfrom
agent/rockets-firestore-parity
Open

fix: align Firestore repository semantics#37
leoafarias wants to merge 3 commits into
agent/rockets-auth-hardeningfrom
agent/rockets-firestore-parity

Conversation

@leoafarias

@leoafarias leoafarias commented Aug 8, 2026

Copy link
Copy Markdown
Member

Summary

Make the Firestore adapter's two execution paths agree with each other and with Firestore's real semantics.

The adapter runs queries through the Admin SDK backend or the in-memory backend, plus a local post-filter stage for any predicate Firestore can't evaluate server-side. Each of those three places had grown its own private copy of "how do I read a field", "how do I compare two values", and "how do I sort" — and they disagreed with each other and with Firestore.

Two of these produced silently wrong result sets, not rough edges:

  • A document-ID predicate discarded every other filter on the same branch. Both backends loaded rows by ID and then applied only postFilters; the pushdown filters were never evaluated. A query like id = X AND ownerId = Y returned the document even when ownerId did not match — so adding an ID to a query quietly widened it instead of narrowing it.
  • Composing document IDs inside an AND was broken in both directions. A second documentId silently overwrote the first (last-write-wins), while any documentIds in an AND branch threw documentIds in an AND branch is not supported — restructure the where clause.

Stack: based on #36 (agent/rockets-auth-hardening); #35 sits on top. Review this diff against agent/rockets-auth-hardening, not main.


What changed and why

1. Document-ID predicates compose instead of colliding

mergeAndBranch now intersects the ID sets contributed by each child of an AND, rather than overwriting or throwing. A contradictory query (id = 'a' AND id = 'b') intersects to the empty set and short-circuits to zero rows instead of degrading into a full collection scan.

This is why every branch.documentIds && branch.documentIds.length > 0 check became branch.documentIds !== undefined: an empty array now means "no document matches", which is semantically different from undefined ("this branch has no ID constraint"). Treating the two alike is exactly the full-scan bug.

2. Branch filters apply on the ID path

applyFirestoreFilters(rows, branch.filters) now runs on the ID-selected path in both backends, closing the widening bug described above.

3. create is part of the backend contract

FirestoreBackend.create() is new: the Admin backend uses the SDK's atomic .create(), the in-memory backend rejects a duplicate id. Previously a create went through set(), so re-creating an existing id silently overwrote the document.

4. One implementation of Firestore value semantics

firestore-value.ts is now the single owner, replacing three divergent copies:

  • Dotted field paths are read structurally, preserving missing vs. explicit null. is_null now requires the field to exist and be null; a missing field is no longer reported as null.
  • Canonical type orderingnull < boolean < number < timestamp < string — matching Firestore's ordering rather than JavaScript's <.
  • Structural equality for arrays, maps, byte arrays, and timestamp-like values, instead of === reference comparison.
  • Range operators are type-scoped. between requires the value and both bounds to share a scalar kind, so a number field no longer compares against a string bound via coercion.
  • Recursive timestamp normalization. Previously only top-level Timestamp fields became Date; timestamps nested inside maps and arrays leaked SDK objects to callers.

5. Sorting and comparison deduplicated

firestore-sort.ts replaces the three near-identical sort implementations in the Admin backend, the in-memory backend, and the query runner.


Breaking changes

These are behavior changes to local query evaluation. They move the adapter toward Firestore semantics, so a caller relying on the old behavior was relying on a bug — but the change is observable.

Change Effect
is_null / is_not_null require the field to exist A row missing the field is no longer matched by is_null.
nin excludes missing and null fields A row missing the field is no longer returned by nin, matching Firestore's not-in.
between no longer matches across types A number field with a string bound now matches nothing instead of coercing.
Local ordering throws on unsupported types Ordering by an array or map field now raises a descriptive error instead of producing an arbitrary order.
documentIds inside AND no longer throws Previously an explicit error; now intersected. Queries that were restructured to work around the error still work.
Re-creating an existing document id fails Previously an overwrite via set().

Review focus

Ranked by blast radius.

1. documentIds: [] vs undefinedadmin-firestore.backend.ts, in-memory-firestore.backend.ts, firestore-where.translator.ts

The entire ID-intersection fix rests on this distinction, and it is easy to regress: any future documentIds?.length guard reintroduces the full-scan bug, because an empty intersection would again read as "no ID constraint". All five call sites currently use !== undefined. Worth confirming that reads as deliberate rather than accidental, and deciding whether the interface should carry a comment making the invariant explicit.

2. compareFirestoreValues throws from inside a sort comparator — firestore-value.ts

Ordering by a field that holds an array or a map now throws mid-sort, failing the whole read. That is defensible (an arbitrary order is worse than an error) but it converts a previously-degraded query into a runtime failure on a repository read path. Confirm that's the posture we want for 1.0, versus falling back to a documented ordering.

3. firestoreValuesEqual duck-types on isEqualfirestore-value.ts

Any object exposing an isEqual(other) method takes that branch, not just Firestore SDK values. That is how the SDK's own types are matched without importing them, but it will also catch unrelated domain objects that happen to define isEqual.

4. create failure modes differ between backends — in-memory-firestore.backend.ts:63

The in-memory backend throws a plain Error; the Admin backend surfaces the SDK's ALREADY_EXISTS error. Since parity between these two backends is this PR's whole point, a caller writing catch logic against one backend won't match the other. Neither is a typed repository exception.

5. asDate accepts anything with a toDate() method

Same duck-typing tradeoff as (3), applied to timestamp normalization.


Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update
  • Refactoring
  • Dependency update

Verification

Run against this branch's tip after rebasing onto the current #36 head:

Command Result
corepack yarn build pass
corepack yarn typecheck:spec pass
corepack yarn lint:all pass
Firestore package specs 4 files / 29 tests passed

New coverage: firestore-value.spec.ts (missing vs. null, canonical type ordering, structural equality, recursive normalization), firestore-where.translator.spec.ts (ID intersection including the contradictory case), and firestore-repository.module.spec.ts.

  • Build succeeds (corepack yarn build)
  • Unit tests pass
  • E2E tests pass (corepack yarn test:e2e — verified at the chore: enforce release readiness #35 tip, 33 files / 167 tests)
  • Lint passes (corepack yarn lint:all)

Checklist

  • My code follows the existing patterns in the codebase
  • I have updated relevant documentation (package README + CHANGELOG)
  • I have added tests for new functionality

Deferred to #35

The emulator contract that proves this PR's parity claim lives in #35, not here. packages/rockets-repository-firestore/src/__tests__/firestore-backend.emulator-spec.ts runs both backends against a real Java-backed Firestore emulator and asserts identical result sets. It is in #35 because it needs firebase-tools, firebase.json, firestore.rules, and the emulator CI job, which are that PR's scope.

The practical consequence for review: the assertions in this PR are verified against the in-memory double only. If you want the real-Firestore signal before approving, corepack yarn test:firestore-emulator at the #35 tip covers three of the claims here — explicit-null vs. missing nested fields, cross-type range exclusion, and nested ordering with recursive timestamp normalization. Document-ID intersection, filters-on-the-ID-path, duplicate-create rejection, and the nin / between post-filter changes are not yet covered by an emulator assertion.

@leoafarias
leoafarias force-pushed the agent/rockets-auth-hardening branch from a089fdb to a080064 Compare August 8, 2026 23:53
@leoafarias
leoafarias force-pushed the agent/rockets-firestore-parity branch from 4b84dcd to 967b45b Compare August 8, 2026 23:53
@leoafarias
leoafarias force-pushed the agent/rockets-auth-hardening branch from a080064 to f2f18fc Compare August 8, 2026 23:56
@leoafarias
leoafarias force-pushed the agent/rockets-firestore-parity branch from 967b45b to 4c27233 Compare August 8, 2026 23:56
@leoafarias
leoafarias force-pushed the agent/rockets-auth-hardening branch from 22887b2 to 06769e3 Compare August 9, 2026 16:14
@leoafarias
leoafarias force-pushed the agent/rockets-firestore-parity branch from 4c27233 to 094e9e5 Compare August 9, 2026 16:14
@leoafarias
leoafarias marked this pull request as ready for review August 9, 2026 18:26
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.

1 participant