Code review comment for lp:~blake-rouse/maas/add-osystem-to-node-form-api

Revision history for this message
Raphaƫl Badin (rvb) wrote :

My general take on how validation should be performed is that we should use model-level validation only to avoid inconsistencies in the DB. The rest of the validation belongs to forms. They give us a much more flexible way to validate the input from the UI and the API. More flexible because we can still manipulate objects directly in tests and migrations. This is just a matter of properly separating the M and the C (in MVC) because they are fundamentally separate steps; mingling them together reduces the flexibility of the whole model.

> osystem has validation in the model, as this field can also be set with the start API call.
> distro_series always had validation using the choices=, but now its just using a function to
> perform the validation. It gives the same result.

The result is not quite the same, no. Doing this kind of application-level integrity enforcement at the model level reduces our ability to deal gracefully with inconsistencies. Consider for instance the consequence of having an OS removed: one would be incapable of modifying the nodes referencing this OS unless he also changes the OS (because the validation would fail at the model level); if the validation happens at the form level, at least we could write a migration to deal with these nodes.

> factory.make_node does not break tests, as the osystem field just like the distro_series
> field was can be blank. If the field is blank then the default is used, so no tests break.

But then all your tests operate with a very special case (osystem=blank). It's much better to have the tests operate on data that is as similar to production data as possible. This is also why we chose to use randomness when creating test objects.

« Back to merge proposal