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()
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
