From d1c6617a22d1cb1887245c9b5182ee289871483c Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Fri, 25 Aug 2023 10:48:59 -0400 Subject: [PATCH] automatically create proxy col for already-used col in values The :class:`.Values` construct will now automatically create a proxy (i.e. a copy) of a :class:`_sql.column` if the column were already associated with an existing FROM clause. This allows that an expression like ``values_obj.c.colname`` will produce the correct FROM clause even in the case that ``colname`` was passed as a :class:`_sql.column` that was already used with a previous :class:`.Values` or other table construct. Originally this was considered to be a candidate for an error condition, however it's likely this pattern is already in widespread use so it's now added to support. * adjust unrelated dml test recently added for update..returning * case to not rely upon ordering Fixes: #10280 Change-Id: I6e60e5b7cb7abd6a7bbd4722970ebf025596ab9c --- doc/build/changelog/unreleased_20/10280.rst | 13 +++ lib/sqlalchemy/sql/selectable.py | 8 ++ test/orm/dml/test_update_delete_where.py | 2 +- test/sql/test_values.py | 93 +++++++++++++++++++++ 4 files changed, 115 insertions(+), 1 deletion(-) create mode 100644 doc/build/changelog/unreleased_20/10280.rst diff --git a/doc/build/changelog/unreleased_20/10280.rst b/doc/build/changelog/unreleased_20/10280.rst new file mode 100644 index 0000000000..6b4c64085f --- /dev/null +++ b/doc/build/changelog/unreleased_20/10280.rst @@ -0,0 +1,13 @@ +.. change:: + :tags: bug, sql + :tickets: 10280 + + The :class:`.Values` construct will now automatically create a proxy (i.e. + a copy) of a :class:`_sql.column` if the column were already associated + with an existing FROM clause. This allows that an expression like + ``values_obj.c.colname`` will produce the correct FROM clause even in the + case that ``colname`` was passed as a :class:`_sql.column` that was already + used with a previous :class:`.Values` or other table construct. + Originally this was considered to be a candidate for an error condition, + however it's likely this pattern is already in widespread use so it's + now added to support. diff --git a/lib/sqlalchemy/sql/selectable.py b/lib/sqlalchemy/sql/selectable.py index b13c53249e..71fca7e1f2 100644 --- a/lib/sqlalchemy/sql/selectable.py +++ b/lib/sqlalchemy/sql/selectable.py @@ -3169,6 +3169,7 @@ class Values(roles.InElementRole, Generative, LateralFromClause): ): super().__init__() self._column_args = columns + if name is None: self._unnamed = True self.name = _anonymous_label.safe_construct(id(self), "anon") @@ -3262,6 +3263,13 @@ class Values(roles.InElementRole, Generative, LateralFromClause): def _populate_column_collection(self) -> None: for c in self._column_args: + if c.table is not None and c.table is not self: + _, c = c._make_proxy(self) + else: + # if the column was used in other contexts, ensure + # no memoizations of other FROM clauses. + # see test_values.py -> test_auto_proxy_select_direct_col + c._reset_memoizations() self._columns.add(c) c.table = self diff --git a/test/orm/dml/test_update_delete_where.py b/test/orm/dml/test_update_delete_where.py index 89d1e5c7fb..edb802d226 100644 --- a/test/orm/dml/test_update_delete_where.py +++ b/test/orm/dml/test_update_delete_where.py @@ -1114,7 +1114,7 @@ class UpdateDeleteTest(fixtures.MappedTest): ) result = sess.execute(stmt) - eq_(result.all(), [(2, "jack", 37), (4, "jane", 27)]) + eq_(set(result), {(2, "jack", 37), (4, "jane", 27)}) eq_([john.age, jack.age, jill.age, jane.age], [25, 37, 29, 27]) eq_( diff --git a/test/sql/test_values.py b/test/sql/test_values.py index 62e1f1cb5b..7f0c8a74a0 100644 --- a/test/sql/test_values.py +++ b/test/sql/test_values.py @@ -7,8 +7,10 @@ from sqlalchemy import ForeignKey from sqlalchemy import Integer from sqlalchemy import String from sqlalchemy import Table +from sqlalchemy import table from sqlalchemy import testing from sqlalchemy import true +from sqlalchemy import values from sqlalchemy.engine import default from sqlalchemy.sql import func from sqlalchemy.sql import select @@ -17,6 +19,8 @@ from sqlalchemy.sql.compiler import FROM_LINTING from sqlalchemy.testing import AssertsCompiledSQL from sqlalchemy.testing import expect_raises_message from sqlalchemy.testing import fixtures +from sqlalchemy.testing import is_ +from sqlalchemy.testing import is_not from sqlalchemy.util import OrderedDict @@ -70,6 +74,95 @@ class ValuesTest(fixtures.TablesTest, AssertsCompiledSQL): ): str(v1) + @testing.fixture + def _auto_proxy_fixture(self): + c1 = column("q", Integer) + c2 = column("p", Integer) + t = table("t", c1) # noqa: F841 + + v1 = values(c1, c2).data([(1, 2), (3, 4)]) + + return c1, c2, t, v1 + + def test_auto_proxy_col_ownership(self, _auto_proxy_fixture): + """test #10280""" + + c1, c2, t, v1 = _auto_proxy_fixture + + is_(c2, v1.c.p) + is_not(c1, v1.c.q) + + def test_auto_proxy_select_c_col(self, _auto_proxy_fixture): + """test #10280""" + + c1, c2, t, v1 = _auto_proxy_fixture + self.assert_compile(select(t.c.q), "SELECT t.q FROM t") + self.assert_compile( + select(v1.c.q), + "SELECT q FROM (VALUES (:param_1, :param_2), " + "(:param_3, :param_4))", + checkparams={ + "param_1": 1, + "param_2": 2, + "param_3": 3, + "param_4": 4, + }, + ) + + def test_auto_proxy_select_direct_col(self, _auto_proxy_fixture): + """test #10280""" + + c1, c2, t, v1 = _auto_proxy_fixture + self.assert_compile(select(c1), "SELECT t.q FROM t") + + # for VALUES, the column does not have its set_parent called up front. + # this is to make the construction of values() faster, as the values.c + # use case is not required in order to use the construct + self.assert_compile(select(c2), "SELECT p") + + # once we call v.c, then it's set up. + # patch for #10280 added an extra step to make sure this works + # even after the previous compile is called. + # is this how it should work? not sure, just testing how it is + # right now + v1.c.p + + self.assert_compile( + select(c2), + "SELECT p FROM (VALUES (:param_1, :param_2), " + "(:param_3, :param_4))", + checkparams={ + "param_1": 1, + "param_2": 2, + "param_3": 3, + "param_4": 4, + }, + ) + + def test_auto_proxy_make_new_values(self, _auto_proxy_fixture): + """test #10280""" + + c1, c2, t, v1 = _auto_proxy_fixture + + self.assert_compile( + select(v1.c.p), + "SELECT p FROM (VALUES (:param_1, :param_2), " + "(:param_3, :param_4))", + checkparams={ + "param_1": 1, + "param_2": 2, + "param_3": 3, + "param_4": 4, + }, + ) + + v2 = values(c1, c2).data([(5, 6)]) + self.assert_compile( + select(v2.c.p), + "SELECT p FROM (VALUES (:param_1, :param_2))", + checkparams={"param_1": 5, "param_2": 6}, + ) + def test_column_quoting(self): v1 = Values( column("CaseSensitive", Integer), -- 2.47.3