Fixed #19861 -- Transaction ._dirty flag improvement

There were a couple of errors in ._dirty flag handling:
  * It started as None, but was never reset to None.
  * The _dirty flag was sometimes used to indicate if the connection
    was inside transaction management, but this was not done
    consistently. This also meant the flag had three separate values.
  * The None value had a special meaning, causing for example inability
    to commit() on new connection unless enter/leave tx management was
    done.
  * The _dirty was tracking "connection in transaction" state, but only
    in managed transactions.
  * Some tests never reset the transaction state of the used connection.
  * And some additional less important changes.

This commit has some potential for regressions, but as the above list
shows, the current situation isn't perfect either.

5 files changed, 128 insertions, 26 deletions

diff --git a/tests/delete_regress/tests.py b/tests/delete_regress/tests.py
index e007ebdd76..9fcc19ba71 100644
--- a/tests/delete_regress/tests.py
+++ b/tests/delete_regress/tests.py
@@ -23,11 +23,13 @@ class DeleteLockingTest(TransactionTestCase):
         # Put both DB connections into managed transaction mode
         transaction.enter_transaction_management()
         transaction.managed(True)
-        self.conn2._enter_transaction_management(True)
+        self.conn2.enter_transaction_management()
+        self.conn2.managed(True)
 
     def tearDown(self):
         # Close down the second connection.
         transaction.leave_transaction_management()
+        self.conn2.abort()
         self.conn2.close()
 
     @skipUnlessDBFeature('test_db_allows_multiple_connections')
diff --git a/tests/middleware/tests.py b/tests/middleware/tests.py
index e6512e4ace..4e5fd8ea6b 100644
--- a/tests/middleware/tests.py
+++ b/tests/middleware/tests.py
@@ -683,6 +683,9 @@ class TransactionMiddlewareTest(TransactionTestCase):
         self.response = HttpResponse()
         self.response.status_code = 200
 
+    def tearDown(self):
+        transaction.abort()
+
     def test_request(self):
         TransactionMiddleware().process_request(self.request)
         self.assertTrue(transaction.is_managed())
@@ -697,10 +700,14 @@ class TransactionMiddlewareTest(TransactionTestCase):
         self.assertEqual(Band.objects.count(), 1)
 
     def test_unmanaged_response(self):
+        transaction.enter_transaction_management()
         transaction.managed(False)
+        self.assertEqual(Band.objects.count(), 0)
         TransactionMiddleware().process_response(self.request, self.response)
         self.assertFalse(transaction.is_managed())
-        self.assertFalse(transaction.is_dirty())
+        # The transaction middleware doesn't commit/rollback if management
+        # has been disabled.
+        self.assertTrue(transaction.is_dirty())
 
     def test_exception(self):
         transaction.enter_transaction_management()
@@ -708,8 +715,8 @@ class TransactionMiddlewareTest(TransactionTestCase):
         Band.objects.create(name='The Beatles')
         self.assertTrue(transaction.is_dirty())
         TransactionMiddleware().process_exception(self.request, None)
-        self.assertEqual(Band.objects.count(), 0)
         self.assertFalse(transaction.is_dirty())
+        self.assertEqual(Band.objects.count(), 0)
 
     def test_failing_commit(self):
         # It is possible that connection.commit() fails. Check that
@@ -724,8 +731,8 @@ class TransactionMiddlewareTest(TransactionTestCase):
             self.assertTrue(transaction.is_dirty())
             with self.assertRaises(IntegrityError):
                 TransactionMiddleware().process_response(self.request, None)
-            self.assertEqual(Band.objects.count(), 0)
             self.assertFalse(transaction.is_dirty())
+            self.assertEqual(Band.objects.count(), 0)
             self.assertFalse(transaction.is_managed())
         finally:
             del connections[DEFAULT_DB_ALIAS].commit
diff --git a/tests/select_for_update/tests.py b/tests/select_for_update/tests.py
index e3e4d9e7e2..c5a04881c9 100644
--- a/tests/select_for_update/tests.py
+++ b/tests/select_for_update/tests.py
@@ -24,7 +24,7 @@ requires_threading = unittest.skipUnless(threading, 'requires threading')
 class SelectForUpdateTests(TransactionTestCase):
 
     def setUp(self):
-        transaction.enter_transaction_management(True)
+        transaction.enter_transaction_management()
         transaction.managed(True)
         self.person = Person.objects.create(name='Reinhardt')
 
@@ -48,9 +48,8 @@ class SelectForUpdateTests(TransactionTestCase):
         try:
             # We don't really care if this fails - some of the tests will set
             # this in the course of their run.
-            transaction.managed(False)
-            transaction.leave_transaction_management()
-            self.new_connection.leave_transaction_management()
+            transaction.abort()
+            self.new_connection.abort()
         except transaction.TransactionManagementError:
             pass
         self.new_connection.close()
@@ -73,7 +72,7 @@ class SelectForUpdateTests(TransactionTestCase):
 
     def end_blocking_transaction(self):
         # Roll back the blocking transaction.
-        self.new_connection._rollback()
+        self.new_connection.rollback()
 
     def has_for_update_sql(self, tested_connection, nowait=False):
         # Examine the SQL that was executed to determine whether it
@@ -119,6 +118,7 @@ class SelectForUpdateTests(TransactionTestCase):
         """
         self.start_blocking_transaction()
         status = []
+
         thread = threading.Thread(
             target=self.run_select_for_update,
             args=(status,),
@@ -164,7 +164,7 @@ class SelectForUpdateTests(TransactionTestCase):
         try:
             # We need to enter transaction management again, as this is done on
             # per-thread basis
-            transaction.enter_transaction_management(True)
+            transaction.enter_transaction_management()
             transaction.managed(True)
             people = list(
                 Person.objects.all().select_for_update(nowait=nowait)
@@ -177,6 +177,7 @@ class SelectForUpdateTests(TransactionTestCase):
         finally:
             # This method is run in a separate thread. It uses its own
             # database connection. Close it without waiting for the GC.
+            transaction.abort()
             connection.close()
 
     @requires_threading
@@ -271,13 +272,3 @@ class SelectForUpdateTests(TransactionTestCase):
         """
         people = list(Person.objects.select_for_update())
         self.assertTrue(transaction.is_dirty())
-
-    @skipUnlessDBFeature('has_select_for_update')
-    def test_transaction_not_dirty_unmanaged(self):
-        """ If we're not under txn management, the txn will never be
-        marked as dirty.
-        """
-        transaction.managed(False)
-        transaction.leave_transaction_management()
-        people = list(Person.objects.select_for_update())
-        self.assertFalse(transaction.is_dirty())
diff --git a/tests/transactions/tests.py b/tests/transactions/tests.py
index f3246eef2a..a1edf53fcb 100644
--- a/tests/transactions/tests.py
+++ b/tests/transactions/tests.py
@@ -165,7 +165,6 @@ class TransactionRollbackTests(TransactionTestCase):
     def execute_bad_sql(self):
         cursor = connection.cursor()
         cursor.execute("INSERT INTO transactions_reporter (first_name, last_name) VALUES ('Douglas', 'Adams');")
-        transaction.set_dirty()
 
     @skipUnlessDBFeature('requires_rollback_on_dirty_transaction')
     def test_bad_sql(self):
@@ -306,5 +305,4 @@ class TransactionContextManagerTests(TransactionTestCase):
             with transaction.commit_on_success():
                 cursor = connection.cursor()
                 cursor.execute("INSERT INTO transactions_reporter (first_name, last_name) VALUES ('Douglas', 'Adams');")
-                transaction.set_dirty()
         transaction.rollback()
diff --git a/tests/transactions_regress/tests.py b/tests/transactions_regress/tests.py
index e76208d72c..8cc68b7e0d 100644
--- a/tests/transactions_regress/tests.py
+++ b/tests/transactions_regress/tests.py
@@ -138,7 +138,8 @@ class TestTransactionClosing(TransactionTestCase):
         @transaction.commit_on_success
         def create_system_user():
             "Create a user in a transaction"
-            user = User.objects.create_user(username='system', password='iamr00t', email='root@SITENAME.com')
+            user = User.objects.create_user(username='system', password='iamr00t',
+                                            email='root@SITENAME.com')
             # Redundant, just makes sure the user id was read back from DB
             Mod.objects.create(fld=user.pk)
 
@@ -161,6 +162,83 @@ class TestTransactionClosing(TransactionTestCase):
         """
         self.test_failing_query_transaction_closed()
 
+@skipIf(connection.vendor == 'sqlite' and
+        (connection.settings_dict['NAME'] == ':memory:' or
+         not connection.settings_dict['NAME']),
+        'Test uses multiple connections, but in-memory sqlite does not support this')
+class TestNewConnection(TransactionTestCase):
+    """
+    Check that new connections don't have special behaviour.
+    """
+    def setUp(self):
+        self._old_backend = connections[DEFAULT_DB_ALIAS]
+        settings = self._old_backend.settings_dict.copy()
+        opts = settings['OPTIONS'].copy()
+        if 'autocommit' in opts:
+            opts['autocommit'] = False
+        settings['OPTIONS'] = opts
+        new_backend = self._old_backend.__class__(settings, DEFAULT_DB_ALIAS)
+        connections[DEFAULT_DB_ALIAS] = new_backend
+
+    def tearDown(self):
+        try:
+            connections[DEFAULT_DB_ALIAS].abort()
+        except Exception:
+            import ipdb; ipdb.set_trace()
+        finally:
+            connections[DEFAULT_DB_ALIAS].close()
+            connections[DEFAULT_DB_ALIAS] = self._old_backend
+
+    def test_commit(self):
+        """
+        Users are allowed to commit and rollback connections.
+        """
+        # The starting value is False, not None.
+        self.assertIs(connection._dirty, False)
+        list(Mod.objects.all())
+        self.assertTrue(connection.is_dirty())
+        connection.commit()
+        self.assertFalse(connection.is_dirty())
+        list(Mod.objects.all())
+        self.assertTrue(connection.is_dirty())
+        connection.rollback()
+        self.assertFalse(connection.is_dirty())
+
+    def test_enter_exit_management(self):
+        orig_dirty = connection._dirty
+        connection.enter_transaction_management()
+        connection.leave_transaction_management()
+        self.assertEqual(orig_dirty, connection._dirty)
+
+    def test_commit_unless_managed(self):
+        cursor = connection.cursor()
+        cursor.execute("INSERT into transactions_regress_mod (fld) values (2)")
+        connection.commit_unless_managed()
+        self.assertFalse(connection.is_dirty())
+        self.assertEqual(len(Mod.objects.all()), 1)
+        self.assertTrue(connection.is_dirty())
+        connection.commit_unless_managed()
+        self.assertFalse(connection.is_dirty())
+
+    def test_commit_unless_managed_in_managed(self):
+        cursor = connection.cursor()
+        connection.enter_transaction_management()
+        transaction.managed(True)
+        cursor.execute("INSERT into transactions_regress_mod (fld) values (2)")
+        connection.commit_unless_managed()
+        self.assertTrue(connection.is_dirty())
+        connection.rollback()
+        self.assertFalse(connection.is_dirty())
+        self.assertEqual(len(Mod.objects.all()), 0)
+        connection.commit()
+        connection.leave_transaction_management()
+        self.assertFalse(connection.is_dirty())
+        self.assertEqual(len(Mod.objects.all()), 0)
+        self.assertTrue(connection.is_dirty())
+        connection.commit_unless_managed()
+        self.assertFalse(connection.is_dirty())
+        self.assertEqual(len(Mod.objects.all()), 0)
+
 
 @skipUnless(connection.vendor == 'postgresql',
             "This test only valid for PostgreSQL")
@@ -171,9 +249,11 @@ class TestPostgresAutocommit(TransactionTestCase):
     """
     def setUp(self):
         from psycopg2.extensions import (ISOLATION_LEVEL_AUTOCOMMIT,
-                                         ISOLATION_LEVEL_READ_COMMITTED)
+                                         ISOLATION_LEVEL_READ_COMMITTED,
+                                         TRANSACTION_STATUS_IDLE)
         self._autocommit = ISOLATION_LEVEL_AUTOCOMMIT
         self._read_committed = ISOLATION_LEVEL_READ_COMMITTED
+        self._idle = TRANSACTION_STATUS_IDLE
 
         # We want a clean backend with autocommit = True, so
         # first we need to do a bit of work to have that.
@@ -186,7 +266,11 @@ class TestPostgresAutocommit(TransactionTestCase):
         connections[DEFAULT_DB_ALIAS] = new_backend
 
     def tearDown(self):
-        connections[DEFAULT_DB_ALIAS] = self._old_backend
+        try:
+            connections[DEFAULT_DB_ALIAS].abort()
+        finally:
+            connections[DEFAULT_DB_ALIAS].close()
+            connections[DEFAULT_DB_ALIAS] = self._old_backend
 
     def test_initial_autocommit_state(self):
         self.assertTrue(connection.features.uses_autocommit)
@@ -214,6 +298,26 @@ class TestPostgresAutocommit(TransactionTestCase):
         transaction.leave_transaction_management()
         self.assertEqual(connection.isolation_level, self._autocommit)
 
+    def test_enter_autocommit(self):
+        transaction.enter_transaction_management()
+        transaction.managed(True)
+        self.assertEqual(connection.isolation_level, self._read_committed)
+        list(Mod.objects.all())
+        self.assertTrue(transaction.is_dirty())
+        # Enter autocommit mode again.
+        transaction.enter_transaction_management(False)
+        transaction.managed(False)
+        self.assertFalse(transaction.is_dirty())
+        self.assertEqual(
+            connection.connection.get_transaction_status(),
+            self._idle)
+        list(Mod.objects.all())
+        self.assertFalse(transaction.is_dirty())
+        transaction.leave_transaction_management()
+        self.assertEqual(connection.isolation_level, self._read_committed)
+        transaction.leave_transaction_management()
+        self.assertEqual(connection.isolation_level, self._autocommit)
+
 
 class TestManyToManyAddTransaction(TransactionTestCase):
     def test_manyrelated_add_commit(self):
@@ -247,7 +351,7 @@ class SavepointTest(TransactionTestCase):
 
         work()
 
-    @skipIf(connection.vendor == 'mysql' and \
+    @skipIf(connection.vendor == 'mysql' and
             connection.features._mysql_storage_engine == 'MyISAM',
             "MyISAM MySQL storage engine doesn't support savepoints")
     @skipUnlessDBFeature('uses_savepoints')