starred/netbox
1
0
Fork 0
mirror of https://github.com/netbox-community/netbox.git synced 2026-01-11 21:10:29 +01:00
Code Issues 273 Packages Projects Releases 10 Wiki Activity

Migrate all model validation logic to model clean methods #3685

New Issue
Closed
opened 2025-12-29 18:30:36 +01:00 by adam · 5 comments
adam commented 2025-12-29 18:30:36 +01:00
Owner
Copy Link

Originally created by @lampwins on GitHub (May 13, 2020).

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.

Originally created by @lampwins on GitHub (May 13, 2020). <!-- NOTE: This template is for use by maintainers only. Please do not submit an issue using this template unless you have been specifically asked to do so. --> ### 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. <!-- Provide justification for the proposed change(s). --> ### 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.
adam added the type: housekeeping label 2025-12-29 18:30:36 +01:00
adam closed this issue 2025-12-29 18:30:36 +01:00
adam commented 2025-12-29 18:30:37 +01:00
Author
Owner
Copy Link

@jeremystretch commented on GitHub (May 13, 2020):

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.

@jeremystretch commented on GitHub (May 13, 2020): 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.
adam commented 2025-12-29 18:30:37 +01:00
Author
Owner
Copy Link

@lampwins commented on GitHub (Nov 1, 2020):

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
@lampwins commented on GitHub (Nov 1, 2020): 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 |
adam commented 2025-12-29 18:30:37 +01:00
Author
Owner
Copy Link

@itdependsnetworks commented on GitHub (Nov 7, 2020):

Is the project accepting PR's to address these?

@itdependsnetworks commented on GitHub (Nov 7, 2020): Is the project accepting PR's to address these?
adam commented 2025-12-29 18:30:37 +01:00
Author
Owner
Copy Link

@jeremystretch commented on GitHub (Nov 9, 2020):

@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.
@jeremystretch commented on GitHub (Nov 9, 2020): @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.
adam commented 2025-12-29 18:30:37 +01:00
Author
Owner
Copy Link

@itdependsnetworks commented on GitHub (Nov 9, 2020):

  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
@itdependsnetworks commented on GitHub (Nov 9, 2020): 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
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
beta
breaking change
complexity: high
complexity: low
complexity: medium
needs milestone
netbox
pending closure
plugin candidate
pull-request
Mirrored from GitHub Pull Request
 severity: high
severity: low
severity: medium
status: accepted
status: backlog
status: blocked
status: duplicate
status: needs owner
status: needs triage
status: revisions needed
status: under review
topic: GraphQL
topic: Internationalization
topic: OpenAPI
topic: UI/UX
topic: cabling
topic: event rules
topic: htmx navigation
topic: industrialization
topic: migrations
topic: plugins
topic: scripts
topic: templating
topic: testing
type: bug
type: deprecation
type: documentation
type: feature
type: housekeeping
type: translation
No Label type: housekeeping
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
adam
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/netbox#3685
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?