From 879166d939314bdc8745dbede3f8af67c61dd1dd Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Fri, 21 Aug 2020 15:16:33 -0400 Subject: [PATCH 01/19] Initial work on reimplementing custom fields --- .../migrations/0020_custom_field_data.py | 23 +++++++++ .../dcim/migrations/0115_custom_field_data.py | 38 ++++++++++++++ .../0050_migrate_customfieldvalues.py | 50 +++++++++++++++++++ netbox/extras/models/customfields.py | 4 ++ .../ipam/migrations/0038_custom_field_data.py | 43 ++++++++++++++++ .../migrations/0010_custom_field_data.py | 18 +++++++ .../migrations/0010_custom_field_data.py | 18 +++++++ .../migrations/0018_custom_field_data.py | 23 +++++++++ 8 files changed, 217 insertions(+) create mode 100644 netbox/circuits/migrations/0020_custom_field_data.py create mode 100644 netbox/dcim/migrations/0115_custom_field_data.py create mode 100644 netbox/extras/migrations/0050_migrate_customfieldvalues.py create mode 100644 netbox/ipam/migrations/0038_custom_field_data.py create mode 100644 netbox/secrets/migrations/0010_custom_field_data.py create mode 100644 netbox/tenancy/migrations/0010_custom_field_data.py create mode 100644 netbox/virtualization/migrations/0018_custom_field_data.py diff --git a/netbox/circuits/migrations/0020_custom_field_data.py b/netbox/circuits/migrations/0020_custom_field_data.py new file mode 100644 index 000000000..c72b937db --- /dev/null +++ b/netbox/circuits/migrations/0020_custom_field_data.py @@ -0,0 +1,23 @@ +# Generated by Django 3.1 on 2020-08-21 18:34 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('circuits', '0019_nullbooleanfield_to_booleanfield'), + ] + + operations = [ + migrations.AddField( + model_name='circuit', + name='custom_field_data', + field=models.JSONField(blank=True, default=dict), + ), + migrations.AddField( + model_name='provider', + name='custom_field_data', + field=models.JSONField(blank=True, default=dict), + ), + ] diff --git a/netbox/dcim/migrations/0115_custom_field_data.py b/netbox/dcim/migrations/0115_custom_field_data.py new file mode 100644 index 000000000..58c320902 --- /dev/null +++ b/netbox/dcim/migrations/0115_custom_field_data.py @@ -0,0 +1,38 @@ +# Generated by Django 3.1 on 2020-08-21 18:34 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('dcim', '0114_update_jsonfield'), + ] + + operations = [ + migrations.AddField( + model_name='device', + name='custom_field_data', + field=models.JSONField(blank=True, default=dict), + ), + migrations.AddField( + model_name='devicetype', + name='custom_field_data', + field=models.JSONField(blank=True, default=dict), + ), + migrations.AddField( + model_name='powerfeed', + name='custom_field_data', + field=models.JSONField(blank=True, default=dict), + ), + migrations.AddField( + model_name='rack', + name='custom_field_data', + field=models.JSONField(blank=True, default=dict), + ), + migrations.AddField( + model_name='site', + name='custom_field_data', + field=models.JSONField(blank=True, default=dict), + ), + ] diff --git a/netbox/extras/migrations/0050_migrate_customfieldvalues.py b/netbox/extras/migrations/0050_migrate_customfieldvalues.py new file mode 100644 index 000000000..9b2402267 --- /dev/null +++ b/netbox/extras/migrations/0050_migrate_customfieldvalues.py @@ -0,0 +1,50 @@ +from django.db import migrations + +from extras.choices import CustomFieldTypeChoices + + +def deserialize_value(field_type, value): + """ + Convert serialized values to JSON equivalents. + """ + if field_type in (CustomFieldTypeChoices.TYPE_INTEGER, CustomFieldTypeChoices.TYPE_SELECT): + return int(value) + if field_type == CustomFieldTypeChoices.TYPE_BOOLEAN: + return bool(int(value)) + return value + + +def migrate_customfieldvalues(apps, schema_editor): + CustomFieldValue = apps.get_model('extras', 'CustomFieldValue') + + for cfv in CustomFieldValue.objects.prefetch_related('field').exclude(serialized_value=''): + model = apps.get_model(cfv.obj_type.app_label, cfv.obj_type.model) + + # Read and update custom field value for each instance + # TODO: This can be done more efficiently once .update() is supported for JSON fields + cf_data = model.objects.filter(pk=cfv.obj_id).values('custom_field_data').first() + try: + cf_data['custom_field_data'][cfv.field.name] = deserialize_value(cfv.field.type, cfv.serialized_value) + except ValueError as e: + print(f'{cfv.field.name} ({cfv.field.type}): {cfv.serialized_value} ({cfv.pk})') + raise e + model.objects.filter(pk=cfv.obj_id).update(**cf_data) + + +class Migration(migrations.Migration): + + dependencies = [ + ('circuits', '0020_custom_field_data'), + ('dcim', '0115_custom_field_data'), + ('extras', '0049_remove_graph'), + ('ipam', '0038_custom_field_data'), + ('secrets', '0010_custom_field_data'), + ('tenancy', '0010_custom_field_data'), + ('virtualization', '0018_custom_field_data'), + ] + + operations = [ + migrations.RunPython( + code=migrate_customfieldvalues + ), + ] diff --git a/netbox/extras/models/customfields.py b/netbox/extras/models/customfields.py index fd09971b6..1c86812e0 100644 --- a/netbox/extras/models/customfields.py +++ b/netbox/extras/models/customfields.py @@ -17,6 +17,10 @@ from extras.utils import FeatureQuery # class CustomFieldModel(models.Model): + custom_field_data = models.JSONField( + blank=True, + default=dict + ) class Meta: abstract = True diff --git a/netbox/ipam/migrations/0038_custom_field_data.py b/netbox/ipam/migrations/0038_custom_field_data.py new file mode 100644 index 000000000..c9d66975c --- /dev/null +++ b/netbox/ipam/migrations/0038_custom_field_data.py @@ -0,0 +1,43 @@ +# Generated by Django 3.1 on 2020-08-21 18:34 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('ipam', '0037_ipaddress_assignment'), + ] + + operations = [ + migrations.AddField( + model_name='aggregate', + name='custom_field_data', + field=models.JSONField(blank=True, default=dict), + ), + migrations.AddField( + model_name='ipaddress', + name='custom_field_data', + field=models.JSONField(blank=True, default=dict), + ), + migrations.AddField( + model_name='prefix', + name='custom_field_data', + field=models.JSONField(blank=True, default=dict), + ), + migrations.AddField( + model_name='service', + name='custom_field_data', + field=models.JSONField(blank=True, default=dict), + ), + migrations.AddField( + model_name='vlan', + name='custom_field_data', + field=models.JSONField(blank=True, default=dict), + ), + migrations.AddField( + model_name='vrf', + name='custom_field_data', + field=models.JSONField(blank=True, default=dict), + ), + ] diff --git a/netbox/secrets/migrations/0010_custom_field_data.py b/netbox/secrets/migrations/0010_custom_field_data.py new file mode 100644 index 000000000..33b5f06e1 --- /dev/null +++ b/netbox/secrets/migrations/0010_custom_field_data.py @@ -0,0 +1,18 @@ +# Generated by Django 3.1 on 2020-08-21 18:34 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('secrets', '0009_secretrole_drop_users_groups'), + ] + + operations = [ + migrations.AddField( + model_name='secret', + name='custom_field_data', + field=models.JSONField(blank=True, default=dict), + ), + ] diff --git a/netbox/tenancy/migrations/0010_custom_field_data.py b/netbox/tenancy/migrations/0010_custom_field_data.py new file mode 100644 index 000000000..4d05a8272 --- /dev/null +++ b/netbox/tenancy/migrations/0010_custom_field_data.py @@ -0,0 +1,18 @@ +# Generated by Django 3.1 on 2020-08-21 18:34 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('tenancy', '0009_standardize_description'), + ] + + operations = [ + migrations.AddField( + model_name='tenant', + name='custom_field_data', + field=models.JSONField(blank=True, default=dict), + ), + ] diff --git a/netbox/virtualization/migrations/0018_custom_field_data.py b/netbox/virtualization/migrations/0018_custom_field_data.py new file mode 100644 index 000000000..5c129e2ab --- /dev/null +++ b/netbox/virtualization/migrations/0018_custom_field_data.py @@ -0,0 +1,23 @@ +# Generated by Django 3.1 on 2020-08-21 18:34 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('virtualization', '0017_update_jsonfield'), + ] + + operations = [ + migrations.AddField( + model_name='cluster', + name='custom_field_data', + field=models.JSONField(blank=True, default=dict), + ), + migrations.AddField( + model_name='virtualmachine', + name='custom_field_data', + field=models.JSONField(blank=True, default=dict), + ), + ] From 2276603ac386a0c7edfe8a7cb6ad856fd44b392f Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Fri, 21 Aug 2020 15:53:38 -0400 Subject: [PATCH 02/19] Drop CustomFieldValue --- docs/administration/netbox-shell.md | 8 +- netbox/circuits/models.py | 11 --- netbox/dcim/models/devices.py | 10 -- netbox/dcim/models/power.py | 6 -- netbox/dcim/models/racks.py | 5 - netbox/dcim/models/sites.py | 5 - netbox/extras/api/customfields.py | 11 +-- netbox/extras/api/views.py | 4 - netbox/extras/forms.py | 26 +----- .../0051_delete_customfieldvalue.py | 16 ++++ netbox/extras/models/__init__.py | 3 +- netbox/extras/models/customfields.py | 91 +++---------------- netbox/extras/querysets.py | 20 +--- netbox/extras/tests/test_changelog.py | 22 ++--- netbox/extras/tests/test_customfields.py | 60 ++++++------ netbox/ipam/models.py | 32 +------ netbox/secrets/models.py | 6 -- netbox/tenancy/models.py | 6 -- netbox/utilities/views.py | 49 ++-------- netbox/virtualization/models.py | 10 -- 20 files changed, 87 insertions(+), 314 deletions(-) create mode 100644 netbox/extras/migrations/0051_delete_customfieldvalue.py diff --git a/docs/administration/netbox-shell.md b/docs/administration/netbox-shell.md index 51a06156a..2a8d31494 100644 --- a/docs/administration/netbox-shell.md +++ b/docs/administration/netbox-shell.md @@ -185,7 +185,7 @@ To delete an object, simply call `delete()` on its instance. This will return a >>> vlan >>> vlan.delete() -(1, {'extras.CustomFieldValue': 0, 'ipam.VLAN': 1}) +(1, {'ipam.VLAN': 1}) ``` To delete multiple objects at once, call `delete()` on a filtered queryset. It's a good idea to always sanity-check the count of selected objects _before_ deleting them. @@ -194,9 +194,9 @@ To delete multiple objects at once, call `delete()` on a filtered queryset. It's >>> Device.objects.filter(name__icontains='test').count() 27 >>> Device.objects.filter(name__icontains='test').delete() -(35, {'extras.CustomFieldValue': 0, 'dcim.DeviceBay': 0, 'secrets.Secret': 0, -'dcim.InterfaceConnection': 4, 'extras.ImageAttachment': 0, 'dcim.Device': 27, -'dcim.Interface': 4, 'dcim.ConsolePort': 0, 'dcim.PowerPort': 0}) +(35, {'dcim.DeviceBay': 0, 'secrets.Secret': 0, 'dcim.InterfaceConnection': 4, +'extras.ImageAttachment': 0, 'dcim.Device': 27, 'dcim.Interface': 4, +'dcim.ConsolePort': 0, 'dcim.PowerPort': 0}) ``` !!! warning diff --git a/netbox/circuits/models.py b/netbox/circuits/models.py index 14f555cc6..fbe568f18 100644 --- a/netbox/circuits/models.py +++ b/netbox/circuits/models.py @@ -1,4 +1,3 @@ -from django.contrib.contenttypes.fields import GenericRelation from django.db import models from django.urls import reverse from taggit.managers import TaggableManager @@ -61,11 +60,6 @@ class Provider(ChangeLoggedModel, CustomFieldModel): comments = models.TextField( blank=True ) - custom_field_values = GenericRelation( - to='extras.CustomFieldValue', - content_type_field='obj_type', - object_id_field='obj_id' - ) tags = TaggableManager(through=TaggedItem) objects = RestrictedQuerySet.as_manager() @@ -186,11 +180,6 @@ class Circuit(ChangeLoggedModel, CustomFieldModel): comments = models.TextField( blank=True ) - custom_field_values = GenericRelation( - to='extras.CustomFieldValue', - content_type_field='obj_type', - object_id_field='obj_id' - ) objects = CircuitQuerySet.as_manager() tags = TaggableManager(through=TaggedItem) diff --git a/netbox/dcim/models/devices.py b/netbox/dcim/models/devices.py index 8bb56101e..b10fd6b74 100644 --- a/netbox/dcim/models/devices.py +++ b/netbox/dcim/models/devices.py @@ -134,11 +134,6 @@ class DeviceType(ChangeLoggedModel, CustomFieldModel): comments = models.TextField( blank=True ) - custom_field_values = GenericRelation( - to='extras.CustomFieldValue', - content_type_field='obj_type', - object_id_field='obj_id' - ) tags = TaggableManager(through=TaggedItem) objects = RestrictedQuerySet.as_manager() @@ -584,11 +579,6 @@ class Device(ChangeLoggedModel, ConfigContextModel, CustomFieldModel): comments = models.TextField( blank=True ) - custom_field_values = GenericRelation( - to='extras.CustomFieldValue', - content_type_field='obj_type', - object_id_field='obj_id' - ) images = GenericRelation( to='extras.ImageAttachment' ) diff --git a/netbox/dcim/models/power.py b/netbox/dcim/models/power.py index f760fea13..08ae194ae 100644 --- a/netbox/dcim/models/power.py +++ b/netbox/dcim/models/power.py @@ -1,4 +1,3 @@ -from django.contrib.contenttypes.fields import GenericRelation from django.core.exceptions import ValidationError from django.core.validators import MaxValueValidator, MinValueValidator from django.db import models @@ -144,11 +143,6 @@ class PowerFeed(ChangeLoggedModel, CableTermination, CustomFieldModel): comments = models.TextField( blank=True ) - custom_field_values = GenericRelation( - to='extras.CustomFieldValue', - content_type_field='obj_type', - object_id_field='obj_id' - ) tags = TaggableManager(through=TaggedItem) objects = RestrictedQuerySet.as_manager() diff --git a/netbox/dcim/models/racks.py b/netbox/dcim/models/racks.py index 3169272b4..e30481cd6 100644 --- a/netbox/dcim/models/racks.py +++ b/netbox/dcim/models/racks.py @@ -261,11 +261,6 @@ class Rack(ChangeLoggedModel, CustomFieldModel): comments = models.TextField( blank=True ) - custom_field_values = GenericRelation( - to='extras.CustomFieldValue', - content_type_field='obj_type', - object_id_field='obj_id' - ) images = GenericRelation( to='extras.ImageAttachment' ) diff --git a/netbox/dcim/models/sites.py b/netbox/dcim/models/sites.py index 1ea083367..0d33da806 100644 --- a/netbox/dcim/models/sites.py +++ b/netbox/dcim/models/sites.py @@ -183,11 +183,6 @@ class Site(ChangeLoggedModel, CustomFieldModel): comments = models.TextField( blank=True ) - custom_field_values = GenericRelation( - to='extras.CustomFieldValue', - content_type_field='obj_type', - object_id_field='obj_id' - ) images = GenericRelation( to='extras.ImageAttachment' ) diff --git a/netbox/extras/api/customfields.py b/netbox/extras/api/customfields.py index 5ef983977..a0238129b 100644 --- a/netbox/extras/api/customfields.py +++ b/netbox/extras/api/customfields.py @@ -8,7 +8,7 @@ from rest_framework.exceptions import ValidationError from rest_framework.fields import CreateOnlyDefault from extras.choices import * -from extras.models import CustomField, CustomFieldChoice, CustomFieldValue +from extras.models import CustomField, CustomFieldChoice from utilities.api import ValidatedModelSerializer @@ -164,15 +164,8 @@ class CustomFieldModelSerializer(ValidatedModelSerializer): instance.custom_fields[field.name] = value def _save_custom_fields(self, instance, custom_fields): - content_type = ContentType.objects.get_for_model(self.Meta.model) for field_name, value in custom_fields.items(): - custom_field = CustomField.objects.get(name=field_name) - CustomFieldValue.objects.update_or_create( - field=custom_field, - obj_type=content_type, - obj_id=instance.pk, - defaults={'serialized_value': custom_field.serialize_value(value)}, - ) + instance.custom_field_data[field_name] = value def create(self, validated_data): diff --git a/netbox/extras/api/views.py b/netbox/extras/api/views.py index 5fa26a0d7..5be8276b6 100644 --- a/netbox/extras/api/views.py +++ b/netbox/extras/api/views.py @@ -93,10 +93,6 @@ class CustomFieldModelViewSet(ModelViewSet): }) return context - def get_queryset(self): - # Prefetch custom field values - return super().get_queryset().prefetch_related('custom_field_values__field') - # # Export templates diff --git a/netbox/extras/forms.py b/netbox/extras/forms.py index 90ec828c7..40c675c4d 100644 --- a/netbox/extras/forms.py +++ b/netbox/extras/forms.py @@ -12,7 +12,7 @@ from utilities.forms import ( ) from virtualization.models import Cluster, ClusterGroup from .choices import * -from .models import ConfigContext, CustomField, CustomFieldValue, ImageAttachment, ObjectChange, Tag +from .models import ConfigContext, CustomField, ImageAttachment, ObjectChange, Tag # @@ -40,11 +40,7 @@ class CustomFieldModelForm(forms.ModelForm): """ # Retrieve initial CustomField values for the instance if self.instance.pk: - for cfv in CustomFieldValue.objects.filter( - obj_type=self.obj_type, - obj_id=self.instance.pk - ).prefetch_related('field'): - self.custom_field_values[cfv.field.name] = cfv.serialized_value + self.custom_field_values = self.instance.custom_field_data # Append form fields; assign initial values if modifying and existing object for cf in CustomField.objects.filter(obj_type=self.obj_type): @@ -64,23 +60,7 @@ class CustomFieldModelForm(forms.ModelForm): def _save_custom_fields(self): for field_name in self.custom_fields: - try: - cfv = CustomFieldValue.objects.prefetch_related('field').get( - field=self.fields[field_name].model, - obj_type=self.obj_type, - obj_id=self.instance.pk - ) - except CustomFieldValue.DoesNotExist: - # Skip this field if none exists already and its value is empty - if self.cleaned_data[field_name] in [None, '']: - continue - cfv = CustomFieldValue( - field=self.fields[field_name].model, - obj_type=self.obj_type, - obj_id=self.instance.pk - ) - cfv.value = self.cleaned_data[field_name] - cfv.save() + self.instance.custom_field_data[field_name[3:]] = self.cleaned_data[field_name] def save(self, commit=True): diff --git a/netbox/extras/migrations/0051_delete_customfieldvalue.py b/netbox/extras/migrations/0051_delete_customfieldvalue.py new file mode 100644 index 000000000..3369289a0 --- /dev/null +++ b/netbox/extras/migrations/0051_delete_customfieldvalue.py @@ -0,0 +1,16 @@ +# Generated by Django 3.1 on 2020-08-21 19:52 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('extras', '0050_migrate_customfieldvalues'), + ] + + operations = [ + migrations.DeleteModel( + name='CustomFieldValue', + ), + ] diff --git a/netbox/extras/models/__init__.py b/netbox/extras/models/__init__.py index 9437fe01f..a4178b911 100644 --- a/netbox/extras/models/__init__.py +++ b/netbox/extras/models/__init__.py @@ -1,5 +1,5 @@ from .change_logging import ChangeLoggedModel, ObjectChange -from .customfields import CustomField, CustomFieldChoice, CustomFieldModel, CustomFieldValue +from .customfields import CustomField, CustomFieldChoice, CustomFieldModel from .models import ( ConfigContext, ConfigContextModel, CustomLink, ExportTemplate, ImageAttachment, JobResult, Report, Script, Webhook, @@ -13,7 +13,6 @@ __all__ = ( 'CustomField', 'CustomFieldChoice', 'CustomFieldModel', - 'CustomFieldValue', 'CustomLink', 'ExportTemplate', 'ImageAttachment', diff --git a/netbox/extras/models/customfields.py b/netbox/extras/models/customfields.py index 1c86812e0..b0ea76cef 100644 --- a/netbox/extras/models/customfields.py +++ b/netbox/extras/models/customfields.py @@ -1,8 +1,6 @@ -from collections import OrderedDict from datetime import date from django import forms -from django.contrib.contenttypes.fields import GenericForeignKey from django.contrib.contenttypes.models import ContentType from django.core.validators import ValidationError from django.db import models @@ -29,36 +27,12 @@ class CustomFieldModel(models.Model): self._cf = custom_fields super().__init__(*args, **kwargs) - def cache_custom_fields(self): - """ - Cache all custom field values for this instance - """ - self._cf = { - field.name: value for field, value in self.get_custom_fields().items() - } - @property def cf(self): """ - Name-based CustomFieldValue accessor for use in templates + Convenience wrapper for custom field data. """ - if self._cf is None: - self.cache_custom_fields() - return self._cf - - def get_custom_fields(self): - """ - Return a dictionary of custom fields for a single object in the form {: value}. - """ - fields = CustomField.objects.get_for_model(self) - - # If the object exists, populate its custom fields with values - if hasattr(self, 'pk'): - values = self.custom_field_values.all() - values_dict = {cfv.field_id: cfv.value for cfv in values} - return OrderedDict([(field, values_dict.get(field.pk)) for field in fields]) - else: - return OrderedDict([(field, None) for field in fields]) + return self.custom_field_data class CustomFieldManager(models.Manager): @@ -235,49 +209,6 @@ class CustomField(models.Model): return field -class CustomFieldValue(models.Model): - field = models.ForeignKey( - to='extras.CustomField', - on_delete=models.CASCADE, - related_name='values' - ) - obj_type = models.ForeignKey( - to=ContentType, - on_delete=models.PROTECT, - related_name='+' - ) - obj_id = models.PositiveIntegerField() - obj = GenericForeignKey( - ct_field='obj_type', - fk_field='obj_id' - ) - serialized_value = models.CharField( - max_length=255 - ) - - class Meta: - ordering = ('obj_type', 'obj_id', 'pk') # (obj_type, obj_id) may be non-unique - unique_together = ('field', 'obj_type', 'obj_id') - - def __str__(self): - return '{} {}'.format(self.obj, self.field) - - @property - def value(self): - return self.field.deserialize_value(self.serialized_value) - - @value.setter - def value(self, value): - self.serialized_value = self.field.serialize_value(value) - - def save(self, *args, **kwargs): - # Delete this object if it no longer has a value to store - if self.pk and self.value is None: - self.delete() - else: - super().save(*args, **kwargs) - - class CustomFieldChoice(models.Model): field = models.ForeignKey( to='extras.CustomField', @@ -304,11 +235,13 @@ class CustomFieldChoice(models.Model): if self.field.type != CustomFieldTypeChoices.TYPE_SELECT: raise ValidationError("Custom field choices can only be assigned to selection fields.") - def delete(self, using=None, keep_parents=False): - # When deleting a CustomFieldChoice, delete all CustomFieldValues which point to it - pk = self.pk - super().delete(using, keep_parents) - CustomFieldValue.objects.filter( - field__type=CustomFieldTypeChoices.TYPE_SELECT, - serialized_value=str(pk) - ).delete() + def delete(self, *args, **kwargs): + # TODO: Prevent deletion of CustomFieldChoices which are in use? + field_name = f'custom_field_data__{self.field.name}' + for ct in self.field.obj_type.all(): + model = ct.model_class() + for instance in model.objects.filter(**{field_name: self.pk}): + instance.custom_field_data.pop(self.field.name) + instance.save() + + super().delete(*args, **kwargs) diff --git a/netbox/extras/querysets.py b/netbox/extras/querysets.py index 9d9b55778..b7019897b 100644 --- a/netbox/extras/querysets.py +++ b/netbox/extras/querysets.py @@ -1,26 +1,8 @@ -from collections import OrderedDict - -from django.db.models import Q, QuerySet +from django.db.models import Q from utilities.querysets import RestrictedQuerySet -class CustomFieldQueryset: - """ - Annotate custom fields on objects within a QuerySet. - """ - def __init__(self, queryset, custom_fields): - self.queryset = queryset - self.model = queryset.model - self.custom_fields = custom_fields - - def __iter__(self): - for obj in self.queryset: - values_dict = {cfv.field_id: cfv.value for cfv in obj.custom_field_values.all()} - obj.custom_fields = OrderedDict([(field, values_dict.get(field.pk)) for field in self.custom_fields]) - yield obj - - class ConfigContextQuerySet(RestrictedQuerySet): def get_for_object(self, obj): diff --git a/netbox/extras/tests/test_changelog.py b/netbox/extras/tests/test_changelog.py index 8c9d09564..5d1dee864 100644 --- a/netbox/extras/tests/test_changelog.py +++ b/netbox/extras/tests/test_changelog.py @@ -5,7 +5,7 @@ from rest_framework import status from dcim.choices import SiteStatusChoices from dcim.models import Site from extras.choices import * -from extras.models import CustomField, CustomFieldValue, ObjectChange, Tag +from extras.models import CustomField, ObjectChange, Tag from utilities.testing import APITestCase from utilities.testing.utils import post_data from utilities.testing.views import ModelViewTestCase @@ -93,16 +93,14 @@ class ChangeLogViewTest(ModelViewTestCase): def test_delete_object(self): site = Site( name='Test Site 1', - slug='test-site-1' + slug='test-site-1', + custom_field_data={ + 'my_field': 'ABC' + } ) site.save() self.create_tags('Tag 1', 'Tag 2') site.tags.set('Tag 1', 'Tag 2') - CustomFieldValue.objects.create( - field=CustomField.objects.get(name='my_field'), - obj=site, - value='ABC' - ) request = { 'path': self._get_url('delete', instance=site), @@ -209,15 +207,13 @@ class ChangeLogAPITest(APITestCase): def test_delete_object(self): site = Site( name='Test Site 1', - slug='test-site-1' + slug='test-site-1', + custom_field_data={ + 'my_field': 'ABC' + } ) site.save() site.tags.set(*Tag.objects.all()[:2]) - CustomFieldValue.objects.create( - field=CustomField.objects.get(name='my_field'), - obj=site, - value='ABC' - ) self.assertEqual(ObjectChange.objects.count(), 0) self.add_permissions('dcim.delete_site') url = reverse('dcim-api:site-detail', kwargs={'pk': site.pk}) diff --git a/netbox/extras/tests/test_customfields.py b/netbox/extras/tests/test_customfields.py index e543b63e3..74c0e7c3b 100644 --- a/netbox/extras/tests/test_customfields.py +++ b/netbox/extras/tests/test_customfields.py @@ -7,7 +7,7 @@ from rest_framework import status from dcim.forms import SiteCSVForm from dcim.models import Site from extras.choices import * -from extras.models import CustomField, CustomFieldValue, CustomFieldChoice +from extras.models import CustomField, CustomFieldChoice from utilities.testing import APITestCase, TestCase from virtualization.models import VirtualMachine @@ -46,18 +46,18 @@ class CustomFieldTest(TestCase): # Assign a value to the first Site site = Site.objects.first() - cfv = CustomFieldValue(field=cf, obj_type=obj_type, obj_id=site.id) - cfv.value = data['field_value'] - cfv.save() + site.custom_field_data[cf.name] = data['field_value'] + site.save() # Retrieve the stored value - cfv = CustomFieldValue.objects.filter(obj_type=obj_type, obj_id=site.pk).first() - self.assertEqual(cfv.value, data['field_value']) + site.refresh_from_db() + self.assertEqual(site.custom_field_data[cf.name], data['field_value']) # Delete the stored value - cfv.value = data['empty_value'] - cfv.save() - self.assertEqual(CustomFieldValue.objects.filter(obj_type=obj_type, obj_id=site.pk).count(), 0) + site.custom_field_data.pop(cf.name) + site.save() + site.refresh_from_db() + self.assertIsNone(site.custom_field_data.get(cf.name)) # Delete the custom field cf.delete() @@ -81,18 +81,18 @@ class CustomFieldTest(TestCase): # Assign a value to the first Site site = Site.objects.first() - cfv = CustomFieldValue(field=cf, obj_type=obj_type, obj_id=site.id) - cfv.value = cf.choices.first() - cfv.save() + site.custom_field_data[cf.name] = cf.choices.first().pk + site.save() # Retrieve the stored value - cfv = CustomFieldValue.objects.filter(obj_type=obj_type, obj_id=site.pk).first() - self.assertEqual(str(cfv.value), 'Option A') + site.refresh_from_db() + self.assertEqual(site.custom_field_data[cf.name], 'Option A') # Delete the stored value - cfv.value = None - cfv.save() - self.assertEqual(CustomFieldValue.objects.filter(obj_type=obj_type, obj_id=site.pk).count(), 0) + site.custom_field_data.pop(cf.name) + site.save() + site.refresh_from_db() + self.assertIsNone(site.custom_field_data.get(cf.name)) # Delete the custom field cf.delete() @@ -164,18 +164,15 @@ class CustomFieldAPITest(APITestCase): Site.objects.bulk_create(cls.sites) # Assign custom field values for site 2 - site2_cfvs = { - cls.cf_text: 'bar', - cls.cf_integer: 456, - cls.cf_boolean: True, - cls.cf_date: '2020-01-02', - cls.cf_url: 'http://example.com/2', - cls.cf_select: cls.cf_select_choice2.pk, + cls.sites[1].custom_field_data = { + cls.cf_text.name: 'bar', + cls.cf_integer.name: 456, + cls.cf_boolean.name: True, + cls.cf_date.name: '2020-01-02', + cls.cf_url.name: 'http://example.com/2', + cls.cf_select.name: cls.cf_select_choice2.pk, } - for field, value in site2_cfvs.items(): - cfv = CustomFieldValue(field=field, obj=cls.sites[1]) - cfv.value = value - cfv.save() + cls.sites[1].save() def test_get_single_object_without_custom_field_values(self): """ @@ -518,7 +515,7 @@ class CustomFieldImportTest(TestCase): # Validate data for site 1 custom_field_values = { - cf.name: value for cf, value in Site.objects.get(name='Site 1').get_custom_fields().items() + cf.name: value for cf, value in Site.objects.get(name='Site 1').custom_field_data } self.assertEqual(len(custom_field_values), 6) self.assertEqual(custom_field_values['text'], 'ABC') @@ -530,7 +527,7 @@ class CustomFieldImportTest(TestCase): # Validate data for site 2 custom_field_values = { - cf.name: value for cf, value in Site.objects.get(name='Site 2').get_custom_fields().items() + cf.name: value for cf, value in Site.objects.get(name='Site 2').custom_field_data } self.assertEqual(len(custom_field_values), 6) self.assertEqual(custom_field_values['text'], 'DEF') @@ -543,8 +540,7 @@ class CustomFieldImportTest(TestCase): # No CustomFieldValues should be created for site 3 obj_type = ContentType.objects.get_for_model(Site) site3 = Site.objects.get(name='Site 3') - self.assertFalse(CustomFieldValue.objects.filter(obj_type=obj_type, obj_id=site3.pk).exists()) - self.assertEqual(CustomFieldValue.objects.count(), 12) # Sanity check + self.assertEqual(site3.custom_field_data, {}) def test_import_missing_required(self): """ diff --git a/netbox/ipam/models.py b/netbox/ipam/models.py index 58dd96089..e5ae122ab 100644 --- a/netbox/ipam/models.py +++ b/netbox/ipam/models.py @@ -1,6 +1,6 @@ import netaddr from django.conf import settings -from django.contrib.contenttypes.fields import GenericForeignKey, GenericRelation +from django.contrib.contenttypes.fields import GenericForeignKey from django.contrib.contenttypes.models import ContentType from django.core.exceptions import ValidationError from django.core.validators import MaxValueValidator, MinValueValidator @@ -70,11 +70,6 @@ class VRF(ChangeLoggedModel, CustomFieldModel): max_length=200, blank=True ) - custom_field_values = GenericRelation( - to='extras.CustomFieldValue', - content_type_field='obj_type', - object_id_field='obj_id' - ) tags = TaggableManager(through=TaggedItem) objects = RestrictedQuerySet.as_manager() @@ -178,11 +173,6 @@ class Aggregate(ChangeLoggedModel, CustomFieldModel): max_length=200, blank=True ) - custom_field_values = GenericRelation( - to='extras.CustomFieldValue', - content_type_field='obj_type', - object_id_field='obj_id' - ) tags = TaggableManager(through=TaggedItem) objects = RestrictedQuerySet.as_manager() @@ -364,11 +354,6 @@ class Prefix(ChangeLoggedModel, CustomFieldModel): max_length=200, blank=True ) - custom_field_values = GenericRelation( - to='extras.CustomFieldValue', - content_type_field='obj_type', - object_id_field='obj_id' - ) tags = TaggableManager(through=TaggedItem) objects = PrefixQuerySet.as_manager() @@ -647,11 +632,6 @@ class IPAddress(ChangeLoggedModel, CustomFieldModel): max_length=200, blank=True ) - custom_field_values = GenericRelation( - to='extras.CustomFieldValue', - content_type_field='obj_type', - object_id_field='obj_id' - ) tags = TaggableManager(through=TaggedItem) objects = IPAddressManager() @@ -928,11 +908,6 @@ class VLAN(ChangeLoggedModel, CustomFieldModel): max_length=200, blank=True ) - custom_field_values = GenericRelation( - to='extras.CustomFieldValue', - content_type_field='obj_type', - object_id_field='obj_id' - ) tags = TaggableManager(through=TaggedItem) objects = RestrictedQuerySet.as_manager() @@ -1043,11 +1018,6 @@ class Service(ChangeLoggedModel, CustomFieldModel): max_length=200, blank=True ) - custom_field_values = GenericRelation( - to='extras.CustomFieldValue', - content_type_field='obj_type', - object_id_field='obj_id' - ) tags = TaggableManager(through=TaggedItem) objects = RestrictedQuerySet.as_manager() diff --git a/netbox/secrets/models.py b/netbox/secrets/models.py index 6209b5700..23a883103 100644 --- a/netbox/secrets/models.py +++ b/netbox/secrets/models.py @@ -6,7 +6,6 @@ from Crypto.Util import strxor from django.conf import settings from django.contrib.auth.hashers import make_password, check_password from django.contrib.auth.models import Group, User -from django.contrib.contenttypes.fields import GenericRelation from django.core.exceptions import ValidationError from django.db import models from django.urls import reverse @@ -306,11 +305,6 @@ class Secret(ChangeLoggedModel, CustomFieldModel): max_length=128, editable=False ) - custom_field_values = GenericRelation( - to='extras.CustomFieldValue', - content_type_field='obj_type', - object_id_field='obj_id' - ) tags = TaggableManager(through=TaggedItem) objects = RestrictedQuerySet.as_manager() diff --git a/netbox/tenancy/models.py b/netbox/tenancy/models.py index cc3abf19a..5a6108e09 100644 --- a/netbox/tenancy/models.py +++ b/netbox/tenancy/models.py @@ -1,4 +1,3 @@ -from django.contrib.contenttypes.fields import GenericRelation from django.db import models from django.urls import reverse from mptt.models import MPTTModel, TreeForeignKey @@ -102,11 +101,6 @@ class Tenant(ChangeLoggedModel, CustomFieldModel): comments = models.TextField( blank=True ) - custom_field_values = GenericRelation( - to='extras.CustomFieldValue', - content_type_field='obj_type', - object_id_field='obj_id' - ) tags = TaggableManager(through=TaggedItem) objects = RestrictedQuerySet.as_manager() diff --git a/netbox/utilities/views.py b/netbox/utilities/views.py index c7db2f649..51c7a26f9 100644 --- a/netbox/utilities/views.py +++ b/netbox/utilities/views.py @@ -26,8 +26,7 @@ from django.views.defaults import ERROR_500_TEMPLATE_NAME from django.views.generic import View from django_tables2 import RequestConfig -from extras.models import CustomField, CustomFieldValue, ExportTemplate -from extras.querysets import CustomFieldQueryset +from extras.models import CustomField, ExportTemplate from utilities.exceptions import AbortTransaction from utilities.forms import BootstrapMixin, BulkRenameForm, CSVDataField, TableConfigForm, restrict_form_fields from utilities.permissions import get_permission_for_model, resolve_permission @@ -229,8 +228,8 @@ class ObjectListView(ObjectPermissionRequiredMixin, View): headers = self.queryset.model.csv_headers.copy() # Add custom field headers, if any - if hasattr(self.queryset.model, 'get_custom_fields'): - for custom_field in self.queryset.model().get_custom_fields(): + if hasattr(self.queryset.model, 'custom_field_data'): + for custom_field in CustomField.objects.get_for_model(self.queryset.model): headers.append(custom_field.name) custom_fields.append(custom_field.name) @@ -255,19 +254,11 @@ class ObjectListView(ObjectPermissionRequiredMixin, View): if self.filterset: self.queryset = self.filterset(request.GET, self.queryset).qs - # If this type of object has one or more custom fields, prefetch any relevant custom field values - custom_fields = CustomField.objects.filter( - obj_type=ContentType.objects.get_for_model(model) - ).prefetch_related('choices') - if custom_fields: - self.queryset = self.queryset.prefetch_related('custom_field_values') - # Check for export template rendering if request.GET.get('export'): et = get_object_or_404(ExportTemplate, content_type=content_type, name=request.GET.get('export')) - queryset = CustomFieldQueryset(self.queryset, custom_fields) if custom_fields else self.queryset try: - return et.render_to_response(queryset) + return et.render_to_response(self.queryset) except Exception as e: messages.error( request, @@ -949,38 +940,18 @@ class BulkEditView(GetReturnURLMixin, ObjectPermissionRequiredMixin, View): elif form.cleaned_data[name] not in (None, ''): setattr(obj, name, form.cleaned_data[name]) - # Cache custom fields on instance prior to save() - if custom_fields: - obj._cf = { - name: form.cleaned_data[name] for name in custom_fields - } + # Update custom fields + for name in custom_fields: + if name in form.nullable_fields and name in nullified_fields: + obj.custom_field_data.pop(name, None) + else: + obj.custom_field_data[name] = form.cleaned_data[name] obj.full_clean() obj.save() updated_objects.append(obj) logger.debug(f"Saved {obj} (PK: {obj.pk})") - # Update custom fields - obj_type = ContentType.objects.get_for_model(model) - for name in custom_fields: - field = form.fields[name].model - if name in form.nullable_fields and name in nullified_fields: - CustomFieldValue.objects.filter( - field=field, obj_type=obj_type, obj_id=obj.pk - ).delete() - elif form.cleaned_data[name] not in [None, '']: - try: - cfv = CustomFieldValue.objects.get( - field=field, obj_type=obj_type, obj_id=obj.pk - ) - except CustomFieldValue.DoesNotExist: - cfv = CustomFieldValue( - field=field, obj_type=obj_type, obj_id=obj.pk - ) - cfv.value = form.cleaned_data[name] - cfv.save() - logger.debug(f"Saved custom fields for {obj} (PK: {obj.pk})") - # Add/remove tags if form.cleaned_data.get('add_tags', None): obj.tags.add(*form.cleaned_data['add_tags']) diff --git a/netbox/virtualization/models.py b/netbox/virtualization/models.py index fb61c5b9e..00d444de9 100644 --- a/netbox/virtualization/models.py +++ b/netbox/virtualization/models.py @@ -150,11 +150,6 @@ class Cluster(ChangeLoggedModel, CustomFieldModel): comments = models.TextField( blank=True ) - custom_field_values = GenericRelation( - to='extras.CustomFieldValue', - content_type_field='obj_type', - object_id_field='obj_id' - ) tags = TaggableManager(through=TaggedItem) objects = RestrictedQuerySet.as_manager() @@ -275,11 +270,6 @@ class VirtualMachine(ChangeLoggedModel, ConfigContextModel, CustomFieldModel): comments = models.TextField( blank=True ) - custom_field_values = GenericRelation( - to='extras.CustomFieldValue', - content_type_field='obj_type', - object_id_field='obj_id' - ) tags = TaggableManager(through=TaggedItem) objects = RestrictedQuerySet.as_manager() From c85a45e5208d21c9b48dd705ecfa6a1e520b548c Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Mon, 24 Aug 2020 14:11:13 -0400 Subject: [PATCH 03/19] Further work on custom fields --- netbox/extras/api/customfields.py | 4 +- netbox/extras/forms.py | 19 +--- netbox/extras/models/customfields.py | 10 ++ netbox/extras/tests/test_customfields.py | 126 ++++++++++------------- 4 files changed, 67 insertions(+), 92 deletions(-) diff --git a/netbox/extras/api/customfields.py b/netbox/extras/api/customfields.py index a0238129b..df00d0c1d 100644 --- a/netbox/extras/api/customfields.py +++ b/netbox/extras/api/customfields.py @@ -148,10 +148,10 @@ class CustomFieldModelSerializer(ValidatedModelSerializer): fields = CustomField.objects.filter(obj_type=content_type) # Populate CustomFieldValues for each instance from database - try: + if type(self.instance) in (list, tuple): for obj in self.instance: self._populate_custom_fields(obj, fields) - except TypeError: + else: self._populate_custom_fields(self.instance, fields) def _populate_custom_fields(self, instance, custom_fields): diff --git a/netbox/extras/forms.py b/netbox/extras/forms.py index 40c675c4d..96290ef0a 100644 --- a/netbox/extras/forms.py +++ b/netbox/extras/forms.py @@ -57,26 +57,13 @@ class CustomFieldModelForm(forms.ModelForm): # Annotate the field in the list of CustomField form fields self.custom_fields.append(field_name) - def _save_custom_fields(self): - - for field_name in self.custom_fields: - self.instance.custom_field_data[field_name[3:]] = self.cleaned_data[field_name] - def save(self, commit=True): - # Cache custom field values on object prior to save to ensure change logging + # Save custom field data on instance for cf_name in self.custom_fields: - self.instance._cf[cf_name[3:]] = self.cleaned_data.get(cf_name) + self.instance.custom_field_data[cf_name[3:]] = self.cleaned_data.get(cf_name) - obj = super().save(commit) - - # Handle custom fields the same way we do M2M fields - if commit: - self._save_custom_fields() - else: - obj.save_custom_fields = self._save_custom_fields - - return obj + return super().save(commit) class CustomFieldModelCSVForm(CSVModelForm, CustomFieldModelForm): diff --git a/netbox/extras/models/customfields.py b/netbox/extras/models/customfields.py index b0ea76cef..166ef5708 100644 --- a/netbox/extras/models/customfields.py +++ b/netbox/extras/models/customfields.py @@ -1,3 +1,4 @@ +from collections import OrderedDict from datetime import date from django import forms @@ -34,6 +35,15 @@ class CustomFieldModel(models.Model): """ return self.custom_field_data + def get_custom_fields(self): + """ + Return a dictionary of custom fields for a single object in the form {: value}. + """ + fields = CustomField.objects.get_for_model(self) + return OrderedDict([ + (field, self.custom_field_data.get(field.name)) for field in fields + ]) + class CustomFieldManager(models.Manager): use_in_migrations = True diff --git a/netbox/extras/tests/test_customfields.py b/netbox/extras/tests/test_customfields.py index 74c0e7c3b..71254ac05 100644 --- a/netbox/extras/tests/test_customfields.py +++ b/netbox/extras/tests/test_customfields.py @@ -174,7 +174,7 @@ class CustomFieldAPITest(APITestCase): } cls.sites[1].save() - def test_get_single_object_without_custom_field_values(self): + def test_get_single_object_without_custom_field_data(self): """ Validate that custom fields are present on an object even if it has no values defined. """ @@ -192,13 +192,11 @@ class CustomFieldAPITest(APITestCase): 'choice_field': None, }) - def test_get_single_object_with_custom_field_values(self): + def test_get_single_object_with_custom_field_data(self): """ Validate that custom fields are present and correctly set for an object with values defined. """ - site2_cfvs = { - cfv.field.name: cfv.value for cfv in self.sites[1].custom_field_values.all() - } + site2_cfvs = self.sites[1].custom_field_data url = reverse('dcim-api:site-detail', kwargs={'pk': self.sites[1].pk}) self.add_permissions('dcim.view_site') @@ -236,15 +234,12 @@ class CustomFieldAPITest(APITestCase): # Validate database data site = Site.objects.get(pk=response.data['id']) - cfvs = { - cfv.field.name: cfv.value for cfv in site.custom_field_values.all() - } - self.assertEqual(cfvs['text_field'], self.cf_text.default) - self.assertEqual(cfvs['number_field'], self.cf_integer.default) - self.assertEqual(cfvs['boolean_field'], self.cf_boolean.default) - self.assertEqual(str(cfvs['date_field']), self.cf_date.default) - self.assertEqual(cfvs['url_field'], self.cf_url.default) - self.assertEqual(cfvs['choice_field'].pk, self.cf_select_choice1.pk) + self.assertEqual(site.custom_field_data['text_field'], self.cf_text.default) + self.assertEqual(site.custom_field_data['number_field'], self.cf_integer.default) + self.assertEqual(site.custom_field_data['boolean_field'], self.cf_boolean.default) + self.assertEqual(str(site.custom_field_data['date_field']), self.cf_date.default) + self.assertEqual(site.custom_field_data['url_field'], self.cf_url.default) + self.assertEqual(site.custom_field_data['choice_field'].pk, self.cf_select_choice1.pk) def test_create_single_object_with_values(self): """ @@ -280,15 +275,12 @@ class CustomFieldAPITest(APITestCase): # Validate database data site = Site.objects.get(pk=response.data['id']) - cfvs = { - cfv.field.name: cfv.value for cfv in site.custom_field_values.all() - } - self.assertEqual(cfvs['text_field'], data_cf['text_field']) - self.assertEqual(cfvs['number_field'], data_cf['number_field']) - self.assertEqual(cfvs['boolean_field'], data_cf['boolean_field']) - self.assertEqual(str(cfvs['date_field']), data_cf['date_field']) - self.assertEqual(cfvs['url_field'], data_cf['url_field']) - self.assertEqual(cfvs['choice_field'].pk, data_cf['choice_field']) + self.assertEqual(site.custom_field_data['text_field'], data_cf['text_field']) + self.assertEqual(site.custom_field_data['number_field'], data_cf['number_field']) + self.assertEqual(site.custom_field_data['boolean_field'], data_cf['boolean_field']) + self.assertEqual(str(site.custom_field_data['date_field']), data_cf['date_field']) + self.assertEqual(site.custom_field_data['url_field'], data_cf['url_field']) + self.assertEqual(site.custom_field_data['choice_field'].pk, data_cf['choice_field']) def test_create_multiple_objects_with_defaults(self): """ @@ -329,15 +321,12 @@ class CustomFieldAPITest(APITestCase): # Validate database data site = Site.objects.get(pk=response.data[i]['id']) - cfvs = { - cfv.field.name: cfv.value for cfv in site.custom_field_values.all() - } - self.assertEqual(cfvs['text_field'], self.cf_text.default) - self.assertEqual(cfvs['number_field'], self.cf_integer.default) - self.assertEqual(cfvs['boolean_field'], self.cf_boolean.default) - self.assertEqual(str(cfvs['date_field']), self.cf_date.default) - self.assertEqual(cfvs['url_field'], self.cf_url.default) - self.assertEqual(cfvs['choice_field'].pk, self.cf_select_choice1.pk) + self.assertEqual(site.custom_field_data['text_field'], self.cf_text.default) + self.assertEqual(site.custom_field_data['number_field'], self.cf_integer.default) + self.assertEqual(site.custom_field_data['boolean_field'], self.cf_boolean.default) + self.assertEqual(str(site.custom_field_data['date_field']), self.cf_date.default) + self.assertEqual(site.custom_field_data['url_field'], self.cf_url.default) + self.assertEqual(site.custom_field_data['choice_field'].pk, self.cf_select_choice1.pk) def test_create_multiple_objects_with_values(self): """ @@ -388,24 +377,20 @@ class CustomFieldAPITest(APITestCase): # Validate database data site = Site.objects.get(pk=response.data[i]['id']) - cfvs = { - cfv.field.name: cfv.value for cfv in site.custom_field_values.all() - } - self.assertEqual(cfvs['text_field'], custom_field_data['text_field']) - self.assertEqual(cfvs['number_field'], custom_field_data['number_field']) - self.assertEqual(cfvs['boolean_field'], custom_field_data['boolean_field']) - self.assertEqual(str(cfvs['date_field']), custom_field_data['date_field']) - self.assertEqual(cfvs['url_field'], custom_field_data['url_field']) - self.assertEqual(cfvs['choice_field'].pk, custom_field_data['choice_field']) + self.assertEqual(site.custom_field_data['text_field'], custom_field_data['text_field']) + self.assertEqual(site.custom_field_data['number_field'], custom_field_data['number_field']) + self.assertEqual(site.custom_field_data['boolean_field'], custom_field_data['boolean_field']) + self.assertEqual(str(site.custom_field_data['date_field']), custom_field_data['date_field']) + self.assertEqual(site.custom_field_data['url_field'], custom_field_data['url_field']) + self.assertEqual(site.custom_field_data['choice_field'].pk, custom_field_data['choice_field']) def test_update_single_object_with_values(self): """ Update an object with existing custom field values. Ensure that only the updated custom field values are modified. """ - site2_original_cfvs = { - cfv.field.name: cfv.value for cfv in self.sites[1].custom_field_values.all() - } + site = self.sites[1] + original_cfvs = {**site.custom_field_data} data = { 'custom_fields': { 'text_field': 'ABCD', @@ -430,15 +415,13 @@ class CustomFieldAPITest(APITestCase): # self.assertEqual(response_cf['choice_field']['label'], site2_original_cfvs['choice_field'].value) # Validate database data - site2_updated_cfvs = { - cfv.field.name: cfv.value for cfv in self.sites[1].custom_field_values.all() - } - self.assertEqual(site2_updated_cfvs['text_field'], data_cf['text_field']) - self.assertEqual(site2_updated_cfvs['number_field'], data_cf['number_field']) - self.assertEqual(site2_updated_cfvs['boolean_field'], site2_original_cfvs['boolean_field']) - self.assertEqual(site2_updated_cfvs['date_field'], site2_original_cfvs['date_field']) - self.assertEqual(site2_updated_cfvs['url_field'], site2_original_cfvs['url_field']) - self.assertEqual(site2_updated_cfvs['choice_field'], site2_original_cfvs['choice_field']) + site.refresh_from_db() + self.assertEqual(site.custom_field_data['text_field'], data_cf['text_field']) + self.assertEqual(site.custom_field_data['number_field'], data_cf['number_field']) + self.assertEqual(site.custom_field_data['boolean_field'], original_cfvs['boolean_field']) + self.assertEqual(site.custom_field_data['date_field'], original_cfvs['date_field']) + self.assertEqual(site.custom_field_data['url_field'], original_cfvs['url_field']) + self.assertEqual(site.custom_field_data['choice_field'], original_cfvs['choice_field']) class CustomFieldChoiceAPITest(APITestCase): @@ -514,31 +497,26 @@ class CustomFieldImportTest(TestCase): self.assertEqual(response.status_code, 200) # Validate data for site 1 - custom_field_values = { - cf.name: value for cf, value in Site.objects.get(name='Site 1').custom_field_data - } - self.assertEqual(len(custom_field_values), 6) - self.assertEqual(custom_field_values['text'], 'ABC') - self.assertEqual(custom_field_values['integer'], 123) - self.assertEqual(custom_field_values['boolean'], True) - self.assertEqual(custom_field_values['date'], date(2020, 1, 1)) - self.assertEqual(custom_field_values['url'], 'http://example.com/1') - self.assertEqual(custom_field_values['select'].value, 'Choice A') + site1 = Site.objects.get(name='Site 1') + self.assertEqual(len(site1.custom_field_data), 6) + self.assertEqual(site1.custom_field_data['text'], 'ABC') + self.assertEqual(site1.custom_field_data['integer'], 123) + self.assertEqual(site1.custom_field_data['boolean'], True) + self.assertEqual(site1.custom_field_data['date'], date(2020, 1, 1)) + self.assertEqual(site1.custom_field_data['url'], 'http://example.com/1') + self.assertEqual(site1.custom_field_data['select'].value, 'Choice A') # Validate data for site 2 - custom_field_values = { - cf.name: value for cf, value in Site.objects.get(name='Site 2').custom_field_data - } - self.assertEqual(len(custom_field_values), 6) - self.assertEqual(custom_field_values['text'], 'DEF') - self.assertEqual(custom_field_values['integer'], 456) - self.assertEqual(custom_field_values['boolean'], False) - self.assertEqual(custom_field_values['date'], date(2020, 1, 2)) - self.assertEqual(custom_field_values['url'], 'http://example.com/2') - self.assertEqual(custom_field_values['select'].value, 'Choice B') + site2 = Site.objects.get(name='Site 2') + self.assertEqual(len(site2.custom_field_data), 6) + self.assertEqual(site2.custom_field_data['text'], 'DEF') + self.assertEqual(site2.custom_field_data['integer'], 456) + self.assertEqual(site2.custom_field_data['boolean'], False) + self.assertEqual(site2.custom_field_data['date'], date(2020, 1, 2)) + self.assertEqual(site2.custom_field_data['url'], 'http://example.com/2') + self.assertEqual(site2.custom_field_data['select'].value, 'Choice B') # No CustomFieldValues should be created for site 3 - obj_type = ContentType.objects.get_for_model(Site) site3 = Site.objects.get(name='Site 3') self.assertEqual(site3.custom_field_data, {}) From d9e5adc03270eb225043267b5dd8fcc03168d2b7 Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Tue, 25 Aug 2020 10:42:22 -0400 Subject: [PATCH 04/19] Update serializer to access custom_field_data directly --- netbox/extras/api/customfields.py | 36 +----------------------- netbox/extras/tests/test_customfields.py | 16 +++++------ 2 files changed, 8 insertions(+), 44 deletions(-) diff --git a/netbox/extras/api/customfields.py b/netbox/extras/api/customfields.py index df00d0c1d..536b65439 100644 --- a/netbox/extras/api/customfields.py +++ b/netbox/extras/api/customfields.py @@ -2,7 +2,6 @@ from datetime import datetime from django.contrib.contenttypes.models import ContentType from django.core.exceptions import ObjectDoesNotExist -from django.db import transaction from rest_framework import serializers from rest_framework.exceptions import ValidationError from rest_framework.fields import CreateOnlyDefault @@ -134,6 +133,7 @@ class CustomFieldModelSerializer(ValidatedModelSerializer): Extends ModelSerializer to render any CustomFields and their values associated with an object. """ custom_fields = CustomFieldsSerializer( + source='custom_field_data', required=False, default=CreateOnlyDefault(CustomFieldDefaultValues()) ) @@ -163,40 +163,6 @@ class CustomFieldModelSerializer(ValidatedModelSerializer): else: instance.custom_fields[field.name] = value - def _save_custom_fields(self, instance, custom_fields): - for field_name, value in custom_fields.items(): - instance.custom_field_data[field_name] = value - - def create(self, validated_data): - - with transaction.atomic(): - - instance = super().create(validated_data) - - # Save custom fields - custom_fields = validated_data.get('custom_fields') - if custom_fields is not None: - self._save_custom_fields(instance, custom_fields) - instance.custom_fields = custom_fields - - return instance - - def update(self, instance, validated_data): - - with transaction.atomic(): - - custom_fields = validated_data.get('custom_fields') - instance._cf = custom_fields - - instance = super().update(instance, validated_data) - - # Save custom fields - if custom_fields is not None: - self._save_custom_fields(instance, custom_fields) - instance.custom_fields = custom_fields - - return instance - class CustomFieldChoiceSerializer(serializers.ModelSerializer): """ diff --git a/netbox/extras/tests/test_customfields.py b/netbox/extras/tests/test_customfields.py index 71254ac05..16785854f 100644 --- a/netbox/extras/tests/test_customfields.py +++ b/netbox/extras/tests/test_customfields.py @@ -1,5 +1,3 @@ -from datetime import date - from django.contrib.contenttypes.models import ContentType from django.urls import reverse from rest_framework import status @@ -30,7 +28,7 @@ class CustomFieldTest(TestCase): {'field_type': CustomFieldTypeChoices.TYPE_INTEGER, 'field_value': 42, 'empty_value': None}, {'field_type': CustomFieldTypeChoices.TYPE_BOOLEAN, 'field_value': True, 'empty_value': None}, {'field_type': CustomFieldTypeChoices.TYPE_BOOLEAN, 'field_value': False, 'empty_value': None}, - {'field_type': CustomFieldTypeChoices.TYPE_DATE, 'field_value': date(2016, 6, 23), 'empty_value': None}, + {'field_type': CustomFieldTypeChoices.TYPE_DATE, 'field_value': '2016-06-23', 'empty_value': None}, {'field_type': CustomFieldTypeChoices.TYPE_URL, 'field_value': 'http://example.com/', 'empty_value': ''}, ) @@ -239,7 +237,7 @@ class CustomFieldAPITest(APITestCase): self.assertEqual(site.custom_field_data['boolean_field'], self.cf_boolean.default) self.assertEqual(str(site.custom_field_data['date_field']), self.cf_date.default) self.assertEqual(site.custom_field_data['url_field'], self.cf_url.default) - self.assertEqual(site.custom_field_data['choice_field'].pk, self.cf_select_choice1.pk) + self.assertEqual(site.custom_field_data['choice_field'], self.cf_select_choice1.pk) def test_create_single_object_with_values(self): """ @@ -280,7 +278,7 @@ class CustomFieldAPITest(APITestCase): self.assertEqual(site.custom_field_data['boolean_field'], data_cf['boolean_field']) self.assertEqual(str(site.custom_field_data['date_field']), data_cf['date_field']) self.assertEqual(site.custom_field_data['url_field'], data_cf['url_field']) - self.assertEqual(site.custom_field_data['choice_field'].pk, data_cf['choice_field']) + self.assertEqual(site.custom_field_data['choice_field'], data_cf['choice_field']) def test_create_multiple_objects_with_defaults(self): """ @@ -326,7 +324,7 @@ class CustomFieldAPITest(APITestCase): self.assertEqual(site.custom_field_data['boolean_field'], self.cf_boolean.default) self.assertEqual(str(site.custom_field_data['date_field']), self.cf_date.default) self.assertEqual(site.custom_field_data['url_field'], self.cf_url.default) - self.assertEqual(site.custom_field_data['choice_field'].pk, self.cf_select_choice1.pk) + self.assertEqual(site.custom_field_data['choice_field'], self.cf_select_choice1.pk) def test_create_multiple_objects_with_values(self): """ @@ -382,7 +380,7 @@ class CustomFieldAPITest(APITestCase): self.assertEqual(site.custom_field_data['boolean_field'], custom_field_data['boolean_field']) self.assertEqual(str(site.custom_field_data['date_field']), custom_field_data['date_field']) self.assertEqual(site.custom_field_data['url_field'], custom_field_data['url_field']) - self.assertEqual(site.custom_field_data['choice_field'].pk, custom_field_data['choice_field']) + self.assertEqual(site.custom_field_data['choice_field'], custom_field_data['choice_field']) def test_update_single_object_with_values(self): """ @@ -502,7 +500,7 @@ class CustomFieldImportTest(TestCase): self.assertEqual(site1.custom_field_data['text'], 'ABC') self.assertEqual(site1.custom_field_data['integer'], 123) self.assertEqual(site1.custom_field_data['boolean'], True) - self.assertEqual(site1.custom_field_data['date'], date(2020, 1, 1)) + self.assertEqual(site1.custom_field_data['date'], '2020-01-01') self.assertEqual(site1.custom_field_data['url'], 'http://example.com/1') self.assertEqual(site1.custom_field_data['select'].value, 'Choice A') @@ -512,7 +510,7 @@ class CustomFieldImportTest(TestCase): self.assertEqual(site2.custom_field_data['text'], 'DEF') self.assertEqual(site2.custom_field_data['integer'], 456) self.assertEqual(site2.custom_field_data['boolean'], False) - self.assertEqual(site2.custom_field_data['date'], date(2020, 1, 2)) + self.assertEqual(site2.custom_field_data['date'], '2020-01-02') self.assertEqual(site2.custom_field_data['url'], 'http://example.com/2') self.assertEqual(site2.custom_field_data['select'].value, 'Choice B') From f7b8d6ede5d5c082b4db1dc259f10157ea1a17b4 Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Tue, 25 Aug 2020 13:24:46 -0400 Subject: [PATCH 05/19] Add choices ArrayField to CustomField; drop CustomFieldChoice --- netbox/extras/admin.py | 8 +- netbox/extras/api/customfields.py | 55 ++---------- netbox/extras/api/urls.py | 3 - netbox/extras/api/views.py | 44 +--------- .../0050_customfield_add_choices.py | 35 ++++++++ ...values.py => 0051_migrate_customfields.py} | 33 +++++-- ...ete_customfieldchoice_customfieldvalue.py} | 7 +- netbox/extras/models/__init__.py | 3 +- netbox/extras/models/customfields.py | 75 ++++++---------- netbox/extras/tests/test_customfields.py | 86 +++++-------------- 10 files changed, 127 insertions(+), 222 deletions(-) create mode 100644 netbox/extras/migrations/0050_customfield_add_choices.py rename netbox/extras/migrations/{0050_migrate_customfieldvalues.py => 0051_migrate_customfields.py} (57%) rename netbox/extras/migrations/{0051_delete_customfieldvalue.py => 0052_delete_customfieldchoice_customfieldvalue.py} (61%) diff --git a/netbox/extras/admin.py b/netbox/extras/admin.py index 5c95025cd..4bd738160 100644 --- a/netbox/extras/admin.py +++ b/netbox/extras/admin.py @@ -2,7 +2,7 @@ from django import forms from django.contrib import admin from utilities.forms import LaxURLField -from .models import CustomField, CustomFieldChoice, CustomLink, ExportTemplate, JobResult, Webhook +from .models import CustomField, CustomLink, ExportTemplate, JobResult, Webhook def order_content_types(field): @@ -81,14 +81,8 @@ class CustomFieldForm(forms.ModelForm): order_content_types(self.fields['obj_type']) -class CustomFieldChoiceAdmin(admin.TabularInline): - model = CustomFieldChoice - extra = 5 - - @admin.register(CustomField) class CustomFieldAdmin(admin.ModelAdmin): - inlines = [CustomFieldChoiceAdmin] list_display = [ 'name', 'models', 'type', 'required', 'filter_logic', 'default', 'weight', 'description', ] diff --git a/netbox/extras/api/customfields.py b/netbox/extras/api/customfields.py index 536b65439..a053642db 100644 --- a/netbox/extras/api/customfields.py +++ b/netbox/extras/api/customfields.py @@ -7,7 +7,7 @@ from rest_framework.exceptions import ValidationError from rest_framework.fields import CreateOnlyDefault from extras.choices import * -from extras.models import CustomField, CustomFieldChoice +from extras.models import CustomField from utilities.api import ValidatedModelSerializer @@ -37,12 +37,6 @@ class CustomFieldDefaultValues: elif field.type == CustomFieldTypeChoices.TYPE_BOOLEAN: # TODO: Fix default value assignment for boolean custom fields field_value = False if field.default.lower() == 'false' else bool(field.default) - elif field.type == CustomFieldTypeChoices.TYPE_SELECT: - try: - field_value = field.choices.get(value=field.default).pk - except ObjectDoesNotExist: - # Invalid default value - field_value = None else: field_value = field.default value[field.name] = field_value @@ -69,9 +63,7 @@ class CustomFieldsSerializer(serializers.BaseSerializer): try: cf = custom_fields[field_name] except KeyError: - raise ValidationError( - "Invalid custom field for {} objects: {}".format(content_type, field_name) - ) + raise ValidationError(f"Invalid custom field for {content_type} objects: {field_name}") # Data validation if value not in [None, '']: @@ -81,15 +73,11 @@ class CustomFieldsSerializer(serializers.BaseSerializer): try: int(value) except ValueError: - raise ValidationError( - "Invalid value for integer field {}: {}".format(field_name, value) - ) + raise ValidationError(f"Invalid value for integer field {field_name}: {value}") # Validate boolean if cf.type == CustomFieldTypeChoices.TYPE_BOOLEAN and value not in [True, False, 1, 0]: - raise ValidationError( - "Invalid value for boolean field {}: {}".format(field_name, value) - ) + raise ValidationError(f"Invalid value for boolean field {field_name}: {value}") # Validate date if cf.type == CustomFieldTypeChoices.TYPE_DATE: @@ -97,25 +85,16 @@ class CustomFieldsSerializer(serializers.BaseSerializer): datetime.strptime(value, '%Y-%m-%d') except ValueError: raise ValidationError( - "Invalid date for field {}: {}. (Required format is YYYY-MM-DD.)".format(field_name, value) + f"Invalid date for field {field_name}: {value}. (Required format is YYYY-MM-DD.)" ) # Validate selected choice if cf.type == CustomFieldTypeChoices.TYPE_SELECT: - try: - value = int(value) - except ValueError: - raise ValidationError( - "{}: Choice selections must be passed as integers.".format(field_name) - ) - valid_choices = [c.pk for c in cf.choices.all()] - if value not in valid_choices: - raise ValidationError( - "Invalid choice for field {}: {}".format(field_name, value) - ) + if value not in cf.choices: + raise ValidationError(f"Invalid choice for field {field_name}: {value}") elif cf.required: - raise ValidationError("Required field {} cannot be empty.".format(field_name)) + raise ValidationError(f"Required field {field_name} cannot be empty.") # Check for missing required fields missing_fields = [] @@ -157,20 +136,4 @@ class CustomFieldModelSerializer(ValidatedModelSerializer): def _populate_custom_fields(self, instance, custom_fields): instance.custom_fields = {} for field in custom_fields: - value = instance.cf.get(field.name) - if field.type == CustomFieldTypeChoices.TYPE_SELECT and value is not None: - instance.custom_fields[field.name] = CustomFieldChoiceSerializer(value).data - else: - instance.custom_fields[field.name] = value - - -class CustomFieldChoiceSerializer(serializers.ModelSerializer): - """ - Imitate utilities.api.ChoiceFieldSerializer - """ - value = serializers.IntegerField(source='pk') - label = serializers.CharField(source='value') - - class Meta: - model = CustomFieldChoice - fields = ['value', 'label'] + instance.custom_fields[field.name] = instance.cf.get(field.name) diff --git a/netbox/extras/api/urls.py b/netbox/extras/api/urls.py index 70e5bc9da..20d0f8d17 100644 --- a/netbox/extras/api/urls.py +++ b/netbox/extras/api/urls.py @@ -5,9 +5,6 @@ from . import views router = OrderedDefaultRouter() router.APIRootView = views.ExtrasRootView -# Custom field choices -router.register('_custom_field_choices', views.CustomFieldChoicesViewSet, basename='custom-field-choice') - # Export templates router.register('export-templates', views.ExportTemplateViewSet) diff --git a/netbox/extras/api/views.py b/netbox/extras/api/views.py index 5be8276b6..278caeda7 100644 --- a/netbox/extras/api/views.py +++ b/netbox/extras/api/views.py @@ -14,9 +14,7 @@ from rq import Worker from extras import filters from extras.choices import JobResultStatusChoices -from extras.models import ( - ConfigContext, CustomFieldChoice, ExportTemplate, ImageAttachment, ObjectChange, JobResult, Tag, -) +from extras.models import ConfigContext, ExportTemplate, ImageAttachment, ObjectChange, JobResult, Tag from extras.reports import get_report, get_reports, run_report from extras.scripts import get_script, get_scripts, run_script from utilities.api import IsAuthenticatedOrLoginNotRequired, ModelViewSet @@ -34,36 +32,6 @@ class ExtrasRootView(APIRootView): return 'Extras' -# -# Custom field choices -# - -class CustomFieldChoicesViewSet(ViewSet): - """ - """ - permission_classes = [IsAuthenticatedOrLoginNotRequired] - - def __init__(self, *args, **kwargs): - super(CustomFieldChoicesViewSet, self).__init__(*args, **kwargs) - - self._fields = OrderedDict() - - for cfc in CustomFieldChoice.objects.all(): - self._fields.setdefault(cfc.field.name, {}) - self._fields[cfc.field.name][cfc.value] = cfc.pk - - def list(self, request): - return Response(self._fields) - - def retrieve(self, request, pk): - if pk not in self._fields: - raise Http404 - return Response(self._fields[pk]) - - def get_view_name(self): - return "Custom Field choices" - - # # Custom fields # @@ -77,19 +45,11 @@ class CustomFieldModelViewSet(ModelViewSet): # Gather all custom fields for the model content_type = ContentType.objects.get_for_model(self.queryset.model) - custom_fields = content_type.custom_fields.prefetch_related('choices') - - # Cache all relevant CustomFieldChoices. This saves us from having to do a lookup per select field per object. - custom_field_choices = {} - for field in custom_fields: - for cfc in field.choices.all(): - custom_field_choices[cfc.id] = cfc.value - custom_field_choices = custom_field_choices + custom_fields = content_type.custom_fields.all() context = super().get_serializer_context() context.update({ 'custom_fields': custom_fields, - 'custom_field_choices': custom_field_choices, }) return context diff --git a/netbox/extras/migrations/0050_customfield_add_choices.py b/netbox/extras/migrations/0050_customfield_add_choices.py new file mode 100644 index 000000000..1ae63a2c3 --- /dev/null +++ b/netbox/extras/migrations/0050_customfield_add_choices.py @@ -0,0 +1,35 @@ +import django.contrib.postgres.fields +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('extras', '0049_remove_graph'), + ] + + operations = [ + # Rename reverse relation on CustomFieldChoice + migrations.AlterField( + model_name='customfieldchoice', + name='field', + field=models.ForeignKey( + limit_choices_to={'type': 'select'}, + on_delete=django.db.models.deletion.CASCADE, + related_name='_choices', + to='extras.customfield' + ), + ), + # Add choices field to CustomField + migrations.AddField( + model_name='customfield', + name='choices', + field=django.contrib.postgres.fields.ArrayField( + base_field=models.CharField(max_length=100), + blank=True, + null=True, + size=None + ), + ), + ] diff --git a/netbox/extras/migrations/0050_migrate_customfieldvalues.py b/netbox/extras/migrations/0051_migrate_customfields.py similarity index 57% rename from netbox/extras/migrations/0050_migrate_customfieldvalues.py rename to netbox/extras/migrations/0051_migrate_customfields.py index 9b2402267..017201259 100644 --- a/netbox/extras/migrations/0050_migrate_customfieldvalues.py +++ b/netbox/extras/migrations/0051_migrate_customfields.py @@ -3,18 +3,38 @@ from django.db import migrations from extras.choices import CustomFieldTypeChoices -def deserialize_value(field_type, value): +def deserialize_value(field, value): """ Convert serialized values to JSON equivalents. """ - if field_type in (CustomFieldTypeChoices.TYPE_INTEGER, CustomFieldTypeChoices.TYPE_SELECT): + if field.type in (CustomFieldTypeChoices.TYPE_INTEGER): return int(value) - if field_type == CustomFieldTypeChoices.TYPE_BOOLEAN: + if field.type == CustomFieldTypeChoices.TYPE_BOOLEAN: return bool(int(value)) + if field.type == CustomFieldTypeChoices.TYPE_SELECT: + return field._choices.get(pk=int(value)).value return value +def migrate_customfieldchoices(apps, schema_editor): + """ + Collect all CustomFieldChoices for each applicable CustomField, and save them locally as an array on + the CustomField instance. + """ + CustomField = apps.get_model('extras', 'CustomField') + CustomFieldChoice = apps.get_model('extras', 'CustomFieldChoice') + + for cf in CustomField.objects.filter(type='select'): + cf.choices = [ + cfc.value for cfc in CustomFieldChoice.objects.filter(field=cf).order_by('weight', 'value') + ] + cf.save() + + def migrate_customfieldvalues(apps, schema_editor): + """ + Copy data from CustomFieldValues into the custom_field_data JSON field on each model instance. + """ CustomFieldValue = apps.get_model('extras', 'CustomFieldValue') for cfv in CustomFieldValue.objects.prefetch_related('field').exclude(serialized_value=''): @@ -24,7 +44,7 @@ def migrate_customfieldvalues(apps, schema_editor): # TODO: This can be done more efficiently once .update() is supported for JSON fields cf_data = model.objects.filter(pk=cfv.obj_id).values('custom_field_data').first() try: - cf_data['custom_field_data'][cfv.field.name] = deserialize_value(cfv.field.type, cfv.serialized_value) + cf_data['custom_field_data'][cfv.field.name] = deserialize_value(cfv.field, cfv.serialized_value) except ValueError as e: print(f'{cfv.field.name} ({cfv.field.type}): {cfv.serialized_value} ({cfv.pk})') raise e @@ -36,7 +56,7 @@ class Migration(migrations.Migration): dependencies = [ ('circuits', '0020_custom_field_data'), ('dcim', '0115_custom_field_data'), - ('extras', '0049_remove_graph'), + ('extras', '0050_customfield_add_choices'), ('ipam', '0038_custom_field_data'), ('secrets', '0010_custom_field_data'), ('tenancy', '0010_custom_field_data'), @@ -44,6 +64,9 @@ class Migration(migrations.Migration): ] operations = [ + migrations.RunPython( + code=migrate_customfieldchoices + ), migrations.RunPython( code=migrate_customfieldvalues ), diff --git a/netbox/extras/migrations/0051_delete_customfieldvalue.py b/netbox/extras/migrations/0052_delete_customfieldchoice_customfieldvalue.py similarity index 61% rename from netbox/extras/migrations/0051_delete_customfieldvalue.py rename to netbox/extras/migrations/0052_delete_customfieldchoice_customfieldvalue.py index 3369289a0..8b5e2ba45 100644 --- a/netbox/extras/migrations/0051_delete_customfieldvalue.py +++ b/netbox/extras/migrations/0052_delete_customfieldchoice_customfieldvalue.py @@ -1,15 +1,16 @@ -# Generated by Django 3.1 on 2020-08-21 19:52 - from django.db import migrations class Migration(migrations.Migration): dependencies = [ - ('extras', '0050_migrate_customfieldvalues'), + ('extras', '0051_migrate_customfields'), ] operations = [ + migrations.DeleteModel( + name='CustomFieldChoice', + ), migrations.DeleteModel( name='CustomFieldValue', ), diff --git a/netbox/extras/models/__init__.py b/netbox/extras/models/__init__.py index a4178b911..c6191bbd2 100644 --- a/netbox/extras/models/__init__.py +++ b/netbox/extras/models/__init__.py @@ -1,5 +1,5 @@ from .change_logging import ChangeLoggedModel, ObjectChange -from .customfields import CustomField, CustomFieldChoice, CustomFieldModel +from .customfields import CustomField, CustomFieldModel from .models import ( ConfigContext, ConfigContextModel, CustomLink, ExportTemplate, ImageAttachment, JobResult, Report, Script, Webhook, @@ -11,7 +11,6 @@ __all__ = ( 'ConfigContext', 'ConfigContextModel', 'CustomField', - 'CustomFieldChoice', 'CustomFieldModel', 'CustomLink', 'ExportTemplate', diff --git a/netbox/extras/models/customfields.py b/netbox/extras/models/customfields.py index 166ef5708..e922a2f77 100644 --- a/netbox/extras/models/customfields.py +++ b/netbox/extras/models/customfields.py @@ -3,6 +3,7 @@ from datetime import date from django import forms from django.contrib.contenttypes.models import ContentType +from django.contrib.postgres.fields import ArrayField from django.core.validators import ValidationError from django.db import models @@ -11,11 +12,10 @@ from extras.choices import * from extras.utils import FeatureQuery -# -# Custom fields -# - class CustomFieldModel(models.Model): + """ + Abstract class for any model which may have custom fields associated with it. + """ custom_field_data = models.JSONField( blank=True, default=dict @@ -104,6 +104,12 @@ class CustomField(models.Model): default=100, help_text='Fields with higher weights appear lower in a form.' ) + choices = ArrayField( + base_field=models.CharField(max_length=100), + blank=True, + null=True, + help_text='Comma-separated list of available choices (for selection fields)' + ) objects = CustomFieldManager() @@ -113,6 +119,19 @@ class CustomField(models.Model): def __str__(self): return self.label or self.name.replace('_', ' ').capitalize() + def clean(self): + # Choices can be set only on selection fields + if self.choices and self.type != CustomFieldTypeChoices.TYPE_SELECT: + raise ValidationError({ + 'choices': "Choices may be set only for selection-type custom fields." + }) + + # A selection field's default (if any) must be present in its available choices + if self.type == CustomFieldTypeChoices.TYPE_SELECT and self.default and self.default not in self.choices: + raise ValidationError({ + 'default': f"The specified default value ({self.default}) is not listed as an available choice." + }) + def serialize_value(self, value): """ Serialize the given value to a string suitable for storage as a CustomFieldValue @@ -187,16 +206,14 @@ class CustomField(models.Model): # Select elif self.type == CustomFieldTypeChoices.TYPE_SELECT: - choices = [(cfc.pk, cfc.value) for cfc in self.choices.all()] + choices = [(c, c) for c in self.choices] if not required: choices = add_blank_choice(choices) - # Set the initial value to the PK of the default choice, if any - if set_initial: - default_choice = self.choices.filter(value=self.default).first() - if default_choice: - initial = default_choice.pk + # Set the initial value to the first available choice (if any) + if set_initial and self.choices: + initial = self.choices[0] field_class = CSVChoiceField if for_csv_import else forms.ChoiceField field = field_class( @@ -217,41 +234,3 @@ class CustomField(models.Model): field.help_text = self.description return field - - -class CustomFieldChoice(models.Model): - field = models.ForeignKey( - to='extras.CustomField', - on_delete=models.CASCADE, - related_name='choices', - limit_choices_to={'type': CustomFieldTypeChoices.TYPE_SELECT} - ) - value = models.CharField( - max_length=100 - ) - weight = models.PositiveSmallIntegerField( - default=100, - help_text='Higher weights appear lower in the list' - ) - - class Meta: - ordering = ['field', 'weight', 'value'] - unique_together = ['field', 'value'] - - def __str__(self): - return self.value - - def clean(self): - if self.field.type != CustomFieldTypeChoices.TYPE_SELECT: - raise ValidationError("Custom field choices can only be assigned to selection fields.") - - def delete(self, *args, **kwargs): - # TODO: Prevent deletion of CustomFieldChoices which are in use? - field_name = f'custom_field_data__{self.field.name}' - for ct in self.field.obj_type.all(): - model = ct.model_class() - for instance in model.objects.filter(**{field_name: self.pk}): - instance.custom_field_data.pop(self.field.name) - instance.save() - - super().delete(*args, **kwargs) diff --git a/netbox/extras/tests/test_customfields.py b/netbox/extras/tests/test_customfields.py index 16785854f..675248a3b 100644 --- a/netbox/extras/tests/test_customfields.py +++ b/netbox/extras/tests/test_customfields.py @@ -5,7 +5,7 @@ from rest_framework import status from dcim.forms import SiteCSVForm from dcim.models import Site from extras.choices import * -from extras.models import CustomField, CustomFieldChoice +from extras.models import CustomField from utilities.testing import APITestCase, TestCase from virtualization.models import VirtualMachine @@ -65,21 +65,19 @@ class CustomFieldTest(TestCase): obj_type = ContentType.objects.get_for_model(Site) # Create a custom field - cf = CustomField(type=CustomFieldTypeChoices.TYPE_SELECT, name='my_field', required=False) + cf = CustomField( + type=CustomFieldTypeChoices.TYPE_SELECT, + name='my_field', + required=False, + choices=['Option A', 'Option B', 'Option C'] + ) cf.save() cf.obj_type.set([obj_type]) cf.save() - # Create some choices for the field - CustomFieldChoice.objects.bulk_create([ - CustomFieldChoice(field=cf, value='Option A'), - CustomFieldChoice(field=cf, value='Option B'), - CustomFieldChoice(field=cf, value='Option C'), - ]) - # Assign a value to the first Site site = Site.objects.first() - site.custom_field_data[cf.name] = cf.choices.first().pk + site.custom_field_data[cf.name] = 'Option A' site.save() # Retrieve the stored value @@ -141,18 +139,10 @@ class CustomFieldAPITest(APITestCase): cls.cf_url.obj_type.set([content_type]) # Select custom field - cls.cf_select = CustomField(type=CustomFieldTypeChoices.TYPE_SELECT, name='choice_field') + cls.cf_select = CustomField(type=CustomFieldTypeChoices.TYPE_SELECT, name='choice_field', choices=['Foo', 'Bar', 'Baz']) + cls.cf_select.default = 'Foo' cls.cf_select.save() cls.cf_select.obj_type.set([content_type]) - cls.cf_select_choice1 = CustomFieldChoice(field=cls.cf_select, value='Foo') - cls.cf_select_choice1.save() - cls.cf_select_choice2 = CustomFieldChoice(field=cls.cf_select, value='Bar') - cls.cf_select_choice2.save() - cls.cf_select_choice3 = CustomFieldChoice(field=cls.cf_select, value='Baz') - cls.cf_select_choice3.save() - - cls.cf_select.default = cls.cf_select_choice1.value - cls.cf_select.save() # Create some sites cls.sites = ( @@ -168,7 +158,7 @@ class CustomFieldAPITest(APITestCase): cls.cf_boolean.name: True, cls.cf_date.name: '2020-01-02', cls.cf_url.name: 'http://example.com/2', - cls.cf_select.name: cls.cf_select_choice2.pk, + cls.cf_select.name: 'Bar', } cls.sites[1].save() @@ -205,7 +195,7 @@ class CustomFieldAPITest(APITestCase): self.assertEqual(response.data['custom_fields']['boolean_field'], site2_cfvs['boolean_field']) self.assertEqual(response.data['custom_fields']['date_field'], site2_cfvs['date_field']) self.assertEqual(response.data['custom_fields']['url_field'], site2_cfvs['url_field']) - self.assertEqual(response.data['custom_fields']['choice_field']['label'], self.cf_select_choice2.value) + self.assertEqual(response.data['custom_fields']['choice_field'], site2_cfvs['choice_field']) def test_create_single_object_with_defaults(self): """ @@ -228,7 +218,7 @@ class CustomFieldAPITest(APITestCase): self.assertEqual(response_cf['boolean_field'], self.cf_boolean.default) self.assertEqual(response_cf['date_field'], self.cf_date.default) self.assertEqual(response_cf['url_field'], self.cf_url.default) - self.assertEqual(response_cf['choice_field'], self.cf_select_choice1.pk) + self.assertEqual(response_cf['choice_field'], self.cf_select.default) # Validate database data site = Site.objects.get(pk=response.data['id']) @@ -237,7 +227,7 @@ class CustomFieldAPITest(APITestCase): self.assertEqual(site.custom_field_data['boolean_field'], self.cf_boolean.default) self.assertEqual(str(site.custom_field_data['date_field']), self.cf_date.default) self.assertEqual(site.custom_field_data['url_field'], self.cf_url.default) - self.assertEqual(site.custom_field_data['choice_field'], self.cf_select_choice1.pk) + self.assertEqual(site.custom_field_data['choice_field'], self.cf_select.default) def test_create_single_object_with_values(self): """ @@ -252,7 +242,7 @@ class CustomFieldAPITest(APITestCase): 'boolean_field': True, 'date_field': '2020-01-02', 'url_field': 'http://example.com/2', - 'choice_field': self.cf_select_choice2.pk, + 'choice_field': 'Bar', }, } url = reverse('dcim-api:site-list') @@ -315,7 +305,7 @@ class CustomFieldAPITest(APITestCase): self.assertEqual(response_cf['boolean_field'], self.cf_boolean.default) self.assertEqual(response_cf['date_field'], self.cf_date.default) self.assertEqual(response_cf['url_field'], self.cf_url.default) - self.assertEqual(response_cf['choice_field'], self.cf_select_choice1.pk) + self.assertEqual(response_cf['choice_field'], self.cf_select.default) # Validate database data site = Site.objects.get(pk=response.data[i]['id']) @@ -324,7 +314,7 @@ class CustomFieldAPITest(APITestCase): self.assertEqual(site.custom_field_data['boolean_field'], self.cf_boolean.default) self.assertEqual(str(site.custom_field_data['date_field']), self.cf_date.default) self.assertEqual(site.custom_field_data['url_field'], self.cf_url.default) - self.assertEqual(site.custom_field_data['choice_field'], self.cf_select_choice1.pk) + self.assertEqual(site.custom_field_data['choice_field'], self.cf_select.default) def test_create_multiple_objects_with_values(self): """ @@ -336,7 +326,7 @@ class CustomFieldAPITest(APITestCase): 'boolean_field': True, 'date_field': '2020-01-02', 'url_field': 'http://example.com/2', - 'choice_field': self.cf_select_choice2.pk, + 'choice_field': 'Bar', } data = ( { @@ -410,7 +400,7 @@ class CustomFieldAPITest(APITestCase): # self.assertEqual(response_cf['boolean_field'], site2_original_cfvs['boolean_field']) # self.assertEqual(response_cf['date_field'], site2_original_cfvs['date_field']) # self.assertEqual(response_cf['url_field'], site2_original_cfvs['url_field']) - # self.assertEqual(response_cf['choice_field']['label'], site2_original_cfvs['choice_field'].value) + # self.assertEqual(response_cf['choice_field'], site2_original_cfvs['choice_field'].value) # Validate database data site.refresh_from_db() @@ -422,36 +412,6 @@ class CustomFieldAPITest(APITestCase): self.assertEqual(site.custom_field_data['choice_field'], original_cfvs['choice_field']) -class CustomFieldChoiceAPITest(APITestCase): - def setUp(self): - super().setUp() - - vm_content_type = ContentType.objects.get_for_model(VirtualMachine) - - self.cf_1 = CustomField.objects.create(name="cf_1", type=CustomFieldTypeChoices.TYPE_SELECT) - self.cf_2 = CustomField.objects.create(name="cf_2", type=CustomFieldTypeChoices.TYPE_SELECT) - - self.cf_choice_1 = CustomFieldChoice.objects.create(field=self.cf_1, value="cf_field_1", weight=100) - self.cf_choice_2 = CustomFieldChoice.objects.create(field=self.cf_1, value="cf_field_2", weight=50) - self.cf_choice_3 = CustomFieldChoice.objects.create(field=self.cf_2, value="cf_field_3", weight=10) - - def test_list_cfc(self): - url = reverse('extras-api:custom-field-choice-list') - response = self.client.get(url, **self.header) - - self.assertEqual(len(response.data), 2) - self.assertEqual(len(response.data[self.cf_1.name]), 2) - self.assertEqual(len(response.data[self.cf_2.name]), 1) - - self.assertTrue(self.cf_choice_1.value in response.data[self.cf_1.name]) - self.assertTrue(self.cf_choice_2.value in response.data[self.cf_1.name]) - self.assertTrue(self.cf_choice_3.value in response.data[self.cf_2.name]) - - self.assertEqual(self.cf_choice_1.pk, response.data[self.cf_1.name][self.cf_choice_1.value]) - self.assertEqual(self.cf_choice_2.pk, response.data[self.cf_1.name][self.cf_choice_2.value]) - self.assertEqual(self.cf_choice_3.pk, response.data[self.cf_2.name][self.cf_choice_3.value]) - - class CustomFieldImportTest(TestCase): user_permissions = ( 'dcim.view_site', @@ -467,18 +427,12 @@ class CustomFieldImportTest(TestCase): CustomField(name='boolean', type=CustomFieldTypeChoices.TYPE_BOOLEAN), CustomField(name='date', type=CustomFieldTypeChoices.TYPE_DATE), CustomField(name='url', type=CustomFieldTypeChoices.TYPE_URL), - CustomField(name='select', type=CustomFieldTypeChoices.TYPE_SELECT), + CustomField(name='select', type=CustomFieldTypeChoices.TYPE_SELECT, choices=['Choice A', 'Choice B', 'Choice C']), ) for cf in custom_fields: cf.save() cf.obj_type.set([ContentType.objects.get_for_model(Site)]) - CustomFieldChoice.objects.bulk_create(( - CustomFieldChoice(field=custom_fields[5], value='Choice A'), - CustomFieldChoice(field=custom_fields[5], value='Choice B'), - CustomFieldChoice(field=custom_fields[5], value='Choice C'), - )) - def test_import(self): """ Import a Site in CSV format, including a value for each CustomField. From fb8904af5476a2624950a708e9bd660a3c5ecc68 Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Tue, 25 Aug 2020 13:42:47 -0400 Subject: [PATCH 06/19] Remove unused attributes, methods --- netbox/extras/forms.py | 13 +-------- netbox/extras/models/customfields.py | 42 +--------------------------- 2 files changed, 2 insertions(+), 53 deletions(-) diff --git a/netbox/extras/forms.py b/netbox/extras/forms.py index 96290ef0a..2cdd5d2ed 100644 --- a/netbox/extras/forms.py +++ b/netbox/extras/forms.py @@ -25,34 +25,23 @@ class CustomFieldModelForm(forms.ModelForm): self.obj_type = ContentType.objects.get_for_model(self._meta.model) self.custom_fields = [] - self.custom_field_values = {} super().__init__(*args, **kwargs) - if self.instance._cf is None: - self.instance._cf = {} - self._append_customfield_fields() def _append_customfield_fields(self): """ Append form fields for all CustomFields assigned to this model. """ - # Retrieve initial CustomField values for the instance - if self.instance.pk: - self.custom_field_values = self.instance.custom_field_data - # Append form fields; assign initial values if modifying and existing object for cf in CustomField.objects.filter(obj_type=self.obj_type): field_name = 'cf_{}'.format(cf.name) if self.instance.pk: self.fields[field_name] = cf.to_form_field(set_initial=False) - value = self.custom_field_values.get(cf.name) - self.fields[field_name].initial = value - self.instance._cf[cf.name] = value + self.fields[field_name].initial = self.instance.custom_field_data.get(cf.name) else: self.fields[field_name] = cf.to_form_field() - self.instance._cf[cf.name] = self.fields[field_name].initial # Annotate the field in the list of CustomField form fields self.custom_fields.append(field_name) diff --git a/netbox/extras/models/customfields.py b/netbox/extras/models/customfields.py index e922a2f77..325853d6d 100644 --- a/netbox/extras/models/customfields.py +++ b/netbox/extras/models/customfields.py @@ -24,10 +24,6 @@ class CustomFieldModel(models.Model): class Meta: abstract = True - def __init__(self, *args, custom_fields=None, **kwargs): - self._cf = custom_fields - super().__init__(*args, **kwargs) - @property def cf(self): """ @@ -132,42 +128,6 @@ class CustomField(models.Model): 'default': f"The specified default value ({self.default}) is not listed as an available choice." }) - def serialize_value(self, value): - """ - Serialize the given value to a string suitable for storage as a CustomFieldValue - """ - if value is None: - return '' - if self.type == CustomFieldTypeChoices.TYPE_BOOLEAN: - return str(int(bool(value))) - if self.type == CustomFieldTypeChoices.TYPE_DATE: - # Could be date/datetime object or string - try: - return value.strftime('%Y-%m-%d') - except AttributeError: - return value - if self.type == CustomFieldTypeChoices.TYPE_SELECT: - # Could be ModelChoiceField or TypedChoiceField - return str(value.id) if hasattr(value, 'id') else str(value) - return value - - def deserialize_value(self, serialized_value): - """ - Convert a string into the object it represents depending on the type of field - """ - if serialized_value == '': - return None - if self.type == CustomFieldTypeChoices.TYPE_INTEGER: - return int(serialized_value) - if self.type == CustomFieldTypeChoices.TYPE_BOOLEAN: - return bool(int(serialized_value)) - if self.type == CustomFieldTypeChoices.TYPE_DATE: - # Read date as YYYY-MM-DD - return date(*[int(n) for n in serialized_value.split('-')]) - if self.type == CustomFieldTypeChoices.TYPE_SELECT: - return self.choices.get(pk=int(serialized_value)) - return serialized_value - def to_form_field(self, set_initial=True, enforce_required=True, for_csv_import=False): """ Return a form field suitable for setting a CustomField's value for an object. @@ -229,7 +189,7 @@ class CustomField(models.Model): field = forms.CharField(max_length=255, required=required, initial=initial) field.model = self - field.label = self.label if self.label else self.name.replace('_', ' ').capitalize() + field.label = str(self) if self.description: field.help_text = self.description From 5b3de8defe125c87cf0d18aeeddf9c91c4446713 Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Tue, 25 Aug 2020 13:57:18 -0400 Subject: [PATCH 07/19] Use DjangoJSONEncoder for encoding custom field data --- .../circuits/migrations/0020_custom_field_data.py | 7 +++---- netbox/dcim/migrations/0115_custom_field_data.py | 13 ++++++------- netbox/extras/models/customfields.py | 3 ++- netbox/ipam/migrations/0038_custom_field_data.py | 15 +++++++-------- .../secrets/migrations/0010_custom_field_data.py | 5 ++--- .../tenancy/migrations/0010_custom_field_data.py | 5 ++--- .../migrations/0018_custom_field_data.py | 7 +++---- 7 files changed, 25 insertions(+), 30 deletions(-) diff --git a/netbox/circuits/migrations/0020_custom_field_data.py b/netbox/circuits/migrations/0020_custom_field_data.py index c72b937db..97da9962c 100644 --- a/netbox/circuits/migrations/0020_custom_field_data.py +++ b/netbox/circuits/migrations/0020_custom_field_data.py @@ -1,5 +1,4 @@ -# Generated by Django 3.1 on 2020-08-21 18:34 - +import django.core.serializers.json from django.db import migrations, models @@ -13,11 +12,11 @@ class Migration(migrations.Migration): migrations.AddField( model_name='circuit', name='custom_field_data', - field=models.JSONField(blank=True, default=dict), + field=models.JSONField(blank=True, default=dict, encoder=django.core.serializers.json.DjangoJSONEncoder), ), migrations.AddField( model_name='provider', name='custom_field_data', - field=models.JSONField(blank=True, default=dict), + field=models.JSONField(blank=True, default=dict, encoder=django.core.serializers.json.DjangoJSONEncoder), ), ] diff --git a/netbox/dcim/migrations/0115_custom_field_data.py b/netbox/dcim/migrations/0115_custom_field_data.py index 58c320902..e6353c916 100644 --- a/netbox/dcim/migrations/0115_custom_field_data.py +++ b/netbox/dcim/migrations/0115_custom_field_data.py @@ -1,5 +1,4 @@ -# Generated by Django 3.1 on 2020-08-21 18:34 - +import django.core.serializers.json from django.db import migrations, models @@ -13,26 +12,26 @@ class Migration(migrations.Migration): migrations.AddField( model_name='device', name='custom_field_data', - field=models.JSONField(blank=True, default=dict), + field=models.JSONField(blank=True, default=dict, encoder=django.core.serializers.json.DjangoJSONEncoder), ), migrations.AddField( model_name='devicetype', name='custom_field_data', - field=models.JSONField(blank=True, default=dict), + field=models.JSONField(blank=True, default=dict, encoder=django.core.serializers.json.DjangoJSONEncoder), ), migrations.AddField( model_name='powerfeed', name='custom_field_data', - field=models.JSONField(blank=True, default=dict), + field=models.JSONField(blank=True, default=dict, encoder=django.core.serializers.json.DjangoJSONEncoder), ), migrations.AddField( model_name='rack', name='custom_field_data', - field=models.JSONField(blank=True, default=dict), + field=models.JSONField(blank=True, default=dict, encoder=django.core.serializers.json.DjangoJSONEncoder), ), migrations.AddField( model_name='site', name='custom_field_data', - field=models.JSONField(blank=True, default=dict), + field=models.JSONField(blank=True, default=dict, encoder=django.core.serializers.json.DjangoJSONEncoder), ), ] diff --git a/netbox/extras/models/customfields.py b/netbox/extras/models/customfields.py index 325853d6d..422438539 100644 --- a/netbox/extras/models/customfields.py +++ b/netbox/extras/models/customfields.py @@ -1,9 +1,9 @@ from collections import OrderedDict -from datetime import date from django import forms from django.contrib.contenttypes.models import ContentType from django.contrib.postgres.fields import ArrayField +from django.core.serializers.json import DjangoJSONEncoder from django.core.validators import ValidationError from django.db import models @@ -17,6 +17,7 @@ class CustomFieldModel(models.Model): Abstract class for any model which may have custom fields associated with it. """ custom_field_data = models.JSONField( + encoder=DjangoJSONEncoder, blank=True, default=dict ) diff --git a/netbox/ipam/migrations/0038_custom_field_data.py b/netbox/ipam/migrations/0038_custom_field_data.py index c9d66975c..86d51e9b8 100644 --- a/netbox/ipam/migrations/0038_custom_field_data.py +++ b/netbox/ipam/migrations/0038_custom_field_data.py @@ -1,5 +1,4 @@ -# Generated by Django 3.1 on 2020-08-21 18:34 - +import django.core.serializers.json from django.db import migrations, models @@ -13,31 +12,31 @@ class Migration(migrations.Migration): migrations.AddField( model_name='aggregate', name='custom_field_data', - field=models.JSONField(blank=True, default=dict), + field=models.JSONField(blank=True, default=dict, encoder=django.core.serializers.json.DjangoJSONEncoder), ), migrations.AddField( model_name='ipaddress', name='custom_field_data', - field=models.JSONField(blank=True, default=dict), + field=models.JSONField(blank=True, default=dict, encoder=django.core.serializers.json.DjangoJSONEncoder), ), migrations.AddField( model_name='prefix', name='custom_field_data', - field=models.JSONField(blank=True, default=dict), + field=models.JSONField(blank=True, default=dict, encoder=django.core.serializers.json.DjangoJSONEncoder), ), migrations.AddField( model_name='service', name='custom_field_data', - field=models.JSONField(blank=True, default=dict), + field=models.JSONField(blank=True, default=dict, encoder=django.core.serializers.json.DjangoJSONEncoder), ), migrations.AddField( model_name='vlan', name='custom_field_data', - field=models.JSONField(blank=True, default=dict), + field=models.JSONField(blank=True, default=dict, encoder=django.core.serializers.json.DjangoJSONEncoder), ), migrations.AddField( model_name='vrf', name='custom_field_data', - field=models.JSONField(blank=True, default=dict), + field=models.JSONField(blank=True, default=dict, encoder=django.core.serializers.json.DjangoJSONEncoder), ), ] diff --git a/netbox/secrets/migrations/0010_custom_field_data.py b/netbox/secrets/migrations/0010_custom_field_data.py index 33b5f06e1..6d48e7cab 100644 --- a/netbox/secrets/migrations/0010_custom_field_data.py +++ b/netbox/secrets/migrations/0010_custom_field_data.py @@ -1,5 +1,4 @@ -# Generated by Django 3.1 on 2020-08-21 18:34 - +import django.core.serializers.json from django.db import migrations, models @@ -13,6 +12,6 @@ class Migration(migrations.Migration): migrations.AddField( model_name='secret', name='custom_field_data', - field=models.JSONField(blank=True, default=dict), + field=models.JSONField(blank=True, default=dict, encoder=django.core.serializers.json.DjangoJSONEncoder), ), ] diff --git a/netbox/tenancy/migrations/0010_custom_field_data.py b/netbox/tenancy/migrations/0010_custom_field_data.py index 4d05a8272..ec05be0ff 100644 --- a/netbox/tenancy/migrations/0010_custom_field_data.py +++ b/netbox/tenancy/migrations/0010_custom_field_data.py @@ -1,5 +1,4 @@ -# Generated by Django 3.1 on 2020-08-21 18:34 - +import django.core.serializers.json from django.db import migrations, models @@ -13,6 +12,6 @@ class Migration(migrations.Migration): migrations.AddField( model_name='tenant', name='custom_field_data', - field=models.JSONField(blank=True, default=dict), + field=models.JSONField(blank=True, default=dict, encoder=django.core.serializers.json.DjangoJSONEncoder), ), ] diff --git a/netbox/virtualization/migrations/0018_custom_field_data.py b/netbox/virtualization/migrations/0018_custom_field_data.py index 5c129e2ab..9a120406a 100644 --- a/netbox/virtualization/migrations/0018_custom_field_data.py +++ b/netbox/virtualization/migrations/0018_custom_field_data.py @@ -1,5 +1,4 @@ -# Generated by Django 3.1 on 2020-08-21 18:34 - +import django.core.serializers.json from django.db import migrations, models @@ -13,11 +12,11 @@ class Migration(migrations.Migration): migrations.AddField( model_name='cluster', name='custom_field_data', - field=models.JSONField(blank=True, default=dict), + field=models.JSONField(blank=True, default=dict, encoder=django.core.serializers.json.DjangoJSONEncoder), ), migrations.AddField( model_name='virtualmachine', name='custom_field_data', - field=models.JSONField(blank=True, default=dict), + field=models.JSONField(blank=True, default=dict, encoder=django.core.serializers.json.DjangoJSONEncoder), ), ] From 0b7d019c024823f919bb77aa22f2a1bf1e9487b5 Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Tue, 25 Aug 2020 15:14:49 -0400 Subject: [PATCH 08/19] Remove unused CustomChoiceFieldInspector --- netbox/netbox/settings.py | 1 - netbox/utilities/custom_inspectors.py | 42 --------------------------- 2 files changed, 43 deletions(-) diff --git a/netbox/netbox/settings.py b/netbox/netbox/settings.py index d49e41b76..92f39cf44 100644 --- a/netbox/netbox/settings.py +++ b/netbox/netbox/settings.py @@ -486,7 +486,6 @@ SWAGGER_SETTINGS = { 'DEFAULT_FIELD_INSPECTORS': [ 'utilities.custom_inspectors.JSONFieldInspector', 'utilities.custom_inspectors.NullableBooleanFieldInspector', - 'utilities.custom_inspectors.CustomChoiceFieldInspector', 'utilities.custom_inspectors.SerializedPKRelatedFieldInspector', 'drf_yasg.inspectors.CamelCaseJSONFilter', 'drf_yasg.inspectors.ReferencingSerializerInspector', diff --git a/netbox/utilities/custom_inspectors.py b/netbox/utilities/custom_inspectors.py index 38297838d..e013bf82a 100644 --- a/netbox/utilities/custom_inspectors.py +++ b/netbox/utilities/custom_inspectors.py @@ -5,7 +5,6 @@ from drf_yasg.utils import get_serializer_ref_name from rest_framework.fields import ChoiceField from rest_framework.relations import ManyRelatedField -from extras.api.customfields import CustomFieldsSerializer from utilities.api import ChoiceField, SerializedPKRelatedField, WritableNestedSerializer @@ -49,47 +48,6 @@ class SerializedPKRelatedFieldInspector(FieldInspector): return NotHandled -class CustomChoiceFieldInspector(FieldInspector): - def field_to_swagger_object(self, field, swagger_object_type, use_references, **kwargs): - # this returns a callable which extracts title, description and other stuff - # https://drf-yasg.readthedocs.io/en/stable/_modules/drf_yasg/inspectors/base.html#FieldInspector._get_partial_types - SwaggerType, _ = self._get_partial_types(field, swagger_object_type, use_references, **kwargs) - - if isinstance(field, ChoiceField): - choices = field._choices - choice_value = list(choices.keys()) - choice_label = list(choices.values()) - value_schema = openapi.Schema(type=openapi.TYPE_STRING, enum=choice_value) - - if set([None] + choice_value) == {None, True, False}: - # DeviceType.subdevice_role, Device.face and InterfaceConnection.connection_status all need to be - # differentiated since they each have subtly different values in their choice keys. - # - subdevice_role and connection_status are booleans, although subdevice_role includes None - # - face is an integer set {0, 1} which is easily confused with {False, True} - schema_type = openapi.TYPE_STRING - if all(type(x) == bool for x in [c for c in choice_value if c is not None]): - schema_type = openapi.TYPE_BOOLEAN - value_schema = openapi.Schema(type=schema_type, enum=choice_value) - value_schema['x-nullable'] = True - - if all(type(x) == int for x in [c for c in choice_value if c is not None]): - # Change value_schema for IPAddressFamilyChoices, RackWidthChoices - value_schema = openapi.Schema(type=openapi.TYPE_INTEGER, enum=choice_value) - - schema = SwaggerType(type=openapi.TYPE_OBJECT, required=["label", "value"], properties={ - "label": openapi.Schema(type=openapi.TYPE_STRING, enum=choice_label), - "value": value_schema - }) - - return schema - - elif isinstance(field, CustomFieldsSerializer): - schema = SwaggerType(type=openapi.TYPE_OBJECT) - return schema - - return NotHandled - - class NullableBooleanFieldInspector(FieldInspector): def process_result(self, result, method_name, obj, **kwargs): From d0f1c733e78c0812c2db9f7ea51d66159d5676f9 Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Tue, 25 Aug 2020 15:22:32 -0400 Subject: [PATCH 09/19] Replace CustomFieldsSerializer with CustomFieldsDataField --- netbox/extras/api/customfields.py | 16 ++++++++++------ netbox/extras/tests/test_customfields.py | 22 ++++++++++------------ 2 files changed, 20 insertions(+), 18 deletions(-) diff --git a/netbox/extras/api/customfields.py b/netbox/extras/api/customfields.py index a053642db..393e4545f 100644 --- a/netbox/extras/api/customfields.py +++ b/netbox/extras/api/customfields.py @@ -1,10 +1,9 @@ from datetime import datetime from django.contrib.contenttypes.models import ContentType -from django.core.exceptions import ObjectDoesNotExist from rest_framework import serializers from rest_framework.exceptions import ValidationError -from rest_framework.fields import CreateOnlyDefault +from rest_framework.fields import CreateOnlyDefault, Field from extras.choices import * from extras.models import CustomField @@ -46,12 +45,18 @@ class CustomFieldDefaultValues: return value -class CustomFieldsSerializer(serializers.BaseSerializer): +class CustomFieldsDataField(Field): def to_representation(self, obj): - return obj + content_type = ContentType.objects.get_for_model(self.parent.Meta.model) + custom_fields = CustomField.objects.filter(obj_type=content_type) + + return {cf.name: obj.get(cf.name) for cf in custom_fields} def to_internal_value(self, data): + # If updating an existing instance, start with existing custom_field_data + if self.parent.instance: + data = {**self.parent.instance.custom_field_data, **data} content_type = ContentType.objects.get_for_model(self.parent.Meta.model) custom_fields = { @@ -111,9 +116,8 @@ class CustomFieldModelSerializer(ValidatedModelSerializer): """ Extends ModelSerializer to render any CustomFields and their values associated with an object. """ - custom_fields = CustomFieldsSerializer( + custom_fields = CustomFieldsDataField( source='custom_field_data', - required=False, default=CreateOnlyDefault(CustomFieldDefaultValues()) ) diff --git a/netbox/extras/tests/test_customfields.py b/netbox/extras/tests/test_customfields.py index 675248a3b..31d3c2be9 100644 --- a/netbox/extras/tests/test_customfields.py +++ b/netbox/extras/tests/test_customfields.py @@ -393,19 +393,17 @@ class CustomFieldAPITest(APITestCase): # Validate response data response_cf = response.data['custom_fields'] - data_cf = data['custom_fields'] - self.assertEqual(response_cf['text_field'], data_cf['text_field']) - self.assertEqual(response_cf['number_field'], data_cf['number_field']) - # TODO: Non-updated fields are missing from the response data - # self.assertEqual(response_cf['boolean_field'], site2_original_cfvs['boolean_field']) - # self.assertEqual(response_cf['date_field'], site2_original_cfvs['date_field']) - # self.assertEqual(response_cf['url_field'], site2_original_cfvs['url_field']) - # self.assertEqual(response_cf['choice_field'], site2_original_cfvs['choice_field'].value) + self.assertEqual(response_cf['text_field'], data['custom_fields']['text_field']) + self.assertEqual(response_cf['number_field'], data['custom_fields']['number_field']) + self.assertEqual(response_cf['boolean_field'], original_cfvs['boolean_field']) + self.assertEqual(response_cf['date_field'], original_cfvs['date_field']) + self.assertEqual(response_cf['url_field'], original_cfvs['url_field']) + self.assertEqual(response_cf['choice_field'], original_cfvs['choice_field']) # Validate database data site.refresh_from_db() - self.assertEqual(site.custom_field_data['text_field'], data_cf['text_field']) - self.assertEqual(site.custom_field_data['number_field'], data_cf['number_field']) + self.assertEqual(site.custom_field_data['text_field'], data['custom_fields']['text_field']) + self.assertEqual(site.custom_field_data['number_field'], data['custom_fields']['number_field']) self.assertEqual(site.custom_field_data['boolean_field'], original_cfvs['boolean_field']) self.assertEqual(site.custom_field_data['date_field'], original_cfvs['date_field']) self.assertEqual(site.custom_field_data['url_field'], original_cfvs['url_field']) @@ -456,7 +454,7 @@ class CustomFieldImportTest(TestCase): self.assertEqual(site1.custom_field_data['boolean'], True) self.assertEqual(site1.custom_field_data['date'], '2020-01-01') self.assertEqual(site1.custom_field_data['url'], 'http://example.com/1') - self.assertEqual(site1.custom_field_data['select'].value, 'Choice A') + self.assertEqual(site1.custom_field_data['select'], 'Choice A') # Validate data for site 2 site2 = Site.objects.get(name='Site 2') @@ -466,7 +464,7 @@ class CustomFieldImportTest(TestCase): self.assertEqual(site2.custom_field_data['boolean'], False) self.assertEqual(site2.custom_field_data['date'], '2020-01-02') self.assertEqual(site2.custom_field_data['url'], 'http://example.com/2') - self.assertEqual(site2.custom_field_data['select'].value, 'Choice B') + self.assertEqual(site2.custom_field_data['select'], 'Choice B') # No CustomFieldValues should be created for site 3 site3 = Site.objects.get(name='Site 3') From a9086b06795e0fdedf4e1614542f101c5b1c670d Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Tue, 25 Aug 2020 15:31:01 -0400 Subject: [PATCH 10/19] Fix import test --- netbox/extras/tests/test_customfields.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/netbox/extras/tests/test_customfields.py b/netbox/extras/tests/test_customfields.py index 31d3c2be9..38d904d41 100644 --- a/netbox/extras/tests/test_customfields.py +++ b/netbox/extras/tests/test_customfields.py @@ -466,9 +466,9 @@ class CustomFieldImportTest(TestCase): self.assertEqual(site2.custom_field_data['url'], 'http://example.com/2') self.assertEqual(site2.custom_field_data['select'], 'Choice B') - # No CustomFieldValues should be created for site 3 + # No custom field data should be set for site 3 site3 = Site.objects.get(name='Site 3') - self.assertEqual(site3.custom_field_data, {}) + self.assertFalse(any(site3.custom_field_data.values())) def test_import_missing_required(self): """ From 378c0ac2594537af106b0aca879d19df8d2f31ae Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Tue, 25 Aug 2020 16:21:54 -0400 Subject: [PATCH 11/19] Fix filtering by custom field value --- netbox/extras/filters.py | 47 +++++++++++++--------------------------- 1 file changed, 15 insertions(+), 32 deletions(-) diff --git a/netbox/extras/filters.py b/netbox/extras/filters.py index 73811c063..c7116fa0d 100644 --- a/netbox/extras/filters.py +++ b/netbox/extras/filters.py @@ -21,15 +21,20 @@ __all__ = ( 'TagFilterSet', ) +EXACT_FILTER_TYPES = ( + CustomFieldTypeChoices.TYPE_BOOLEAN, + CustomFieldTypeChoices.TYPE_DATE, + CustomFieldTypeChoices.TYPE_INTEGER, + CustomFieldTypeChoices.TYPE_SELECT, +) + class CustomFieldFilter(django_filters.Filter): """ Filter objects by the presence of a CustomFieldValue. The filter's name is used as the CustomField name. """ - def __init__(self, custom_field, *args, **kwargs): - self.cf_type = custom_field.type - self.filter_logic = custom_field.filter_logic + self.custom_field = custom_field super().__init__(*args, **kwargs) def filter(self, queryset, value): @@ -38,44 +43,22 @@ class CustomFieldFilter(django_filters.Filter): if value is None or not value.strip(): return queryset - # Selection fields get special treatment (values must be integers) - if self.cf_type == CustomFieldTypeChoices.TYPE_SELECT: - try: - # Treat 0 as None - if int(value) == 0: - return queryset.exclude( - custom_field_values__field__name=self.field_name, - ) - # Match on exact CustomFieldChoice PK - else: - return queryset.filter( - custom_field_values__field__name=self.field_name, - custom_field_values__serialized_value=value, - ) - except ValueError: - return queryset.none() - # Apply the assigned filter logic (exact or loose) - if (self.cf_type == CustomFieldTypeChoices.TYPE_BOOLEAN or - self.filter_logic == CustomFieldFilterLogicChoices.FILTER_EXACT): - queryset = queryset.filter( - custom_field_values__field__name=self.field_name, - custom_field_values__serialized_value=value - ) + if ( + self.custom_field.type in EXACT_FILTER_TYPES or + self.custom_field.filter_logic == CustomFieldFilterLogicChoices.FILTER_EXACT + ): + kwargs = {f'custom_field_data__{self.field_name}': value} else: - queryset = queryset.filter( - custom_field_values__field__name=self.field_name, - custom_field_values__serialized_value__icontains=value - ) + kwargs = {f'custom_field_data__{self.field_name}__icontains': value} - return queryset + return queryset.filter(**kwargs) class CustomFieldFilterSet(django_filters.FilterSet): """ Dynamically add a Filter for each CustomField applicable to the parent model. """ - def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) From d2b7eb161c090b5fc541813cf6681daf25745dd1 Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Tue, 25 Aug 2020 16:43:40 -0400 Subject: [PATCH 12/19] Cache CustomField assignments for API queries --- netbox/extras/api/customfields.py | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/netbox/extras/api/customfields.py b/netbox/extras/api/customfields.py index 393e4545f..52c9a18ab 100644 --- a/netbox/extras/api/customfields.py +++ b/netbox/extras/api/customfields.py @@ -1,7 +1,6 @@ from datetime import datetime from django.contrib.contenttypes.models import ContentType -from rest_framework import serializers from rest_framework.exceptions import ValidationError from rest_framework.fields import CreateOnlyDefault, Field @@ -47,28 +46,33 @@ class CustomFieldDefaultValues: class CustomFieldsDataField(Field): - def to_representation(self, obj): - content_type = ContentType.objects.get_for_model(self.parent.Meta.model) - custom_fields = CustomField.objects.filter(obj_type=content_type) + def _get_custom_fields(self): + """ + Cache CustomFields assigned to this model to avoid redundant database queries + """ + if not hasattr(self, '_custom_fields'): + content_type = ContentType.objects.get_for_model(self.parent.Meta.model) + self._custom_fields = CustomField.objects.filter(obj_type=content_type) + return self._custom_fields - return {cf.name: obj.get(cf.name) for cf in custom_fields} + def to_representation(self, obj): + return { + cf.name: obj.get(cf.name) for cf in self._get_custom_fields() + } def to_internal_value(self, data): # If updating an existing instance, start with existing custom_field_data if self.parent.instance: data = {**self.parent.instance.custom_field_data, **data} - content_type = ContentType.objects.get_for_model(self.parent.Meta.model) - custom_fields = { - field.name: field for field in CustomField.objects.filter(obj_type=content_type) - } + custom_fields = {field.name: field for field in self._get_custom_fields()} for field_name, value in data.items(): try: cf = custom_fields[field_name] except KeyError: - raise ValidationError(f"Invalid custom field for {content_type} objects: {field_name}") + raise ValidationError(f"Invalid custom field name: {field_name}") # Data validation if value not in [None, '']: From bde25e69f8284a739074fb3dfd3bdbfe3e789798 Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Wed, 26 Aug 2020 14:36:45 -0400 Subject: [PATCH 13/19] Add CustomFieldsDataFieldInspector for OpenAPI spec --- netbox/netbox/settings.py | 1 + netbox/utilities/custom_inspectors.py | 12 ++++++++++++ 2 files changed, 13 insertions(+) diff --git a/netbox/netbox/settings.py b/netbox/netbox/settings.py index 92f39cf44..7bcc806d7 100644 --- a/netbox/netbox/settings.py +++ b/netbox/netbox/settings.py @@ -484,6 +484,7 @@ REST_FRAMEWORK = { SWAGGER_SETTINGS = { 'DEFAULT_AUTO_SCHEMA_CLASS': 'utilities.custom_inspectors.NetBoxSwaggerAutoSchema', 'DEFAULT_FIELD_INSPECTORS': [ + 'utilities.custom_inspectors.CustomFieldsDataFieldInspector', 'utilities.custom_inspectors.JSONFieldInspector', 'utilities.custom_inspectors.NullableBooleanFieldInspector', 'utilities.custom_inspectors.SerializedPKRelatedFieldInspector', diff --git a/netbox/utilities/custom_inspectors.py b/netbox/utilities/custom_inspectors.py index e013bf82a..bee2f7d92 100644 --- a/netbox/utilities/custom_inspectors.py +++ b/netbox/utilities/custom_inspectors.py @@ -5,6 +5,7 @@ from drf_yasg.utils import get_serializer_ref_name from rest_framework.fields import ChoiceField from rest_framework.relations import ManyRelatedField +from extras.api.customfields import CustomFieldsDataField from utilities.api import ChoiceField, SerializedPKRelatedField, WritableNestedSerializer @@ -60,6 +61,17 @@ class NullableBooleanFieldInspector(FieldInspector): return result +class CustomFieldsDataFieldInspector(FieldInspector): + + def field_to_swagger_object(self, field, swagger_object_type, use_references, **kwargs): + SwaggerType, ChildSwaggerType = self._get_partial_types(field, swagger_object_type, use_references, **kwargs) + + if isinstance(field, CustomFieldsDataField) and swagger_object_type == openapi.Schema: + return SwaggerType(type=openapi.TYPE_OBJECT) + + return NotHandled + + class JSONFieldInspector(FieldInspector): """Required because by default, Swagger sees a JSONField as a string and not dict """ From 53e09a924c0d8331391ed02ea41aaca91c46ed2f Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Wed, 26 Aug 2020 15:04:22 -0400 Subject: [PATCH 14/19] Restore and rename CustomChoiceFieldInspector --- netbox/netbox/settings.py | 1 + netbox/utilities/custom_inspectors.py | 37 +++++++++++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/netbox/netbox/settings.py b/netbox/netbox/settings.py index 7bcc806d7..66aabfcf5 100644 --- a/netbox/netbox/settings.py +++ b/netbox/netbox/settings.py @@ -487,6 +487,7 @@ SWAGGER_SETTINGS = { 'utilities.custom_inspectors.CustomFieldsDataFieldInspector', 'utilities.custom_inspectors.JSONFieldInspector', 'utilities.custom_inspectors.NullableBooleanFieldInspector', + 'utilities.custom_inspectors.ChoiceFieldInspector', 'utilities.custom_inspectors.SerializedPKRelatedFieldInspector', 'drf_yasg.inspectors.CamelCaseJSONFilter', 'drf_yasg.inspectors.ReferencingSerializerInspector', diff --git a/netbox/utilities/custom_inspectors.py b/netbox/utilities/custom_inspectors.py index bee2f7d92..1d5c9c0a0 100644 --- a/netbox/utilities/custom_inspectors.py +++ b/netbox/utilities/custom_inspectors.py @@ -49,6 +49,43 @@ class SerializedPKRelatedFieldInspector(FieldInspector): return NotHandled +class ChoiceFieldInspector(FieldInspector): + def field_to_swagger_object(self, field, swagger_object_type, use_references, **kwargs): + # this returns a callable which extracts title, description and other stuff + # https://drf-yasg.readthedocs.io/en/stable/_modules/drf_yasg/inspectors/base.html#FieldInspector._get_partial_types + SwaggerType, _ = self._get_partial_types(field, swagger_object_type, use_references, **kwargs) + + if isinstance(field, ChoiceField): + choices = field._choices + choice_value = list(choices.keys()) + choice_label = list(choices.values()) + value_schema = openapi.Schema(type=openapi.TYPE_STRING, enum=choice_value) + + if set([None] + choice_value) == {None, True, False}: + # DeviceType.subdevice_role, Device.face and InterfaceConnection.connection_status all need to be + # differentiated since they each have subtly different values in their choice keys. + # - subdevice_role and connection_status are booleans, although subdevice_role includes None + # - face is an integer set {0, 1} which is easily confused with {False, True} + schema_type = openapi.TYPE_STRING + if all(type(x) == bool for x in [c for c in choice_value if c is not None]): + schema_type = openapi.TYPE_BOOLEAN + value_schema = openapi.Schema(type=schema_type, enum=choice_value) + value_schema['x-nullable'] = True + + if all(type(x) == int for x in [c for c in choice_value if c is not None]): + # Change value_schema for IPAddressFamilyChoices, RackWidthChoices + value_schema = openapi.Schema(type=openapi.TYPE_INTEGER, enum=choice_value) + + schema = SwaggerType(type=openapi.TYPE_OBJECT, required=["label", "value"], properties={ + "label": openapi.Schema(type=openapi.TYPE_STRING, enum=choice_label), + "value": value_schema + }) + + return schema + + return NotHandled + + class NullableBooleanFieldInspector(FieldInspector): def process_result(self, result, method_name, obj, **kwargs): From a7431025676d0b5a0602904b506bf847a05f96a0 Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Tue, 15 Sep 2020 15:53:59 -0400 Subject: [PATCH 15/19] Fix serialization of custom_fields for change logging --- netbox/utilities/utils.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/netbox/utilities/utils.py b/netbox/utilities/utils.py index 81baadb7a..fbb7830e2 100644 --- a/netbox/utilities/utils.py +++ b/netbox/utilities/utils.py @@ -91,11 +91,9 @@ def serialize_object(obj, extra=None, exclude=None): json_str = serialize('json', [obj]) data = json.loads(json_str)[0]['fields'] - # Include any custom fields - if hasattr(obj, 'get_custom_fields'): - data['custom_fields'] = { - field: str(value) for field, value in obj.cf.items() - } + # Include custom_field_data as "custom_fields" + if hasattr(obj, 'custom_field_data'): + data['custom_fields'] = data.pop('custom_field_data') # Include any tags. Check for tags cached on the instance; fall back to using the manager. if is_taggable(obj): From 2d56a658b3f79b6eff70fd1ef8a8fcb336d19e99 Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Wed, 16 Sep 2020 17:03:31 -0400 Subject: [PATCH 16/19] Clean up stale data when a custom field is changed/deleted --- netbox/extras/models/customfields.py | 11 +++++++++++ netbox/extras/signals.py | 27 ++++++++++++++++++++++++++- 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/netbox/extras/models/customfields.py b/netbox/extras/models/customfields.py index 422438539..eac481bef 100644 --- a/netbox/extras/models/customfields.py +++ b/netbox/extras/models/customfields.py @@ -116,6 +116,17 @@ class CustomField(models.Model): def __str__(self): return self.label or self.name.replace('_', ' ').capitalize() + def remove_stale_data(self, content_types): + """ + Delete custom field data which is no longer relevant (either because the CustomField is + no longer assigned to a model, or because it has been deleted). + """ + for ct in content_types: + model = ct.model_class() + for obj in model.objects.filter(**{f'custom_field_data__{self.name}__isnull': False}): + del(obj.custom_field_data[self.name]) + obj.save() + def clean(self): # Choices can be set only on selection fields if self.choices and self.type != CustomFieldTypeChoices.TYPE_SELECT: diff --git a/netbox/extras/signals.py b/netbox/extras/signals.py index e10c41d34..d4e187b5c 100644 --- a/netbox/extras/signals.py +++ b/netbox/extras/signals.py @@ -3,12 +3,14 @@ from datetime import timedelta from cacheops.signals import cache_invalidated, cache_read from django.conf import settings +from django.contrib.contenttypes.models import ContentType +from django.db.models.signals import m2m_changed, pre_delete from django.utils import timezone from django_prometheus.models import model_deletes, model_inserts, model_updates from prometheus_client import Counter from .choices import ObjectChangeActionChoices -from .models import ObjectChange +from .models import CustomField, ObjectChange from .webhooks import enqueue_webhooks @@ -71,6 +73,29 @@ def _handle_deleted_object(request, sender, instance, **kwargs): model_deletes.labels(instance._meta.model_name).inc() +# +# Custom fields +# + +def handle_cf_removed_obj_types(instance, action, pk_set, **kwargs): + """ + Handle the cleanup of old custom field data when a CustomField is removed from one or more ContentTypes. + """ + if action == 'post_remove': + instance.remove_stale_data(ContentType.objects.filter(pk__in=pk_set)) + + +def handle_cf_deleted(instance, **kwargs): + """ + Handle the cleanup of old custom field data when a CustomField is deleted. + """ + instance.remove_stale_data(instance.obj_type.all()) + + +m2m_changed.connect(handle_cf_removed_obj_types, sender=CustomField.obj_type.through) +pre_delete.connect(handle_cf_deleted, sender=CustomField) + + # # Caching # From 4ecd3d23f7f3790d5fba1d894f87e7772145a8bc Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Thu, 17 Sep 2020 12:14:02 -0400 Subject: [PATCH 17/19] Disable bulk deletion of CustomFields in admin UI --- netbox/extras/admin.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/netbox/extras/admin.py b/netbox/extras/admin.py index 4bd738160..9af9077ac 100644 --- a/netbox/extras/admin.py +++ b/netbox/extras/admin.py @@ -83,13 +83,14 @@ class CustomFieldForm(forms.ModelForm): @admin.register(CustomField) class CustomFieldAdmin(admin.ModelAdmin): + actions = None + form = CustomFieldForm list_display = [ 'name', 'models', 'type', 'required', 'filter_logic', 'default', 'weight', 'description', ] list_filter = [ 'type', 'required', 'obj_type', ] - form = CustomFieldForm def models(self, obj): return ', '.join([ct.name for ct in obj.obj_type.all()]) From 3d2f6c07031d0abbd8c08f304f7b5665ece9106b Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Thu, 17 Sep 2020 12:26:02 -0400 Subject: [PATCH 18/19] Simplify form field for boolean CustomFields --- netbox/extras/models/customfields.py | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/netbox/extras/models/customfields.py b/netbox/extras/models/customfields.py index eac481bef..bb577fd4b 100644 --- a/netbox/extras/models/customfields.py +++ b/netbox/extras/models/customfields.py @@ -159,15 +159,11 @@ class CustomField(models.Model): elif self.type == CustomFieldTypeChoices.TYPE_BOOLEAN: choices = ( (None, '---------'), - (1, 'True'), - (0, 'False'), + (True, 'True'), + (False, 'False'), ) - if initial is not None and initial.lower() in ['true', 'yes', '1']: - initial = 1 - elif initial is not None and initial.lower() in ['false', 'no', '0']: - initial = 0 - else: - initial = None + if initial is not None: + initial = bool(initial) field = forms.NullBooleanField( required=required, initial=initial, widget=StaticSelect2(choices=choices) ) From 61cf9030285697f5e1bc70eccb958d0219594784 Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Thu, 17 Sep 2020 12:39:57 -0400 Subject: [PATCH 19/19] Clean up CustomField admin form --- netbox/extras/admin.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/netbox/extras/admin.py b/netbox/extras/admin.py index 9af9077ac..a8b4c0eb8 100644 --- a/netbox/extras/admin.py +++ b/netbox/extras/admin.py @@ -2,6 +2,7 @@ from django import forms from django.contrib import admin from utilities.forms import LaxURLField +from .choices import CustomFieldTypeChoices from .models import CustomField, CustomLink, ExportTemplate, JobResult, Webhook @@ -80,6 +81,14 @@ class CustomFieldForm(forms.ModelForm): order_content_types(self.fields['obj_type']) + def clean(self): + + # Validate selection choices + if self.cleaned_data['type'] == CustomFieldTypeChoices.TYPE_SELECT and len(self.cleaned_data['choices']) < 2: + raise forms.ValidationError({ + 'choices': 'Selection fields must specify at least two choices.' + }) + @admin.register(CustomField) class CustomFieldAdmin(admin.ModelAdmin): @@ -91,6 +100,19 @@ class CustomFieldAdmin(admin.ModelAdmin): list_filter = [ 'type', 'required', 'obj_type', ] + fieldsets = ( + ('Custom Field', { + 'fields': ('type', 'name', 'weight', 'label', 'description', 'required', 'default', 'filter_logic') + }), + ('Assignment', { + 'description': 'A custom field must be assigned to one or more object types.', + 'fields': ('obj_type',) + }), + ('Choices', { + 'description': 'A selection field must have two or more choices assigned to it.', + 'fields': ('choices',) + }) + ) def models(self, obj): return ', '.join([ct.name for ct in obj.obj_type.all()])