-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
fix: avoid escaping bind-like params containing colons #17419
Conversation
Codecov Report
@@ Coverage Diff @@
## master #17419 +/- ##
==========================================
- Coverage 77.01% 76.83% -0.19%
==========================================
Files 1040 1041 +1
Lines 56077 56057 -20
Branches 7738 7738
==========================================
- Hits 43190 43069 -121
- Misses 12629 12730 +101
Partials 258 258
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
52e5d88
to
cbd89c3
Compare
cbd89c3
to
cd55b41
Compare
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.
LGTM, with one non-blocking comment. Its somewhat strange that we need to handle this logic, i.e., blindly I would have assumed that SQLAlchemy would deal with the escaping.
7ac565e
to
5c1cf7f
Compare
Thanks for the review @john-bodley, good comments to my oversights!
AFAIK, currently there is no way of indicating that the clause is literal (like |
* fix: avoid escaping bind-like params containing colons * fix query for mysql * address comments (cherry picked from commit ad8a7c4)
* fix: avoid escaping bind-like params containing colons * fix query for mysql * address comments
SUMMARY
A recent PR #17111 that fixed 500s being returned when a virtual table query contained a word prefixed by a colon (e.g. ":foo") caused a regression that unnecessarily also escaped similar strings that contained/ended in a colon (e.g. ":foo:" or ":foo:bar").
This PR simplifies the escaping logic by simply escaping all colons with a backslash when
sa.text
is used, as these elements are never expected to contain binds. In addition, an integration test is added that contains colons in the virtual table query, metric, filter and where clauses. The new test fails on current master with a 500.AFTER
Now all strings including colons are displayed properly on the query:
BEFORE
Before the virtual table was incorrectly escaped in multiple places:
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION