From 9e725062ba532c2f61e9a7a39123011fb87ffc0b Mon Sep 17 00:00:00 2001
From: Erwan Rouchet <rouchet@teklia.com>
Date: Tue, 23 May 2023 08:51:02 +0000
Subject: [PATCH] Add exclusion constraints on ElementPath ordering
---
arkindex/documents/api/elements.py | 34 +--------
arkindex/documents/api/ml.py | 2 +-
arkindex/documents/fixtures/data.json | 12 +--
.../0065_elementpath_constraints.py | 40 ++++++++++
arkindex/documents/models.py | 73 ++++++++++++++-----
.../commands/test_move_lines_to_parent.py | 6 +-
.../tests/tasks/test_move_element.py | 3 -
.../tests/test_bulk_element_transcriptions.py | 12 +--
.../documents/tests/test_bulk_elements.py | 12 +--
.../documents/tests/test_children_elements.py | 13 ++--
.../documents/tests/test_create_elements.py | 18 ++---
.../documents/tests/test_edit_elementpath.py | 24 +++++-
.../documents/tests/test_element_paths_api.py | 48 +-----------
arkindex/documents/tests/test_neighbors.py | 2 +-
arkindex/sql_validation/add_first_parent.sql | 13 ++--
arkindex/sql_validation/add_second_parent.sql | 13 ++--
.../element_move_with_children.sql | 13 ++--
.../element_move_without_child.sql | 13 ++--
.../remove_child_last_parent.sql | 6 +-
19 files changed, 185 insertions(+), 172 deletions(-)
create mode 100644 arkindex/documents/migrations/0065_elementpath_constraints.py
diff --git a/arkindex/documents/api/elements.py b/arkindex/documents/api/elements.py
index a744a3cd9f..41df04fa10 100644
--- a/arkindex/documents/api/elements.py
+++ b/arkindex/documents/api/elements.py
@@ -8,18 +8,7 @@ from uuid import UUID
from django.conf import settings
from django.core.exceptions import ValidationError as DjangoValidationError
from django.db import connection, transaction
-from django.db.models import (
- CharField,
- Count,
- F,
- FloatField,
- Max,
- Prefetch,
- Q,
- QuerySet,
- Value,
- prefetch_related_objects,
-)
+from django.db.models import CharField, Count, F, FloatField, Prefetch, Q, QuerySet, Value, prefetch_related_objects
from django.db.models.functions import Cast
from django.shortcuts import get_object_or_404
from django.utils.functional import cached_property
@@ -2126,27 +2115,12 @@ class ElementBulkCreate(CreateAPIView):
worker_run = serializer.validated_data['worker_run']
elements = serializer.validated_data['elements']
- # ElementPath.get_next_order, but for multiple types
- next_orderings = defaultdict(
- int,
- ElementPath
- .objects
- .using('default')
- .filter(
- # Using __overlap before __last makes Postgres use the GIN index instead of scanning the whole table
- path__overlap=[self.element.id],
- path__last=self.element.id,
- element__type_id__in=set(element_data['type'] for element_data in elements)
- )
- .values('element__type_id')
- .annotate(next_ordering=Max('ordering') + 1)
- .values_list('element__type_id', 'next_ordering')
- )
+ next_ordering = self.element.get_next_order()
# Prepare elements
for element_data in elements:
# Keep ordering in the element payload to optimize element path creation later
- element_data['ordering'] = next_orderings[element_data['type']]
- next_orderings[element_data['type']] += 1
+ element_data['ordering'] = next_ordering
+ next_ordering += 1
element_data['element'] = Element(
id=uuid.uuid4(),
diff --git a/arkindex/documents/api/ml.py b/arkindex/documents/api/ml.py
index 2538611d20..a951a0bb6b 100644
--- a/arkindex/documents/api/ml.py
+++ b/arkindex/documents/api/ml.py
@@ -236,7 +236,7 @@ class ElementTranscriptionsBulk(CreateAPIView):
if not paths:
# Support top level elements, by adding an empty initial path to trigger loops below
paths = [[]]
- next_path_ordering = self.element.get_next_order(elt_type)
+ next_path_ordering = self.element.get_next_order()
for annotation in annotations:
# Look for a direct children with the right type and zone
annotation['element'] = children.get(annotation['polygon'].wkb)
diff --git a/arkindex/documents/fixtures/data.json b/arkindex/documents/fixtures/data.json
index f64a4cbc4e..980e532421 100644
--- a/arkindex/documents/fixtures/data.json
+++ b/arkindex/documents/fixtures/data.json
@@ -485,7 +485,7 @@
"fields": {
"element": "baea57f6-ac29-441f-9f54-6ba6fd6a9ad5",
"path": "[\"61028c97-6f49-4cdd-b85f-802929964e86\"]",
- "ordering": 2
+ "ordering": 5
}
},
{
@@ -521,7 +521,7 @@
"fields": {
"element": "e88a7ef5-c172-41b3-b180-bcdcf96598da",
"path": "[\"61028c97-6f49-4cdd-b85f-802929964e86\"]",
- "ordering": 1
+ "ordering": 4
}
},
{
@@ -548,7 +548,7 @@
"fields": {
"element": "fb66f858-83d5-404b-bd16-75c1e41b4c9b",
"path": "[\"61028c97-6f49-4cdd-b85f-802929964e86\"]",
- "ordering": 0
+ "ordering": 3
}
},
{
@@ -575,7 +575,7 @@
"fields": {
"element": "d47bd411-be3c-4f08-9b2c-b886e91b9513",
"path": "[\"61028c97-6f49-4cdd-b85f-802929964e86\", \"2b1a6efb-6667-4329-a2da-13727695b8a4\"]",
- "ordering": 0
+ "ordering": 3
}
},
{
@@ -611,7 +611,7 @@
"fields": {
"element": "a328d7ee-624a-489b-9dff-6cc32a1935df",
"path": "[\"61028c97-6f49-4cdd-b85f-802929964e86\"]",
- "ordering": 3
+ "ordering": 6
}
},
{
@@ -683,7 +683,7 @@
"fields": {
"element": "9b4bb46a-b466-4d24-a23f-2c58a04cf47a",
"path": "[\"61028c97-6f49-4cdd-b85f-802929964e86\"]",
- "ordering": 4
+ "ordering": 7
}
},
{
diff --git a/arkindex/documents/migrations/0065_elementpath_constraints.py b/arkindex/documents/migrations/0065_elementpath_constraints.py
new file mode 100644
index 0000000000..6f8ad212c6
--- /dev/null
+++ b/arkindex/documents/migrations/0065_elementpath_constraints.py
@@ -0,0 +1,40 @@
+# Generated by Django 4.1.7 on 2023-04-26 14:12
+
+from django.contrib.postgres.constraints import ExclusionConstraint
+from django.contrib.postgres.fields import RangeOperators
+from django.db import migrations
+from django.db.models import Deferrable
+
+
+class Migration(migrations.Migration):
+
+ dependencies = [
+ ('documents', '0064_alter_entity_type_alter_entityrole_child_type_and_more'),
+ ]
+
+ operations = [
+ migrations.AddConstraint(
+ model_name='elementpath',
+ constraint=ExclusionConstraint(
+ name='unique_element_orderings',
+ expressions=[
+ ('element', RangeOperators.EQUAL),
+ ('path__last', RangeOperators.EQUAL),
+ ('ordering', RangeOperators.NOT_EQUAL),
+ ],
+ deferrable=Deferrable.DEFERRED,
+ ),
+ ),
+ migrations.AddConstraint(
+ model_name='elementpath',
+ constraint=ExclusionConstraint(
+ name='unique_parent_orderings',
+ expressions=[
+ ('path__last', RangeOperators.EQUAL),
+ ('ordering', RangeOperators.EQUAL),
+ ('element', RangeOperators.NOT_EQUAL),
+ ],
+ deferrable=Deferrable.DEFERRED,
+ ),
+ ),
+ ]
diff --git a/arkindex/documents/models.py b/arkindex/documents/models.py
index b8f3c5bae8..b4990d7dac 100644
--- a/arkindex/documents/models.py
+++ b/arkindex/documents/models.py
@@ -119,7 +119,36 @@ class ElementPath(models.Model):
(Least('path__len', 1), RangeOperators.NOT_EQUAL),
],
deferrable=Deferrable.DEFERRED,
- )
+ ),
+ # An element cannot be in two positions within the same parent, but its paths can be duplicated
+ # as there might be multiple paths to handle multiple grandparents on the same parent.
+ # We cannot use a unique constraint, but we can use an exclusion constraint to forbid different orderings.
+ ExclusionConstraint(
+ name='unique_element_orderings',
+ expressions=[
+ # On the same element…
+ ('element', RangeOperators.EQUAL),
+ # Within the same parent…
+ ('path__last', RangeOperators.EQUAL),
+ # There cannot be any orderings different from each other.
+ ('ordering', RangeOperators.NOT_EQUAL),
+ ],
+ deferrable=Deferrable.DEFERRED,
+ ),
+ # Within the same parent, there cannot be two elements with the same ordering, but since paths can be
+ # duplicated, there can be the same element twice with the same ordering, so exclusion constraint again.
+ ExclusionConstraint(
+ name='unique_parent_orderings',
+ expressions=[
+ # Within the same parent…
+ ('path__last', RangeOperators.EQUAL),
+ # At the same ordering…
+ ('ordering', RangeOperators.EQUAL),
+ # There cannot be any element IDs different from each other.
+ ('element', RangeOperators.NOT_EQUAL),
+ ],
+ deferrable=Deferrable.DEFERRED,
+ ),
]
@@ -260,6 +289,7 @@ class Element(IndexableModel):
# - Whether the parent is a descendant of the child, which would create a cycle
# - How many parents the parent has
# - The first path of any ElementPath of the parent
+ # - The next available ordering for direct children of the parent
# This can almost entirely be expressed using a QuerySet, but Django is stubborn
# and always tries to add a GROUP BY documents_elementpath.id, which returns
# wildly incorrect results.
@@ -279,14 +309,28 @@ class Element(IndexableModel):
EXISTS (SELECT 1 FROM child_paths WHERE path @> ARRAY[%(parent_id)s]),
EXISTS (SELECT 1 FROM parent_paths WHERE path @> ARRAY[%(self_id)s]),
(SELECT COUNT(*) FROM parent_paths),
- (SELECT path FROM parent_paths LIMIT 1)
+ (SELECT path FROM parent_paths LIMIT 1),
+ (
+ SELECT COALESCE(MAX(ordering) + 1, 0)
+ FROM documents_elementpath
+ WHERE path @> ARRAY[%(parent_id)s]
+ AND path[array_length(path, 1)] = %(parent_id)s
+ )
""",
{
'self_id': self.id,
'parent_id': parent.id,
},
)
- has_paths, existing_parent_path, already_has_parent, causes_cycle, parent_paths_count, first_parent_path = cursor.fetchone()
+ (
+ has_paths,
+ existing_parent_path,
+ already_has_parent,
+ causes_cycle,
+ parent_paths_count,
+ first_parent_path,
+ ordering,
+ ) = cursor.fetchone()
# If the child we are trying to add is among the parents of the new parent,
# then it would cause a cycle; an element cannot be its own ancestor.
@@ -304,10 +348,6 @@ class Element(IndexableModel):
parent_paths_count = 1
first_parent_path = []
- # Get the next order for this type in the parent
- # TODO: Move this into the first query once orderings are independent of element types
- ordering = parent.get_next_order(self.type_id if type_ordering else None)
-
# Add or update parent paths for this element first.
# When the child element has other parents, or when the child element has no paths (not even top-level),
# we only need to insert new paths.
@@ -417,21 +457,20 @@ class Element(IndexableModel):
}
)
- def get_next_order(self, type):
+ def get_next_order(self):
"""
Find the next ordering for a new child in ElementPath
Uses the primary database to avoid stale reads and duplicate orderings,
and path__overlap to let Postgres use the GIN index before filtering by last item.
+ Using the GIN index before path__last is quicker than using path__last alone.
"""
- assert type is None or isinstance(type, (ElementType, uuid.UUID))
-
- paths = ElementPath.objects \
- .using('default') \
- .filter(path__overlap=[self.id], path__last=self.id)
- if type:
- paths = paths.filter(element__type=type)
-
- return paths.aggregate(max=models.Max('ordering') + 1)['max'] or 0
+ return (
+ ElementPath
+ .objects
+ .using('default')
+ .filter(path__overlap=[self.id], path__last=self.id)
+ .aggregate(max=models.Max('ordering') + 1)['max'] or 0
+ )
@transaction.atomic
def remove_child(self, child):
diff --git a/arkindex/documents/tests/commands/test_move_lines_to_parent.py b/arkindex/documents/tests/commands/test_move_lines_to_parent.py
index 9719738adb..ae29bbec02 100644
--- a/arkindex/documents/tests/commands/test_move_lines_to_parent.py
+++ b/arkindex/documents/tests/commands/test_move_lines_to_parent.py
@@ -38,7 +38,7 @@ class TestMoveLinesToParentCommand(FixtureTestCase):
self.assertEqual(len(parent_paths), 1)
self.assertListEqual(list(parent_paths.values('path')), [{"path": [self.double_page.id]}])
- with self.assertNumQueries(19):
+ with self.assertNumQueries(18):
call_command('move_lines_to_parent', corpus=self.corpus, single_page_type='single_page', double_page_type='double_page')
# Make sure that lines have been moved
@@ -57,7 +57,7 @@ class TestMoveLinesToParentCommand(FixtureTestCase):
self.assertEqual(len(parent_paths), 1)
self.assertListEqual(list(parent_paths.values('path')), [{"path": [self.double_page.id]}])
- with self.assertNumQueries(19):
+ with self.assertNumQueries(18):
call_command('move_lines_to_parent', corpus=self.corpus, single_page_type='single_page', double_page_type='double_page')
# Make sure that paragraph have been moved
@@ -102,7 +102,7 @@ class TestMoveLinesToParentCommand(FixtureTestCase):
self.assertEqual(len(parent_paths), 1)
self.assertListEqual(list(parent_paths.values('path')), [{"path": [self.double_page.id]}])
- with self.assertNumQueries(30):
+ with self.assertNumQueries(28):
call_command('move_lines_to_parent', corpus=self.corpus, single_page_type='single_page', double_page_type='double_page')
for line in [same_x_text_line, same_y_text_line]:
diff --git a/arkindex/documents/tests/tasks/test_move_element.py b/arkindex/documents/tests/tasks/test_move_element.py
index 2bd029f1b2..4870ae5957 100644
--- a/arkindex/documents/tests/tasks/test_move_element.py
+++ b/arkindex/documents/tests/tasks/test_move_element.py
@@ -11,7 +11,6 @@ class TestMoveElement(FixtureTestCase):
@classmethod
def setUpTestData(cls):
super().setUpTestData()
- cls.page_type = cls.corpus.types.get(slug='page')
cls.destination = cls.corpus.elements.get(name='Volume 2')
cls.parent = cls.corpus.elements.get(name='Volume 1')
cls.source_with_children = cls.corpus.elements.get(name='Volume 1, page 1r')
@@ -30,7 +29,6 @@ class TestMoveElement(FixtureTestCase):
'source_id': str(self.source_without_child.id),
'parent_id': str(self.parent.id),
'destination_id': str(self.destination.id),
- 'page_type_id': str(self.page_type.id),
'savepoints': [f"s{_thread.get_ident()}_x{connections['default'].savepoint_state + 1}", f"s{_thread.get_ident()}_x{connections['default'].savepoint_state + 2}"]
}):
move_element(self.source_without_child, self.destination)
@@ -57,7 +55,6 @@ class TestMoveElement(FixtureTestCase):
'source_id': str(self.source_with_children.id),
'parent_id': str(self.parent.id),
'destination_id': str(self.destination.id),
- 'page_type_id': str(self.page_type.id),
'savepoints': [f"s{_thread.get_ident()}_x{connections['default'].savepoint_state + 1}", f"s{_thread.get_ident()}_x{connections['default'].savepoint_state + 2}"]
}):
move_element(self.source_with_children, self.destination)
diff --git a/arkindex/documents/tests/test_bulk_element_transcriptions.py b/arkindex/documents/tests/test_bulk_element_transcriptions.py
index 932b7258ad..d6a2a15853 100644
--- a/arkindex/documents/tests/test_bulk_element_transcriptions.py
+++ b/arkindex/documents/tests/test_bulk_element_transcriptions.py
@@ -106,8 +106,8 @@ class TestBulkElementTranscriptions(FixtureAPITestCase):
for elt in created_elts
],
[
- (1, '2', ((13, 37), (13, 137), (133, 137), (133, 37), (13, 37)), self.worker_version.id, self.local_worker_run.id),
- (2, '3', ((24, 42), (24, 142), (64, 142), (64, 42), (24, 42)), self.worker_version.id, self.local_worker_run.id)
+ (4, '5', ((13, 37), (13, 137), (133, 137), (133, 37), (13, 37)), self.worker_version.id, self.local_worker_run.id),
+ (5, '6', ((24, 42), (24, 142), (64, 142), (64, 42), (24, 42)), self.worker_version.id, self.local_worker_run.id)
]
)
self.assertCountEqual(
@@ -166,8 +166,8 @@ class TestBulkElementTranscriptions(FixtureAPITestCase):
for elt in created_elts
],
[
- (1, '2', ((13, 37), (13, 137), (133, 137), (133, 37), (13, 37)), self.worker_version.id, self.local_worker_run.id, 0.5),
- (2, '3', ((24, 42), (24, 142), (64, 142), (64, 42), (24, 42)), self.worker_version.id, self.local_worker_run.id, 0.7)
+ (4, '5', ((13, 37), (13, 137), (133, 137), (133, 37), (13, 37)), self.worker_version.id, self.local_worker_run.id, 0.5),
+ (5, '6', ((24, 42), (24, 142), (64, 142), (64, 42), (24, 42)), self.worker_version.id, self.local_worker_run.id, 0.7)
]
)
self.assertCountEqual(
@@ -859,7 +859,7 @@ class TestBulkElementTranscriptions(FixtureAPITestCase):
)
],
[
- (1, '2', self.worker_run.version.id, ['I <3 JavaScript']),
+ (4, '5', self.worker_run.version.id, ['I <3 JavaScript']),
]
)
@@ -902,6 +902,6 @@ class TestBulkElementTranscriptions(FixtureAPITestCase):
)
],
[
- (1, '2', local_worker_run.version.id, ['I <3 JavaScript']),
+ (4, '5', local_worker_run.version.id, ['I <3 JavaScript']),
]
)
diff --git a/arkindex/documents/tests/test_bulk_elements.py b/arkindex/documents/tests/test_bulk_elements.py
index 0e280a0c7e..e2d3a5f0f6 100644
--- a/arkindex/documents/tests/test_bulk_elements.py
+++ b/arkindex/documents/tests/test_bulk_elements.py
@@ -184,10 +184,10 @@ class TestBulkElements(FixtureAPITestCase):
self.assertEqual(a_path1.ordering, 0)
self.assertEqual(a_path2.ordering, 0)
- self.assertEqual(b_path1.ordering, 0)
- self.assertEqual(b_path2.ordering, 0)
- self.assertEqual(c_path1.ordering, 1)
- self.assertEqual(c_path2.ordering, 1)
+ self.assertEqual(b_path1.ordering, 1)
+ self.assertEqual(b_path2.ordering, 1)
+ self.assertEqual(c_path1.ordering, 2)
+ self.assertEqual(c_path2.ordering, 2)
def test_bulk_create_top_level(self):
@@ -499,8 +499,8 @@ class TestBulkElements(FixtureAPITestCase):
list(elements.values_list('name', 'type__slug', 'paths__ordering', 'polygon')),
[
('A', 'act', 0, LineString(self.payload['elements'][0]['polygon'])),
- ('B', 'surface', 0, LineString(self.payload['elements'][1]['polygon'])),
- ('C', 'surface', 1, LineString(self.payload['elements'][2]['polygon'])),
+ ('B', 'surface', 1, LineString(self.payload['elements'][1]['polygon'])),
+ ('C', 'surface', 2, LineString(self.payload['elements'][2]['polygon'])),
],
)
diff --git a/arkindex/documents/tests/test_children_elements.py b/arkindex/documents/tests/test_children_elements.py
index 645c6ec0bd..d016ff3f1c 100644
--- a/arkindex/documents/tests/test_children_elements.py
+++ b/arkindex/documents/tests/test_children_elements.py
@@ -76,8 +76,8 @@ class TestChildrenElements(FixtureAPITestCase):
self.assertListEqual(
[r['name'] for r in response.json()['results']],
[
- 'Act 2',
'Volume 1, page 2r',
+ 'Act 2',
]
)
@@ -381,10 +381,11 @@ class TestChildrenElements(FixtureAPITestCase):
'Act 5',
'Act 1',
'Act 2',
- 'Act 3'
+ 'Act 3',
]
+ offset = Element.objects.filter(paths__path__last=self.vol.id).exclude(type__slug='act').count()
for ordering, name in enumerate(expected):
- Element.objects.get(type__slug='act', name=name).paths.update(ordering=ordering)
+ Element.objects.get(type__slug='act', name=name).paths.update(ordering=ordering + offset)
response = self.client.get(
reverse('api:elements-children', kwargs={'pk': str(self.vol.id)})
@@ -500,16 +501,16 @@ class TestChildrenElements(FixtureAPITestCase):
reverse('api:elements-children', kwargs={'pk': str(page.id)}),
{'recursive': True},
)
- self.assertEqual(response.status_code, status.HTTP_200_OK)
- # cell_b and cell_c are duplicated due to a bug in Element.objects.get_descending
+ self.assertEqual(response.status_code, status.HTTP_200_OK)
+ # col1, cell_b and cell_c are duplicated due to a bug in Element.objects.get_descending
# See https://gitlab.com/teklia/arkindex/backend/-/issues/769
- self.assertEqual(response.json()['count'], 12)
self.assertCountEqual([item['id'] for item in response.json()['results']], [
str(table_1.id),
str(table_2.id),
str(row1.id),
str(row2.id),
str(col1.id),
+ str(col1.id),
str(col2.id),
str(cell_a.id),
str(cell_b.id),
diff --git a/arkindex/documents/tests/test_create_elements.py b/arkindex/documents/tests/test_create_elements.py
index ade8d13d2d..7b6ae2a6f3 100644
--- a/arkindex/documents/tests/test_create_elements.py
+++ b/arkindex/documents/tests/test_create_elements.py
@@ -124,7 +124,7 @@ class TestCreateElements(FixtureAPITestCase):
name='The castle of my dreams',
image=str(self.image.id),
)
- with self.assertNumQueries(16):
+ with self.assertNumQueries(15):
response = self.client.post(**request)
self.assertEqual(response.status_code, status.HTTP_201_CREATED)
data = response.json()
@@ -179,7 +179,7 @@ class TestCreateElements(FixtureAPITestCase):
name='The castle of my dreams',
polygon=polygon,
)
- with self.assertNumQueries(16):
+ with self.assertNumQueries(15):
response = self.client.post(**request)
self.assertEqual(response.status_code, status.HTTP_201_CREATED)
data = response.json()
@@ -199,7 +199,7 @@ class TestCreateElements(FixtureAPITestCase):
name='Castle story',
elt_type='act'
)
- with self.assertNumQueries(15):
+ with self.assertNumQueries(14):
response = self.client.post(**request)
self.assertEqual(response.status_code, status.HTTP_201_CREATED)
act = Element.objects.get(id=response.json()['id'])
@@ -249,7 +249,7 @@ class TestCreateElements(FixtureAPITestCase):
image=str(self.image.id),
polygon=polygon
)
- with self.assertNumQueries(16):
+ with self.assertNumQueries(15):
response = self.client.post(**request)
self.assertEqual(response.status_code, status.HTTP_201_CREATED)
data = response.json()
@@ -299,7 +299,7 @@ class TestCreateElements(FixtureAPITestCase):
image=str(self.huge_image.id),
polygon=polygon
)
- with self.assertNumQueries(16):
+ with self.assertNumQueries(15):
response = self.client.post(**request)
self.assertEqual(response.status_code, status.HTTP_201_CREATED, response.json())
self.assertListEqual(response.json()['zone']['polygon'], polygon)
@@ -366,7 +366,7 @@ class TestCreateElements(FixtureAPITestCase):
)
self.image.status = S3FileStatus.Error
self.image.save()
- with self.assertNumQueries(16):
+ with self.assertNumQueries(15):
response = self.client.post(**request)
self.assertEqual(response.status_code, status.HTTP_201_CREATED)
element = self.corpus.elements.get(id=response.json()['id'])
@@ -411,7 +411,7 @@ class TestCreateElements(FixtureAPITestCase):
image=str(self.image.id),
)
request['path'] += '?slim_output=true'
- with self.assertNumQueries(14):
+ with self.assertNumQueries(13):
response = self.client.post(**request)
self.assertEqual(response.status_code, status.HTTP_201_CREATED)
element = self.corpus.elements.get(id=response.json()['id'])
@@ -448,7 +448,7 @@ class TestCreateElements(FixtureAPITestCase):
if mirrored is not None:
request['data']['mirrored'] = mirrored
- with self.assertNumQueries(16):
+ with self.assertNumQueries(15):
response = self.client.post(**request)
self.assertEqual(response.status_code, status.HTTP_201_CREATED)
@@ -513,7 +513,7 @@ class TestCreateElements(FixtureAPITestCase):
confidence=0.42,
)
- with self.assertNumQueries(16):
+ with self.assertNumQueries(15):
response = self.client.post(**request)
self.assertEqual(response.status_code, status.HTTP_201_CREATED)
diff --git a/arkindex/documents/tests/test_edit_elementpath.py b/arkindex/documents/tests/test_edit_elementpath.py
index c69ca68193..980f87df36 100644
--- a/arkindex/documents/tests/test_edit_elementpath.py
+++ b/arkindex/documents/tests/test_edit_elementpath.py
@@ -96,7 +96,6 @@ class TestEditElementPath(FixtureTestCase):
# add_parent uses transaction.atomic(), and we are running in a unit test, which is already in a transaction.
# This will cause a savepoint to be created, with a name that is hard to mock.
'savepoint': f"s{_thread.get_ident()}_x{connections['default'].savepoint_state + 1}",
- 'type_id': elements['A'].type_id,
'A': elements['A'].id,
'B': elements['B'].id,
'X': elements['X'].id,
@@ -167,7 +166,6 @@ class TestEditElementPath(FixtureTestCase):
# add_parent uses transaction.atomic(), and we are running in a unit test, which is already in a transaction.
# This will cause a savepoint to be created, with a name that is hard to mock.
'savepoint': f"s{_thread.get_ident()}_x{connections['default'].savepoint_state + 1}",
- 'type_id': elements['A'].type_id,
'A': elements['A'].id,
'B': elements['B'].id,
'K': elements['K'].id,
@@ -344,6 +342,26 @@ class TestEditElementPath(FixtureTestCase):
self.check_parents(elements, 'F', ['X', 'A', 'B', 'E'],
['Y', 'A', 'B', 'E'])
+ path1, path2 = elements['B'].paths.filter(path__last=elements['A'].id)
+
+ # remove_child picks the first parent path to perform an update on it, but it really just needs any random one,
+ # therefore it is retrieved without an ordering. This can cause intermittent failures in tests since either
+ # X or Y will be picked. Adding an ORDER BY in the original method would not help much either since we cannot
+ # order by any value that would not change randomly during the execution.
+ # This class implements a workaround that works with assertExactQueries: the __str__ will be called after
+ # remove_child has run, and just before assertExactQueries checks the queries against the expected SQL.
+ # B will only have one remaining path: the first path that got picked by remove_child for the update.
+ # The other path will have been deleted. We can therefore get this remaining path and compare it by its ID
+ # to the two paths that we had before, and pick the parent that was in the old version of this path.
+ class FirstParent(object):
+ def __str__(self):
+ path_id = elements['B'].paths.get().id
+ if path1.id == path_id:
+ return str(path1.path[0])
+ elif path2.id == path_id:
+ return str(path2.path[0])
+ raise AssertionError('Unexpected top-level path ID')
+
with self.assertExactQueries(
'remove_child_last_parent.sql', params={
# remove_child uses transaction.atomic(), and we are running in a unit test, which is already in a transaction.
@@ -351,7 +369,7 @@ class TestEditElementPath(FixtureTestCase):
'savepoint': f"s{_thread.get_ident()}_x{connections['default'].savepoint_state + 1}",
'A': elements['A'].id,
'B': elements['B'].id,
- 'Y': elements['Y'].id
+ 'first_parent': FirstParent(),
}
):
elements['A'].remove_child(elements['B'])
diff --git a/arkindex/documents/tests/test_element_paths_api.py b/arkindex/documents/tests/test_element_paths_api.py
index 01c77092eb..27dca725a1 100644
--- a/arkindex/documents/tests/test_element_paths_api.py
+++ b/arkindex/documents/tests/test_element_paths_api.py
@@ -41,7 +41,7 @@ class TestElementsAPI(FixtureAPITestCase):
self.client.force_login(self.user)
# Link desk to its parent room
- with self.assertNumQueries(14):
+ with self.assertNumQueries(13):
response = self.client.post(reverse('api:element-parent', kwargs=self.default_kwargs))
self.assertEqual(response.status_code, status.HTTP_201_CREATED)
@@ -50,7 +50,7 @@ class TestElementsAPI(FixtureAPITestCase):
self.assertEqual(element_path.ordering, 0)
# Link room to its parent house
- with self.assertNumQueries(12):
+ with self.assertNumQueries(11):
response = self.client.post(reverse('api:element-parent', kwargs={
'child': str(self.room.id),
'parent': str(self.house.id)
@@ -61,50 +61,6 @@ class TestElementsAPI(FixtureAPITestCase):
self.assertListEqual(element_path.path, [self.house.id, self.room.id])
self.assertEqual(element_path.ordering, 0)
- def test_create_type_ordering(self):
- self.client.force_login(self.user)
- self.link_elements()
-
- # The desk is inside the kitchen in first position
- element_path = self.desk.paths.get()
- self.assertListEqual(element_path.path, [self.house.id, self.room.id])
- self.assertEqual(element_path.ordering, 0)
-
- person_type = self.corpus.types.create(slug='person', display_name='Person')
- person = self.corpus.elements.create(type=person_type, name='Brian')
-
- cases = [
- # With type ordering, Brian is the first person in the kitchen
- (True, 0),
- # Any value that is not 0 or false is treated as true
- ('yes', 0),
- # Including nothing
- ('', 0),
- # Without type ordering, Brian is the second element after the desk
- (False, 1),
- ('false', 1),
- ('0', 1),
- ]
- for type_ordering, expected_ordering in cases:
- # Reset all paths before testing
- person.paths.all().delete()
- person.paths.create(path=[], ordering=0)
-
- with self.subTest(type_ordering=type_ordering):
- with self.assertNumQueries(14):
- response = self.client.post(
- reverse('api:element-parent', kwargs={
- 'child': str(person.id),
- 'parent': str(self.room.id),
- })
- + f'?type_ordering={type_ordering}'
- )
- self.assertEqual(response.status_code, status.HTTP_201_CREATED)
-
- element_path = person.paths.get()
- self.assertListEqual(element_path.path, [self.house.id, self.room.id])
- self.assertEqual(element_path.ordering, expected_ordering)
-
def test_create_forbidden(self):
response = self.client.post(reverse('api:element-parent', kwargs=self.default_kwargs))
self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN)
diff --git a/arkindex/documents/tests/test_neighbors.py b/arkindex/documents/tests/test_neighbors.py
index 6757e1ff74..546467af3d 100644
--- a/arkindex/documents/tests/test_neighbors.py
+++ b/arkindex/documents/tests/test_neighbors.py
@@ -130,7 +130,7 @@ class TestElementNeighbors(FixtureAPITestCase):
'name': elements['Y'].name
}]
}, {
- 'position': 1,
+ 'position': 3,
'element': {
'id': str(element.id),
'type': 'volume',
diff --git a/arkindex/sql_validation/add_first_parent.sql b/arkindex/sql_validation/add_first_parent.sql
index 75693fbd32..f00ec074bf 100644
--- a/arkindex/sql_validation/add_first_parent.sql
+++ b/arkindex/sql_validation/add_first_parent.sql
@@ -25,14 +25,11 @@ SELECT EXISTS
FROM parent_paths),
(SELECT path
FROM parent_paths
- LIMIT 1) ;
-
-SELECT (MAX("documents_elementpath"."ordering") + 1) AS "max"
-FROM "documents_elementpath"
-INNER JOIN "documents_element" ON ("documents_elementpath"."element_id" = "documents_element"."id")
-WHERE ("documents_elementpath"."path"[array_length("documents_elementpath"."path", 1)] = '{A}'::uuid
- AND "documents_elementpath"."path" && (ARRAY['{A}'::uuid])::uuid[]
- AND "documents_element"."type_id" = '{type_id}'::uuid);
+ LIMIT 1),
+ (SELECT COALESCE(MAX(ordering) + 1, 0)
+ FROM documents_elementpath
+ WHERE path @ > ARRAY['{A}'::uuid]
+ AND path[array_length(path, 1)] = '{A}'::uuid ) ;
INSERT INTO documents_elementpath (id, element_id, path, ordering)
SELECT uuid_generate_v4(),
diff --git a/arkindex/sql_validation/add_second_parent.sql b/arkindex/sql_validation/add_second_parent.sql
index 794827a573..4aaa684eea 100644
--- a/arkindex/sql_validation/add_second_parent.sql
+++ b/arkindex/sql_validation/add_second_parent.sql
@@ -25,14 +25,11 @@ SELECT EXISTS
FROM parent_paths),
(SELECT path
FROM parent_paths
- LIMIT 1) ;
-
-SELECT (MAX("documents_elementpath"."ordering") + 1) AS "max"
-FROM "documents_elementpath"
-INNER JOIN "documents_element" ON ("documents_elementpath"."element_id" = "documents_element"."id")
-WHERE ("documents_elementpath"."path"[array_length("documents_elementpath"."path", 1)] = '{A}'::uuid
- AND "documents_elementpath"."path" && (ARRAY['{A}'::uuid])::uuid[]
- AND "documents_element"."type_id" = '{type_id}'::uuid);
+ LIMIT 1),
+ (SELECT COALESCE(MAX(ordering) + 1, 0)
+ FROM documents_elementpath
+ WHERE path @ > ARRAY['{A}'::uuid]
+ AND path[array_length(path, 1)] = '{A}'::uuid ) ;
INSERT INTO documents_elementpath (id, element_id, path, ordering)
SELECT uuid_generate_v4(),
diff --git a/arkindex/sql_validation/element_move_with_children.sql b/arkindex/sql_validation/element_move_with_children.sql
index 3645cc093a..b06caf357c 100644
--- a/arkindex/sql_validation/element_move_with_children.sql
+++ b/arkindex/sql_validation/element_move_with_children.sql
@@ -60,14 +60,11 @@ SELECT EXISTS
FROM parent_paths),
(SELECT path
FROM parent_paths
- LIMIT 1) ;
-
-SELECT (MAX("documents_elementpath"."ordering") + 1) AS "max"
-FROM "documents_elementpath"
-INNER JOIN "documents_element" ON ("documents_elementpath"."element_id" = "documents_element"."id")
-WHERE ("documents_elementpath"."path"[array_length("documents_elementpath"."path", 1)] = '{destination_id}'::uuid
- AND "documents_elementpath"."path" && (ARRAY['{destination_id}'::uuid])::uuid[]
- AND "documents_element"."type_id" = '{page_type_id}'::uuid);
+ LIMIT 1),
+ (SELECT COALESCE(MAX(ordering) + 1, 0)
+ FROM documents_elementpath
+ WHERE path @ > ARRAY['{destination_id}'::uuid]
+ AND path[array_length(path, 1)] = '{destination_id}'::uuid ) ;
UPDATE "documents_elementpath"
SET "path" = ARRAY['{destination_id}'::uuid]::uuid[], "ordering" = 3
diff --git a/arkindex/sql_validation/element_move_without_child.sql b/arkindex/sql_validation/element_move_without_child.sql
index 3645cc093a..b06caf357c 100644
--- a/arkindex/sql_validation/element_move_without_child.sql
+++ b/arkindex/sql_validation/element_move_without_child.sql
@@ -60,14 +60,11 @@ SELECT EXISTS
FROM parent_paths),
(SELECT path
FROM parent_paths
- LIMIT 1) ;
-
-SELECT (MAX("documents_elementpath"."ordering") + 1) AS "max"
-FROM "documents_elementpath"
-INNER JOIN "documents_element" ON ("documents_elementpath"."element_id" = "documents_element"."id")
-WHERE ("documents_elementpath"."path"[array_length("documents_elementpath"."path", 1)] = '{destination_id}'::uuid
- AND "documents_elementpath"."path" && (ARRAY['{destination_id}'::uuid])::uuid[]
- AND "documents_element"."type_id" = '{page_type_id}'::uuid);
+ LIMIT 1),
+ (SELECT COALESCE(MAX(ordering) + 1, 0)
+ FROM documents_elementpath
+ WHERE path @ > ARRAY['{destination_id}'::uuid]
+ AND path[array_length(path, 1)] = '{destination_id}'::uuid ) ;
UPDATE "documents_elementpath"
SET "path" = ARRAY['{destination_id}'::uuid]::uuid[], "ordering" = 3
diff --git a/arkindex/sql_validation/remove_child_last_parent.sql b/arkindex/sql_validation/remove_child_last_parent.sql
index b35c23b5a3..9dd30683ac 100644
--- a/arkindex/sql_validation/remove_child_last_parent.sql
+++ b/arkindex/sql_validation/remove_child_last_parent.sql
@@ -14,17 +14,17 @@ WHERE element_id = '{B}'::uuid ;
UPDATE documents_elementpath
SET path = path[3:]
-WHERE path @ > ARRAY['{Y}'::uuid,
+WHERE path @ > ARRAY['{first_parent}'::uuid,
'{A}'::uuid,
'{B}'::uuid]
- AND path[0:3] = ARRAY['{Y}'::uuid,
+ AND path[0:3] = ARRAY['{first_parent}'::uuid,
'{A}'::uuid,
'{B}'::uuid] ;
UPDATE "documents_elementpath"
SET "path" = '{{}}'::uuid[]
WHERE ("documents_elementpath"."element_id" = '{B}'::uuid
- AND "documents_elementpath"."path" = (ARRAY['{Y}'::uuid,
+ AND "documents_elementpath"."path" = (ARRAY['{first_parent}'::uuid,
'{A}'::uuid])::uuid[]);
DELETE
--
GitLab