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(mariadb): do not automatically parse JSON fields #14800

Merged
merged 7 commits into from Sep 7, 2022

Conversation

lesion
Copy link
Contributor

@lesion lesion commented Jul 25, 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

MariaDB connector automatically parse JSON fields when autoJsonMap is specified in dialectOptions (this is the default value) so there's no need to handleJsonSelectQuery:

Sequelize tries to re-parse it causing issues (#10946, #13646, #12583, #13027, #13273) especially when the real value is a string (not a JSON string), because handleJsonSelectQuery checks for it:

if (row[modelField.fieldName] && typeof row[modelField.fieldName] === 'string') {

note that with this PR JSON fields from included model are parsed automatically (same behaviour as with other backends).

example code to play with:
https://gist.github.com/lesion/0b3e001f75c1fa89593ff365c2e931f7

@lesion
Copy link
Contributor Author

lesion commented Jul 25, 2022

@lesion
Copy link
Contributor Author

lesion commented Jul 25, 2022

this test should pass also with other connectors, why is sqlite required?

https://github.com/sequelize/sequelize/blob/main/test/integration/json.test.js#L232

the point is that it's the only test with a string as a JSON value: emergency_contact: 'Unknown' so could not pass because of

if (row[modelField.fieldName] && typeof row[modelField.fieldName] === 'string') {

this line has to be changed but as mariadb connector behave differently based on mariadb version I've no idea (should we check the version?).
Using autoJsonMap: false does not help neither as this will have a really different output from other connectors.

@lesion lesion marked this pull request as draft July 25, 2022 21:02
@lesion lesion marked this pull request as ready for review July 27, 2022 13:17
lesion added a commit to lesion/gancio that referenced this pull request Aug 7, 2022
force mariadb sequelize dialect to not re-parse JSON field (mariadb >= 10.5.2 required)

sequelize/sequelize#14800
@sdepold sdepold merged commit d047f32 into sequelize:v6 Sep 7, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2022

🎉 This PR is included in version 6.21.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

if (row[modelField.fieldName] && typeof row[modelField.fieldName] === 'string') {
// JSON fields for MariaDB server 10.5.2+ already results in JSON format, skip JSON.parse
// this is due to this https://jira.mariadb.org/browse/MDEV-17832 and how mysql2 connector interacts with MariaDB and JSON fields
if (row[modelField.fieldName] && typeof row[modelField.fieldName] === 'string' && !this.connection.info.hasMinVersion(10, 5, 2)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is not correct:

encrypt94@7e6c9f7

Copy link
Member

Choose a reason for hiding this comment

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

Do I understand correctly that the commit by @encrypt94 is more correct than the current code? If so; feel free to open a PR for this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've investigated, that's true, that commit is more correct.
The type of field returned by mariadb does not depend on the version of mariadb, but on the version of mariadb WHEN the table was created!

https://mariadb.com/kb/en/result-set-packets/#field-types
For a JSON column, the column type field will be MYSQL_TYPE_STRING, but the extended type will indicate 'json'.

the connector use this field to decide to parse or not the response:
https://github.com/mariadb-corporation/mariadb-connector-nodejs/blob/59da962ce4ae6338bb14ed8854a70526b429911f/lib/cmd/decoder/text-decoder.js#L98

during a select the connector put fields details inside a "meta" key that could be used to check if returned value from the connector is already parsed or not:
https://github.com/mariadb-corporation/mariadb-connector-nodejs/blob/f25081715c9e1f24a10017e70787108d0d2ced01/documentation/promise-api.md#array-result-sets

a gist to test things:
https://gist.github.com/lesion/af0c7f1370ab6eb05cafdabd5b39ac33

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

3 participants