launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Badin has proposed merging lp:~rvb/maas/maas-pserv-fail into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~rvb/maas/maas-pserv-fail/+merge/99476

This branch simply changes the order in settings.MIDDLEWARE_CLASSES.  'maasserver.middleware.APIErrorsMiddleware' should be before 'django.middleware.transaction.TransactionMiddleware'. APIErrorsMiddleware processes exceptions and returns responses and doing that it prevents all the middleware classes setup before it to be able to process exception. See https://docs.djangoproject.com/en/dev/topics/http/middleware/.
-- 
https://code.launchpad.net/~rvb/maas/maas-pserv-fail/+merge/99476
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/maas-pserv-fail into lp:maas.

=== modified file 'src/maas/settings.py'
--- src/maas/settings.py	2012-03-21 13:03:25 +0000
+++ src/maas/settings.py	2012-03-27 08:01:17 +0000
@@ -207,6 +207,8 @@
 MIDDLEWARE_CLASSES = (
     'django.middleware.common.CommonMiddleware',
     'django.contrib.sessions.middleware.SessionMiddleware',
+    'maasserver.middleware.APIErrorsMiddleware',
+    'metadataserver.middleware.MetadataErrorsMiddleware',
     'django.middleware.transaction.TransactionMiddleware',
     'django.middleware.csrf.CsrfViewMiddleware',
     'django.middleware.csrf.CsrfResponseMiddleware',
@@ -214,8 +216,6 @@
     'django.contrib.auth.middleware.AuthenticationMiddleware',
     'django.contrib.messages.middleware.MessageMiddleware',
     'maasserver.middleware.AccessMiddleware',
-    'maasserver.middleware.APIErrorsMiddleware',
-    'metadataserver.middleware.MetadataErrorsMiddleware',
 )
 
 ROOT_URLCONF = 'maas.urls'

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-03-23 13:40:48 +0000
+++ src/maasserver/tests/test_api.py	2012-03-27 08:01:17 +0000
@@ -42,6 +42,7 @@
     LoggedInTestCase,
     TestCase,
     )
+from maastesting.testcase import TransactionTestCase
 from metadataserver.models import (
     NodeKey,
     NodeUserData,
@@ -1234,7 +1235,7 @@
         self.assertEqual(stored_value, value)
 
 
-class APIErrorsTest(APITestCase):
+class APIErrorsTest(APIv10TestMixin, TransactionTestCase):
 
     def test_internal_error_generate_proper_api_response(self):
         error_message = factory.getRandomString()
@@ -1248,3 +1249,29 @@
         self.assertEqual(
             (httplib.INTERNAL_SERVER_ERROR, error_message),
             (response.status_code, response.content))
+
+    def test_Node_post_save_error_rollbacks_transaction(self):
+        # If post_save raises an exception after a Node is added, the
+        # whole transaction is rolledback.
+        error_message = factory.getRandomString()
+
+        def raise_exception(*args, **kwargs):
+            raise RuntimeError(error_message)
+        post_save.connect(raise_exception, sender=Node)
+        self.addCleanup(post_save.disconnect, raise_exception, sender=Node)
+
+        architecture = factory.getRandomChoice(ARCHITECTURE_CHOICES)
+        hostname = factory.getRandomString()
+        response = self.client.post(self.get_uri('nodes/'), {
+            'op': 'new',
+            'hostname': hostname,
+            'architecture': architecture,
+            'after_commissioning_action': '2',
+            'mac_addresses': ['aa:bb:cc:dd:ee:ff'],
+        })
+
+        self.assertEqual(
+            (httplib.INTERNAL_SERVER_ERROR, error_message),
+            (response.status_code, response.content))
+        self.assertRaises(
+            Node.DoesNotExist, Node.objects.get, hostname=hostname)

=== modified file 'src/maastesting/testcase.py'
--- src/maastesting/testcase.py	2012-03-19 17:05:35 +0000
+++ src/maastesting/testcase.py	2012-03-27 08:01:17 +0000
@@ -12,6 +12,7 @@
 __all__ = [
     'TestCase',
     'TestModelTestCase',
+    'TransactionTestCase',
     ]
 
 import unittest
@@ -24,12 +25,7 @@
 import testtools
 
 
-class TestCase(testtools.TestCase, django.test.TestCase):
-    """`TestCase` for Metal as a Service.
-
-    Supports test resources and fixtures.
-    """
-
+class TestCaseBase(testtools.TestCase):
     # testresources.ResourcedTestCase does something similar to this class
     # (with respect to setUpResources and tearDownResources) but it explicitly
     # up-calls to unittest.TestCase instead of using super() even though it is
@@ -39,7 +35,7 @@
     resources = ()
 
     def setUp(self):
-        super(TestCase, self).setUp()
+        super(TestCaseBase, self).setUp()
         self.setUpResources()
 
     def setUpResources(self):
@@ -48,7 +44,7 @@
 
     def tearDown(self):
         self.tearDownResources()
-        super(TestCase, self).tearDown()
+        super(TestCaseBase, self).tearDown()
 
     def tearDownResources(self):
         testresources.tearDownResources(
@@ -59,6 +55,23 @@
     assertItemsEqual = unittest.TestCase.assertItemsEqual
 
 
+class TestCase(TestCaseBase, django.test.TestCase):
+    """`TestCase` for Metal as a Service.
+
+    Supports test resources and fixtures.
+    """
+
+
+class TransactionTestCase(TestCaseBase, django.test.TransactionTestCase):
+    """`TransactionTestCase` for Metal as a Service.
+
+    A version of TestCase that supports transactions.
+
+    The basic Django TestCase class uses transactions to speed up tests
+    so this class should be used when tests involve transactions.
+    """
+
+
 class TestModelTestCase(TestCase):
     """A custom test case that adds support for test-only models.