Improved documentation and tests of ordering filter (#1334)

Author: sliverc

docs/usage.md

@@ -133,12 +133,28 @@ simply don't use this filter backend.
 
 #### OrderingFilter
 
-`OrderingFilter` implements the [JSON:API `sort`](https://jsonapi.org/format/#fetching-sorting) and uses
-DRF's [ordering filter](https://www.django-rest-framework.org/api-guide/filtering/#orderingfilter).
+`OrderingFilter` implements [JSON:API `sorting`](https://jsonapi.org/format/#fetching-sorting) and uses
+Django REST framework's [ordering filter](https://www.django-rest-framework.org/api-guide/filtering/#orderingfilter) as a basis.
+
+Define `ordering_fields` on your view to define what fields are supported for sorting. If not defined per default only direct readable fields
+will be allowed.
+
+Example:
+
+```python
+class BookViewSet(views.ModelViewSet):
+    queryset = Book.objects.all()
+    ordering_fields = ['name', 'author__name']
+```
+
+For sorting of relationship fields the dot notation can be used. For instance in above example to
+sort by name of author, `sort` query parameter needs to be `?sort=author.name`.
+
+Unlike in DRF, it needs to be noted that in JSON:API when the `sort` query parameter
+contains invalid field names, a `400 Bad Request` will be returned.
+
+A error response will look like the following.
 
-Per the JSON:API specification, "If the server does not support sorting as specified in the query parameter `sort`,
-it **MUST** return `400 Bad Request`." For example, for `?sort=abc,foo,def` where `foo` is a valid
-field name and the other two are not valid:
 ```json
 {
     "errors": [
@@ -153,9 +169,6 @@ field name and the other two are not valid:
 }
 ```
 
-If you want to silently ignore bad sort fields, just use `rest_framework.filters.OrderingFilter` and set
-`ordering_param` to `sort`.
-
 #### DjangoFilterBackend
 
 `DjangoFilterBackend` implements a Django ORM-style [JSON:API `filter`](https://jsonapi.org/format/#fetching-filtering)

example/tests/test_filters.py

@@ -1,7 +1,7 @@
 from rest_framework.reverse import reverse
 from rest_framework.test import APITestCase
 
-from example.models import Blog, Entry
+from example.models import Entry
 
 
 class DJATestFilters(APITestCase):
@@ -13,112 +13,10 @@ class DJATestFilters(APITestCase):
 
     def setUp(self):
         self.entries = Entry.objects.all()
-        self.blogs = Blog.objects.all()
         self.url = reverse("nopage-entry-list")
         self.fs_url = reverse("filterset-entry-list")
         self.no_fs_url = reverse("nofilterset-entry-list")
 
-    def test_sort(self):
-        """
-        test sort
-        """
-        response = self.client.get(self.url, data={"sort": "headline"})
-        self.assertEqual(
-            response.status_code, 200, msg=response.content.decode("utf-8")
-        )
-        dja_response = response.json()
-        headlines = [c["attributes"]["headline"] for c in dja_response["data"]]
-        sorted_headlines = sorted(headlines)
-        self.assertEqual(headlines, sorted_headlines)
-
-    def test_sort_reverse(self):
-        """
-        confirm switching the sort order actually works
-        """
-        response = self.client.get(self.url, data={"sort": "-headline"})
-        self.assertEqual(
-            response.status_code, 200, msg=response.content.decode("utf-8")
-        )
-        dja_response = response.json()
-        headlines = [c["attributes"]["headline"] for c in dja_response["data"]]
-        sorted_headlines = sorted(headlines)
-        self.assertNotEqual(headlines, sorted_headlines)
-
-    def test_sort_double_negative(self):
-        """
-        what if they provide multiple `-`'s? It's OK.
-        """
-        response = self.client.get(self.url, data={"sort": "--headline"})
-        self.assertEqual(
-            response.status_code, 200, msg=response.content.decode("utf-8")
-        )
-        dja_response = response.json()
-        headlines = [c["attributes"]["headline"] for c in dja_response["data"]]
-        sorted_headlines = sorted(headlines)
-        self.assertNotEqual(headlines, sorted_headlines)
-
-    def test_sort_invalid(self):
-        """
-        test sort of invalid field
-        """
-        response = self.client.get(
-            self.url, data={"sort": "nonesuch,headline,-not_a_field"}
-        )
-        self.assertEqual(
-            response.status_code, 400, msg=response.content.decode("utf-8")
-        )
-        dja_response = response.json()
-        self.assertEqual(
-            dja_response["errors"][0]["detail"],
-            "invalid sort parameters: nonesuch,-not_a_field",
-        )
-
-    def test_sort_camelcase(self):
-        """
-        test sort of camelcase field name
-        """
-        response = self.client.get(self.url, data={"sort": "bodyText"})
-        self.assertEqual(
-            response.status_code, 200, msg=response.content.decode("utf-8")
-        )
-        dja_response = response.json()
-        blog_ids = [(c["attributes"]["bodyText"] or "") for c in dja_response["data"]]
-        sorted_blog_ids = sorted(blog_ids)
-        self.assertEqual(blog_ids, sorted_blog_ids)
-
-    def test_sort_underscore(self):
-        """
-        test sort of underscore field name
-        Do we allow this notation in a search even if camelcase is in effect?
-        "Be conservative in what you send, be liberal in what you accept"
-                   --  https://en.wikipedia.org/wiki/Robustness_principle
-        """
-        response = self.client.get(self.url, data={"sort": "body_text"})
-        self.assertEqual(
-            response.status_code, 200, msg=response.content.decode("utf-8")
-        )
-        dja_response = response.json()
-        blog_ids = [(c["attributes"]["bodyText"] or "") for c in dja_response["data"]]
-        sorted_blog_ids = sorted(blog_ids)
-        self.assertEqual(blog_ids, sorted_blog_ids)
-
-    def test_sort_related(self):
-        """
-        test sort via related field using JSON:API path `.` and django orm `__` notation.
-        ORM relations must be predefined in the View's .ordering_fields attr
-        """
-        for datum in ("blog__id", "blog.id"):
-            response = self.client.get(self.url, data={"sort": datum})
-            self.assertEqual(
-                response.status_code, 200, msg=response.content.decode("utf-8")
-            )
-            dja_response = response.json()
-            blog_ids = [
-                c["relationships"]["blog"]["data"]["id"] for c in dja_response["data"]
-            ]
-            sorted_blog_ids = sorted(blog_ids)
-            self.assertEqual(blog_ids, sorted_blog_ids)
-
     def test_filter_exact(self):
         """
         filter for an exact match

requirements/requirements-testing.txt

@@ -1,5 +1,6 @@
 factory-boy==3.3.3
 Faker==40.37.0
+inline-snapshot==0.36.0
 pytest==9.1.1
 pytest-cov==7.1.0
 pytest-django==4.14.0

tests/conftest.py

@@ -34,7 +34,7 @@ def use_rest_framework_json_api_defaults(settings):
 
 @pytest.fixture
 def model(db):
-    return BasicModel.objects.create(text="Model")
+    return BasicModel.objects.create(text="Model", long_text="LongText")
 
 
 @pytest.fixture

tests/models.py

@@ -13,6 +13,7 @@ class Meta:
 
 class BasicModel(DJAModel):
     text = models.CharField(max_length=100)
+    long_text = models.CharField(max_length=100)
 
     class Meta:
         ordering = ("id",)

tests/serializers.py

@@ -14,7 +14,7 @@
 
 class BasicModelSerializer(serializers.ModelSerializer):
     class Meta:
-        fields = ("text",)
+        fields = ("text", "long_text")
         model = BasicModel
 
 

tests/test_filters.py

@@ -0,0 +1,106 @@
+import pytest
+from django.urls import reverse
+from rest_framework import status
+
+from tests.models import BasicModel, ForeignKeySource, ForeignKeyTarget
+
+
+@pytest.mark.urls("tests.urls")
+class TestOrderingFilter:
+    @pytest.mark.parametrize(
+        "sort_param,expected",
+        [
+            # ascending sort
+            ("text", ["1", "2", "3"]),
+            # descending sort
+            ("-text", ["3", "2", "1"]),
+            # descending sort with two dashes should work
+            ("--text", ["3", "2", "1"]),
+        ],
+    )
+    def test_sort(self, db, client, sort_param, expected):
+        BasicModel.objects.create(text="1")
+        BasicModel.objects.create(text="3")
+        BasicModel.objects.create(text="2")
+
+        url = reverse("basicmodel-list")
+        response = client.get(url, {"sort": sort_param, "page[size]": 3})
+        result = response.json()
+
+        assert response.status_code == status.HTTP_200_OK
+        assert [d["attributes"]["text"] for d in result["data"]] == expected
+
+    @pytest.mark.parametrize(
+        "sort_param,error",
+        [
+            (
+                "nonesuch,text,-not-a-field",
+                ["invalid sort parameters: nonesuch,-not-a-field"],
+            )
+        ],
+    )
+    def test_sort_invalid(self, db, client, sort_param, error):
+        url = reverse("basicmodel-list")
+        response = client.get(url, {"sort": sort_param, "page[size]": 3})
+        result = response.json()
+
+        assert response.status_code == status.HTTP_400_BAD_REQUEST
+        assert [e["detail"] for e in result["errors"]] == error
+
+    @pytest.mark.parametrize(
+        "format_type,sort_param,expected",
+        [
+            (False, "long_text", ["1", "2", "3"]),
+            (False, "-long_text", ["3", "2", "1"]),
+            ("underscore", "long_text", ["1", "2", "3"]),
+            ("underscore", "-long_text", ["3", "2", "1"]),
+            ("dasherize", "long-text", ["1", "2", "3"]),
+            ("dasherize", "-long-text", ["3", "2", "1"]),
+            ("camelize", "longText", ["1", "2", "3"]),
+            ("camelize", "-longText", ["3", "2", "1"]),
+            ("capitalize", "LongText", ["1", "2", "3"]),
+            ("capitalize", "-LongText", ["3", "2", "1"]),
+        ],
+    )
+    def test_sort_format(self, db, client, settings, format_type, sort_param, expected):
+        settings.JSON_API_FORMAT_FIELD_NAMES = format_type
+
+        BasicModel.objects.create(long_text="1")
+        BasicModel.objects.create(long_text="3")
+        BasicModel.objects.create(long_text="2")
+
+        url = reverse("basicmodel-list")
+        response = client.get(url, {"sort": sort_param, "page[size]": 3})
+        result = response.json()
+
+        assert response.status_code == status.HTTP_200_OK
+        values = [d["attributes"][sort_param.lstrip("-")] for d in result["data"]]
+        assert values == expected
+
+    @pytest.mark.parametrize(
+        "sort_param,expected",
+        [
+            ("target.name,name", ["1", "2", "3"]),
+            ("-target.name,-name", ["3", "2", "1"]),
+            ("target__name,name", ["1", "2", "3"]),
+            ("-target__name,-name", ["3", "2", "1"]),
+        ],
+    )
+    def test_sort_related(self, db, client, sort_param, expected):
+        ForeignKeySource.objects.create(
+            name="3", target=ForeignKeyTarget.objects.create(name="3")
+        )
+        ForeignKeySource.objects.create(
+            name="1", target=ForeignKeyTarget.objects.create(name="1")
+        )
+        ForeignKeySource.objects.create(
+            name="2", target=ForeignKeyTarget.objects.create(name="1")
+        )
+
+        url = reverse("foreignkeysource-list")
+        response = client.get(url, {"sort": sort_param, "page[size]": 3})
+        result = response.json()
+
+        assert response.status_code == status.HTTP_200_OK
+        values = [d["attributes"]["name"] for d in result["data"]]
+        assert values == expected