Skip to content
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

[Bug Fix] Removing escape_sql so we dont double escape #4590

Merged

Conversation

michellethomas
Copy link
Contributor

@john-bodley Removing escape_sql because this seems to be handled by qry.compile(engine) here and both of them escaping double escapes.

@michellethomas
Copy link
Contributor Author

michellethomas commented Mar 10, 2018

@mistercrunch we are a little unsure what would have caused this regression, as nothing seems to have changed in the related code recently. It looks like in sqlalchemy 1.2 they did some work to their autoescape feature. Maybe our sqlalchemy upgrade caused this issue, but it's a little unclear. Any thoughts on why we might be seeing double % escaping? Anything else we should be aware of in removing this part of the code?

@codecov-io
Copy link

codecov-io commented Mar 10, 2018

Codecov Report

Merging #4590 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4590      +/-   ##
==========================================
+ Coverage   71.17%   71.18%   +<.01%     
==========================================
  Files         188      188              
  Lines       14834    14828       -6     
  Branches     1086     1086              
==========================================
- Hits        10558    10555       -3     
+ Misses       4273     4270       -3     
  Partials        3        3
Impacted Files Coverage Δ
superset/db_engine_specs.py 52.26% <ø> (+0.01%) ⬆️
superset/connectors/sqla/models.py 77.91% <ø> (+0.11%) ⬆️

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 f988110...e1af421. Read the comment docs.

@john-bodley
Copy link
Member

john-bodley commented Mar 13, 2018

@michellethomas upon further investigation this regression doesn't seem to be related to the auto-escape feature but rather this SQLAlchemy commit (released in version 1.2.0) which escapes percent signs (%) for all dialects where the param style is either format or pyformat per this statement which is true for Presto. Hence by upgrading SQLAlchemy from 1.1.9 to 1.2.2 (PR) the percent signs were doubly escaped in Presto per the PrestoEngineSpec causing the regression.

@john-bodley john-bodley merged commit 4250e23 into apache:master Mar 13, 2018
john-bodley added a commit to john-bodley/superset that referenced this pull request Mar 13, 2018
…pe_presto

Removing escape_sql so we dont double escape
(cherry picked from commit 4250e23)
@michellethomas michellethomas changed the title Removing escape_sql so we dont double escape [Bug Fix] Removing escape_sql so we dont double escape Mar 19, 2018
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
…pe_presto

Removing escape_sql so we dont double escape
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants