From: Mike Bayer Date: Sat, 15 Jun 2019 22:06:50 +0000 (-0400) Subject: Turn off the is_literal flag when proxying literal_column() to Label X-Git-Tag: rel_1_3_5~4^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=be206b7916582d52a89e84a9307f1d220dd8ec75;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git Turn off the is_literal flag when proxying literal_column() to Label Fixed a series of quoting issues which all stemmed from the concept of the :func:`.literal_column` construct, which when being "proxied" through a subquery to be referred towards by a label that matches its text, the label would not have quoting rules applied to it, even if the string in the :class:`.Label` were set up as a :class:`.quoted_name` construct. Not applying quoting to the text of the :class:`.Label` is a bug because this text is strictly a SQL identifier name and not a SQL expression, and the string should not have quotes embedded into it already unlike the :func:`.literal_column` which it may be applied towards. The existing behavior of a non-labeled :func:`.literal_column` being propagated as is on the outside of a subquery is maintained in order to help with manual quoting schemes, although it's not clear if valid SQL can be generated for such a construct in any case. Fixes: #4730 Change-Id: I300941f27872fc4298c74a1d1ed65aef1a5cdd82 (cherry picked from commit 009acc95b8804b5b62fbd43c6fdd61d6fd407ef7) --- diff --git a/doc/build/changelog/unreleased_13/4730.rst b/doc/build/changelog/unreleased_13/4730.rst new file mode 100644 index 0000000000..05354898d5 --- /dev/null +++ b/doc/build/changelog/unreleased_13/4730.rst @@ -0,0 +1,17 @@ +.. change:: + :tags: bug, sql + :tickets: 4730 + + Fixed a series of quoting issues which all stemmed from the concept of the + :func:`.literal_column` construct, which when being "proxied" through a + subquery to be referred towards by a label that matches its text, the label + would not have quoting rules applied to it, even if the string in the + :class:`.Label` were set up as a :class:`.quoted_name` construct. Not + applying quoting to the text of the :class:`.Label` is a bug because this + text is strictly a SQL identifier name and not a SQL expression, and the + string should not have quotes embedded into it already unlike the + :func:`.literal_column` which it may be applied towards. The existing + behavior of a non-labeled :func:`.literal_column` being propagated as is on + the outside of a subquery is maintained in order to help with manual + quoting schemes, although it's not clear if valid SQL can be generated for + such a construct in any case. \ No newline at end of file diff --git a/lib/sqlalchemy/sql/compiler.py b/lib/sqlalchemy/sql/compiler.py index c7fe3dc50e..ba43d52783 100644 --- a/lib/sqlalchemy/sql/compiler.py +++ b/lib/sqlalchemy/sql/compiler.py @@ -848,6 +848,8 @@ class SQLCompiler(Compiled): ) if is_literal: + # note we are not currently accommodating for + # literal_column(quoted_name('ident', True)) here name = self.escape_literal_column(name) else: name = self.preparer.quote(name) diff --git a/lib/sqlalchemy/sql/elements.py b/lib/sqlalchemy/sql/elements.py index b0d0feff5d..411b38a7dc 100644 --- a/lib/sqlalchemy/sql/elements.py +++ b/lib/sqlalchemy/sql/elements.py @@ -3774,7 +3774,9 @@ class Label(ColumnElement): def _make_proxy(self, selectable, name=None, **kw): e = self.element._make_proxy( - selectable, name=name if name else self.name + selectable, + name=name if name else self.name, + disallow_is_literal=True, ) e._proxies.append(self) if self._type is not None: @@ -3908,7 +3910,6 @@ class ColumnClause(Immutable, ColumnElement): :ref:`sqlexpression_literal_column` """ - self.key = self.name = text self.table = _selectable self.type = type_api.to_instance(type_) @@ -4029,11 +4030,25 @@ class ColumnClause(Immutable, ColumnElement): name=None, attach=True, name_is_truncatable=False, + disallow_is_literal=False, **kw ): - # propagate the "is_literal" flag only if we are keeping our name, - # otherwise its considered to be a label - is_literal = self.is_literal and (name is None or name == self.name) + # the "is_literal" flag normally should never be propagated; a proxied + # column is always a SQL identifier and never the actual expression + # being evaluated. however, there is a case where the "is_literal" flag + # might be used to allow the given identifier to have a fixed quoting + # pattern already, so maintain the flag for the proxy unless a + # :class:`.Label` object is creating the proxy. See [ticket:4730]. + is_literal = ( + not disallow_is_literal + and self.is_literal + and ( + # note this does not accommodate for quoted_name differences + # right now + name is None + or name == self.name + ) + ) c = self._constructor( _as_truncated(name or self.name) if name_is_truncatable diff --git a/lib/sqlalchemy/sql/selectable.py b/lib/sqlalchemy/sql/selectable.py index f15e685a85..1167b15258 100644 --- a/lib/sqlalchemy/sql/selectable.py +++ b/lib/sqlalchemy/sql/selectable.py @@ -3692,7 +3692,6 @@ class Select(HasPrefixes, HasSuffixes, GenerativeSelect): key = c.anon_label else: key = None - c._make_proxy(self, key=key, name=name, name_is_truncatable=True) def _refresh_for_new_column(self, column): diff --git a/test/dialect/oracle/test_compiler.py b/test/dialect/oracle/test_compiler.py index 596161ef27..981a5f9c73 100644 --- a/test/dialect/oracle/test_compiler.py +++ b/test/dialect/oracle/test_compiler.py @@ -9,6 +9,7 @@ from sqlalchemy import ForeignKey from sqlalchemy import func from sqlalchemy import Index from sqlalchemy import Integer +from sqlalchemy import literal_column from sqlalchemy import MetaData from sqlalchemy import or_ from sqlalchemy import outerjoin @@ -25,6 +26,7 @@ from sqlalchemy.dialects.oracle import base as oracle from sqlalchemy.dialects.oracle import cx_oracle from sqlalchemy.engine import default from sqlalchemy.sql import column +from sqlalchemy.sql import quoted_name from sqlalchemy.sql import table from sqlalchemy.testing import assert_raises_message from sqlalchemy.testing import AssertsCompiledSQL @@ -239,6 +241,62 @@ class CompileTest(fixtures.TestBase, AssertsCompiledSQL): "UPDATE", ) + def test_limit_special_quoting(self): + """Oracle-specific test for #4730. + + Even though this issue is generic, test the originally reported Oracle + use case. + + """ + + col = literal_column("SUM(ABC)").label("SUM(ABC)") + tbl = table("my_table") + query = select([col]).select_from(tbl).order_by(col).limit(100) + + self.assert_compile( + query, + 'SELECT "SUM(ABC)" FROM ' + '(SELECT SUM(ABC) AS "SUM(ABC)" ' + "FROM my_table ORDER BY SUM(ABC)) " + "WHERE ROWNUM <= :param_1", + ) + + col = literal_column("SUM(ABC)").label(quoted_name("SUM(ABC)", True)) + tbl = table("my_table") + query = select([col]).select_from(tbl).order_by(col).limit(100) + + self.assert_compile( + query, + 'SELECT "SUM(ABC)" FROM ' + '(SELECT SUM(ABC) AS "SUM(ABC)" ' + "FROM my_table ORDER BY SUM(ABC)) " + "WHERE ROWNUM <= :param_1", + ) + + col = literal_column("SUM(ABC)").label("SUM(ABC)_") + tbl = table("my_table") + query = select([col]).select_from(tbl).order_by(col).limit(100) + + self.assert_compile( + query, + 'SELECT "SUM(ABC)_" FROM ' + '(SELECT SUM(ABC) AS "SUM(ABC)_" ' + "FROM my_table ORDER BY SUM(ABC)) " + "WHERE ROWNUM <= :param_1", + ) + + col = literal_column("SUM(ABC)").label(quoted_name("SUM(ABC)_", True)) + tbl = table("my_table") + query = select([col]).select_from(tbl).order_by(col).limit(100) + + self.assert_compile( + query, + 'SELECT "SUM(ABC)_" FROM ' + '(SELECT SUM(ABC) AS "SUM(ABC)_" ' + "FROM my_table ORDER BY SUM(ABC)) " + "WHERE ROWNUM <= :param_1", + ) + def test_for_update(self): table1 = table( "mytable", column("myid"), column("name"), column("description") diff --git a/test/sql/test_quote.py b/test/sql/test_quote.py index 49dfbba9f3..8ef4728781 100644 --- a/test/sql/test_quote.py +++ b/test/sql/test_quote.py @@ -695,6 +695,108 @@ class QuoteTest(fixtures.TestBase, AssertsCompiledSQL): ') AS "Alias1"', ) + def test_literal_column_label_embedded_select_samename(self): + col = sql.literal_column("NEEDS QUOTES").label("NEEDS QUOTES") + + # embedded SELECT use case, going away in 1.4 however use a + # SelectStatementGrouping here when that merges + self.assert_compile( + select([col]).select(), + 'SELECT "NEEDS QUOTES" FROM (SELECT NEEDS QUOTES AS ' + '"NEEDS QUOTES")', + ) + + def test_literal_column_label_alias_samename(self): + col = sql.literal_column("NEEDS QUOTES").label("NEEDS QUOTES") + + self.assert_compile( + select([col]).alias().select(), + 'SELECT anon_1."NEEDS QUOTES" FROM (SELECT NEEDS QUOTES AS ' + '"NEEDS QUOTES") AS anon_1', + ) + + def test_literal_column_label_embedded_select_diffname(self): + col = sql.literal_column("NEEDS QUOTES").label("NEEDS QUOTES_") + + # embedded SELECT use case, going away in 1.4 however use a + # SelectStatementGrouping here when that merges + self.assert_compile( + select([col]).select(), + 'SELECT "NEEDS QUOTES_" FROM (SELECT NEEDS QUOTES AS ' + '"NEEDS QUOTES_")', + ) + + def test_literal_column_label_alias_diffname(self): + col = sql.literal_column("NEEDS QUOTES").label("NEEDS QUOTES_") + + self.assert_compile( + select([col]).alias().select(), + 'SELECT anon_1."NEEDS QUOTES_" FROM (SELECT NEEDS QUOTES AS ' + '"NEEDS QUOTES_") AS anon_1', + ) + + def test_literal_column_label_embedded_select_samename_explcit_quote(self): + col = sql.literal_column("NEEDS QUOTES").label( + quoted_name("NEEDS QUOTES", True) + ) + + # embedded SELECT use case, going away in 1.4 however use a + # SelectStatementGrouping here when that merges + self.assert_compile( + select([col]).select(), + 'SELECT "NEEDS QUOTES" FROM ' + '(SELECT NEEDS QUOTES AS "NEEDS QUOTES")', + ) + + def test_literal_column_label_alias_samename_explcit_quote(self): + col = sql.literal_column("NEEDS QUOTES").label( + quoted_name("NEEDS QUOTES", True) + ) + + self.assert_compile( + select([col]).alias().select(), + 'SELECT anon_1."NEEDS QUOTES" FROM ' + '(SELECT NEEDS QUOTES AS "NEEDS QUOTES") AS anon_1', + ) + + def test_literal_column_label_embedded_select_diffname_explcit_quote(self): + col = sql.literal_column("NEEDS QUOTES").label( + quoted_name("NEEDS QUOTES_", True) + ) + + # embedded SELECT use case, going away in 1.4 however use a + # SelectStatementGrouping here when that merges + self.assert_compile( + select([col]).select(), + 'SELECT "NEEDS QUOTES_" FROM ' + '(SELECT NEEDS QUOTES AS "NEEDS QUOTES_")', + ) + + def test_literal_column_label_alias_diffname_explcit_quote(self): + col = sql.literal_column("NEEDS QUOTES").label( + quoted_name("NEEDS QUOTES_", True) + ) + + self.assert_compile( + select([col]).alias().select(), + 'SELECT anon_1."NEEDS QUOTES_" FROM ' + '(SELECT NEEDS QUOTES AS "NEEDS QUOTES_") AS anon_1', + ) + + def test_literal_column_wo_label_currently_maintained(self): + # test related to [ticket:4730] where we are maintaining that + # literal_column() proxied outwards *without* a label is maintained + # as is; in most cases literal_column would need proxying however + # at least if the column is being used to generate quoting in some + # way, it's maintined as given + col = sql.literal_column('"NEEDS QUOTES"') + + self.assert_compile( + select([col]).alias().select(), + 'SELECT anon_1."NEEDS QUOTES" FROM ' + '(SELECT "NEEDS QUOTES") AS anon_1', + ) + def test_apply_labels_should_quote(self): # Not lower case names, should quote metadata = MetaData() diff --git a/test/sql/test_text.py b/test/sql/test_text.py index 188ac3878c..60d7518cb8 100644 --- a/test/sql/test_text.py +++ b/test/sql/test_text.py @@ -19,6 +19,7 @@ from sqlalchemy import text from sqlalchemy import union from sqlalchemy import util from sqlalchemy.sql import column +from sqlalchemy.sql import quoted_name from sqlalchemy.sql import table from sqlalchemy.sql import util as sql_util from sqlalchemy.testing import assert_raises_message @@ -27,7 +28,6 @@ from sqlalchemy.testing import eq_ from sqlalchemy.testing import fixtures from sqlalchemy.types import NullType - table1 = table( "mytable", column("myid", Integer), @@ -136,7 +136,9 @@ class SelectCompositionTest(fixtures.TestBase, AssertsCompiledSQL): def test_select_composition_six(self): # test that "auto-labeling of subquery columns" # doesn't interfere with literal columns, - # exported columns don't get quoted + # exported columns don't get quoted. + # [ticket:4730] refines this but for the moment the behavior with + # no columns is being maintained. self.assert_compile( select( [ @@ -708,6 +710,44 @@ class OrderByLabelResolutionTest(fixtures.TestBase, AssertsCompiledSQL): "GROUP BY anon_1.myid", ) + def test_order_by_literal_col_quoting_one(self): + col = literal_column("SUM(ABC)").label("SUM(ABC)") + tbl = table("my_table") + query = select([col]).select_from(tbl).order_by(col) + self.assert_compile( + query, + 'SELECT SUM(ABC) AS "SUM(ABC)" FROM my_table ORDER BY "SUM(ABC)"', + ) + + def test_order_by_literal_col_quoting_two(self): + col = literal_column("SUM(ABC)").label("SUM(ABC)_") + tbl = table("my_table") + query = select([col]).select_from(tbl).order_by(col) + self.assert_compile( + query, + 'SELECT SUM(ABC) AS "SUM(ABC)_" FROM my_table ORDER BY ' + '"SUM(ABC)_"', + ) + + def test_order_by_literal_col_quoting_one_explict_quote(self): + col = literal_column("SUM(ABC)").label(quoted_name("SUM(ABC)", True)) + tbl = table("my_table") + query = select([col]).select_from(tbl).order_by(col) + self.assert_compile( + query, + 'SELECT SUM(ABC) AS "SUM(ABC)" FROM my_table ORDER BY "SUM(ABC)"', + ) + + def test_order_by_literal_col_quoting_two_explicit_quote(self): + col = literal_column("SUM(ABC)").label(quoted_name("SUM(ABC)_", True)) + tbl = table("my_table") + query = select([col]).select_from(tbl).order_by(col) + self.assert_compile( + query, + 'SELECT SUM(ABC) AS "SUM(ABC)_" FROM my_table ORDER BY ' + '"SUM(ABC)_"', + ) + def test_order_by_func_label_desc(self): stmt = select([func.foo("bar").label("fb"), table1]).order_by( desc("fb")