Is it important to use full_clean?

[llamafilm]

I'm learning how to write Netbox custom scripts, trying to do things "the right way".
Suppose I want to create a bunch of IP addresses by iterating through a list. At first I didn't know about full_clean so I wrote error handling like this:

from django.core.exceptions import ValidationError
try:
    ip = IPAddress(address='???')
    ip.save()
except ValidationError as e:
    self.log_failure(e)
    continue

This works well, and if the IP address is invalid I get a nice error message like: ['Invalid IP address format: ???]

But then I found some posts suggesting to use full_clean before every save. So if I rewrite the code like this, then it throws a generic AttributeError that does not explain the root cause:

try:
    ip = IPAddress(address='???')
    ip.full_clean()
    ip.save()
except AttributeError as e:
    self.log_failure(e)
    continue

Exception: AttributeError: 'str' object has no attribute 'prefixlen'

I prefer my first approach, and the documentation isn't super clear about this. Is there any important reason to do the full_clean before save?

[kkthxbye-code]

The reason the first way of doing it is raising a ValidationError is an edgecase as the custom field type netbox uses for that field does validation when converting values:

netbox/netbox/ipam/fields.py

Line 35 in c78a792

raise ValidationError("Invalid IP address format: {}".format(value))

In general, when not running full_clean, none of the following methods are run:

• Model.clean_fields()
• Model.clean()
• Model.validate_unique() (if validate_unique is True)
• Model.validate_constraints(

https://docs.djangoproject.com/en/5.0/ref/models/instances/#django.db.models.Model.full_clean

You have to always call full_clean, as this is how it works both in the API and the UI. Not doing so will allow you to corrupt various data.

The reason your second example fails is because the clean method itself expects a proper ip address object with the prefixlen attribute and as such throws an exeception when validating the data:

netbox/netbox/ipam/models/ip.py

Line 842 in c78a792

if self.address.prefixlen == 0:

This is technically a bug, but I wouldn't expect it being fixed.

Is there any important reason to do the full_clean before save?

Short answer, yes to avoid corrupt data.

[llamafilm]

Thank you! This is very helpful.

[llamafilm]

It seems like it would be very easy to add an additional validation check there to ensure it’s a valid IP. Why do you think this bug won’t be fixed?

[candlerb]

The requirement to call full_clean() before save() is an issue with Django, not Netbox. Reference:

Note that full_clean() will not be called automatically when you call your model’s save() method. You’ll need to call it manually when you want to run one-step model validation for your own manually created models.

If you want an explanation of this, then Google for it: e.g.

• https://stackoverflow.com/questions/4441539/why-doesnt-djangos-model-save-call-full-clean
• https://jamescooke.info/djangos-model-save-vs-full_clean.html
• https://groups.google.com/g/pyweb-il/c/En2AxEd4VKM

The explanations don't seem very convincing to me - mainly to do with backwards compatibility to ancient versions of Django it seems. Very nasty bugs occur if you forget it.

[candlerb]

I forgot to mention: a specific requirement in Netbox is that you also need to call snapshot() before modifying an existing object. So the full sequence becomes:

o = Foo.objects.get(...)
o.snapshot()   # required by Netbox
o.foo = bar
o.full_clean() # required by Django
o.save()

(IMO it would be more robust if this could be implemented using Postgres triggers - or alternatively if the application moved to a full event sourcing architecture, i.e. the current state tables are built entirely from the event log)

[DanSheps]

I believe snapshot() is only required if you care about your change log (you probably should)

[llamafilm]

I'm trying to use snapshot but it doesn't seem to have any effect. For example I ran this code in nbshell, to change some fields that I never use:

for dev in Device.objects.all():
    dev.snapshot()
    dev.airflow = 'front-to-rear'
    dev.serial = '123'
    dev.full_clean()
    dev.save()

I expected a new record in the changelog for each device but that did not happen. I do see that the serial and airflow have been updated in the GUI. Oddly, the output of this command in the shell is this for each one. I'm not sure what this has to do with cable termination:

Updating denormalized values for <class 'dcim.models.cables.CableTermination'>._device
Updated 0 rows

[candlerb]

I don't know for sure why you're not getting a Change Log entry, but I suspect it's because what you're doing in nbshell is not in the context of a HTTP Request (which would know things like the user who made the change).

As for the message you're getting, as it suggests, the CableTermination model has a reference to _device:

class CableTermination(ChangeLoggedModel):
...
    # Cached associations to enable efficient filtering
    _device = models.ForeignKey(
        to='dcim.Device',
        on_delete=models.CASCADE,
        blank=True,
        null=True
    )
    _rack = models.ForeignKey(
        to='dcim.Rack',
        on_delete=models.CASCADE,
        blank=True,
        null=True
    )
    _location = models.ForeignKey(
        to='dcim.Location',
        on_delete=models.CASCADE,
        blank=True,
        null=True
    )
    _site = models.ForeignKey(
        to='dcim.Site',
        on_delete=models.CASCADE,
        blank=True,
        null=True
    )

        # Register denormalized fields
        denormalized.register(CableTermination, '_device', {
            '_rack': 'rack',
            '_location': 'location',
            '_site': 'site',
        })

AFAICT, whenever you save a Device there is a chance that its Rack, Location or Site might have changed, so it must trigger a potential update to the related fields on any CableTermination objects that are on this Device.