feat(firestore): add DML stages, literals source, and atomic execution option to Node SDK pipelines - #9118
feat(firestore): add DML stages, literals source, and atomic execution option to Node SDK pipelines#9118wu-hui wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for DML stages (insert, upsert, and literals) and an atomic execution option within Firestore pipelines, complete with updated type definitions and tests. The feedback highlights a critical transaction precedence issue where the atomic option could bypass active transactions, an options-parsing bug in the literals stage, and the need for recursive user data validation to prevent nested custom classes from bypassing checks.
| if (structuredPipeline.options?.atomic) { | ||
| request.newTransaction = {readWrite: {}}; | ||
| request.autoCommitTransaction = true; | ||
| } else if (transactionOrReadTime instanceof Uint8Array) { | ||
| request.transaction = transactionOrReadTime; | ||
| } else if (transactionOrReadTime instanceof Timestamp) { | ||
| request.readTime = transactionOrReadTime.toProto().timestampValue; |
There was a problem hiding this comment.
When executing a pipeline within an active transaction runner, the transaction ID (Uint8Array) must take precedence over the atomic option. If atomic is checked first, any pipeline with atomic: true will ignore the active transaction and execute as a separate, independent transaction, violating transaction isolation and atomicity. Reordering the checks ensures the active transaction is always respected.
if (transactionOrReadTime instanceof Uint8Array) {
request.transaction = transactionOrReadTime;
} else if (structuredPipeline.options?.atomic) {
request.newTransaction = {readWrite: {}};
request.autoCommitTransaction = true;
} else if (transactionOrReadTime instanceof Timestamp) {
request.readTime = transactionOrReadTime.toProto().timestampValue;
}| if (Array.isArray(docsOrOptions)) { | ||
| documents = docsOrOptions; | ||
| opts = options ?? {}; | ||
| } else if ( | ||
| docsOrOptions && | ||
| Array.isArray( | ||
| (docsOrOptions as firestore.Pipelines.LiteralsStageOptions).documents, | ||
| ) | ||
| ) { | ||
| const {documents: docs, ...rest} = | ||
| docsOrOptions as firestore.Pipelines.LiteralsStageOptions; | ||
| documents = docs ?? []; | ||
| opts = rest; | ||
| } |
There was a problem hiding this comment.
If docsOrOptions is passed as an options object without a documents array property (e.g., pipeline.literals({ rawOptions: ... })), the else if condition fails because Array.isArray(docsOrOptions.documents) is false. This causes the options to be silently ignored and opts to remain empty. Simplifying the condition to check if docsOrOptions is truthy and not an array ensures all options are correctly captured.
if (Array.isArray(docsOrOptions)) {
documents = docsOrOptions;
opts = options ?? {};
} else if (docsOrOptions) {
const {documents: docs, ...rest} =
docsOrOptions as firestore.Pipelines.LiteralsStageOptions;
documents = docs ?? [];
opts = rest;
}| if (Array.isArray(docsOrOptions)) { | ||
| documents = docsOrOptions; | ||
| opts = options ?? {}; | ||
| } else if ( | ||
| docsOrOptions && | ||
| Array.isArray( | ||
| (docsOrOptions as firestore.Pipelines.LiteralsStageOptions).documents, | ||
| ) | ||
| ) { | ||
| const {documents: docs, ...rest} = | ||
| docsOrOptions as firestore.Pipelines.LiteralsStageOptions; | ||
| documents = docs ?? []; | ||
| opts = rest; | ||
| } |
There was a problem hiding this comment.
If docsOrOptions is passed as an options object without a documents array property (e.g., pipeline.literals({ rawOptions: ... })), the else if condition fails because Array.isArray(docsOrOptions.documents) is false. This causes the options to be silently ignored and opts to remain empty. Simplifying the condition to check if docsOrOptions is truthy and not an array ensures all options are correctly captured.
if (Array.isArray(docsOrOptions)) {
documents = docsOrOptions;
opts = options ?? {};
} else if (docsOrOptions) {
const {documents: docs, ...rest} =
docsOrOptions as firestore.Pipelines.LiteralsStageOptions;
documents = docs ?? [];
opts = rest;
}| _validateUserData(ignoreUndefinedProperties: boolean): void { | ||
| this.documents.forEach(doc => { | ||
| Object.values(doc).forEach(val => { | ||
| if (hasUserData(val)) { | ||
| val._validateUserData(ignoreUndefinedProperties); | ||
| } | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The current implementation of _validateUserData only checks the direct top-level properties of each document. If a document contains nested plain objects or arrays that hold custom classes (such as DocumentReference or FieldValue), those nested values will bypass validation. Implementing a recursive traversal ensures all nested user data is thoroughly validated.
_validateUserData(ignoreUndefinedProperties: boolean): void {
const validate = (val: unknown) => {
if (hasUserData(val)) {
val._validateUserData(ignoreUndefinedProperties);
} else if (Array.isArray(val)) {
val.forEach(validate);
} else if (val && typeof val === 'object') {
Object.values(val).forEach(validate);
}
};
this.documents.forEach(validate);
}…erals source, and atomic execution option to Node SDK pipelines
Adds full support for DML stages (insert, upsert, delete, update), literals data source stage, and atomic execution option to Node SDK Firestore pipelines to achieve parity with Web SDK.