Repository navigation
feat(firestore): DML - #14703
Open
dlarocque wants to merge 19 commits into
Open
feat(firestore): DML#14703dlarocque wants to merge 19 commits into
dlarocque wants to merge 19 commits into
Conversation
… option to Java SDK pipelines - Implement Insert.java and Upsert.java stages in com.google.cloud.firestore.pipeline.stages - Add insert() and upsert() builder methods to Pipeline.java - Add withAtomic() option to PipelineExecuteOptions.java and wire proto request (newTransaction + autoCommitTransaction) - Add proto serialization unit tests in PipelineProtoTest.java - Add system integration test suite in ITPipelineTest.java covering insert, upsert, atomic execution, and transaction execution BUG=b/545136773 TAG=agy
The backend evaluates expressions only in the top-level fields of each literal document. Previously, an Expression nested inside a Map or List value was sent as a raw function_value inside a map_value/array_value and the request was rejected with `INVALID_ARGUMENT: Value type is not supported: FUNCTION_VALUE`. 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, so constant-only data (including maps shaped like a proto Value) is unaffected. literals() shipped in 3.40.0, so the conversion is applied only to inputs that failed before. Adds a proto unit test and an integration test with expressions nested in a map (two levels deep) and in a list. Proposal P-3.
…t-side
- literals() now requires at least one document and fails fast with
IllegalArgumentException "Function literals() requires at least one
document." The backend previously rejected an empty literals stage
with INVALID_ARGUMENT. The varargs signature shipped in 3.40.0, so
this is a runtime check rather than a signature change.
- A null document now fails with a clear NullPointerException
("Function literals() called with invalid data. Document 0 is null.")
instead of an NPE from Map.entrySet().
- FieldValue sentinels anywhere in a literal value (including inside
nested maps and lists) are rejected with literals() wording and the
real field path, for example: "Function literals() called with invalid
data. FieldValue.serverTimestamp() can only be used with update() and
set() (found in field a.b)". Previously the message ended with
"at field ''". Values are still rejected before any RPC is sent, now
when literals() is called rather than when the pipeline is executed.
- null field values remain allowed.
POJO values are unchanged: they still fail with "Cannot convert ... to
Firestore Value". Whether literals() should map POJOs the way set()
does is left as an open decision (proposal P-4e).
Proposal P-4 (a, b, c, d).
…e.withTransformedFields() Add PipelineSource.literals(List<Map<String, Object>>) and Update.withTransformedFields(List<Selectable>) to mirror the existing varargs overloads and the List overloads offered elsewhere in the pipeline API. Note: with both overloads present, an untyped literals(null) call is now ambiguous at compile time (edge case; null was already rejected). Proposal P-2/P-6 (minor).
…xpose DML stage classes Add the missing convenience overloads so every DML stage has both a no-arg form and a form taking a pre-configured stage: - Pipeline.insert() (equivalent to insert(new Insert())) - Pipeline.delete(Delete) (equivalent to delete()) Remove the class-level @internalapi annotation from the user-constructed stage classes Insert, Upsert, Update and Delete. They are passed to public Pipeline methods (insert(Insert), upsert(Upsert), update(Update), delete(Delete)) and must be constructed by users, so marking them internal contradicted the public API. Literals keeps @internalapi: it is only created by PipelineSource.literals() and no public method accepts it. Proposal P-7 + P-12.
…Upsert Remove the duplicate alias methods so each option has a single name across the pipeline DML stages: - Insert.withDocumentId(Expression) -> use withDocumentIdExpression - Upsert.withDocumentId(Expression) -> use withDocumentIdExpression - Upsert.withTransformedFields(Selectable...) and (List<Selectable>) -> use withAdditionalFields These aliases (and the Insert/Upsert classes themselves) exist only on this feature branch and were never released (absent from origin/main), so removing them breaks no released API. Update.withTransformedFields is released and unchanged. The "backward compatibility" blocks in PipelineProtoTest that exercised the aliases are removed. Proposal P-9.
… or at a read time PipelineExecuteOptions.withAtomic(true) makes the backend start and auto-commit its own read-write transaction. Combining it with an enclosing transaction (Transaction.execute(pipeline, options)) or with a read time (read-only transactions) was silently ignored: the transaction ID took precedence and the atomic flag was dropped. Throw IllegalArgumentException from the request builder instead, before any RPC is sent: - atomic + transaction: "Atomic pipeline execution cannot be used inside a transaction. ..." - atomic + read time: "Atomic pipeline execution cannot be combined with a read time." Non-atomic execution in transactions and at a read time is unchanged. Adds a PipelineProtoTest unit test and the ITPipelineTest testAtomicExecutionInsideTransactionIsRejected integration test. (proposal P-14; pending team decision)
…ecution - Added Javadoc for the Insert and Upsert classes, their constructors and builder methods, Pipeline.insert(), insert(Insert), upsert(Upsert), upsert(Selectable...), and PipelineExecuteOptions.withAtomic()/isAtomic(). - Documented the insert/upsert target rules: with neither a collection nor a document ID expression the input path is reused (so inserting documents read from the database fails with ALREADY_EXISTS); with only a collection the input document ID is reused (generated if the input has none); with only a document ID expression the input document's parent collection is used. The document ID expression must evaluate to an ID, not a path. Upsert replaces the target document; stored fields are not merged. - Documented atomic execution: default false; when true, all writes are applied or none are; not allowed in Transaction.execute() or with a read time (throws IllegalArgumentException). - Documented the literals() contract on both overloads: at least one document, expressions (including nested ones) are evaluated, FieldValue sentinels are rejected (use currentTimestamp()), and __name__ sets the document path. - Extended the delete() and update() Javadoc (result shape, replace semantics, no document creation, atomicity). - Removed the stray "Performs an insert operation..." first sentence from the rawStage() Javadoc. CHANGELOG.md is generated by release-please and is not edited. Proposal P-16 (and the P-10 target rules).
Remove the client-side "at least one document" check from Literals
(and therefore from both PipelineSource.literals() overloads). The
backend already rejects an empty literals stage with INVALID_ARGUMENT
("... must have at least one document"), and the SDK should not
duplicate checks the backend enforces. The error now surfaces from
execute() instead of at pipeline build time.
Kept client-side:
- checkNotNull for a null documents array or list and for a null
document, since the SDK would otherwise fail with a bare
NullPointerException while encoding. The null-array message is now
"Function literals() called with invalid data. Documents must not be
null."
- FieldValue sentinel rejection, because sentinels cannot be encoded
in a pipeline, so the backend never sees them.
The literals() Javadoc now says that an empty set of documents is
rejected by the backend at execute time. The unit tests assert that an
empty literals() builds a stage with no arguments, and that
literals(emptyList()) encodes the same way. A new IT,
testLiteralsWithNoDocumentsIsRejectedByBackend, checks the backend
error.
Follows the decision not to duplicate backend-enforced checks
(proposal D-1 / P-4a).
Tighten the Javadoc and comments added for the pipeline DML stages and literals so they describe behavior or explain non-obvious code, without narrating how the change was made: - Document that insert() and upsert() without a target collection use the input document's __name__ path, and that a document without __name__ fails with INVALID_ARGUMENT. - State the empty literals() behavior plainly (execution fails with INVALID_ARGUMENT) and drop an internal comment that described why no client-side check exists. - Reword the sentinel validation Javadoc to say why sentinels are rejected. - Remove test comments that only restate the assertions, and reword the remaining ones as statements of behavior. No code changes.
…ction or at a read time This reverts commit 74d47c4. Atomic execution inside Transaction.execute is redundant but harmless, and atomic execution with a read time is rejected by the backend, so the SDK doesn't need dedicated checks for either.
…UserDataConverter Literal values are already encoded with UserDataConverter, which rejects FieldValue sentinels in pipeline arguments. Drop the separate sentinel walk and the literals-specific message, and use plain null-check messages. Sentinels are now reported when the pipeline is encoded rather than when literals() is called.
Document atomic execution once, on PipelineExecuteOptions.withAtomic, instead of on every DML method. Describe the insert and upsert target rules only on the Insert and Upsert classes, and remove the Javadoc for the rejected-atomic-in-transaction behavior that is no longer thrown.
![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 introduces new 'Insert' and 'Upsert' stages to the Firestore pipeline, enabling document insertion and upsert operations. It also adds support for atomic pipeline execution and improves literal expression handling. My review identified potential issues where 'collection' and 'document_id' options are not correctly cleared when set to null in the 'Insert' and 'Upsert' stages, as well as opportunities to simplify the 'withAdditionalFields' methods in the 'Upsert' class by removing redundant null checks.
Comment on lines
+111
to
+118
| Map<String, Value> optsMap = new HashMap<>(baseOptions.options); | ||
| if (collectionPath != null) { | ||
| String path = collectionPath.startsWith("/") ? collectionPath : "/" + collectionPath; | ||
| optsMap.put("collection", Value.newBuilder().setReferenceValue(path).build()); | ||
| } | ||
| if (documentIdExpression != null) { | ||
| optsMap.put("document_id", PipelineUtils.encodeValue(documentIdExpression)); | ||
| } |
Contributor
There was a problem hiding this comment.
If collectionPath or documentIdExpression is set to null (for example, to clear them), the old values are copied from baseOptions.options and retained in the final options map because they are never explicitly removed. This causes the object's state to become out of sync with the underlying proto options. We should explicitly remove these keys from optsMap if the corresponding parameters are null.
Map<String, Value> optsMap = new HashMap<>(baseOptions.options);
if (collectionPath != null) {
String path = collectionPath.startsWith("/") ? collectionPath : "/" + collectionPath;
optsMap.put("collection", Value.newBuilder().setReferenceValue(path).build());
} else {
optsMap.remove("collection");
}
if (documentIdExpression != null) {
optsMap.put("document_id", PipelineUtils.encodeValue(documentIdExpression));
} else {
optsMap.remove("document_id");
}
Comment on lines
+162
to
+169
| Map<String, Value> optsMap = new HashMap<>(baseOptions.options); | ||
| if (collectionPath != null) { | ||
| String path = collectionPath.startsWith("/") ? collectionPath : "/" + collectionPath; | ||
| optsMap.put("collection", Value.newBuilder().setReferenceValue(path).build()); | ||
| } | ||
| if (documentIdExpression != null) { | ||
| optsMap.put("document_id", PipelineUtils.encodeValue(documentIdExpression)); | ||
| } |
Contributor
There was a problem hiding this comment.
If collectionPath or documentIdExpression is set to null (for example, to clear them), the old values are copied from baseOptions.options and retained in the final options map because they are never explicitly removed. This causes the object's state to become out of sync with the underlying proto options. We should explicitly remove these keys from optsMap if the corresponding parameters are null.
Map<String, Value> optsMap = new HashMap<>(baseOptions.options);
if (collectionPath != null) {
String path = collectionPath.startsWith("/") ? collectionPath : "/" + collectionPath;
optsMap.put("collection", Value.newBuilder().setReferenceValue(path).build());
} else {
optsMap.remove("collection");
}
if (documentIdExpression != null) {
optsMap.put("document_id", PipelineUtils.encodeValue(documentIdExpression));
} else {
optsMap.remove("document_id");
}
Comment on lines
+110
to
+116
| public Upsert withAdditionalFields(Selectable... additionalFields) { | ||
| return new Upsert( | ||
| ImmutableList.copyOf(additionalFields), | ||
| this.collectionPath, | ||
| this.documentIdExpression, | ||
| this.options); | ||
| } |
Contributor
There was a problem hiding this comment.
Since the constructor Upsert(Selectable...) already handles a null array gracefully by applying a default value, we can avoid a redundant null check in withAdditionalFields by passing additionalFields directly to the constructor instead of using ImmutableList.copyOf.
Suggested change
| public Upsert withAdditionalFields(Selectable... additionalFields) { | |
| return new Upsert( | |
| ImmutableList.copyOf(additionalFields), | |
| this.collectionPath, | |
| this.documentIdExpression, | |
| this.options); | |
| } | |
| public Upsert withAdditionalFields(Selectable... additionalFields) { | |
| return new Upsert( | |
| additionalFields, | |
| this.collectionPath, | |
| this.documentIdExpression, | |
| this.options); | |
| } |
References
- Avoid redundant null checks for optional parameters if the underlying model or setter method handles null by applying a default value.
Comment on lines
+125
to
+131
| public Upsert withAdditionalFields(List<Selectable> additionalFields) { | ||
| return new Upsert( | ||
| ImmutableList.copyOf(additionalFields), | ||
| this.collectionPath, | ||
| this.documentIdExpression, | ||
| this.options); | ||
| } |
Contributor
There was a problem hiding this comment.
Since the constructor already handles a null list gracefully by applying a default value, we can avoid a redundant null check in withAdditionalFields by passing additionalFields directly to the constructor instead of using ImmutableList.copyOf.
Suggested change
| public Upsert withAdditionalFields(List<Selectable> additionalFields) { | |
| return new Upsert( | |
| ImmutableList.copyOf(additionalFields), | |
| this.collectionPath, | |
| this.documentIdExpression, | |
| this.options); | |
| } | |
| public Upsert withAdditionalFields(List<Selectable> additionalFields) { | |
| return new Upsert( | |
| additionalFields, | |
| this.collectionPath, | |
| this.documentIdExpression, | |
| this.options); | |
| } |
References
- Avoid redundant null checks for optional parameters if the underlying model or setter method handles null by applying a default value.
This branch has not been deployed
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.
No description provided.