Repository navigation
Firestore DML fixes - #9565
Merged
Merged
Firestore DML fixes#9565
Conversation
…line API conventions
Follow the existing Pipeline stage convention of `stage(requiredArg)` /
variadic convenience overloads plus a single `stage(options)` overload,
where the options object carries the required params.
- literals (PipelineSource and Pipeline): remove the
`(documents, options?)` overload; `documents` is now required in
LiteralsStageOptions.
- update: remove the `Record<string, Expression> | Map` overload (not in
the API proposal, no precedent, and indistinguishable at runtime from
an options object); add `update(options: UpdateStageOptions)` with
`transformedFields`.
- delete: add `delete(options: DeleteStageOptions)` so rawOptions can be
provided.
- upsert: remove the `(additionalFields[], options?)` overload; use
`upsert({additionalFields, ...})` instead.
…verloads Follow the precedent set by database() for stages with no required params, consistent with delete().
…tageOptions transforms was an earlier name for additionalFields that was never released in the Node or Web SDKs, so the deprecated alias is unnecessary API surface.
…neSource only literals is a source stage (LiteralsSource). It was only ever used and tested as the first stage via PipelineSource, and the Web SDK exposes it only on PipelineSource.
…ents FieldValue sentinels (serverTimestamp(), increment(), arrayUnion(), arrayRemove(), delete(), ...) cannot be represented in an encoded pipeline. Previously they were silently dropped from literal documents (and a document containing only sentinels failed with an INTERNAL serialization error). Literal documents are now validated recursively and sentinels throw the same error as the Web SDK: Function literals() called with invalid data. <method>() can only be used with update() and set() (found in field <path>) Nested values with user data are now also validated, not just top-level fields.
hasUserData() dereferences its argument, so a null value in a literal document crashed _validateUserData with a TypeError.
…ents
The backend only evaluates expressions in the top-level fields of a
literal document, so nested expressions (e.g. {a: {b: add(...)}}) failed
with "Value type is not supported: FUNCTION_VALUE". Encode each document
as a top-level mapValue whose fields are converted with
valueToDefaultExpr, so nested objects and arrays become map() and array()
expressions, matching rawStage() and expression arguments.
Undefined values are now rejected unless ignoreUndefinedProperties is
enabled, preserving the previous serializer behavior.
…ic option Describe how collection and documentIdExpression determine the target document, that a string documentIdExpression is a field name (use constant() for a fixed ID), that upsert requires collection when documentIdExpression is set, and what atomic does. Behavior verified against nightly; adds system tests for the field-name semantics and the upsert collection requirement.
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors several Firestore pipeline stages—including literals, delete, update, insert, and upsert—to accept options objects, updating their signatures, type definitions, and tests accordingly. The review feedback focuses on improving robustness and decoupling: it suggests moving the selectablesToMap conversion into the UpdateStage constructor to keep the Pipeline class clean, adding safe default fallbacks to prevent runtime crashes when options are undefined, and strengthening validation in the literals stage to ensure each document is a plain object.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #9118. This aligns the DML and
literalspipeline APIs with the existing Pipeline API conventions, fixes several encoding and validation bugs in theliteralsstage, and documents how insert/upsert choose their target documents and whatatomicdoes.Fixes API problems that the proposal review missed, before the API ships. It also fixes
literalsinputs that the SDK previously dropped silently, crashed on, or encoded in a way the backend rejected.Changes
API shape
literals(documents)+literals(options), withdocumentsrequired inLiteralsStageOptions. Why: other stages take their required argument directly and also offer an options overload that carries it. The backend rejects aliteralsstage with no documents, sodocumentsshouldn't be optional.update: removed theRecord<string, Expression> | Mapoverload and addedupdate(options: UpdateStageOptions)with an optionaltransformedFields. Why: no other stage accepts a map of fields; they takeAliasedExpressions. Every stage should also have an options overload. This removed overload was not in the API proposal or tests.delete()+delete(options: DeleteStageOptions). Why: every stage should have an options overload, e.g. forrawOptions. This follows thedatabase()precedent for stages with no required parameters.insert()+insert(options), replacing a single optional-options signature. Why: this matches thedatabase()precedent and the other DML stages.upsert(),upsert(field, ...fields)andupsert(options); removedupsert(fields[], options?). Why: this matches the variadic convention used byselect(),addFields(), etc. The removed overload could pass fields in two places, and the code had to merge them.transformsoption fromUpsertStageOptions. Why: it was a pre-release name foradditionalFields, added and renamed within feat(firestore): add DML stages, literals source, and atomic execution option to Node SDK pipelines #9118. It was never released in Node or Web, so there's nothing to deprecate.literalsis only onPipelineSourcenow, not onPipeline. Why:literalsis a source stage. It was never tested in a non-first position, and the Web SDK only exposes it onPipelineSource.rawOptions. Why: they were silently dropping them, unlike every other stage.literalsencoding and validationserverTimestamp(),increment(),arrayUnion(),delete(), ...) in literal documents now throwFunction literals() called with invalid data. FieldValue.serverTimestamp() can only be used with update() and set() (found in field <path>). Why: a pipeline has no way to represent sentinels. Before this change they were silently dropped, nested data was lost, and a document containing only a sentinel failed with an opaque13 INTERNALserialization error. The message matches the Web SDK. Validation is now recursive, which also covers nested user data.currentTimestamp()is the server-side alternative toserverTimestamp().map()/array()expressions. Why: the backend only evaluates expressions in the top-level fields of a literal document, so{a: {b: add(...)}}and{arr: [add(...)]}failed withValue type is not supported: FUNCTION_VALUE. Each document is still sent as a top-levelmapValue. Its fields are converted withvalueToDefaultExpr, the same conversion used for expression arguments andrawStage().undefinedin literal documents is rejected unlessignoreUndefinedPropertiesis enabled. Why: this keeps the previous serializer behaviour, which the new encoding would otherwise have bypassed by silently droppingundefined.