rebind safety #775
rebind safety #775
Conversation
…is not safe with respect to string literals and comments. As someone who routinely comments their SQL, this seemed really danagerous, especially the the danager of having a question mark inside a comment or string literal. For another project, I needed to do some SQL tokenizing and after writing a tokenizer, I thought to apply it to sqlx too. When doing so, I was not able to match the speed of the existing sqlx code. On the other hand, the existing code is not safe. In compileNmaedQuery, I discoverd the old code rewrites "::" to ":". That behavior is not documented anywhere. I imagine that it is quite a surprise to someone who is doing a type cast to discover it doesn't work. On the other hand, fixing that behavior, as I've done, will break code so accepting my change requires a major version change even though the behavior changed isn't documented. As mentioned, I couldn't match the speed. I've tried to make sqltoken fast. % go test -bench=CompileNamed\|Rebind -benchmem goos: linux goarch: amd64 pkg: github.com/jmoiron/sqlx cpu: AMD Ryzen 7 3700X 8-Core Processor BenchmarkRebind-16 360132 3374 ns/op 5040 B/op 10 allocs/op BenchmarkOldRebind-16 3341792 358.3 ns/op 336 B/op 4 allocs/op BenchmarkRebindBuffer-16 1000000 1018 ns/op 592 B/op 6 allocs/op BenchmarkCompileNamedQuery-16 237378 4995 ns/op 7504 B/op 28 allocs/op BenchmarkOldCompileNamedQuery-16 264847 4520 ns/op 2064 B/op 40 allocs/op PASS ok github.com/jmoiron/sqlx 11.112s
| @@ -282,7 +282,7 @@ func bindArray(bindType int, query string, arg interface{}, m *reflectx.Mapper) | |||
| if arrayLen == 0 { | |||
| return "", []interface{}{}, fmt.Errorf("length of array is 0: %#v", arg) | |||
| } | |||
| var arglist = make([]interface{}, 0, len(names)*arrayLen) | |||
| arglist := make([]interface{}, 0, len(names)*arrayLen) | |||
There was a problem hiding this comment.
This change was made by gofmt and so were a bunch of other similar changes.
There was a problem hiding this comment.
I think you're using gofumpt rather than gofmt? gofmt doesn't make these kind of changes anyway.
| @@ -30,15 +31,15 @@ func TestCompileQuery(t *testing.T) { | |||
| V: []string{"name1", "name2"}, | |||
| }, | |||
| { | |||
| Q: `SELECT "::foo" FROM a WHERE first_name=:name1 AND last_name=:name2`, | |||
| Q: `SELECT ":foo" FROM a WHERE first_name=:name1 AND last_name=:name2`, | |||
There was a problem hiding this comment.
I assume idea here is/was that ::foo would "escape" the :foo; sqltoken is smarter about this as it recognizes the quotes so you don't need to use ::foo – which is clearly better – but this is an incompatible change that will break people's SQL if they're using it like this.
As an aside, this library is "sporadically maintained", at best, so I'm not expecting a merge or feedback from the maintainer(s) any time soon, so I wouldn't rush to fix this. I'm (probably) going to use in my own library/fork I'm working on though. Just wanted to point this out.
Also might be nice to replace those four calls to testify in your library? Don't want to start a discussion about testify vs. stdlib tests, but smaller dependency trees are always nice, and with just four calls it's not that much benefit 😅
There was a problem hiding this comment.
I called this out in my PR description. Since this is not backwards compatible it requires a major version change. The current behavior isn't documented, but people may still be relying on it.
If getting rid of the calls to testify would mean this gets merged, I would certainly do it.
There was a problem hiding this comment.
Oh sorry, I missed that 😅
If getting rid of the calls to testify would mean this gets merged, I would certainly do it.
I have no say in the matter at all; just something I noticed.
dlsniper
added
the
requires attention
I happened to notice a while ago that the way sqlx rebinds variables is not safe
with respect to string literals and comments. As someone who routinely comments their
SQL, this seemed really dangerous, especially the the danger of having a question
mark inside a comment or string literal.
For another project, I needed to do some SQL tokenizing and after writing a tokenizer,
I thought to apply it to sqlx too.
When doing so, I was not able to match the speed of the existing sqlx code. On the other
hand, the existing code is not safe.
In compileNamedQuery, I discovered the old code rewrites "::" to ":". That behavior is not
documented anywhere. I imagine that it is quite a surprise to someone who is doing a
type cast to discover it doesn't work. On the other hand, fixing that behavior, as I've
done, will break code so accepting my change requires a major version change even though
the behavior changed isn't documented.
As mentioned, I couldn't match the speed. I've tried to make sqltoken fast.