Transaction.begin() didn't do anything.

begin() is supposed to abort the current transaction, but
Transaction.begin() did not.  Calling begin() on a transaction
*manager* worked fine, and is the intended way to do a begin()
in 3.3.  But calling begin() on a Transaction object is still
very easy to do (e.g., the older get_transaction().begin()
spelling still works), and shouldn't be a subtle disaster.

NEWS.txt

@@ -2,6 +2,25 @@ What's new in ZODB3 3.3 ?
 =========================
 Release date: DD-MMM-YYYY
 
+transaction
+-----------
+
+Growing pains:  ZODB 3.1 and 3.2 had a bug wherein Transaction.begin()
+didn't abort the current transaction if the only pending changes were in a
+subtransaction.  In ZODB 3.3, it's intended that transaction managers be
+used instead of invoking methods directly on Transaction objects, and
+calling begin() on a transaction manager didn't have this old bug.  However,
+Transaction.begin() still exists in 3.3, and it had a worse bug:  it never
+aborted the transaction (not even if changes were pending outside of
+subtransactions).  Transaction.begin() has been changed to abort the
+transaction, although it's still strongly recommended to invoke begin() on
+the relevant transaction manager instead.  For example,
+
+    import transaction
+    transaction.begin()
+
+if using the default ThreadTransactionManager (see news for 3.3a3 below).
+
 BTrees
 ------
 

src/ZODB/tests/testZODB.py

@@ -344,6 +344,77 @@ class ZODBTests(unittest.TestCase):
         self.obj = DecoyIndependent()
         self.readConflict()
 
+    def checkTxnBeginImpliesAbort(self):
+        # begin() should do an abort() first, if needed.
+        cn = self._db.open()
+        rt = cn.root()
+        rt['a'] = 1
+
+        transaction.begin()  # should abort adding 'a' to the root
+        rt = cn.root()
+        self.assertRaises(KeyError, rt.__getitem__, 'a')
+
+        # A longstanding bug:  this didn't work if changes were only in
+        # subtransactions.
+        transaction.begin()
+        rt = cn.root()
+        rt['a'] = 2
+        transaction.commit(1)
+
+        transaction.begin()
+        rt = cn.root()
+        self.assertRaises(KeyError, rt.__getitem__, 'a')
+
+        # One more time, mixing "top level" and subtransaction changes.
+        transaction.begin()
+        rt = cn.root()
+        rt['a'] = 3
+        transaction.commit(1)
+        rt['b'] = 4
+
+        transaction.begin()
+        rt = cn.root()
+        self.assertRaises(KeyError, rt.__getitem__, 'a')
+        self.assertRaises(KeyError, rt.__getitem__, 'b')
+
+        # That used methods of the default transaction *manager*.  Alas,
+        # that's not necessarily the same as using methods of the current
+        # transaction, and, in fact, when this test was written,
+        # Transaction.begin() didn't do anything (everything from here
+        # down failed).
+        cn = self._db.open()
+        rt = cn.root()
+        rt['a'] = 1
+
+        transaction.get().begin()  # should abort adding 'a' to the root
+        rt = cn.root()
+        self.assertRaises(KeyError, rt.__getitem__, 'a')
+
+        # A longstanding bug:  this didn't work if changes were only in
+        # subtransactions.
+        transaction.get().begin()
+        rt = cn.root()
+        rt['a'] = 2
+        transaction.get().commit(1)
+
+        transaction.get().begin()
+        rt = cn.root()
+        self.assertRaises(KeyError, rt.__getitem__, 'a')
+
+        # One more time, mixing "top level" and subtransaction changes.
+        transaction.get().begin()
+        rt = cn.root()
+        rt['a'] = 3
+        transaction.get().commit(1)
+        rt['b'] = 4
+
+        transaction.get().begin()
+        rt = cn.root()
+        self.assertRaises(KeyError, rt.__getitem__, 'a')
+        self.assertRaises(KeyError, rt.__getitem__, 'b')
+
+        cn.close()
+
 def test_suite():
     return unittest.makeSuite(ZODBTests, 'check')
 

src/transaction/_transaction.py

@@ -230,11 +230,14 @@ class Transaction(object):
                 self._resources.append(adapter)
 
     def begin(self):
-        # TODO: I'm not sure how this should be implemented.  Not doing
-        # anything now, but my best guess is: If nothing has happened
-        # yet, it's fine.  Otherwise, abort this transaction and let
-        # the txn manager create a new one.
-        pass
+        if (self._resources or
+              self._sub or
+              self._nonsub or
+              self._synchronizers):
+            self.abort()
+        # Else aborting wouldn't do anything, except if _manager is non-None,
+        # in which case it would do nothing besides uselessly free() this
+        # transaction.
 
     def commit(self, subtransaction=False):
         if not subtransaction and self._sub and self._resources:
@@ -247,8 +250,6 @@ class Transaction(object):
         if not subtransaction:
             for s in self._synchronizers:
                 s.beforeCompletion(self)
-
-        if not subtransaction:
             self.status = Status.COMMITTING
 
         self._commitResources(subtransaction)