Return an error for a batch insert whose VALUES group never closes

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

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)
 	}

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)
+		}
+	}
+}

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.