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

[BUG] - MSSQL - bulkCreate on BigInt fields fails #15411

Closed
3 of 6 tasks
marra85 opened this issue Dec 7, 2022 · 5 comments
Closed
3 of 6 tasks

[BUG] - MSSQL - bulkCreate on BigInt fields fails #15411

marra85 opened this issue Dec 7, 2022 · 5 comments
Labels
released type: feature For issues and PRs. For new features. Never breaking changes. v6

Comments

@marra85
Copy link

marra85 commented Dec 7, 2022

Issue Creation Checklist

  • I understand that my issue will be automatically closed if I don't fill in the requested information
  • I have read the contribution guidelines

Bug Description

Reproducible Example

Here is the link to the SSCCE for this issue:
sequelize/sequelize-sscce#242

What do you expect to happen?

the bulkCreate method should complete the operation.

What is actually happening?

The operation is failing because the bigint primitive is not well handled by the escape function in sql-string.js helper.

Adding a case statement on top of the existing number management would solve the issue because it will generate a correct SQL statement by removing the n at the end. (reference)

before

case 'number':
      return val.toString();

after

case 'bigint':
case 'number':
      return val.toString();

Environment

  • Sequelize version: 6.26.0
  • Node.js version: 14.20.0
  • If TypeScript related: TypeScript version: N\A
  • Database & Version: MSSQL 2016
  • Connector library & Version: tedious

Would you be willing to resolve this issue by submitting a Pull Request?

I can open a PR but I need assistance on how manage the automatic testing (if required)

  • Yes, I have the time and I know how to start.
  • Yes, I have the time but I will need guidance.
  • No, I don't have the time, but my company or I are supporting Sequelize through donations on OpenCollective.
  • No, I don't have the time, and I understand that I will need to wait until someone from the community or maintainers is interested in resolving my issue.

Indicate your interest in the resolution of this issue by adding the 👍 reaction. Comments such as "+1" will be removed.

@ephys ephys added type: feature For issues and PRs. For new features. Never breaking changes. v6 labels Dec 7, 2022
@ephys
Copy link
Member

ephys commented Dec 7, 2022

Just FYI, this is available in Sequelize 7 thanks to #14485 :)

In the meantime, you can convert the bigint to a string, that's what sequelize used before bigint were a thing in JS

@marra85
Copy link
Author

marra85 commented Dec 7, 2022

Hello @ephys,

thanks for the information!
Sequelize 7 is still in alpha so it is not an option (for now) since our software is running in production.

As for the conversion it will require us to modify the codebase in more than 100 functions to just to handle a cast that could be centralised on sequelize side.

I'm available to open a PR if it will speedup the resolution :)

@ephys
Copy link
Member

ephys commented Dec 7, 2022

That would definitely speed things up :) If you're willing to port #14485 to our v6 branch, I'll make sure to review it in a timely manner

@github-actions
Copy link
Contributor

🎉 This issue has been resolved in version 6.27.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@WikiRik
Copy link
Member

WikiRik commented Dec 12, 2022

Thanks for working on this!

@WikiRik WikiRik closed this as completed Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released type: feature For issues and PRs. For new features. Never breaking changes. v6
Projects
None yet
Development

No branches or pull requests

3 participants