Detect no params w/ manual version_id counter and set to itself

Fixed bug where programmatic version_id counter in conjunction with
joined table inheritance would fail if the version_id counter
were not actually incremented and no other values on the base table
were modified, as the UPDATE would have an empty SET clause.  Since
programmatic version_id where version counter is not incremented
is a documented use case, this specific condition is now detected
and the UPDATE now sets the version_id value to itself, so that
concurrency checks still take place.

Change-Id: I80e385bffeed4851cc20131cbe983c173a46f655
Fixes: #3996

diff --git a/doc/build/changelog/changelog_12.rst b/doc/build/changelog/changelog_12.rst
index 0f7b7b9af39869c9b7482a67fa54c09964f1d496..1324da8bbd6c3b777d8d6cd8dd897d4f58396c46 100644 (file)
--- a/doc/build/changelog/changelog_12.rst
+++ b/doc/build/changelog/changelog_12.rst
@@ -13,6 +13,20 @@
 .. changelog::
     :version: 1.2.0b1
 
+    .. change:: 3996
+        :tags: bug, orm
+        :tickets: 3996
+
+        Fixed bug where programmatic version_id counter in conjunction with
+        joined table inheritance would fail if the version_id counter
+        were not actually incremented and no other values on the base table
+        were modified, as the UPDATE would have an empty SET clause.  Since
+        programmatic version_id where version counter is not incremented
+        is a documented use case, this specific condition is now detected
+        and the UPDATE now sets the version_id value to itself, so that
+        concurrency checks still take place.
+
+
     .. change:: 3796
         :tags: bug, orm
         :tickets: 3796

diff --git a/lib/sqlalchemy/orm/persistence.py b/lib/sqlalchemy/orm/persistence.py
index 588a1d6962dfd2579533f79aeb88f4ac5407df78..86125d6f22a4819443c9dca6b604b5a42f11c791 100644 (file)
--- a/lib/sqlalchemy/orm/persistence.py
+++ b/lib/sqlalchemy/orm/persistence.py
@@ -511,12 +511,19 @@ def _collect_update_commands(
                     continue
 
             col = mapper.version_id_col
+            no_params = not params and not value_params
             params[col._label] = update_version_id
 
             if (bulk or col.key not in params) and \
                     mapper.version_id_generator is not False:
                 val = mapper.version_id_generator(update_version_id)
                 params[col.key] = val
+            elif mapper.version_id_generator is False and no_params:
+                # no version id generator, no values set on the table,
+                # and version id wasn't manually incremented.
+                # set version id to itself so we get an UPDATE
+                # statement
+                params[col.key] = update_version_id
 
         elif not (params or value_params):
             continue

diff --git a/test/orm/test_versioning.py b/test/orm/test_versioning.py
index 40b37309701f27f302e434c0de8440a7f4266af2..4c339d3bb55dff652cdeea5af92fb2cfc647d5df 100644 (file)
--- a/test/orm/test_versioning.py
+++ b/test/orm/test_versioning.py
@@ -1284,3 +1284,65 @@ class ManualVersionTest(fixtures.MappedTest):
         sess.commit()
 
         eq_(a1.vid, 2)
+
+
+class ManualInheritanceVersionTest(fixtures.MappedTest):
+    run_define_tables = 'each'
+    __backend__ = True
+
+    @classmethod
+    def define_tables(cls, metadata):
+        Table(
+            "a", metadata,
+            Column(
+                'id', Integer, primary_key=True,
+                test_needs_autoincrement=True),
+            Column('data', String(30)),
+            Column('vid', Integer, nullable=False)
+        )
+
+        Table(
+            "b", metadata,
+            Column(
+                'id', Integer, ForeignKey('a.id'), primary_key=True),
+            Column('b_data', String(30)),
+        )
+
+    @classmethod
+    def setup_classes(cls):
+        class A(cls.Basic):
+            pass
+
+        class B(A):
+            pass
+
+    @classmethod
+    def setup_mappers(cls):
+        mapper(
+            cls.classes.A, cls.tables.a, version_id_col=cls.tables.a.c.vid,
+            version_id_generator=False)
+
+        mapper(
+            cls.classes.B, cls.tables.b, inherits=cls.classes.A)
+
+    def test_no_increment(self):
+        sess = Session()
+        b1 = self.classes.B()
+
+        b1.vid = 1
+        b1.data = 'd1'
+        sess.add(b1)
+        sess.commit()
+
+        # change col on subtable only without
+        # incrementing version id
+        b1.b_data = 'bd2'
+        sess.commit()
+
+        eq_(b1.vid, 1)
+
+        b1.b_data = 'd3'
+        b1.vid = 2
+        sess.commit()
+
+        eq_(b1.vid, 2)