From 69b5bbb09ae302532c6f6800879490dcd5a2ef37 Mon Sep 17 00:00:00 2001
From: Erwan Rouchet <rouchet@teklia.com>
Date: Thu, 1 Jul 2021 16:16:28 +0200
Subject: [PATCH] Ensure child path unicity in add_parent when there are
multiple parents
---
arkindex/documents/models.py | 24 ++++---
arkindex/documents/tasks.py | 6 +-
.../documents/tests/test_edit_elementpath.py | 20 ++++++
.../element_move_with_children.sql | 71 +++----------------
.../element_move_without_child.sql | 15 ++--
5 files changed, 51 insertions(+), 85 deletions(-)
diff --git a/arkindex/documents/models.py b/arkindex/documents/models.py
index 387a81502f..c4f60c2093 100644
--- a/arkindex/documents/models.py
+++ b/arkindex/documents/models.py
@@ -181,7 +181,8 @@ class Element(IndexableModel):
# No existing parent, adding new line
parent_paths = [[]]
- new_paths = []
+ # Set of (element_id, path, ordering) tuples, to ensure path unicity
+ new_paths = set()
for parent_path in parent_paths:
# Build new full path and check it does not already exist
@@ -194,10 +195,10 @@ class Element(IndexableModel):
raise ValueError('Cannot add a descendant as a parent')
# Try to immediately insert
- new_paths.append(ElementPath(
- element=self,
- path=path,
- ordering=order,
+ new_paths.add((
+ self.id,
+ tuple(path),
+ order,
))
# Update children's paths in a non optimized way as this is pretty rare
@@ -215,10 +216,10 @@ class Element(IndexableModel):
# Add new paths to create for the children
for parent_path in parent_paths:
- new_paths.append(ElementPath(
- element=child.element,
- path=parent_path + [parent.id] + base_path,
- ordering=child.ordering,
+ new_paths.add((
+ child.element_id,
+ tuple(parent_path + [parent.id] + base_path),
+ child.ordering,
))
# Clear old children paths, if any leftover
@@ -226,7 +227,10 @@ class Element(IndexableModel):
ElementPath.objects.filter(id__in=to_delete).delete()
# All new paths are finally created
- ElementPath.objects.bulk_create(new_paths)
+ ElementPath.objects.bulk_create(
+ ElementPath(element_id=element_id, path=path, ordering=ordering)
+ for element_id, path, ordering in new_paths
+ )
def get_next_order(self, type):
"""
diff --git a/arkindex/documents/tasks.py b/arkindex/documents/tasks.py
index 74ea767bb8..9ec17df031 100644
--- a/arkindex/documents/tasks.py
+++ b/arkindex/documents/tasks.py
@@ -170,7 +170,7 @@ def worker_results_delete(corpus_id: str, version_id: str, parent_id: str) -> No
@job('high')
def move_element(source: Element, destination: Element) -> None:
- paths = ElementPath.objects.filter(element_id=source.id)
- for path in paths:
- Element.objects.get(id=path.path[-1]).remove_child(source)
+ parents = Element.objects.filter(id__in=source.paths.values('path__last'))
+ for parent in parents:
+ parent.remove_child(source)
source.add_parent(destination)
diff --git a/arkindex/documents/tests/test_edit_elementpath.py b/arkindex/documents/tests/test_edit_elementpath.py
index 1c1ea24019..26fdea474f 100644
--- a/arkindex/documents/tests/test_edit_elementpath.py
+++ b/arkindex/documents/tests/test_edit_elementpath.py
@@ -316,3 +316,23 @@ class TestEditElementPath(FixtureTestCase):
for child in ('B', 'D', 'E', 'F'):
with self.assertRaises(Element.DoesNotExist):
elements[child].refresh_from_db()
+
+ def test_add_many_parents(self):
+ """
+ Ensure that add_parent still works when adding a lot of parents.
+ We previously had issues that were not detected in unit tests because we were not testing with enough parents!
+ """
+ parent_names = [f'Parent {i}' for i in range(50)]
+ elements = build_tree(
+ {
+ 'Child 1': ['Element'],
+ 'Child 2': ['Element'],
+ 'Element': parent_names,
+ },
+ corpus=self.corpus,
+ type=self.element_type,
+ )
+
+ self.check_parents(elements, 'Element', *[[parent_name] for parent_name in parent_names])
+ self.check_parents(elements, 'Child 1', *[[parent_name, 'Element'] for parent_name in parent_names])
+ self.check_parents(elements, 'Child 2', *[[parent_name, 'Element'] for parent_name in parent_names])
diff --git a/arkindex/sql_validation/element_move_with_children.sql b/arkindex/sql_validation/element_move_with_children.sql
index ce66cc7e13..d33b7a2f17 100644
--- a/arkindex/sql_validation/element_move_with_children.sql
+++ b/arkindex/sql_validation/element_move_with_children.sql
@@ -1,10 +1,3 @@
-SELECT "documents_elementpath"."id",
- "documents_elementpath"."element_id",
- "documents_elementpath"."path",
- "documents_elementpath"."ordering"
-FROM "documents_elementpath"
-WHERE "documents_elementpath"."element_id" = '{source_id}'::uuid;
-
SELECT "documents_element"."id",
"documents_element"."created",
"documents_element"."updated",
@@ -14,8 +7,10 @@ SELECT "documents_element"."id",
"documents_element"."zone_id",
"documents_element"."worker_version_id"
FROM "documents_element"
-WHERE "documents_element"."id" = '{parent_id}'::uuid
-LIMIT 21;
+WHERE "documents_element"."id" IN
+ (SELECT U0."path"[array_length(U0."path", 1)]
+ FROM "documents_elementpath" U0
+ WHERE U0."element_id" = '{source_id}'::uuid);
SAVEPOINT "{savepoints[0]}";
@@ -170,54 +165,6 @@ SELECT "documents_elementpath"."id",
FROM "documents_elementpath"
WHERE "documents_elementpath"."path" @ > ARRAY['{source_id}'::uuid]::uuid[];
-SELECT "documents_element"."id",
- "documents_element"."created",
- "documents_element"."updated",
- "documents_element"."corpus_id",
- "documents_element"."type_id",
- "documents_element"."name",
- "documents_element"."zone_id",
- "documents_element"."worker_version_id"
-FROM "documents_element"
-WHERE "documents_element"."id" = '{children_ids[0]}'::uuid
-LIMIT 21;
-
-SELECT "documents_element"."id",
- "documents_element"."created",
- "documents_element"."updated",
- "documents_element"."corpus_id",
- "documents_element"."type_id",
- "documents_element"."name",
- "documents_element"."zone_id",
- "documents_element"."worker_version_id"
-FROM "documents_element"
-WHERE "documents_element"."id" = '{children_ids[1]}'::uuid
-LIMIT 21;
-
-SELECT "documents_element"."id",
- "documents_element"."created",
- "documents_element"."updated",
- "documents_element"."corpus_id",
- "documents_element"."type_id",
- "documents_element"."name",
- "documents_element"."zone_id",
- "documents_element"."worker_version_id"
-FROM "documents_element"
-WHERE "documents_element"."id" = '{children_ids[2]}'::uuid
-LIMIT 21;
-
-SELECT "documents_element"."id",
- "documents_element"."created",
- "documents_element"."updated",
- "documents_element"."corpus_id",
- "documents_element"."type_id",
- "documents_element"."name",
- "documents_element"."zone_id",
- "documents_element"."worker_version_id"
-FROM "documents_element"
-WHERE "documents_element"."id" = '{children_ids[3]}'::uuid
-LIMIT 21;
-
DELETE
FROM "documents_elementpath"
WHERE "documents_elementpath"."id" IN ('{paths_ids[0]}'::uuid,
@@ -229,10 +176,10 @@ INSERT INTO "documents_elementpath" ("id",
"element_id",
"path",
"ordering")
-VALUES ('{paths_ids[4]}'::uuid, '{source_id}'::uuid, ARRAY['{destination_id}'::uuid]::uuid[], 3),
+VALUES ('{paths_ids[4]}'::uuid, '{children_ids[2]}'::uuid, ARRAY['{destination_id}'::uuid,'{source_id}'::uuid]::uuid[], 0),
('{paths_ids[5]}'::uuid, '{children_ids[0]}'::uuid, ARRAY['{destination_id}'::uuid,'{source_id}'::uuid]::uuid[], 1),
- ('{paths_ids[6]}'::uuid, '{children_ids[1]}'::uuid, ARRAY['{destination_id}'::uuid,'{source_id}'::uuid]::uuid[], 2),
- ('{paths_ids[7]}'::uuid, '{children_ids[2]}'::uuid, ARRAY['{destination_id}'::uuid,'{source_id}'::uuid]::uuid[], 0),
- ('{paths_ids[8]}'::uuid, '{children_ids[3]}'::uuid, ARRAY['{destination_id}'::uuid,'{source_id}'::uuid]::uuid[], 0);
+ ('{paths_ids[6]}'::uuid, '{source_id}'::uuid, ARRAY['{destination_id}'::uuid]::uuid[], 3),
+ ('{paths_ids[7]}'::uuid, '{children_ids[3]}'::uuid, ARRAY['{destination_id}'::uuid,'{source_id}'::uuid]::uuid[], 0),
+ ('{paths_ids[8]}'::uuid, '{children_ids[1]}'::uuid, ARRAY['{destination_id}'::uuid,'{source_id}'::uuid]::uuid[], 2);
-RELEASE SAVEPOINT "{savepoints[1]}"
\ No newline at end of file
+RELEASE SAVEPOINT "{savepoints[1]}"
diff --git a/arkindex/sql_validation/element_move_without_child.sql b/arkindex/sql_validation/element_move_without_child.sql
index 4f40c8fef6..8074398184 100644
--- a/arkindex/sql_validation/element_move_without_child.sql
+++ b/arkindex/sql_validation/element_move_without_child.sql
@@ -1,10 +1,3 @@
-SELECT "documents_elementpath"."id",
- "documents_elementpath"."element_id",
- "documents_elementpath"."path",
- "documents_elementpath"."ordering"
-FROM "documents_elementpath"
-WHERE "documents_elementpath"."element_id" = '{source_id}'::uuid;
-
SELECT "documents_element"."id",
"documents_element"."created",
"documents_element"."updated",
@@ -14,8 +7,10 @@ SELECT "documents_element"."id",
"documents_element"."zone_id",
"documents_element"."worker_version_id"
FROM "documents_element"
-WHERE "documents_element"."id" = '{parent_id}'::uuid
-LIMIT 21;
+WHERE "documents_element"."id" IN
+ (SELECT U0."path"[array_length(U0."path", 1)]
+ FROM "documents_elementpath" U0
+ WHERE U0."element_id" = '{source_id}'::uuid);
SAVEPOINT "{savepoints[0]}";
@@ -104,4 +99,4 @@ INSERT INTO "documents_elementpath" ("id",
"ordering")
VALUES ('{path_id}'::uuid, '{source_id}'::uuid, ARRAY['{destination_id}'::uuid]::uuid[], 3);
-RELEASE SAVEPOINT "{savepoints[1]}"
\ No newline at end of file
+RELEASE SAVEPOINT "{savepoints[1]}"
--
GitLab