From 615ec3e640373bcb10311ba1d5cfc72e570b6534 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Fri, 10 Jan 2020 13:16:43 -0500 Subject: [PATCH] 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 --- doc/build/changelog/unreleased_14/4194.rst | 11 ++++++ doc/build/changelog/unreleased_14/4195.rst | 10 +++++ lib/sqlalchemy/orm/strategies.py | 6 ++- test/orm/test_versioning.py | 44 ++++++++++++++++++++++ 4 files changed, 70 insertions(+), 1 deletion(-) create mode 100644 doc/build/changelog/unreleased_14/4194.rst create mode 100644 doc/build/changelog/unreleased_14/4195.rst diff --git a/doc/build/changelog/unreleased_14/4194.rst b/doc/build/changelog/unreleased_14/4194.rst new file mode 100644 index 0000000000..08d343934d --- /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 index 0000000000..f5ca494015 --- /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 79e36026c8..9abb5be408 100644 --- 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 05132c3094..ef53e613c0 100644 --- 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() -- 2.47.3