68150: resolve collection handles to collection in XmlWorkflowFactory & forgotten exception

dspace-api/src/main/java/org/dspace/xmlworkflow/XmlWorkflowFactoryImpl.java

@@ -17,6 +17,7 @@ import org.apache.logging.log4j.Logger;
 import org.dspace.content.Collection;
 import org.dspace.content.service.CollectionService;
 import org.dspace.core.Context;
+import org.dspace.handle.service.HandleService;
 import org.dspace.utils.DSpace;
 import org.dspace.xmlworkflow.factory.XmlWorkflowFactory;
 import org.dspace.xmlworkflow.state.Workflow;
@@ -25,9 +26,12 @@ import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.beans.factory.annotation.Required;
 
 /**
- * The workflowfactory is responsible for parsing the
- * workflow xml file and is used to retrieve the workflow for
- * a certain collection
+ * The workflowfactory is responsible for parsing the workflow xml file and is used to retrieve info about the workflow:
+ * - the workflow for a certain collection
+ * - collections mapped to a certain workflow
+ * - collections not mapped to any workflow
+ * - configured workflows and the default workflow
+ * - workflow action by name
  *
  * @author Bram De Schouwer (bram.deschouwer at dot com)
  * @author Kevin Van de Velde (kevin at atmire dot com)
@@ -46,6 +50,9 @@ public class XmlWorkflowFactoryImpl implements XmlWorkflowFactory {
     @Autowired(required = true)
     protected CollectionService collectionService;
 
+    @Autowired(required = true)
+    protected HandleService handleService;
+
     @Override
     public Workflow getWorkflow(Collection collection) throws WorkflowConfigurationException {
         // Attempt to retrieve our workflow object
@@ -89,30 +96,38 @@ public class XmlWorkflowFactoryImpl implements XmlWorkflowFactory {
     }
 
     @Override
-    public List<String> getCollectionHandlesMappedToWorklow(String workflowName) {
-        List<String> collectionsMapped = new ArrayList<>();
+    public List<Collection> getCollectionHandlesMappedToWorklow(Context context, String workflowName) {
+        List<Collection> collectionsMapped = new ArrayList<>();
         for (String handle : this.workflowMapping.keySet()) {
             if (this.workflowMapping.get(handle).getID().equals(workflowName)) {
-                collectionsMapped.add(handle);
+                try {
+                    Collection collection = (Collection) handleService.resolveToObject(context, handle);
+                    if (collection != null) {
+                        collectionsMapped.add(collection);
+                    }
+                } catch (SQLException e) {
+                    log.error("SQLException in XmlWorkflowFactoryImpl.getCollectionHandlesMappedToWorklow trying to " +
+                            "retrieve collection with handle: " + handle, e);
+                }
             }
         }
         return collectionsMapped;
     }
 
     @Override
-    public List<String> getAllNonMappedCollectionsHandles(Context context) {
-        List<String> nonMappedCollectionHandles = new ArrayList<>();
+    public List<Collection> getAllNonMappedCollectionsHandles(Context context) {
+        List<Collection> nonMappedCollections = new ArrayList<>();
         try {
             for (Collection collection : this.collectionService.findAll(context)) {
                 if (workflowMapping.get(collection.getHandle()) == null) {
-                    nonMappedCollectionHandles.add(collection.getHandle());
+                    nonMappedCollections.add(collection);
                 }
             }
         } catch (SQLException e) {
             log.error("SQLException in XmlWorkflowFactoryImpl.getAllNonMappedCollectionsHandles trying to " +
-                    "retrieve all collections");
+                    "retrieve all collections", e);
         }
-        return nonMappedCollectionHandles;
+        return nonMappedCollections;
     }
 
     @Override
@@ -141,10 +156,7 @@ public class XmlWorkflowFactoryImpl implements XmlWorkflowFactory {
     public WorkflowActionConfig getActionByName(String workflowActionName) {
         WorkflowActionConfig actionConfig
                 = new DSpace().getServiceManager().getServiceByName(workflowActionName, WorkflowActionConfig.class);
-        if (actionConfig != null) {
-            return actionConfig;
-        }
-        return null;
+        return actionConfig;
     }
 
 }

dspace-api/src/main/java/org/dspace/xmlworkflow/factory/XmlWorkflowFactory.java

@@ -16,9 +16,12 @@ import org.dspace.xmlworkflow.state.Workflow;
 import org.dspace.xmlworkflow.state.actions.WorkflowActionConfig;
 
 /**
- * The xmlworkflowfactory is responsible for parsing the
- * workflow xml file and is used to retrieve the workflow for
- * a certain collection
+ * The workflowfactory is responsible for parsing the workflow xml file and is used to retrieve info about the workflow:
+ * - the workflow for a certain collection
+ * - collections mapped to a certain workflow
+ * - collections not mapped to any workflow
+ * - configured workflows and the default workflow
+ * - workflow action by name
  *
  * @author Bram De Schouwer (bram.deschouwer at dot com)
  * @author Kevin Van de Velde (kevin at atmire dot com)
@@ -28,8 +31,6 @@ import org.dspace.xmlworkflow.state.actions.WorkflowActionConfig;
  */
 public interface XmlWorkflowFactory {
 
-    public final String LEGACY_WORKFLOW_NAME = "default";
-
     /**
      * Retrieve the workflow configuration for a single collection
      *
@@ -60,7 +61,6 @@ public interface XmlWorkflowFactory {
      *
      * @param workflowName Name of a possible configured workflow
      * @return True if there is a workflow configured by this name, false otherwise
-     * @throws WorkflowConfigurationException occurs if there is no workflow configured by that name
      */
     public boolean workflowByThisNameExists(String workflowName);
 
@@ -78,26 +78,27 @@ public interface XmlWorkflowFactory {
     public Workflow getDefaultWorkflow();
 
     /**
-     * Return a list of collections handles that are mapped to the given workflow in the workflow configuration.
-     * Makes use of a cache so it only retrieves the workflowName->List<collectionHandle> if it's not cached
+     * Return a list of collections that are mapped to the given workflow in the workflow configuration.
+     * * Makes use of a cache so it only retrieves the workflowName->List<collectionHandle> if it's not cached
      *
+     * @param context      Dspace context
      * @param workflowName Name of workflow we want the collections of that are mapped to is
-     * @return List of collection handles mapped to the requested workflow
+     * @return List of collections mapped to the requested workflow
      */
-    public List<String> getCollectionHandlesMappedToWorklow(String workflowName);
+    public List<Collection> getCollectionHandlesMappedToWorklow(Context context, String workflowName);
 
     /**
-     * Returns list of collection handles that are not mapped to any configured workflow, and thus use the default
-     * workflow
+     * Returns list of collections that are not mapped to any configured workflow, and thus use the default workflow
      *
-     * @return List of collection handles not mapped to any workflow
+     * @return List of collections not mapped to any workflow
      */
-    public List<String> getAllNonMappedCollectionsHandles(Context context);
+    public List<Collection> getAllNonMappedCollectionsHandles(Context context);
 
     /**
      * Retrieve a WorkflowActionConfig object based on its name, should correspond with bean id in workflow-actions.xml
-     * @param workflowActionName    Name of workflow action we want to retrieve
-     * @return  WorkflowActionConfig object corresponding to the given workflowActionName
+     *
+     * @param workflowActionName Name of workflow action we want to retrieve
+     * @return WorkflowActionConfig object corresponding to the given workflowActionName
      */
     public WorkflowActionConfig getActionByName(String workflowActionName);
 }

dspace-server-webapp/src/main/java/org/dspace/app/rest/WorkflowDefinitionController.java

@@ -7,11 +7,8 @@
  */
 package org.dspace.app.rest;
 
-import java.sql.SQLException;
-import java.util.ArrayList;
 import java.util.List;
 import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpServletResponse;
 
 import org.dspace.app.rest.converter.ConverterService;
 import org.dspace.app.rest.model.CollectionRest;
@@ -20,10 +17,7 @@ import org.dspace.app.rest.utils.ContextUtil;
 import org.dspace.app.rest.utils.Utils;
 import org.dspace.content.Collection;
 import org.dspace.core.Context;
-import org.dspace.handle.factory.HandleServiceFactory;
-import org.dspace.handle.service.HandleService;
 import org.dspace.xmlworkflow.factory.XmlWorkflowFactory;
-import org.dspace.xmlworkflow.factory.XmlWorkflowServiceFactory;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.data.domain.Page;
 import org.springframework.data.domain.Pageable;
@@ -42,8 +36,8 @@ import org.springframework.web.bind.annotation.RestController;
 @RequestMapping("/api/" + WorkflowDefinitionRest.CATEGORY + "/" + WorkflowDefinitionRest.NAME_PLURAL)
 public class WorkflowDefinitionController {
 
-    protected XmlWorkflowFactory xmlWorkflowFactory = XmlWorkflowServiceFactory.getInstance().getWorkflowFactory();
-    protected HandleService handleService = HandleServiceFactory.getInstance().getHandleService();
+    @Autowired
+    protected XmlWorkflowFactory xmlWorkflowFactory;
 
     @Autowired
     protected ConverterService converter;
@@ -57,31 +51,22 @@ public class WorkflowDefinitionController {
      * but this collection is not included in the list returned by this method.
      *
      * @param request      The request object
-     * @param response     The response object
      * @param workflowName Name of workflow we want the collections of that are mapped to is
      * @return List of collections mapped to the requested workflow
-     * @throws SQLException if db error
      */
     @GetMapping("{workflowName}/collections")
-    public Page<CollectionRest> get(HttpServletRequest request, HttpServletResponse response,
-                                    @PathVariable String workflowName, Pageable pageable) throws SQLException {
+    public Page<CollectionRest> get(HttpServletRequest request, @PathVariable String workflowName, Pageable pageable) {
         if (xmlWorkflowFactory.workflowByThisNameExists(workflowName)) {
             Context context = ContextUtil.obtainContext(request);
-            List<String> collectionsHandlesMappedToWorkflow;
+            List<Collection> collectionsMappedToWorkflow;
             if (xmlWorkflowFactory.isDefaultWorkflow(workflowName)) {
-                collectionsHandlesMappedToWorkflow = xmlWorkflowFactory.getAllNonMappedCollectionsHandles(context);
+                collectionsMappedToWorkflow = xmlWorkflowFactory.getAllNonMappedCollectionsHandles(context);
             } else {
-                collectionsHandlesMappedToWorkflow
-                        = xmlWorkflowFactory.getCollectionHandlesMappedToWorklow(workflowName);
+                collectionsMappedToWorkflow
+                        = xmlWorkflowFactory.getCollectionHandlesMappedToWorklow(context, workflowName);
             }
-            List<Collection> collectionsFromHandles = new ArrayList<>();
-            for (String handle : collectionsHandlesMappedToWorkflow) {
-                Collection collection = (Collection) handleService.resolveToObject(context, handle);
-                if (collection != null) {
-                    collectionsFromHandles.add(collection);
-                }
-            }
-            return converter.toRestPage(utils.getPage(collectionsFromHandles, pageable), utils.obtainProjection(true));
+            return converter.toRestPage(utils.getPage(collectionsMappedToWorkflow, pageable),
+                    utils.obtainProjection(true));
         } else {
             throw new ResourceNotFoundException("No workflow with name " + workflowName + " is configured");
         }

dspace-server-webapp/src/main/java/org/dspace/app/rest/model/WorkflowActionRest.java

@@ -7,11 +7,11 @@
  */
 package org.dspace.app.rest.model;
 
+import java.util.List;
+
 import com.fasterxml.jackson.annotation.JsonIgnore;
 import org.dspace.app.rest.RestResourceController;
 
-import java.util.List;
-
 /**
  * The rest resource used for workflow actions
  *

dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/WorkflowActionRestRepository.java

@@ -34,7 +34,8 @@ public class WorkflowActionRestRepository extends DSpaceRestRepository<WorkflowA
         if (actionConfig != null) {
             return converter.toRest(actionConfig, utils.obtainProjection(true));
         } else {
-            throw new ResourceNotFoundException("No workflow action with name " + workflowActionName + " is configured");
+            throw new ResourceNotFoundException("No workflow action with name " + workflowActionName
+                    + " is configured");
         }
     }
 

dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/WorkflowDefinitionRestRepository.java

@@ -19,7 +19,6 @@ import org.dspace.content.service.CollectionService;
 import org.dspace.core.Context;
 import org.dspace.xmlworkflow.WorkflowConfigurationException;
 import org.dspace.xmlworkflow.factory.XmlWorkflowFactory;
-import org.dspace.xmlworkflow.factory.XmlWorkflowServiceFactory;
 import org.dspace.xmlworkflow.state.Workflow;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.data.domain.Page;
@@ -35,7 +34,8 @@ import org.springframework.stereotype.Component;
 @Component(WorkflowDefinitionRest.CATEGORY + "." + WorkflowDefinitionRest.NAME)
 public class WorkflowDefinitionRestRepository extends DSpaceRestRepository<WorkflowDefinitionRest, String> {
 
-    protected XmlWorkflowFactory xmlWorkflowFactory = XmlWorkflowServiceFactory.getInstance().getWorkflowFactory();
+    @Autowired
+    protected XmlWorkflowFactory xmlWorkflowFactory;
 
     @Autowired
     private CollectionService collectionService;
@@ -63,7 +63,7 @@ public class WorkflowDefinitionRestRepository extends DSpaceRestRepository<Workf
      * GET endpoint that returns the workflow definition that applies to a specific collection eventually fallback
      * to the default configuration.
      *
-     * @param collectionId     Uuid of the collection
+     * @param collectionId Uuid of the collection
      * @return the workflow definition for this collection
      */
     @SearchRestMethod(name = "findByCollection")

dspace-server-webapp/src/test/java/org/dspace/app/rest/WorkflowDefinitionRestRepositoryIT.java

@@ -212,7 +212,7 @@ public class WorkflowDefinitionRestRepositoryIT extends AbstractControllerIntegr
     @Test
     public void getCollectionsOfWorkflowByName_DefaultWorkflow_AllNonMappedCollections() throws Exception {
         Workflow defaultWorkflow = xmlWorkflowFactory.getDefaultWorkflow();
-        List<String> allNonMappedCollections = xmlWorkflowFactory.getAllNonMappedCollectionsHandles(context);
+        List<Collection> allNonMappedCollections = xmlWorkflowFactory.getAllNonMappedCollectionsHandles(context);
 
         //When we call this facets endpoint
         getClient().perform(get(WORKFLOW_DEFINITIONS_ENDPOINT + "/" + defaultWorkflow.getID()
@@ -245,11 +245,10 @@ public class WorkflowDefinitionRestRepositoryIT extends AbstractControllerIntegr
         context.restoreAuthSystemState();
 
         Workflow defaultWorkflow = xmlWorkflowFactory.getDefaultWorkflow();
-        List<String> allNonMappedCollections = xmlWorkflowFactory.getAllNonMappedCollectionsHandles(context);
+        List<Collection> allNonMappedCollections = xmlWorkflowFactory.getAllNonMappedCollectionsHandles(context);
 
         if (allNonMappedCollections.size() > 0) {
-            Collection firstNonMappedCollection =
-                    (Collection) handleService.resolveToObject(context, allNonMappedCollections.get(0));
+            Collection firstNonMappedCollection = allNonMappedCollections.get(0);
 
             //When we call this facets endpoint
             getClient().perform(get(WORKFLOW_DEFINITIONS_ENDPOINT + "/" + defaultWorkflow.getID()
@@ -299,21 +298,20 @@ public class WorkflowDefinitionRestRepositoryIT extends AbstractControllerIntegr
         }
 
         if (StringUtils.isNotBlank(firstNonDefaultWorkflowName)) {
-            List<String> handlesOfMappedCollections
-                    = xmlWorkflowFactory.getCollectionHandlesMappedToWorklow(firstNonDefaultWorkflowName);
+            List<Collection> mappedCollections
+                    = xmlWorkflowFactory.getCollectionHandlesMappedToWorklow(context, firstNonDefaultWorkflowName);
             //When we call this facets endpoint
-            if (handlesOfMappedCollections.size() > 0) {
+            if (mappedCollections.size() > 0) {
                 //returns array of collection jsons that are mapped to given workflow
                 //When we call this facets endpoint
-                Collection firstMappedCollection =
-                        (Collection) handleService.resolveToObject(context, handlesOfMappedCollections.get(0));
+                Collection firstMappedCollection = mappedCollections.get(0);
                 getClient().perform(get(WORKFLOW_DEFINITIONS_ENDPOINT + "/" + firstNonDefaultWorkflowName
                         + "/collections")
                         .param("size", "1"))
                         //We expect a 200 OK status
                         .andExpect(status().isOk())
                         //Number of total workflows is equals to number of configured workflows
-                        .andExpect(jsonPath("$.totalElements", is(handlesOfMappedCollections.size())))
+                        .andExpect(jsonPath("$.totalElements", is(mappedCollections.size())))
                         //Page size is 1
                         .andExpect(jsonPath("$.size", is(1)))
                         //Page nr is 1

dspace-server-webapp/src/test/java/org/dspace/app/rest/matcher/WorkflowDefinitionMatcher.java

@@ -14,14 +14,14 @@ import static org.hamcrest.Matchers.is;
 
 import java.util.UUID;
 
-import org.hamcrest.Matcher;
-import org.hamcrest.Matchers;
-
 import org.dspace.app.rest.model.WorkflowDefinitionRest;
 import org.dspace.xmlworkflow.factory.XmlWorkflowFactory;
 import org.dspace.xmlworkflow.factory.XmlWorkflowServiceFactory;
 import org.dspace.xmlworkflow.state.Workflow;
 
+import org.hamcrest.Matcher;
+import org.hamcrest.Matchers;
+
 /**
  * @author Maria Verdonck (Atmire) on 03/01/2020
  */