INSERT column sorting is broken

[benpaxton-hf]

395bcf3 adds sorting to the builder.Insert function, but sorts incorrectly, resulting in mangled column/value mapping (e.g. an Insert(builder.Eq{"y":0, "x":1}) may result in INSERT INTO foo (x, y) VALUES (0, 1) instead of the correct (1, 0)).

builder/builder.go

Lines 278 to 281 in ef80bec

	sort.Slice(b.insertVals, func(i, j int) bool {
	return b.insertCols[i] < b.insertCols[j]
	})
	sort.Strings(b.insertCols)

Due to how the "sort" package works, Swap() and Less() are called interleaved - using sort.Slice() where the Less() function uses the original order of the slice won't work - it needs to use the current order.

See https://play.golang.org/p/ITcqKVBvdbW for an example of how this goes wrong - both keys and values are in the same order to start with, but due to Less() using the original order, the sorting puts the values slice in a semi-random order dependent on the original order of the keys slice. Uncommenting line 26, i.e. keeping the keys and values slices in the same order as the sorting algorithm progresses, fixes the sort (and makes the sort.Ints() call redundant as a bonus!)

[benpaxton-hf]

Example failing test:

diff --git a/builder_insert_test.go b/builder_insert_test.go
index 5ea8662..ef3db52 100644
--- a/builder_insert_test.go
+++ b/builder_insert_test.go
@@ -15,6 +15,10 @@ func TestBuilderInsert(t *testing.T) {
        assert.NoError(t, err)
        assert.EqualValues(t, "INSERT INTO table1 (c,d) Values (1,2)", sql)
 
+       sql, err = Insert(Eq{"e": 3}, Eq{"c": 1}, Eq{"d": 2}).Into("table1").ToBoundSQL()
+       assert.NoError(t, err)
+       assert.EqualValues(t, "INSERT INTO table1 (c,d,e) Values (1,2,3)", sql)
+
        sql, err = Insert(Eq{"c": 1, "d": Expr("SELECT b FROM t WHERE d=? LIMIT 1", 2)}).Into("table1").ToBoundSQL()
        assert.NoError(t, err)
        assert.EqualValues(t, "INSERT INTO table1 (c,d) Values (1,(SELECT b FROM t WHERE d=2 LIMIT 1))", sql)

$ go test -v -run TestBuilderInsert
=== RUN   TestBuilderInsert
--- FAIL: TestBuilderInsert (0.00s)
    builder_insert_test.go:20: 
        	Error Trace:	builder_insert_test.go:20
        	Error:      	Not equal: 
        	            	expected: "INSERT INTO table1 (c,d,e) Values (1,2,3)"
        	            	actual  : "INSERT INTO table1 (c,d,e) Values (1,3,2)"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-INSERT INTO table1 (c,d,e) Values (1,2,3)
        	            	+INSERT INTO table1 (c,d,e) Values (1,3,2)
        	Test:       	TestBuilderInsert
FAIL
exit status 1
FAIL	github.com/go-xorm/builder	0.006s

[miketonks]

This seems quite a serious bug for us. We have pinned to 0.3.1 but would be nice to fix.

Options seem to be to either remove the sort or fix it. If you indicate which option is preferred or other issues to consider, we could work on a PR.

[BetaCat0]

@benpaxton-hf Thanks for reporting!

[BetaCat0]

@miketonks Thank you for reporting and you can make a PR to fix it. 😄

[lunny]

@benpaxton-hf @miketonks @BetaCat0 #42 will resolve this issue, please confirm.