fix(spanner): preserve query immutability and optimize trace config handling - #9264
fix(spanner): preserve query immutability and optimize trace config handling#9264olavloite wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
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.
…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`.
71129fa to
6682ea2
Compare
|
/gemini review |
There was a problem hiding this comment.
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.
|
The lint failures are unrelated to this change. |
getQueryTraceConfighelper to eliminate duplicated trace configuration logic and guarantee null safety.deleteoperations with rest destructuring acrossSnapshot,Dml, andBatchTransactionto preserve caller query immutability and prevent property loss on stream retries.Snapshot.prototype.runStream.SPANNER_ENABLE_UUID_AS_UNTYPEDlookup inSnapshot.encodeParamsand propagate it tocodec.getTypeto avoid repeatedprocess.envchecks on parameter serialization.test/transaction.tsto eliminateMaxListenersExceededWarning.