From 08e227d2f0f1a5da96ac9c5a82a341ba73d2e0d9 Mon Sep 17 00:00:00 2001 From: Terje Kvernes Date: Wed, 29 May 2024 20:53:27 +0200 Subject: [PATCH] Django 5 support. (#537) Django 5 support. * Add python 3.12 to github actions. * Drop python 3.7 support due to djangorestframework and django-auth-ldap having dropped support. * Drop Django 3 support due to django-filter 24.* needing 4.2+. * Add tzdada dependency (Needed for docker unit tests) * Clean up implementation of manually finding conflicts from requests via get_object_from_request. * "Fix" and test filtering for HostFilterSet. :( Note: This relies on django-rest-framework 3.14.0 and not 3.15.1 (which has formal Django 5 support)... This is due to changes in 3.15.* with regards to unique_together in models. For us, that hits Ipaddress when creating a host, leading to, where we an error as follows (this is reported as part of https://github.com/encode/django-rest-framework/issues/9358). ```python ERROR django.request:log.py:241 Internal Server Error: /api/v1/hosts/ Traceback (most recent call last): File ".../projects/uio/mreg/env/lib/python3.11/site-packages/django/core/handlers/exception.py", line 55, in inner response = get_response(request) ^^^^^^^^^^^^^^^^^^^^^ File ".../projects/uio/mreg/env/lib/python3.11/site-packages/django/core/handlers/base.py", line 197, in _get_response response = wrapped_callback(request, *callback_args, **callback_kwargs) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File ".../projects/uio/mreg/env/lib/python3.11/site-packages/django/views/decorators/csrf.py", line 65, in _view_wrapper return view_func(request, *args, **kwargs) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File ".../projects/uio/mreg/env/lib/python3.11/site-packages/django/views/generic/base.py", line 104, in view return self.dispatch(request, *args, **kwargs) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File ".../projects/uio/mreg/env/lib/python3.11/site-packages/rest_framework/views.py", line 509, in dispatch response = self.handle_exception(exc) ^^^^^^^^^^^^^^^^^^^^^^^^^^ File ".../projects/uio/mreg/env/lib/python3.11/site-packages/rest_framework/views.py", line 469, in handle_exception self.raise_uncaught_exception(exc) File ".../projects/uio/mreg/env/lib/python3.11/site-packages/rest_framework/views.py", line 480, in raise_uncaught_exception raise exc File ".../projects/uio/mreg/env/lib/python3.11/site-packages/rest_framework/views.py", line 506, in dispatch response = handler(request, *args, **kwargs) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File ".../projects/uio/mreg/mreg/api/v1/views.py", line 354, in post if ipserializer.is_valid(): ^^^^^^^^^^^^^^^^^^^^^^^ File ".../projects/uio/mreg/env/lib/python3.11/site-packages/rest_framework/serializers.py", line 223, in is_valid self._validated_data = self.run_validation(self.initial_data) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File ".../projects/uio/mreg/env/lib/python3.11/site-packages/rest_framework/serializers.py", line 444, in run_validation self.run_validators(value) File ".../projects/uio/mreg/env/lib/python3.11/site-packages/rest_framework/serializers.py", line 477, in run_validators super().run_validators(to_validate) File ".../projects/uio/mreg/env/lib/python3.11/site-packages/rest_framework/fields.py", line 553, in run_validators validator(value, self) File ".../projects/uio/mreg/env/lib/python3.11/site-packages/rest_framework/validators.py", line 169, in __call__ checked_values = [ ^ File ".../projects/uio/mreg/env/lib/python3.11/site-packages/rest_framework/validators.py", line 172, in if field in self.fields and value != getattr(serializer.instance, field) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File ".../projects/uio/mreg/env/lib/python3.11/site-packages/django/db/models/fields/related_descriptors.py", line 264, in __get__ raise self.RelatedObjectDoesNotExist( mreg.models.host.Ipaddress.host.RelatedObjectDoesNotExist: Ipaddress has no host. ``` --- .github/workflows/test.yml | 4 +- hostpolicy/api/v1/views.py | 20 ++++----- mreg/api/v1/filters.py | 6 +++ mreg/api/v1/tests/test_labels.py | 7 ++++ mreg/api/v1/tests/tests.py | 31 +++++++++++++- mreg/api/v1/tests/tests_bacnet.py | 4 +- mreg/api/v1/views.py | 14 +++---- mreg/api/v1/views_hostgroups.py | 12 +++--- mreg/api/v1/views_labels.py | 16 ++++---- mreg/managers.py | 30 ++++++++++---- mreg/mixins.py | 67 +++++++++++++++++++++++++++++-- requirements-test.txt | 20 ++++----- requirements.txt | 19 ++++----- tox.ini | 11 +++-- 14 files changed, 186 insertions(+), 75 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 044f4c51..5cd7ec9a 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -32,11 +32,11 @@ jobs: matrix: os: [ubuntu-latest] python-version: - - "3.7" - "3.8" - "3.9" - "3.10" - "3.11" + - "3.12" steps: - name: Checkout uses: actions/checkout@v3 @@ -90,11 +90,11 @@ jobs: matrix: os: [ubuntu-latest] python-version: - - "3.7" - "3.8" - "3.9" - "3.10" - "3.11" + - "3.12" steps: - name: Checkout uses: actions/checkout@v3 diff --git a/hostpolicy/api/v1/views.py b/hostpolicy/api/v1/views.py index 1b87e92e..b7f970e3 100644 --- a/hostpolicy/api/v1/views.py +++ b/hostpolicy/api/v1/views.py @@ -66,7 +66,7 @@ class HostPolicyPermissionsUpdateDestroy(M2MPermissions, permission_classes = (IsSuperOrHostPolicyAdminOrReadOnly, ) -class HostPolicyAtomList(HostPolicyAtomLogMixin, MregListCreateAPIView): +class HostPolicyAtomList(HostPolicyAtomLogMixin, LowerCaseLookupMixin, MregListCreateAPIView): queryset = HostPolicyAtom.objects.all() serializer_class = serializers.HostPolicyAtomSerializer @@ -75,11 +75,9 @@ class HostPolicyAtomList(HostPolicyAtomLogMixin, MregListCreateAPIView): filterset_class = HostPolicyAtomFilterSet def post(self, request, *args, **kwargs): - if "name" in request.data: - # Due to the overriding of get_queryset, we need to manually use lower() - if self.get_queryset().filter(name=request.data['name'].lower()).exists(): - content = {'ERROR': 'name already in use'} - return Response(content, status=status.HTTP_409_CONFLICT) + if self.get_object_from_request(request): + content = {"ERROR": "name already in use"} + return Response(content, status=status.HTTP_409_CONFLICT) return super().post(request, *args, **kwargs) @@ -99,7 +97,7 @@ def _role_prefetcher(qs): 'atoms', queryset=HostPolicyAtom.objects.order_by('name'))) -class HostPolicyRoleList(HostPolicyRoleLogMixin, MregListCreateAPIView): +class HostPolicyRoleList(HostPolicyRoleLogMixin, LowerCaseLookupMixin, MregListCreateAPIView): queryset = HostPolicyRole.objects.all() serializer_class = serializers.HostPolicyRoleSerializer @@ -108,11 +106,9 @@ class HostPolicyRoleList(HostPolicyRoleLogMixin, MregListCreateAPIView): filterset_class = HostPolicyRoleFilterSet def post(self, request, *args, **kwargs): - if "name" in request.data: - # Due to the overriding of get_queryset, we need to manually use lower() - if self.get_queryset().filter(name=request.data['name'].lower()).exists(): - content = {'ERROR': 'name already in use'} - return Response(content, status=status.HTTP_409_CONFLICT) + if self.get_object_from_request(request): + content = {"ERROR": "name already in use"} + return Response(content, status=status.HTTP_409_CONFLICT) return super().post(request, *args, **kwargs) diff --git a/mreg/api/v1/filters.py b/mreg/api/v1/filters.py index 8ea3e011..28e8b633 100644 --- a/mreg/api/v1/filters.py +++ b/mreg/api/v1/filters.py @@ -65,6 +65,12 @@ class Meta: class HostFilterSet(filters.FilterSet): + + # It's weird that we have to define the id field here, but it's necessary for the filters to work. + id = filters.NumberFilter(field_name="id") + id__in = filters.BaseInFilter(field_name="id") + id__gt = filters.NumberFilter(field_name="id", lookup_expr="gt") + id__lt = filters.NumberFilter(field_name="id", lookup_expr="lt") class Meta: model = Host fields = "__all__" diff --git a/mreg/api/v1/tests/test_labels.py b/mreg/api/v1/tests/test_labels.py index 2a00f8ee..021ea236 100644 --- a/mreg/api/v1/tests/test_labels.py +++ b/mreg/api/v1/tests/test_labels.py @@ -40,3 +40,10 @@ def test_change_label_name(self): response = self.assert_get('/api/v1/labels/') data = response.json() self.assertEqual("newname", data['results'][0]['name']) + + def test_label_name_case_insensitive(self): + """Test that label names are case insensitive.""" + self.assert_post('/api/v1/labels/', {'name': 'case_insensitive', 'description': 'Case insensitive'}) + self.assert_post_and_409('/api/v1/labels/', {'name': 'CASE_INSENSITIVE', 'description': 'Case insensitive'}) + self.assert_get_and_200('/api/v1/labels/name/case_insensitive') + self.assert_get_and_200('/api/v1/labels/name/CASE_INSENSITIVE') diff --git a/mreg/api/v1/tests/tests.py b/mreg/api/v1/tests/tests.py index 6b4a3e49..b5efc099 100644 --- a/mreg/api/v1/tests/tests.py +++ b/mreg/api/v1/tests/tests.py @@ -474,9 +474,38 @@ def setUp(self): clean_and_save(self.host_one) clean_and_save(self.host_two) + def _one_hit_and_host_one(self, query: str): + """Check that we only have one hit and it is host_one""" + response = self.assert_get(f"/hosts/?{query}") + hits = response.json()['results'] + self.assertEqual(len(hits), 1) + self.assertEqual(hits[0]['name'], self.host_one.name) + def test_hosts_get_200_ok(self): """"Getting an existing entry should return 200""" - self.assert_get('/hosts/%s' % self.host_one.name) + self.assert_get('/hosts/%s' % self.host_one.name) + + def test_host_get_200_ok_by_id(self): + """Getting an existing entry by id should return 200""" + self._one_hit_and_host_one(f"id={self.host_one.id}") + + def test_host_get_200_ok_by_id_gt_and_lt(self): + """Getting an existing entry by id should return 200""" + id = self.host_one.id + (id_after, id_before) = (id + 1, id - 1) + self._one_hit_and_host_one(f"id__gt={id_before}&id__lt={id_after}") + + def test_host_get_200_ok_by_id_in(self): + """Getting an existing entry by id should return 200""" + self._one_hit_and_host_one(f"id__in={self.host_one.id}") + + def test_host_get_200_ok_by_contact(self): + """Getting an existing entry by ip should return 200""" + self._one_hit_and_host_one(f"contact={self.host_one.contact}") + + def test_host_get_200_ok_by_name(self): + """Getting an existing entry by name should return 200""" + self._one_hit_and_host_one(f"name={self.host_one.name}") def test_hosts_get_case_insensitive_200_ok(self): """"Getting an existing entry should return 200""" diff --git a/mreg/api/v1/tests/tests_bacnet.py b/mreg/api/v1/tests/tests_bacnet.py index 1a6001ff..346b402f 100644 --- a/mreg/api/v1/tests/tests_bacnet.py +++ b/mreg/api/v1/tests/tests_bacnet.py @@ -52,14 +52,14 @@ def test_post_no_id_201_created(self): response = self.assert_post(self.basepath, post_data) response = self.assert_get(response['Location']) self.assertIn('id', response.data) - self.assertEquals(response.data['host'], self.host_two.id) + self.assertEqual(response.data['host'], self.host_two.id) def test_post_with_hostname_instead_of_id(self): post_data = {'hostname': self.host_two.name} response = self.assert_post(self.basepath, post_data) response = self.assert_get(response['Location']) self.assertIn('id', response.data) - self.assertEquals(response.data['host'], self.host_two.id) + self.assertEqual(response.data['host'], self.host_two.id) def test_post_without_host_400(self): """Posting a new entry without specifying a host should return 400 bad request""" diff --git a/mreg/api/v1/views.py b/mreg/api/v1/views.py index 3342b857..6b21e932 100644 --- a/mreg/api/v1/views.py +++ b/mreg/api/v1/views.py @@ -343,13 +343,13 @@ def post(self, request, *args, **kwargs): ipdata = {"host": host.pk, "ipaddress": ipkey} ip = Ipaddress() ipserializer = IpaddressSerializer(ip, data=ipdata) - if ipserializer.is_valid(raise_exception=True): - self.perform_create(ipserializer) - location = request.path + host.name - return Response( - status=status.HTTP_201_CREATED, - headers={"Location": location}, - ) + ipserializer.is_valid(raise_exception=True) + self.perform_create(ipserializer) + location = request.path + host.name + return Response( + status=status.HTTP_201_CREATED, + headers={"Location": location}, + ) else: host = Host() hostserializer = HostSerializer(host, data=hostdata) diff --git a/mreg/api/v1/views_hostgroups.py b/mreg/api/v1/views_hostgroups.py index d2c8055f..d8680461 100644 --- a/mreg/api/v1/views_hostgroups.py +++ b/mreg/api/v1/views_hostgroups.py @@ -63,7 +63,7 @@ def _hostgroup_prefetcher(qs): 'owners', queryset=Group.objects.order_by('name'))) -class HostGroupList(HostGroupLogMixin, MregListCreateAPIView): +class HostGroupList(HostGroupLogMixin, LowerCaseLookupMixin, MregListCreateAPIView): """ get: Lists all hostgroups in use. @@ -76,14 +76,12 @@ class HostGroupList(HostGroupLogMixin, MregListCreateAPIView): serializer_class = serializers.HostGroupSerializer permission_classes = (IsSuperOrGroupAdminOrReadOnly, ) filterset_class = HostGroupFilterSet + lookup_field = 'name' def post(self, request, *args, **kwargs): - if "name" in request.data: - # We need to manually use lower() here due to the overriden get_queryset() - if self.get_queryset().filter(name=request.data['name'].lower()).exists(): - content = {'ERROR': 'hostgroup name already in use'} - return Response(content, status=status.HTTP_409_CONFLICT) - self.lookup_field = 'name' + if self.get_object_from_request(request): + content = {'ERROR': 'hostgroup name already in use'} + return Response(content, status=status.HTTP_409_CONFLICT) return super().post(request, *args, **kwargs) diff --git a/mreg/api/v1/views_labels.py b/mreg/api/v1/views_labels.py index 9b14ec2a..4f158b44 100644 --- a/mreg/api/v1/views_labels.py +++ b/mreg/api/v1/views_labels.py @@ -11,18 +11,17 @@ from .filters import LabelFilterSet -class LabelList(MregListCreateAPIView): +class LabelList(MregListCreateAPIView, LowerCaseLookupMixin): queryset = Label.objects.all() serializer_class = serializers.LabelSerializer permission_classes = (IsSuperOrAdminOrReadOnly,) filterset_class = LabelFilterSet + lookup_field = "name" - def post(self, request, *args, **kwargs): - if "name" in request.data: - if self.get_queryset().filter(name=request.data["name"]).exists(): - content = {"ERROR": "Label name already in use"} - return Response(content, status=status.HTTP_409_CONFLICT) - self.lookup_field = "name" + def post(self, request, *args, **kwargs): + if self.get_object_from_request(request): + content = {"ERROR": "Label name already in use"} + return Response(content, status=status.HTTP_409_CONFLICT) return super().post(request, *args, **kwargs) @@ -43,8 +42,9 @@ class LabelDetail(LowerCaseLookupMixin, MregRetrieveUpdateDestroyAPIView): permission_classes = (IsSuperOrAdminOrReadOnly,) -class LabelDetailByName(MregRetrieveUpdateDestroyAPIView): +class LabelDetailByName(LowerCaseLookupMixin, MregRetrieveUpdateDestroyAPIView): queryset = Label.objects.all() serializer_class = serializers.LabelSerializer permission_classes = (IsSuperOrAdminOrReadOnly,) + filterset_class = LabelFilterSet lookup_field = "name" diff --git a/mreg/managers.py b/mreg/managers.py index d2c5299c..3d4dbc5f 100644 --- a/mreg/managers.py +++ b/mreg/managers.py @@ -1,13 +1,21 @@ + +from typing import Any, Dict, Type from django.db import models from .fields import LowerCaseCharField -class LowerCaseManager(models.Manager): +class LowerCaseManager(models.Manager[Any]): """A manager that lowercases all values of LowerCaseCharFields in filter/exclude/get calls.""" @property def lowercase_fields(self): + """A list of field names that are LowerCaseCharFields. + + Note: This is a cached property to avoid recalculating the list every time it is accessed. + We are making the assumption that the model's fields do not change during runtime... + """ + if not hasattr(self, "_lowercase_fields_cache"): self._lowercase_fields_cache = [ field.name @@ -16,8 +24,10 @@ def lowercase_fields(self): ] return self._lowercase_fields_cache - def _lowercase_fields(self, **kwargs): - lower_kwargs = {} + def _lowercase_fields(self, **kwargs: Dict[str, Any]) -> Dict[str, Any]: + """Lowercase all values of LowerCaseCharFields in kwargs.""" + + lower_kwargs: Dict[str, Any] = {} for key, value in kwargs.items(): field_name = key.split("__")[0] if field_name in self.lowercase_fields and isinstance(value, str): @@ -25,18 +35,24 @@ def _lowercase_fields(self, **kwargs): lower_kwargs[key] = value return lower_kwargs - def filter(self, **kwargs): + def filter(self, **kwargs: Dict[str, Any]): + """Lowercase all values of LowerCaseCharFields in kwargs during filtering.""" return super().filter(**self._lowercase_fields(**kwargs)) - def exclude(self, **kwargs): + def exclude(self, **kwargs: Dict[str, Any]): + """Lowercase all values of LowerCaseCharFields in kwargs during excluding.""" return super().exclude(**self._lowercase_fields(**kwargs)) - def get(self, **kwargs): + def get(self, **kwargs: Dict[str, Any]): + """Lowercase all values of LowerCaseCharFields in kwargs during get.""" return super().get(**self._lowercase_fields(**kwargs)) -def lower_case_manager_factory(base_manager): +def lower_case_manager_factory(base_manager: Type[models.Manager[Any]]): + """A factory function to create a LowerCaseManager for a given base_manager.""" + class LowerCaseBaseManager(base_manager, LowerCaseManager): + """A manager that lowercases all values of LowerCaseCharFields in filter/exclude/get calls.""" pass return LowerCaseBaseManager diff --git a/mreg/mixins.py b/mreg/mixins.py index 455785d4..df577327 100644 --- a/mreg/mixins.py +++ b/mreg/mixins.py @@ -1,15 +1,40 @@ +from typing import Protocol, Any, Dict, Union from django.shortcuts import get_object_or_404 +from django.http import HttpRequest +from rest_framework.request import Request +from django.db.models import QuerySet + + +class DetailViewProtocol(Protocol): + """Protocol that defines the expected methods and attributes for the mixin.""" + + request: HttpRequest + kwargs: Dict[str, Any] + lookup_field: str + + def get_queryset(self) -> QuerySet[Any]: + """Method to get the queryset.""" + ... + + def filter_queryset(self, queryset: QuerySet[Any]) -> QuerySet[Any]: + """Method to filter the queryset.""" + ... + + def check_object_permissions(self, request: HttpRequest, obj: Any) -> None: + """Method to check object permissions.""" + ... class LowerCaseLookupMixin: """A mixin to make DRF detail view lookup case insensitive.""" - def get_object(self): + def get_object(self: DetailViewProtocol) -> Any: """Returns the object the view is displaying. - This method is overriden to make the lookup case insensitive. - """ + This method is overridden to make the lookup case insensitive. + :returns: The object the view is displaying. + """ queryset = self.filter_queryset(self.get_queryset()) filter_kwargs = {self.lookup_field: self.kwargs[self.lookup_field].lower()} @@ -18,4 +43,38 @@ def get_object(self): # May raise a permission denied self.check_object_permissions(self.request, obj) - return obj + return obj + + def get_object_from_request( + self: DetailViewProtocol, request: Request, field: Union[str, None] = None + ) -> Union[Any, None]: + """Return an object from the queryset based on data from the request, if any. + + The object is found in the queryset by querying with field = request.data[field]. If the field + is not defined, and the view offers a self.lookup_field, that field is used as a fallback. + + Note: This is part of the LowerCaseLookupMixin, so the value of the field in request.data will + be lowercased when querying. + + :param request: The request object. + :param field: The field to use for the lookup. If None, the view's lookup_field is used. + + :returns: The object from the queryset or None. + + :raises AttributeError: If no field is specified and the view does not have a lookup_field defined. + """ + if not field and not self.lookup_field: + raise AttributeError("If not specifying a field, the view must have lookup_field defined.") + + lfield: str = field if field else self.lookup_field + + if not request.data or not isinstance(request.data, dict): + return None + + if self.lookup_field not in request.data: + return None + + queryset = self.filter_queryset(self.get_queryset()) + filter_kwargs: Dict[str, str] = {lfield: request.data[lfield].lower()} + + return queryset.filter(**filter_kwargs).first() \ No newline at end of file diff --git a/requirements-test.txt b/requirements-test.txt index 736aa133..1bd77db6 100644 --- a/requirements-test.txt +++ b/requirements-test.txt @@ -1,17 +1,17 @@ -Django>3 +Django>4 djangorestframework==3.14.0 -django-auth-ldap==4.0.0 +django-auth-ldap==4.8.0 django-logging-json==1.15 -django-netfields==1.2.4 -django-filter==23.1 -structlog==23.1.0 -rich==13.4.2 +django-netfields==1.3.2 +django-filter==24.2 +structlog==24.1.0 +rich==13.7.1 gunicorn==22.0.0 idna==3.7 -psycopg2-binary==2.9.5 -pika==1.3.1 -sentry-sdk==1.15.0 - +psycopg2-binary==2.9.9 +pika==1.3.2 +sentry-sdk==2.3.1 +tzdata==2024.1 # Testing framwork tox pytest diff --git a/requirements.txt b/requirements.txt index f0fd459b..190c1b14 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,15 +1,16 @@ -Django==3.2.25 +Django==5.0.6 djangorestframework==3.14.0 -django-auth-ldap==4.0.0 -django-netfields==1.2.4 -django-filter==23.1 -structlog==23.1.0 -rich==13.4.2 +django-auth-ldap==4.8.0 +django-netfields==1.3.2 +django-filter==24.2 +structlog==24.1.0 +rich==13.7.1 gunicorn==22.0.0 idna==3.7 -psycopg2-binary==2.9.5 -pika==1.3.1 -sentry-sdk==1.15.0 +psycopg2-binary==2.9.9 +pika==1.3.2 +sentry-sdk==2.3.1 +tzdata==2024.1 # For OpenAPI schema generation. uritemplate pyyaml diff --git a/tox.ini b/tox.ini index 3b1de8b6..d1ac0dab 100644 --- a/tox.ini +++ b/tox.ini @@ -4,20 +4,18 @@ skip_missing_interpreters = true envlist = lint coverage - python{37,38,39}-django32 - python{38,39,310}-django40 - python{38,39,310,311}-django41 - python{38,39,310,311}-django42 + python{38,39,310,311,312}-django42 + python{310,311,312}-django50 toxworkdir = {env:TOX_WORKDIR:.tox} [gh-actions] python = - 3.7: python37 3.8: python38 3.9: python39 3.10: python310 3.11: python311 + 3.12: python312 [testenv] setenv = @@ -30,12 +28,13 @@ deps = django40: Django>=4.0,<4.1 django41: Django>=4.1,<4.2 django42: Django>=4.2.a1,<4.3 + django50: Django>=5.0a1,<5.1 basepython = - python37: python3.7 python38: python3.8 python39: python3.9 python310: python3.10 python311: python3.11 + python312: python3.12 allowlist_externals = coverage python