Add sorting parameters on element lists

arkindex/documents/api/elements.py

@@ -134,6 +134,8 @@ def _fetch_has_children(elements):
 
 METADATA_OPERATORS = {'eq', 'lt', 'gt', 'lte', 'gte'}
 
+ELEMENT_ORDERING_FIELDS = {'name', 'created'}
+
 
 class ElementsListAutoSchema(AutoSchema):
     """
@@ -213,6 +215,20 @@ class ElementsListAutoSchema(AutoSchema):
                     default='eq',
                     required=False,
                 ),
+                OpenApiParameter(
+                    'order',
+                    description='Sort elements by a specific field',
+                    enum=ELEMENT_ORDERING_FIELDS,
+                    default='name',
+                    required=False,
+                ),
+                OpenApiParameter(
+                    'order_direction',
+                    description='Direction in which to sort elements',
+                    enum={'asc', 'desc'},
+                    default='asc',
+                    required=False,
+                ),
                 OpenApiParameter(
                     'with_best_classes',
                     description='Returns best classifications for each element. '
@@ -499,8 +515,21 @@ class ElementsListBase(CorpusACLMixin, DestroyModelMixin, ListAPIView):
         return prefetch
 
     def get_order_by(self):
-        # Ordering by ID is required by postgres to order elements with the same name
-        return ('name', 'id')
+        errors = {}
+
+        sort_field = self.clean_params.get('order', 'name').lower()
+        if sort_field not in ELEMENT_ORDERING_FIELDS:
+            errors['order'] = ['Unknown sorting field']
+
+        direction = self.clean_params.get('order_direction', 'asc').lower()
+        if direction not in ('asc', 'desc'):
+            errors['order_direction'] = ['Unknown sorting direction']
+
+        if errors:
+            raise ValidationError(errors)
+
+        # The sorting field is not unique; fallback on ordering by ID to ensure a consistent ordering
+        return (f'{"-" if direction == "desc" else ""}{sort_field}', 'id')
 
     def filter_queryset(self, queryset):
         queryset = queryset \
@@ -583,14 +612,14 @@ class ElementsListBase(CorpusACLMixin, DestroyModelMixin, ListAPIView):
     get=extend_schema(operation_id='ListElements'),
     delete=extend_schema(operation_id='DestroyElements', description=(
         'Destroy elements in bulk.\n\n'
-        'Requires an **admin** access to the process.'
+        'Requires an **admin** access to the corpus.'
     )),
 )
 class CorpusElements(ElementsListBase):
     """
-    List elements in a corpus and filter by type, name, ML class.
+    List elements in a corpus.
 
-    Requires an **read** access to the corpus.
+    Requires a **read** access to the corpus.
     """
 
     @property
@@ -668,7 +697,15 @@ class ElementParents(ElementsListBase):
     ]
 )
 @extend_schema_view(
-    get=extend_schema(operation_id='ListElementChildren'),
+    get=extend_schema(operation_id='ListElementChildren', parameters=[
+        OpenApiParameter(
+            'order',
+            description='Sort elements by a specific field',
+            enum=ELEMENT_ORDERING_FIELDS | {'position'},
+            default='name',
+            required=False,
+        )
+    ]),
     delete=extend_schema(operation_id='DestroyElementChildren', description=(
         'Delete child elements in bulk.\n\n'
         "Requires an **admin** access to the element's corpus."
@@ -711,7 +748,14 @@ class ElementChildren(ElementsListBase):
         return filters
 
     def get_order_by(self):
-        return ('paths__ordering', 'id')
+        """
+        Override the ElementsListBase ordering to default by path ordering.
+        The only endpoint where a path ordering is made visible in the API
+        is ListElementNeighbors, in a field named `position`.
+        """
+        if self.clean_params.get('order', 'position').lower() == 'position':
+            return ('paths__ordering', 'id')
+        return super().get_order_by()
 
     def get_queryset(self):
         self.element = get_object_or_404(

arkindex/documents/tests/test_children_elements.py

 import uuid
+from datetime import datetime, timedelta, timezone
 
 from django.urls import reverse
 from rest_framework import status
@@ -477,3 +478,71 @@ class TestChildrenElements(FixtureAPITestCase):
                     [element['name'] for element in response.json()['results']],
                     expected_elements
                 )
+
+    def test_children_invalid_sort(self):
+        cases = [
+            ({'order': 'blah', 'order_direction': 'asc'}, {'order': ['Unknown sorting field']}),
+            ({'order': 'name', 'order_direction': 'left'}, {'order_direction': ['Unknown sorting direction']}),
+            (
+                {'order': 'blah', 'order_direction': 'left'},
+                {'order': ['Unknown sorting field'], 'order_direction': ['Unknown sorting direction']}
+            ),
+        ]
+        for params, expected in cases:
+            with self.subTest(**params):
+                response = self.client.get(
+                    reverse('api:elements-children', kwargs={'pk': self.vol.id}),
+                    params,
+                )
+                self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
+                self.assertDictEqual(response.json(), expected)
+
+    def test_children_sort_created(self):
+        elements = list(Element.objects.get_descending(self.vol.id).order_by('?').only('id'))
+        created = datetime(2020, 2, 2, tzinfo=timezone.utc)
+        for element in elements:
+            element.created = created
+            created += timedelta(seconds=1)
+        Element.objects.bulk_update(elements, ['created'])
+
+        cases = [
+            ('asc', elements[:20]),
+            ('desc', reversed(elements[-20:])),
+        ]
+        for direction, expected in cases:
+            with self.subTest(direction=direction), self.assertNumQueries(5):
+                response = self.client.get(
+                    reverse('api:elements-children', kwargs={'pk': self.vol.id}),
+                    {'order': 'created', 'order_direction': direction, 'recursive': True}
+                )
+                self.assertEqual(response.status_code, status.HTTP_200_OK)
+                self.assertListEqual(
+                    [element['id'] for element in response.json()['results']],
+                    [str(element.id) for element in expected]
+                )
+
+    def test_children_sort_name(self):
+        names = list(Element.objects.get_descending(self.vol.id).order_by('name', 'id').values_list('name', flat=True))
+
+        # Ensure the default position ordering returns a different ordering than by name;
+        # otherwise, this test would be useless
+        self.assertNotEqual(
+            list(Element.objects.get_descending(self.vol.id).values_list('name', flat=True)),
+            names,
+        )
+
+        cases = [
+            ('asc', names[:20]),
+            ('desc', list(reversed(names[-20:]))),
+        ]
+        for direction, expected in cases:
+            with self.subTest(direction=direction), self.assertNumQueries(5):
+                response = self.client.get(
+                    reverse('api:elements-children', kwargs={'pk': self.vol.id}),
+                    {'order': 'name', 'order_direction': direction, 'recursive': True}
+                )
+                self.assertEqual(response.status_code, status.HTTP_200_OK)
+                self.assertListEqual(
+                    [element['name'] for element in response.json()['results']],
+                    expected
+                )

arkindex/documents/tests/test_corpus_elements.py

 import uuid
+from datetime import datetime, timedelta, timezone
 
 import sqlparse
 from django.db.models.sql.constants import LOUTER
@@ -571,3 +572,45 @@ class TestListElements(FixtureAPITestCase):
         data = response.json()
         self.assertEqual(len(data['results']), 500)
         self.assertEqual(data['count'], 999)
+
+    def test_list_elements_invalid_sort(self):
+        cases = [
+            ({'order': 'blah', 'order_direction': 'asc'}, {'order': ['Unknown sorting field']}),
+            ({'order': 'name', 'order_direction': 'left'}, {'order_direction': ['Unknown sorting direction']}),
+            (
+                {'order': 'blah', 'order_direction': 'left'},
+                {'order': ['Unknown sorting field'], 'order_direction': ['Unknown sorting direction']}
+            ),
+        ]
+        for params, expected in cases:
+            with self.subTest(**params):
+                response = self.client.get(
+                    reverse('api:corpus-elements', kwargs={'corpus': self.corpus.id}),
+                    params,
+                )
+                self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
+                self.assertDictEqual(response.json(), expected)
+
+    def test_list_elements_sort_created(self):
+        elements = list(self.corpus.elements.order_by('?').only('id'))
+        created = datetime(2020, 2, 2, tzinfo=timezone.utc)
+        for element in elements:
+            element.created = created
+            created += timedelta(seconds=1)
+        Element.objects.bulk_update(elements, ['created'])
+
+        cases = [
+            ('asc', elements[:20]),
+            ('desc', reversed(elements[-20:])),
+        ]
+        for direction, expected in cases:
+            with self.subTest(direction=direction), self.assertNumQueries(5):
+                response = self.client.get(
+                    reverse('api:corpus-elements', kwargs={'corpus': self.corpus.id}),
+                    {'order': 'created', 'order_direction': direction}
+                )
+                self.assertEqual(response.status_code, status.HTTP_200_OK)
+                self.assertListEqual(
+                    [element['id'] for element in response.json()['results']],
+                    [str(element.id) for element in expected]
+                )

arkindex/documents/tests/test_parents_elements.py

 import uuid
+from datetime import datetime, timedelta, timezone
 
 from django.urls import reverse
 from rest_framework import status
@@ -294,3 +295,45 @@ class TestParentsElements(FixtureAPITestCase):
                     [element['name'] for element in response.json()['results']],
                     expected_elements
                 )
+
+    def test_parents_invalid_sort(self):
+        cases = [
+            ({'order': 'blah', 'order_direction': 'asc'}, {'order': ['Unknown sorting field']}),
+            ({'order': 'name', 'order_direction': 'left'}, {'order_direction': ['Unknown sorting direction']}),
+            (
+                {'order': 'blah', 'order_direction': 'left'},
+                {'order': ['Unknown sorting field'], 'order_direction': ['Unknown sorting direction']}
+            ),
+        ]
+        for params, expected in cases:
+            with self.subTest(**params):
+                response = self.client.get(
+                    reverse('api:elements-parents', kwargs={'pk': self.page.id}),
+                    params,
+                )
+                self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
+                self.assertDictEqual(response.json(), expected)
+
+    def test_parents_sort_created(self):
+        elements = list(Element.objects.get_ascending(self.page.id).order_by('?').only('id'))
+        created = datetime(2020, 2, 2, tzinfo=timezone.utc)
+        for element in elements:
+            element.created = created
+            created += timedelta(seconds=1)
+        Element.objects.bulk_update(elements, ['created'])
+
+        cases = [
+            ('asc', elements[:20]),
+            ('desc', reversed(elements[-20:])),
+        ]
+        for direction, expected in cases:
+            with self.subTest(direction=direction), self.assertNumQueries(4):
+                response = self.client.get(
+                    reverse('api:elements-parents', kwargs={'pk': self.page.id}),
+                    {'order': 'created', 'order_direction': direction, 'recursive': True}
+                )
+                self.assertEqual(response.status_code, status.HTTP_200_OK)
+                self.assertListEqual(
+                    [element['id'] for element in response.json()['results']],
+                    [str(element.id) for element in expected]
+                )