Merge branch 'deletion' into 'master'

Improve element deletion performance

Closes #362 and #300

See merge request !1015

arkindex/documents/api/elements.py

@@ -436,14 +436,6 @@ class ElementRetrieve(RetrieveUpdateDestroyAPIView):
         elif request.method != 'GET' and Right.Write not in rights:
             self.permission_denied(request, message='You do not have write access to this element.')
 
-    def perform_destroy(self, instance):
-        children_count = ElementPath.objects.filter(path__contains=[instance.id]).count()
-        if children_count:
-            raise ValidationError({
-                'element': "Element '{}' is linked to {} children elements".format(instance.id, children_count)
-            })
-        super().perform_destroy(instance)
-
     def get_serializer_context(self):
         context = super().get_serializer_context()
         if self.request and self.request.method != 'GET':

arkindex/documents/management/commands/delete.py

+#!/usr/bin/env python3
+from django.core.management.base import BaseCommand
+from arkindex.documents.models import Element
+import logging
+import uuid
+
+logging.basicConfig(
+    level=logging.INFO,
+    format='[%(levelname)s] %(message)s',
+)
+logger = logging.getLogger(__name__)
+
+
+def valid_uuid(value):
+    try:
+        return uuid.UUID(value)
+    except ValueError:
+        raise Exception(f"Not an UUID: {value}")
+
+
+class Command(BaseCommand):
+    help = 'Delete an element and its children'
+
+    def add_arguments(self, parser):
+        super().add_arguments(parser)
+        parser.add_argument(
+            'element_id',
+            help='ID of the top element to delete (multiple supported)',
+            type=valid_uuid,
+            nargs='+',
+        )
+
+    def handle(self, element_id, verbosity=0, **options):
+        for element in Element.objects.filter(id__in=element_id):
+            logger.info(f"Deleting {element.id} : {element}")
+            element.delete()

arkindex/documents/signals.py

 from django.db.models.signals import pre_delete
 from django.dispatch import receiver
-from arkindex.documents.models import Element, ElementPath
+from django.db.models import Q
+from arkindex.documents.models import Element, ElementPath, Transcription, Classification, MetaData, Selection, TranscriptionEntity
+from arkindex.dataimport.models import DataImportElement, DataImport
+import logging
+
+logger = logging.getLogger(__name__)
 
 
 @receiver(pre_delete, sender=Element)
-def pre_delete_handler(sender, instance, *args, **kwargs):
+def pre_delete_handler(sender, instance, using, *args, **kwargs):
     assert isinstance(instance, Element)
-    # Remove the instance's children
-    for child in Element.objects.filter(paths__path__last=instance.id):
-        instance.remove_child(child)
-    # Remove instance from its parents
-    parent_ids = [epath.path[-1] for epath in ElementPath.objects.filter(element=instance)]
-    for parent in Element.objects.filter(id__in=parent_ids):
-        parent.remove_child(instance)
+
+    # Build queryset to lookup all children, with or without top element
+    children = Element.objects.get_descending(instance.id).values('id')
+    elements = Q(element_id__in=children) | Q(element_id=instance.id)
+
+    # Remove all transcription entity links
+    deleted, _ = TranscriptionEntity.objects.filter(Q(transcription__element_id__in=children) | Q(transcription__element_id=instance.id)).delete()
+    logger.info(f"Deleted {deleted} transcriptions entities")
+
+    # Remove all transcriptions
+    deleted, _ = Transcription.objects.filter(elements).delete()
+    logger.info(f"Deleted {deleted} transcriptions")
+
+    # Remove all classifications
+    deleted, _ = Classification.objects.filter(elements).delete()
+    logger.info(f"Deleted {deleted} classifications")
+
+    # Remove all metadatas
+    deleted, _ = MetaData.objects.filter(elements).delete()
+    logger.info(f"Deleted {deleted} metadatas")
+
+    # Remove all selections from users
+    deleted, _ = Selection.objects.filter(elements).delete()
+    logger.info(f"Deleted {deleted} user selections")
+
+    # Remove all usage from a dataimport from users selection
+    deleted, _ = DataImportElement.objects.filter(elements).delete()
+    logger.info(f"Deleted {deleted} usage from dataimport as selection")
+
+    # Remove all direct usage on a dataimport, without removing the dataimport
+    updated = DataImport.objects.filter(elements).update(element_id=None)
+    logger.info(f"Deleted {updated} usage from dataimport as element")
+
+    # Save children IDs reference before deleting the paths
+    # or the final element deletion query will never get anything to delete
+    children_ids = list(children.values_list('id', flat=True))
+
+    # Remove all paths for these elements
+    deleted, _ = ElementPath.objects.filter(elements).delete()
+    logger.info(f"Deleted {deleted} paths")
+
+    # Finally Remove all children, calling this pre_delete signal recursively
+    # The instance element will be deleted by the parent delete() call
+    deleted = Element.objects.filter(id__in=children_ids)._raw_delete(using=using)
+    logger.info(f"Deleted {deleted} children")

arkindex/documents/tests/test_edit_elementpath.py

 from arkindex.project.tests import FixtureTestCase
 from arkindex.project.tools import build_tree
-from arkindex.documents.models import ElementPath
+from arkindex.documents.models import ElementPath, Element
 import itertools
 
 
@@ -299,12 +299,19 @@ class TestEditElementPath(FixtureTestCase):
 
         elements['B'].delete()
 
+        # Check parents still exists
+        for parent in ('X', 'Y', 'A', 'C'):
+            elements[parent].refresh_from_db()
+            self.assertIsNotNone(elements[parent].id)
+
         self.check_parents(elements, 'X')
         self.check_parents(elements, 'Y')
         self.check_parents(elements, 'A', ['X'],
                                           ['Y'])
         self.check_parents(elements, 'C', ['X', 'A'],
                                           ['Y', 'A'])
-        self.check_parents(elements, 'D')
-        self.check_parents(elements, 'E')
-        self.check_parents(elements, 'F', ['E'])
+
+        # Check all children are deleted
+        for child in ('B', 'D', 'E', 'F'):
+            with self.assertRaises(Element.DoesNotExist):
+                elements[child].refresh_from_db()

arkindex/documents/tests/test_retrieve_elements.py

@@ -3,7 +3,7 @@ from rest_framework import status
 from arkindex_common.enums import MetaType, EntityType
 from arkindex_common.ml_tool import MLToolType
 from arkindex.documents.models import \
-    DataSource, Corpus, Entity, MLClass, Classification
+    DataSource, Corpus, Entity, MLClass, Classification, Element
 from arkindex.project.tests import FixtureAPITestCase
 
 
@@ -128,13 +128,15 @@ class TestRetrieveElements(FixtureAPITestCase):
         self.assertIsNone(response.json()['thumbnail_put_url'])
 
     def test_non_empty_folder(self):
+        """
+        We can now delete a non-empty folder
+        """
         self.client.force_login(self.user)
         response = self.client.delete(reverse('api:element-retrieve', kwargs={'pk': str(self.vol.id)}))
-        self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
-        self.assertDictEqual(
-            response.json(),
-            {'element': "Element '{}' is linked to 15 children elements".format(self.vol.id)}
-        )
+        self.assertEqual(response.status_code, status.HTTP_204_NO_CONTENT)
+
+        with self.assertRaises(Element.DoesNotExist):
+            self.vol.refresh_from_db()
 
     def test_list_element_metadata(self):
         with self.assertNumQueries(6):