Repository navigation
Conversation
The backend evaluates expressions only in the top-level fields of each literal document. Previously, an Expression nested inside a map value was sent as a raw function_value inside a map_value (rejected with INVALID_ARGUMENT: Value type is not supported: FUNCTION_VALUE), and an Expression inside a list failed client-side with a confusing CustomClassMapper serialization error. A top-level map or list value that contains an Expression at any depth is now converted into a map(...) / array(...) expression, so nested expressions are evaluated. Values that contain no expressions keep their existing encoding (UserDataReader.parseQueryValue), so constant-only data, BSON types and POJOs are unaffected; leaves inside wrapped containers are also parsed with parseQueryValue, so FieldValue sentinels are still rejected. Proposal P-3.
…t-side - PipelineSource.literals() now requires at least one document and fails fast with "Function literals() requires at least one document." The backend previously rejected an empty literals stage with INVALID_ARGUMENT. - Invalid literal values (FieldValue sentinels, unsupported types) are now reported with literals() context and the field path, for example: "Function literals() called with invalid data. FieldValue.serverTimestamp() can only be used with set() and update() (found in field a.b)". Values are still rejected before any RPC is sent. Proposal P-4 (a, b).
- Pipeline.upsert(collectionPath, documentIdExpression, additionalFields) now takes Array<out Selectable>, so Kotlin callers can pass an existing Array<AliasedExpression> (the invariant Array<Selectable> rejected it). - Removed the redundant upsert(collectionPath, documentIdExpression = null, additionalFields: List<Selectable>) overload. Its defaulted middle parameter before a required one made it awkward to call, and it duplicated the @jvmoverloads array overload. In-place upsert keeps both the vararg and List forms. Both overloads are unreleased. Proposal P-8 (minor).
…ecution - Added KDoc for Pipeline.delete(), update(), insert(), ExecuteOptions.withAtomic(), PipelineSource.literals(List), documents(List<String>) (documentsByPath) and Expression.as(). - Documented the literal-values contract on both literals() overloads (at least one document, expressions evaluated including nested ones, FieldValue sentinels not supported). - Fixed the insert/upsert KDoc: a null documentIdExpression reuses the input document's ID (an ID is generated only when the input document has none, for example from literals()); it does not always auto-generate an ID. Upsert replaces the target document; stored fields are not merged. - Restored the KDoc of documents(vararg DocumentReference), which had been displaced onto the new documents(List<DocumentReference>) overload, and gave the List overload its own KDoc. - Added CHANGELOG.md entries (Unreleased) for the DML stages, literals(), withAtomic(), the documents() List overloads, the relaxed removeFields() validation, and the execution-time NullPointerException fix. Proposal P-16 (and the P-10 target rules for the current API).
Expression.as(alias) was added on this branch as an exact duplicate of Expression.alias(alias). It is not on main and has never been released, so it is removed rather than shipped as a redundant alias. Tests now use alias(). (proposal P-9; pending team decision)
Make `collectionPath` on `Pipeline.insert` nullable with a default of null and add `@JvmOverloads`, so callers can insert without naming a target collection: - only `collectionPath`: documents are written to that collection, keeping the input document ID. - only `documentIdExpression`: documents are written to the parent collection of the input document, with the evaluated ID. - both: documents are written to `collectionPath` with the evaluated ID. - neither: documents are written back to their input path, which fails with ALREADY_EXISTS for documents that already exist. Adds unit tests for the new argument combinations and integration tests for the parent-collection and ALREADY_EXISTS behaviors. Regenerates api.txt. (proposal P-10; pending team decision)
When the server response omits `execution_time`, for example for pipelines that write to the database, `Pipeline.Snapshot.executionTime` falls back to the device time at which the result was received. Document this in the `executionTime` KDoc and in the CHANGELOG entry for the execution-time NullPointerException fix. No API change. (proposal P-15; pending team decision)
Replace the separate pipeline DML, literals, atomic execution, documents() overload, removeFields() and execution time entries with a single feature entry, since these changes ship together.
dlarocque
added this pull request to stack #8688
October 8, 2026 19:07
Contributor
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
Contributor
📝 PRs merging into main branchOur main branch should always be in a releasable state. If you are working on a larger change, or if you don't want this change to see the light of the day just yet, consider using a feature branch first, and only merge into the main branch when the code complete and ready to be released. |
…e fallback" This reverts the KDoc part of commit 5c0b69e. The device-clock fallback for `Pipeline.Snapshot.executionTime` stays in the code, but it should not be documented as a contract: it will be tracked as a bug and fixed later without a breaking change to the released Pipeline API. The CHANGELOG part of 5c0b69e was already superseded by the consolidated pipeline DML entry, so CHANGELOG.md is unchanged.
Remove `Pipeline.upsert(List<Selectable>)`. No stage method in the released Android Pipeline API (origin/main api.txt) takes a List of stage inputs: `select`, `addFields`, `removeFields`, `distinct`, `aggregate`, `sort` and `define` all take a first argument plus a vararg, and List parameters only appear for values, such as `array()` or `equalAny()`. `upsert(vararg Selectable)` remains, and callers holding a list can spread it. The overload was never released. Removes its unit test and the `testUpsertExistingDocWithList` integration test, and regenerates api.txt.
Remove `PipelineSource.literals(List<Map<String, Any?>>)` and keep only `literals(vararg Map<String, Any?>)`. No source or stage method in the released Android Pipeline API (origin/main api.txt) takes a List of inputs. The closest source, `PipelineSource.documents()`, only has vararg forms, and List parameters only appear for values, such as `array()` or `equalAny()`. Callers holding a list can spread it. The overload was never released. Moves the at-least-one-document check into the vararg overload, updates the unit test and regenerates api.txt.
Remove `PipelineSource.documents(List<DocumentReference>)` and `documents(List<String>)` (`documentsByPath` for Java callers), so that `documents()` again has only its released vararg forms. No source or stage method in the released Android Pipeline API (origin/main api.txt) takes a List of inputs. The commit that added these overloads (85e6219) gives no reason for them, and callers holding a list can spread it. The overloads were never released. The pipeline DML CHANGELOG entry no longer mentions them, and api.txt is regenerated.
…Android conventions
Rework the unreleased `Pipeline.insert` and `Pipeline.upsert` overload
sets to follow the conventions of the released Android Pipeline API
(origin/main api.txt):
| Released convention | Evidence on origin/main | Change |
|---|---|---|
| Optional parameters are explicit overloads; no Kotlin default arguments or `@JvmOverloads` | 0 default arguments in api.txt; `unnest(Selectable)` + `unnest(Selectable, UnnestOptions)`, `aggregate(...)` + `aggregate(..., AggregateOptions)`, three `findNearest` overloads | `@JvmOverloads insert(String? = null, Expression? = null)` and `@JvmOverloads upsert(String, Expression? = null, Array<out Selectable> = emptyArray())` become explicit overloads |
| No nullable parameters on stage methods | no `?` parameter on any `Pipeline` stage method | the explicit overloads take non-null `String`, `CollectionReference` and `Expression` |
| A collection is accepted as a String path or a `CollectionReference` | `PipelineSource.collection(String)` + `collection(CollectionReference)`; `documents(String...)` + `documents(DocumentReference...)` | add `insert(CollectionReference)`, `insert(CollectionReference, Expression)`, `upsert(CollectionReference)` and `upsert(CollectionReference, Expression)`; a reference from another Firestore instance throws `IllegalArgumentException` |
| Multiple stage inputs are passed as vararg, never as an array parameter | `select`, `addFields`, `removeFields`, `sort`, `aggregate`, `define` | remove the `Array<out Selectable>` parameter of the target-collection `upsert`; call `addFields(...)` before `upsert(collection, ...)` instead |
| Vararg-only signatures where zero inputs are valid | `PipelineSource.documents(vararg)` | `update(vararg Selectable)` and `upsert(vararg Selectable)` are unchanged, because zero fields is a valid update or upsert |
Unchanged: `delete()`, `update(vararg Selectable)`,
`upsert(vararg Selectable)`, `PipelineSource.literals(vararg Map)` and
`ExecuteOptions.withAtomic(Boolean)`, which already match the
`withIndexMode(IndexMode)` style.
Resulting insert overloads: `insert()`, `insert(String)`,
`insert(CollectionReference)`, `insert(Expression)`,
`insert(String, Expression)`, `insert(CollectionReference, Expression)`.
Resulting target-collection upsert overloads: `upsert(String)`,
`upsert(CollectionReference)`, `upsert(String, Expression)`,
`upsert(CollectionReference, Expression)`.
There is no `upsert(Expression)`. Because `Field` is both an
`Expression` and a `Selectable`, `upsert(field("x"))` would silently
resolve to the document ID overload instead of
`upsert(vararg Selectable)`.
All of these APIs are unreleased. Adds unit tests for the
CollectionReference overloads and for rejecting a reference from another
instance. Adds the `testInsertIntoCollectionReference` integration test,
and moves the upsert integration tests from the removed array parameter
to `addFields(...)`. Regenerates api.txt.
Remove two client-side checks that commit 27c9367 added to `PipelineSource.literals()`, because the backend already enforces them: - At least one document. An empty `literals()` is now sent as is, and the backend rejects it with INVALID_ARGUMENT ("The 'literals(...)' must have at least one document"). - Non-empty field names. Literal maps are no longer validated with the write-path field name rules, so an empty key is sent as is. A nightly probe confirmed that the backend rejects it with INVALID_ARGUMENT, at the top level ("property path cannot contain empty segments.") and in a nested map ("Property names cannot be empty."). FieldValue sentinels are still rejected client-side with the literals context and field path, because they cannot be encoded and the backend never sees them. Replaces the at-least-one-document unit test with tests showing that an empty literals stage and empty field names are encoded and left for the backend to reject. No API change.
Commit 8fe4bb6 stopped validating literal field names client-side, but maps inside a list were still parsed by UserDataReader, so an empty key such as `{l: [{"": 1}]}` still failed client-side with "Document fields must not be empty". `parseLiteralValue` now also walks lists, so field names are left to the backend at every depth. A nightly probe confirmed that the backend rejects `{l: [{"": 1}]}` with INVALID_ARGUMENT ("Property names cannot be empty."). Everything else is unchanged: nested arrays and FieldValue sentinels are still rejected client-side with the literals context. Extends the empty field name unit test to cover lists, and adds unit tests for nested arrays and for a sentinel inside a list. No API change.
…error in DML stages The insert and upsert overloads that take a `CollectionReference` now use the same check and message as the released `PipelineSource.collection(CollectionReference)`. The check compares both the database ID and the project ID, and the message is "Invalid CollectionReference. The Firestore instance of the CollectionReference must match the Firestore instance of the Pipeline." It names `Pipeline` instead of `PipelineSource`, because the reference is passed to a `Pipeline` stage. Updates the unit tests. No API change.
Since the client-side at-least-one-document check was removed, the KDoc of `PipelineSource.literals()` now says that the backend rejects a pipeline without literal documents when it is executed. No API change.
Firestore 27.0.0 has been released, so gradle.properties on this branch was stale: version (27.0.0) is already on GMaven and latestReleasedVersion (26.6.0) is no longer the latest release, which fails gmavenVersionCheck. The pipeline DML stages, PipelineSource.literals() and ExecuteOptions.withAtomic() are new public API that is not in 27.0.0. metalavaSemver reports them as MINOR changes, so the next version needs a minor bump. Set latestReleasedVersion to 27.0.0 and version to 27.1.0, which matches the values main will have once this work is merged.
Contributor
Author
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request adds pipeline DML stages (delete, update, insert, upsert), the PipelineSource.literals() source, atomic execution options, and support for removing reserved fields. It also renames the as method to alias to avoid using backticks in Kotlin. The review feedback highlights an inconsistency in client-side validation where nested arrays containing expressions bypass checks in toLiteralExpression and are only rejected by the backend, unlike nested arrays without expressions which are caught immediately. It is recommended to add the missing validation check and include corresponding unit tests to verify this behavior.
MarkDuckworth
previously approved these changes
Oct 9, 2026
…() KDoc Each insert overload already documents how it chooses the target document, so the summary of the other overloads in the insert() KDoc only repeated that information in a place where it could be confusing.
… target path insert() and upsert() without a target write each document to the path in its __name__ field and never generate an ID. A document from literals() has no path unless it sets __name__ to a DocumentReference, and the backend then fails the pipeline with INVALID_ARGUMENT. The upsert overloads that take a collection generate an ID for such documents, like the matching insert overloads already document. Add integration tests for literals with and without __name__ written by insert() and upsert(), and for upsert into a collection generating an ID.
field("__name__") is the document reference, so the tests compared
document_id(__name__) with an ID string to select a document. Compare
field(FieldPath.documentId()) with a DocumentReference instead, which is
what Query.whereEqualTo(FieldPath.documentId(), id) does when a query is
converted to a pipeline, and avoids evaluating a function on every
document. The test that stores the ID as a field still uses documentId().
…rDataReader LiteralsSource walked every map and list itself so that empty field names were not validated client-side. That duplicated part of UserDataReader and skipped the validation that every other API applies to user data. Only maps and lists that contain an Expression are walked now, because they have to be converted into map() and array() expressions. Every other value is parsed by UserDataReader with the field's parse context, so empty field names, nested arrays and FieldValue sentinels are rejected with the same errors, including the field path, as for other user data.
Tighten the insert(), upsert() and literals() KDoc around the __name__ path, describe literal documents without an ID accurately, and reduce the LiteralsSource comments to the reason maps and lists with expressions are converted. Name the empty literals test after what it asserts and drop the test counts from the integration test section headers.
MarkDuckworth
approved these changes
Oct 9, 2026
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
2 participants
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.
Changes
literals()are now evaluated (the backend used to reject them asFUNCTION_VALUE).literals()fail with an error namingliterals()and the field path.insert/upsertuse explicit overloads taking a String path orCollectionReference; the insert collection is optional.upsert(List),literals(List),documents(List)anddocumentsByPath(List).Expression.as(), an undocumented duplicate ofalias().literals,withAtomicanddocumentsAPI Change Decisions
No released stage method takes a List of inputs (List parameters only appear for values, like
array()), so the List overloads are gone andupdate(List)wasn't added. Callers can spread a list.The released Pipeline API has no default arguments or nullable stage parameters, and takes collections by path or reference (
PipelineSource.collection). The new overloads follow that:insert(),insert(String),insert(CollectionReference),insert(Expression),insert(String, Expression),insert(CollectionReference, Expression)upsert(vararg Selectable),upsert(String),upsert(CollectionReference),upsert(String, Expression),upsert(CollectionReference, Expression)A target upsert no longer takes extra fields, because an array parameter breaks the vararg convention and a trailing vararg would collide with
(String, Expression); useaddFields(...).upsert(collection, id). There's noupsert(Expression):Fieldis both anExpressionand aSelectable, soupsert(field("x"))would pick the ID overload. A reference from another Firestore instance gets thePipelineSource.collectionerror.