launchpad-reviewers team mailing list archive

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

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/clean-save into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/clean-save/+merge/105024

As per discussion here: https://code.launchpad.net/~jtv/maas/timestamped-model/+merge/104503

This re-unifies the save() definitions I created on the model classes that used to derive from CommonInfo and now derive from TimestampedModel.  It turns out that calling self.full_clean() on model save() is something we basically want to do all the time.  (Raphaël and I went into alternative places to put the check, but nothing was quite as good as the save method).

And so in this branch I give these save() definitions their own mix-in base class.  And for good measure, I made all of our concrete model classes use it.  Along the way this revealed, as you'll note in the test, that saving a non-unique commissioning result should have been a ValidationError (an error raised by python-side validation) rather than an IntegrityError (an error raised by the database).

I also noticed along the way that I accidentally gave SSHKey a Meta class while it already had one (not noticed because the older one came last and overrode the newer one, which wasn't going to have any effect until the model migrated into its own module).  And that, for some reason I haven't figured out yet, the imports-formatting tool didn't do its job on src/maasserver/models/timestampedmodel.py.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/clean-save/+merge/105024
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/clean-save into lp:maas.

=== modified file 'src/maasserver/models/__init__.py'
--- src/maasserver/models/__init__.py	2012-05-05 09:47:03 +0000
+++ src/maasserver/models/__init__.py	2012-05-08 03:06:19 +0000
@@ -72,6 +72,7 @@
     NodeStateViolation,
     )
 from maasserver.fields import MACAddressField
+from maasserver.models.cleansave import CleanSave
 from maasserver.models.config import Config
 from maasserver.models.timestampedmodel import TimestampedModel
 from metadataserver import nodeinituser
@@ -364,7 +365,7 @@
         return None
 
 
-class Node(TimestampedModel):
+class Node(CleanSave, TimestampedModel):
     """A `Node` represents a physical machine used by the MAAS Server.
 
     :ivar system_id: The unique identifier for this `Node`.
@@ -426,11 +427,6 @@
         else:
             return self.system_id
 
-    def save(self, *args, **kwargs):
-        # Automatically check validity before saving.
-        self.full_clean()
-        return super(Node, self).save(*args, **kwargs)
-
     def clean_status(self):
         """Check a node's status transition against the node-status FSM."""
         old_status = get_db_state(self, 'status')
@@ -595,7 +591,7 @@
 mac_re = re.compile(r'^([0-9a-fA-F]{2}:){5}[0-9a-fA-F]{2}$')
 
 
-class MACAddress(TimestampedModel):
+class MACAddress(CleanSave, TimestampedModel):
     """A `MACAddress` represents a `MAC address
     <http://en.wikipedia.org/wiki/MAC_address>`_ attached to a :class:`Node`.
 
@@ -613,11 +609,6 @@
     def __unicode__(self):
         return self.mac_address
 
-    def save(self, *args, **kwargs):
-        # Automatically check validity before saving.
-        self.full_clean()
-        return super(MACAddress, self).save(*args, **kwargs)
-
     def unique_error_message(self, model_class, unique_check):
         if unique_check == ('mac_address',):
                 return "This MAC address is already registered."
@@ -688,7 +679,7 @@
         return User.objects.filter(id__in=user_ids)
 
 
-class UserProfile(models.Model):
+class UserProfile(CleanSave, models.Model):
     """A User profile to store MAAS specific methods and fields.
 
     :ivar user: The related User_.
@@ -844,7 +835,7 @@
 MAX_KEY_DISPLAY = 50
 
 
-class SSHKey(TimestampedModel):
+class SSHKey(CleanSave, TimestampedModel):
     """A `SSHKey` represents a user public SSH key.
 
     Users will be able to access `Node`s using any of their registered keys.
@@ -853,9 +844,6 @@
     :ivar key: The ssh public key.
     """
 
-    class Meta(DefaultMeta):
-        """Needed for South to recognize this model."""
-
     objects = SSHKeyManager()
 
     user = models.ForeignKey(User, null=False, editable=False)
@@ -863,15 +851,10 @@
     key = models.TextField(
         null=False, editable=True, validators=[validate_ssh_public_key])
 
-    class Meta:
+    class Meta(DefaultMeta):
         verbose_name = "SSH key"
         unique_together = ('user', 'key')
 
-    def save(self, *args, **kwargs):
-        # Automatically check validity before saving.
-        self.full_clean()
-        return super(SSHKey, self).save(*args, **kwargs)
-
     def unique_error_message(self, model_class, unique_check):
         if unique_check == ('user', 'key'):
                 return "This key has already been added for this user."
@@ -993,7 +976,7 @@
                 FileStorage.storage.delete(path)
 
 
-class FileStorage(models.Model):
+class FileStorage(CleanSave, models.Model):
     """A simple file storage keyed on file name.
 
     :ivar filename: A unique file name to use for the data being stored.

=== added file 'src/maasserver/models/cleansave.py'
--- src/maasserver/models/cleansave.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/models/cleansave.py	2012-05-08 03:06:19 +0000
@@ -0,0 +1,36 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Model mixin: check `full_clean` on every `save`."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = [
+    'CleanSave',
+    ]
+
+
+class CleanSave:
+    """Mixin for model classes.
+
+    This adds a call to `self.full_clean` to every `save`, so that you can
+    never save a model object in an unaccepted state.
+
+    This was meant to be a standard part of Django model classes, but was
+    left out purely for backwards compatibility_, which is not an issue for
+    us.
+
+    Derive your model from :class:`CleanSave` before deriving from
+    :class:`django.db.models.Model` if you need the `full_clean` to happen
+    before the real `save` to the database.
+
+    .. _compatibility: https://code.djangoproject.com/ticket/13100#comment:2
+    """
+    def save(self, *args, **kwargs):
+        self.full_clean()
+        return super(CleanSave, self).save(*args, **kwargs)

=== modified file 'src/maasserver/models/timestampedmodel.py'
--- src/maasserver/models/timestampedmodel.py	2012-05-04 03:13:45 +0000
+++ src/maasserver/models/timestampedmodel.py	2012-05-08 03:06:19 +0000
@@ -15,7 +15,10 @@
     ]
 
 
-from django.db.models import DateTimeField, Model
+from django.db.models import (
+    DateTimeField,
+    Model,
+    )
 from maasserver import DefaultMeta
 
 

=== modified file 'src/metadataserver/models/__init__.py'
--- src/metadataserver/models/__init__.py	2012-04-23 04:20:42 +0000
+++ src/metadataserver/models/__init__.py	2012-05-08 03:06:19 +0000
@@ -30,6 +30,7 @@
     create_auth_token,
     Node,
     )
+from maasserver.models.cleansave import CleanSave
 from metadataserver.fields import (
     Bin,
     BinaryField,
@@ -117,7 +118,7 @@
         return self.get(key=key).node
 
 
-class NodeKey(Model):
+class NodeKey(CleanSave, Model):
     """Associate a Node with its OAuth (token) key.
 
     :ivar node: A Node.
@@ -168,7 +169,7 @@
         self.filter(node=node).delete()
 
 
-class NodeUserData(Model):
+class NodeUserData(CleanSave, Model):
     """User-data portion of a node's metadata.
 
     When cloud-init sets up a node, it retrieves specific data for that node
@@ -206,7 +207,7 @@
         return ncr.data
 
 
-class NodeCommissionResult(Model):
+class NodeCommissionResult(CleanSave, Model):
     """Storage for data returned from node commissioning.
 
     Commissioning a node results in various bits of data that need to be

=== modified file 'src/metadataserver/tests/test_models.py'
--- src/metadataserver/tests/test_models.py	2012-04-19 15:48:46 +0000
+++ src/metadataserver/tests/test_models.py	2012-05-08 03:06:19 +0000
@@ -12,7 +12,7 @@
 __metaclass__ = type
 __all__ = []
 
-from django.db import IntegrityError
+from django.core.exceptions import ValidationError
 from django.http import Http404
 from maasserver.testing.factory import factory
 from maastesting.djangotestcase import DjangoTestCase
@@ -155,7 +155,7 @@
         node = factory.make_node()
         factory.make_node_commission_result(node=node, name="foo")
         self.assertRaises(
-            IntegrityError,
+            ValidationError,
             factory.make_node_commission_result, node=node, name="foo")
 
     def test_different_nodes_can_have_same_data_name(self):