launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06850
[Merge] lp:~rvb/maas/maas-unique-mac-bug-960694 into lp:maas
Raphaël Badin has proposed merging lp:~rvb/maas/maas-unique-mac-bug-960694 into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #960694 in MAAS: "Adding nodes are not mac validated"
https://bugs.launchpad.net/maas/+bug/960694
For more details, see:
https://code.launchpad.net/~rvb/maas/maas-unique-mac-bug-960694/+merge/98826
This branch adds unique=True to the mac_address field inside the MACAddress object. South created the transition. The only trick is that the validation error (that happens when one is trying to save a duplicated mac) is triggered internally by Django; since the form uses a MultipleMACAddressField I had to catch the ValidationError to re-raise a proper error.
Drive-by fix: I refactored the tests to remove the setUp method (which was creating objects for many tests that simply were not using them).
--
https://code.launchpad.net/~rvb/maas/maas-unique-mac-bug-960694/+merge/98826
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/maas-unique-mac-bug-960694 into lp:maas.
=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py 2012-03-21 07:30:59 +0000
+++ src/maasserver/forms.py 2012-03-22 12:18:31 +0000
@@ -120,10 +120,23 @@
['One or more MAC Addresses is invalid.'])
return valid
+ def add_mac(self, node, mac):
+ try:
+ node.add_mac_address(mac)
+ except ValidationError, e:
+ is_unique_validation_error = (
+ 'mac_address' in e.message_dict and
+ e.message_dict['mac_address'] == [
+ 'Mac address with this Mac address already exists.'])
+ if is_unique_validation_error:
+ raise ValidationError(
+ {'mac_addresses': [
+ 'Mac address %s already in use.' % mac]})
+
def save(self):
node = super(NodeWithMACAddressesForm, self).save()
for mac in self.cleaned_data['mac_addresses']:
- node.add_mac_address(mac)
+ self.add_mac(node, mac)
if self.cleaned_data['hostname'] == "":
node.set_mac_based_hostname(self.cleaned_data['mac_addresses'][0])
return node
=== added file 'src/maasserver/migrations/0002_macaddress_unique.py'
--- src/maasserver/migrations/0002_macaddress_unique.py 1970-01-01 00:00:00 +0000
+++ src/maasserver/migrations/0002_macaddress_unique.py 2012-03-22 12:18:31 +0000
@@ -0,0 +1,104 @@
+# encoding: utf-8
+import datetime
+from south.db import db
+from south.v2 import SchemaMigration
+from django.db import models
+
+class Migration(SchemaMigration):
+
+ def forwards(self, orm):
+
+ # Adding unique constraint on 'MACAddress', fields ['mac_address']
+ db.create_unique('maasserver_macaddress', ['mac_address'])
+
+
+ def backwards(self, orm):
+
+ # Removing unique constraint on 'MACAddress', fields ['mac_address']
+ db.delete_unique('maasserver_macaddress', ['mac_address'])
+
+
+ models = {
+ 'auth.group': {
+ 'Meta': {'object_name': 'Group'},
+ 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+ 'name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '80'}),
+ 'permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'})
+ },
+ 'auth.permission': {
+ 'Meta': {'ordering': "('content_type__app_label', 'content_type__model', 'codename')", 'unique_together': "(('content_type', 'codename'),)", 'object_name': 'Permission'},
+ 'codename': ('django.db.models.fields.CharField', [], {'max_length': '100'}),
+ 'content_type': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['contenttypes.ContentType']"}),
+ 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+ 'name': ('django.db.models.fields.CharField', [], {'max_length': '50'})
+ },
+ 'auth.user': {
+ 'Meta': {'object_name': 'User'},
+ 'date_joined': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}),
+ 'email': ('django.db.models.fields.EmailField', [], {'unique': 'True', 'max_length': '75', 'blank': 'True'}),
+ 'first_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}),
+ 'groups': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Group']", 'symmetrical': 'False', 'blank': 'True'}),
+ 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+ 'is_active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}),
+ 'is_staff': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
+ 'is_superuser': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
+ 'last_login': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}),
+ 'last_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}),
+ 'password': ('django.db.models.fields.CharField', [], {'max_length': '128'}),
+ 'user_permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}),
+ 'username': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '30'})
+ },
+ 'contenttypes.contenttype': {
+ 'Meta': {'ordering': "('name',)", 'unique_together': "(('app_label', 'model'),)", 'object_name': 'ContentType', 'db_table': "'django_content_type'"},
+ 'app_label': ('django.db.models.fields.CharField', [], {'max_length': '100'}),
+ 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+ 'model': ('django.db.models.fields.CharField', [], {'max_length': '100'}),
+ 'name': ('django.db.models.fields.CharField', [], {'max_length': '100'})
+ },
+ 'maasserver.config': {
+ 'Meta': {'object_name': 'Config'},
+ 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+ 'name': ('django.db.models.fields.CharField', [], {'max_length': '255'}),
+ 'value': ('maasserver.fields.JSONObjectField', [], {'null': 'True'})
+ },
+ 'maasserver.filestorage': {
+ 'Meta': {'object_name': 'FileStorage'},
+ 'data': ('django.db.models.fields.files.FileField', [], {'max_length': '255'}),
+ 'filename': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '100'}),
+ 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'})
+ },
+ 'maasserver.macaddress': {
+ 'Meta': {'object_name': 'MACAddress'},
+ 'created': ('django.db.models.fields.DateField', [], {}),
+ 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+ 'mac_address': ('maasserver.fields.MACAddressField', [], {'unique': 'True'}),
+ 'node': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['maasserver.Node']"}),
+ 'updated': ('django.db.models.fields.DateTimeField', [], {})
+ },
+ 'maasserver.node': {
+ 'Meta': {'object_name': 'Node'},
+ 'after_commissioning_action': ('django.db.models.fields.IntegerField', [], {'default': '0'}),
+ 'architecture': ('django.db.models.fields.CharField', [], {'default': "u'i386'", 'max_length': '10'}),
+ 'created': ('django.db.models.fields.DateField', [], {}),
+ 'hostname': ('django.db.models.fields.CharField', [], {'default': "u''", 'max_length': '255', 'blank': 'True'}),
+ 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+ 'owner': ('django.db.models.fields.related.ForeignKey', [], {'default': 'None', 'to': "orm['auth.User']", 'null': 'True', 'blank': 'True'}),
+ 'power_type': ('django.db.models.fields.CharField', [], {'default': "u''", 'max_length': '10', 'blank': 'True'}),
+ 'status': ('django.db.models.fields.IntegerField', [], {'default': '0', 'max_length': '10'}),
+ 'system_id': ('django.db.models.fields.CharField', [], {'default': "u'node-ce4489fe-73f9-11e1-a631-00219bd0a2de'", 'unique': 'True', 'max_length': '41'}),
+ 'updated': ('django.db.models.fields.DateTimeField', [], {})
+ },
+ 'maasserver.sshkeys': {
+ 'Meta': {'object_name': 'SSHKeys'},
+ 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+ 'key': ('django.db.models.fields.TextField', [], {}),
+ 'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['maasserver.UserProfile']"})
+ },
+ 'maasserver.userprofile': {
+ 'Meta': {'object_name': 'UserProfile'},
+ 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+ 'user': ('django.db.models.fields.related.OneToOneField', [], {'to': "orm['auth.User']", 'unique': 'True'})
+ }
+ }
+
+ complete_apps = ['maasserver']
=== modified file 'src/maasserver/models.py'
--- src/maasserver/models.py 2012-03-22 09:43:50 +0000
+++ src/maasserver/models.py 2012-03-22 12:18:31 +0000
@@ -464,7 +464,7 @@
:ivar node: The `Node` related to this `MACAddress`.
"""
- mac_address = MACAddressField()
+ mac_address = MACAddressField(unique=True)
node = models.ForeignKey(Node, editable=False)
class Meta:
=== modified file 'src/maasserver/testing/factory.py'
--- src/maasserver/testing/factory.py 2012-03-21 05:56:36 +0000
+++ src/maasserver/testing/factory.py 2012-03-22 12:18:31 +0000
@@ -69,6 +69,7 @@
node = Node()
node.save()
mac = MACAddress(mac_address=address, node=node)
+ mac.save()
return mac
def make_user(self, username=None, password=None, email=None):
=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py 2012-03-21 05:56:36 +0000
+++ src/maasserver/tests/test_api.py 2012-03-22 12:18:31 +0000
@@ -182,6 +182,27 @@
self.assertIn('text/html', response['Content-Type'])
self.assertEqual("Unknown operation.", response.content)
+ def test_POST_fails_if_mac_duplicated(self):
+ # Mac Addresses should be unique.
+ mac = 'aa:bb:cc:dd:ee:ff'
+ factory.make_mac_address(mac)
+ architecture = factory.getRandomChoice(ARCHITECTURE_CHOICES)
+ response = self.client.post(
+ self.get_uri('nodes/'),
+ {
+ 'op': 'new',
+ 'architecture': architecture,
+ 'hostname': factory.getRandomString(),
+ 'mac_addresses': [mac],
+ })
+ parsed_result = json.loads(response.content)
+
+ self.assertEqual(httplib.BAD_REQUEST, response.status_code)
+ self.assertIn('application/json', response['Content-Type'])
+ self.assertEqual(
+ ["Mac address %s already in use." % mac],
+ parsed_result['mac_addresses'])
+
def test_POST_fails_with_bad_operation(self):
# If the operation ('op=operation_name') specified in the
# request data is unknown, a 'Bad request' response is returned.
@@ -667,24 +688,25 @@
class MACAddressAPITest(APITestCase):
- def setUp(self):
- super(MACAddressAPITest, self).setUp()
- self.node = factory.make_node()
- self.mac1 = self.node.add_mac_address('aa:bb:cc:dd:ee:ff')
- self.mac2 = self.node.add_mac_address('22:bb:cc:dd:aa:ff')
+ def createNodeWithMacs(self):
+ node = factory.make_node()
+ mac1 = node.add_mac_address('aa:bb:cc:dd:ee:ff')
+ mac2 = node.add_mac_address('22:bb:cc:dd:aa:ff')
+ return node, mac1, mac2
def test_macs_GET(self):
# The api allows for fetching the list of the MAC Addresss for a node.
+ node, mac1, mac2 = self.createNodeWithMacs()
response = self.client.get(
- self.get_uri('nodes/%s/macs/') % self.node.system_id)
+ self.get_uri('nodes/%s/macs/') % node.system_id)
parsed_result = json.loads(response.content)
self.assertEqual(httplib.OK, response.status_code)
self.assertEqual(2, len(parsed_result))
self.assertEqual(
- self.mac1.mac_address, parsed_result[0]['mac_address'])
+ mac1.mac_address, parsed_result[0]['mac_address'])
self.assertEqual(
- self.mac2.mac_address, parsed_result[1]['mac_address'])
+ mac2.mac_address, parsed_result[1]['mac_address'])
def test_macs_GET_forbidden(self):
# When fetching MAC Addresses, the api returns a 'Forbidden' (403)
@@ -706,9 +728,10 @@
def test_macs_GET_node_not_found(self):
# When fetching a MAC Address, the api returns a 'Not Found' (404)
# error if the MAC Address does not exist.
+ node = factory.make_node()
response = self.client.get(
self.get_uri(
- 'nodes/%s/macs/00-aa-22-cc-44-dd/') % self.node.system_id)
+ 'nodes/%s/macs/00-aa-22-cc-44-dd/') % node.system_id)
self.assertEqual(httplib.NOT_FOUND, response.status_code)
@@ -726,30 +749,33 @@
def test_macs_GET_node_bad_request(self):
# When fetching a MAC Address, the api returns a 'Bad Request' (400)
# error if the MAC Address is not valid.
+ node = factory.make_node()
response = self.client.get(
- self.get_uri('nodes/%s/macs/invalid-mac/') % self.node.system_id)
+ self.get_uri('nodes/%s/macs/invalid-mac/') % node.system_id)
self.assertEqual(400, response.status_code)
def test_macs_POST_add_mac(self):
# The api allows to add a MAC Address to an existing node.
- nb_macs = MACAddress.objects.filter(node=self.node).count()
+ node = factory.make_node()
+ nb_macs = MACAddress.objects.filter(node=node).count()
response = self.client.post(
- self.get_uri('nodes/%s/macs/') % self.node.system_id,
- {'mac_address': 'AA:BB:CC:DD:EE:FF'})
+ self.get_uri('nodes/%s/macs/') % node.system_id,
+ {'mac_address': '01:BB:CC:DD:EE:FF'})
parsed_result = json.loads(response.content)
self.assertEqual(httplib.OK, response.status_code)
- self.assertEqual('AA:BB:CC:DD:EE:FF', parsed_result['mac_address'])
+ self.assertEqual('01:BB:CC:DD:EE:FF', parsed_result['mac_address'])
self.assertEqual(
nb_macs + 1,
- MACAddress.objects.filter(node=self.node).count())
+ MACAddress.objects.filter(node=node).count())
def test_macs_POST_add_mac_invalid(self):
# A 'Bad Request' response is returned if one tries to add an invalid
# MAC Address to a node.
+ node = self.createNodeWithMacs()[0]
response = self.client.post(
- self.get_uri('nodes/%s/macs/') % self.node.system_id,
+ self.get_uri('nodes/%s/macs/') % node.system_id,
{'mac_address': 'invalid-mac'})
parsed_result = json.loads(response.content)
@@ -761,42 +787,46 @@
def test_macs_DELETE_mac(self):
# The api allows to delete a MAC Address.
- nb_macs = self.node.macaddress_set.count()
+ node, mac1, mac2 = self.createNodeWithMacs()
+ nb_macs = node.macaddress_set.count()
response = self.client.delete(
self.get_uri('nodes/%s/macs/%s/') % (
- self.node.system_id, self.mac1.mac_address))
+ node.system_id, mac1.mac_address))
self.assertEqual(204, response.status_code)
self.assertEqual(
nb_macs - 1,
- self.node.macaddress_set.count())
+ node.macaddress_set.count())
def test_macs_DELETE_mac_forbidden(self):
# When deleting a MAC Address, the api returns a 'Forbidden' (403)
# error if the node is not visible to the logged-in user.
+ node, mac1, _ = self.createNodeWithMacs()
other_node = factory.make_node(
status=NODE_STATUS.ALLOCATED, owner=factory.make_user())
response = self.client.delete(
self.get_uri('nodes/%s/macs/%s/') % (
- other_node.system_id, self.mac1.mac_address))
+ other_node.system_id, mac1.mac_address))
self.assertEqual(httplib.FORBIDDEN, response.status_code)
def test_macs_DELETE_not_found(self):
# When deleting a MAC Address, the api returns a 'Not Found' (404)
# error if no existing MAC Address is found.
+ node = factory.make_node()
response = self.client.delete(
self.get_uri('nodes/%s/macs/%s/') % (
- self.node.system_id, '00-aa-22-cc-44-dd'))
+ node.system_id, '00-aa-22-cc-44-dd'))
self.assertEqual(httplib.NOT_FOUND, response.status_code)
def test_macs_DELETE_bad_request(self):
# When deleting a MAC Address, the api returns a 'Bad Request' (400)
# error if the provided MAC Address is not valid.
+ node = factory.make_node()
response = self.client.delete(
self.get_uri('nodes/%s/macs/%s/') % (
- self.node.system_id, 'invalid-mac'))
+ node.system_id, 'invalid-mac'))
self.assertEqual(httplib.BAD_REQUEST, response.status_code)
