fix: type mismatch in Snapshot.run error handler #2487
Conversation
Summary of ChangesHello @alkatrivedi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a TypeScript compilation error within the Snapshot.run method's error handling. It addresses a type mismatch where a generic Error was being passed to a callback expecting a ServiceError, and cleans up unused parameters in the error handler. The changes ensure type safety and correct propagation of partial results and error information in the Spanner client's stream processing. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a TypeScript compilation error and a bug in the Snapshot.run error handler. By adjusting the error handler signature to only accept the err parameter, it now correctly uses the rows, stats, and metadata from the outer scope. The explicit cast to ServiceError resolves the type mismatch. However, I have a suggestion to make this cast safer to handle cases where a non-ServiceError might be emitted by the stream pipeline.
| setSpanError(span, err); | ||
| span.end(); | ||
| callback!(err, rows, stats, metadata); | ||
| callback!(err as ServiceError, rows, stats, metadata); |
There was a problem hiding this comment.
While it's true that most errors from the stream will be ServiceError instances, it's possible for other parts of the stream processing pipeline to emit a generic Error. For example, partial-result-stream.ts can emit a plain Error if it fails to resume after multiple retries.
The cast err as ServiceError is unsafe in that case and could lead to runtime errors in user code if they try to access properties like code or metadata on what they believe is a ServiceError.
To make this safer, we should ensure the error object conforms to the ServiceError interface before passing it to the callback.
if (!('code' in err)) {
Object.assign(err, {
code: grpc.status.UNKNOWN,
details: err.message,
metadata: new grpc.Metadata(),
});
}
callback!(err as ServiceError, rows, stats, metadata);86d9611 to
8c8ea39
Compare
🤖 I have created a release *beep* *boop* --- ## [8.4.0](https://togithub.com/googleapis/nodejs-spanner/compare/v8.3.1...v8.4.0) (2026-01-09) ### Features * Add a ClientContext field to Spanner requests ([#2493](https://togithub.com/googleapis/nodejs-spanner/issues/2493)) ([37504ad](https://togithub.com/googleapis/nodejs-spanner/commit/37504adcc37a7e95acfb2530313ff783d0c1fe7d)) * Exposing total CPU related fields in AutoscalingConfig ([#2490](https://togithub.com/googleapis/nodejs-spanner/issues/2490)) ([508f0ff](https://togithub.com/googleapis/nodejs-spanner/commit/508f0ff95636b004f4200522018a199263eda8ca)) * **spanner:** Support for type UUID ([#2482](https://togithub.com/googleapis/nodejs-spanner/issues/2482)) ([0047e94](https://togithub.com/googleapis/nodejs-spanner/commit/0047e9407d86521571626c69011b70307f83f8ba)) ### Bug Fixes * **deps:** Update dependency google-gax to v5.0.6 ([#2452](https://togithub.com/googleapis/nodejs-spanner/issues/2452)) ([f9e6b86](https://togithub.com/googleapis/nodejs-spanner/commit/f9e6b86ff4da03110642c17e5ebc8fac8d903d3a)) * Flaky metric test ([#2472](https://togithub.com/googleapis/nodejs-spanner/issues/2472)) ([e169cc5](https://togithub.com/googleapis/nodejs-spanner/commit/e169cc5344d38812b1ebf20c7a987715a73d6f79)) * Memory leak and deadlock due to error event in multiplexed session ([#2477](https://togithub.com/googleapis/nodejs-spanner/issues/2477)) ([c624619](https://togithub.com/googleapis/nodejs-spanner/commit/c624619a3960892b1d2d412ff79faa5a74de45df)) * Presubmit failure for samples backups test ([#2492](https://togithub.com/googleapis/nodejs-spanner/issues/2492)) ([01eb3d5](https://togithub.com/googleapis/nodejs-spanner/commit/01eb3d5801ddb21517f185b9d585fbce4fa1475c)) * Type check for key in deleteRows ([#2486](https://togithub.com/googleapis/nodejs-spanner/issues/2486)) ([7347a16](https://togithub.com/googleapis/nodejs-spanner/commit/7347a1628ad8635b8f84b36ad1d3850b78862ac7)) * Type mismatch in Snapshot.run error handler ([#2487](https://togithub.com/googleapis/nodejs-spanner/issues/2487)) ([4ac0360](https://togithub.com/googleapis/nodejs-spanner/commit/4ac036047e3a03c073300f288c746389a38d8e42)) --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
This PR fixes a TypeScript compilation error in the
Snapshot.runmethod.The problem
1. Type Mismatch: The RunCallback expects a ServiceError (with code, metadata, etc.), but the Node.js stream error event is typed to emit a generic Error. This caused a compilation failure:
Argument of type 'Error' is not assignable to parameter of type 'ServiceError'2. Undefined Variables as a argument: The .on('error', (err, rows, stats, metadata) => ...) handler was declaring rows, stats, and metadata as arguments. Since the standard error event only emits a single argument (err), these variables were always undefined inside the handler
The Fix
1. Removed undefined arguments: The error handler now only accepts (err). This allows the closure to access the correct rows, stats, and metadata variables from the outer scope, ensuring that partial results collected before the error are correctly passed to the user
2. Added Type Cast: Explicitly cast err to ServiceError. In the context of the Spanner client, runtime errors from the stream are expected to be ServiceError objects, making this cast safe and compatible with our callback signature