diff --git a/arkindex/documents/api/elements.py b/arkindex/documents/api/elements.py
index 9ae8e392cda46ca78c8b37ea33ea0f2abe49d7bc..1349c7fd809c1e26490085ad73baa74ed550f8da 100644
--- a/arkindex/documents/api/elements.py
+++ b/arkindex/documents/api/elements.py
@@ -1273,13 +1273,15 @@ class ElementNeighbors(ACLMixin, ListAPIView):
Requires a **read** access to the element's corpus.
"""
serializer_class = ElementNeighborsSerializer
+ pagination_class = None
# For OpenAPI type discovery
queryset = Element.objects.none()
def get_queryset(self):
element = get_object_or_404(
- Element.objects.select_related('corpus').only('id', 'corpus__public'),
+ # Include the attributes required for ACL checks and the API response
+ Element.objects.select_related('corpus', 'type').only('id', 'name', 'type__slug', 'corpus__public'),
id=self.kwargs['pk']
)
diff --git a/arkindex/documents/managers.py b/arkindex/documents/managers.py
index b4127e56b317258692c7dcc6bb74ef5db2f081bc..298a82a1c9b8e98bd58511821f65018ef476c5ee 100644
--- a/arkindex/documents/managers.py
+++ b/arkindex/documents/managers.py
@@ -193,47 +193,27 @@ class ElementManager(models.Manager):
def get_neighbors(self, element):
"""
- Returns a list of neighboring ElementPaths for an element, with a prefetched `element` attribute
- and a list of prefetched parent elements in the `parents` attribute.
+ Returns a list of neighboring ElementPaths for an element, with overridden attributes:
+ - `ElementPath.path` is an array of all elements in the path, instead of element IDs.
+ - `ElementPath.previous` is the element that precedes this one in the same parent, or None if there is none.
+ - `ElementPath.previous` is the element that succeeds this one in the same parent, or None if there is none.
"""
paths = list(self.get_neighbor_paths(element))
# Build a set of all IDs to load related elements (neighbors, parents) then load them into a dict
- related_elt_ids = set(chain(
- (element.id,),
+ related_elt_ids = set(filter(None, chain(
*((path.previous, path.next) for path in paths),
*(path.path for path in paths),
- ))
- elements = {
- elt.id: elt
- for elt in (
- self.filter(id__in=filter(None, related_elt_ids))
- .select_related('type')
- .only('id', 'type__slug', 'name')
- )
- }
+ )))
+ elements = self.select_related('type').only('id', 'type__slug', 'name').in_bulk(related_elt_ids)
- # Generate an output corresponding to endpoint expectations (compatibility purpose)
- output = []
for path in paths:
- if path.previous:
- output.append({
- 'ordering': path.previous_ord,
- 'element': elements.get(path.previous),
- 'parents': list(map(elements.get, path.path)),
- })
- output.append({
- 'ordering': path.ordering,
- 'element': elements.get(element.id),
- 'parents': list(map(elements.get, path.path)),
- })
- if path.next:
- output.append({
- 'ordering': path.next_ord,
- 'element': elements.get(path.next),
- 'parents': list(map(elements.get, path.path)),
- })
- return output
+ path.element = element
+ path.previous = elements.get(path.previous)
+ path.next = elements.get(path.next)
+ path.path = list(map(elements.get, path.path))
+
+ return paths
class CorpusManager(models.Manager):
diff --git a/arkindex/documents/serializers/elements.py b/arkindex/documents/serializers/elements.py
index 5b4168afa43495adaa3f64c7523a0050e26feaa3..7e9a9dfb544a2d33b1c5af64fbd6286ac18e7b00 100644
--- a/arkindex/documents/serializers/elements.py
+++ b/arkindex/documents/serializers/elements.py
@@ -673,14 +673,23 @@ class ElementSerializer(ElementSlimSerializer):
return instance
-class ElementNeighborsSerializer(serializers.Serializer):
- # position attribute is left for compatibility but represents the real path ordering
- position = serializers.IntegerField(source='ordering')
- element = ElementLightSerializer()
- parents = serializers.ListField(
+class ElementNeighborsSerializer(serializers.ModelSerializer):
+ previous = ElementLightSerializer(allow_null=True)
+ next = ElementLightSerializer(allow_null=True)
+ path = serializers.ListField(
child=ElementLightSerializer()
)
+ class Meta:
+ model = ElementPath
+ fields = (
+ 'path',
+ 'ordering',
+ 'previous',
+ 'next',
+ )
+ read_only_fields = fields
+
@extend_schema_serializer(deprecate_fields=('worker_version', ))
class ElementCreateSerializer(ElementLightSerializer):
diff --git a/arkindex/documents/tests/test_element_manager.py b/arkindex/documents/tests/test_element_manager.py
index 2564de889501575b26ff191addb88c8a13d9659e..d53efb4a881d4b850a7ac79e2c30ca401d416bdd 100644
--- a/arkindex/documents/tests/test_element_manager.py
+++ b/arkindex/documents/tests/test_element_manager.py
@@ -40,8 +40,8 @@ class TestElementManager(FixtureTestCase):
def test_get_neighbors(self):
with self.assertNumQueries(2):
- self.assertEqual(len(Element.objects.get_neighbors(self.p1)), 2)
+ self.assertEqual(len(Element.objects.get_neighbors(self.p1)), 1)
with self.assertNumQueries(2):
- self.assertEqual(len(Element.objects.get_neighbors(self.p2)), 3)
+ self.assertEqual(len(Element.objects.get_neighbors(self.p2)), 1)
with self.assertNumQueries(2):
- self.assertEqual(len(Element.objects.get_neighbors(self.p3)), 2)
+ self.assertEqual(len(Element.objects.get_neighbors(self.p3)), 1)
diff --git a/arkindex/documents/tests/test_neighbors.py b/arkindex/documents/tests/test_neighbors.py
index 89ae801d9af786b78e0a1ff2ba389ea249817965..4bcd73ff27e387a888265a5a29e41cff8b3aa467 100644
--- a/arkindex/documents/tests/test_neighbors.py
+++ b/arkindex/documents/tests/test_neighbors.py
@@ -1,9 +1,8 @@
from django.db import transaction
-from django.db.models import F
from django.urls import reverse
from rest_framework import status
-from arkindex.documents.models import Corpus, Element, ElementPath
+from arkindex.documents.models import Corpus, Element
from arkindex.project.tests import FixtureAPITestCase
from arkindex.project.tools import build_tree
@@ -60,71 +59,36 @@ class TestElementNeighbors(FixtureAPITestCase):
response = self.client.get(reverse('api:elements-neighbors', kwargs={'pk': str(elements['A'].id)}))
self.assertEqual(response.status_code, status.HTTP_200_OK)
- results = response.json()['results']
self.assertListEqual(
- results,
+ response.json(),
sorted([{
# 2 neighbors via X (B, A, C) A has position 1
- 'position': 0,
- 'element': {
+ 'ordering': 1,
+ 'previous': {
'id': str(elements['B'].id),
'type': 'volume',
'name': elements['B'].name
},
- 'parents': [{
- 'id': str(elements['X'].id),
- 'type': 'volume',
- 'name': elements['X'].name
- }]
- }, {
- 'position': 1,
- 'element': {
- 'id': str(elements['A'].id),
- 'type': 'volume',
- 'name': elements['A'].name
- },
- 'parents': [{
- 'id': str(elements['X'].id),
- 'type': 'volume',
- 'name': elements['X'].name
- }]
- }, {
- 'position': 2,
- 'element': {
+ 'next': {
'id': str(elements['C'].id),
'type': 'volume',
'name': elements['C'].name
},
- 'parents': [{
+ 'path': [{
'id': str(elements['X'].id),
'type': 'volume',
'name': elements['X'].name
- }]
+ }],
}, {
# 1 neighbors via Y (A, D) A has position 0
- 'position': 0,
- 'element': {
- 'id': str(elements['A'].id),
- 'type': 'volume',
- 'name': elements['A'].name
- },
- 'parents': [{
- 'id': str(elements['Z'].id),
- 'type': 'volume',
- 'name': elements['Z'].name
- }, {
- 'id': str(elements['Y'].id),
- 'type': 'volume',
- 'name': elements['Y'].name
- }]
- }, {
- 'position': 1,
- 'element': {
+ 'ordering': 0,
+ 'previous': None,
+ 'next': {
'id': str(elements['D'].id),
'type': 'volume',
'name': elements['D'].name
},
- 'parents': [{
+ 'path': [{
'id': str(elements['Z'].id),
'type': 'volume',
'name': elements['Z'].name
@@ -135,7 +99,7 @@ class TestElementNeighbors(FixtureAPITestCase):
}]
}],
# Order result by paths, as we can't predict the test generated uuids
- key=lambda path: ''.join([p['id'] for p in path['parents']]))
+ key=lambda path: ''.join([p['id'] for p in path['path']]))
)
def test_element_neighbors_missing_ordering(self):
@@ -166,41 +130,21 @@ class TestElementNeighbors(FixtureAPITestCase):
transaction.savepoint_rollback(sid)
self.assertEqual(response.status_code, status.HTTP_200_OK)
- results = response.json()['results']
self.assertListEqual(
- results,
+ response.json(),
[{
- 'position': 0,
- 'element': {
+ 'ordering': 1,
+ 'previous': {
'id': str(elements['A'].id),
'type': 'volume',
'name': elements['A'].name
},
- 'parents': [{
- 'id': str(elements['W'].id),
- 'type': 'volume',
- 'name': elements['W'].name
- }]
- }, {
- 'position': 1,
- 'element': {
- 'id': str(elements['B'].id),
- 'type': 'volume',
- 'name': 'B'
- },
- 'parents': [{
- 'id': str(elements['W'].id),
- 'type': 'volume',
- 'name': elements['W'].name
- }]
- }, {
- 'position': 6584,
- 'element': {
+ 'next': {
'id': str(elements['C'].id),
'type': 'volume',
'name': elements['C'].name
},
- 'parents': [{
+ 'path': [{
'id': str(elements['W'].id),
'type': 'volume',
'name': elements['W'].name
@@ -230,42 +174,22 @@ class TestElementNeighbors(FixtureAPITestCase):
)
response = self.client.get(reverse('api:elements-neighbors', kwargs={'pk': str(elements['B'].id)}))
self.assertEqual(response.status_code, status.HTTP_200_OK)
- results = response.json()['results']
self.assertListEqual(
- results,
+ response.json(),
[{
- 'position': 0,
- 'element': {
+ 'ordering': 1,
+ 'previous': {
'id': str(elements['A'].id),
'type': 'volume',
'name': elements['A'].name
},
- 'parents': [{
- 'id': str(elements['V'].id),
- 'type': 'volume',
- 'name': elements['V'].name
- }]
- }, {
- 'position': 1,
- 'element': {
- 'id': str(elements['B'].id),
- 'type': 'volume',
- 'name': 'B'
- },
- 'parents': [{
- 'id': str(elements['V'].id),
- 'type': 'volume',
- 'name': elements['V'].name
- }]
- }, {
- 'position': 2,
- 'element': {
+ 'next': {
'id': str(elements['C'].id),
'type': 'volume',
'name': elements['C'].name
},
- 'parents': [{
+ 'path': [{
'id': str(elements['V'].id),
'type': 'volume',
'name': elements['V'].name
@@ -280,55 +204,3 @@ class TestElementNeighbors(FixtureAPITestCase):
kwargs={'pk': 'aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa'},
))
self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND)
-
- def test_no_duplicate_positions(self):
- """
- ListElementNeighbors had an issue where, if the element was found at position 10 in one parent
- and 20 in the other, elements at positions 9, 10, 11, 19, 20, and 21 were returned on both parents
- (12 rows instead of the expected 6).
- A B
- â•â”€â”¬â”€â”¬â”´â”¬â”€â”¬â”€â•® â•â”€â”¬â”€â”¬â”´â”¬â”€â”¬â”€â•®
- C X D E F G H I J K X L
- """
- elements = build_tree(
- {
- 'C': 'A',
- 'D': 'A',
- 'E': 'A',
- 'F': 'A',
- 'G': 'A',
- 'H': 'B',
- 'I': 'B',
- 'J': 'B',
- 'K': 'B',
- 'L': 'B',
- 'X': ['A', 'B'],
- },
- corpus=self.corpus,
- type=self.volume_type,
- )
- # The unique_element_orderings trigger will fail because an element is not supposed to have
- # multiple orderings within the same parent. This trigger is deferred, meaning it will only run
- # after the test ends, during teardown, so we cannot ignore it with pgtrigger.ignore().
- # Instead, we use a savepoint that we rollback immediately after the request, so that the trigger never runs.
- sid = transaction.savepoint()
-
- try:
- # Put X at the orderings we want it to have, then add 1 to D E F G and L to account for the move
- elements['X'].paths.filter(path__contains=[elements['A'].id]).update(ordering=1)
- elements['X'].paths.filter(path__contains=[elements['B'].id]).update(ordering=4)
- ElementPath.objects.filter(element__name__in=list('DEFGL')).update(ordering=F('ordering') + 1)
-
- # Ask for X's neighbors, should get C, X, D, K, X, and L.
- # Without the fix, would get the 12 children
- response = self.client.get(reverse('api:elements-neighbors', kwargs={'pk': str(elements['X'].id)}))
- finally:
- transaction.savepoint_rollback(sid)
-
- self.assertEqual(response.status_code, status.HTTP_200_OK)
-
- # The endpoint is ordered by ID, so depending on the random UUIDs we get, we can have CXDKXL or KXLCXD
- self.assertCountEqual(
- [result['element']['name'] for result in response.json()['results']],
- ['C', 'X', 'D', 'K', 'X', 'L'],
- )