From fcd3076bb5c47ef7e1c49d4dab376b4da586c32b Mon Sep 17 00:00:00 2001 From: August Vi Date: Mon, 19 Oct 2026 14:52:11 -0700 Subject: [PATCH] Return an error for a batch insert whose VALUES group never closes, and skip brackets inside literals and comments when expanding it. --- CHANGELOG.md | 15 ++++++++ named.go | 101 +++++++++++++++++++++++++++++++------------------- named_test.go | 72 +++++++++++++++++++++++++++++++++++ scan.go | 100 ++++++++++++++++++++++++------------------------- 4 files changed, 199 insertions(+), 89 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7d0c790..d174692 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,21 @@ Notable changes to this fork, newest first. An entry that imports an upstream pull request names its number and author. Releases up to v1.4.0 are releases of upstream [jmoiron/sqlx](https://github.com/jmoiron/sqlx). +## Unreleased + +### Fixes + +- A batch insert (a slice or array passed to `NamedExec`, `BindNamed` and the + like) whose `VALUES (` group never closes now returns the error + `missing closing bracket in VALUES`, whatever the number of rows. Before, the + query came back unexpanded with every row's arguments, and the mistake only + surfaced when the database rejected the argument count. Single-struct and map + arguments are unchanged. +- The batch insert expansion skips brackets inside string literals, quoted + identifiers and comments. `VALUES (:a, ')')` or `VALUES (:a, /* ) */ :b)` + with several rows used to be cut at the bracket inside the literal or comment + and expanded into invalid SQL; `VALUES (:a, '(')` was not expanded at all. + ## v1.4.2 (2026-10-19) No API change. sqlx's `go.mod` now requires no other module, so importing sqlx diff --git a/named.go b/named.go index 26d6670..8305081 100644 --- a/named.go +++ b/named.go @@ -237,63 +237,85 @@ func bindStruct(bindType int, query string, arg interface{}, m *reflectx.Mapper) var valuesReg = regexp.MustCompile(`(?i)\WVALUES\s*\(`) -func findMatchingClosingBracketIndex(s string) int { - count := 0 - for i, ch := range s { - if ch == '(' { - count++ - } - if ch == ')' { - count-- - if count == 0 { +// errUnclosedValues is returned when a batch insert's VALUES group, the one +// holding the bind variables, has no closing bracket. +var errUnclosedValues = errors.New("missing closing bracket in VALUES") + +// matchingCloseParen returns the index of the ')' that closes the '(' at +// q[open], or -1 when there is none. Brackets inside string literals, quoted +// identifiers, comments and dollar-quoted strings are skipped. +func matchingCloseParen(q string, open int) int { + depth := 0 + for i := open; i < len(q); { + switch q[i] { + case '(': + depth++ + i++ + case ')': + depth-- + if depth == 0 { return i } + i++ + case '\'', '"', '`', '-', '/', '$': + i = skipOpaque(q, i, 0) + default: + i++ } } - return 0 + return -1 } -// fixBound repeats the VALUES group of a batch insert loop-1 more times. -// The group chosen is the first one that contains a bind variable, so that a -// table named "values" or MySQL's VALUES(col) in an ON DUPLICATE KEY UPDATE -// clause is not mistaken for it. When no group contains a bind variable the -// first match is used, as before. -func fixBound(bound string, loop int) string { - locs := valuesReg.FindAllStringIndex(bound, -1) - // defensive guard when "VALUES (...)" not found - if len(locs) == 0 { - return bound - } - - chosen := 0 - for i, loc := range locs { - openingBracketIndex := loc[1] - 1 - index := findMatchingClosingBracketIndex(bound[openingBracketIndex:]) - if index == 0 { +// valuesGroup finds the VALUES group of a batch insert and returns the indexes +// of its brackets. The group is the first one whose brackets close and that +// contains a bind variable, so that a table named "values" or MySQL's +// VALUES(col) in an ON DUPLICATE KEY UPDATE clause is not mistaken for it; +// when no closed group contains a bind variable the first closed group is +// used. ok is false when the query has no closed VALUES group. err is +// errUnclosedValues when no closed group holds a bind variable but an +// unclosed one does. +func valuesGroup(bound string) (openIdx, closeIdx int, ok bool, err error) { + unclosedBind := false + for _, loc := range valuesReg.FindAllStringIndex(bound, -1) { + o := loc[1] - 1 + c := matchingCloseParen(bound, o) + if c < 0 { + if strings.ContainsAny(bound[o:], "?$:@") { + unclosedBind = true + } continue } - if strings.ContainsAny(bound[openingBracketIndex:openingBracketIndex+index+1], "?$:@") { - chosen = i - break + if strings.ContainsAny(bound[o:c+1], "?$:@") { + return o, c, true, nil + } + if !ok { + openIdx, closeIdx, ok = o, c, true } } + if unclosedBind { + return 0, 0, false, errUnclosedValues + } + return openIdx, closeIdx, ok, nil +} - openingBracketIndex := locs[chosen][1] - 1 - index := findMatchingClosingBracketIndex(bound[openingBracketIndex:]) - // defensive guard. must have closing bracket - if index == 0 { +// fixBound repeats the VALUES group of a batch insert loop-1 more times. A +// query without a usable VALUES group is returned unchanged; bindArray rejects +// an unclosed group before it gets here. +func fixBound(bound string, loop int) string { + openIdx, closeIdx, ok, err := valuesGroup(bound) + if !ok || err != nil { return bound } - closingBracketIndex := openingBracketIndex + index + 1 + end := closeIdx + 1 var buffer bytes.Buffer - buffer.WriteString(bound[0:closingBracketIndex]) + buffer.WriteString(bound[0:end]) for i := 0; i < loop-1; i++ { buffer.WriteString(",") - buffer.WriteString(bound[openingBracketIndex:closingBracketIndex]) + buffer.WriteString(bound[openIdx:end]) } - buffer.WriteString(bound[closingBracketIndex:]) + buffer.WriteString(bound[end:]) return buffer.String() } @@ -319,6 +341,9 @@ func bindArray(bindType int, query string, arg interface{}, m *reflectx.Mapper) } arglist = append(arglist, elemArglist...) } + if _, _, _, err := valuesGroup(bound); err != nil { + return "", []interface{}{}, err + } if arrayLen > 1 { bound = fixBound(bound, arrayLen) } diff --git a/named_test.go b/named_test.go index 12a24cd..2618c95 100644 --- a/named_test.go +++ b/named_test.go @@ -279,6 +279,30 @@ WHERE t.id = u.id expect: `INSERT INTO foo (a,b) settings async_insert=1, wait_for_async_insert=1 VALUES(:a, :b),(:a, :b)`, loop: 2, }, + { + name: `bracket inside a string literal in the group`, + query: `INSERT INTO foo (a,b) VALUES (?, ')')`, + expect: `INSERT INTO foo (a,b) VALUES (?, ')'),(?, ')')`, + loop: 2, + }, + { + name: `opening bracket inside a string literal in the group`, + query: `INSERT INTO foo (a,b) VALUES (?, '(') RETURNING id`, + expect: `INSERT INTO foo (a,b) VALUES (?, '('),(?, '(') RETURNING id`, + loop: 2, + }, + { + name: `bracket inside a comment in the group`, + query: "INSERT INTO foo (a,b) VALUES (?, /* ) */ ?)", + expect: "INSERT INTO foo (a,b) VALUES (?, /* ) */ ?),(?, /* ) */ ?)", + loop: 2, + }, + { + name: `unclosed group is left unchanged`, + query: `INSERT INTO foo (a,b) VALUES (?, ?`, + expect: `INSERT INTO foo (a,b) VALUES (?, ?`, + loop: 2, + }, } for _, tc := range table { @@ -290,3 +314,51 @@ WHERE t.id = u.id }) } } + +// A batch insert whose VALUES group never closes is an error, whatever the +// number of rows, instead of SQL that fails later at the database. +func TestBindArrayUnclosedValues(t *testing.T) { + type contact struct { + ID int `db:"id"` + Name string `db:"name"` + } + one := []contact{{1, "Bob"}} + two := []contact{{1, "Bob"}, {2, "Cat"}} + for _, tc := range []struct { + name string + query string + arg interface{} + }{ + {"one row", `INSERT INTO foo (id, name) VALUES(:id, :name`, one}, + {"two rows", `INSERT INTO foo (id, name) VALUES(:id, :name`, two}, + {"pointer to slice", `INSERT INTO foo (id, name) VALUES (:id, :name`, &two}, + {"bracket in a literal", `INSERT INTO foo (id, name) VALUES (:id, :name, '('`, two}, + {"closed group without binds", `INSERT INTO foo (id, name) VALUES () VALUES (:id, :name`, two}, + } { + for _, bt := range []int{QUESTION, DOLLAR} { + q, args, err := BindNamed(bt, tc.query, tc.arg) + if err != errUnclosedValues || err.Error() != "missing closing bracket in VALUES" { + t.Errorf("%s (bind type %d): got q=%q args=%d err=%v, want %v", tc.name, bt, q, len(args), err, errUnclosedValues) + } + } + } + + // Unchanged: balanced groups, queries without VALUES, and single-struct or + // map arguments, which do not take the batch path. + for _, tc := range []struct { + name, query, want string + arg interface{} + }{ + {"balanced", `INSERT INTO foo (id, name) VALUES (:id, :name)`, `INSERT INTO foo (id, name) VALUES (?, ?),(?, ?)`, two}, + {"literal bracket", `INSERT INTO foo (id, name) VALUES (:id, ')')`, `INSERT INTO foo (id, name) VALUES (?, ')'),(?, ')')`, two}, + {"upsert with VALUES(col)", `INSERT INTO foo (id, name) VALUES (:id, :name) ON DUPLICATE KEY UPDATE name = VALUES(name`, `INSERT INTO foo (id, name) VALUES (?, ?),(?, ?) ON DUPLICATE KEY UPDATE name = VALUES(name`, two}, + {"no VALUES", `UPDATE foo SET name = :name WHERE id = :id`, `UPDATE foo SET name = ? WHERE id = ?`, one}, + {"struct", `INSERT INTO foo (id, name) VALUES(:id, :name`, `INSERT INTO foo (id, name) VALUES(?, ?`, contact{1, "Bob"}}, + {"map", `INSERT INTO foo (id, name) VALUES(:id, :name`, `INSERT INTO foo (id, name) VALUES(?, ?`, map[string]interface{}{"id": 1, "name": "Bob"}}, + } { + q, _, err := BindNamed(QUESTION, tc.query, tc.arg) + if err != nil || q != tc.want { + t.Errorf("%s: got q=%q err=%v, want %q", tc.name, q, err, tc.want) + } + } +} diff --git a/scan.go b/scan.go index 74a0612..64c91c1 100644 --- a/scan.go +++ b/scan.go @@ -91,62 +91,60 @@ func (s *sqlScanner) next() (kind tokKind, start, end int) { return tokName, i, j } i++ // ':=' , ' : ', ':1', ':)' ... are plain text - case '\'': - // E'...' (Postgres escape string): backslash escapes are certain. - esc := i > 0 && (q[i-1] == 'E' || q[i-1] == 'e') && (i < 2 || !isIdentByte(q[i-2])) - if j, ok := scanQuoted(q, i+1, '\'', esc); ok { - i = j - } else { - i++ // unterminated: not a string after all, treat the quote as text - } - case '"': - if j, ok := scanQuoted(q, i+1, '"', false); ok { - i = j - } else { - i++ - } - case '`': - if j, ok := scanBacktick(q, i+1); ok { - i = j - } else { - i++ - } - case '[': - if s.opt&optBrackets != 0 { - if j, ok := scanBracket(q, i+1); ok { - i = j - continue - } - } - i++ - case '-': - if i+1 < n && q[i+1] == '-' { - if j := strings.IndexByte(q[i:], '\n'); j >= 0 { - i += j + 1 - } else { - i = n - } - } else { - i++ + default: + i = skipOpaque(q, i, s.opt) + } + } + s.pos = n + return tokEOF, n, n +} + +// skipOpaque returns the offset just past the string literal, quoted +// identifier, comment or dollar-quoted string that starts at q[i], or i+1 when +// nothing opaque starts there (or it is unterminated, in which case the opening +// byte is plain text). Brackets are opaque only with optBrackets. +func skipOpaque(q string, i int, opt scanOpts) int { + n := len(q) + switch q[i] { + case '\'': + // E'...' (Postgres escape string): backslash escapes are certain. + esc := i > 0 && (q[i-1] == 'E' || q[i-1] == 'e') && (i < 2 || !isIdentByte(q[i-2])) + if j, ok := scanQuoted(q, i+1, '\'', esc); ok { + return j + } + case '"': + if j, ok := scanQuoted(q, i+1, '"', false); ok { + return j + } + case '`': + if j, ok := scanBacktick(q, i+1); ok { + return j + } + case '[': + if opt&optBrackets != 0 { + if j, ok := scanBracket(q, i+1); ok { + return j } - case '/': - if i+1 < n && q[i+1] == '*' { - i = skipBlockComment(q, i+2) - } else { - i++ + } + case '-': + if i+1 < n && q[i+1] == '-' { + if j := strings.IndexByte(q[i:], '\n'); j >= 0 { + return i + j + 1 } - case '$': - if i == 0 || !isIdentByte(q[i-1]) { - if j, ok := scanDollarQuote(q, i); ok { - i = j - continue - } + return n + } + case '/': + if i+1 < n && q[i+1] == '*' { + return skipBlockComment(q, i+2) + } + case '$': + if i == 0 || !isIdentByte(q[i-1]) { + if j, ok := scanDollarQuote(q, i); ok { + return j } - i++ } } - s.pos = n - return tokEOF, n, n + return i + 1 } // scanQuoted scans a '...' or "..." literal whose opening quote is at i-1.