search

tediousjs / tedious

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

fix: fix `bigint` validation #1620

Closed MichaelSun90 closed 2 months ago

MichaelSun90 commented 2 months ago

This PR is for fixing issue #1610 Now the validate function will throw an error if a invalid value is passed into BigInt type : "The number cannot be converted to a BigInt because it is not an integer"

codecov[bot] commented 2 months ago

Codecov Report

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

Project coverage is 79.21%. Comparing base (a51e57d) to head (5309b5a).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1620 +/- ## ========================================== - Coverage 79.30% 79.21% -0.09% ========================================== Files 93 93 Lines 4860 4860 Branches 934 933 -1 ========================================== - Hits 3854 3850 -4 - Misses 700 709 +9 + Partials 306 301 -5 ```

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

github-actions[bot] commented 2 months ago

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

The release is available on:

Your semantic-release bot :package::rocket:

dhensby commented 1 week ago

Hi team, it seems that this change has caused a regression that means a test in the mssql lib is breaking.

We're making an assertion that a bigint with value 0 === 0 but since 18.2.0 it is now null (not entirely sure how that's happening, but it's what I've observed) - we can see it here: https://github.com/tediousjs/node-mssql/pull/1664

dhensby commented 1 week ago

OK - so this was a problem in our library because the cast function we have doesn't support native bigints. I would suggest that changing the return type of BigInt to use native bigints is a breaking change, so probably shouldn't have been released in a minor release.

arthurschreiber commented 1 week ago

@dhensby Ugh, I'm sorry. 😞

it's hard to be aware of all the things that might break when we change things like the validation code which is supposed to be more or less private / internal to tedious, but apparently is used by other libraries such as node-mssql.

What is your suggested approach to fixing this? I guess we could revert the change, publish a new version and yank the broken one, and then re-do the change as a breaking change?

Also, do you think it would make sense to add a smoke test build where we run the node-mssql tests with the changes made in tedious for each PR?

dhensby commented 1 week ago

It's ok, these things happen.

What is your suggested approach to fixing this? I guess we could revert the change, publish a new version and yank the broken one, and then re-do the change as a breaking change?

These kinds of releases are all down to acceptable and anticipated impact anyway, and I guess if these are thought of as private interfaces, then that's fair enough. The fix is actually really simple for the node-mssql lib, we just need to handle the library returning a bigint and passing it along as is seems to be all that's needed.

Also, do you think it would make sense to add a smoke test build where we run the node-mssql tests with the changes made in tedious for each PR?

We've always had a bit of chatter about that as a possibility. If you think it's valuable to do, then that could work - but the mssql lib can run a version or two behind (I'm only just getting round to upgrading the tedious version now), so I'm not sure how we'd solve that and if something does break in a major release, that may well be expected and you don't want your tests failing because mssql is lagging behind.