From: Mike Bayer <mike_mp@zzzcomputing.com>
Date: Sat, 11 May 2019 02:36:40 +0000 (-0400)
Subject: Correct fix and tests for #4661
X-Git-Tag: rel_1_3_4~18
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=77d947b88ddf4878543ae230ced5cffd295fdb36;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git

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 e9e6975ca3..f1a73c7cd5 100644
--- 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 fdb50f37db..5ae0dc5dc0 100644
--- 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
         ):