Best effort support for JS BigInt in queries#14485
Merged
ephys merged 17 commits intosequelize:mainsequelize/sequelize:mainfrom May 14, 2022
papandreou:feature/queryBigintpapandreou/sequelize:feature/queryBigintCopy head branch name to clipboard
Merged
Best effort support for JS BigInt in queries#14485ephys merged 17 commits intosequelize:mainsequelize/sequelize:mainfrom papandreou:feature/queryBigintpapandreou/sequelize:feature/queryBigintCopy head branch name to clipboard
ephys merged 17 commits intosequelize:mainsequelize/sequelize:mainfrom
papandreou:feature/queryBigintpapandreou/sequelize:feature/queryBigintCopy head branch name to clipboard
Conversation
4162dc2 to
e7696e1
Compare
7154930 to
89e43b8
Compare
89e43b8 to
49a5d2d
Compare
Member
|
Thank you for this PR, I'll set some time aside this friday/weekend to review this |
ephys
requested changes
May 13, 2022
Member
ephys
left a comment
There was a problem hiding this comment.
Very nice, only a few notes. Thank you very much for your work on this!
Co-authored-by: Zoé <zoe@ephys.dev>
Co-authored-by: Zoé <zoe@ephys.dev>
Co-authored-by: Zoé <zoe@ephys.dev>
ephys
approved these changes
May 14, 2022
Member
ephys
left a comment
There was a problem hiding this comment.
I really appreciate you taking this off my backlog, thank you! :)
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
6 tasks
marra85
pushed a commit
to marra85/sequelize
that referenced
this pull request
Dec 7, 2022
5 tasks
marra85
added a commit
to marra85/sequelize
that referenced
this pull request
Dec 8, 2022
ephys
pushed a commit
that referenced
this pull request
Dec 12, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request Checklist
Please make sure to review and check all of these items:
yarn testoryarn test-DIALECTpass with this change (including linting)?Description Of Change
bigintprimitives inModel.findByPkbigints in SQL queries unless necessary because of limitations in the underlying driverBackground
I'm working on an app that uses Postgres and needs to use the full
BIGINT/INT8range for PK columns. The obvious thing is to use JavaScriptBigInts for that to prevent loss of precision when going overNumber.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 thepgdriver to deserializeINT8asBigIntactually gets me very far:That just left
Model.findByPk, as passing aBigIntto that throws: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
BigInttoModel.findByPkor into awhereobject, 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 supportBigIntyet. Thus the "best effort" label. I still think it's worth it to try to DTRT when aBigIntis passed into a query.