launchpad-reviewers team mailing list archive

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

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/bug-1034523 into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1034523 in MAAS: "MAAS fails to upgrade"
  https://bugs.launchpad.net/maas/+bug/1034523

For more details, see:
https://code.launchpad.net/~jtv/maas/bug-1034523/+merge/119091

This is a sad branch.  A retreat after a long battle over a small piece of ground.  There just does not seem to be any way to get a NOT NULL constraint onto Node.nodegroup.  We can't reasonably create the master nodegroup from a database migration, which we need in order to update pre-existing null fields, so we have to do both after.  And therefore we have to allow null values during the schema-migration phase.  Forever.

In this branch I remove the schema migration that added the NOT NULL constraint, because it was breaking upgrades.  This affects the subsequent migration as well, because it also has to specify its notion of what the schema looks like.  The field is still non-blankable, which in theory should mean that django will reject null values.  It doesn't seem to do much in practice though.

Debugging showed that, because Node.nodegroup is non-editable, Django's field validation will quietly skip it.  I also tried custom model validation at the model level (rather than validating at the field level), but that failed because the form code needs to set a default nodegroup *after* saving the form — which unfortunately entails model validation, and so breaks.

This also highlights an inconsistency in Django's design, at least in the version we use.  On the one hand, the developers have made it clear that a model absolutely should go through validation during save(), and the only reason why it doesn't do so by default is backwards compatibility.  That's why we have the CleanSave mix-in class, and as far as I can see, that mix-in is the only reason why “Django will prevent any attempt to save an incomplete model,” as the documentation misleadingly puts it.

On the other hand, Django lacks proper separations between storage, models, and forms.  The documentation I just quoted was talking about model forms, but called them models.  Model specification and validation are designed to support validation of model forms, but not of models as such (or of forms as such, I guess).  The “editable” option on a model field only affects forms, but validation seems to assume that a non-editable field never needs to be validated again once an object has been created.  Django's recommended practice creates a paradox: setting a field to None will be disallowed, as you would expect, if the field is non-blankable but editable; but is accepted if you try to disallow both None values _and_ changing the field in the first place.

I tried writing tests for the blank=False validation, but couldn't get it to raise an error even with Node(nodegroup=None).save().  We'll just have to leave this column poorly protected against missing values.  ☹


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

=== modified file 'src/maasserver/migrations/0016_node_nodegroup_not_null.py'
--- src/maasserver/migrations/0016_node_nodegroup_not_null.py	2012-07-31 07:04:29 +0000
+++ src/maasserver/migrations/0016_node_nodegroup_not_null.py	2012-08-10 08:20:29 +0000
@@ -21,10 +21,10 @@
 
     def forwards(self, orm):
 
-        # Adding NOT NULL constraint on Node.nodegroup.
+        # Disallowing nulls in new Node.nodegroup values.
         db.alter_column(
             'maasserver_node', 'nodegroup_id',
-            models.ForeignKey(NodeGroup, editable=False))
+            models.ForeignKey(NodeGroup, editable=False, null=True, blank=False))
 
     def backwards(self, orm):
 
@@ -108,7 +108,7 @@
             'hostname': ('django.db.models.fields.CharField', [], {'default': "u''", 'max_length': '255', 'blank': 'True'}),
             'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
             'netboot': ('django.db.models.fields.BooleanField', [], {'default': 'True'}),
-            'nodegroup': ('django.db.models.fields.related.ForeignKey', [], {'to': u"orm['maasserver.NodeGroup']"}),
+            'nodegroup': ('django.db.models.fields.related.ForeignKey', [], {'to': u"orm['maasserver.NodeGroup']", 'null': 'True'}),
             'owner': ('django.db.models.fields.related.ForeignKey', [], {'default': 'None', 'to': "orm['auth.User']", 'null': 'True', 'blank': 'True'}),
             'power_parameters': ('maasserver.fields.JSONObjectField', [], {'default': "u''", 'blank': 'True'}),
             'power_type': ('django.db.models.fields.CharField', [], {'default': "u''", 'max_length': '10', 'blank': 'True'}),

=== modified file 'src/maasserver/migrations/0017_add_dhcp_key_to_nodegroup.py'
--- src/maasserver/migrations/0017_add_dhcp_key_to_nodegroup.py	2012-08-01 10:26:11 +0000
+++ src/maasserver/migrations/0017_add_dhcp_key_to_nodegroup.py	2012-08-10 08:20:29 +0000
@@ -91,7 +91,7 @@
             'hostname': ('django.db.models.fields.CharField', [], {'default': "u''", 'max_length': '255', 'blank': 'True'}),
             'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
             'netboot': ('django.db.models.fields.BooleanField', [], {'default': 'True'}),
-            'nodegroup': ('django.db.models.fields.related.ForeignKey', [], {'to': u"orm['maasserver.NodeGroup']"}),
+            'nodegroup': ('django.db.models.fields.related.ForeignKey', [], {'to': u"orm['maasserver.NodeGroup']", 'null': 'True'}),
             'owner': ('django.db.models.fields.related.ForeignKey', [], {'default': 'None', 'to': "orm['auth.User']", 'null': 'True', 'blank': 'True'}),
             'power_parameters': ('maasserver.fields.JSONObjectField', [], {'default': "u''", 'blank': 'True'}),
             'power_type': ('django.db.models.fields.CharField', [], {'default': "u''", 'max_length': '10', 'blank': 'True'}),

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2012-08-09 05:39:28 +0000
+++ src/maasserver/models/node.py	2012-08-10 08:20:29 +0000
@@ -383,7 +383,14 @@
 
     netboot = BooleanField(default=True)
 
-    nodegroup = ForeignKey('maasserver.NodeGroup', editable=False)
+    # This field can't be null, but we can't enforce that in the
+    # database schema: we make sure that no existing nodes have a null
+    # here, but a NOT NULL constraint would have to be applied at an
+    # earlier stage.
+    # To get around this, we allow nulls in the database ("null=True")
+    # but reject them in validation ("blank=False").
+    nodegroup = ForeignKey(
+        'maasserver.NodeGroup', editable=False, null=True, blank=False)
 
     objects = NodeManager()