15671 save module before sync_classes

[arthanson]

Fixes: #15671

save Module before doing sync_classes as it can cause M2M error: ValueError: save() prohibited to prevent data loss due to unsaved related object 'module'.

[peteeckel]

Hi @arthanson, pardon me if this is a stupid question, but does save() really return a value? The Django docs say no, but is there any place in NetBox where this is overridden and the returned value is actually used?

    def save(self, *args, **kwargs):
        self.file_root = ManagedFileRootPathChoices.SCRIPTS
        obj = super().save(*args, **kwargs)
        self.sync_classes()
        return obj

[arthanson]

Hi @arthanson, pardon me if this is a stupid question, but does save() really return a value? The Django docs say no, but is there any place in NetBox where this is overridden and the returned value is actually used?

    def save(self, *args, **kwargs):
        self.file_root = ManagedFileRootPathChoices.SCRIPTS
        obj = super().save(*args, **kwargs)
        self.sync_classes()
        return obj

Good catch, fixed.

[peteeckel]

There are some other places in the NetBox code where save() also returns a value, which made me think that this could be intentional after all.

# git grep -n 'return super().save' 
netbox/core/forms/model_forms.py:86:        return super().save(*args, **kwargs)
netbox/core/forms/model_forms.py:120:        return super().save(*args, **kwargs)
netbox/dcim/models/device_components.py:567:        return super().save(*args, **kwargs)
netbox/extras/models/change_logging.py:131:        return super().save(*args, **kwargs)
netbox/extras/models/customfields.py:784:        return super().save(*args, **kwargs)
netbox/extras/models/scripts.py:169:        return super().save(*args, **kwargs)
netbox/users/forms/bulk_import.py:34:        return super().save(*args, **kwargs)
netbox/users/forms/model_forms.py:98:        return super().save(*args, **kwargs)
netbox/users/models/tokens.py:92:        return super().save(*args, **kwargs)
netbox/vpn/forms/bulk_import.py:140:        return super().save(*args, **kwargs)

git grep -n '= super().save'
netbox/core/forms/model_forms.py:218:        instance = super().save(commit=False)
netbox/dcim/forms/object_create.py:439:        instance = super().save(*args, **kwargs)
netbox/dcim/models/devices.py:341:        ret = super().save(*args, **kwargs)
netbox/ipam/forms/bulk_import.py:377:        ipaddress = super().save(*args, **kwargs)
netbox/ipam/forms/model_forms.py:177:        instance = super().save(*args, **kwargs)
netbox/ipam/forms/model_forms.py:393:        ipaddress = super().save(*args, **kwargs)
netbox/ipam/forms/model_forms.py:475:        instance = super().save(*args, **kwargs)
netbox/netbox/models/features.py:494:        ret = super().save(*args, **kwargs)
netbox/users/forms/model_forms.py:211:        instance = super().save(*args, **kwargs)
netbox/users/forms/model_forms.py:259:        instance = super().save(*args, **kwargs)
netbox/users/forms/model_forms.py:379:        instance = super().save(*args, **kwargs)
netbox/vpn/forms/model_forms.py:201:        instance = super().save(*args, **kwargs)

(in all likelihood not a complete list)

[jeremystretch]

Django's Model.save() always returns None, so technically nothing needs to be returned. However, when invoking the method of a parent class using super(), it's common to always return its original output (if any) just out of convention. (Also, many of the instances above are actually forms, not models, and ModelForm.save() does return an instance.)

[peteeckel]

Django's Model.save() always returns None, so technically nothing needs to be returned. However, when invoking the method of a parent class using super(), it's common to always return its original output (if any) just out of convention.

I would totally agree with you (I love consistency) if the convention in NetBox wouldn't seem to be to not return the value of super().save():

netbox/dcim/models/cables.py:        super().save(*args, **kwargs)
netbox/dcim/models/cables.py:        super().save(*args, **kwargs)
netbox/dcim/models/cables.py:        super().save(*args, **kwargs)
netbox/dcim/models/device_components.py:        return super().save(*args, **kwargs)
netbox/dcim/models/device_components.py:        super().save(*args, **kwargs)
netbox/dcim/models/devices.py:        ret = super().save(*args, **kwargs)
netbox/dcim/models/devices.py:        super().save(*args, **kwargs)
netbox/dcim/models/devices.py:        super().save(*args, **kwargs)
netbox/dcim/models/mixins.py:        super().save(*args, **kwargs)
netbox/dcim/models/power.py:        super().save(*args, **kwargs)
netbox/dcim/models/racks.py:        super().save(*args, **kwargs)
netbox/extras/models/change_logging.py:        return super().save(*args, **kwargs)
netbox/extras/models/customfields.py:        return super().save(*args, **kwargs)
netbox/extras/models/scripts.py:        super().save(*args, **kwargs)
netbox/ipam/models/ip.py:        super().save(*args, **kwargs)
netbox/ipam/models/ip.py:        super().save(*args, **kwargs)
netbox/ipam/models/ip.py:        super().save(*args, **kwargs)
netbox/netbox/models/features.py:        ret = super().save(*args, **kwargs)
netbox/users/models/tokens.py:        return super().save(*args, **kwargs)
netbox/virtualization/models/virtualmachines.py:        super().save(*args, **kwargs)
netbox/wireless/models.py:        super().save(*args, **kwargs)

That's 6 "return" vs. 15 "don't return" - for models, I was mistaken about the forms, sorry.