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(abstract): patch jsonb operator for pg if value is json #13780

Merged
merged 11 commits into from Dec 22, 2021

Conversation

Drethic
Copy link
Contributor

@Drethic Drethic commented Dec 16, 2021

Pull Request Checklist

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

  • Have you added new tests to prevent regressions?
  • Does npm run test or npm run 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

Issue: When querying a JSONB column in PG and the data stored in the column is an Object PG uses #>> when it should be #>

Updated abstract/query-generator to check the operator and value type while running _traverseJSON. If the value is a string and the operator is Op.contains it attempts to parse the value as JSON. If it doesn't fail it sends true to jsonPathExtractionQuery new third parameter isJson. This is only used in postgres so it knows to use #> instead of #>> to join the query string.

Drethic and others added 3 commits December 16, 2021 12:52
Added check to traverseJSON that checks if value is JSON and passes
result to jsonPathExtrationQuery

Added isJson param to jsonPathExtractionQuery

Updated postgres JSON join arrow to change if value is JSON

 with '#' will be ignored, and an empty message aborts the commit.
Added check if value is string and operator is contains

Updated unit tests with new expected outputs
@sdepold
Copy link
Member

sdepold commented Dec 21, 2021

Could you add a test that verifies the new query?

sdepold and others added 6 commits December 21, 2021 10:29
Added tests to validate jsonPathExtractionQuery works with and without
isJSON
Removed extra bracket from merge conflict
Updated query generator tests to handle all supported DB types
@Drethic
Copy link
Contributor Author

Drethic commented Dec 21, 2021

Could you add a test that verifies the new query?

Tests have been added and catches put in place for mssql and snowflake to ensure CI passes.

@WikiRik WikiRik requested a review from sdepold December 21, 2021 21:12
@sdepold sdepold merged commit a2375c5 into sequelize:main Dec 22, 2021
@sdepold
Copy link
Member

sdepold commented Dec 22, 2021

👍

@github-actions
Copy link
Contributor

🎉 This PR is included in version 6.12.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@TrurlMcByte
Copy link

in this case

item = this.jsonPathExtractionQuery(identifier, path);
also must be like
item = this.jsonPathExtractionQuery(identifier, path, true);
because "order by json" is better as "order by json converted to text, including integer and float"

@sdepold
Copy link
Member

sdepold commented Dec 30, 2021

Wanna file a pr?

aliatsis pushed a commit to creditiq/sequelize that referenced this pull request Jun 2, 2022
…e#13780)

* fix(abstract): dialog query-generator for JSON in PG

Added check to traverseJSON that checks if value is JSON and passes
result to jsonPathExtrationQuery

Added isJson param to jsonPathExtractionQuery

Updated postgres JSON join arrow to change if value is JSON

 with '#' will be ignored, and an empty message aborts the commit.

* fix(abstract): updated unit tests

Added check if value is string and operator is contains

Updated unit tests with new expected outputs

* fix(abstract): add query generator unit tests

Added tests to validate jsonPathExtractionQuery works with and without
isJSON

* fix(abstract): Fix merge error

Removed extra bracket from merge conflict

* fix(abstract): Patch failing tests

Updated query generator tests to handle all supported DB types

* fix(tests): update test name

Co-authored-by: Sascha Depold <sdepold@users.noreply.github.com>
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