Enhance interpolateParams to correctly handle placeholders #1731
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
WalkthroughA new function was added to accurately find SQL parameter placeholders ('?') in queries, ignoring those inside comments, strings, or backticks. The parameter interpolation logic was updated to use this function, ensuring only valid placeholders are replaced. Related tests were updated: a test was removed and a new comprehensive test was added for these scenarios. Changes
Sequence Diagram(s)sequenceDiagram participant Client participant mysqlConn Client->>mysqlConn: interpolateParams(query, args) mysqlConn->>mysqlConn: findParamPositions(query) alt Placeholder count matches args mysqlConn->>mysqlConn: Replace valid ? with args mysqlConn-->>Client: Return interpolated query else Mismatch or ambiguous mysqlConn-->>Client: Return driver.ErrSkip end ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File ( |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
connection.go (1)
303-306: Consider adding carriage return handling for cross-platform compatibility.The line ending detection only handles
\nbut not\r\nor\r. This could cause issues on Windows systems or with SQL files created on different platforms.Consider adding carriage return handling:
case LINE_FEED_BYTE: if state == stateEOLComment { state = stateNormal } +case '\r': + if state == stateEOLComment { + state = stateNormal + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
connection.go(4 hunks)connection_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: shogo82148 PR: go-sql-driver/mysql#1576 File: CHANGELOG.md:1-6 Timestamp: 2024-07-26T20:55:56.049Z Learning: PRs #1562 and #1570 in the go-sql-driver/mysql repository collectively address the same underlying issue related to fixing a race condition when a context is canceled, as documented in the CHANGELOG.md for version 1.8.1. connection.go (1)
Learnt from: shogo82148 PR: go-sql-driver/mysql#1576 File: CHANGELOG.md:1-6 Timestamp: 2024-07-26T20:55:56.049Z Learning: PRs #1562 and #1570 in the go-sql-driver/mysql repository collectively address the same underlying issue related to fixing a race condition when a context is canceled, as documented in the CHANGELOG.md for version 1.8.1. 🧬 Code Graph Analysis (1)
connection_test.go (1)
dsn.go (1)
Config(37-84)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: test (windows-latest, 1.24, mariadb-11.1)
- GitHub Check: test (windows-latest, 1.24, mariadb-10.6)
- GitHub Check: test (windows-latest, 1.24, 5.7)
- GitHub Check: test (windows-latest, 1.24, mariadb-11.2)
- GitHub Check: test (macos-latest, 1.24, mariadb-11.2)
- GitHub Check: test (windows-latest, 1.24, mariadb-10.5)
- GitHub Check: test (macos-latest, 1.24, mariadb-10.6)
- GitHub Check: test (windows-latest, 1.24, mariadb-11.4)
- GitHub Check: test (windows-latest, 1.24, 9.0)
- GitHub Check: test (ubuntu-latest, 1.24, mariadb-10.5)
- GitHub Check: test (windows-latest, 1.24, 8.4)
- GitHub Check: test (macos-latest, 1.24, 8.4)
- GitHub Check: test (ubuntu-latest, 1.24, mariadb-10.6)
- GitHub Check: test (ubuntu-latest, 1.24, 8.0)
- GitHub Check: test (ubuntu-latest, 1.24, mariadb-11.2)
- GitHub Check: test (ubuntu-latest, 1.24, mariadb-11.1)
- GitHub Check: test (ubuntu-latest, 1.24, 8.4)
- GitHub Check: test (ubuntu-latest, 1.24, mariadb-11.4)
- GitHub Check: lint
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
connection_test.go (1)
190-236: Excellent comprehensive test coverage for the new parameter interpolation logic.The test thoroughly covers all the critical scenarios:
- Single-line comments with
--and#- Multi-line comments with
/* */- String literals with single quotes
- Backtick identifiers
- Complex queries with multiple comment types and parameter mismatches
The test structure is clean and the expected behavior is correctly validated.
connection.go (2)
346-347: Well-implemented parameter interpolation using the new parsing logic.The updated
interpolateParamsfunction correctly:
- Uses
findParamPositionsto identify real parameter locations- Validates parameter count matches argument count
- Reconstructs the query by copying segments between parameters
- Maintains proper error handling with
driver.ErrSkipAlso applies to: 361-371, 434-436
248-343: Overall excellent state machine implementation for SQL parsing.The state machine approach is well-designed and handles the complex parsing requirements:
- Proper string literal handling with escape sequences
- Correct multi-line comment detection
- Backtick identifier support
- Clean state transitions
This significantly improves the robustness of parameter interpolation by preventing incorrect replacement of placeholders within comments and strings.
connection.go Outdated
Comment on lines 248 to 343
| // 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 | ||
| } |
There was a problem hiding this comment.
Fix incorrect // comment detection for MySQL compatibility.
The state machine incorrectly treats // as a comment delimiter (lines 292-294), but MySQL does not support // comments. MySQL only supports:
--(two dashes followed by space) for single-line comments#for single-line comments/* */for multi-line comments
This could cause valid // sequences in SQL to be incorrectly treated as comments.
Remove the // comment detection:
case SLASH_BYTE: if state == stateSlashStarComment && lastChar == STAR_BYTE { state = stateNormal - } else if state == stateNormal && lastChar == SLASH_BYTE { - state = stateEOLComment }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // 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 | |
| } | |
| // in connection.go, within the switch over currentChar: | |
| case SLASH_BYTE: | |
| if state == stateSlashStarComment && lastChar == STAR_BYTE { | |
| state = stateNormal | |
| } |
🤖 Prompt for AI Agents
In connection.go from lines 248 to 343, the current state machine incorrectly treats double slashes '//' as single-line comment delimiters, which is not compatible with MySQL syntax. To fix this, remove the logic that sets the state to stateEOLComment when encountering two consecutive SLASH_BYTE characters. Instead, ensure only '-- ' (two dashes followed by a space), '#' for single-line comments, and '/* */' for multi-line comments are recognized as comment delimiters. Adjust the conditions around lines 292-294 to eliminate handling of '//' comments. 
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
Enhance client side statement to correctly handle placeholders in queries with comments, strings, and backticks.
findParamPositionsto identify real parameter positions