NULL literals in prepared statements

[marshall-mcmullen]

goqu rocks :-)

So..I may be doing this wrong or have incorrect expectations and if so please set me straight :-).

I'm trying to use goqu to create prepared statements. I saw #127 which was super helpful in getting me going. However, it seems that UpdateDataset is interpolating NULL values even though I used .Prepared(true) which as I understand it should cause it to not interpolate the values so the expression can be reused with different parameters in the future. Here's a concrete example:

func TestUpdatePreparedWithNullValue(t *testing.T) {

    dialect := goqu.Dialect("postgres")

    var value1 *int = nil 

    updateDataset := dialect.Update("foo").Prepared(true).Set(map[string]interface{}{
        "value1": value1,
    })  

    sql, _, _ := updateDataset.ToSQL()
    assert.Equal(t, `UPDATE "foo" SET "value1"=$1`, sql)
}

And here's the test output:

        	Error Trace:	user_test.go:95
        	Error:      	Not equal: 
        	            	expected: "UPDATE \"foo\" SET \"value1\"=$1"
        	            	actual  : "UPDATE \"foo\" SET \"value1\"=NULL"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-UPDATE "foo" SET "value1"=$1
        	            	+UPDATE "foo" SET "value1"=NULL
        	Test:       	TestUpdatePreparedWithNullValue

[doug-martin]

Yeah it should be changed to not interpolate nil, or boolean values.

This is an artifact approaching the builder from interpolation first.

[marshall-mcmullen]

That makes sense. Thanks for the confirmation @doug-martin. I've worked around this locally again so I'm not blocked on this... But would definitely like to see it fixed as the workaround is kinda gross :)

[marshall-mcmullen]

Any chance of this getting fixed in the next week or so @doug-martin ? I'd like to start memoizing prepared statements in our system and I think that this will be a blocker for that. If you don't think you can get to it, but can point me in the right direction I'd be happy to start working on a fix.

[doug-martin]

@marshallmcmullen Yeah Im not sure how soon Ill be able to get to this.

If you want to take a stab at this most of the changes should be isolated to sqlgen/expression_sql_generator.go.

To get started you should take a look at

1. placeHolderSQL this method is what will generate the place holder for a type.
2. literalNil This method is what serialized nil into NULL.
3. literalBool This method turns bools into TRUE or FALSE.

You can take a look at literalInt for an example of how to use placeholders instead of the interpolated value.

Now for the tricky part :) Currently goqu will turn any (in)equality on nil or bool value into an IS expression. I think if goqu continues to use this strategy users may end up with unintended SQL generation when the value could be a non-nil value. To change this behavior checkBoolExpType should be changed or potentially removed, not sure yet.

Changing this will warrant a major version change as it wont be backwards compatible.

If you have any ideas please share them!

[marshall-mcmullen]

@doug-martin Your instructions were spot-on. I think I've gotten a PR ready that addresses this issue based on my understanding: #151

The harder part, as you envisioned, is what to do about checkBoolExpType. I modified it to no longer turn an (in)equality on nil or bool into an IS expression. This seemed like the most obvious path forward but I can see how this might be a big change to swallow. FWIW, I do feel like this led to more consistent SQL generation.

[marshall-mcmullen]

@doug-martin I've pushed some updates to a new PR for this. If you can take a look when you have time I would appreciate it greatly. Thanks!

[doug-martin]

Published!

[marshall-mcmullen]

@doug-martin woohoo! Very glad we got this one pushed across the finish line. Thanks so much for your help! I just pulled this new tagged version into our product and verified everything is working as expected now. Awesome!!