Treat collation names as identifiers

The expression used for COLLATE as rendered by the column-level
:func:`.expression.collate` and :meth:`.ColumnOperators.collate` is now
quoted as an identifier when the name is case sensitive, e.g. has
uppercase characters.  Note that this does not impact type-level
collation, which is already quoted.

Change-Id: I83d5d9cd1e66a4f20b96303bb84c5f360d5d6a1a
Fixes: #3785

diff --git a/doc/build/changelog/changelog_12.rst b/doc/build/changelog/changelog_12.rst
index 9e0f558994cd06306c5381cb7eb691ad3b2f446a..834dc074de8f69c54aa156fc3a26d30f6c4571bb 100644 (file)
--- a/doc/build/changelog/changelog_12.rst
+++ b/doc/build/changelog/changelog_12.rst
@@ -13,6 +13,20 @@
 .. changelog::
     :version: 1.2.0b1
 
+    .. change:: 3785
+        :tags: bug, sql
+        :tickets: 3785
+
+        The expression used for COLLATE as rendered by the column-level
+        :func:`.expression.collate` and :meth:`.ColumnOperators.collate` is now
+        quoted as an identifier when the name is case sensitive, e.g. has
+        uppercase characters.  Note that this does not impact type-level
+        collation, which is already quoted.
+
+        .. seealso::
+
+            :ref:`change_3785`
+
     .. change:: 3229
         :tags: feature, orm, ext
         :tickets: 3229

diff --git a/doc/build/changelog/migration_12.rst b/doc/build/changelog/migration_12.rst
index 0fc60e9d9822f846dcc4aeabfc0dc2050c2bda94..9aebf6042a520289bb2d9e757d88506ec0b84287 100644 (file)
--- a/doc/build/changelog/migration_12.rst
+++ b/doc/build/changelog/migration_12.rst
@@ -578,6 +578,32 @@ warning.   However, it is anticipated that most users will appreciate the
 
 :ticket:`3907`
 
+.. _change_3785:
+
+The column-level COLLATE keyword now quotes the collation name
+--------------------------------------------------------------
+
+A bug in the :func:`.expression.collate` and :meth:`.ColumnOperators.collate`
+functions, used to supply ad-hoc column collations at the statement level,
+is fixed, where a case sensitive name would not be quoted::
+
+    stmt = select([mytable.c.x, mytable.c.y]).\
+        order_by(mytable.c.somecolumn.collate("fr_FR"))
+
+now renders::
+
+    SELECT mytable.x, mytable.y,
+    FROM mytable ORDER BY mytable.somecolumn COLLATE "fr_FR"
+
+Previously, the case sensitive name `"fr_FR"` would not be quoted.   Currently,
+manual quoting of the "fr_FR" name is **not** detected, so applications that
+are manually quoting the identifier should be adjusted.   Note that this change
+does not impact the use of collations at the type level (e.g. specified
+on the datatype like :class:`.String` at the table level), where quoting
+is already applied.
+
+:ticket:`3785`
+
 Dialect Improvements and Changes - PostgreSQL
 =============================================
 

diff --git a/lib/sqlalchemy/sql/elements.py b/lib/sqlalchemy/sql/elements.py
index a450efaf028efba96882c0aaa298d00a81b98777..1f812938297396f8b5c0baa1e7a0eb4ee7b0b5c2 100644 (file)
--- a/lib/sqlalchemy/sql/elements.py
+++ b/lib/sqlalchemy/sql/elements.py
@@ -41,12 +41,18 @@ def collate(expression, collation):
 
         mycolumn COLLATE utf8_bin
 
+    The collation expression is also quoted if it is a case sensitive
+    identifer, e.g. contains uppercase characters.
+
+    .. versionchanged:: 1.2 quoting is automatically applied to COLLATE
+       expressions if they are case sensitive.
+
     """
 
     expr = _literal_as_binds(expression)
     return BinaryExpression(
         expr,
-        _literal_as_text(collation),
+        ColumnClause(collation),
         operators.collate, type_=expr.type)
 
 

diff --git a/lib/sqlalchemy/sql/operators.py b/lib/sqlalchemy/sql/operators.py
index 8f697b27eb71ade52826a7da163f94b8ead059ab..49642acdd5b6499c2706afd3e3fbb3d071e29c99 100644 (file)
--- a/lib/sqlalchemy/sql/operators.py
+++ b/lib/sqlalchemy/sql/operators.py
@@ -695,7 +695,13 @@ class ColumnOperators(Operators):
 
     def collate(self, collation):
         """Produce a :func:`~.expression.collate` clause against
-        the parent object, given the collation string."""
+        the parent object, given the collation string.
+
+        .. seealso::
+
+            :func:`~.expression.collate`
+
+        """
         return self.operate(collate, collation)
 
     def __radd__(self, other):

diff --git a/test/orm/test_query.py b/test/orm/test_query.py
index ce119cf502942a18503b22a41ace5f80b826e3d3..9924c9547c7e5578532baa8c298d883b6b2dbd5c 100644 (file)
--- a/test/orm/test_query.py
+++ b/test/orm/test_query.py
@@ -1396,9 +1396,9 @@ class OperatorTest(QueryTest, AssertsCompiledSQL):
     def test_collate(self):
         User = self.classes.User
 
-        self._test(collate(User.id, 'binary'), "users.id COLLATE binary")
+        self._test(collate(User.id, 'utf8_bin'), "users.id COLLATE utf8_bin")
 
-        self._test(User.id.collate('binary'), "users.id COLLATE binary")
+        self._test(User.id.collate('utf8_bin'), "users.id COLLATE utf8_bin")
 
     def test_selfref_between(self):
         User = self.classes.User

diff --git a/test/sql/test_operators.py b/test/sql/test_operators.py
index 0e0a8b29c3a04b05b163423cbd812b84b0c74115..7c3ce1389828d9aae637fa2a4d822a5e35a1238a 100644 (file)
--- a/test/sql/test_operators.py
+++ b/test/sql/test_operators.py
@@ -1389,19 +1389,19 @@ class OperatorPrecedenceTest(fixtures.TestBase, testing.AssertsCompiledSQL):
     def test_operator_precedence_collate_1(self):
         self.assert_compile(
             self.table1.c.name == literal('foo').collate('utf-8'),
-            "mytable.name = (:param_1 COLLATE utf-8)"
+            'mytable.name = (:param_1 COLLATE "utf-8")'
         )
 
     def test_operator_precedence_collate_2(self):
         self.assert_compile(
             (self.table1.c.name == literal('foo')).collate('utf-8'),
-            "mytable.name = :param_1 COLLATE utf-8"
+            'mytable.name = :param_1 COLLATE "utf-8"'
         )
 
     def test_operator_precedence_collate_3(self):
         self.assert_compile(
             self.table1.c.name.collate('utf-8') == 'foo',
-            "(mytable.name COLLATE utf-8) = :param_1"
+            '(mytable.name COLLATE "utf-8") = :param_1'
         )
 
     def test_operator_precedence_collate_4(self):
@@ -1410,8 +1410,8 @@ class OperatorPrecedenceTest(fixtures.TestBase, testing.AssertsCompiledSQL):
                 (self.table1.c.name == literal('foo')).collate('utf-8'),
                 (self.table2.c.field == literal('bar')).collate('utf-8'),
             ),
-            "mytable.name = :param_1 COLLATE utf-8 "
-            "AND op.field = :param_2 COLLATE utf-8"
+            'mytable.name = :param_1 COLLATE "utf-8" '
+            'AND op.field = :param_2 COLLATE "utf-8"'
         )
 
     def test_operator_precedence_collate_5(self):
@@ -1419,7 +1419,7 @@ class OperatorPrecedenceTest(fixtures.TestBase, testing.AssertsCompiledSQL):
             select([self.table1.c.name]).order_by(
                 self.table1.c.name.collate('utf-8').desc()),
             "SELECT mytable.name FROM mytable "
-            "ORDER BY mytable.name COLLATE utf-8 DESC"
+            'ORDER BY mytable.name COLLATE "utf-8" DESC'
         )
 
     def test_operator_precedence_collate_6(self):
@@ -1427,7 +1427,7 @@ class OperatorPrecedenceTest(fixtures.TestBase, testing.AssertsCompiledSQL):
             select([self.table1.c.name]).order_by(
                 self.table1.c.name.collate('utf-8').desc().nullslast()),
             "SELECT mytable.name FROM mytable "
-            "ORDER BY mytable.name COLLATE utf-8 DESC NULLS LAST"
+            'ORDER BY mytable.name COLLATE "utf-8" DESC NULLS LAST'
         )
 
     def test_operator_precedence_collate_7(self):
@@ -1435,7 +1435,7 @@ class OperatorPrecedenceTest(fixtures.TestBase, testing.AssertsCompiledSQL):
             select([self.table1.c.name]).order_by(
                 self.table1.c.name.collate('utf-8').asc()),
             "SELECT mytable.name FROM mytable "
-            "ORDER BY mytable.name COLLATE utf-8 ASC"
+            'ORDER BY mytable.name COLLATE "utf-8" ASC'
         )
 
     def test_commutative_operators(self):

diff --git a/test/sql/test_quote.py b/test/sql/test_quote.py
index 94f9d62ac4582e0e614bd6f22458b7882d1e7d21..a436dde670335bfced1141b92bd1cf117b905504 100644 (file)
--- a/test/sql/test_quote.py
+++ b/test/sql/test_quote.py
@@ -454,6 +454,23 @@ class QuoteTest(fixtures.TestBase, AssertsCompiledSQL):
             'SELECT t1.col1 AS "ShouldQuote" FROM t1 ORDER BY "ShouldQuote"'
         )
 
+    def test_collate(self):
+        self.assert_compile(
+            column('foo').collate('utf8'),
+            "foo COLLATE utf8"
+        )
+
+        self.assert_compile(
+            column('foo').collate('fr_FR'),
+            'foo COLLATE "fr_FR"'
+        )
+
+        self.assert_compile(
+            column('foo').collate('utf8_GERMAN_ci'),
+            'foo COLLATE `utf8_GERMAN_ci`',
+            dialect="mysql"
+        )
+
     def test_join(self):
         # Lower case names, should not quote
         metadata = MetaData()