Repository: netbox-community/netbox
Pull request: Fixes #14482 - Fix validation error when primary IP is moved
### Fixes: #14482

When an Primary IP address is moved, it should result in a validation error.  However, it does not normally due to the fact that the `clean()` method on the model returns with the field `assigned_object` instead of `interface`, `vminterface`, or `fhrpgroup`.  When the `_post_clean()` method tries to resolve the fields, it will fail because the form does not contain `assigned_object` as a field and returns a ValueError.

This will correctly map the `assigned_object` field to the correct field, however we should maybe look at instead providing a method to have `assigned_object` (or similar GFK fields) as proper form fields in the future, perhaps using a custom form field.

Problem statement:
Cannot move primary IP from device to VM or vice versa
### Deployment Type

Self-hosted

### NetBox Version

v3.6.6

### Python Version

3.8

### Steps to Reproduce

1. Add an IP to a device and set it as the device's primary IP address
2. On a VM interface, assign the IP from step 1 and check "Make this the primary IP for the device/VM"
3. On save, a `ValueError` is raised

This bug also works the other way around (VM to Device).

### Expected Behavior

The IP address should be assigned to the VM interface.

### Observed Behavior

```
<class 'ValueError'>

'IPAddressForm' has no field named 'assigned_object'.

Python version: 3.8.10
NetBox version: 3.6.6
Plugins: 
  netbox_demo: 0.3.1
```

Hints from issue discussion:
To be clear, moving the primary IP for a device/VM is *not* supported. However, attempting to do so should return a validation error rather than raise an exception.

Commit message: Fix views test

Patch to review:
diff --git a/netbox/ipam/forms/model_forms.py b/netbox/ipam/forms/model_forms.py
index 41b31dc761c..931dd471c43 100644
--- a/netbox/ipam/forms/model_forms.py
+++ b/netbox/ipam/forms/model_forms.py
@@ -341,6 +341,19 @@ def __init__(self, *args, **kwargs):
             self.fields['vminterface'].disabled = True
             self.fields['fhrpgroup'].disabled = True
 
+    # Correctly assigned assigned_object if the error exists when validating this form.
+    def add_error(self, field, errors):
+        if isinstance(errors, ValidationError) and hasattr(errors, 'error_dict') and \
+                errors.error_dict.get('assigned_object', None):
+            error = errors.error_dict.pop('assigned_object')
+            if isinstance(self.instance.assigned_object, Interface):
+                errors.error_dict.update({'interface': error})
+            elif isinstance(self.instance.assigned_object, VMInterface):
+                errors.error_dict.update({'vminterface': error})
+            elif isinstance(self.instance.assigned_object, FHRPGroup):
+                errors.error_dict.update({'fhrpgroup': error})
+        super().add_error(field, errors)
+
     def clean(self):
         super().clean()
 


Code review comments:
diff_hunk: @@ -341,6 +341,19 @@ def __init__(self, *args, **kwargs):
             self.fields['vminterface'].disabled = True
             self.fields['fhrpgroup'].disabled = True
 
+    # Correctly assigned assigned_object if the error exists when validating this form.
+    def add_error(self, field, errors):
path: netbox/ipam/forms/model_forms.py
text: @user1:
This isn't a tenable approach to fixing the validation. We need to adjust the logic under the model form's `clean()` method to ensure it's catching the error and raising a ValidationError against the appropriate field before the model's `full_clean()` method is called.

@author:
> We need to adjust the logic under the model form's `clean()` method to ensure it's catching the error and raising a ValidationError against the appropriate field before the model's `full_clean()` method is called.

This isn't an option.

The "clean" flow is:

```
form.save() -> form.full_clean() -> form._clean_fields()
                                                       form._clean_form() -> form.clean()
                                                       form._post_clean() -> model.full_clean()
```

the form.clean_form() catches the validation error and adds it to the stack, however the post_clean calls the model clean regardless of whether there is already an error on the stack.

The error stack looks like this:

```
{
    "assigned_object": "Cannot reassign IP address while it is designated as the primary IP for the parent object",
    "vminterface": "Cannot reassign IP address while it is designated as the primary IP for the parent object"
}
```

The only way to remove this problem seems to be to also remove the ErrorDict from the model clean() and just throw a straight string validation error (pushing this change now).

I think realistically we need to re-investigate GFK's and see if there isn't a way to:

* Create a fieldgroup called "assigned_object" (or whatever the GFK name is)
* Create a select field called "assigned_object_type" (or whatever the GFK type field name is) that is populated with the types available for selection
* Create a select field called "assigned_object_id" (or whatever the GFK id field name is) that is populated with the objects available for selection based on the type selected
* Modify add_error to also match on the assigned_object fieldgroup

This obviously wouldn't be do-able in this FR but something to think about.