FIX: Preserve escaped %% in format_group_by_params to fix IndexError on GROUP BY queries - #537
Merged
Merged
Conversation
…on GROUP BY queries (#476) format_group_by_params converts %s placeholders to {} for a later query.format(*args) call. The old regex %\w+ greedily matched the middle of a '%%abc%%' literal (e.g. a LIKE pattern with escaped percents), injecting an extra placeholder and causing IndexError: Replacement index N out of range whenever a query had GROUP BY plus %%-escape plus a real param. Fix: match %% first and preserve it, only convert %s to {}. Also narrows the placeholder pattern from %\w+ to %s since format_sql only supports %s (it uses sql % tuple('?' * n)). Adds regression tests covering the original LIKE-with-escaped-percent case and an escape-adjacent-to-placeholder edge case. Issue: #476 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
📊 Code Coverage Report
Diff CoverageDiff: dev...HEAD, staged and unstaged changesNo lines with coverage information in this diff. 📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)- mssql/client.py: 19.5% (41 lines)
- mssql/__init__.py: 50.0% (2 lines)
- mssql/creation.py: 56.8% (74 lines)
- mssql/base.py: 68.3% (514 lines)
- mssql/compiler.py: 74.4% (640 lines)
- mssql/operations.py: 77.8% (396 lines)
- mssql/functions.py: 83.2% (428 lines)
- mssql/schema.py: 86.1% (719 lines)
- mssql/introspection.py: 90.7% (129 lines)
- mssql/features.py: 98.8% (83 lines)🔗 Quick Links
|
The previous test_escaped_percent_with_placeholder_outside_literal passed
on dev too, so it did not guard the fix. Replace it with a real regression
test: a no-param GROUP BY query using an unescaped '%abc%' LIKE pattern.
On dev the old '%\w+' regex rewrites '%abc' to '{}', turning LIKE '%abc%'
into LIKE '{}%' and silently returning zero rows (issue #394). The narrowed
'%%|%s' regex leaves it untouched.
Both tests in TestGroupByEscapedPercent now fail on dev (one IndexError for
#476, one wrong-rows for #394) and pass on the fix. Full testapp suite: 180
passed.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…//github.com/microsoft/mssql-django into bewithgaurav/fix-groupby-escaped-percent-476
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes mssql/base.py’s format_group_by_params() placeholder rewriting for GROUP BY queries so that it no longer over-matches %-prefixed tokens, preserving escaped percent literals (%%) and only treating %s as a placeholder. This addresses the IndexError reported in #476 and prevents silent query corruption in a concrete #394 scenario.
Changes:
- Narrow
format_group_by_params()’s placeholder substitution fromr'%\w+'to a targeted match of%%(preserved) and%s(rewritten to{}for.format()). - Add regression tests covering: (1)
GROUP BY+ escaped%%+ real params (formerlyIndexError), and (2)GROUP BY+ unescaped%wordpattern with no params (formerly silent wrong results).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
mssql/base.py |
Fixes placeholder rewriting to preserve %% literals and only rewrite %s, preventing IndexError and avoiding unintended LIKE pattern modification. |
testapp/tests/test_queries.py |
Adds regression tests for #476 and a specific over-matching case behind #394 to prevent future regressions. |
Reword the no-param test comment to describe precisely what it guards (the old %\w+ overmatch on word chars after %), not "printf spec" behavior it does not exercise. Swap the #476 test predicate from `id >= %s` to `name <> %s` so the param is tied to the data instead of relying on the IDENTITY seed. Behavior unchanged: both tests still fail on dev (IndexError for #476, wrong rows for #394) and pass on the fix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…by-escaped-percent-476 # Conflicts: # testapp/tests/test_queries.py
jahnvi480
approved these changes
Jul 24, 2026
Merged
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.
fixes #476.
format_group_by_paramsrunsre.sub(r'%\w+', '{}', query)on any query with a GROUP BY, thenquery.format(*args). that regex matches far more than real placeholders. it splits%%abc%%literals, injecting a phantom{}and raisingIndexError: Replacement index N out of rangewhen a query has GROUP BY +%%-escape + a real param.the fix narrows the match to
%%(preserved verbatim) and%s(the only real placeholder).format_sqlonly ever emits%s, so nothing else should be touched.also fixes one #394 case
an unescaped
%wordpattern likeLIKE '%abc%'in a no-param query. the old%\w+rewrote it toLIKE '{}%'and silently returned the wrong rows. the narrowed regex leaves it alone.what this does NOT fix (still #394, needs a quote-aware rewrite)
%sinside a string literal is still read as a placeholder.%that is not%s(e.g.%d,%p) in a query that also carries params still breaks informat_sql's%-formatting. that surfaces as a loud TypeError/ValueError now, not silent bad data, but it is not handled here.tracked for a follow-up that parses SQL literal-aware instead of regex-substituting unparsed text.
tests (both fail on dev, pass here)
test_like_with_escaped_percent_and_group_by: format_group_by_params mishandles escaped %% causing IndexError on queries with GROUP BY #476, escaped%%plus a param. IndexError on dev.test_unescaped_percent_pattern_no_params_preserved: Query results different on 1.5 on static query. #394,LIKE '%abc%'no params. wrong rows on dev.full testapp suite: 180 passed locally against SQL Server 2022.