From 8935554a117c0124dcdf8ce4671b43cea17b83c5 Mon Sep 17 00:00:00 2001 From: Stanislav Khlud Date: Fri, 10 Jan 2025 16:50:45 +0700 Subject: [PATCH 1/5] Fix issues with query params parsing --- import_export_extensions/api/views/export_job.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/import_export_extensions/api/views/export_job.py b/import_export_extensions/api/views/export_job.py index a2d3e9e..d9524bb 100644 --- a/import_export_extensions/api/views/export_job.py +++ b/import_export_extensions/api/views/export_job.py @@ -157,12 +157,11 @@ def get_export_create_serializer_class(self): def start(self, request: Request): """Validate request data and start ExportJob.""" - query_params = dict(request.query_params) - ordering = query_params.pop("ordering", self.ordering) + ordering = request.query_params.getlist("ordering", default=()) serializer = self.get_serializer( data=request.data, ordering=ordering, - filter_kwargs=query_params, + filter_kwargs=request.query_params, ) serializer.is_valid(raise_exception=True) export_job = serializer.save() From 14e3c31d794380b880b991bfedb97b964d740743 Mon Sep 17 00:00:00 2001 From: Stanislav Khlud Date: Fri, 10 Jan 2025 16:54:44 +0700 Subject: [PATCH 2/5] Make `get_queryset` consistent for start actions --- import_export_extensions/api/mixins.py | 3 +++ import_export_extensions/api/views/export_job.py | 3 +++ 2 files changed, 6 insertions(+) diff --git a/import_export_extensions/api/mixins.py b/import_export_extensions/api/mixins.py index dc4bf9a..30e8f44 100644 --- a/import_export_extensions/api/mixins.py +++ b/import_export_extensions/api/mixins.py @@ -3,6 +3,9 @@ class LimitQuerySetToCurrentUserMixin: def get_queryset(self): """Return user's jobs.""" + if self.action == "start": + # To make it consistent and for better support of drf-spectacular + return super().get_queryset() return ( super() .get_queryset() diff --git a/import_export_extensions/api/views/export_job.py b/import_export_extensions/api/views/export_job.py index d9524bb..fd2a6f1 100644 --- a/import_export_extensions/api/views/export_job.py +++ b/import_export_extensions/api/views/export_job.py @@ -125,6 +125,9 @@ class ExportJobViewSet( def get_queryset(self): """Filter export jobs by resource used in viewset.""" + if self.action == "start": + # To make it consistent and for better support of drf-spectacular + return super().get_queryset() return super().get_queryset().filter( resource_path=self.resource_class.class_path, ) From b02d8891b4f0935fda6f91feaead5c79c15183eb Mon Sep 17 00:00:00 2001 From: Stanislav Khlud Date: Fri, 10 Jan 2025 16:57:03 +0700 Subject: [PATCH 3/5] Update changelog --- HISTORY.rst | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/HISTORY.rst b/HISTORY.rst index 1748999..d4384e7 100644 --- a/HISTORY.rst +++ b/HISTORY.rst @@ -2,6 +2,12 @@ History ======= +UNRELEASED +------------------ + +* Fix issues with query params parsing +* Make `get_queryset` consistent for start actions + 1.3.0 (2025-01-09) ------------------ From 04a851a45df71eed65f6adb3972361219e36a344 Mon Sep 17 00:00:00 2001 From: Stanislav Khlud Date: Fri, 10 Jan 2025 17:07:13 +0700 Subject: [PATCH 4/5] Add # pragma: no cover to unreachable places They could be reached by custom integrations like drf-spectacular --- import_export_extensions/api/mixins.py | 2 +- import_export_extensions/api/views/export_job.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/import_export_extensions/api/mixins.py b/import_export_extensions/api/mixins.py index 30e8f44..c0952a1 100644 --- a/import_export_extensions/api/mixins.py +++ b/import_export_extensions/api/mixins.py @@ -5,7 +5,7 @@ def get_queryset(self): """Return user's jobs.""" if self.action == "start": # To make it consistent and for better support of drf-spectacular - return super().get_queryset() + return super().get_queryset() # pragma: no cover return ( super() .get_queryset() diff --git a/import_export_extensions/api/views/export_job.py b/import_export_extensions/api/views/export_job.py index fd2a6f1..f98a4a4 100644 --- a/import_export_extensions/api/views/export_job.py +++ b/import_export_extensions/api/views/export_job.py @@ -127,7 +127,7 @@ def get_queryset(self): """Filter export jobs by resource used in viewset.""" if self.action == "start": # To make it consistent and for better support of drf-spectacular - return super().get_queryset() + return super().get_queryset() # pragma: no cover return super().get_queryset().filter( resource_path=self.resource_class.class_path, ) From 2fdcecaf8e94046898f270daaa739c29579bb564 Mon Sep 17 00:00:00 2001 From: Stanislav Khlud Date: Mon, 13 Jan 2025 12:12:12 +0700 Subject: [PATCH 5/5] Add tests for filter/ordering params in API --- .../api/views/export_job.py | 4 +- pyproject.toml | 2 + test_project/fake_app/filters.py | 14 ++- .../integration_tests/test_api/test_export.py | 104 ++++++++++++++++-- 4 files changed, 109 insertions(+), 15 deletions(-) diff --git a/import_export_extensions/api/views/export_job.py b/import_export_extensions/api/views/export_job.py index f98a4a4..16b3061 100644 --- a/import_export_extensions/api/views/export_job.py +++ b/import_export_extensions/api/views/export_job.py @@ -160,7 +160,9 @@ def get_export_create_serializer_class(self): def start(self, request: Request): """Validate request data and start ExportJob.""" - ordering = request.query_params.getlist("ordering", default=()) + ordering = request.query_params.get("ordering", "") + if ordering: + ordering = ordering.split(",") serializer = self.get_serializer( data=request.data, ordering=ordering, diff --git a/pyproject.toml b/pyproject.toml index b03df63..68bb12a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -246,6 +246,8 @@ ignore = [ "C408", # https://docs.astral.sh/ruff/rules/mutable-class-default "RUF012", + # https://docs.astral.sh/ruff/rules/assignment-in-assert/ + "RUF018", # https://docs.astral.sh/ruff/rules/raise-vanilla-args "TRY003", # https://docs.astral.sh/ruff/rules/try-consider-else diff --git a/test_project/fake_app/filters.py b/test_project/fake_app/filters.py index 139b286..ec32edd 100644 --- a/test_project/fake_app/filters.py +++ b/test_project/fake_app/filters.py @@ -8,7 +8,13 @@ class ArtistFilterSet(filters.FilterSet): class Meta: model = Artist - fields = [ - "id", - "name", - ] + fields = { + "id": ( + "exact", + "in", + ), + "name": ( + "exact", + "in", + ), + } diff --git a/test_project/tests/integration_tests/test_api/test_export.py b/test_project/tests/integration_tests/test_api/test_export.py index 7a23afc..96f9cec 100644 --- a/test_project/tests/integration_tests/test_api/test_export.py +++ b/test_project/tests/integration_tests/test_api/test_export.py @@ -1,3 +1,5 @@ +import collections.abc + from django.contrib.auth.models import User from django.urls import reverse @@ -9,33 +11,115 @@ @pytest.mark.django_db(transaction=True) +def test_export_api_creates_export_job( + admin_api_client: test.APIClient, +): + """Ensure export start API creates new export job.""" + response = admin_api_client.post( + path=reverse("export-artist-start"), + data={ + "file_format": "csv", + }, + ) + assert response.status_code == status.HTTP_201_CREATED, response.data + assert response.data["export_status"] == ExportJob.ExportStatus.CREATED + assert ExportJob.objects.filter(id=response.data["id"]).exists() + + @pytest.mark.parametrize( - argnames=["export_url"], + argnames=[ + "filter_query", + "filter_name", + "filter_value", + ], argvalues=[ pytest.param( - reverse("export-artist-start"), - id="Url without filter_kwargs", + "name=Artist", + "name", + "Artist", + id="Simple str filter", + ), + pytest.param( + "id=1", + "id", + "1", + id="Simple int filter", ), pytest.param( - f"{reverse('export-artist-start')}?name=Artist", - id="Url with valid filter_kwargs", + "name__in=Some,Artist", + "name__in", + "Some,Artist", + id="Simple `in` filter", ), ], ) -def test_export_api_creates_export_job( +def test_export_api_filtering( admin_api_client: test.APIClient, - export_url: str, + filter_query: str, + filter_name: str, + filter_value: str, ): - """Ensure export start API creates new export job.""" + """Ensure export start API passes filter kwargs correctly.""" response = admin_api_client.post( - path=export_url, + path=f"{reverse('export-artist-start')}?{filter_query}", data={ "file_format": "csv", }, ) assert response.status_code == status.HTTP_201_CREATED, response.data assert response.data["export_status"] == ExportJob.ExportStatus.CREATED - assert ExportJob.objects.filter(id=response.data["id"]).exists() + assert ( + export_job := ExportJob.objects.filter(id=response.data["id"]).first() + ) + assert ( + export_job.resource_kwargs["filter_kwargs"][filter_name] + == filter_value + ), export_job.resource_kwargs["filter_kwargs"] + + +@pytest.mark.parametrize( + argnames=[ + "ordering_query", + "ordering_value", + ], + argvalues=[ + pytest.param( + "ordering=name", + [ + "name", + ], + id="One field", + ), + pytest.param( + "ordering=name%2C-id", + [ + "name", + "-id", + ], + id="Many fields", + ), + ], +) +def test_export_api_ordering( + admin_api_client: test.APIClient, + ordering_query: str, + ordering_value: collections.abc.Sequence[str], +): + """Ensure export start API passes ordering correctly.""" + response = admin_api_client.post( + path=f"{reverse('export-artist-start')}?{ordering_query}", + data={ + "file_format": "csv", + }, + ) + assert response.status_code == status.HTTP_201_CREATED, response.data + assert response.data["export_status"] == ExportJob.ExportStatus.CREATED + assert ( + export_job := ExportJob.objects.filter(id=response.data["id"]).first() + ) + assert ( + export_job.resource_kwargs["ordering"] == ordering_value + ), export_job.resource_kwargs["ordering"] @pytest.mark.django_db(transaction=True)