Netbox: Migrate all model validation logic to model clean methods

[lampwins]

Proposed Changes

4393 highlights that we have several instances of model validation business logic implemented in forms or API serializers rather than in the model's clean method. We should standardize on using the model's clean method and migrate form clean logic here.

Tests also need to be implemented along the way, where applicable, to ensure we are not altering functionality.

Justification

Django offers several places to implement validation logic and this has created cases of inconsistent use of the form clean vs model clean methods. This is confusing for ongoing maintenance and in most cases, there is no need to use anything other than the model clean method for the vast majority of business logic we implement. To be clear, this proposal is scoped only to that logic which directly affects the model itself, not any ancillary form control logic which may exist. Centralizing logic in the model clean method will also ensure validation feature parity between the UI and API.

[jeremystretch]

Also: because calling save() directly on a model does not call clean(), we need to be especially vigilant to identify any occurrences where save() is called outside of a form or serializer.

[lampwins]

I went through and identified all instances where we need to move validation logic. There is a lot less than I had anticipated. Although we do use the form clean method a lot, most occurrences do not actually have to do with model validation logic.

Here is what I found:

| App | Type | Name | Comments | Action |
|--------|-------------|---------------------------|--------------------------------------------------------------------------------------|---------------------------------------------------------------------|
| dcim | forms | ChildDeviceCSVForm | Setting object parent relationships | Move this logic to model save method |
| dcim | forms | InterfaceBulkEditForm | 1) Validation for tagged vlans & model. 2) Removing tagged vlans in tagged all model | 1) Move to model clean method 2) Move to model save method |
| dcim | forms | InterfaceBulkEditForm | Validating length and length_unit | Move this logic to model clean method |
| dcim | serializers | RackSerializer | Custom unique together validator for the group and facility fields | Investigate if this can actually be moved to the model clean method |
| dcim | serializers | DeviceSerializer | Custom unique together validator for the rack, position, and face fields | Investigate if this can actually be moved to the model clean method |
| dcim | serializers | InterfaceSerializer | Vlan tagging validation | Move this logic to model clean method |
| extras | serializers | ImageAttachmentSerializer | Validating that the parent object exists | Investigate if this can actually be moved to the model clean method |
| ipam | forms | IPAddressForm | Validating IP Address model constraints | Move this logic to model clean method |
| ipam | serializers | VLANGroupSerializer | Custom unique together validator for the name and slug fields | Investigate if this can actually be moved to the model clean method |
| ipam | serializers | VLANSerializer | Custom unique together validator for the name and vid fields | Investigate if this can actually be moved to the model clean method |

[itdependsnetworks]

Is the project accepting PR's to address these?

[jeremystretch]

@itdependsnetworks In theory, yes, but there are several requirements to consider:

1. This work needs to be done using the v2.10 development branch as a base. (However, it's too much to fit into the upcoming beta release. It will probably be undertaken under one or more v2.10.x releases.)
2. Due to the breadth of the changes that need to be made, it likely makes sense to split these out into smaller, more manageable tasks. At a minimum, we should separate out form and serializer changes IMO.
3. All of the changes must include relevant tests to ensure that validation works as expected.
4. Whoever takes on the work should be comfortable owning it, at least until v2.11. Too often we get PRs from the community that require follow-up work, and the burden falls to the maintainers to prioritize fixes for code which was itself not a priority.

[itdependsnetworks]

1. makes sense
2. Agreed, thinking 1 change per model, with analysis as it's own issue
3. Agreed
4. I will take ownership through 2.11