diff --git a/docs/customization/custom-fields.md b/docs/customization/custom-fields.md index 51aa77183..4bebd56fd 100644 --- a/docs/customization/custom-fields.md +++ b/docs/customization/custom-fields.md @@ -135,3 +135,5 @@ To set or change these values, simply include nested JSON data. For example: ``` As with built-in choice fields, selection custom fields are written by passing the raw value (e.g. `"site_type": "datacenter"`), not the `{value, label}` object returned on read. + +The GraphQL API's `custom_fields` field resolves selection and multiple selection values to the same `{value, label}` representation. diff --git a/netbox/extras/api/customfields.py b/netbox/extras/api/customfields.py index 953bedafa..238890bc8 100644 --- a/netbox/extras/api/customfields.py +++ b/netbox/extras/api/customfields.py @@ -76,17 +76,8 @@ class CustomFieldsDataField(Field): elif value is not None and cf.type == CustomFieldTypeChoices.TYPE_MULTIOBJECT: serializer = get_serializer_for_model(cf.related_object_type.model_class()) value = serializer(value, nested=True, many=True, context=self.parent.context).data - elif value is not None and cf.type == CustomFieldTypeChoices.TYPE_SELECT: - # Represent the selected choice as an object with its value and resolved label - value = { - 'value': value, - 'label': cf.get_choice_label(value), - } - elif value is not None and cf.type == CustomFieldTypeChoices.TYPE_MULTISELECT: - # Represent each selected choice as an object with its value and resolved label - value = [ - {'value': v, 'label': cf.get_choice_label(v)} for v in value - ] + elif cf.type in (CustomFieldTypeChoices.TYPE_SELECT, CustomFieldTypeChoices.TYPE_MULTISELECT): + value = cf.resolve_selection_value(value) data[cf.name] = value return data diff --git a/netbox/extras/graphql/mixins.py b/netbox/extras/graphql/mixins.py index 21e33c563..b2afd62ac 100644 --- a/netbox/extras/graphql/mixins.py +++ b/netbox/extras/graphql/mixins.py @@ -4,7 +4,7 @@ import strawberry import strawberry_django from strawberry.types import Info -from extras.models import ImageAttachment, JournalEntry +from extras.models import CustomField, ImageAttachment, JournalEntry from utilities.querysets import RestrictedPrefetch __all__ = ( @@ -50,9 +50,13 @@ class ConfigContextMixin: @strawberry.type class CustomFieldsMixin: - @strawberry_django.field + @strawberry_django.field(only=['custom_field_data']) def custom_fields(self) -> strawberry.scalars.JSON: - return self.custom_field_data + data = dict(self.custom_field_data) + for cf in CustomField.objects.get_for_model(type(self)): + if cf.name in data: + data[cf.name] = cf.resolve_selection_value(data[cf.name]) + return data @strawberry.type diff --git a/netbox/extras/models/customfields.py b/netbox/extras/models/customfields.py index c2f0250b9..6def456d0 100644 --- a/netbox/extras/models/customfields.py +++ b/netbox/extras/models/customfields.py @@ -77,7 +77,9 @@ class CustomFieldManager(models.Manager.from_queryset(RestrictedQuerySet)): return custom_fields content_type = ObjectType.objects.get_for_model(model._meta.concrete_model) - custom_fields = self.get_queryset().filter(object_types=content_type).select_related('related_object_type') + custom_fields = self.get_queryset().filter(object_types=content_type).select_related( + 'related_object_type', 'choice_set' + ) # Populate the request cache to avoid redundant lookups if cache is not None: @@ -328,6 +330,20 @@ class CustomField(CloningMixin, ExportTemplatesMixin, OwnerMixin, ChangeLoggedMo return self.choice_set.get_choice_color(value) return None + def resolve_selection_value(self, value): + """ + For a Selection or Multiple selection field, wrap the value(s) with their resolved label as + {'value': ..., 'label': ...} (a list thereof for multi-select). Other field types pass through + unchanged. Shared by the REST API and GraphQL so selection labels resolve consistently (#20897). + """ + if value is None: + return value + if self.type == CustomFieldTypeChoices.TYPE_SELECT: + return {'value': value, 'label': self.get_choice_label(value)} + if self.type == CustomFieldTypeChoices.TYPE_MULTISELECT: + return [{'value': v, 'label': self.get_choice_label(v)} for v in value] + return value + def populate_initial_data(self, content_types): """ Populate initial custom field data upon either a) the creation of a new CustomField, or diff --git a/netbox/extras/tests/test_customfields.py b/netbox/extras/tests/test_customfields.py index 799fbe287..3c8cfaf07 100644 --- a/netbox/extras/tests/test_customfields.py +++ b/netbox/extras/tests/test_customfields.py @@ -3,7 +3,9 @@ import json from decimal import Decimal from django.core.exceptions import ValidationError +from django.db import connection from django.test import override_settings, tag +from django.test.utils import CaptureQueriesContext from django.urls import reverse from rest_framework import status @@ -15,6 +17,7 @@ from extras.choices import * from extras.models import CustomField, CustomFieldChoiceSet from ipam.models import VLAN from netbox.choices import CSVDelimiterChoices, ImportFormatChoices +from netbox.context import query_cache from netbox.tables.columns import CustomFieldColumn from utilities.testing import APITestCase, TestCase from virtualization.models import VirtualMachine @@ -1135,6 +1138,87 @@ class CustomFieldAPITestCase(APITestCase): 'label': 'stale', }) + def test_graphql_selection_field_representation_matches_rest(self): + site2 = Site.objects.get(name='Site 2') + self.add_permissions('dcim.view_site') + + query = f'{{ site(id: {site2.pk}) {{ custom_fields }} }}' + response = self.client.post(reverse('graphql'), data={'query': query}, format='json', **self.header) + self.assertHttpStatus(response, status.HTTP_200_OK) + data = json.loads(response.content) + self.assertNotIn('errors', data) + custom_fields = data['data']['site']['custom_fields'] + + self.assertEqual(custom_fields['select_field'], self._select('bar')) + self.assertEqual(custom_fields['multiselect_field'], self._multiselect(['bar', 'baz'])) + + def test_graphql_selection_field_unresolved_label(self): + site2 = Site.objects.get(name='Site 2') + site2.custom_field_data['select_field'] = 'stale' + site2.save() + self.add_permissions('dcim.view_site') + + query = f'{{ site(id: {site2.pk}) {{ custom_fields }} }}' + response = self.client.post(reverse('graphql'), data={'query': query}, format='json', **self.header) + self.assertHttpStatus(response, status.HTTP_200_OK) + data = json.loads(response.content) + self.assertNotIn('errors', data) + self.assertEqual(data['data']['site']['custom_fields']['select_field'], { + 'value': 'stale', + 'label': 'stale', + }) + + def test_graphql_non_selection_fields_pass_through_unchanged(self): + site2 = Site.objects.get(name='Site 2') + self.add_permissions('dcim.view_site') + + query = f'{{ site(id: {site2.pk}) {{ custom_fields }} }}' + response = self.client.post(reverse('graphql'), data={'query': query}, format='json', **self.header) + self.assertHttpStatus(response, status.HTTP_200_OK) + data = json.loads(response.content) + self.assertNotIn('errors', data) + custom_fields = data['data']['site']['custom_fields'] + + self.assertEqual(custom_fields['text_field'], 'bar') + self.assertEqual(custom_fields['integer_field'], 456) + self.assertEqual(custom_fields['boolean_field'], True) + + def test_graphql_selection_field_list_query_is_not_n_plus_one(self): + self.add_permissions('dcim.view_site') + query = '{ site_list { custom_fields } }' + + Site.objects.bulk_create([Site(name=f'Site {i}', slug=f'site-{i}') for i in range(3, 8)]) + # Prime process-level caches (e.g. ContentType) outside the measured request. + self.client.post(reverse('graphql'), data={'query': query}, format='json', **self.header) + with CaptureQueriesContext(connection) as ctx: + response = self.client.post(reverse('graphql'), data={'query': query}, format='json', **self.header) + self.assertHttpStatus(response, status.HTTP_200_OK) + data = json.loads(response.content) + self.assertNotIn('errors', data) + self.assertEqual(len(data['data']['site_list']), 7) + baseline_query_count = len(ctx.captured_queries) + + Site.objects.bulk_create([Site(name=f'Site {i}', slug=f'site-{i}') for i in range(8, 13)]) + with CaptureQueriesContext(connection) as ctx: + response = self.client.post(reverse('graphql'), data={'query': query}, format='json', **self.header) + self.assertHttpStatus(response, status.HTTP_200_OK) + data = json.loads(response.content) + self.assertNotIn('errors', data) + self.assertEqual(len(data['data']['site_list']), 12) + + self.assertEqual( + len(ctx.captured_queries), baseline_query_count, + "custom_fields label resolution should not scale with the number of objects returned" + ) + + def test_get_for_model_select_related_choice_set(self): + query_cache.set(None) + custom_fields = list(CustomField.objects.get_for_model(Site)) + with self.assertNumQueries(0): + resolved = {cf.name: cf.resolve_selection_value(cf.default) for cf in custom_fields} + self.assertEqual(resolved['select_field'], self._select('foo')) + self.assertEqual(resolved['multiselect_field'], self._multiselect(['foo'])) + @tag('regression') def test_update_selection_field_rejects_read_format(self): """