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 whereJsonPath queries for postgres and cockroachdb #5011

Merged
merged 1 commit into from Feb 8, 2022

Conversation

jscek
Copy link
Contributor

@jscek jscek commented Feb 8, 2022

This PR fixes 2 bugs I've found working with where clauses build with 'whereJsonPath' for postgres database:

  1. If a passed value is a number, it is casted to integer or float on postgres side. However, is casted also if value only starts with a number but contains also letters or other characters. This is because parseInt and parseFloat are used for value check and they just ignore all non-numeric characters.
  2. Queries using 'orWhereJsonPath' don't have proper binding for value.

For 1st I've added additional check if value is not a NaN and for 2nd I've fixed arguments passed in query builder method.

Copy link
Collaborator

@OlivierCavadenti OlivierCavadenti left a comment

Choose a reason for hiding this comment

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

Thanks for the PR !
Some tests failed.

@jscek jscek changed the title Fix whereJsonPath queries for postgres Fix whereJsonPath queries for postgres and cockroachdb Feb 8, 2022
@jscek
Copy link
Contributor Author

jscek commented Feb 8, 2022

I'm a little confused why 'orWhereJsonPath' doesn't work for some dialects (sqlite, mysql) after my changes. I'll try to figure this out and create a new PR (or I'll report a bug if couldn't) so this one include fix only for 1st issue I've mentioned ;)

@OlivierCavadenti
Copy link
Collaborator

OlivierCavadenti commented Feb 8, 2022

I'm a little confused why 'orWhereJsonPath' doesn't work for some dialects (sqlite, mysql) after my changes. I'll try to figure this out and create a new PR (or I'll report a bug if couldn't) so this one include fix only for 1st issue I've mentioned ;)

Ok, I will merge this one. Can you make an issue for the second point ? I will take a look.

@OlivierCavadenti OlivierCavadenti merged commit 1461b9b into knex:master Feb 8, 2022
@kibertoad
Copy link
Collaborator

Released in 1.0.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants