Correct fix and tests for #4661

For #4661 we need to still warn if we are only deleting one row,
even if sane multi rowcount is false.   Tests were failing for
pyodbc since the warning was removed for the single-row case.
the UPDATE logic raises if a single row doesn't match even
if sane multi rowcount is false, so this is now more consistent
with that.  Add tests for the UPDATE case also.  It is possible
there are already tests for this but as the DELETE case wasn't
well covered it's not clear.

Fixes: #4661
Change-Id: Ie57f765ff31bf806206837c5fbfe449b02ebf4be
(cherry picked from commit bdafff8982d9e7fbf9a39182f1fb8e65d475bbb9)

diff --git a/lib/sqlalchemy/orm/persistence.py b/lib/sqlalchemy/orm/persistence.py
index e9e6975ca3dd1b4d440e30539c62cb1e0143cd29..f1a73c7cd5f5b4fd491964c36c64c695f600afb9 100644 (file)
--- a/lib/sqlalchemy/orm/persistence.py
+++ b/lib/sqlalchemy/orm/persistence.py
@@ -1301,15 +1301,11 @@ def _emit_delete_statements(
         expected = len(del_objects)
         rows_matched = -1
         only_warn = False
-        if connection.dialect.supports_sane_multi_rowcount:
-            c = connection.execute(statement, del_objects)
-
-            if not need_version_id:
-                only_warn = True
-
-            rows_matched = c.rowcount
 
-        elif need_version_id:
+        if (
+            need_version_id
+            and not connection.dialect.supports_sane_multi_rowcount
+        ):
             if connection.dialect.supports_sane_rowcount:
                 rows_matched = 0
                 # execute deletes individually so that versioned
@@ -1335,10 +1331,15 @@ def _emit_delete_statements(
 
         if (
             base_mapper.confirm_deleted_rows
-            and connection.dialect.supports_sane_multi_rowcount
             and rows_matched > -1
             and expected != rows_matched
+            and (
+                connection.dialect.supports_sane_multi_rowcount
+                or len(del_objects) == 1
+            )
         ):
+            # TODO: why does this "only warn" if versioning is turned off,
+            # whereas the UPDATE raises?
             if only_warn:
                 util.warn(
                     "DELETE statement on table '%s' expected to "

diff --git a/test/orm/test_unitofworkv2.py b/test/orm/test_unitofworkv2.py
index fdb50f37dbb658e67c211ef01835ceb4f4f4a53a..5ae0dc5dc0c8b09ccc1045aa9f6d925d07ba1dcc 100644 (file)
--- a/test/orm/test_unitofworkv2.py
+++ b/test/orm/test_unitofworkv2.py
@@ -1569,6 +1569,8 @@ class RowswitchM2OTest(fixtures.MappedTest):
 
 
 class BasicStaleChecksTest(fixtures.MappedTest):
+    __backend__ = True
+
     @classmethod
     def define_tables(cls, metadata):
         Table(
@@ -1758,7 +1760,72 @@ class BasicStaleChecksTest(fixtures.MappedTest):
             sess.flush,
         )
 
-    def test_delete_multi_broken_multi_rowcount(self):
+    def test_update_single_broken_multi_rowcount_still_raises(self):
+        # raise occurs for single row UPDATE that misses even if
+        # supports_sane_multi_rowcount is False
+        Parent, Child = self._fixture()
+        sess = Session()
+        p1 = Parent(id=1, data=2, child=None)
+        sess.add(p1)
+        sess.flush()
+
+        sess.execute(self.tables.parent.delete())
+        p1.data = 3
+
+        with patch.object(
+            config.db.dialect, "supports_sane_multi_rowcount", False
+        ):
+            assert_raises_message(
+                orm_exc.StaleDataError,
+                r"UPDATE statement on table 'parent' expected to "
+                r"update 1 row\(s\); 0 were matched.",
+                sess.flush,
+            )
+
+    def test_update_multi_broken_multi_rowcount_doesnt_raise(self):
+        # raise does not occur for multirow UPDATE that misses if
+        # supports_sane_multi_rowcount is False, even if rowcount is still
+        # correct
+        Parent, Child = self._fixture()
+        sess = Session()
+        p1 = Parent(id=1, data=2, child=None)
+        p2 = Parent(id=2, data=3, child=None)
+        sess.add_all([p1, p2])
+        sess.flush()
+
+        sess.execute(self.tables.parent.delete())
+        p1.data = 3
+        p2.data = 4
+
+        with patch.object(
+            config.db.dialect, "supports_sane_multi_rowcount", False
+        ):
+            # no raise
+            sess.flush()
+
+    def test_delete_single_broken_multi_rowcount_still_warns(self):
+        Parent, Child = self._fixture()
+        sess = Session()
+        p1 = Parent(id=1, data=2, child=None)
+        sess.add(p1)
+        sess.flush()
+        sess.flush()
+
+        sess.execute(self.tables.parent.delete())
+        sess.delete(p1)
+
+        # only one row, so it warns
+        with patch.object(
+            config.db.dialect, "supports_sane_multi_rowcount", False
+        ):
+            assert_raises_message(
+                exc.SAWarning,
+                r"DELETE statement on table 'parent' expected to "
+                r"delete 1 row\(s\); 0 were matched.",
+                sess.flush,
+            )
+
+    def test_delete_multi_broken_multi_rowcount_doesnt_warn(self):
         Parent, Child = self._fixture()
         sess = Session()
         p1 = Parent(id=1, data=2, child=None)
@@ -1770,6 +1837,10 @@ class BasicStaleChecksTest(fixtures.MappedTest):
         sess.delete(p1)
         sess.delete(p2)
 
+        # if the dialect reports supports_sane_multi_rowcount as false,
+        # if there were more than one row deleted, need to ensure the
+        # rowcount result is ignored.  psycopg2 + batch mode reports the
+        # wrong number, not -1. see issue #4661
         with patch.object(
             config.db.dialect, "supports_sane_multi_rowcount", False
         ):