sub-class OrderingFilter to ensure nulls ordered last (#5800)

kitsune/kpi/api.py

@@ -10,7 +10,7 @@ from django.db.models.functions import Now
 
 import django_filters
 from django_filters.rest_framework import DjangoFilterBackend
-from rest_framework import filters, serializers, viewsets
+from rest_framework import serializers, viewsets
 from rest_framework.views import APIView
 from rest_framework.response import Response
 
@@ -29,6 +29,7 @@ from kitsune.kpi.models import (
     EXIT_SURVEY_DONT_KNOW_CODE,
 )
 from kitsune.questions.models import Question, Answer, AnswerVote
+from kitsune.sumo.api_utils import OrderingFilter
 from kitsune.wiki.models import HelpfulVote
 from functools import reduce
 
@@ -491,7 +492,7 @@ class CohortViewSet(viewsets.ReadOnlyModelViewSet):
     filterset_class = CohortFilter
     filter_backends = [
         DjangoFilterBackend,
-        filters.OrderingFilter,
+        OrderingFilter,
     ]
     ordering_fields = [
         "start",

kitsune/questions/api.py

@@ -6,7 +6,7 @@ import django_filters
 from django import forms
 from django.db.models import Q
 from django_filters.rest_framework import DjangoFilterBackend
-from rest_framework import filters, pagination, permissions, serializers, status, viewsets
+from rest_framework import pagination, permissions, serializers, status, viewsets
 from rest_framework.decorators import action
 from rest_framework.response import Response
 from taggit.models import Tag
@@ -26,6 +26,7 @@ from kitsune.sumo.api_utils import (
     DateTimeUTCField,
     GenericAPIException,
     OnlyCreatorEdits,
+    OrderingFilter,
     SplitSourceField,
 )
 from kitsune.sumo.utils import is_ratelimited
@@ -246,7 +247,7 @@ class QuestionViewSet(viewsets.ModelViewSet):
     filterset_class = QuestionFilter
     filter_backends = [
         DjangoFilterBackend,
-        filters.OrderingFilter,
+        OrderingFilter,
     ]
     ordering_fields = [
         "id",
@@ -486,7 +487,7 @@ class AnswerViewSet(viewsets.ModelViewSet):
     filterset_class = AnswerFilter
     filter_backends = [
         DjangoFilterBackend,
-        filters.OrderingFilter,
+        OrderingFilter,
     ]
     filterset_fields = [
         "question",

kitsune/questions/tests/test_api.py

@@ -350,18 +350,29 @@ class TestQuestionViewSet(TestCase):
     def test_ordering(self):
         q1 = QuestionFactory()
         q2 = QuestionFactory()
+        q3 = QuestionFactory()
+        AnswerFactory(question=q1)
+        AnswerFactory(question=q2)
 
         res = self.client.get(reverse("question-list"))
-        self.assertEqual(res.data["results"][0]["id"], q2.id)
-        self.assertEqual(res.data["results"][1]["id"], q1.id)
+        self.assertEqual(res.data["results"][0]["id"], q3.id)
+        self.assertEqual(res.data["results"][1]["id"], q2.id)
+        self.assertEqual(res.data["results"][2]["id"], q1.id)
 
         res = self.client.get(reverse("question-list") + "?ordering=id")
         self.assertEqual(res.data["results"][0]["id"], q1.id)
         self.assertEqual(res.data["results"][1]["id"], q2.id)
+        self.assertEqual(res.data["results"][2]["id"], q3.id)
 
-        res = self.client.get(reverse("question-list") + "?ordering=-id")
+        res = self.client.get(reverse("question-list") + "?ordering=-last_answer")
         self.assertEqual(res.data["results"][0]["id"], q2.id)
         self.assertEqual(res.data["results"][1]["id"], q1.id)
+        self.assertEqual(res.data["results"][2]["id"], q3.id)
+
+        res = self.client.get(reverse("question-list") + "?ordering=last_answer")
+        self.assertEqual(res.data["results"][0]["id"], q1.id)
+        self.assertEqual(res.data["results"][1]["id"], q2.id)
+        self.assertEqual(res.data["results"][2]["id"], q3.id)
 
     def test_filter_product_with_slug(self):
         p1 = ProductFactory()

kitsune/sumo/api_utils.py

@@ -3,14 +3,14 @@ from zoneinfo import ZoneInfo
 from django import forms
 from django.contrib.contenttypes.models import ContentType
 from django.contrib.auth.models import User
+from django.db.models import F
 from django.http import HttpResponse
 from django.utils import translation
 from django.utils.translation import pgettext
 
-from rest_framework import fields, permissions, serializers
+from rest_framework import fields, filters, permissions, serializers
 from rest_framework.authentication import SessionAuthentication, CSRFCheck
 from rest_framework.exceptions import APIException, AuthenticationFailed
-from rest_framework.filters import BaseFilterBackend
 from rest_framework.renderers import JSONRenderer as DRFJSONRenderer
 
 from kitsune.users.models import Profile
@@ -184,7 +184,7 @@ class GenericRelatedField(fields.ReadOnlyField):
         return data
 
 
-class InequalityFilterBackend(BaseFilterBackend):
+class InequalityFilterBackend(filters.BaseFilterBackend):
     """A filter backend that allows for field__gt style filtering."""
 
     def filter_queryset(self, request, queryset, view):
@@ -350,3 +350,20 @@ class JSONRenderer(DRFJSONRenderer):
         # JSON spec: http://json.org/
 
         return json.replace(b"</", b"<\\/")
+
+
+class OrderingFilter(filters.OrderingFilter):
+    """
+    Sub-class of rest_framework.filters.OrderingFilter that simply ensures that
+    any null values in fields requested in descending order are sorted last.
+    """
+
+    def get_ordering(self, request, queryset, view):
+        """
+        Replaces any ordering fields requested in descending order with an F()
+        expression that ensures any null values are sorted last.
+        """
+        return [
+            F(field[1:]).desc(nulls_last=True) if field.startswith("-") else field
+            for field in super().get_ordering(request, queryset, view)
+        ]

kitsune/users/api.py

@@ -8,7 +8,7 @@ from django.db.models.functions import Now
 from django.utils.encoding import force_str
 from django.views.decorators.http import require_GET
 from django_filters.rest_framework import DjangoFilterBackend
-from rest_framework import filters, mixins, permissions, serializers, viewsets
+from rest_framework import mixins, permissions, serializers, viewsets
 from rest_framework.decorators import api_view, permission_classes
 from rest_framework.permissions import IsAuthenticated
 from rest_framework.response import Response
@@ -16,7 +16,7 @@ from rest_framework.response import Response
 from kitsune.access.decorators import login_required
 from kitsune.questions.models import Answer
 from kitsune.questions.utils import num_answers, num_questions, num_solutions
-from kitsune.sumo.api_utils import DateTimeUTCField, PermissionMod
+from kitsune.sumo.api_utils import DateTimeUTCField, OrderingFilter, PermissionMod
 from kitsune.sumo.decorators import json_view
 from kitsune.users.models import Profile, Setting
 from kitsune.users.templatetags.jinja_helpers import profile_avatar
@@ -248,7 +248,7 @@ class ProfileViewSet(
     ]
     filter_backends = [
         DjangoFilterBackend,
-        filters.OrderingFilter,
+        OrderingFilter,
     ]
     filterset_fields: list[str] = []
     ordering_fields: list[str] = []