Skip to content

fix(spanner): preserve query immutability and optimize trace config handling - #9264

Open
olavloite wants to merge 1 commit into
mainfrom
spanner-query-immutability
Open

fix(spanner): preserve query immutability and optimize trace config handling#9264
olavloite wants to merge 1 commit into
mainfrom
spanner-query-immutability

Conversation

@olavloite

Copy link
Copy Markdown
Contributor
  • Avoid spreading string queries into traceConfig, preventing span attribute pollution and correctly setting db.statement for string queries.
  • Extract getQueryTraceConfig helper to eliminate duplicated trace configuration logic and guarantee null safety.
  • Replace delete operations with rest destructuring across Snapshot, Dml, and BatchTransaction to preserve caller query immutability and prevent property loss on stream retries.
  • Eliminate redundant intermediate shallow copies in Snapshot.prototype.runStream.
  • Hoist SPANNER_ENABLE_UUID_AS_UNTYPED lookup in Snapshot.encodeParams and propagate it to codec.getType to avoid repeated process.env checks on parameter serialization.
  • Reset mock stream handlers in test/transaction.ts to eliminate MaxListenersExceededWarning.

@product-auto-label product-auto-label Bot added the api: spanner Issues related to the Spanner API. label Sep 8, 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 refactors the Spanner client to avoid mutating user-provided query and option objects by replacing delete operations with object destructuring. It also introduces getQueryTraceConfig to prevent spreading query objects into trace configurations, which previously caused numeric attributes to appear on spans when queries were strings. However, two critical issues were identified in the review: first, in transaction.ts, the refactored runStream method fails to merge the encoded params and paramTypes into reqOpts, which will break parameterized queries; second, in batch-transaction.ts, destructuring query directly when it is a string will lead to string-spreading bugs, and request should be destructured instead.

Comment thread handwritten/spanner/src/transaction.ts
Comment thread handwritten/spanner/src/batch-transaction.ts
…andling

- Avoid spreading string queries into traceConfig, preventing span attribute pollution and correctly setting db.statement for string queries.
- Extract `getQueryTraceConfig` helper to eliminate duplicated trace configuration logic and guarantee null safety.
- Replace `delete` operations with rest destructuring across `Snapshot`, `Dml`, and `BatchTransaction` to preserve caller query immutability and prevent property loss on stream retries.
- Eliminate redundant intermediate shallow copies in `Snapshot.prototype.runStream`.
- Hoist `SPANNER_ENABLE_UUID_AS_UNTYPED` lookup in `Snapshot.encodeParams` and propagate it to `codec.getType` to avoid repeated `process.env` checks on parameter serialization.
- Reset mock stream handlers in `test/transaction.ts` to eliminate `MaxListenersExceededWarning`.
@olavloite
olavloite force-pushed the spanner-query-immutability branch from 71129fa to 6682ea2 Compare September 9, 2026 08:07
@olavloite

Copy link
Copy Markdown
Contributor Author

/gemini review

@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 refactors the Spanner client to prevent the mutation of original query and request objects by replacing property deletions with rest destructuring. It also introduces a helper function getQueryTraceConfig to safely extract SQL statements and request tags for tracing, resolving an issue where string queries were spread into trace spans as numeric attributes. However, the reviewer identified two critical issues in transaction.ts where the destructuring refactoring missed omitting client-side options like gaxOptions, json, and jsonOptions. These properties will now be incorrectly sent to the Spanner gRPC API, which could lead to validation failures or runtime errors.

Comment thread handwritten/spanner/src/transaction.ts
Comment thread handwritten/spanner/src/transaction.ts
@olavloite

Copy link
Copy Markdown
Contributor Author

The lint failures are unrelated to this change.

@olavloite
olavloite marked this pull request as ready for review September 9, 2026 09:36
@olavloite
olavloite requested a review from a team as a code owner September 9, 2026 09:36
@github-actions
github-actions Bot requested a review from shivanee-p September 9, 2026 09:36
@shivanee-p
shivanee-p removed their request for review September 9, 2026 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: spanner Issues related to the Spanner API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant