tediousjs / tedious

Node TDS module for connecting to SQL Server databases.
http://tediousjs.github.io/tedious/
MIT License
1.58k stars 439 forks source link

fix: don't throw in _transform of BulkLoad #1590

Closed everhardt closed 3 months ago

everhardt commented 1 year ago

I got an uncaughtException, caused by trying to write a very large (negative) number using BulkLoad to a decimal column. Seems similar to https://github.com/tediousjs/tedious/issues/421. The stack trace I got:

RangeError: The value of "value" is out of range. It must be >= 0 and <= 4294967295. Received 3_.40_282_346_638_528_86e_+42
  File "node:internal/errors", line 496, in __node_internal_captureLargerStackTrace
  File "node:internal/errors", line 405, in new NodeError
  File "node:internal/buffer", line 74, in checkInt
  File "node:internal/buffer", line 694, in writeU_Int32LE
  File "node:internal/buffer", line 707, in Buffer.writeUInt32LE
  File "/app/node_modules/tedious/src/data-types/decimal.ts", line 77, in Object.generateParameterData
  File "<anonymous>", line unknown, in generateParameterData.next
  File "/app/node_modules/tedious/src/bulk-load.ts", line 209, in _transform
  File "node:internal/streams/transform", line 175, in Transform._write
  File "node:internal/streams/writable", line 392, in writeOrBuffer
  File "node:internal/streams/writable", line 333, in _write
  File "node:internal/streams/writable", line 337, in Writable.write
  File "node:internal/streams/readable", line 777, in Readable.ondata
  File "node:events", line 517, in Readable.emit
  File "node:internal/streams/readable", line 550, in Readable.read
  File "node:internal/streams/readable", line 1032, in flow
  File "node:internal/streams/readable", line 1013, in resume_
  File "node:internal/process/task_queues", line 82, in process.processTicksAndRejections

bulk-load:209 is inside the _tranform function, for which I think the following in the nodejs documentation holds:

Throwing an Error from within these methods or manually emitting an 'error' event results in undefined behavior.

Therefore, I propose to catch such errors and call the callback with it. I verified that solved my case.

arthurschreiber commented 1 year ago

Can you add tests for this change?

codecov[bot] commented 1 year ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 78.29%. Comparing base (42617e4) to head (b6b88f1).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1590 +/- ## ========================================== - Coverage 79.01% 78.29% -0.73% ========================================== Files 93 93 Lines 4876 4878 +2 Branches 937 937 ========================================== - Hits 3853 3819 -34 - Misses 720 761 +41 + Partials 303 298 -5 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

everhardt commented 1 year ago

Can you add tests for this change?

Done!

github-actions[bot] commented 3 months ago

:tada: This PR is included in version 18.4.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket: