fix: avoid escaping bind-like params containing colons

[villebro]

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

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[codecov]

Codecov Report

Merging #17419 (65acf8a) into master (ffa55f7) will decrease coverage by 0.18%.
The diff coverage is 94.11%.

❗ Current head 65acf8a differs from pull request most recent head 5c1cf7f. Consider uploading reports for the commit 5c1cf7f to get more accurate results

@@            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              

Flag	Coverage Δ
hive	?
mysql	81.94% <94.11%> (+0.05%)	⬆️
postgres	81.95% <94.11%> (+0.05%)	⬆️
presto	?
python	82.04% <94.11%> (-0.36%)	⬇️
sqlite	81.63% <94.11%> (+0.05%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...frontend/src/dashboard/components/Header/index.jsx	68.85% <ø> (ø)
...teFilterControl/components/DateFunctionTooltip.tsx	100.00% <ø> (ø)
superset/charts/commands/data.py	97.95% <ø> (+1.59%)	⬆️
superset/utils/core.py	89.86% <ø> (-0.25%)	⬇️
superset/connectors/sqla/models.py	86.60% <83.33%> (-1.38%)	⬇️
superset/charts/data/query_context_cache_loader.py	90.00% <90.00%> (ø)
superset/charts/api.py	85.22% <100.00%> (+4.17%)	⬆️
superset/charts/data/api.py	88.48% <100.00%> (+0.25%)	⬆️
superset/utils/date_parser.py	96.63% <100.00%> (+0.04%)	⬆️
superset/db_engines/hive.py	0.00% <0.00%> (-85.19%)	⬇️
... and 9 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ffa55f7...5c1cf7f. Read the comment docs.

[john-bodley]

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.

[villebro]

Thanks for the review @john-bodley, good comments to my oversights!

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.

AFAIK, currently there is no way of indicating that the clause is literal (like literal_column, for instance). Hence colons need to be escaped to indicate to SQLAlchemy that they should not be made available as bind parameters during compilation. But I'm going to propose adding is_literal to the text() factory, and then adding a literal_text factory as shorthand to match (literal_column), as I've seen multiple other people ask for the same thing elsewhere (in all the posts I've found, the author of SQLAlchemy always recommends just escaping the bind-like parameters).