fix: use strings.Replacer to replace args #495
Conversation
WalkthroughRefactors placeholder replacement logic into a new internal helper function Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/repo/filters.go(5 hunks)
🔇 Additional comments (1)
pkg/repo/filters.go (1)
339-345: LGTM! NotIn constructor properly mirrors In filter.The implementation correctly validates the input and uses the existing
notInFilterstruct, maintaining consistency with theInfilter constructor.
| func (f *existsFilter) String(column string, argIdx int) string { | ||
| // Replace placeholders in subquery with actual argument indices | ||
| subquery := f.subquery | ||
| for i := 0; i < len(f.values); i++ { | ||
| // Replace $1, $2, etc. with actual indices | ||
| placeholder := fmt.Sprintf("$%d", i+1) | ||
| actualPlaceholder := fmt.Sprintf("$%d", argIdx+i) | ||
| subquery = strings.ReplaceAll(subquery, placeholder, actualPlaceholder) | ||
| } | ||
| return subquery | ||
| return replaceArgs(f.subquery, f.values, argIdx) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that the refactor preserves existing behavior.
Since the replaceArgs function has placeholder replacement limitations, verify that the original inline implementation (before this refactor) had the same constraints, or if this introduces a regression.
Also applies to: 297-300, 313-315
🏁 Script executed:
#!/bin/bash
# Verify the old implementation of placeholder replacement in these three filter types
# Check the git history to see what logic was replaced
git show HEAD~1:pkg/repo/filters.go | grep -A 10 "func (f \*existsFilter) String"
echo "---"
git show HEAD~1:pkg/repo/filters.go | grep -A 10 "func (f \*subqueryFilter) String"
echo "---"
git show HEAD~1:pkg/repo/filters.go | grep -A 10 "func (f \*rawFilter) String"Length of output: 1208
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and display the current replaceArgs function implementation
rg -n "func.*replaceArgs" --type=go -A 15 pkg/repo/filters.go
Length of output: 442
Replace replaceArgs implementation to match original behavior - critical regression.
The refactor introduces a regression. The new replaceArgs function (lines 382-394) only replaces placeholders followed by , or ), while the original inline code replaced all occurrences of bare $N placeholders:
- Old behavior:
strings.ReplaceAllreplaces all$1,$2, etc. globally - New behavior: Only replaces
$1,,$1),$2,,$2)– missing cases likeWHERE id = $1 AND status = $2
Restore the original replacement logic or extend replaceArgs to handle placeholders in all contexts, not just those adjacent to punctuation.
🤖 Prompt for AI Agents
In pkg/repo/filters.go around lines 283-285, the existsFilter.String currently
calls replaceArgs which was refactored to only match placeholders followed by
',' or ')' causing a regression (it misses bare occurrences like "WHERE id = $1
AND ..."). Restore original behavior by updating replaceArgs to replace all
occurrences of the placeholder token $N globally (e.g., loop over values and
call strings.ReplaceAll for fmt.Sprintf("$%d", i) or use a regex that matches
standalone $N regardless of following punctuation), ensuring every $N in the
subquery is replaced, not just those followed by commas or parentheses.
| func replaceArgs(queryString string, values []any, argIdx int) string { | ||
| replaces := make([]string, 0, len(values)*2) | ||
| for i := range values { | ||
| replaces = append( | ||
| replaces, | ||
| fmt.Sprintf("$%d,", i+1), fmt.Sprintf("$%d,", i+argIdx), | ||
| fmt.Sprintf("$%d)", i+1), fmt.Sprintf("$%d)", i+argIdx), | ||
| ) | ||
| } | ||
|
|
||
| replacer := strings.NewReplacer(replaces...) | ||
| return replacer.Replace(queryString) | ||
| } |
There was a problem hiding this comment.
Critical: Incomplete placeholder replacement logic.
The current implementation only replaces placeholders followed by , or ), missing many valid cases such as placeholders at the end of strings, followed by spaces, or other delimiters like ], ;, >, etc.
Example failure:
-- Input with argIdx=5
"SELECT id FROM users WHERE name = $1"
-- Expected: "SELECT id FROM users WHERE name = $5"
-- Actual: "SELECT id FROM users WHERE name = $1" (unchanged!)This could lead to incorrect SQL generation and parameter binding errors.
Apply this diff to use a regex-based approach that handles all placeholder patterns:
+import (
+ "regexp"
+)
+
+var placeholderRegex = regexp.MustCompile(`\$(\d+)\b`)
+
func replaceArgs(queryString string, values []any, argIdx int) string {
- replaces := make([]string, 0, len(values)*2)
- for i := range values {
- replaces = append(
- replaces,
- fmt.Sprintf("$%d,", i+1), fmt.Sprintf("$%d,", i+argIdx),
- fmt.Sprintf("$%d)", i+1), fmt.Sprintf("$%d)", i+argIdx),
- )
- }
-
- replacer := strings.NewReplacer(replaces...)
- return replacer.Replace(queryString)
+ return placeholderRegex.ReplaceAllStringFunc(queryString, func(match string) string {
+ var n int
+ fmt.Sscanf(match, "$%d", &n)
+ if n > 0 && n <= len(values) {
+ return fmt.Sprintf("$%d", n+argIdx-1)
+ }
+ return match
+ })
}
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.