detect and raise for version_id is NULL

The versioning feature does not support NULL for the version counter.
An exception is now raised if the version id is programmatic and
was set to NULL for an UPDATE.  Pull request courtesy Diana Clarke.

Fixes: #3673
Change-Id: I8b0da56234a7c7f5e7fde35536e09a6216a5e48a

diff --git a/doc/build/changelog/changelog_12.rst b/doc/build/changelog/changelog_12.rst
index 8cc8cd862bb72e92ff71ae22c359de93b85fa109..2d15358490b2366869ed2582f5eb4bcc0e28993b 100644 (file)
--- a/doc/build/changelog/changelog_12.rst
+++ b/doc/build/changelog/changelog_12.rst
@@ -26,6 +26,13 @@
         and the UPDATE now sets the version_id value to itself, so that
         concurrency checks still take place.
 
+    .. change:: 3673
+        :tags: bug, orm
+        :tickets: 3673
+
+        The versioning feature does not support NULL for the version counter.
+        An exception is now raised if the version id is programmatic and
+        was set to NULL for an UPDATE.  Pull request courtesy Diana Clarke.
 
     .. change:: 3796
         :tags: bug, orm

diff --git a/lib/sqlalchemy/orm/persistence.py b/lib/sqlalchemy/orm/persistence.py
index 86125d6f22a4819443c9dca6b604b5a42f11c791..e8a7e4c33e028276ebc7f38591039d7db00094c4 100644 (file)
--- a/lib/sqlalchemy/orm/persistence.py
+++ b/lib/sqlalchemy/orm/persistence.py
@@ -1032,6 +1032,11 @@ def _finalize_insert_update_commands(base_mapper, uowtransaction, states):
         else:
             mapper.dispatch.after_update(mapper, connection, state)
 
+        if mapper.version_id_col is not None:
+            if state_dict[mapper._version_id_prop.key] is None:
+                raise orm_exc.FlushError(
+                    "Instance does not contain a non-NULL version value")
+
 
 def _postfetch(mapper, uowtransaction, table,
                state, dict_, result, params, value_params):

diff --git a/test/orm/test_versioning.py b/test/orm/test_versioning.py
index 4c339d3bb55dff652cdeea5af92fb2cfc647d5df..9ade0a4ff133667e536943ddb665282b7433518e 100644 (file)
--- a/test/orm/test_versioning.py
+++ b/test/orm/test_versioning.py
@@ -20,6 +20,79 @@ def make_uuid():
     return uuid.uuid4().hex
 
 
+class NullVersionIdTest(fixtures.MappedTest):
+    __backend__ = True
+
+    @classmethod
+    def define_tables(cls, metadata):
+        Table('version_table', metadata,
+              Column('id', Integer, primary_key=True,
+                     test_needs_autoincrement=True),
+              Column('version_id', Integer),
+              Column('value', String(40), nullable=False))
+
+    @classmethod
+    def setup_classes(cls):
+        class Foo(cls.Basic):
+            pass
+
+    def _fixture(self):
+        Foo, version_table = self.classes.Foo, self.tables.version_table
+
+        mapper(
+            Foo, version_table,
+            version_id_col=version_table.c.version_id,
+            version_id_generator=False,
+        )
+
+        s1 = Session()
+        return s1
+
+    def test_null_version_id_insert(self):
+        Foo = self.classes.Foo
+
+        s1 = self._fixture()
+        f1 = Foo(value='f1')
+        s1.add(f1)
+
+        # Prior to the fix for #3673, you would have been allowed to insert
+        # the above record with a NULL version_id and you would have gotten
+        # the following error when you tried to update it. Now you should
+        # get a FlushError on the initial insert.
+        #
+        # A value is required for bind parameter 'version_table_version_id'
+        # UPDATE version_table SET value=?
+        #    WHERE version_table.id = ?
+        #    AND version_table.version_id = ?
+        # parameters: [{'version_table_id': 1, 'value': 'f1rev2'}]]
+
+        assert_raises_message(
+            sa.orm.exc.FlushError,
+            "Instance does not contain a non-NULL version value",
+            s1.commit)
+
+    def test_null_version_id_update(self):
+        Foo = self.classes.Foo
+
+        s1 = self._fixture()
+        f1 = Foo(value='f1', version_id=1)
+        s1.add(f1)
+        s1.commit()
+
+        # Prior to the fix for #3673, you would have been allowed to update
+        # the above record with a NULL version_id, and it would look like
+        # this, post commit: Foo(id=1, value='f1rev2', version_id=None). Now
+        # you should get a FlushError on update.
+
+        f1.value = 'f1rev2'
+        f1.version_id = None
+
+        assert_raises_message(
+            sa.orm.exc.FlushError,
+            "Instance does not contain a non-NULL version value",
+            s1.commit)
+
+
 class VersioningTest(fixtures.MappedTest):
     __backend__ = True