Skip to content

feat(firestore): add DML stages, literals source, and atomic execution option to Node SDK pipelines - #9118

Open
wu-hui wants to merge 2 commits into
mainfrom
feat-node-dml
Open

feat(firestore): add DML stages, literals source, and atomic execution option to Node SDK pipelines#9118
wu-hui wants to merge 2 commits into
mainfrom
feat-node-dml

Conversation

@wu-hui

@wu-hui wu-hui commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

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.

@wu-hui
wu-hui requested a review from a team as a code owner August 10, 2026 17:43
@product-auto-label product-auto-label Bot added the api: firestore Issues related to the Firestore API. label Aug 10, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +267 to 273
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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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;
        }

Comment on lines +156 to +169
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;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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;
    }

Comment on lines +1907 to +1920
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;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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;
    }

Comment on lines +1149 to +1157
_validateUserData(ignoreUndefinedProperties: boolean): void {
this.documents.forEach(doc => {
Object.values(doc).forEach(val => {
if (hasUserData(val)) {
val._validateUserData(ignoreUndefinedProperties);
}
});
});
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: firestore Issues related to the Firestore API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant