From 55ff836db771cffa87d4ce6d78bf34dc5fd73300 Mon Sep 17 00:00:00 2001 From: Arthur Hanson Date: Mon, 29 Jul 2024 21:27:37 +0700 Subject: [PATCH 1/5] 16946 raise error if filterset is not valid --- netbox/netbox/graphql/filter_mixins.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/netbox/netbox/graphql/filter_mixins.py b/netbox/netbox/graphql/filter_mixins.py index 5075e9aa282..997c1209d91 100644 --- a/netbox/netbox/graphql/filter_mixins.py +++ b/netbox/netbox/graphql/filter_mixins.py @@ -201,4 +201,10 @@ def wrapper(cls): class BaseFilterMixin: def filter_by_filterset(self, queryset, key): - return self.filterset(data={key: getattr(self, key)}, queryset=queryset).qs + filterset = self.filterset(data={key: getattr(self, key)}, queryset=queryset) + if not filterset.is_valid(): + # filterset.errors is errorDict - return first error as exception + k, v = next(iter(filterset.errors.items())) + raise Exception(f"{k}: {v[0]}") + qs = filterset.qs + return qs From d8de18b9bf51e354ae29f8aff8a7e702d2ef473e Mon Sep 17 00:00:00 2001 From: Arthur Hanson Date: Mon, 29 Jul 2024 21:29:24 +0700 Subject: [PATCH 2/5] 16946 cleanup --- netbox/netbox/graphql/filter_mixins.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/netbox/netbox/graphql/filter_mixins.py b/netbox/netbox/graphql/filter_mixins.py index 997c1209d91..3a24170e3d4 100644 --- a/netbox/netbox/graphql/filter_mixins.py +++ b/netbox/netbox/graphql/filter_mixins.py @@ -206,5 +206,4 @@ def filter_by_filterset(self, queryset, key): # filterset.errors is errorDict - return first error as exception k, v = next(iter(filterset.errors.items())) raise Exception(f"{k}: {v[0]}") - qs = filterset.qs - return qs + return filterset.qs From 0180a3546d2c874153dbda471760c3e997026f42 Mon Sep 17 00:00:00 2001 From: Arthur Hanson Date: Mon, 26 Aug 2024 11:19:35 -0700 Subject: [PATCH 3/5] 16946 change to None qs return and add test --- netbox/netbox/graphql/filter_mixins.py | 6 ++-- netbox/netbox/tests/test_graphql.py | 45 +++++++++++++++++++++++++- 2 files changed, 48 insertions(+), 3 deletions(-) diff --git a/netbox/netbox/graphql/filter_mixins.py b/netbox/netbox/graphql/filter_mixins.py index 3a24170e3d4..b815e59a8a6 100644 --- a/netbox/netbox/graphql/filter_mixins.py +++ b/netbox/netbox/graphql/filter_mixins.py @@ -4,7 +4,7 @@ import django_filters import strawberry import strawberry_django -from django.core.exceptions import FieldDoesNotExist +from django.core.exceptions import FieldDoesNotExist, ValidationError from strawberry import auto from ipam.fields import ASNField from netbox.graphql.scalars import BigInt @@ -205,5 +205,7 @@ def filter_by_filterset(self, queryset, key): if not filterset.is_valid(): # filterset.errors is errorDict - return first error as exception k, v = next(iter(filterset.errors.items())) - raise Exception(f"{k}: {v[0]}") + return filterset.qs.none() + # We could raise validation error but strawberry logs it all to the + # console i.e. raise ValidationError(f"{k}: {v[0]}") return filterset.qs diff --git a/netbox/netbox/tests/test_graphql.py b/netbox/netbox/tests/test_graphql.py index 2cf9ee87b9d..256a4399d69 100644 --- a/netbox/netbox/tests/test_graphql.py +++ b/netbox/netbox/tests/test_graphql.py @@ -1,7 +1,13 @@ +import json + from django.test import override_settings from django.urls import reverse -from utilities.testing import disable_warnings, TestCase +from core.models import ObjectType +from rest_framework import status +from users.models import ObjectPermission, Token +from users.models import User +from utilities.testing import disable_warnings, APITestCase, TestCase class GraphQLTestCase(TestCase): @@ -34,3 +40,40 @@ def test_graphiql_interface(self): response = self.client.get(url, **header) with disable_warnings('django.request'): self.assertHttpStatus(response, 302) # Redirect to login page + + +class GraphQLAPITestCase(APITestCase): + + @override_settings(LOGIN_REQUIRED=True) + @override_settings(EXEMPT_VIEW_PERMISSIONS=['*', 'auth.user']) + def test_graphql_filter_objects(self): + from dcim.models import Site, Location + + site = Site.objects.create(name='Site A', slug='site-a') + location = Location.objects.create(site=site, name='Location A1', slug='location-a1') + + url = reverse('graphql') + field_name = 'location_list' + query = '{location_list(filters: {site_id: "' + str(site.id) + '"}) {id site {id}}}' + + # Add object-level permission + obj_perm = ObjectPermission( + name='Test permission', + actions=['view'] + ) + obj_perm.save() + obj_perm.users.add(self.user) + obj_perm.object_types.add(ObjectType.objects.get_for_model(Location)) + + response = self.client.post(url, data={'query': query}, format="json", **self.header) + self.assertHttpStatus(response, status.HTTP_200_OK) + data = json.loads(response.content) + self.assertNotIn('errors', data) + self.assertGreater(len(data['data']['location_list']), 0) + + query = '{location_list(filters: {site_id: "' + str(site.id + 1234) + '"}) {id site {id}}}' + response = self.client.post(url, data={'query': query}, format="json", **self.header) + + self.assertHttpStatus(response, status.HTTP_200_OK) + data = json.loads(response.content) + self.assertEqual(len(data['data']['location_list']), 0) From 39ac70b03653a433a3143669c9405d07e13e51ac Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Tue, 27 Aug 2024 10:10:43 -0400 Subject: [PATCH 4/5] Remove obsolete logic --- netbox/netbox/graphql/filter_mixins.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/netbox/netbox/graphql/filter_mixins.py b/netbox/netbox/graphql/filter_mixins.py index b815e59a8a6..76cfd891551 100644 --- a/netbox/netbox/graphql/filter_mixins.py +++ b/netbox/netbox/graphql/filter_mixins.py @@ -203,9 +203,7 @@ class BaseFilterMixin: def filter_by_filterset(self, queryset, key): filterset = self.filterset(data={key: getattr(self, key)}, queryset=queryset) if not filterset.is_valid(): - # filterset.errors is errorDict - return first error as exception - k, v = next(iter(filterset.errors.items())) - return filterset.qs.none() # We could raise validation error but strawberry logs it all to the # console i.e. raise ValidationError(f"{k}: {v[0]}") + return filterset.qs.none() return filterset.qs From ee030f8c1decbbc75f4d93f983eab61bd4f0d27c Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Tue, 27 Aug 2024 10:11:15 -0400 Subject: [PATCH 5/5] Clean up test case --- netbox/netbox/tests/test_graphql.py | 33 +++++++++++++++++------------ 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/netbox/netbox/tests/test_graphql.py b/netbox/netbox/tests/test_graphql.py index 256a4399d69..ab80c79c762 100644 --- a/netbox/netbox/tests/test_graphql.py +++ b/netbox/netbox/tests/test_graphql.py @@ -2,11 +2,11 @@ from django.test import override_settings from django.urls import reverse +from rest_framework import status from core.models import ObjectType -from rest_framework import status -from users.models import ObjectPermission, Token -from users.models import User +from dcim.models import Site, Location +from users.models import ObjectPermission from utilities.testing import disable_warnings, APITestCase, TestCase @@ -47,14 +47,16 @@ class GraphQLAPITestCase(APITestCase): @override_settings(LOGIN_REQUIRED=True) @override_settings(EXEMPT_VIEW_PERMISSIONS=['*', 'auth.user']) def test_graphql_filter_objects(self): - from dcim.models import Site, Location - - site = Site.objects.create(name='Site A', slug='site-a') - location = Location.objects.create(site=site, name='Location A1', slug='location-a1') - - url = reverse('graphql') - field_name = 'location_list' - query = '{location_list(filters: {site_id: "' + str(site.id) + '"}) {id site {id}}}' + """ + Test the operation of filters for GraphQL API requests. + """ + sites = ( + Site(name='Site 1', slug='site-1'), + Site(name='Site 2', slug='site-2'), + ) + Site.objects.bulk_create(sites) + Location.objects.create(site=sites[0], name='Location 1', slug='location-1'), + Location.objects.create(site=sites[1], name='Location 2', slug='location-2'), # Add object-level permission obj_perm = ObjectPermission( @@ -65,15 +67,18 @@ def test_graphql_filter_objects(self): obj_perm.users.add(self.user) obj_perm.object_types.add(ObjectType.objects.get_for_model(Location)) + # A valid request should return the filtered list + url = reverse('graphql') + query = '{location_list(filters: {site_id: "' + str(sites[0].pk) + '"}) {id site {id}}}' response = self.client.post(url, data={'query': query}, format="json", **self.header) self.assertHttpStatus(response, status.HTTP_200_OK) data = json.loads(response.content) self.assertNotIn('errors', data) - self.assertGreater(len(data['data']['location_list']), 0) + self.assertEqual(len(data['data']['location_list']), 1) - query = '{location_list(filters: {site_id: "' + str(site.id + 1234) + '"}) {id site {id}}}' + # An invalid request should return an empty list + query = '{location_list(filters: {site_id: "99999"}) {id site {id}}}' # Invalid site ID response = self.client.post(url, data={'query': query}, format="json", **self.header) - self.assertHttpStatus(response, status.HTTP_200_OK) data = json.loads(response.content) self.assertEqual(len(data['data']['location_list']), 0)