launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/broken-build into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/broken-build/+merge/95142

Fixing something that broke the tests: some places called NodeKey.objects.create_token.  But Node creation has a signal handler attached to it which generates credentials and location information for the node to access the metadata service, which in turn creates a NodeKey if one did not already exist.  And so the unique constraint on NodeKey.node was violated when anything else called create_token for the node.

The fix: stop using create_node anywhere except as a helper to get_token_for_node (which first checks if a NodeKey already existed).  Make it private so that we don't accidentally increase its use again.
-- 
https://code.launchpad.net/~jtv/maas/broken-build/+merge/95142
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/broken-build into lp:maas.

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-02-28 11:39:59 +0000
+++ src/maasserver/tests/test_api.py	2012-02-29 10:45:22 +0000
@@ -179,7 +179,7 @@
         self.assertEqual(httplib.OK, response.status_code)
 
     def test_node_init_user_cannot_access(self):
-        token = NodeKey.objects.create_token(factory.make_node())
+        token = NodeKey.objects.get_token_for_node(factory.make_node())
         client = OAuthAuthenticatedClient(get_node_init_user(), token)
         response = client.get(self.get_uri('nodes/'), {'op': 'list'})
         self.assertEqual(httplib.FORBIDDEN, response.status_code)

=== modified file 'src/metadataserver/models.py'
--- src/metadataserver/models.py	2012-02-28 10:31:13 +0000
+++ src/metadataserver/models.py	2012-02-29 10:45:22 +0000
@@ -56,7 +56,7 @@
         and looks up the associated Node.
     """
 
-    def create_token(self, node):
+    def _create_token(self, node):
         """Create an OAuth token for a given node.
 
         :param node: The system that is to be allowed access to the metadata
@@ -91,7 +91,7 @@
             "Found %d keys for node (expected at most one)."
             % len(existing_nodekey))
         if len(existing_nodekey) == 0:
-            return self.create_token(node)
+            return self._create_token(node)
         else:
             [nodekey] = existing_nodekey
             return nodekey.token

=== modified file 'src/metadataserver/tests/test_api.py'
--- src/metadataserver/tests/test_api.py	2012-02-24 13:55:23 +0000
+++ src/metadataserver/tests/test_api.py	2012-02-29 10:45:22 +0000
@@ -96,7 +96,7 @@
 
     def test_get_node_for_request_finds_node(self):
         node = factory.make_node()
-        token = NodeKey.objects.create_token(node)
+        token = NodeKey.objects.get_token_for_node(node)
         request = self.fake_request(
             HTTP_AUTHORIZATION=self.make_oauth_header(oauth_token=token.key))
         self.assertEqual(node, get_node_for_request(request))
@@ -114,7 +114,7 @@
         """Create a test client logged in as if it were `node`."""
         if node is None:
             node = factory.make_node()
-        token = NodeKey.objects.create_token(node)
+        token = NodeKey.objects.get_token_for_node(node)
         return OAuthAuthenticatedClient(get_node_init_user(), token)
 
     def get(self, path, client=None):

=== modified file 'src/metadataserver/tests/test_models.py'
--- src/metadataserver/tests/test_models.py	2012-02-27 13:08:46 +0000
+++ src/metadataserver/tests/test_models.py	2012-02-29 10:45:22 +0000
@@ -22,15 +22,16 @@
 class TestNodeKeyManager(TestCase):
     """Test NodeKeyManager."""
 
-    def test_create_key_registers_node_key(self):
+    def test_get_token_for_node_registers_node_key(self):
         node = factory.make_node()
-        token = NodeKey.objects.create_token(node)
+        token = NodeKey.objects.get_token_for_node(node)
         nodekey = NodeKey.objects.get(node=node, key=token.key)
         self.assertNotEqual(None, nodekey)
+        self.assertEqual(token, nodekey.token)
 
     def test_get_node_for_key_finds_node(self):
         node = factory.make_node()
-        token = NodeKey.objects.create_token(node)
+        token = NodeKey.objects.get_token_for_node(node)
         self.assertEqual(node, NodeKey.objects.get_node_for_key(token.key))
 
     def test_get_node_for_key_raises_DoesNotExist_if_key_not_found(self):