From: Mike Bayer Date: Tue, 10 Jan 2023 14:51:23 +0000 (-0500) Subject: fix ORM support for column-named bindparam() in crud .values() X-Git-Tag: rel_2_0_0rc3~25 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e636917a721f4bb01264a23736c9c81e462863cb;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git 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 --- diff --git a/doc/build/changelog/unreleased_14/9075.rst b/doc/build/changelog/unreleased_14/9075.rst new file mode 100644 index 0000000000..0d96be7708 --- /dev/null +++ b/doc/build/changelog/unreleased_14/9075.rst @@ -0,0 +1,18 @@ +.. change:: + :tags: bug, sql + :tickets: 9075 + :versions: 2.0.0rc3 + + 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. diff --git a/lib/sqlalchemy/sql/crud.py b/lib/sqlalchemy/sql/crud.py index ca215bd889..5017afa78e 100644 --- a/lib/sqlalchemy/sql/crud.py +++ b/lib/sqlalchemy/sql/crud.py @@ -212,25 +212,29 @@ def _get_crud_params( assert mp is not None spd = mp[0] stmt_parameter_tuples = list(spd.items()) + spd_str_key = {_column_as_key(key) for key in spd} elif compile_state._ordered_values: spd = compile_state._dict_parameters stmt_parameter_tuples = compile_state._ordered_values + assert spd is not None + spd_str_key = {_column_as_key(key) for key in spd} elif compile_state._dict_parameters: spd = compile_state._dict_parameters stmt_parameter_tuples = list(spd.items()) + spd_str_key = {_column_as_key(key) for key in spd} else: - stmt_parameter_tuples = spd = None + stmt_parameter_tuples = spd = spd_str_key = None # if we have statement parameters - set defaults in the # compiled params if compiler.column_keys is None: parameters = {} elif stmt_parameter_tuples: - assert spd is not None + assert spd_str_key is not None parameters = { _column_as_key(key): REQUIRED for key in compiler.column_keys - if key not in spd + if key not in spd_str_key } else: parameters = { diff --git a/test/orm/test_core_compilation.py b/test/orm/test_core_compilation.py index b71d644734..6736d55895 100644 --- a/test/orm/test_core_compilation.py +++ b/test/orm/test_core_compilation.py @@ -40,11 +40,13 @@ from sqlalchemy.testing import AssertsCompiledSQL from sqlalchemy.testing import eq_ from sqlalchemy.testing import fixtures from sqlalchemy.testing import is_ +from sqlalchemy.testing import Variation from sqlalchemy.testing.fixtures import fixture_session from sqlalchemy.testing.util import resolve_lambda from sqlalchemy.util.langhelpers import hybridproperty from .inheritance import _poly_fixtures from .test_query import QueryTest +from ..sql import test_compiler from ..sql.test_compiler import CorrelateTest as _CoreCorrelateTest # TODO: @@ -2689,3 +2691,29 @@ class CorrelateTest(fixtures.DeclarativeMappedTest, _CoreCorrelateTest): def _fixture(self): t1, t2 = self.classes("T1", "T2") return t1, t2, select(t1).where(t1.c.a == t2.c.a) + + +class CrudParamOverlapTest(test_compiler.CrudParamOverlapTest): + @testing.fixture( + params=Variation.generate_cases("type_", ["orm"]), + ids=["orm"], + ) + def crud_table_fixture(self, request): + type_ = request.param + + if type_.orm: + from sqlalchemy.orm import declarative_base + + Base = declarative_base() + + class Foo(Base): + __tablename__ = "mytable" + myid = Column(Integer, primary_key=True) + name = Column(String) + description = Column(String) + + table1 = Foo + else: + type_.fail() + + yield table1 diff --git a/test/sql/test_compiler.py b/test/sql/test_compiler.py index 2907c6e0e7..9947f34b6b 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 1c24d4c793..308f654f73 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 66971f64eb..ef8f117bcd 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"))