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

7 files changed, 81 insertions, 12 deletions

diff --git a/doc/build/changelog/changelog_12.rst b/doc/build/changelog/changelog_12.rst
index 9e0f55899..834dc074d 100644
--- 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 0fc60e9d9..9aebf6042 100644
--- 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 a450efaf0..1f8129382 100644
--- 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 8f697b27e..49642acdd 100644
--- 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 ce119cf50..9924c9547 100644
--- 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 0e0a8b29c..7c3ce1389 100644
--- 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 94f9d62ac..a436dde67 100644
--- 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()