-
Notifications
You must be signed in to change notification settings - Fork 159
fix: prepared placeholder replacement #212
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes the placeholder replacement logic in SQL statements to correctly handle PostgreSQL-style numbered placeholders ($1, $2, etc.) by using the numeric index to look up parameters, rather than relying on sequential order. The fix ensures that parameters are matched correctly when placeholders are used out of order (e.g., $2 before $1).
Key Changes:
- Modified the regex replacement logic to capture the numeric part of $N placeholders and use it for direct parameter lookup (1-indexed)
- Separated handling for $N placeholders (numeric lookup) from ? placeholders (sequential lookup)
- Added documentation example demonstrating the use case with reversed parameter order ($2 before $1)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| frontend/src/components/views/ToolDetailView.tsx | Updated transformedStatement function to handle $N placeholders with numeric indexing instead of sequential order |
| dbhub.toml.example | Added example configuration showing update_employee_department tool with $2 and $1 placeholders in reverse order |
| transformedSql = transformedSql.replace(/\$(\d+)|\?/g, (match, num) => { | ||
| let param; | ||
| if (num) { | ||
| // $N placeholder - use the number (1-indexed) to find parameter | ||
| param = tool.parameters[parseInt(num, 10) - 1]; | ||
| } else { | ||
| // ? placeholder - use sequential order | ||
| param = tool.parameters[questionMarkIndex]; | ||
| questionMarkIndex++; | ||
| } |
Copilot
AI
Dec 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The replacement logic has a potential bug when handling SQL statements that mix both $N and ? placeholders. The questionMarkIndex counter is only incremented for ? placeholders, but if the SQL contains both types (e.g., "SELECT * FROM table WHERE id = $1 AND name = ?"), the sequential ordering for ? placeholders may not align correctly with the intended parameter positions. Consider either validating that statements use only one placeholder style, or tracking separate indices for each placeholder type more carefully.
| param = tool.parameters[parseInt(num, 10) - 1]; | ||
| } else { | ||
| // ? placeholder - use sequential order | ||
| param = tool.parameters[questionMarkIndex]; | ||
| questionMarkIndex++; |
Copilot
AI
Dec 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic doesn't handle the case where $N references a parameter index that doesn't exist in the parameters array. For example, if the SQL contains "$5" but only 3 parameters are defined, parseInt(num, 10) - 1 would be 4, which would result in undefined being accessed from the array. While the fallback :? handles the undefined case, this could indicate a configuration error that should be caught and logged or validated earlier.
| param = tool.parameters[parseInt(num, 10) - 1]; | |
| } else { | |
| // ? placeholder - use sequential order | |
| param = tool.parameters[questionMarkIndex]; | |
| questionMarkIndex++; | |
| const index = parseInt(num, 10) - 1; | |
| if ( | |
| Number.isNaN(index) || | |
| index < 0 || | |
| !tool.parameters || | |
| index >= tool.parameters.length | |
| ) { | |
| console.warn( | |
| `Invalid parameter index $${num} for tool "${tool.name ?? 'unknown'}". ` + | |
| `Tool has ${tool.parameters ? tool.parameters.length : 0} parameter(s).` | |
| ); | |
| param = undefined; | |
| } else { | |
| param = tool.parameters[index]; | |
| } | |
| } else { | |
| // ? placeholder - use sequential order | |
| if (!tool.parameters || questionMarkIndex >= tool.parameters.length) { | |
| console.warn( | |
| `Too many positional parameters ("?") in SQL for tool "${tool.name ?? 'unknown'}". ` + | |
| `Tool has ${tool.parameters ? tool.parameters.length : 0} parameter(s).` | |
| ); | |
| param = undefined; | |
| } else { | |
| param = tool.parameters[questionMarkIndex]; | |
| questionMarkIndex++; | |
| } |
No description provided.