Conversation
- Fix quote escaping bug (single quotes now properly escaped) - Fix partial matching bug (:id no longer matches inside :product_id) - Remove sqlglot dependency, use stdlib re module - Add tests
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #5 by replacing naive str.replace() parameter substitution with a token-based regex approach that prevents two critical bugs:
- Partial matching bug: The old code would incorrectly replace
:idinside:product_id, resulting in malformed queries - Quote escaping bug: Single quotes in parameter values (e.g.,
O'Brien) were not properly escaped for SQL parsing, causing syntax errors
Changes:
- Added token-based
_substitute_params()method using regex to split SQL on parameter patterns, preventing partial matches - Implemented SQL-standard quote escaping (single quote → double single quote) for string values
- Added comprehensive test suite covering partial matching, quote escaping, and edge cases
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| sql_redis/executor.py | Replaced naive str.replace() with token-based _substitute_params() method that uses regex splitting to prevent partial matches and properly escapes quotes |
| tests/test_parameter_substitution.py | Added comprehensive test suite with 14 tests covering partial matching bugs, quote escaping, and edge cases like empty strings and special characters |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| index_name = "param_test" | ||
| try: | ||
| redis_client.execute_command("FT.DROPINDEX", index_name, "DD") | ||
| except redis.ResponseError: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
Suggested change
| except redis.ResponseError: | |
| except redis.ResponseError: | |
| # Ignore errors when the index does not exist; we recreate it below for tests. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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
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.
Fix for #5
Fix: Token-Based Parameter Substitution
Summary
Replaces naive
str.replace()parameter substitution with token-based regex approach to fix two critical bugs discovered through TDD investigation.Bugs Fixed
1. Quote Escaping Bug (CRITICAL)
Single quotes in parameters weren't escaped, breaking SQL parsing:
2. Partial Matching Bug
:idincorrectly matched inside:product_id:Solution
Token-based approach using regex
(:[a-zA-Z_][a-zA-Z0-9_]*)::identifieras complete token'→'')Why Token-Based?
resqlglotDecision: Best balance of simplicity, correctness, and performance.
Changes
Modified
sql_redis/executor.py- Replaced implementation with token-based approach, added comprehensive docsAdded
tests/test_parameter_substitution.py- 12 TDD tests validating bug fixesImplementation Details
Regex Pattern:
(:[a-zA-Z_][a-zA-Z0-9_]*):idand:product_idare separate tokensQuote Escaping: SQL standard (
'→'')Type Handling:
int/float→ stringstr→ quoted with escapingbytes→ placeholder kept (for vectors)Known Limitations
Colons in string literals: Theoretical issue with SQL like
WHERE x = 'test:value'Why it doesn't matter: