From ce1568322af61a6b41ccc5dc2c631c6927ed5e71 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=96mer=20Faruk=20Abac=C4=B1?= <omerfabaci@gmail.com>
Date: Tue, 16 Mar 2021 15:53:39 +0300
Subject: [PATCH] Ordering filter bug with model property serializer field
 (#7609)

* Add failing tests for ordering filter with model property

* Fix get_default_valid_fields of OrderingFilter

* Filter model properties in get_default_valid_fields of OrderingFilter
---
 rest_framework/filters.py | 12 ++++++++-
 tests/test_filters.py     | 51 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/rest_framework/filters.py b/rest_framework/filters.py
index 366577519..1ffd9edc0 100644
--- a/rest_framework/filters.py
+++ b/rest_framework/filters.py
@@ -226,10 +226,20 @@ class OrderingFilter(BaseFilterBackend):
             )
             raise ImproperlyConfigured(msg % self.__class__.__name__)
 
+        model_class = queryset.model
+        model_property_names = [
+            # 'pk' is a property added in Django's Model class, however it is valid for ordering.
+            attr for attr in dir(model_class) if isinstance(getattr(model_class, attr), property) and attr != 'pk'
+        ]
+
         return [
             (field.source.replace('.', '__') or field_name, field.label)
             for field_name, field in serializer_class(context=context).fields.items()
-            if not getattr(field, 'write_only', False) and not field.source == '*'
+            if (
+                not getattr(field, 'write_only', False) and
+                not field.source == '*' and
+                field.source not in model_property_names
+            )
         ]
 
     def get_valid_fields(self, queryset, view, context={}):
diff --git a/tests/test_filters.py b/tests/test_filters.py
index 567e5f83f..37ae4c7cf 100644
--- a/tests/test_filters.py
+++ b/tests/test_filters.py
@@ -424,6 +424,10 @@ class OrderingFilterModel(models.Model):
     title = models.CharField(max_length=20, verbose_name='verbose title')
     text = models.CharField(max_length=100)
 
+    @property
+    def description(self):
+        return self.title + ": " + self.text
+
 
 class OrderingFilterRelatedModel(models.Model):
     related_object = models.ForeignKey(OrderingFilterModel, related_name="relateds", on_delete=models.CASCADE)
@@ -436,6 +440,17 @@ class OrderingFilterSerializer(serializers.ModelSerializer):
         fields = '__all__'
 
 
+class OrderingFilterSerializerWithModelProperty(serializers.ModelSerializer):
+    class Meta:
+        model = OrderingFilterModel
+        fields = (
+            "id",
+            "title",
+            "text",
+            "description"
+        )
+
+
 class OrderingDottedRelatedSerializer(serializers.ModelSerializer):
     related_text = serializers.CharField(source='related_object.text')
     related_title = serializers.CharField(source='related_object.title')
@@ -551,6 +566,42 @@ class OrderingFilterTests(TestCase):
             {'id': 1, 'title': 'zyx', 'text': 'abc'},
         ]
 
+    def test_ordering_without_ordering_fields(self):
+        class OrderingListView(generics.ListAPIView):
+            queryset = OrderingFilterModel.objects.all()
+            serializer_class = OrderingFilterSerializerWithModelProperty
+            filter_backends = (filters.OrderingFilter,)
+            ordering = ('title',)
+
+        view = OrderingListView.as_view()
+
+        # Model field ordering works fine.
+        request = factory.get('/', {'ordering': 'text'})
+        response = view(request)
+        assert response.data == [
+            {'id': 1, 'title': 'zyx', 'text': 'abc', 'description': 'zyx: abc'},
+            {'id': 2, 'title': 'yxw', 'text': 'bcd', 'description': 'yxw: bcd'},
+            {'id': 3, 'title': 'xwv', 'text': 'cde', 'description': 'xwv: cde'},
+        ]
+
+        # `incorrectfield` ordering works fine.
+        request = factory.get('/', {'ordering': 'foobar'})
+        response = view(request)
+        assert response.data == [
+            {'id': 3, 'title': 'xwv', 'text': 'cde', 'description': 'xwv: cde'},
+            {'id': 2, 'title': 'yxw', 'text': 'bcd', 'description': 'yxw: bcd'},
+            {'id': 1, 'title': 'zyx', 'text': 'abc', 'description': 'zyx: abc'},
+        ]
+
+        # `description` is a Model property, which should be ignored.
+        request = factory.get('/', {'ordering': 'description'})
+        response = view(request)
+        assert response.data == [
+            {'id': 3, 'title': 'xwv', 'text': 'cde', 'description': 'xwv: cde'},
+            {'id': 2, 'title': 'yxw', 'text': 'bcd', 'description': 'yxw: bcd'},
+            {'id': 1, 'title': 'zyx', 'text': 'abc', 'description': 'zyx: abc'},
+        ]
+
     def test_default_ordering(self):
         class OrderingListView(generics.ListAPIView):
             queryset = OrderingFilterModel.objects.all()