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

Best effort support for JS BigInt in queries #14485

Merged
merged 17 commits into from May 14, 2022

Conversation

papandreou
Copy link
Contributor

@papandreou papandreou commented May 5, 2022

Pull Request Checklist

Please make sure to review and check all of these items:

  • Have you added new tests to prevent regressions?
  • Does yarn test or yarn test-DIALECT pass with this change (including linting)?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you update the typescript typings accordingly (if applicable)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description Of Change

  • Allow bigint primitives in Model.findByPk
  • Do not stringify bigints in SQL queries unless necessary because of limitations in the underlying driver

Background

I'm working on an app that uses Postgres and needs to use the full BIGINT/INT8 range for PK columns. The obvious thing is to use JavaScript BigInts for that to prevent loss of precision when going over Number.MAX_SAFE_INTEGER.

I saw #14296 that aims to address how values from the database get mapped to values in JavaScript.

Even without something like that, I found out that sequelize already has some support for BigInt, and that configuring the pg driver to deserialize INT8 as BigInt actually gets me very far:

const pg = require('pg');

// https://github.com/brianc/node-postgres/issues/378
function nullHandler(parser) {
  return function (val) {
    return val === null ? null : parser(val);
  };
}

pg.types.setTypeParser(pg.types.builtins.INT8, nullHandler(BigInt));
const bigintArrayOid = 1016; // Postgres OID for bigint[]. See SELECT typarray FROM pg_type WHERE typname = 'int8';
const parseBigIntArray = pg.types.getTypeParser(bigintArrayOid);
pg.types.setTypeParser(bigintArrayOid, (a) => parseBigIntArray(a).map(BigInt));

That just left Model.findByPk, as passing a BigInt to that throws:

Argument passed to findByPk is invalid: 123

That was easy to fix (at least for Postgres), it was just a matter of allowing BigInts through in the type check that threw the error.

Then I noticed that when I passed a BigInt to Model.findByPk or into a where object, the number would become a string literal in the resulting SQL. That seemed a bit wasteful, so I tried to fix that also. Getting it to work with all the dialects was a bit tricky, as not all of the underlying drivers support BigInt yet. Thus the "best effort" label. I still think it's worth it to try to DTRT when a BigInt is passed into a query.

@WikiRik WikiRik requested a review from ephys May 7, 2022 08:46
@papandreou papandreou force-pushed the feature/queryBigint branch 2 times, most recently from 7154930 to 89e43b8 Compare May 7, 2022 20:38
@ephys
Copy link
Member

ephys commented May 11, 2022

Thank you for this PR, I'll set some time aside this friday/weekend to review this

Copy link
Member

@ephys ephys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, only a few notes. Thank you very much for your work on this!

src/dialects/mssql/query.js Outdated Show resolved Hide resolved
src/model.d.ts Outdated Show resolved Hide resolved
test/unit/query-generator/select-query.test.ts Outdated Show resolved Hide resolved
Copy link
Member

@ephys ephys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really appreciate you taking this off my backlog, thank you! :)

@ephys ephys merged commit 15b5619 into sequelize:main May 14, 2022
@github-actions
Copy link
Contributor

🎉 This PR is included in version 7.0.0-alpha.13 🎉

The release is available on:

Your semantic-release bot 📦🚀

vanthome pushed a commit to vanthome/sequelize that referenced this pull request Jun 12, 2022
This adds supports for sending JS BigInt values to databases. For retrieving SQL BigInts as JS BigInts, see sequelize#14296
marra85 pushed a commit to marra85/sequelize that referenced this pull request Dec 7, 2022
marra85 added a commit to marra85/sequelize that referenced this pull request Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants