From 9aa34505bedbfe32e5fc7e44e3b946ce777aceda Mon Sep 17 00:00:00 2001 From: Frederik Wallner Date: Wed, 9 Sep 2026 16:23:38 +0200 Subject: [PATCH 1/4] fix(firestore): reject undeclared update keys and support typed dotted field paths Repository.update encoded through the model's update struct, which strips excess keys, so a Firestore field path like 'metaData.deleted' vanished and the SDK rejected the empty payload with an unrelated error (#63). - Encode all repository request payloads with onExcessProperty: 'error': an undeclared key fails with a SchemaError naming it. - Fail an empty update payload with FirestoreError invalid-argument. - Accept dotted field paths in update, typed via UpdateData. Each path resolves to its leaf schema in Model.update (through Struct, Model.Struct, Class, Record, optional, OptionalDeletable) and encodes on its own, so nested sentinels and transforms work. Fixes #63 Co-Authored-By: Claude Fable 5.1 --- README.md | 15 ++ packages/effect-firebase/AGENTS.md | 53 +++-- .../src/lib/firestore/model/fetch.ts | 24 ++- .../lib/firestore/model/repository.spec.ts | 181 +++++++++++++++++- .../src/lib/firestore/model/repository.ts | 116 +++++++++-- .../lib/firestore/model/update-path.spec.ts | 62 ++++++ .../src/lib/firestore/model/update-path.ts | 114 +++++++++++ 7 files changed, 525 insertions(+), 40 deletions(-) create mode 100644 packages/effect-firebase/src/lib/firestore/model/update-path.spec.ts create mode 100644 packages/effect-firebase/src/lib/firestore/model/update-path.ts diff --git a/README.md b/README.md index 3f7e86d..15ca029 100644 --- a/README.md +++ b/README.md @@ -132,6 +132,21 @@ for a document you expect to be new, `'update'` with `merge: true` for one you expect to exist. Both are assertions rather than checks — `set` will not verify which case it is actually in. +### Updating nested fields + +`update` accepts Firestore dotted field paths, typed against the model, so a +nested field can change without rewriting its siblings. A whole-field key +replaces the entire map, as in the Firestore SDKs. + +```typescript +yield * repo.update(postId, { 'metaData.deleted': true }); // touches only metaData.deleted +yield * repo.update(postId, { metaData: { deleted: true, tags: [] } }); // replaces metaData +yield * repo.update(postId, { 'stats.likes': Firestore.increment(1) }); // nested sentinel +``` + +Keys the model does not declare fail with a `SchemaError` naming the key; an +empty payload fails with `FirestoreError` code `invalid-argument`. + ### Client app ```typescript diff --git a/packages/effect-firebase/AGENTS.md b/packages/effect-firebase/AGENTS.md index b5fe347..01a01dc 100644 --- a/packages/effect-firebase/AGENTS.md +++ b/packages/effect-firebase/AGENTS.md @@ -130,19 +130,19 @@ service via `Admin.layer`/`Client.layer`/mock). Repository methods (all fail with `ModelError = FirestoreError | UnknownError | NoSuchElementError | SchemaError`): -| Method | Returns | Notes | -| ------------------------------------- | ------------------------------ | --------------------------------------------------------------------- | -| `add(data)` | `Effect` | `data: typeof Model.insert.Type`. Firestore picks the id. | -| `set(id, { data, variant?, merge? })` | `Effect` | Upsert at a known id. See "Choosing `set` variant" below. | -| `update(id, partial)` | `Effect` | Fails `FirestoreError` code `not-found` if absent. Accepts sentinels. | -| `getById(id)` | `Effect>` | `Option.none()` when missing. | -| `getByIdStream(id)` | `Stream>` | Live `onSnapshot`. | -| `delete(id)` | `Effect` | | -| `deleteRecursive(id)` | `Effect` | **Admin SDK only**; dies on the client layer. | -| `query(constraints)` | `Effect>` | | -| `queryStream(constraints)` | `Stream>` | Live. | -| `getByQuery(constraints)` | `Effect>` | First match. | -| `getByQueryStream(constraints)` | `Stream>` | Live first match. | +| Method | Returns | Notes | +| ------------------------------------- | ------------------------------ | ----------------------------------------------------------------------- | +| `add(data)` | `Effect` | `data: typeof Model.insert.Type`. Firestore picks the id. | +| `set(id, { data, variant?, merge? })` | `Effect` | Upsert at a known id. See "Choosing `set` variant" below. | +| `update(id, data)` | `Effect` | Fails `not-found` if absent. Sentinels + dotted field paths. See below. | +| `getById(id)` | `Effect>` | `Option.none()` when missing. | +| `getByIdStream(id)` | `Stream>` | Live `onSnapshot`. | +| `delete(id)` | `Effect` | | +| `deleteRecursive(id)` | `Effect` | **Admin SDK only**; dies on the client layer. | +| `query(constraints)` | `Effect>` | | +| `queryStream(constraints)` | `Stream>` | Live. | +| `getByQuery(constraints)` | `Effect>` | First match. | +| `getByQueryStream(constraints)` | `Stream>` | Live first match. | ### Choosing `set` variant @@ -161,6 +161,30 @@ To create-only at a known id without clobbering, read then write inside `Firestore.withTransaction`; a bare `getById` then `set` is a race. Prefer `add` (always insert) or `update` (always update) when the intent is fixed. +### Updating nested fields + +`update` takes any subset of `Model.update.Type` plus Firestore dotted field +paths into nested maps. A dotted key touches only that nested field; a +whole-field key replaces the whole map (Firestore semantics). + +```ts +yield * repo.update(id, { 'metaData.deleted': true }); // only metaData.deleted +yield * repo.update(id, { metaData: { deleted: true, tags: [] } }); // replaces metaData +``` + +Paths are typed (`UpdateData`) and descend through `Schema.Struct`, +`Model.Struct`, `Schema.Class`, `Schema.Record` (any key), `Schema.optional` +and `OptionalDeletable`; arrays, `DateTime`, `Timestamp`, `GeoPoint`, +`Reference` and sentinel classes are leaves. Each leaf is encoded through its +own field schema, so a nested `Firestore.Number` accepts `increment(n)` at +`'stats.likes'`. + +Keys the model does not declare (typos, paths into scalars) fail with +`SchemaError` naming the key; they are never dropped. An empty payload fails +with `FirestoreError` code `invalid-argument` instead of the SDK's "At least +one field must be updated". `add` and `set` reject undeclared keys the same +way. + ## Queries ```ts @@ -406,6 +430,9 @@ root: https://github.com/fwal/effect-firebase/blob/main/REACT.md. variants that helper allows. 9. Style used throughout the library: explicit lambdas (`Effect.map((x) => f(x))`), no point-free `Effect.map(f)`. +10. Nested updates use dotted keys (`'a.b': v`), not nested partial objects: + `{ a: { b: v } }` replaces the whole `a` map. Undeclared keys fail with + `SchemaError`; `update` never silently drops them. ## Where to look diff --git a/packages/effect-firebase/src/lib/firestore/model/fetch.ts b/packages/effect-firebase/src/lib/firestore/model/fetch.ts index 66fa6f0..b3cd466 100644 --- a/packages/effect-firebase/src/lib/firestore/model/fetch.ts +++ b/packages/effect-firebase/src/lib/firestore/model/fetch.ts @@ -1,4 +1,16 @@ import { Array as Arr, Cause, Effect, Option, Schema, Stream } from 'effect'; +import type { SchemaAST } from 'effect'; + +/** + * Request payloads are encoded strictly: a key the request schema does not + * declare fails with a `SchemaError` naming the key instead of being + * silently dropped. Without this, an undeclared key (a typo, or a dotted + * field path handed to a struct schema) vanishes before the write reaches + * Firestore, which then rejects the empty payload with an unrelated error. + */ +export const strictEncoding: SchemaAST.ParseOptions = { + onExcessProperty: 'error', +}; /** * Find all records in the collection. @@ -15,7 +27,7 @@ export const findAll = < request: Req['Encoded'], ) => Effect.Effect, E, R>; }) => { - const encodeRequest = Schema.encodeEffect(options.Request); + const encodeRequest = Schema.encodeEffect(options.Request, strictEncoding); const decode = Schema.decodeUnknownEffect( Schema.mutable(Schema.Array(options.Result)), ); @@ -69,7 +81,7 @@ const _void = (options: { readonly Request: Req; readonly execute: (request: Req['Encoded']) => Effect.Effect; }) => { - const encode = Schema.encodeEffect(options.Request); + const encode = Schema.encodeEffect(options.Request, strictEncoding); return ( request: Req['Type'], ): Effect.Effect => @@ -93,7 +105,7 @@ export const findOneOption = < request: Req['Encoded'], ) => Effect.Effect, E, R>; }) => { - const encodeRequest = Schema.encodeEffect(options.Request); + const encodeRequest = Schema.encodeEffect(options.Request, strictEncoding); const decode = Schema.decodeUnknownEffect(options.Result); return ( request: Req['Type'], @@ -132,7 +144,7 @@ export const findOne = < request: Req['Encoded'], ) => Effect.Effect, E, R>; }) => { - const encodeRequest = Schema.encodeEffect(options.Request); + const encodeRequest = Schema.encodeEffect(options.Request, strictEncoding); const decode = Schema.decodeUnknownEffect(options.Result); return ( request: Req['Type'], @@ -171,7 +183,7 @@ export const streamOne = < request: Req['Encoded'], ) => Stream.Stream, E, R>; }) => { - const encodeRequest = Schema.encodeEffect(options.Request); + const encodeRequest = Schema.encodeEffect(options.Request, strictEncoding); const decode = Schema.decodeUnknownEffect(options.Result); return ( request: Req['Type'], @@ -207,7 +219,7 @@ export const streamAll = < request: Req['Encoded'], ) => Stream.Stream, E, R>; }) => { - const encodeRequest = Schema.encodeEffect(options.Request); + const encodeRequest = Schema.encodeEffect(options.Request, strictEncoding); const decode = Schema.decodeUnknownEffect( Schema.mutable(Schema.Array(options.Result)), ); diff --git a/packages/effect-firebase/src/lib/firestore/model/repository.spec.ts b/packages/effect-firebase/src/lib/firestore/model/repository.spec.ts index 8e5b3f7..533906b 100644 --- a/packages/effect-firebase/src/lib/firestore/model/repository.spec.ts +++ b/packages/effect-firebase/src/lib/firestore/model/repository.spec.ts @@ -1,8 +1,12 @@ import { describe, expect, it, vi } from 'vitest'; -import { Effect, Layer, Option, Schema } from 'effect'; +import { DateTime, Effect, Layer, Option, Schema } from 'effect'; import { Model } from 'effect/unstable/schema'; import { makeRepository } from './repository.js'; import * as FirestoreModel from './datetime.js'; +import * as FirestoreNumber from './number.js'; +import { OptionalDeletable } from './optional.js'; +import { increment } from '../fields/increment.js'; +import { Timestamp, TimestampDateTimeUtc } from '../schema/timestamp.js'; import { FirestoreService } from '../firestore-service.js'; import type { FirestoreServiceShape } from '../firestore-service.js'; import type { Snapshot } from '../snapshot.js'; @@ -22,6 +26,21 @@ class StampedModel extends Model.Class('StampedModel')({ updatedAt: FirestoreModel.DateTimeUpdate, }) {} +/** Nested maps declared through the combinators a field path may cross. */ +class NestedModel extends Model.Class('NestedModel')({ + id: Model.GeneratedByDb(PostId), + title: Schema.String, + metaData: Schema.Struct({ + deleted: Schema.Boolean, + tags: Schema.Array(Schema.String), + }), + stats: Model.Struct({ likes: FirestoreNumber.Number }), + profile: OptionalDeletable( + Schema.Struct({ lastSeenAt: TimestampDateTimeUtc }), + ), + counters: Schema.Record(Schema.String, Schema.Number), +}) {} + const notMocked = (name: string) => (): never => { throw new Error(`FirestoreService.${name}: not mocked`); }; @@ -54,6 +73,16 @@ const makeStampedRepo = (overrides: Partial) => spanPrefix: 'test', }).pipe(Effect.provide(makeLayer(overrides))); +const makeNestedRepo = (overrides: Partial) => + makeRepository(NestedModel, { + collectionPath: 'posts', + idField: 'id', + spanPrefix: 'test', + }).pipe(Effect.provide(makeLayer(overrides))); + +const failureOf = (effect: Effect.Effect) => + Effect.runPromise(Effect.flip(effect)); + const snap = (id: string, data: Record): Snapshot => [{ id, path: `posts/${id}` }, data] as const; @@ -242,6 +271,23 @@ describe('Repository', () => { }); }); + describe('set strictness', () => { + it('rejects a key the model does not declare', async () => { + const setMock = vi.fn(() => Effect.succeed(undefined)); + const repo = await Effect.runPromise(makeRepo({ set: setMock })); + const error = await failureOf( + repo.set(PostId.make('post-1'), { + // @ts-expect-error not a declared field + data: { title: 'Hello', extra: 1 }, + }), + ); + + expect(error._tag).toBe('SchemaError'); + expect(String(error)).toContain('extra'); + expect(setMock).not.toHaveBeenCalled(); + }); + }); + describe('update', () => { it('calls firestore.update with the correct path and partial data', async () => { const updateMock = vi.fn(() => Effect.succeed(undefined)); @@ -254,6 +300,139 @@ describe('Repository', () => { title: 'Updated', }); }); + + it('rejects a key the model does not declare instead of dropping it (#63)', async () => { + const updateMock = vi.fn(() => Effect.succeed(undefined)); + const repo = await Effect.runPromise(makeRepo({ update: updateMock })); + const error = await failureOf( + repo.update(PostId.make('post-1'), { + // @ts-expect-error not a declared field + 'metaData.deleted': true, + }), + ); + + expect(error._tag).toBe('SchemaError'); + expect(String(error)).toContain('metaData.deleted'); + expect(updateMock).not.toHaveBeenCalled(); + }); + + it('fails an empty payload with invalid-argument before reaching Firestore', async () => { + const updateMock = vi.fn(() => Effect.succeed(undefined)); + const repo = await Effect.runPromise(makeRepo({ update: updateMock })); + const error = await failureOf(repo.update(PostId.make('post-1'), {})); + + expect(error).toMatchObject({ + _tag: 'FirestoreError', + code: 'invalid-argument', + }); + expect(updateMock).not.toHaveBeenCalled(); + }); + + describe('field paths', () => { + const payloadOf = (mock: ReturnType) => + (mock.mock.calls[0] as unknown as [string, Record])[1]; + + it('passes a dotted path into a nested struct through unchanged', async () => { + const updateMock = vi.fn(() => Effect.succeed(undefined)); + const repo = await Effect.runPromise( + makeNestedRepo({ update: updateMock }), + ); + await Effect.runPromise( + repo.update(PostId.make('post-1'), { + title: 'Updated', + 'metaData.deleted': true, + }), + ); + + expect(updateMock).toHaveBeenCalledWith('posts/post-1', { + title: 'Updated', + 'metaData.deleted': true, + }); + }); + + it('encodes a nested leaf through its own field schema', async () => { + const updateMock = vi.fn(() => Effect.succeed(undefined)); + const repo = await Effect.runPromise( + makeNestedRepo({ update: updateMock }), + ); + await Effect.runPromise( + repo.update(PostId.make('post-1'), { + 'profile.lastSeenAt': DateTime.makeUnsafe(1_000), + }), + ); + + const encoded = payloadOf(updateMock)['profile.lastSeenAt']; + expect(encoded).toBeInstanceOf(Timestamp); + expect((encoded as Timestamp).toMillis()).toBe(1_000); + }); + + it('accepts a sentinel on a nested field declared for it', async () => { + const updateMock = vi.fn(() => Effect.succeed(undefined)); + const repo = await Effect.runPromise( + makeNestedRepo({ update: updateMock }), + ); + await Effect.runPromise( + repo.update(PostId.make('post-1'), { 'stats.likes': increment(1) }), + ); + + expect(payloadOf(updateMock)['stats.likes']).toEqual(increment(1)); + }); + + it('resolves dynamic keys through a Record field', async () => { + const updateMock = vi.fn(() => Effect.succeed(undefined)); + const repo = await Effect.runPromise( + makeNestedRepo({ update: updateMock }), + ); + await Effect.runPromise( + repo.update(PostId.make('post-1'), { 'counters.visits': 3 }), + ); + + expect(payloadOf(updateMock)).toEqual({ 'counters.visits': 3 }); + }); + + it('rejects a path whose leaf is not declared, naming the path', async () => { + const updateMock = vi.fn(() => Effect.succeed(undefined)); + const repo = await Effect.runPromise( + makeNestedRepo({ update: updateMock }), + ); + const error = await failureOf( + repo.update(PostId.make('post-1'), { + // @ts-expect-error `nope` is not a field of metaData + 'metaData.nope': true, + }), + ); + + expect(error._tag).toBe('SchemaError'); + expect(String(error)).toContain('metaData.nope'); + expect(updateMock).not.toHaveBeenCalled(); + }); + + it('rejects a leaf value of the wrong type, naming the path', async () => { + const updateMock = vi.fn(() => Effect.succeed(undefined)); + const repo = await Effect.runPromise( + makeNestedRepo({ update: updateMock }), + ); + const error = await failureOf( + repo.update(PostId.make('post-1'), { + // @ts-expect-error deleted is a boolean + 'metaData.deleted': 'yes', + }), + ); + + expect(error._tag).toBe('SchemaError'); + expect(String(error)).toContain('metaData.deleted'); + expect(updateMock).not.toHaveBeenCalled(); + }); + + it('does not treat a scalar field as a map', () => { + const repo = Effect.runSync(makeNestedRepo({})); + const write = repo.update(PostId.make('post-1'), { + // @ts-expect-error title is a string, not a map + 'title.length': 1, + }); + expect(write).toBeDefined(); + }); + }); }); describe('delete', () => { diff --git a/packages/effect-firebase/src/lib/firestore/model/repository.ts b/packages/effect-firebase/src/lib/firestore/model/repository.ts index 8638b32..ad724c4 100644 --- a/packages/effect-firebase/src/lib/firestore/model/repository.ts +++ b/packages/effect-firebase/src/lib/firestore/model/repository.ts @@ -6,6 +6,13 @@ import { NoSuchElementError, UnknownError } from 'effect/Cause'; import { FirestoreError } from '../errors.js'; import * as Fetch from './fetch.js'; import type { QueryConstraint } from '../query/constraints.js'; +import { + isFieldPath, + resolveFieldPath, + type UpdateData, +} from './update-path.js'; + +export type { UpdateData } from './update-path.js'; export type ModelError = FirestoreError | UnknownError | NoSuchElementError | Schema.SchemaError; @@ -122,14 +129,23 @@ export type Repository< >; /** - * Update a document model. + * Update a document model. Fails with `FirestoreError` code `not-found` + * if the document is absent. + * + * `data` is any subset of the `update` variant's fields, plus Firestore + * dot-separated paths into nested maps (`'metaData.deleted': true`) + * which update just that nested field. A whole-field key replaces the + * whole value, so `metaData: { ... }` overwrites the entire map. Keys + * the model does not declare fail with a `SchemaError` naming the key; + * an empty payload fails with `FirestoreError` code `invalid-argument`. + * * @param id - The ID of the document model to update. - * @param data - The partial data to update the document model with. All fields are optional. + * @param data - The fields and field paths to update. See {@link UpdateData}. * @returns A unit value. */ readonly update: ( id: IdSchema['Type'], - data: Partial>, + data: UpdateData>, ) => Effect.Effect< void, ModelError, @@ -392,7 +408,9 @@ export const makeRepository = < ); }; - // Create schema for update: required id + partial data fields (all optional) + // Request schema for update: required id + partial data fields (all + // optional). Encoded strictly, so an undeclared key fails with a + // SchemaError naming it instead of being dropped from the payload. const PartialDataSchema = ( Model.update as Schema.Struct ) @@ -403,30 +421,88 @@ export const makeRepository = < [options.idField]: idSchema, }).pipe(Schema.fieldsAssign(PartialDataSchema.fields)); - const updateSchema = Fetch.void({ - Request: updateFieldsSchema, - execute: (input: unknown) => { - const record = input as Record; - const { [options.idField as string]: id, ...data } = record; - return firestore.update( - `${options.collectionPath}/${id as string}`, - data, + const encodeUpdateFields = Schema.encodeUnknownEffect( + updateFieldsSchema, + Fetch.strictEncoding, + ); + + // A dotted key ('metaData.deleted') names a nested field. It resolves to + // its leaf schema in Model.update and is encoded on its own, wrapped in a + // one-key struct so a failure still reports the offending path. A key + // that does not resolve stays in the struct payload, where the strict + // encoder rejects it by name. Encoders are cached per path because + // Schema.encodeUnknownEffect compiles on construction. + type LeafEncoder = ( + value: unknown, + ) => Effect.Effect; + const leafEncoders = new Map(); + const leafEncoder = (path: string): Option.Option => { + const cached = leafEncoders.get(path); + if (cached !== undefined) return Option.some(cached); + return Option.map(resolveFieldPath(PartialDataSchema, path), (leaf) => { + const encodeLeaf = Schema.encodeUnknownEffect( + Schema.Struct({ [path]: leaf }), + Fetch.strictEncoding, ); - }, - }); + const encoder: LeafEncoder = (value) => + Effect.map( + encodeLeaf({ [path]: value }), + (encoded) => (encoded as Record)[path], + ); + leafEncoders.set(path, encoder); + return encoder; + }); + }; const update = ( id: IdSchema['Type'], - data: Partial>, + data: UpdateData>, ) => - updateSchema({ - [options.idField]: id, - ...data, - } as Parameters[0]).pipe( + Effect.gen(function* () { + const fields: Record = { [options.idField]: id }; + const paths: Array = []; + for (const [key, value] of Object.entries( + data as Record, + )) { + const encoder = isFieldPath(key) ? leafEncoder(key) : Option.none(); + if (Option.isSome(encoder)) { + paths.push([key, value, encoder.value]); + } else { + fields[key] = value; + } + } + + const { [options.idField as string]: encodedId, ...payload } = + (yield* encodeUpdateFields(fields)) as Record; + for (const [key, value, encode] of paths) { + payload[key] = yield* encode(value); + } + + // Firestore rejects an empty update with a message about argument + // shape; name the actual problem instead. + if (Object.keys(payload).length === 0) { + return yield* new FirestoreError({ + code: 'invalid-argument', + name: 'FirestoreError', + message: `${options.spanPrefix}.update: at least one field must be updated (payload was empty)`, + }); + } + + yield* firestore.update( + `${options.collectionPath}/${encodedId as string}`, + payload, + ); + }).pipe( Effect.withSpan(`${options.spanPrefix}.update`, { attributes: { id, data }, }), - ); + ) as Effect.Effect< + void, + ModelError, + | S['DecodingServices'] + | S['EncodingServices'] + | S['update']['EncodingServices'] + >; const getByIdSchema = Fetch.findOneOption({ Request: idSchema, diff --git a/packages/effect-firebase/src/lib/firestore/model/update-path.spec.ts b/packages/effect-firebase/src/lib/firestore/model/update-path.spec.ts new file mode 100644 index 0000000..39b5b5b --- /dev/null +++ b/packages/effect-firebase/src/lib/firestore/model/update-path.spec.ts @@ -0,0 +1,62 @@ +import { describe, expect, it } from 'vitest'; +import { Option, Schema } from 'effect'; +import { Model } from 'effect/unstable/schema'; +import { isFieldPath, resolveFieldPath } from './update-path.js'; +import { OptionalDeletable } from './optional.js'; +import * as FirestoreNumber from './number.js'; + +class Inner extends Schema.Class('Inner')({ x: Schema.Number }) {} + +class Doc extends Model.Class('Doc')({ + id: Schema.String, + plain: Schema.Struct({ a: Schema.Struct({ b: Schema.Boolean }) }), + optional: Schema.optional(Schema.Struct({ a: Schema.String })), + deletable: OptionalDeletable(Schema.Struct({ a: Schema.String })), + variant: Model.Struct({ likes: FirestoreNumber.Number }), + klass: Inner, + record: Schema.Record(Schema.String, Schema.Number), + scalar: Schema.String, +}) {} + +const root = Doc.update; +const resolves = (path: string) => Option.isSome(resolveFieldPath(root, path)); + +describe('resolveFieldPath', () => { + it('descends through nested structs', () => { + expect(resolves('plain.a')).toBe(true); + expect(resolves('plain.a.b')).toBe(true); + }); + + it('unwraps optional, OptionalDeletable and variant-struct fields', () => { + expect(resolves('optional.a')).toBe(true); + expect(resolves('deletable.a')).toBe(true); + expect(resolves('variant.likes')).toBe(true); + }); + + it('descends into Schema.Class fields', () => { + expect(resolves('klass.x')).toBe(true); + }); + + it('resolves any key under a Record to its value schema', () => { + expect(resolves('record.anything')).toBe(true); + }); + + it('returns None for undeclared segments and for paths into scalars', () => { + expect(resolves('plain.nope')).toBe(false); + expect(resolves('plain.a.b.c')).toBe(false); + expect(resolves('scalar.length')).toBe(false); + expect(resolves('missing.a')).toBe(false); + }); + + it('returns the leaf schema, not a wrapper, so it encodes standalone', () => { + const leaf = Option.getOrThrow(resolveFieldPath(root, 'plain.a.b')); + expect(Schema.encodeUnknownSync(leaf as Schema.Boolean)(true)).toBe(true); + }); +}); + +describe('isFieldPath', () => { + it('is true only for dotted keys', () => { + expect(isFieldPath('a.b')).toBe(true); + expect(isFieldPath('a')).toBe(false); + }); +}); diff --git a/packages/effect-firebase/src/lib/firestore/model/update-path.ts b/packages/effect-firebase/src/lib/firestore/model/update-path.ts new file mode 100644 index 0000000..1afccd7 --- /dev/null +++ b/packages/effect-firebase/src/lib/firestore/model/update-path.ts @@ -0,0 +1,114 @@ +import { Option, Schema } from 'effect'; + +/** + * The payload accepted by {@link Repository.update}: any subset of the + * model's `update` fields, plus Firestore dot-separated field paths into + * nested maps (`'metaData.deleted': true`). + * + * A dotted key updates just that nested field and leaves its siblings + * untouched. A whole-field key (`metaData: { ... }`) replaces the entire + * map, exactly as the Firestore SDKs do, so nested values under whole-field + * keys must be complete. + * + * Paths descend through plain object types (`Schema.Struct`, `Model.Struct`, + * `Schema.Class`, `Schema.Record`) and through `Option` (so an + * `OptionalDeletable` map is reachable even when currently absent). They stop + * at arrays, `DateTime`, `Timestamp`, `GeoPoint`, `Reference` and sentinel + * classes, which are written whole. + */ +export type UpdateData = Partial & NestedUpdateFields; + +type UnionToIntersection = ( + U extends unknown ? (k: U) => void : never +) extends (k: infer I) => void + ? I + : never; + +type NestedUpdateFields = UnionToIntersection< + { + [K in keyof T & string]: ChildUpdateFields; + }[keyof T & string] +>; + +// Interfaces and class instances (DateTime, Option, Timestamp, sentinels…) +// lack an implicit index signature, so they fail `Record` +// and are treated as leaves; struct types and records pass and are descended. +type ChildUpdateFields = + V extends Option.Option + ? ChildUpdateFields + : V extends Record + ? AddPrefixToKeys> + : never; + +type AddPrefixToKeys = { + [K in keyof T & string as `${Prefix}.${K}`]?: T[K]; +}; + +/** + * Structural view of the schema combinators a field path may pass through. + * These are the public properties `Schema.Struct`, `Schema.Class`, + * `Schema.optional`/`optionalKey`, `Schema.Union`, `Schema.decodeTo` (and + * `OptionFromUndefinedOr` etc.) and `Schema.Record` expose. + */ +type Walkable = Schema.Top & { + readonly fields?: Record; + readonly schema?: Schema.Top; + readonly members?: ReadonlyArray; + readonly from?: Schema.Top; + readonly key?: Schema.Top; + readonly value?: Schema.Top; +}; + +/** + * Find the schema for a single child key of `schema`, unwrapping the + * combinators a nested map may be declared through. + * + * Transformations (`decodeTo`) are followed on their `from` side: that is + * the codec that knows how to encode the nested leaf, whereas the `to` side + * of e.g. `OptionFromUndefinedOr` is the plain in-memory type. This assumes + * the transformation keeps keys in place, which every combinator in this + * library does. + */ +const child = (schema: Schema.Top, key: string): Option.Option => { + const s = schema as Walkable; + if (s.fields !== undefined) { + const field = s.fields[key]; + return field === undefined ? Option.none() : Option.some(field); + } + if (s.key !== undefined && s.value !== undefined) { + return Option.some(s.value); + } + if (s.from !== undefined) { + return child(s.from, key); + } + if (s.schema !== undefined) { + return child(s.schema, key); + } + if (s.members !== undefined) { + for (const member of s.members) { + const found = child(member, key); + if (Option.isSome(found)) return found; + } + } + return Option.none(); +}; + +/** + * Resolve a Firestore field path (`'a.b.c'`) against a struct schema to the + * schema of the leaf it names. `None` when any segment is not a declared + * field, so the caller can reject the key instead of dropping it. + */ +export const resolveFieldPath = ( + root: Schema.Top, + path: string, +): Option.Option => + path + .split('.') + .reduce>( + (current, segment) => + Option.flatMap(current, (schema) => child(schema, segment)), + Option.some(root), + ); + +/** Whether a payload key is a Firestore dotted field path. */ +export const isFieldPath = (key: string): boolean => key.includes('.'); From e5cfd4ba7620f5180a4624635aa401a41e2d2887 Mon Sep 17 00:00:00 2001 From: Frederik Wallner Date: Wed, 9 Sep 2026 21:55:47 +0200 Subject: [PATCH 2/4] fix(firestore): validate Record keys in field paths; document strict encoding in MIGRATION Co-Authored-By: Claude Fable 5.1 --- packages/effect-firebase/MIGRATION.md | 15 +++++++++++++++ .../src/lib/firestore/model/update-path.spec.ts | 9 +++++++++ .../src/lib/firestore/model/update-path.ts | 10 ++++++++-- 3 files changed, 32 insertions(+), 2 deletions(-) diff --git a/packages/effect-firebase/MIGRATION.md b/packages/effect-firebase/MIGRATION.md index 8358f00..798b219 100644 --- a/packages/effect-firebase/MIGRATION.md +++ b/packages/effect-firebase/MIGRATION.md @@ -256,6 +256,21 @@ encoder, replace it with `repo.set` and choose the `variant` deliberately — `'insert'` re-stamps `DateTimeInsert` fields on every write, `'update'` with `merge: true` preserves them. +**Undeclared keys are rejected.** `add`, `set` and `update` used to strip any +key the model does not declare before writing; a `repo.update(id, { 'a.b': 1 })` +degenerated into an empty write that Firestore rejected with "At least one +field must be updated". They now fail with a `SchemaError` naming the key, +and an empty `update` payload fails with `FirestoreError` code +`invalid-argument`. If a call site relied on extra keys being dropped, remove +them from the payload. + +**Nested updates use dotted paths.** `update` accepts Firestore field paths +typed against the model (`'metaData.deleted': true`) and encodes each leaf +through its own field schema, so nested sentinels work. Replace hand-rolled +`FirestoreService.update(path, { 'a.b': v })` calls with `repo.update(id, +{ 'a.b': v })`. Note that a whole-field key (`metaData: { ... }`) still +replaces the entire map. + ### 8. `FirestoreService` shape changes (custom layers and test doubles) Only relevant if you implement `FirestoreService` yourself or pass overrides diff --git a/packages/effect-firebase/src/lib/firestore/model/update-path.spec.ts b/packages/effect-firebase/src/lib/firestore/model/update-path.spec.ts index 39b5b5b..cc64d84 100644 --- a/packages/effect-firebase/src/lib/firestore/model/update-path.spec.ts +++ b/packages/effect-firebase/src/lib/firestore/model/update-path.spec.ts @@ -15,6 +15,10 @@ class Doc extends Model.Class('Doc')({ variant: Model.Struct({ likes: FirestoreNumber.Number }), klass: Inner, record: Schema.Record(Schema.String, Schema.Number), + keyed: Schema.Record( + Schema.TemplateLiteral(['k_', Schema.String]), + Schema.Number, + ), scalar: Schema.String, }) {} @@ -41,6 +45,11 @@ describe('resolveFieldPath', () => { expect(resolves('record.anything')).toBe(true); }); + it('checks Record segments against a constrained key schema', () => { + expect(resolves('keyed.k_visits')).toBe(true); + expect(resolves('keyed.visits')).toBe(false); + }); + it('returns None for undeclared segments and for paths into scalars', () => { expect(resolves('plain.nope')).toBe(false); expect(resolves('plain.a.b.c')).toBe(false); diff --git a/packages/effect-firebase/src/lib/firestore/model/update-path.ts b/packages/effect-firebase/src/lib/firestore/model/update-path.ts index 1afccd7..a3ffad4 100644 --- a/packages/effect-firebase/src/lib/firestore/model/update-path.ts +++ b/packages/effect-firebase/src/lib/firestore/model/update-path.ts @@ -48,7 +48,8 @@ type AddPrefixToKeys = { * Structural view of the schema combinators a field path may pass through. * These are the public properties `Schema.Struct`, `Schema.Class`, * `Schema.optional`/`optionalKey`, `Schema.Union`, `Schema.decodeTo` (and - * `OptionFromUndefinedOr` etc.) and `Schema.Record` expose. + * `OptionFromUndefinedOr` etc.) and `Schema.Record` expose. Record + * segments are checked against the record's key schema. */ type Walkable = Schema.Top & { readonly fields?: Record; @@ -76,7 +77,12 @@ const child = (schema: Schema.Top, key: string): Option.Option => { return field === undefined ? Option.none() : Option.some(field); } if (s.key !== undefined && s.value !== undefined) { - return Option.some(s.value); + // A Record admits any key its key schema accepts; a segment the key + // schema rejects (e.g. a template literal or branded key) is not a + // field, so the caller rejects it like any undeclared key. + return Schema.is(s.key as Schema.Schema)(key) + ? Option.some(s.value) + : Option.none(); } if (s.from !== undefined) { return child(s.from, key); From f307fbd0b3d4c1948714ab8b6a63c4d308d1fd30 Mon Sep 17 00:00:00 2001 From: Frederik Wallner Date: Thu, 10 Sep 2026 08:27:49 +0200 Subject: [PATCH 3/4] feat(firestore): resolve field paths via the schema AST; cap depth for recursive schemas Walk SchemaAST (Objects, Union, Suspend, Declaration, encoding links) instead of the Schema wrapper API so Schema.suspend maps resolve. Cap paths at MAX_FIELD_PATH_DEPTH (5) at both type and runtime level: without a cap, UpdateData on a recursive type alias is a circular mapped type (TS2615). Co-Authored-By: Claude Fable 5.1 --- packages/effect-firebase/AGENTS.md | 16 +- .../src/lib/firestore/firestore.ts | 1 + .../lib/firestore/model/update-path.spec.ts | 69 ++++++- .../src/lib/firestore/model/update-path.ts | 169 ++++++++++-------- 4 files changed, 179 insertions(+), 76 deletions(-) diff --git a/packages/effect-firebase/AGENTS.md b/packages/effect-firebase/AGENTS.md index 01a01dc..d7c7e9c 100644 --- a/packages/effect-firebase/AGENTS.md +++ b/packages/effect-firebase/AGENTS.md @@ -173,11 +173,17 @@ yield * repo.update(id, { metaData: { deleted: true, tags: [] } }); // replaces ``` Paths are typed (`UpdateData`) and descend through `Schema.Struct`, -`Model.Struct`, `Schema.Class`, `Schema.Record` (any key), `Schema.optional` -and `OptionalDeletable`; arrays, `DateTime`, `Timestamp`, `GeoPoint`, -`Reference` and sentinel classes are leaves. Each leaf is encoded through its -own field schema, so a nested `Firestore.Number` accepts `increment(n)` at -`'stats.likes'`. +`Model.Struct`, `Schema.Class`, `Schema.Record` (keys checked against the key +schema), `Schema.suspend`, `Schema.optional` and `OptionalDeletable`; arrays, +`DateTime`, `Timestamp`, `GeoPoint`, `Reference` and sentinel classes are +leaves. Each leaf is encoded through its own field schema, so a nested +`Firestore.Number` accepts `increment(n)` at `'stats.likes'`. + +Depth is capped at `Firestore.MAX_FIELD_PATH_DEPTH` (5 levels below a +top-level field) at both the type and runtime level; deeper writes go through +`FirestoreService.update`. Recursive schemas work: declare the recursive type +as a type alias to get typed paths into it (an interface is a leaf at the type +level; the runtime resolves either). Keys the model does not declare (typos, paths into scalars) fail with `SchemaError` naming the key; they are never dropped. An empty payload fails diff --git a/packages/effect-firebase/src/lib/firestore/firestore.ts b/packages/effect-firebase/src/lib/firestore/firestore.ts index ef04978..c8f4d9e 100644 --- a/packages/effect-firebase/src/lib/firestore/firestore.ts +++ b/packages/effect-firebase/src/lib/firestore/firestore.ts @@ -14,6 +14,7 @@ export * from './model/number.js'; // Repository factory export { makeRepository } from './model/repository.js'; +export { MAX_FIELD_PATH_DEPTH, type UpdateData } from './model/update-path.js'; // Transaction and batch helpers export { withTransaction, withBatch } from './transaction.js'; diff --git a/packages/effect-firebase/src/lib/firestore/model/update-path.spec.ts b/packages/effect-firebase/src/lib/firestore/model/update-path.spec.ts index cc64d84..4bd561e 100644 --- a/packages/effect-firebase/src/lib/firestore/model/update-path.spec.ts +++ b/packages/effect-firebase/src/lib/firestore/model/update-path.spec.ts @@ -1,12 +1,33 @@ import { describe, expect, it } from 'vitest'; import { Option, Schema } from 'effect'; import { Model } from 'effect/unstable/schema'; -import { isFieldPath, resolveFieldPath } from './update-path.js'; +import { + MAX_FIELD_PATH_DEPTH, + isFieldPath, + resolveFieldPath, + type UpdateData, +} from './update-path.js'; import { OptionalDeletable } from './optional.js'; import * as FirestoreNumber from './number.js'; class Inner extends Schema.Class('Inner')({ x: Schema.Number }) {} +// Recursive map declared the usual way (interface + suspend)… +interface INode { + readonly label: string; + readonly next?: INode; +} +const INode: Schema.Codec = Schema.Struct({ + label: Schema.String, + next: Schema.optionalKey(Schema.suspend((): Schema.Codec => INode)), +}); +// …and as a type alias, which gets typed paths. +type ANode = { readonly label: string; readonly next?: ANode }; +const ANode: Schema.Codec = Schema.Struct({ + label: Schema.String, + next: Schema.optionalKey(Schema.suspend((): Schema.Codec => ANode)), +}); + class Doc extends Model.Class('Doc')({ id: Schema.String, plain: Schema.Struct({ a: Schema.Struct({ b: Schema.Boolean }) }), @@ -20,6 +41,8 @@ class Doc extends Model.Class('Doc')({ Schema.Number, ), scalar: Schema.String, + inode: INode, + anode: ANode, }) {} const root = Doc.update; @@ -63,6 +86,50 @@ describe('resolveFieldPath', () => { }); }); +describe('resolveFieldPath on recursive schemas', () => { + it('follows Schema.suspend for as many levels as the path names', () => { + expect(resolves('inode.label')).toBe(true); + expect(resolves('inode.next.label')).toBe(true); + expect(resolves('inode.next.next.next.label')).toBe(true); + expect(resolves('inode.next.nope')).toBe(false); + }); + + it('rejects paths deeper than MAX_FIELD_PATH_DEPTH', () => { + const atCap = [ + 'anode', + ...Array(MAX_FIELD_PATH_DEPTH - 1).fill('next'), + 'label', + ]; + expect(atCap.length - 1).toBe(MAX_FIELD_PATH_DEPTH); + expect(resolves(atCap.join('.'))).toBe(true); + expect(resolves([...atCap.slice(0, -1), 'next', 'label'].join('.'))).toBe( + false, + ); + }); + + it('types paths into recursive type aliases up to the cap, and none into interfaces', () => { + type U = UpdateData>; + const ok: U = { + 'anode.next.label': 'x', + 'anode.next.next.next.next.label': 'x', // depth 5 + inode: { label: 'whole value only' }, + }; + const tooDeep: U = { + // @ts-expect-error depth 6 exceeds MAX_FIELD_PATH_DEPTH + 'anode.next.next.next.next.next.label': 'x', + }; + const wrongLeaf: U = { + // @ts-expect-error label is a string + 'anode.next.label': 1, + }; + const intoInterface: U = { + // @ts-expect-error interfaces are leaves at the type level + 'inode.label': 'x', + }; + expect([ok, tooDeep, wrongLeaf, intoInterface]).toBeDefined(); + }); +}); + describe('isFieldPath', () => { it('is true only for dotted keys', () => { expect(isFieldPath('a.b')).toBe(true); diff --git a/packages/effect-firebase/src/lib/firestore/model/update-path.ts b/packages/effect-firebase/src/lib/firestore/model/update-path.ts index a3ffad4..ab563a1 100644 --- a/packages/effect-firebase/src/lib/firestore/model/update-path.ts +++ b/packages/effect-firebase/src/lib/firestore/model/update-path.ts @@ -1,4 +1,18 @@ -import { Option, Schema } from 'effect'; +import { Option, Schema, SchemaAST } from 'effect'; + +/** + * How many map levels a dotted field path may descend below a top-level + * field: `'a.b'` is depth 1, `'a.b.c'` depth 2. Applied identically at the + * type level ({@link UpdateData}) and at runtime ({@link resolveFieldPath}), + * so a path the type accepts always resolves and vice versa. + * + * The cap is what lets `UpdateData` be computed for recursive schemas + * (`Schema.suspend`) without TypeScript reporting a circular mapped type, + * and it bounds type-checking cost on wide models. Firestore itself allows + * 20 levels; anything deeper than this cap can still be written through + * `FirestoreService.update`. + */ +export const MAX_FIELD_PATH_DEPTH = 5; /** * The payload accepted by {@link Repository.update}: any subset of the @@ -11,12 +25,17 @@ import { Option, Schema } from 'effect'; * keys must be complete. * * Paths descend through plain object types (`Schema.Struct`, `Model.Struct`, - * `Schema.Class`, `Schema.Record`) and through `Option` (so an - * `OptionalDeletable` map is reachable even when currently absent). They stop - * at arrays, `DateTime`, `Timestamp`, `GeoPoint`, `Reference` and sentinel - * classes, which are written whole. + * `Schema.Class`, `Schema.Record`, `Schema.suspend`) and through `Option` + * (so an `OptionalDeletable` map is reachable even when currently absent), + * up to {@link MAX_FIELD_PATH_DEPTH} levels. They stop at arrays, + * `DateTime`, `Timestamp`, `GeoPoint`, `Reference` and sentinel classes, + * which are written whole. Recursive types declared as interfaces (the usual + * pattern for `Schema.suspend`) are leaves at the type level, since + * interfaces have no implicit index signature; declare them as type aliases + * to get typed paths into them. */ -export type UpdateData = Partial & NestedUpdateFields; +export type UpdateData = Partial & + NestedUpdateFields; type UnionToIntersection = ( U extends unknown ? (k: U) => void : never @@ -24,20 +43,25 @@ type UnionToIntersection = ( ? I : never; -type NestedUpdateFields = UnionToIntersection< - { - [K in keyof T & string]: ChildUpdateFields; - }[keyof T & string] ->; +/** Decrement table for the depth counter. */ +type Prev = [never, 0, 1, 2, 3, 4, 5, 6, 7, 8]; + +type NestedUpdateFields = [D] extends [0] + ? unknown + : UnionToIntersection< + { + [K in keyof T & string]: ChildUpdateFields; + }[keyof T & string] + >; // Interfaces and class instances (DateTime, Option, Timestamp, sentinels…) // lack an implicit index signature, so they fail `Record` // and are treated as leaves; struct types and records pass and are descended. -type ChildUpdateFields = +type ChildUpdateFields = V extends Option.Option - ? ChildUpdateFields + ? ChildUpdateFields : V extends Record - ? AddPrefixToKeys> + ? AddPrefixToKeys & NestedUpdateFields> : never; type AddPrefixToKeys = { @@ -45,76 +69,81 @@ type AddPrefixToKeys = { }; /** - * Structural view of the schema combinators a field path may pass through. - * These are the public properties `Schema.Struct`, `Schema.Class`, - * `Schema.optional`/`optionalKey`, `Schema.Union`, `Schema.decodeTo` (and - * `OptionFromUndefinedOr` etc.) and `Schema.Record` expose. Record - * segments are checked against the record's key schema. + * Follow a codec's encoding chain to the AST on its Encoded side. That is + * where the nested map lives for transformations such as + * `OptionFromUndefinedOr(Struct)`, whose Type side is an opaque `Option` + * declaration. Nodes reached this way are still full codecs, so leaves found + * below them encode correctly. This assumes the transformation keeps keys in + * place, which every combinator in this library does. */ -type Walkable = Schema.Top & { - readonly fields?: Record; - readonly schema?: Schema.Top; - readonly members?: ReadonlyArray; - readonly from?: Schema.Top; - readonly key?: Schema.Top; - readonly value?: Schema.Top; -}; +const encodedSide = (ast: SchemaAST.AST): SchemaAST.AST => + ast.encoding === undefined + ? ast + : encodedSide(ast.encoding[ast.encoding.length - 1].to); /** - * Find the schema for a single child key of `schema`, unwrapping the - * combinators a nested map may be declared through. - * - * Transformations (`decodeTo`) are followed on their `from` side: that is - * the codec that knows how to encode the nested leaf, whereas the `to` side - * of e.g. `OptionFromUndefinedOr` is the plain in-memory type. This assumes - * the transformation keeps keys in place, which every combinator in this - * library does. + * Find the AST for a single child key of `ast`, unwrapping the nodes a + * nested map may be declared through. */ -const child = (schema: Schema.Top, key: string): Option.Option => { - const s = schema as Walkable; - if (s.fields !== undefined) { - const field = s.fields[key]; - return field === undefined ? Option.none() : Option.some(field); - } - if (s.key !== undefined && s.value !== undefined) { - // A Record admits any key its key schema accepts; a segment the key - // schema rejects (e.g. a template literal or branded key) is not a - // field, so the caller rejects it like any undeclared key. - return Schema.is(s.key as Schema.Schema)(key) - ? Option.some(s.value) - : Option.none(); - } - if (s.from !== undefined) { - return child(s.from, key); - } - if (s.schema !== undefined) { - return child(s.schema, key); - } - if (s.members !== undefined) { - for (const member of s.members) { - const found = child(member, key); - if (Option.isSome(found)) return found; +const child = ( + ast: SchemaAST.AST, + key: string, +): Option.Option => { + const node = encodedSide(ast); + switch (node._tag) { + case 'Objects': { + const property = node.propertySignatures.find((p) => p.name === key); + if (property !== undefined) return Option.some(property.type); + // A record admits any key its key schema accepts; a segment the key + // schema rejects (e.g. a template literal or branded key) is not a + // field, so the caller rejects it like any undeclared key. + const index = node.indexSignatures.find((i) => + Schema.is(Schema.make>(i.parameter))(key), + ); + return index === undefined ? Option.none() : Option.some(index.type); + } + case 'Union': { + for (const member of node.types) { + const found = child(member, key); + if (Option.isSome(found)) return found; + } + return Option.none(); } + case 'Suspend': + return child(node.thunk(), key); + case 'Declaration': + // `Schema.Class` is a declaration over the struct of its fields. Other + // declarations (DateTime, Timestamp, sentinels…) have no children or + // were unwrapped to their encoded side above. + return node.typeParameters.length === 1 + ? child(node.typeParameters[0], key) + : Option.none(); + default: + return Option.none(); } - return Option.none(); }; /** - * Resolve a Firestore field path (`'a.b.c'`) against a struct schema to the - * schema of the leaf it names. `None` when any segment is not a declared - * field, so the caller can reject the key instead of dropping it. + * Resolve a Firestore field path (`'a.b.c'`) against a schema to the schema + * of the leaf it names. `None` when any segment is not a declared field or + * the path exceeds {@link MAX_FIELD_PATH_DEPTH}, so the caller can reject + * the key instead of dropping it. */ export const resolveFieldPath = ( root: Schema.Top, path: string, -): Option.Option => - path - .split('.') - .reduce>( +): Option.Option => { + const segments = path.split('.'); + if (segments.length - 1 > MAX_FIELD_PATH_DEPTH) return Option.none(); + return Option.map( + segments.reduce>( (current, segment) => - Option.flatMap(current, (schema) => child(schema, segment)), - Option.some(root), - ); + Option.flatMap(current, (ast) => child(ast, segment)), + Option.some(root.ast), + ), + (ast) => Schema.make(ast), + ); +}; /** Whether a payload key is a Firestore dotted field path. */ export const isFieldPath = (key: string): boolean => key.includes('.'); From 6c7e916708354532a089f814b336ff84129eb981 Mon Sep 17 00:00:00 2001 From: Frederik Wallner Date: Thu, 10 Sep 2026 08:50:09 +0200 Subject: [PATCH 4/4] feat(firestore): add { merge: true } to Repository.update Flattens a deep-partial payload into dotted field paths before the write, so nested objects merge into the stored map instead of replacing it. Leaves (arrays, class instances, sentinels, Option.none) are written whole; Option.some is merged into; empty objects contribute nothing. Co-Authored-By: Claude Fable 5.1 --- README.md | 6 + packages/effect-firebase/AGENTS.md | 14 +- packages/effect-firebase/MIGRATION.md | 3 +- .../src/lib/firestore/firestore.ts | 6 +- .../src/lib/firestore/model/datetime.spec.ts | 8 +- .../lib/firestore/model/repository.spec.ts | 157 ++++++++++++++++++ .../src/lib/firestore/model/repository.ts | 83 +++++---- .../lib/firestore/model/update-path.spec.ts | 37 +++++ .../src/lib/firestore/model/update-path.ts | 51 ++++++ 9 files changed, 326 insertions(+), 39 deletions(-) diff --git a/README.md b/README.md index 15ca029..97aa8e6 100644 --- a/README.md +++ b/README.md @@ -142,8 +142,14 @@ replaces the entire map, as in the Firestore SDKs. yield * repo.update(postId, { 'metaData.deleted': true }); // touches only metaData.deleted yield * repo.update(postId, { metaData: { deleted: true, tags: [] } }); // replaces metaData yield * repo.update(postId, { 'stats.likes': Firestore.increment(1) }); // nested sentinel +yield * repo.update(postId, { metaData: { deleted: true } }, { merge: true }); // flattened to metaData.deleted ``` +With `{ merge: true }` the payload is a deep partial: nested objects are +flattened into dotted paths before the write, so absent siblings are left +untouched. Arrays, `DateTime`, sentinels and other non-plain values are +written whole. + Keys the model does not declare fail with a `SchemaError` naming the key; an empty payload fails with `FirestoreError` code `invalid-argument`. diff --git a/packages/effect-firebase/AGENTS.md b/packages/effect-firebase/AGENTS.md index d7c7e9c..37a4a1d 100644 --- a/packages/effect-firebase/AGENTS.md +++ b/packages/effect-firebase/AGENTS.md @@ -170,8 +170,15 @@ whole-field key replaces the whole map (Firestore semantics). ```ts yield * repo.update(id, { 'metaData.deleted': true }); // only metaData.deleted yield * repo.update(id, { metaData: { deleted: true, tags: [] } }); // replaces metaData +yield * repo.update(id, { metaData: { deleted: true } }, { merge: true }); // only metaData.deleted ``` +`{ merge: true }` takes a deep partial and flattens nested objects into dotted +paths before the write, like Firestore's `set(..., { merge: true })`. Arrays, +class instances (`DateTime`, sentinels…) and `Option.none()` are leaves and +are written whole; `Option.some({ ... })` is merged into. An empty object +contributes nothing (writing an empty map would clobber the existing one). + Paths are typed (`UpdateData`) and descend through `Schema.Struct`, `Model.Struct`, `Schema.Class`, `Schema.Record` (keys checked against the key schema), `Schema.suspend`, `Schema.optional` and `OptionalDeletable`; arrays, @@ -436,9 +443,10 @@ root: https://github.com/fwal/effect-firebase/blob/main/REACT.md. variants that helper allows. 9. Style used throughout the library: explicit lambdas (`Effect.map((x) => f(x))`), no point-free `Effect.map(f)`. -10. Nested updates use dotted keys (`'a.b': v`), not nested partial objects: - `{ a: { b: v } }` replaces the whole `a` map. Undeclared keys fail with - `SchemaError`; `update` never silently drops them. +10. Nested updates use dotted keys (`'a.b': v`) or `{ merge: true }` with a + nested partial; a plain `{ a: { b: v } }` replaces the whole `a` map. + Undeclared keys fail with `SchemaError`; `update` never silently drops + them. ## Where to look diff --git a/packages/effect-firebase/MIGRATION.md b/packages/effect-firebase/MIGRATION.md index 798b219..ca2514f 100644 --- a/packages/effect-firebase/MIGRATION.md +++ b/packages/effect-firebase/MIGRATION.md @@ -269,7 +269,8 @@ typed against the model (`'metaData.deleted': true`) and encodes each leaf through its own field schema, so nested sentinels work. Replace hand-rolled `FirestoreService.update(path, { 'a.b': v })` calls with `repo.update(id, { 'a.b': v })`. Note that a whole-field key (`metaData: { ... }`) still -replaces the entire map. +replaces the entire map; pass `{ merge: true }` as a third argument to have a +nested partial flattened into dotted paths instead. ### 8. `FirestoreService` shape changes (custom layers and test doubles) diff --git a/packages/effect-firebase/src/lib/firestore/firestore.ts b/packages/effect-firebase/src/lib/firestore/firestore.ts index c8f4d9e..ea47598 100644 --- a/packages/effect-firebase/src/lib/firestore/firestore.ts +++ b/packages/effect-firebase/src/lib/firestore/firestore.ts @@ -14,7 +14,11 @@ export * from './model/number.js'; // Repository factory export { makeRepository } from './model/repository.js'; -export { MAX_FIELD_PATH_DEPTH, type UpdateData } from './model/update-path.js'; +export { + MAX_FIELD_PATH_DEPTH, + type MergeUpdateData, + type UpdateData, +} from './model/update-path.js'; // Transaction and batch helpers export { withTransaction, withBatch } from './transaction.js'; diff --git a/packages/effect-firebase/src/lib/firestore/model/datetime.spec.ts b/packages/effect-firebase/src/lib/firestore/model/datetime.spec.ts index 67c4498..fb16083 100644 --- a/packages/effect-firebase/src/lib/firestore/model/datetime.spec.ts +++ b/packages/effect-firebase/src/lib/firestore/model/datetime.spec.ts @@ -196,9 +196,7 @@ describe('Model.WithServerTimestamp', () => { const encode = Schema.encodeSync(TestModel.insert); const result = encode({ lastSeenAt: serverTimestamp() }); - expect(result.lastSeenAt).toBeInstanceOf( - FirestoreSchema.ServerTimestamp, - ); + expect(result.lastSeenAt).toBeInstanceOf(FirestoreSchema.ServerTimestamp); }); }); @@ -219,9 +217,7 @@ describe('Model.WithServerTimestamp', () => { const encode = Schema.encodeSync(TestModel.update); const result = encode({ lastSeenAt: serverTimestamp() }); - expect(result.lastSeenAt).toBeInstanceOf( - FirestoreSchema.ServerTimestamp, - ); + expect(result.lastSeenAt).toBeInstanceOf(FirestoreSchema.ServerTimestamp); }); }); diff --git a/packages/effect-firebase/src/lib/firestore/model/repository.spec.ts b/packages/effect-firebase/src/lib/firestore/model/repository.spec.ts index 533906b..881f5a7 100644 --- a/packages/effect-firebase/src/lib/firestore/model/repository.spec.ts +++ b/packages/effect-firebase/src/lib/firestore/model/repository.spec.ts @@ -1,5 +1,6 @@ import { describe, expect, it, vi } from 'vitest'; import { DateTime, Effect, Layer, Option, Schema } from 'effect'; +import { delete as deleteField } from '../fields/delete.js'; import { Model } from 'effect/unstable/schema'; import { makeRepository } from './repository.js'; import * as FirestoreModel from './datetime.js'; @@ -433,6 +434,162 @@ describe('Repository', () => { expect(write).toBeDefined(); }); }); + + describe('merge', () => { + const payloadOf = (mock: ReturnType) => + (mock.mock.calls[0] as unknown as [string, Record])[1]; + + it('flattens a nested partial into dotted paths', async () => { + const updateMock = vi.fn(() => Effect.succeed(undefined)); + const repo = await Effect.runPromise( + makeNestedRepo({ update: updateMock }), + ); + await Effect.runPromise( + repo.update( + PostId.make('post-1'), + { title: 'Updated', metaData: { deleted: true } }, + { merge: true }, + ), + ); + + expect(payloadOf(updateMock)).toEqual({ + title: 'Updated', + 'metaData.deleted': true, + }); + }); + + it('replaces the whole map without merge', async () => { + const updateMock = vi.fn(() => Effect.succeed(undefined)); + const repo = await Effect.runPromise( + makeNestedRepo({ update: updateMock }), + ); + await Effect.runPromise( + repo.update(PostId.make('post-1'), { + metaData: { deleted: true, tags: [] }, + }), + ); + + expect(payloadOf(updateMock)).toEqual({ + metaData: { deleted: true, tags: [] }, + }); + }); + + it('keeps arrays and sentinels whole and encodes nested leaves', async () => { + const updateMock = vi.fn(() => Effect.succeed(undefined)); + const repo = await Effect.runPromise( + makeNestedRepo({ update: updateMock }), + ); + await Effect.runPromise( + repo.update( + PostId.make('post-1'), + { + metaData: { tags: ['a', 'b'] }, + stats: { likes: increment(1) }, + profile: Option.some({ lastSeenAt: DateTime.makeUnsafe(1_000) }), + counters: { visits: 3 }, + }, + { merge: true }, + ), + ); + + const payload = payloadOf(updateMock); + expect(Object.keys(payload).sort()).toEqual([ + 'counters.visits', + 'metaData.tags', + 'profile.lastSeenAt', + 'stats.likes', + ]); + expect(payload['metaData.tags']).toEqual(['a', 'b']); + expect(payload['stats.likes']).toEqual(increment(1)); + expect(payload['profile.lastSeenAt']).toBeInstanceOf(Timestamp); + expect(payload['counters.visits']).toBe(3); + }); + + it('treats Option.none() and Firestore.delete() as leaves', async () => { + const updateMock = vi.fn(() => Effect.succeed(undefined)); + const repo = await Effect.runPromise( + makeNestedRepo({ update: updateMock }), + ); + await Effect.runPromise( + repo.update( + PostId.make('post-1'), + { profile: Option.none() }, + { merge: true }, + ), + ); + await Effect.runPromise( + repo.update( + PostId.make('post-1'), + { profile: Option.some(deleteField()) }, + { merge: true }, + ), + ); + + expect(updateMock.mock.calls).toHaveLength(2); + expect(payloadOf(updateMock)).toEqual({ profile: undefined }); + expect( + ( + updateMock.mock.calls[1] as unknown as [ + string, + Record, + ] + )[1], + ).toEqual({ profile: deleteField() }); + }); + + it('drops empty objects, failing invalid-argument if nothing is left', async () => { + const updateMock = vi.fn(() => Effect.succeed(undefined)); + const repo = await Effect.runPromise( + makeNestedRepo({ update: updateMock }), + ); + const error = await failureOf( + repo.update(PostId.make('post-1'), { metaData: {} }, { merge: true }), + ); + + expect(error).toMatchObject({ + _tag: 'FirestoreError', + code: 'invalid-argument', + }); + expect(updateMock).not.toHaveBeenCalled(); + }); + + it('still rejects undeclared nested keys, naming the flattened path', async () => { + const updateMock = vi.fn(() => Effect.succeed(undefined)); + const repo = await Effect.runPromise( + makeNestedRepo({ update: updateMock }), + ); + const error = await failureOf( + repo.update( + PostId.make('post-1'), + // @ts-expect-error nope is not a field of metaData + { metaData: { nope: true } }, + { merge: true }, + ), + ); + + expect(error._tag).toBe('SchemaError'); + expect(String(error)).toContain('metaData.nope'); + expect(updateMock).not.toHaveBeenCalled(); + }); + + it('requires complete nested values without merge', () => { + const repo = Effect.runSync(makeNestedRepo({})); + const write = repo.update(PostId.make('post-1'), { + // @ts-expect-error tags is required when replacing the map + metaData: { deleted: true }, + }); + expect(write).toBeDefined(); + }); + + it('does not treat a scalar field as a map', () => { + const repo = Effect.runSync(makeNestedRepo({})); + const write = repo.update(PostId.make('post-1'), { + // @ts-expect-error title is a string, not a map + 'title.length': 1, + }); + expect(write).toBeDefined(); + }); + }); }); describe('delete', () => { diff --git a/packages/effect-firebase/src/lib/firestore/model/repository.ts b/packages/effect-firebase/src/lib/firestore/model/repository.ts index ad724c4..2fd8509 100644 --- a/packages/effect-firebase/src/lib/firestore/model/repository.ts +++ b/packages/effect-firebase/src/lib/firestore/model/repository.ts @@ -7,12 +7,23 @@ import { FirestoreError } from '../errors.js'; import * as Fetch from './fetch.js'; import type { QueryConstraint } from '../query/constraints.js'; import { + flattenForMerge, isFieldPath, resolveFieldPath, + type MergeUpdateData, type UpdateData, } from './update-path.js'; -export type { UpdateData } from './update-path.js'; +export type { MergeUpdateData, UpdateData } from './update-path.js'; + +export type UpdateOptions = { + /** + * Flatten nested objects into dotted field paths before writing, so a + * nested partial merges into the stored map instead of replacing it. + * @default false + */ + readonly merge?: boolean; +}; export type ModelError = FirestoreError | UnknownError | NoSuchElementError | Schema.SchemaError; @@ -135,24 +146,45 @@ export type Repository< * `data` is any subset of the `update` variant's fields, plus Firestore * dot-separated paths into nested maps (`'metaData.deleted': true`) * which update just that nested field. A whole-field key replaces the - * whole value, so `metaData: { ... }` overwrites the entire map. Keys - * the model does not declare fail with a `SchemaError` naming the key; - * an empty payload fails with `FirestoreError` code `invalid-argument`. + * whole value, so `metaData: { ... }` overwrites the entire map. With + * `{ merge: true }`, `data` is instead a deep partial: nested objects are + * flattened into dotted paths, so `metaData: { deleted: true }` updates + * only `metaData.deleted`. + * + * Keys the model does not declare fail with a `SchemaError` naming the + * key; an empty payload fails with `FirestoreError` code + * `invalid-argument`. * * @param id - The ID of the document model to update. - * @param data - The fields and field paths to update. See {@link UpdateData}. + * @param data - The fields and field paths to update. See + * {@link UpdateData} and {@link MergeUpdateData}. + * @param options - See {@link UpdateOptions}. * @returns A unit value. */ - readonly update: ( - id: IdSchema['Type'], - data: UpdateData>, - ) => Effect.Effect< - void, - ModelError, - | S['DecodingServices'] - | S['EncodingServices'] - | S['update']['EncodingServices'] - >; + readonly update: { + ( + id: IdSchema['Type'], + data: UpdateData>, + options?: { readonly merge?: false }, + ): Effect.Effect< + void, + ModelError, + | S['DecodingServices'] + | S['EncodingServices'] + | S['update']['EncodingServices'] + >; + ( + id: IdSchema['Type'], + data: MergeUpdateData>, + options: { readonly merge: true }, + ): Effect.Effect< + void, + ModelError, + | S['DecodingServices'] + | S['EncodingServices'] + | S['update']['EncodingServices'] + >; + }; /** * Get a document model by ID. @@ -456,14 +488,15 @@ export const makeRepository = < const update = ( id: IdSchema['Type'], - data: UpdateData>, + data: Record, + updateOptions?: UpdateOptions, ) => Effect.gen(function* () { + const entries = + updateOptions?.merge === true ? flattenForMerge(data) : data; const fields: Record = { [options.idField]: id }; const paths: Array = []; - for (const [key, value] of Object.entries( - data as Record, - )) { + for (const [key, value] of Object.entries(entries)) { const encoder = isFieldPath(key) ? leafEncoder(key) : Option.none(); if (Option.isSome(encoder)) { paths.push([key, value, encoder.value]); @@ -494,15 +527,9 @@ export const makeRepository = < ); }).pipe( Effect.withSpan(`${options.spanPrefix}.update`, { - attributes: { id, data }, + attributes: { id, data, merge: updateOptions?.merge ?? false }, }), - ) as Effect.Effect< - void, - ModelError, - | S['DecodingServices'] - | S['EncodingServices'] - | S['update']['EncodingServices'] - >; + ); const getByIdSchema = Fetch.findOneOption({ Request: idSchema, @@ -644,7 +671,7 @@ export const makeRepository = < return { add, set, - update, + update: update as Repository['update'], getById, getByIdStream, delete: deleteById, diff --git a/packages/effect-firebase/src/lib/firestore/model/update-path.spec.ts b/packages/effect-firebase/src/lib/firestore/model/update-path.spec.ts index 4bd561e..4981fa5 100644 --- a/packages/effect-firebase/src/lib/firestore/model/update-path.spec.ts +++ b/packages/effect-firebase/src/lib/firestore/model/update-path.spec.ts @@ -3,6 +3,7 @@ import { Option, Schema } from 'effect'; import { Model } from 'effect/unstable/schema'; import { MAX_FIELD_PATH_DEPTH, + flattenForMerge, isFieldPath, resolveFieldPath, type UpdateData, @@ -130,6 +131,42 @@ describe('resolveFieldPath on recursive schemas', () => { }); }); +describe('flattenForMerge', () => { + it('flattens plain objects and the contents of Option.some', () => { + expect( + flattenForMerge({ + a: { b: { c: 1 }, d: 2 }, + e: Option.some({ f: 3 }), + 'g.h': 4, + }), + ).toEqual({ 'a.b.c': 1, 'a.d': 2, 'e.f': 3, 'g.h': 4 }); + }); + + it('keeps arrays, class instances, Option.none and Option.some(leaf) whole', () => { + const date = new Date(0); + const some = Option.some(1); + expect( + flattenForMerge({ + arr: [{ x: 1 }], + date, + none: Option.none(), + some, + nested: { some }, + }), + ).toEqual({ + arr: [{ x: 1 }], + date, + none: Option.none(), + some, + 'nested.some': some, + }); + }); + + it('contributes nothing for empty objects', () => { + expect(flattenForMerge({ a: {}, b: { c: {} } })).toEqual({}); + }); +}); + describe('isFieldPath', () => { it('is true only for dotted keys', () => { expect(isFieldPath('a.b')).toBe(true); diff --git a/packages/effect-firebase/src/lib/firestore/model/update-path.ts b/packages/effect-firebase/src/lib/firestore/model/update-path.ts index ab563a1..7efe017 100644 --- a/packages/effect-firebase/src/lib/firestore/model/update-path.ts +++ b/packages/effect-firebase/src/lib/firestore/model/update-path.ts @@ -145,5 +145,56 @@ export const resolveFieldPath = ( ); }; +/** + * The payload accepted by {@link Repository.update} with `{ merge: true }`: + * a deep partial of the model's `update` fields. Nested plain objects are + * flattened into dotted field paths before the write, so every present leaf + * is written and every absent sibling is left untouched. Leaves are the same + * as for {@link UpdateData}: arrays, class instances and sentinels are + * written whole. `Option.some(x)` is merged into; `Option.none()` is a leaf. + */ +export type MergeUpdateData = { + readonly [K in keyof T]?: MergeValue; +}; + +type MergeValue = + V extends Option.Option + ? Option.Option> + : V extends Record + ? MergeUpdateData + : V; + +const isPlainObject = (value: unknown): value is Record => { + if (typeof value !== 'object' || value === null) return false; + const proto = Object.getPrototypeOf(value); + return proto === Object.prototype || proto === null; +}; + +/** + * Flatten a merge payload into dotted field paths. Plain objects (and the + * contents of `Option.some`) are descended; everything else is a leaf and is + * kept as-is, `Option.some(leaf)` included, so the leaf encoder still sees the + * `Option`. An empty object contributes no paths: writing an empty map would + * clobber the existing one, which is the opposite of a merge. + */ +export const flattenForMerge = ( + data: Record, +): Record => { + const out: Record = {}; + const visit = (value: unknown, path: string): void => { + const inner = + Option.isOption(value) && Option.isSome(value) ? value.value : value; + if (isPlainObject(inner)) { + for (const [key, child] of Object.entries(inner)) { + visit(child, `${path}.${key}`); + } + return; + } + out[path] = value; + }; + for (const [key, value] of Object.entries(data)) visit(value, key); + return out; +}; + /** Whether a payload key is a Firestore dotted field path. */ export const isFieldPath = (key: string): boolean => key.includes('.');