launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #07555
[Merge] lp:~jtv/maas/timestamped-model into lp:maas
Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/timestamped-model into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~jtv/maas/timestamped-model/+merge/104503
Remember CommonInfo? It's a base class used for some of our models. Its purpose is not very clear, which will probably lead to either complications or ossification.
And so in this branch I break it up into separate ways to address its two purposes:
1. Add timestamps to certain models. This becomes a new, single-purpose class.
2. Always check object validity before saving. This is now a method on the model itself.
That also means that we can decouple the two. I'm not sure integrating the validity check with save() is the best thing to do, but nor do I see any other clear “line of defense” where these things should be checked. Somebody with more Django experience (that means you Raphaël) will surely be able to help.
The custom skip_check parameter to save() is gone. This kind of parameter is usually a symptom that too much functionality is being heaped into a single function: if there's a part you don't always need, it's usually best off as a separate call. In this case, the parameter was really only there so that the factory can create nodes in any initial state, rather than just the ones that we allow in production. And so I had the factory temporarily sabotage that specific check at just that one critical point.
Jeroen
--
https://code.launchpad.net/~jtv/maas/timestamped-model/+merge/104503
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/timestamped-model into lp:maas.
=== modified file 'docs/models.rst'
--- docs/models.rst 2012-04-20 15:16:41 +0000
+++ docs/models.rst 2012-05-03 09:26:27 +0000
@@ -5,7 +5,7 @@
.. automodule:: maasserver.models
-.. autoclass:: CommonInfo
+.. autoclass:: TimestampedModel
:show-inheritance:
:members:
=== modified file 'src/maasserver/models/__init__.py'
--- src/maasserver/models/__init__.py 2012-05-01 17:35:22 +0000
+++ src/maasserver/models/__init__.py 2012-05-03 09:26:27 +0000
@@ -78,7 +78,7 @@
JSONObjectField,
MACAddressField,
)
-from maasserver.models.commoninfo import CommonInfo
+from maasserver.models.timestampedmodel import TimestampedModel
from metadataserver import nodeinituser
from piston.models import (
Consumer,
@@ -369,7 +369,7 @@
return None
-class Node(CommonInfo):
+class Node(TimestampedModel):
"""A `Node` represents a physical machine used by the MAAS Server.
:ivar system_id: The unique identifier for this `Node`.
@@ -431,6 +431,11 @@
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 +600,7 @@
mac_re = re.compile(r'^([0-9a-fA-F]{2}:){5}[0-9a-fA-F]{2}$')
-class MACAddress(CommonInfo):
+class MACAddress(TimestampedModel):
"""A `MACAddress` represents a `MAC address
<http://en.wikipedia.org/wiki/MAC_address>`_ attached to a :class:`Node`.
@@ -613,6 +618,11 @@
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."
@@ -839,7 +849,7 @@
MAX_KEY_DISPLAY = 50
-class SSHKey(CommonInfo):
+class SSHKey(TimestampedModel):
"""A `SSHKey` represents a user public SSH key.
Users will be able to access `Node`s using any of their registered keys.
@@ -862,6 +872,11 @@
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."
=== removed file 'src/maasserver/models/commoninfo.py'
--- src/maasserver/models/commoninfo.py 2012-04-30 04:00:31 +0000
+++ src/maasserver/models/commoninfo.py 1970-01-01 00:00:00 +0000
@@ -1,50 +0,0 @@
-# Copyright 2012 Canonical Ltd. This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-
-"""Utility base class with common model fields."""
-
-from __future__ import (
- absolute_import,
- print_function,
- unicode_literals,
- )
-
-__metaclass__ = type
-__all__ = [
- 'CommonInfo',
- ]
-
-
-from django.db import models
-from maasserver import DefaultMeta
-
-
-class CommonInfo(models.Model):
- """A base model which:
- - calls full_clean before saving the model (by default).
- - records the creation date and the last modification date.
-
- :ivar created: The creation date.
- :ivar updated: The last modification date.
-
- This class has no database table of its own. Its fields are incorporated
- in the tables of derived classes.
- """
-
- class Meta(DefaultMeta):
- abstract = True
-
- created = models.DateTimeField(editable=False)
- updated = models.DateTimeField(editable=False)
-
- def save(self, skip_check=False, *args, **kwargs):
- # Avoid circular imports.
- from maasserver.models import now
-
- date_now = now()
- if not self.id:
- self.created = date_now
- self.updated = date_now
- if not skip_check:
- self.full_clean()
- return super(CommonInfo, self).save(*args, **kwargs)
=== added file 'src/maasserver/models/timestampedmodel.py'
--- src/maasserver/models/timestampedmodel.py 1970-01-01 00:00:00 +0000
+++ src/maasserver/models/timestampedmodel.py 2012-05-03 09:26:27 +0000
@@ -0,0 +1,45 @@
+# Copyright 2012 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Model base class with creation/update timestamps."""
+
+from __future__ import (
+ absolute_import,
+ print_function,
+ unicode_literals,
+ )
+
+__metaclass__ = type
+__all__ = [
+ 'TimestampedModel',
+ ]
+
+
+from django.db.models import DateTimeField, Model
+from maasserver import DefaultMeta
+
+
+class TimestampedModel(Model):
+ """Abstract base model with creation/update timestamps.
+
+ Timestamps are taken from the database transaction clock.
+
+ :ivar created: Object's creation time.
+ :ivar updated: Time of object's latest update.
+ """
+
+ class Meta(DefaultMeta):
+ abstract = True
+
+ created = DateTimeField(editable=False)
+ updated = DateTimeField(editable=False)
+
+ def save(self, *args, **kwargs):
+ # Avoid circular imports.
+ from maasserver.models import now
+
+ current_time = now()
+ if not self.id:
+ self.created = current_time
+ self.updated = current_time
+ return super(TimestampedModel, self).save(*args, **kwargs)
=== modified file 'src/maasserver/testing/factory.py'
--- src/maasserver/testing/factory.py 2012-04-30 16:26:38 +0000
+++ src/maasserver/testing/factory.py 2012-05-03 09:26:27 +0000
@@ -27,6 +27,7 @@
FileStorage,
MACAddress,
Node,
+ NODE_TRANSITIONS,
SSHKey,
)
from maasserver.testing import (
@@ -42,6 +43,9 @@
MAX_PUBLIC_KEYS = 5
+ALL_NODE_STATES = map_enum(NODE_STATUS).values()
+
+
class Factory(maastesting.factory.Factory):
def getRandomEnum(self, enum):
@@ -69,6 +73,15 @@
return random.choice(
[choice for choice in choices if choice[0] not in but_not])[0]
+ def _save_node_unchecked(self, node):
+ """Save a :class:`Node`, but circumvent status transition checks."""
+ valid_initial_states = NODE_TRANSITIONS[None]
+ NODE_TRANSITIONS[None] = ALL_NODE_STATES
+ try:
+ node.save()
+ finally:
+ NODE_TRANSITIONS[None] = valid_initial_states
+
def make_node(self, hostname='', set_hostname=False, status=None,
architecture=ARCHITECTURE.i386, updated=None,
created=None, **kwargs):
@@ -80,7 +93,8 @@
node = Node(
hostname=hostname, status=status, architecture=architecture,
**kwargs)
- node.save(skip_check=True)
+ self._save_node_unchecked(node)
+
# Update the 'updated'/'created' fields with a call to 'update'
# preventing a call to save() from overriding the values.
if updated is not None:
=== modified file 'src/maasserver/tests/models.py'
--- src/maasserver/tests/models.py 2012-04-24 14:26:13 +0000
+++ src/maasserver/tests/models.py 2012-05-03 09:26:27 +0000
@@ -16,7 +16,7 @@
from django.db import models
from maasserver.fields import JSONObjectField
-from maasserver.models import CommonInfo
+from maasserver.models.timestampedmodel import TimestampedModel
class JSONFieldModel(models.Model):
@@ -28,7 +28,7 @@
name = models.CharField(max_length=255, unique=False)
-class CommonInfoTestModel(CommonInfo):
- # This model inherits from CommonInfo so it will have a 'created'
+class TimestampedModelTestModel(TimestampedModel):
+ # This model inherits from TimestampedModel so it will have a 'created'
# field and an 'updated' field.
pass
=== modified file 'src/maasserver/tests/test_models.py'
--- src/maasserver/tests/test_models.py 2012-05-02 10:01:04 +0000
+++ src/maasserver/tests/test_models.py 2012-05-03 09:26:27 +0000
@@ -69,7 +69,7 @@
TestCase,
TestModelTestCase,
)
-from maasserver.tests.models import CommonInfoTestModel
+from maasserver.tests.models import TimestampedModelTestModel
from maasserver.utils import map_enum
from maastesting.djangotestcase import (
TestModelTransactionalTestCase,
@@ -114,41 +114,41 @@
self.assertLessEqual(date_now, now())
-class CommonInfoTest(TestModelTestCase):
- """Testing for the class `CommonInfo`."""
+class TimestampedModelTest(TestModelTestCase):
+ """Testing for the class `TimestampedModel`."""
app = 'maasserver.tests'
def test_created_populated_when_object_saved(self):
- obj = CommonInfoTestModel()
+ obj = TimestampedModelTestModel()
obj.save()
self.assertIsNotNone(obj.created)
def test_updated_populated_when_object_saved(self):
- obj = CommonInfoTestModel()
+ obj = TimestampedModelTestModel()
obj.save()
self.assertIsNotNone(obj.updated)
def test_updated_and_created_are_the_same_after_first_save(self):
- obj = CommonInfoTestModel()
+ obj = TimestampedModelTestModel()
obj.save()
self.assertEqual(obj.created, obj.updated)
def test_created_not_modified_by_subsequent_calls_to_save(self):
- obj = CommonInfoTestModel()
+ obj = TimestampedModelTestModel()
obj.save()
old_created = obj.created
obj.save()
self.assertEqual(old_created, obj.created)
-class CommonInfoTransactionalTest(TestModelTransactionalTestCase):
+class TimestampedModelTransactionalTest(TestModelTransactionalTestCase):
app = 'maasserver.tests'
def test_created_bracketed_by_before_and_after_time(self):
before = now()
- obj = CommonInfoTestModel()
+ obj = TimestampedModelTestModel()
obj.save()
transaction.commit()
after = now()
@@ -156,7 +156,7 @@
self.assertGreaterEqual(after, obj.created)
def test_updated_is_updated_when_object_saved(self):
- obj = CommonInfoTestModel()
+ obj = TimestampedModelTestModel()
obj.save()
old_updated = obj.updated
transaction.commit()
@@ -435,15 +435,6 @@
"Invalid transition: Retired -> Allocated.",
node.save)
- def test_save_does_not_check_status_transition_if_skip_check(self):
- # RETIRED -> ALLOCATED is an invalid transition.
- node = factory.make_node(
- status=NODE_STATUS.RETIRED, owner=factory.make_user())
- node.status = NODE_STATUS.ALLOCATED
- node.save(skip_check=True)
- # The test is that this does not raise an error.
- pass
-
class NodeTransitionsTests(TestCase):
"""Test the structure of NODE_TRANSITIONS."""
@@ -1093,13 +1084,22 @@
ValidationError, key2.full_clean)
def test_sshkey_user_and_key_unique_together_db_level(self):
+ # Even if we hack our way around model-level checks, uniqueness
+ # of the user/key combination is enforced at the database level.
key_string = get_data('data/test_rsa0.pub')
user = factory.make_user()
- key = SSHKey(key=key_string, user=user)
- key.save()
- key2 = SSHKey(key=key_string, user=user)
+ existing_key = SSHKey(key=key_string, user=user)
+ existing_key.save()
+ # The trick to hack around the model-level checks: create a
+ # duplicate key for another user, then attach it to the same
+ # user as the existing key by updating it directly in the
+ # database.
+ redundant_key = SSHKey(key=key_string, user=factory.make_user())
+ redundant_key.save()
self.assertRaises(
- IntegrityError, key2.save, skip_check=True)
+ IntegrityError,
+ SSHKey.objects.filter(id=redundant_key.id).update,
+ user=user)
def test_sshkey_same_key_can_be_used_by_different_users(self):
key_string = get_data('data/test_rsa0.pub')
=== modified file 'src/maasserver/tests/test_provisioning.py'
--- src/maasserver/tests/test_provisioning.py 2012-04-30 16:26:38 +0000
+++ src/maasserver/tests/test_provisioning.py 2012-05-03 09:26:27 +0000
@@ -340,7 +340,7 @@
self.patch(self.papi.proxy, 'add_node', raise_missing_profile)
with ExpectedException(ExternalComponentException):
- node = factory.make_node(architecture='amd32k')
+ node = factory.make_node()
provisioning.provision_post_save_Node(
sender=Node, instance=node, created=True)
@@ -351,7 +351,7 @@
self.patch(self.papi.proxy, 'add_node', raise_fault)
with ExpectedException(ExternalComponentException):
- node = factory.make_node(architecture='amd32k')
+ node = factory.make_node()
provisioning.provision_post_save_Node(
sender=Node, instance=node, created=True)
Follow ups
