fix landing_zone_create crash with large projects (#1905)

CHANGELOG.rst

@@ -19,6 +19,8 @@ Added
     - UI tests for project details card (#1902)
 - **Taskflowbackend**
     - Django settings in siteinfo (#1901)
+    - ``BatchSetAccessTask`` in iRODS tasks (#1905)
+    - ``IrodsAccessMixin`` task helper mixin (#1905)
 
 Changed
 -------
@@ -62,6 +64,7 @@ Fixed
 - **Taskflowbackend**
     - Hardcoded iRODS path length in ``landing_zone_move`` (#1888)
     - Uncaught exceptions in ``SetAccessTask`` (#1906)
+    - Crash in ``landing_zone_create`` with large amount of collections (#1905)
 
 Removed
 -------

docs_manual/source/sodar_release_notes.rst

@@ -20,6 +20,7 @@ Release for minor updates, maintenance and bug fixes.
 - Fix LDAP TLS settings
 - Fix iRODS stats badge stuck in "updating"
 - Fix landing zone status updating not working in project details card
+- Fix landing zone creation crash with large amount of created collections
 - Minor updates and bug fixes
 - Upgrade to SODAR Core v0.13.4
 

taskflowbackend/flows/landing_zone_create.py

@@ -154,26 +154,30 @@ def build(self, force_fail=False):
                 )
             )
         # Create collections
-        for d in self.flow_data['colls']:
-            coll_path = os.path.join(zone_path, d)
+        if self.flow_data['colls']:
+            colls_full_path = [
+                os.path.join(zone_path, c) for c in self.flow_data['colls']
+            ]
+            coll_count = len(self.flow_data['colls'])
             self.add_task(
-                irods_tasks.CreateCollectionTask(
-                    name='Create collection {}'.format(coll_path),
+                irods_tasks.BatchCreateCollectionsTask(
+                    name='Batch create {} collection{}'.format(
+                        coll_count, 's' if coll_count != 1 else ''
+                    ),
                     irods=self.irods,
-                    inject={'path': coll_path},
+                    inject={'paths': colls_full_path},
                 )
             )
             # Enforce collection access if set
             if self.flow_data['restrict_colls']:
                 self.add_task(
-                    irods_tasks.SetAccessTask(
-                        name='Set user owner access to collection {}'.format(
-                            coll_path
-                        ),
+                    irods_tasks.BatchSetAccessTask(
+                        name='Batch set user owner access to created '
+                        'collections',
                         irods=self.irods,
                         inject={
                             'access_name': 'own',
-                            'path': coll_path,
+                            'paths': colls_full_path,
                             'user_name': zone.user.username,
                             'access_lookup': access_lookup,
                             'irods_backend': self.irods_backend,

taskflowbackend/tasks/irods_tasks.py

@@ -26,6 +26,96 @@
 MD5_RE = re.compile(r'([^\w.])')
 
 
+# Mixins -----------------------------------------------------------------------
+
+
+class IrodsAccessMixin:
+    """Mixin for iRODS access helpers"""
+
+    def execute_set_access(
+        self,
+        access_name,
+        path,
+        user_name,
+        access_lookup,
+        obj_target,
+        recursive,
+    ):
+        """
+        Set access for user in a single data object or collection.
+
+        :param access_name: Access level to set (string)
+        :param path: Full iRODS path to collection or data object (string)
+        :param user_name: Name of user or group (string)
+        :param access_lookup: Access level lookup for iRODS compatibility (dict)
+        :param obj_target: Whether target is a data object (boolean)
+        :param recursive: Set collection access recursively if True (boolean)
+        """
+        if not self.execute_data.get('access_names'):
+            self.execute_data['access_names'] = {}
+        if obj_target:
+            target = self.irods.data_objects.get(path)
+            recursive = False
+        else:
+            target = self.irods.collections.get(path)
+            recursive = recursive
+        target_access = self.irods.permissions.get(target=target)
+
+        user_access = next(
+            (x for x in target_access if x.user_name == user_name), None
+        )
+        modifying_data = False
+        if (
+            user_access
+            and user_access.access_name != access_lookup[access_name]
+        ):
+            self.execute_data['access_names'][path] = access_lookup[
+                user_access.access_name
+            ]
+            modifying_data = True
+        elif not user_access:
+            self.execute_data['access_names'][path] = 'null'
+            modifying_data = True
+
+        if modifying_data:
+            acl = iRODSAccess(
+                access_name=access_name,
+                path=path,
+                user_name=user_name,
+                user_zone=self.irods.zone,
+            )
+            self.irods.permissions.set(acl, recursive=recursive)
+            self.data_modified = True  # Access was modified
+
+    def revert_set_access(
+        self,
+        path,
+        user_name,
+        obj_target,
+        recursive,
+    ):
+        """
+        Revert setting access for user in a single collection or data object.
+
+        :param path: Full iRODS path to collection or data object (string)
+        :param user_name: Name of user or group (string)
+        :param obj_target: Whether target is a data object (boolean)
+        :param recursive: Set collection access recursively if True (boolean)
+        """
+        if self.data_modified:
+            acl = iRODSAccess(
+                access_name=self.execute_data['access_names'][path],
+                path=path,
+                user_name=user_name,
+                user_zone=self.irods.zone,
+            )
+            recursive = False if obj_target else recursive
+            self.irods.permissions.set(acl, recursive=recursive)
+
+
+# Base Task --------------------------------------------------------------------
+
+
 class IrodsBaseTask(BaseTask):
     """Base iRODS task"""
 
@@ -51,6 +141,9 @@ def _raise_irods_exception(self, ex, info=None):
         raise Exception(desc)
 
 
+# Tasks ------------------------------------------------------------------------
+
+
 class CreateCollectionTask(IrodsBaseTask):
     """
     Create collection and its parent collections if they doesn't exist (imkdir)
@@ -274,7 +367,7 @@ def revert(self, path, inherit=True, *args, **kwargs):
         '''
 
 
-class SetAccessTask(IrodsBaseTask):
+class SetAccessTask(IrodsAccessMixin, IrodsBaseTask):
     """
     Set user/group access to target (ichmod). If the target is a data object
     (obj_target=True), the recursive argument will be ignored.
@@ -293,44 +386,16 @@ def execute(
         **kwargs
     ):
         try:
-            if obj_target:
-                target = self.irods.data_objects.get(path)
-                recursive = False
-            else:
-                target = self.irods.collections.get(path)
-                recursive = recursive
-            target_access = self.irods.permissions.get(target=target)
-        except Exception as ex:
-            self._raise_irods_exception(ex, path)
-
-        user_access = next(
-            (x for x in target_access if x.user_name == user_name), None
-        )
-        modifying_data = False
-        if (
-            user_access
-            and user_access.access_name != access_lookup[access_name]
-        ):
-            self.execute_data['access_name'] = access_lookup[
-                user_access.access_name
-            ]
-            modifying_data = True
-        elif not user_access:
-            self.execute_data['access_name'] = 'null'
-            modifying_data = True
-
-        if modifying_data:
-            acl = iRODSAccess(
-                access_name=access_name,
-                path=path,
-                user_name=user_name,
-                user_zone=self.irods.zone,
+            self.execute_set_access(
+                access_name,
+                path,
+                user_name,
+                access_lookup,
+                obj_target,
+                recursive,
             )
-            try:
-                self.irods.permissions.set(acl, recursive=recursive)
-            except Exception as ex:
-                self._raise_irods_exception(ex, user_name)
-            self.data_modified = True
+        except Exception as ex:
+            self._raise_irods_exception(ex, user_name)
         super().execute(*args, **kwargs)
 
     def revert(
@@ -345,15 +410,10 @@ def revert(
         *args,
         **kwargs
     ):
-        if self.data_modified:
-            acl = iRODSAccess(
-                access_name=self.execute_data['access_name'],
-                path=path,
-                user_name=user_name,
-                user_zone=self.irods.zone,
-            )
-            recursive = False if obj_target else recursive
-            self.irods.permissions.set(acl, recursive=recursive)
+        try:
+            self.revert_set_access(path, user_name, obj_target, recursive)
+        except Exception:
+            pass  # TODO: Log revert() exceptions?
 
 
 class IssueTicketTask(IrodsBaseTask):
@@ -496,7 +556,53 @@ def revert(self, src_path, dest_path, *args, **kwargs):
             self.irods.data_objects.move(src_path=new_src, dest_path=new_dest)
 
 
-# Batch file tasks -------------------------------------------------------------
+# Batch Tasks ------------------------------------------------------------------
+
+
+class BatchSetAccessTask(IrodsAccessMixin, IrodsBaseTask):
+    """
+    Set user/group access to multiple targets (ichmod). If a target is a data
+    object (obj_target=True), the recursive argument will be ignored.
+    """
+
+    def execute(
+        self,
+        access_name,
+        paths,
+        user_name,
+        access_lookup,
+        irods_backend,
+        obj_target=False,
+        recursive=True,
+        *args,
+        **kwargs
+    ):
+        # NOTE: Exception handling is done within execute_set_access()
+        for path in paths:
+            self.execute_set_access(
+                access_name,
+                path,
+                user_name,
+                access_lookup,
+                obj_target,
+                recursive,
+            )
+        super().execute(*args, **kwargs)
+
+    def revert(
+        self,
+        access_name,
+        paths,
+        user_name,
+        access_lookup,
+        irods_backend,
+        obj_target=False,
+        recursive=True,
+        *args,
+        **kwargs
+    ):
+        for path in paths:
+            self.revert_set_access(path, user_name, obj_target, recursive)
 
 
 class BatchCheckFilesTask(IrodsBaseTask):
@@ -666,6 +772,7 @@ def execute(
                     'Error getting permissions of target "{}"'.format(target),
                 )
 
+            # TODO: Remove repetition, use IrodsAccessMixin
             user_access = next(
                 (x for x in target_access if x.user_name == user_name), None
             )