Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fix bigint validation #1620

Merged
merged 1 commit into from
Apr 18, 2024
Merged

fix: fix bigint validation #1620

merged 1 commit into from
Apr 18, 2024

Conversation

MichaelSun90
Copy link
Contributor

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"

Copy link

codecov bot commented Apr 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@arthurschreiber arthurschreiber changed the title chore: fix bigint validation fix: fix bigint validation Apr 18, 2024
@MichaelSun90 MichaelSun90 merged commit 2b21b12 into master Apr 18, 2024
25 of 27 checks passed
Copy link

🎉 This PR is included in version 18.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@dhensby
Copy link
Contributor

dhensby commented Jun 18, 2024

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: tediousjs/node-mssql#1664

@dhensby
Copy link
Contributor

dhensby commented Jun 18, 2024

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
Copy link
Collaborator

@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
Copy link
Contributor

dhensby commented Jun 18, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants