fix ORM support for column-named bindparam() in crud .values()

Fixed bug / regression where using :func:`.bindparam()` with the same name
as a column in the :meth:`.Update.values` method of :class:`.Update`, as
well as the :meth:`.Insert.values` method of :class:`.Insert` in 2.0 only,
would in some cases silently fail to honor the SQL expression in which the
parameter were presented, replacing the expression with a new parameter of
the same name and discarding any other elements of the SQL expression, such
as SQL functions, etc. The specific case would be statements that were
constructed against ORM entities rather than plain :class:`.Table`
instances, but would occur if the statement were invoked with a
:class:`.Session` or a :class:`.Connection`.

:class:`.Update` part of the issue was present in both 2.0 and 1.4 and is
backported to 1.4.

Fixes: #9075
Change-Id: Ie954bc1f492ec6a566163588182ef4910c7ee452

3 files changed, 186 insertions, 2 deletions

diff --git a/test/sql/test_compiler.py b/test/sql/test_compiler.py
index 2907c6e0e..9947f34b6 100644
--- a/test/sql/test_compiler.py
+++ b/test/sql/test_compiler.py
@@ -34,6 +34,7 @@ from sqlalchemy import Float
 from sqlalchemy import ForeignKey
 from sqlalchemy import func
 from sqlalchemy import Index
+from sqlalchemy import insert
 from sqlalchemy import Integer
 from sqlalchemy import intersect
 from sqlalchemy import join
@@ -62,6 +63,7 @@ from sqlalchemy import type_coerce
 from sqlalchemy import types
 from sqlalchemy import union
 from sqlalchemy import union_all
+from sqlalchemy import update
 from sqlalchemy import util
 from sqlalchemy.dialects import mssql
 from sqlalchemy.dialects import mysql
@@ -100,6 +102,7 @@ from sqlalchemy.testing import is_none
 from sqlalchemy.testing import is_true
 from sqlalchemy.testing import mock
 from sqlalchemy.testing import ne_
+from sqlalchemy.testing import Variation
 from sqlalchemy.testing.schema import pep435_enum
 from sqlalchemy.types import UserDefinedType
 
@@ -5192,6 +5195,179 @@ class BindParameterTest(AssertsCompiledSQL, fixtures.TestBase):
         )
 
 
+class CrudParamOverlapTest(AssertsCompiledSQL, fixtures.TestBase):
+    """tests for #9075.
+
+    we apparently allow same-column-named bindparams in values(), even though
+    we do *not* allow same-column-named bindparams in other parts of the
+    statement, but only if the bindparam is associated with that column in the
+    VALUES / SET clause. If you use a name that matches that of a column in
+    values() but associate it with a different column, you also get the error.
+
+    This is supported, see
+    test_insert.py::InsertTest::test_binds_that_match_columns and
+    test_update.py::UpdateTest::test_binds_that_match_columns.  The use
+    case makes sense because the "overlapping binds" issue is that using
+    a column name in bindparam() will conflict with the bindparam()
+    that crud.py is going to make for that column in VALUES / SET; but if we
+    are replacing the actual expression that would be in VALUES / SET, then
+    it's fine, there is no conflict.
+
+    The test suite is extended in
+    test/orm/test_core_compilation.py with ORM mappings that caused
+    the failure that was fixed by #9075.
+
+
+    """
+
+    __dialect__ = "default"
+
+    @testing.fixture(
+        params=Variation.generate_cases("type_", ["lowercase", "uppercase"]),
+        ids=["lowercase", "uppercase"],
+    )
+    def crud_table_fixture(self, request):
+        type_ = request.param
+
+        if type_.lowercase:
+            table1 = table(
+                "mytable",
+                column("myid", Integer),
+                column("name", String),
+                column("description", String),
+            )
+        elif type_.uppercase:
+            table1 = Table(
+                "mytable",
+                MetaData(),
+                Column("myid", Integer),
+                Column("name", String),
+                Column("description", String),
+            )
+        else:
+            type_.fail()
+
+        yield table1
+
+    def test_same_named_binds_insert_values(self, crud_table_fixture):
+        table1 = crud_table_fixture
+        stmt = insert(table1).values(
+            myid=bindparam("myid"),
+            description=func.coalesce(bindparam("description"), "default"),
+        )
+        self.assert_compile(
+            stmt,
+            "INSERT INTO mytable (myid, description) VALUES "
+            "(:myid, coalesce(:description, :coalesce_1))",
+        )
+
+        self.assert_compile(
+            stmt,
+            "INSERT INTO mytable (myid, description) VALUES "
+            "(:myid, coalesce(:description, :coalesce_1))",
+            params={"myid": 5, "description": "foo"},
+            checkparams={
+                "coalesce_1": "default",
+                "description": "foo",
+                "myid": 5,
+            },
+        )
+
+        self.assert_compile(
+            stmt,
+            "INSERT INTO mytable (myid, name, description) VALUES "
+            "(:myid, :name, coalesce(:description, :coalesce_1))",
+            params={"myid": 5, "description": "foo", "name": "bar"},
+            checkparams={
+                "coalesce_1": "default",
+                "description": "foo",
+                "myid": 5,
+                "name": "bar",
+            },
+        )
+
+    def test_same_named_binds_update_values(self, crud_table_fixture):
+        table1 = crud_table_fixture
+        stmt = update(table1).values(
+            myid=bindparam("myid"),
+            description=func.coalesce(bindparam("description"), "default"),
+        )
+        self.assert_compile(
+            stmt,
+            "UPDATE mytable SET myid=:myid, "
+            "description=coalesce(:description, :coalesce_1)",
+        )
+
+        self.assert_compile(
+            stmt,
+            "UPDATE mytable SET myid=:myid, "
+            "description=coalesce(:description, :coalesce_1)",
+            params={"myid": 5, "description": "foo"},
+            checkparams={
+                "coalesce_1": "default",
+                "description": "foo",
+                "myid": 5,
+            },
+        )
+
+        self.assert_compile(
+            stmt,
+            "UPDATE mytable SET myid=:myid, name=:name, "
+            "description=coalesce(:description, :coalesce_1)",
+            params={"myid": 5, "description": "foo", "name": "bar"},
+            checkparams={
+                "coalesce_1": "default",
+                "description": "foo",
+                "myid": 5,
+                "name": "bar",
+            },
+        )
+
+    def test_different_named_binds_insert_values(self, crud_table_fixture):
+        table1 = crud_table_fixture
+        stmt = insert(table1).values(
+            myid=bindparam("myid"),
+            name=func.coalesce(bindparam("description"), "default"),
+        )
+        self.assert_compile(
+            stmt,
+            "INSERT INTO mytable (myid, name) VALUES "
+            "(:myid, coalesce(:description, :coalesce_1))",
+        )
+
+        with expect_raises_message(
+            exc.CompileError, r"bindparam\(\) name 'description' is reserved "
+        ):
+            stmt.compile(column_keys=["myid", "description"])
+
+        with expect_raises_message(
+            exc.CompileError, r"bindparam\(\) name 'description' is reserved "
+        ):
+            stmt.compile(column_keys=["myid", "description", "name"])
+
+    def test_different_named_binds_update_values(self, crud_table_fixture):
+        table1 = crud_table_fixture
+        stmt = update(table1).values(
+            myid=bindparam("myid"),
+            name=func.coalesce(bindparam("description"), "default"),
+        )
+        self.assert_compile(
+            stmt,
+            "UPDATE mytable SET myid=:myid, "
+            "name=coalesce(:description, :coalesce_1)",
+        )
+
+        with expect_raises_message(
+            exc.CompileError, r"bindparam\(\) name 'description' is reserved "
+        ):
+            stmt.compile(column_keys=["myid", "description"])
+
+        with expect_raises_message(
+            exc.CompileError, r"bindparam\(\) name 'description' is reserved "
+        ):
+            stmt.compile(column_keys=["myid", "description", "name"])
+
+
 class CompileUXTest(fixtures.TestBase):
     """tests focused on calling stmt.compile() directly, user cases"""
 
diff --git a/test/sql/test_insert.py b/test/sql/test_insert.py
index 1c24d4c79..308f654f7 100644
--- a/test/sql/test_insert.py
+++ b/test/sql/test_insert.py
@@ -96,7 +96,11 @@ class InsertTest(_InsertTestBase, fixtures.TablesTest, AssertsCompiledSQL):
 
     def test_binds_that_match_columns(self):
         """test bind params named after column names
-        replace the normal SET/VALUES generation."""
+        replace the normal SET/VALUES generation.
+
+        See also test_compiler.py::CrudParamOverlapTest
+
+        """
 
         t = table("foo", column("x"), column("y"))
 
diff --git a/test/sql/test_update.py b/test/sql/test_update.py
index 66971f64e..ef8f117bc 100644
--- a/test/sql/test_update.py
+++ b/test/sql/test_update.py
@@ -317,7 +317,11 @@ class UpdateTest(_UpdateFromTestBase, fixtures.TablesTest, AssertsCompiledSQL):
 
     def test_binds_that_match_columns(self):
         """test bind params named after column names
-        replace the normal SET/VALUES generation."""
+        replace the normal SET/VALUES generation.
+
+        See also test_compiler.py::CrudParamOverlapTest
+
+        """
 
         t = table("foo", column("x"), column("y"))