Enhance interpolateParams to correctly handle placeholders in queries with comments, strings, and backticks.

* Add `findParamPositions` to identify real parameter positions
* Update and expand related tests.

Author: rusher

connection.go

@@ -172,7 +172,7 @@ func (mc *mysqlConn) close() {
 }
 
 // Closes the network connection and unsets internal variables. Do not call this
-// function after successfully authentication, call Close instead. This function
+// function after successful authentication, call Close instead. This function
 // is called before auth or on auth failure because MySQL will have already
 // closed the network connection.
 func (mc *mysqlConn) cleanup() {
@@ -245,9 +245,106 @@ func (mc *mysqlConn) Prepare(query string) (driver.Stmt, error) {
 	return stmt, err
 }
 
+// findParamPositions returns the positions of real parameter holders ('?') in the query, ignoring those in comments, strings, or backticks.
+func findParamPositions(query string) []int {
+	const (
+		stateNormal = iota
+		stateString
+		stateEscape
+		stateEOLComment
+		stateSlashStarComment
+		stateBacktick
+	)
+
+	var (
+		QUOTE_BYTE         = byte('\'')
+		DBL_QUOTE_BYTE     = byte('"')
+		BACKSLASH_BYTE     = byte('\\')
+		QUESTION_MARK_BYTE = byte('?')
+		SLASH_BYTE         = byte('/')
+		STAR_BYTE          = byte('*')
+		HASH_BYTE          = byte('#')
+		MINUS_BYTE         = byte('-')
+		LINE_FEED_BYTE     = byte('\n')
+		RADICAL_BYTE       = byte('`')
+	)
+
+	paramPositions := make([]int, 0)
+	state := stateNormal
+	singleQuotes := false
+	lastChar := byte(0)
+	lenq := len(query)
+	for i := 0; i < lenq; i++ {
+		currentChar := query[i]
+		if state == stateEscape && !((currentChar == QUOTE_BYTE && singleQuotes) || (currentChar == DBL_QUOTE_BYTE && !singleQuotes)) {
+			state = stateString
+			lastChar = currentChar
+			continue
+		}
+		switch currentChar {
+		case STAR_BYTE:
+			if state == stateNormal && lastChar == SLASH_BYTE {
+				state = stateSlashStarComment
+			}
+		case SLASH_BYTE:
+			if state == stateSlashStarComment && lastChar == STAR_BYTE {
+				state = stateNormal
+			} else if state == stateNormal && lastChar == SLASH_BYTE {
+				state = stateEOLComment
+			}
+		case HASH_BYTE:
+			if state == stateNormal {
+				state = stateEOLComment
+			}
+		case MINUS_BYTE:
+			if state == stateNormal && lastChar == MINUS_BYTE {
+				state = stateEOLComment
+			}
+		case LINE_FEED_BYTE:
+			if state == stateEOLComment {
+				state = stateNormal
+			}
+		case DBL_QUOTE_BYTE:
+			if state == stateNormal {
+				state = stateString
+				singleQuotes = false
+			} else if state == stateString && !singleQuotes {
+				state = stateNormal
+			} else if state == stateEscape {
+				state = stateString
+			}
+		case QUOTE_BYTE:
+			if state == stateNormal {
+				state = stateString
+				singleQuotes = true
+			} else if state == stateString && singleQuotes {
+				state = stateNormal
+			} else if state == stateEscape {
+				state = stateString
+			}
+		case BACKSLASH_BYTE:
+			if state == stateString {
+				state = stateEscape
+			}
+		case QUESTION_MARK_BYTE:
+			if state == stateNormal {
+				paramPositions = append(paramPositions, i)
+			}
+		case RADICAL_BYTE:
+			if state == stateBacktick {
+				state = stateNormal
+			} else if state == stateNormal {
+				state = stateBacktick
+			}
+		}
+		lastChar = currentChar
+	}
+	return paramPositions
+}
+
 func (mc *mysqlConn) interpolateParams(query string, args []driver.Value) (string, error) {
-	// Number of ? should be same to len(args)
-	if strings.Count(query, "?") != len(args) {
+	paramPositions := findParamPositions(query)
+	if len(paramPositions) != len(args) {
 		return "", driver.ErrSkip
 	}
 
@@ -261,21 +358,16 @@ func (mc *mysqlConn) interpolateParams(query string, args []driver.Value) (strin
 	}
 	buf = buf[:0]
 	argPos := 0
+	lastIdx := 0
 
-	for i := 0; i < len(query); i++ {
-		q := strings.IndexByte(query[i:], '?')
-		if q == -1 {
-			buf = append(buf, query[i:]...)
-			break
-		}
-		buf = append(buf, query[i:i+q]...)
-		i += q
-
+	for _, qmIdx := range paramPositions {
+		buf = append(buf, query[lastIdx:qmIdx]...)
 		arg := args[argPos]
 		argPos++
 
 		if arg == nil {
 			buf = append(buf, "NULL"...)
+			lastIdx = qmIdx + 1
 			continue
 		}
 
@@ -339,7 +431,9 @@ func (mc *mysqlConn) interpolateParams(query string, args []driver.Value) (strin
 		if len(buf)+4 > mc.maxAllowedPacket {
 			return "", driver.ErrSkip
 		}
+		lastIdx = qmIdx + 1
 	}
+	buf = append(buf, query[lastIdx:]...)
 	if argPos != len(args) {
 		return "", driver.ErrSkip
 	}

connection_test.go

@@ -79,24 +79,6 @@ func TestInterpolateParamsTooManyPlaceholders(t *testing.T) {
 	}
 }
 
-// We don't support placeholder in string literal for now.
-// https://github.com/go-sql-driver/mysql/pull/490
-func TestInterpolateParamsPlaceholderInString(t *testing.T) {
-	mc := &mysqlConn{
-		buf:              newBuffer(),
-		maxAllowedPacket: maxPacketSize,
-		cfg: &Config{
-			InterpolateParams: true,
-		},
-	}
-
-	q, err := mc.interpolateParams("SELECT 'abc?xyz',?", []driver.Value{int64(42)})
-	// When InterpolateParams support string literal, this should return `"SELECT 'abc?xyz', 42`
-	if err != driver.ErrSkip {
-		t.Errorf("Expected err=driver.ErrSkip, got err=%#v, q=%#v", err, q)
-	}
-}
-
 func TestInterpolateParamsUint64(t *testing.T) {
 	mc := &mysqlConn{
 		buf:              newBuffer(),
@@ -204,3 +186,51 @@ func (bc badConnection) Write(b []byte) (n int, err error) {
 func (bc badConnection) Close() error {
 	return nil
 }
+
+func TestInterpolateParamsWithComments(t *testing.T) {
+	mc := &mysqlConn{
+		buf:              newBuffer(),
+		maxAllowedPacket: maxPacketSize,
+		cfg: &Config{
+			InterpolateParams: true,
+		},
+	}
+
+	tests := []struct {
+		query      string
+		args       []driver.Value
+		expected   string
+		shouldSkip bool
+	}{
+		// ? in single-line comment (--) should not be replaced
+		{"SELECT 1 -- ?\n, ?", []driver.Value{int64(42)}, "SELECT 1 -- ?\n, 42", false},
+		// ? in single-line comment (#) should not be replaced
+		{"SELECT 1 # ?\n, ?", []driver.Value{int64(42)}, "SELECT 1 # ?\n, 42", false},
+		// ? in multi-line comment should not be replaced
+		{"SELECT /* ? */ ?", []driver.Value{int64(42)}, "SELECT /* ? */ 42", false},
+		// ? in string literal should not be replaced
+		{"SELECT '?', ?", []driver.Value{int64(42)}, "SELECT '?', 42", false},
+		// ? in backtick identifier should not be replaced
+		{"SELECT `?`, ?", []driver.Value{int64(42)}, "SELECT `?`, 42", false},
+		// Multiple comments and real placeholders
+		{"SELECT ? -- comment ?\n, ? /* ? */ , ? # ?\n, ?", []driver.Value{int64(1), int64(2), int64(3)}, "SELECT 1 -- comment ?\n, 2 /* ? */ , 3 # ?\n, ?", true},
+	}
+
+	for i, test := range tests {
+
+		q, err := mc.interpolateParams(test.query, test.args)
+		if test.shouldSkip {
+			if err != driver.ErrSkip {
+				t.Errorf("Test %d: Expected driver.ErrSkip, got err=%#v, q=%#v", i, err, q)
+			}
+			continue
+		}
+		if err != nil {
+			t.Errorf("Test %d: Expected err=nil, got %#v", i, err)
+			continue
+		}
+		if q != test.expected {
+			t.Errorf("Test %d: Expected: %q\nGot: %q", i, test.expected, q)
+		}
+	}
+}