From 983032189abb1f0e26835f761ae71ea6da0c509a Mon Sep 17 00:00:00 2001 From: Arthur Hanson Date: Tue, 24 Sep 2024 09:56:39 -0700 Subject: [PATCH 1/6] 17558 raise validation error if removing choice from choiceset that is currently used --- netbox/extras/models/customfields.py | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/netbox/extras/models/customfields.py b/netbox/extras/models/customfields.py index 8895949027d..017e65c24e5 100644 --- a/netbox/extras/models/customfields.py +++ b/netbox/extras/models/customfields.py @@ -785,6 +785,10 @@ class Meta: def __str__(self): return self.name + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self._original_extra_choices = self.__dict__.get('extra_choices') + def get_absolute_url(self): return reverse('extras:customfieldchoiceset', args=[self.pk]) @@ -818,6 +822,29 @@ def clean(self): if not self.base_choices and not self.extra_choices: raise ValidationError(_("Must define base or extra choices.")) + # check if removing any used choices + original_choices = [obj[1] for obj in self._original_extra_choices] + new_choices = [obj[1] for obj in self.extra_choices] + diff_choices = list(set(original_choices) - set(new_choices)) + if diff_choices: + # CustomFields using this ChoiceSet + for custom_field in self.choices_for.all(): + for object_type in custom_field.object_types.all(): + # unfortunately querying the whole array of diff_choices doesn't work + for choice in diff_choices: + # Need to OR query as can be multiple choice or single choice so + # have to do both contains and equals to catch both + query_args = { + f"custom_field_data__{custom_field.name}__contains": choice, + f"custom_field_data__{custom_field.name}": choice + } + if object_type.model_class().objects.filter(models.Q(**query_args, _connector=models.Q.OR)).exists(): + raise ValidationError( + _("Cannot remove choice {choice} as it is used in model {model}").format( + choice=choice, model=object_type + ) + ) + def save(self, *args, **kwargs): # Sort choices if alphabetical ordering is enforced From b59ae0fd934273eecd22bf3d60cdbe7c2f43c753 Mon Sep 17 00:00:00 2001 From: Arthur Hanson Date: Tue, 24 Sep 2024 10:23:16 -0700 Subject: [PATCH 2/6] 17558 raise validation error if removing choice from choiceset that is currently used --- netbox/extras/models/customfields.py | 44 +++++++++++++++------------- 1 file changed, 24 insertions(+), 20 deletions(-) diff --git a/netbox/extras/models/customfields.py b/netbox/extras/models/customfields.py index 017e65c24e5..24d808c03ad 100644 --- a/netbox/extras/models/customfields.py +++ b/netbox/extras/models/customfields.py @@ -823,27 +823,31 @@ def clean(self): raise ValidationError(_("Must define base or extra choices.")) # check if removing any used choices - original_choices = [obj[1] for obj in self._original_extra_choices] - new_choices = [obj[1] for obj in self.extra_choices] - diff_choices = list(set(original_choices) - set(new_choices)) - if diff_choices: - # CustomFields using this ChoiceSet - for custom_field in self.choices_for.all(): - for object_type in custom_field.object_types.all(): - # unfortunately querying the whole array of diff_choices doesn't work - for choice in diff_choices: - # Need to OR query as can be multiple choice or single choice so - # have to do both contains and equals to catch both - query_args = { - f"custom_field_data__{custom_field.name}__contains": choice, - f"custom_field_data__{custom_field.name}": choice - } - if object_type.model_class().objects.filter(models.Q(**query_args, _connector=models.Q.OR)).exists(): - raise ValidationError( - _("Cannot remove choice {choice} as it is used in model {model}").format( - choice=choice, model=object_type + if self._original_extra_choices: + original_choices = new_choices = [] + original_choices = [obj[1] for obj in self._original_extra_choices] + if self.extra_choices: + new_choices = [obj[1] for obj in self.extra_choices] + + diff_choices = list(set(original_choices) - set(new_choices)) + if diff_choices: + # CustomFields using this ChoiceSet + for custom_field in self.choices_for.all(): + for object_type in custom_field.object_types.all(): + # unfortunately querying the whole array of diff_choices doesn't work + for choice in diff_choices: + # Need to OR query as can be multiple choice or single choice so + # have to do both contains and equals to catch both + query_args = { + f"custom_field_data__{custom_field.name}__contains": choice, + f"custom_field_data__{custom_field.name}": choice + } + if object_type.model_class().objects.filter(models.Q(**query_args, _connector=models.Q.OR)).exists(): + raise ValidationError( + _("Cannot remove choice {choice} as it is used in model {model}").format( + choice=choice, model=object_type + ) ) - ) def save(self, *args, **kwargs): From 52b8bd5523054a0e267a3b3c1294282409b2cf81 Mon Sep 17 00:00:00 2001 From: Arthur Hanson Date: Tue, 24 Sep 2024 11:28:40 -0700 Subject: [PATCH 3/6] 17558 raise validation error if removing choice from choiceset that is currently used --- netbox/extras/models/customfields.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/netbox/extras/models/customfields.py b/netbox/extras/models/customfields.py index 24d808c03ad..73b541a9b8d 100644 --- a/netbox/extras/models/customfields.py +++ b/netbox/extras/models/customfields.py @@ -829,10 +829,11 @@ def clean(self): if self.extra_choices: new_choices = [obj[1] for obj in self.extra_choices] - diff_choices = list(set(original_choices) - set(new_choices)) - if diff_choices: - # CustomFields using this ChoiceSet + # only care about what fields are being deleted. + if diff_choices := list(set(original_choices) - set(new_choices)): + # Get all CustomFields using this ChoiceSet for custom_field in self.choices_for.all(): + # Then the models using those custom fields for object_type in custom_field.object_types.all(): # unfortunately querying the whole array of diff_choices doesn't work for choice in diff_choices: From 822a4a4bc81b2ec339ae841590989643e7104d49 Mon Sep 17 00:00:00 2001 From: Arthur Hanson Date: Tue, 24 Sep 2024 12:16:14 -0700 Subject: [PATCH 4/6] 17558 add tests --- netbox/extras/models/customfields.py | 6 +- netbox/extras/tests/test_customfields.py | 72 ++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 3 deletions(-) diff --git a/netbox/extras/models/customfields.py b/netbox/extras/models/customfields.py index 73b541a9b8d..270de348864 100644 --- a/netbox/extras/models/customfields.py +++ b/netbox/extras/models/customfields.py @@ -825,9 +825,9 @@ def clean(self): # check if removing any used choices if self._original_extra_choices: original_choices = new_choices = [] - original_choices = [obj[1] for obj in self._original_extra_choices] + original_choices = [obj[0] for obj in self._original_extra_choices] if self.extra_choices: - new_choices = [obj[1] for obj in self.extra_choices] + new_choices = [obj[0] for obj in self.extra_choices] # only care about what fields are being deleted. if diff_choices := list(set(original_choices) - set(new_choices)): @@ -845,7 +845,7 @@ def clean(self): } if object_type.model_class().objects.filter(models.Q(**query_args, _connector=models.Q.OR)).exists(): raise ValidationError( - _("Cannot remove choice {choice} as it is used in model {model}").format( + _("Cannot remove choice {choice} as it is used in records of model {model}").format( choice=choice, model=object_type ) ) diff --git a/netbox/extras/tests/test_customfields.py b/netbox/extras/tests/test_customfields.py index 697b756ecb3..f440ebd2f5d 100644 --- a/netbox/extras/tests/test_customfields.py +++ b/netbox/extras/tests/test_customfields.py @@ -343,6 +343,78 @@ def test_multiselect_field(self): instance.refresh_from_db() self.assertIsNone(instance.custom_field_data.get(cf.name)) + def test_select_validation(self): + CHOICES = ( + ('a', 'Option A'), + ('b', 'Option B'), + ('c', 'Option C'), + ('d', 'Option D'), + ) + + # Create a set of custom field choices + choice_set = CustomFieldChoiceSet.objects.create( + name='Custom Field Choice Set 1', + extra_choices=CHOICES + ) + + # Create a custom field & check that initial value is null + cf = CustomField.objects.create( + name='select_field', + type=CustomFieldTypeChoices.TYPE_SELECT, + required=False, + choice_set=choice_set + ) + cf.object_types.set([self.object_type]) + + # Create a custom field & check that initial value is null + cf_multiselect = CustomField.objects.create( + name='multiselect_field', + type=CustomFieldTypeChoices.TYPE_MULTISELECT, + required=False, + choice_set=choice_set + ) + cf_multiselect.object_types.set([self.object_type]) + + instance = Site.objects.first() + + # Assign a value and check that it is saved + instance.custom_field_data[cf.name] = 'a' + instance.custom_field_data[cf_multiselect.name] = ['b', 'c'] + instance.save() + instance.refresh_from_db() + + # check can't delete single choice custom field option + with self.assertRaises(ValidationError): + CHOICES = ( + ('b', 'Option B'), + ('c', 'Option C'), + ('d', 'Option D'), + ) + choice_set.extra_choices = CHOICES + choice_set.full_clean() + choice_set.save() + + # check can't delete multi choice custom field option + with self.assertRaises(ValidationError): + CHOICES = ( + ('a', 'Option A'), + ('b', 'Option B'), + ('d', 'Option D'), + ) + choice_set.extra_choices = CHOICES + choice_set.full_clean() + choice_set.save() + + # delete non selected option should work fine + CHOICES = ( + ('a', 'Option A'), + ('b', 'Option B'), + ('c', 'Option C'), + ) + choice_set.extra_choices = CHOICES + choice_set.full_clean() + choice_set.save() + def test_object_field(self): value = VLAN.objects.create(name='VLAN 1', vid=1).pk From 0cb5d9f168e10d8744ada67e10928a0615e28387 Mon Sep 17 00:00:00 2001 From: Arthur Hanson Date: Wed, 25 Sep 2024 08:50:17 -0700 Subject: [PATCH 5/6] 17558 add tests --- netbox/extras/tests/test_customfields.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/netbox/extras/tests/test_customfields.py b/netbox/extras/tests/test_customfields.py index f440ebd2f5d..c365e9653d6 100644 --- a/netbox/extras/tests/test_customfields.py +++ b/netbox/extras/tests/test_customfields.py @@ -349,6 +349,7 @@ def test_select_validation(self): ('b', 'Option B'), ('c', 'Option C'), ('d', 'Option D'), + ('abcde', 'Option ABCDE'), ) # Create a set of custom field choices @@ -389,6 +390,7 @@ def test_select_validation(self): ('b', 'Option B'), ('c', 'Option C'), ('d', 'Option D'), + ('abcde', 'Option ABCDE'), ) choice_set.extra_choices = CHOICES choice_set.full_clean() @@ -400,6 +402,7 @@ def test_select_validation(self): ('a', 'Option A'), ('b', 'Option B'), ('d', 'Option D'), + ('abcde', 'Option ABCDE'), ) choice_set.extra_choices = CHOICES choice_set.full_clean() @@ -410,6 +413,7 @@ def test_select_validation(self): ('a', 'Option A'), ('b', 'Option B'), ('c', 'Option C'), + ('abcde', 'Option ABCDE'), ) choice_set.extra_choices = CHOICES choice_set.full_clean() From 7f372b98a86c9f621903448c9900744cb48459c7 Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Mon, 30 Sep 2024 11:57:10 -0400 Subject: [PATCH 6/6] Tightened up choice evaluation logic a bit; cleaned up test --- netbox/extras/models/customfields.py | 54 ++++++++++++------------ netbox/extras/tests/test_customfields.py | 36 ++++++---------- 2 files changed, 41 insertions(+), 49 deletions(-) diff --git a/netbox/extras/models/customfields.py b/netbox/extras/models/customfields.py index 270de348864..dbf0868723b 100644 --- a/netbox/extras/models/customfields.py +++ b/netbox/extras/models/customfields.py @@ -787,6 +787,8 @@ def __str__(self): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) + + # Cache the initial set of choices for comparison under clean() self._original_extra_choices = self.__dict__.get('extra_choices') def get_absolute_url(self): @@ -822,33 +824,31 @@ def clean(self): if not self.base_choices and not self.extra_choices: raise ValidationError(_("Must define base or extra choices.")) - # check if removing any used choices - if self._original_extra_choices: - original_choices = new_choices = [] - original_choices = [obj[0] for obj in self._original_extra_choices] - if self.extra_choices: - new_choices = [obj[0] for obj in self.extra_choices] - - # only care about what fields are being deleted. - if diff_choices := list(set(original_choices) - set(new_choices)): - # Get all CustomFields using this ChoiceSet - for custom_field in self.choices_for.all(): - # Then the models using those custom fields - for object_type in custom_field.object_types.all(): - # unfortunately querying the whole array of diff_choices doesn't work - for choice in diff_choices: - # Need to OR query as can be multiple choice or single choice so - # have to do both contains and equals to catch both - query_args = { - f"custom_field_data__{custom_field.name}__contains": choice, - f"custom_field_data__{custom_field.name}": choice - } - if object_type.model_class().objects.filter(models.Q(**query_args, _connector=models.Q.OR)).exists(): - raise ValidationError( - _("Cannot remove choice {choice} as it is used in records of model {model}").format( - choice=choice, model=object_type - ) - ) + # Check whether any choices have been removed. If so, check whether any of the removed + # choices are still set in custom field data for any object. + original_choices = set([ + c[0] for c in self._original_extra_choices + ]) if self._original_extra_choices else set() + current_choices = set([ + c[0] for c in self.extra_choices + ]) if self.extra_choices else set() + if removed_choices := original_choices - current_choices: + for custom_field in self.choices_for.all(): + for object_type in custom_field.object_types.all(): + model = object_type.model_class() + for choice in removed_choices: + # Form the query based on the type of custom field + if custom_field.type == CustomFieldTypeChoices.TYPE_MULTISELECT: + query_args = {f"custom_field_data__{custom_field.name}__contains": choice} + else: + query_args = {f"custom_field_data__{custom_field.name}": choice} + # Raise a ValidationError if there are any objects which still reference the removed choice + if model.objects.filter(models.Q(**query_args)).exists(): + raise ValidationError( + _( + "Cannot remove choice {choice} as there are {model} objects which reference it." + ).format(choice=choice, model=object_type) + ) def save(self, *args, **kwargs): diff --git a/netbox/extras/tests/test_customfields.py b/netbox/extras/tests/test_customfields.py index c365e9653d6..2bc9b5accef 100644 --- a/netbox/extras/tests/test_customfields.py +++ b/netbox/extras/tests/test_customfields.py @@ -343,13 +343,16 @@ def test_multiselect_field(self): instance.refresh_from_db() self.assertIsNone(instance.custom_field_data.get(cf.name)) - def test_select_validation(self): + def test_remove_selected_choice(self): + """ + Removing a ChoiceSet choice that is referenced by an object should raise + a ValidationError exception. + """ CHOICES = ( ('a', 'Option A'), ('b', 'Option B'), ('c', 'Option C'), ('d', 'Option D'), - ('abcde', 'Option ABCDE'), ) # Create a set of custom field choices @@ -358,7 +361,7 @@ def test_select_validation(self): extra_choices=CHOICES ) - # Create a custom field & check that initial value is null + # Create a select custom field cf = CustomField.objects.create( name='select_field', type=CustomFieldTypeChoices.TYPE_SELECT, @@ -367,7 +370,7 @@ def test_select_validation(self): ) cf.object_types.set([self.object_type]) - # Create a custom field & check that initial value is null + # Create a multi-select custom field cf_multiselect = CustomField.objects.create( name='multiselect_field', type=CustomFieldTypeChoices.TYPE_MULTISELECT, @@ -376,48 +379,37 @@ def test_select_validation(self): ) cf_multiselect.object_types.set([self.object_type]) + # Assign a choice for both custom fields on an object instance = Site.objects.first() - - # Assign a value and check that it is saved instance.custom_field_data[cf.name] = 'a' instance.custom_field_data[cf_multiselect.name] = ['b', 'c'] instance.save() - instance.refresh_from_db() - # check can't delete single choice custom field option + # Attempting to delete a selected choice should fail with self.assertRaises(ValidationError): - CHOICES = ( + choice_set.extra_choices = ( ('b', 'Option B'), ('c', 'Option C'), ('d', 'Option D'), - ('abcde', 'Option ABCDE'), ) - choice_set.extra_choices = CHOICES choice_set.full_clean() - choice_set.save() - # check can't delete multi choice custom field option + # Attempting to delete either of the multi-select choices should fail with self.assertRaises(ValidationError): - CHOICES = ( + choice_set.extra_choices = ( ('a', 'Option A'), ('b', 'Option B'), ('d', 'Option D'), - ('abcde', 'Option ABCDE'), ) - choice_set.extra_choices = CHOICES choice_set.full_clean() - choice_set.save() - # delete non selected option should work fine - CHOICES = ( + # Removing a non-selected choice should succeed + choice_set.extra_choices = ( ('a', 'Option A'), ('b', 'Option B'), ('c', 'Option C'), - ('abcde', 'Option ABCDE'), ) - choice_set.extra_choices = CHOICES choice_set.full_clean() - choice_set.save() def test_object_field(self): value = VLAN.objects.create(name='VLAN 1', vid=1).pk