Fixes for table-bound version_id_col against mapped selectable

Fixed bug where a versioning column specified on a mapper against a
:func:`.select` construct where the version_id_col itself were against the
underlying table would incur additional loads when accessed, even if the
value were locally persisted by the flush.  The actual fix is a result of
the changes in :ticket:`4617`,  by fact that a :func:`.select` object no
longer has a ``.c`` attribute and therefore does not confuse the mapper
into thinking there's an unknown column value present.

Fixed bug in ORM versioning feature where assignment of an explicit
version_id for a counter configured against a mapped selectable where
version_id_col is against the underlying table would fail if the previous
value were expired; this was due to the fact that the  mapped attribute
would not be configured with active_history=True.

Fixes: #4194
Fixes: #4195
Change-Id: I214879f441f905bdd85a7411f90352af7399051d

diff --git a/doc/build/changelog/unreleased_14/4194.rst b/doc/build/changelog/unreleased_14/4194.rst
new file mode 100644 (file)
index 0000000..08d3439
--- /dev/null
+++ b/doc/build/changelog/unreleased_14/4194.rst
@@ -0,0 +1,11 @@
+.. change::
+    :tags: bug, orm
+    :tickets: 4194
+
+    Fixed bug where a versioning column specified on a mapper against a
+    :func:`.select` construct where the version_id_col itself were against the
+    underlying table would incur additional loads when accessed, even if the
+    value were locally persisted by the flush.  The actual fix is a result of
+    the changes in :ticket:`4617`,  by fact that a :func:`.select` object no
+    longer has a ``.c`` attribute and therefore does not confuse the mapper
+    into thinking there's an unknown column value present.

diff --git a/doc/build/changelog/unreleased_14/4195.rst b/doc/build/changelog/unreleased_14/4195.rst
new file mode 100644 (file)
index 0000000..f5ca494
--- /dev/null
+++ b/doc/build/changelog/unreleased_14/4195.rst
@@ -0,0 +1,10 @@
+.. change::
+    :tags: bug, orm
+    :tickets: 4195
+
+    Fixed bug in ORM versioning feature where assignment of an explicit
+    version_id for a counter configured against a mapped selectable where
+    version_id_col is against the underlying table would fail if the previous
+    value were expired; this was due to the fact that the  mapped attribute
+    would not be configured with active_history=True.
+

diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py
index 79e36026c8411796fc4a9913d922d28f169892ba..9abb5be40883a1aa8f88cf90e911be5987484800 100644 (file)
--- a/lib/sqlalchemy/orm/strategies.py
+++ b/lib/sqlalchemy/orm/strategies.py
@@ -198,7 +198,11 @@ class ColumnLoader(LoaderStrategy):
         active_history = (
             self.parent_property.active_history
             or self.columns[0].primary_key
-            or mapper.version_id_col in set(self.columns)
+            or (
+                mapper.version_id_col is not None
+                and mapper._columntoproperty.get(mapper.version_id_col, None)
+                is self.parent_property
+            )
         )
 
         _register_attribute(

diff --git a/test/orm/test_versioning.py b/test/orm/test_versioning.py
index 05132c30947c8d4a624a1cd7c548fff991eb60cc..ef53e613c0778e3980c90e0d5fbfe2df660e704f 100644 (file)
--- a/test/orm/test_versioning.py
+++ b/test/orm/test_versioning.py
@@ -6,6 +6,7 @@ import sqlalchemy as sa
 from sqlalchemy import Date
 from sqlalchemy import exc
 from sqlalchemy import ForeignKey
+from sqlalchemy import inspect
 from sqlalchemy import Integer
 from sqlalchemy import orm
 from sqlalchemy import select
@@ -13,6 +14,7 @@ from sqlalchemy import String
 from sqlalchemy import testing
 from sqlalchemy import TypeDecorator
 from sqlalchemy import util
+from sqlalchemy.orm import configure_mappers
 from sqlalchemy.orm import create_session
 from sqlalchemy.orm import exc as orm_exc
 from sqlalchemy.orm import mapper
@@ -26,6 +28,8 @@ from sqlalchemy.testing import engines
 from sqlalchemy.testing import eq_
 from sqlalchemy.testing import expect_warnings
 from sqlalchemy.testing import fixtures
+from sqlalchemy.testing import is_false
+from sqlalchemy.testing import is_true
 from sqlalchemy.testing.assertsql import CompiledSQL
 from sqlalchemy.testing.mock import patch
 from sqlalchemy.testing.schema import Column
@@ -2003,3 +2007,43 @@ class VersioningMappedSelectTest(fixtures.MappedTest):
             s1.query(Foo.id, Foo.value, Foo.version_id).order_by(Foo.id).all(),
             [(f1.id, "f1rev2", 2), (f2.id, "f2rev2", 2)],
         )
+
+    def test_implicit_no_readonly(self):
+        # test issue 4194
+        Foo = self.classes.Foo
+
+        s1 = self._implicit_version_fixture()
+        f1 = Foo(value="f1")
+        s1.add(f1)
+        s1.flush()
+
+        is_false(bool(inspect(Foo)._readonly_props))
+
+        def go():
+            eq_(f1.version_id, 1)
+
+        self.assert_sql_count(testing.db, go, 0)
+
+    def test_explicit_assign_from_expired(self):
+        # test issue 4195
+        Foo = self.classes.Foo
+
+        s1 = self._explicit_version_fixture()
+
+        configure_mappers()
+        is_true(Foo.version_id.impl.active_history)
+
+        f1 = Foo(value="f1", version_id=1)
+        s1.add(f1)
+
+        s1.flush()
+
+        s1.expire_all()
+
+        f1.value = "f2"
+        f1.version_id = 2
+
+        with conditional_sane_rowcount_warnings(
+            update=True, only_returning=True
+        ):
+            s1.flush()