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

feat: add support for bigints (backport of #14485) #15413

Merged
merged 10 commits into from Dec 12, 2022

Conversation

marra85
Copy link

@marra85 marra85 commented Dec 7, 2022

Pull Request Checklist

Description Of Change

this PR backports #14485 to v6 branch with some exceptions regarding original PR changes

  • src/dialects/mssql/query.js L93-L98 because the paramType object changed
  • test/integration/sequelize/query.test.js because changes tests that are not related to BigInt support
  • test/registerEsbuild.js because increases the node version
  • test/unit/query-generator/select-query.test.ts because does not exists in v6

the other changes have been applied.

Copy link
Member

@WikiRik WikiRik left a comment

Choose a reason for hiding this comment

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

Maybe you can add the test of select-query.test.ts in https://github.com/sequelize/sequelize/blob/v6/test/unit/sql/select.test.js ?

test/integration/model/findOne.test.js Outdated Show resolved Hide resolved
test/integration/model/findOne.test.js Outdated Show resolved Hide resolved
@marra85 marra85 force-pushed the packport/14485_bigint_support branch from 02e4f6a to d935880 Compare December 8, 2022 14:46
@marra85
Copy link
Author

marra85 commented Dec 8, 2022

Hello @ephys,

I've revised CI the failures and implemented the suggestions by @WikiRik.

I tried to fix oracle specific issues, since in v7 is not present, from what I could see from stable v6 the data was cast to string before arriving in the run executor, so I applied the same logic from sqlite (stringify bigints).
It seems that it works for .create but not for .bulkCreate, since I'm not familiar with oracle I need some advice.
could you please tell me the best approach (or help me fix it?)

One side note: the ported test from typescript has different values since the sql generated in v6 is slightly different, I've left the original SQL statements to let you decide if they are good values or not.

@ephys
Copy link
Member

ephys commented Dec 8, 2022

I believe the problem comes from this:

_getBindDef(oracledb) {
return { type: oracledb.DB_TYPE_NUMBER };
}
. It's saying that the bind parameter is a number instead of a string

It looks like oracle set up their own way of passing bind parameter types around (due to this not being available yet). It's called here: https://github.com/sequelize/sequelize/blob/v6/src/dialects/oracle/query-generator.js#L716

I've pinged them on slack to see if they have an opinion on how to solve this

One side note: the ported test from typescript has different values since the sql generated in v6 is slightly different, I've left the original SQL statements to let you decide if they are good values or not.

I saw :) That's perfectly fine, we simply made further changes in v7 that aren't part of v6

@ephys
Copy link
Member

ephys commented Dec 8, 2022

I think a good way to fix that would be to update this.options.inbindAttributes here:

// Since the bigint primitive is not automatically translated
// we stringify the value as was done previously
if (_.isPlainObject(parameters)) {
const newParameters = Object.create(null);
for (const key of Object.keys(parameters)) {
newParameters[`${key}`] = stringifyIfBigint(parameters[key]);
}
parameters = newParameters;
} else if (_.isArray(parameters)) {
parameters = parameters.map(stringifyIfBigint);
}

@nilabja-bhattacharya
Copy link

Hi @marra85,

Can you try iterating over the out parameter list/map and convert the type from DB_TYPE_NUMBER to DB_TYPE_VARCHAR2 when we pass an int > MAX_SAFE_INTEGER. Let me know if it works.

Copy link
Member

@fzn0x fzn0x left a comment

Choose a reason for hiding this comment

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

Looks good, also want to say to @nilabja-bhattacharya for the review help :)

@marra85
Copy link
Author

marra85 commented Dec 12, 2022

@ephys I'm glad you approved the PR!

could you share an ETA for the release containing this fix?
it's really important for us :)

PS: about the CI / Postgres 10 (Node 10) (minified aliases) (pull_request) lane I saw that the failure seems to be related to a schema not existing, but I'm not able to reproduce the issue locally, moreover all other PG CI lanes are passing.

@ephys
Copy link
Member

ephys commented Dec 12, 2022

Pretty much as soon as the CI gives the greenlight, the release will be published automatically

The CI issue is something we fixed in v7, but fails every now and then in v6. It's just a badly written test, I've just re-run that job :)

@ephys ephys changed the title feat: backport #14485 in v6 branch to resolve #15411 feat: add support for bigints (backport of #14485) Dec 12, 2022
@ephys ephys merged commit 1247c01 into sequelize:v6 Dec 12, 2022
@github-actions
Copy link
Contributor

🎉 This PR is included in version 6.27.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

None yet

5 participants