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

Do not interpolate literals when prepared=true. #151

Closed
wants to merge 1 commit into from
Closed

Do not interpolate literals when prepared=true. #151

wants to merge 1 commit into from

Conversation

marshall-mcmullen
Copy link
Contributor

This addresses #132. The gist
of this fix is to modify the builders to not interpolate nil and boolean
values so that prepared statements can work properly. This makes
prepared=true work more consistently across all types.

The scope of this change:

(1) Modify literalNil to use a placeholder if prepared=true similar to
literalBool and literalInt

(2) Modify checkBoolExpType to not turn (in)equality on nil or bool
values to an IS expression.

(3) LOTS of tests and examples updated to reflect the new behavior.

This addresses #132. The gist
of this fix is to modify the builders to not interpolate nil and boolean
values so that prepared statements can work properly. This makes
prepared=true work more consistently across all types.

The scope of this change:

(1) Modify literalNil to use a placeholder if prepared=true similar to
literalBool and literalInt

(2) Modify checkBoolExpType to not turn (in)equality on nil or bool
values to an `IS` expression.

(3) LOTS of tests and examples updated to reflect the new behavior.
@marshall-mcmullen
Copy link
Contributor Author

I tested these changes locally with:

  • ./go.test.sh
  • go test -v -race ./...
  • GO_VERSION=latest POSTGRES_VERSION=11.4 MYSQL_VERSION=8.0.12 docker-compose run goqu

@marshall-mcmullen
Copy link
Contributor Author

I don't know why the file vendor/github.com/mattn/go-sqlite3/sqlite3-binding.c is showing as modified. It looks to me locally identical to what is in master. Is this expected?

@doug-martin doug-martin mentioned this pull request Sep 4, 2019
// ds.Where(I("a").IsNot(nil)) //("a" IS NOT NULL)
// ds.Where(I("a").IsNot(true)) //("a" IS NOT TRUE)
// ds.Where(I("a").IsNot(false)) //("a" IS NOT FALSE)
// ds.Where(I("a").IsNot(nil)) //("a" != NULL)
Copy link
Owner

Choose a reason for hiding this comment

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

These shouldn't be changed because it is using the Is methods.

@@ -151,31 +151,31 @@ func (swfet *sqlWindowFunctionExpressionTest) TestAllOthers() {

expIsNull := wf.IsNull()
swfet.Equal(expIsNull.LHS(), wf)
swfet.Equal(expIsNull.Op(), IsOp)
swfet.Equal(expIsNull.Op(), EqOp)
Copy link
Owner

Choose a reason for hiding this comment

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

This should be reverted to work like it did previously. See https://stackoverflow.com/questions/9822154/standard-sql-boolean-operator-is-vs-equals-operator is and equality work differently for NULL, TRUE and FALSE.

@@ -151,31 +151,31 @@ func (swfet *sqlWindowFunctionExpressionTest) TestAllOthers() {

expIsNull := wf.IsNull()
swfet.Equal(expIsNull.LHS(), wf)
swfet.Equal(expIsNull.Op(), IsOp)
swfet.Equal(expIsNull.Op(), EqOp)
Copy link
Owner

Choose a reason for hiding this comment

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

The is methods should work like they did previously. Take a look at https://stackoverflow.com/questions/9822154/standard-sql-boolean-operator-is-vs-equals-operator it explains the difference pretty well.

@doug-martin
Copy link
Owner

After reading through https://stackoverflow.com/questions/9822154/standard-sql-boolean-operator-is-vs-equals-operator we may want to test this out some more especially with NULL values, TRUE and FALSE will work in the case that the column is not UNKNOWN.

Here is a simple example you can run in postgres to demonstrate the problem.

select
       NULL = NULL as eq_null,
       NULL = TRUE as eq_true,
       NULL = FALSE as eq_false,
       NULL IS NULL as is_null,
       NULL IS NOT NULL as is_not_null,
       NULL IS TRUE as is_true,
       NULL IS NOT TRUE is_not_true,
       NULL IS FALSE as is_false,
       NULL IS NOT FALSE is_not_false;
select
       TRUE = NULL as eq_null,
       TRUE = TRUE as eq_true,
       TRUE = FALSE as eq_false,
       TRUE IS NULL as is_null,
       TRUE IS NOT NULL as is_not_null,
       TRUE IS TRUE as is_true,
       TRUE IS NOT TRUE is_not_true,
       TRUE IS FALSE as is_false,
       TRUE IS NOT FALSE is_not_false;

select
       FALSE = NULL as eq_null,
       FALSE = TRUE as eq_true,
       FALSE = FALSE as eq_false,
       FALSE IS NULL as is_null,
       FALSE IS NOT NULL as is_not_null,
       FALSE IS TRUE as is_true,
       FALSE IS NOT TRUE is_not_true,
       FALSE IS FALSE as is_false,
       FALSE IS NOT FALSE is_not_false;

My real concern here is NULL as goqu would effectively allow you to create SQL that does not function as one would expect, same for boolean but to a lesser extent.

@marshall-mcmullen
Copy link
Contributor Author

Thanks for the detailed review and feedback @doug-martin. I wanted to get the SetError PR finished up first (now done!). I'll pick this one up tonight as I'd like to try to address all the issues you brought up. Thanks again!

@doug-martin
Copy link
Owner

doug-martin commented Sep 6, 2019

Im closing this for now please re-open once you have it figure out. If you have any further questions don't hesitate to reach out.

@doug-martin doug-martin closed this Sep 6, 2019
marshall-mcmullen pushed a commit to marshall-mcmullen/goqu that referenced this pull request Sep 10, 2019
This addresses doug-martin#132

It is a reset of closed PR: doug-martin#151

The gist of this fix is to modify the builders to not interpolate
nil and boolean values so that prepared statements can work properly.
This makes prepared=true work more consistently across all types.

The scope of this change:

(1) Modify literalNil to use a placeholder if prepared=true similar to
literalBool and literalInt

(2) Modify checkBoolExpType to not turn (in)equality on nil or bool
values to an IS expression.

(3) LOTS of tests and examples updated to reflect the new behavior.

(4) Fix bugs from doug-martin#151 found by
@doug-martin to preserve correct behavior with Is/IsNot and not
incorrectly map them to Eq/NotEq.
marshall-mcmullen pushed a commit to marshall-mcmullen/goqu that referenced this pull request Sep 10, 2019
This addresses doug-martin#132

It is a reset of closed PR: doug-martin#151

The gist of this fix is to modify the builders to not interpolate
nil and boolean values so that prepared statements can work properly.
This makes prepared=true work more consistently across all types.

The scope of this change:

(1) Modify literalNil to use a placeholder if prepared=true similar to
literalBool and literalInt

(2) Modify checkBoolExpType to not turn (in)equality on nil or bool
values to an IS expression.

(3) LOTS of tests and examples updated to reflect the new behavior.

(4) Fix bugs from doug-martin#151 found by
@doug-martin to preserve correct behavior with Is/IsNot and not
incorrectly map them to Eq/NotEq.

(5) Fix incorrect interpoloation for DEFAULT also.
marshall-mcmullen pushed a commit to marshall-mcmullen/goqu that referenced this pull request Sep 10, 2019
This addresses doug-martin#132

It is a reset of closed PR: doug-martin#151

The gist of this fix is to modify the builders to not interpolate
nil and boolean values so that prepared statements can work properly.
This makes prepared=true work more consistently across all types.

The scope of this change:

(1) Modify literalNil to use a placeholder if prepared=true similar to
literalBool and literalInt

(2) Modify checkBoolExpType to not turn (in)equality on nil or bool
values to an IS expression.

(3) LOTS of tests and examples updated to reflect the new behavior.

(4) Fix bugs from doug-martin#151 found by
@doug-martin to preserve correct behavior with Is/IsNot and not
incorrectly map them to Eq/NotEq.
marshall-mcmullen pushed a commit to marshall-mcmullen/goqu that referenced this pull request Sep 10, 2019
This addresses doug-martin#132

It is a reset of closed PR: doug-martin#151

The gist of this fix is to modify the builders to not interpolate
nil and boolean values so that prepared statements can work properly.
This makes prepared=true work more consistently across all types.

The scope of this change:

(1) Modify literalNil to use a placeholder if prepared=true similar to
literalBool and literalInt

(2) Modify checkBoolExpType to not turn (in)equality on nil or bool
values to an IS expression.

(3) LOTS of tests and examples updated to reflect the new behavior.

(4) Fix bugs from doug-martin#151 found by
@doug-martin to preserve correct behavior with Is/IsNot and not
incorrectly map them to Eq/NotEq.
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

2 participants