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

diff --git a/doc/build/changelog/unreleased_20/10280.rst b/doc/build/changelog/unreleased_20/10280.rst
new file mode 100644 (file)
index 0000000..6b4c640
--- /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 b13c53249e75f87587cdbeb52eceeff1ffbe8fbc..71fca7e1f2b095a002bc0b70f08907503dcd2670 100644 (file)
--- 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 89d1e5c7fb2cb8281eb40536881d07ee99e92841..edb802d226ddcaa3f9c014d714c832444ccaa116 100644 (file)
--- 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 62e1f1cb5b88472494f3f0ffbe34bdecb2b1bd66..7f0c8a74a03710c6c789627f73f49a0bf6a6e938 100644 (file)
--- 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),