Merge pull request DSpace#10185 from tdonohue/fix_flakey_tests

Fix for flakey IdentifierProvider Integration Tests

dspace-api/src/main/java/org/dspace/ctask/general/CreateMissingIdentifiers.java

@@ -10,6 +10,7 @@
 
 import java.io.IOException;
 import java.sql.SQLException;
+import java.util.List;
 
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
@@ -25,7 +26,6 @@
 import org.dspace.identifier.VersionedHandleIdentifierProviderWithCanonicalHandles;
 import org.dspace.identifier.factory.IdentifierServiceFactory;
 import org.dspace.identifier.service.IdentifierService;
-import org.dspace.services.factory.DSpaceServicesFactory;
 
 /**
  * Ensure that an object has all of the identifiers that it should, minting them
@@ -45,20 +45,6 @@ public int perform(DSpaceObject dso)
             return Curator.CURATE_SKIP;
         }
 
-        // XXX Temporary escape when an incompatible provider is configured.
-        // XXX Remove this when the provider is fixed.
-        boolean compatible = DSpaceServicesFactory
-                .getInstance()
-                .getServiceManager()
-                .getServiceByName(
-                        VersionedHandleIdentifierProviderWithCanonicalHandles.class.getCanonicalName(),
-                        IdentifierProvider.class) == null;
-        if (!compatible) {
-            setResult("This task is not compatible with VersionedHandleIdentifierProviderWithCanonicalHandles");
-            return Curator.CURATE_ERROR;
-        }
-        // XXX End of escape
-
         String typeText = Constants.typeText[dso.getType()];
 
         // Get a Context
@@ -75,6 +61,18 @@ public int perform(DSpaceObject dso)
                 .getInstance()
                 .getIdentifierService();
 
+        // XXX Temporary escape when an incompatible provider is configured.
+        // XXX Remove this when the provider is fixed.
+        List<IdentifierProvider> providerList = identifierService.getProviders();
+        boolean compatible =
+            providerList.stream().noneMatch(p -> p instanceof VersionedHandleIdentifierProviderWithCanonicalHandles);
+
+        if (!compatible) {
+            setResult("This task is not compatible with VersionedHandleIdentifierProviderWithCanonicalHandles");
+            return Curator.CURATE_ERROR;
+        }
+        // XXX End of escape
+
         // Register any missing identifiers.
         try {
             identifierService.register(context, dso);

dspace-api/src/main/java/org/dspace/identifier/IdentifierServiceImpl.java

@@ -57,6 +57,11 @@ public void setProviders(List<IdentifierProvider> providers) {
         }
     }
 
+    @Override
+    public List<IdentifierProvider> getProviders() {
+        return this.providers;
+    }
+
     /**
      * Reserves identifiers for the item
      *

dspace-api/src/main/java/org/dspace/identifier/service/IdentifierService.java

@@ -19,6 +19,7 @@
 import org.dspace.identifier.IdentifierException;
 import org.dspace.identifier.IdentifierNotFoundException;
 import org.dspace.identifier.IdentifierNotResolvableException;
+import org.dspace.identifier.IdentifierProvider;
 
 /**
  * @author Fabio Bolognesi (fabio at atmire dot com)
@@ -194,4 +195,9 @@ void register(Context context, DSpaceObject dso, String identifier)
     void delete(Context context, DSpaceObject dso, String identifier)
         throws AuthorizeException, SQLException, IdentifierException;
 
+    /**
+     * Get List of currently enabled IdentifierProviders
+     * @return List of enabled IdentifierProvider objects.
+     */
+    List<IdentifierProvider> getProviders();
 }

dspace-api/src/test/java/org/dspace/ctask/general/CreateMissingIdentifiersIT.java

@@ -10,24 +10,19 @@
 import static org.junit.Assert.assertEquals;
 
 import java.io.IOException;
-import java.util.ArrayList;
-import java.util.List;
 
-import org.dspace.AbstractIntegrationTestWithDatabase;
 import org.dspace.builder.CollectionBuilder;
 import org.dspace.builder.CommunityBuilder;
 import org.dspace.builder.ItemBuilder;
 import org.dspace.content.Collection;
 import org.dspace.content.Item;
 import org.dspace.core.factory.CoreServiceFactory;
 import org.dspace.curate.Curator;
-import org.dspace.identifier.IdentifierProvider;
-import org.dspace.identifier.IdentifierServiceImpl;
+import org.dspace.identifier.AbstractIdentifierProviderIT;
+import org.dspace.identifier.VersionedHandleIdentifierProvider;
 import org.dspace.identifier.VersionedHandleIdentifierProviderWithCanonicalHandles;
-import org.dspace.kernel.ServiceManager;
 import org.dspace.services.ConfigurationService;
 import org.dspace.services.factory.DSpaceServicesFactory;
-import org.junit.After;
 import org.junit.Test;
 
 /**
@@ -36,30 +31,19 @@
  * @author mwood
  */
 public class CreateMissingIdentifiersIT
-        extends AbstractIntegrationTestWithDatabase {
-    private ServiceManager serviceManager;
-    private IdentifierServiceImpl identifierService;
+    extends AbstractIdentifierProviderIT {
+
     private static final String P_TASK_DEF
             = "plugin.named.org.dspace.curate.CurationTask";
     private static final String TASK_NAME = "test";
 
-    @Override
-    public void setUp() throws Exception {
-        super.setUp();
-        context.turnOffAuthorisationSystem();
-
-        serviceManager = DSpaceServicesFactory.getInstance().getServiceManager();
-        identifierService = serviceManager.getServicesByType(IdentifierServiceImpl.class).get(0);
-        // Clean out providers to avoid any being used for creation of community and collection
-        identifierService.setProviders(new ArrayList<>());
-    }
+    private ConfigurationService configurationService = DSpaceServicesFactory.getInstance().getConfigurationService();
 
     @Test
     public void testPerform()
             throws IOException {
         // Must remove any cached named plugins before creating a new one
         CoreServiceFactory.getInstance().getPluginService().clearNamedPluginClasses();
-        ConfigurationService configurationService = kernelImpl.getConfigurationService();
         // Define a new task dynamically
         configurationService.setProperty(P_TASK_DEF,
                 CreateMissingIdentifiers.class.getCanonicalName() + " = " + TASK_NAME);
@@ -76,14 +60,7 @@ public void testPerform()
                                .build();
 
         /*
-         * Curate with regular test configuration -- should succeed.
-         */
-        curator.curate(context, item);
-        int status = curator.getStatus(TASK_NAME);
-        assertEquals("Curation should succeed", Curator.CURATE_SUCCESS, status);
-
-        /*
-         * Now install an incompatible provider to make the task fail.
+         * First, install an incompatible provider to make the task fail.
          */
         registerProvider(VersionedHandleIdentifierProviderWithCanonicalHandles.class);
 
@@ -92,22 +69,18 @@ public void testPerform()
                 curator.getResult(TASK_NAME));
         assertEquals("Curation should fail", Curator.CURATE_ERROR,
                 curator.getStatus(TASK_NAME));
-    }
-
-    @Override
-    @After
-    public void destroy() throws Exception {
-        super.destroy();
-        DSpaceServicesFactory.getInstance().getServiceManager().getApplicationContext().refresh();
-    }
 
-    private void registerProvider(Class type) {
-        // Register our new provider
-        serviceManager.registerServiceClass(type.getName(), type);
-        IdentifierProvider identifierProvider =
-                (IdentifierProvider) serviceManager.getServiceByName(type.getName(), type);
+        // Unregister this non-default provider
+        unregisterProvider(VersionedHandleIdentifierProviderWithCanonicalHandles.class);
+        // Re-register the default provider (for later tests which may depend on it)
+        registerProvider(VersionedHandleIdentifierProvider.class);
 
-        // Overwrite the identifier-service's providers with the new one to ensure only this provider is used
-        identifierService.setProviders(List.of(identifierProvider));
+        /*
+         * Now, verify curate with default Handle Provider works
+         * (and that our re-registration of the default provider above was successful)
+         */
+        curator.curate(context, item);
+        int status = curator.getStatus(TASK_NAME);
+        assertEquals("Curation should succeed", Curator.CURATE_SUCCESS, status);
     }
 }

dspace-api/src/test/java/org/dspace/identifier/AbstractIdentifierProviderIT.java

@@ -0,0 +1,68 @@
+/**
+ * The contents of this file are subject to the license and copyright
+ * detailed in the LICENSE and NOTICE files at the root of the source
+ * tree and available online at
+ *
+ * http://www.dspace.org/license/
+ */
+package org.dspace.identifier;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.dspace.AbstractIntegrationTestWithDatabase;
+import org.dspace.kernel.ServiceManager;
+import org.dspace.services.factory.DSpaceServicesFactory;
+
+/**
+ * AbstractIdentifierProviderIT which contains a few useful utility methods for IdentifierProvider Integration Tests
+ */
+public class AbstractIdentifierProviderIT extends AbstractIntegrationTestWithDatabase {
+
+    protected final ServiceManager serviceManager = DSpaceServicesFactory.getInstance().getServiceManager();
+    protected final IdentifierServiceImpl identifierService =
+        serviceManager.getServicesByType(IdentifierServiceImpl.class).get(0);
+
+    /**
+     * Register a specific IdentifierProvider into the current IdentifierService (replacing any existing providers).
+     * This method will also ensure the IdentifierProvider service is registered in the DSpace Service Manager.
+     * @param type IdentifierProvider Class
+     */
+    protected void registerProvider(Class type) {
+        // Register our new provider
+        IdentifierProvider identifierProvider =
+            (IdentifierProvider) DSpaceServicesFactory.getInstance().getServiceManager()
+                                                      .getServiceByName(type.getName(), type);
+        if (identifierProvider == null) {
+            DSpaceServicesFactory.getInstance().getServiceManager().registerServiceClass(type.getName(), type);
+            identifierProvider = (IdentifierProvider) DSpaceServicesFactory.getInstance().getServiceManager()
+                                                                           .getServiceByName(type.getName(), type);
+        }
+
+        identifierService.setProviders(List.of(identifierProvider));
+    }
+
+    /**
+     * Unregister a specific IdentifierProvider from the current IdentifierService (removing all existing providers).
+     * This method will also ensure the IdentifierProvider service is unregistered in the DSpace Service Manager,
+     * which ensures it does not conflict with other IdentifierProvider services.
+     * @param type IdentifierProvider Class
+     */
+    protected void unregisterProvider(Class type) {
+        // Find the provider service
+        IdentifierProvider identifierProvider =
+            (IdentifierProvider) DSpaceServicesFactory.getInstance().getServiceManager()
+                                                      .getServiceByName(type.getName(), type);
+        // If found, unregister it
+        if (identifierProvider == null) {
+            DSpaceServicesFactory.getInstance().getServiceManager().unregisterService(type.getName());
+        }
+
+        // Overwrite the identifier-service's providers with an empty list
+        identifierService.setProviders(new ArrayList<>());
+    }
+
+}
+
+
+

dspace-api/src/test/java/org/dspace/identifier/VersionedHandleIdentifierProviderIT.java

@@ -11,26 +11,18 @@
 import static org.junit.Assert.assertTrue;
 
 import java.sql.SQLException;
-import java.util.ArrayList;
-import java.util.List;
 
-import org.dspace.AbstractIntegrationTestWithDatabase;
 import org.dspace.authorize.AuthorizeException;
 import org.dspace.builder.CollectionBuilder;
 import org.dspace.builder.CommunityBuilder;
 import org.dspace.builder.ItemBuilder;
 import org.dspace.builder.VersionBuilder;
 import org.dspace.content.Collection;
 import org.dspace.content.Item;
-import org.dspace.kernel.ServiceManager;
-import org.dspace.services.factory.DSpaceServicesFactory;
-import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 
-public class VersionedHandleIdentifierProviderIT extends AbstractIntegrationTestWithDatabase {
-    private ServiceManager serviceManager;
-    private IdentifierServiceImpl identifierService;
+public class VersionedHandleIdentifierProviderIT extends AbstractIdentifierProviderIT  {
 
     private String firstHandle;
 
@@ -44,12 +36,6 @@ public class VersionedHandleIdentifierProviderIT extends AbstractIntegrationTest
     public void setUp() throws Exception {
         super.setUp();
         context.turnOffAuthorisationSystem();
-
-        serviceManager = DSpaceServicesFactory.getInstance().getServiceManager();
-        identifierService = serviceManager.getServicesByType(IdentifierServiceImpl.class).get(0);
-        // Clean out providers to avoid any being used for creation of community and collection
-        identifierService.setProviders(new ArrayList<>());
-
         parentCommunity = CommunityBuilder.createCommunity(context)
                 .withName("Parent Community")
                 .build();
@@ -58,33 +44,6 @@ public void setUp() throws Exception {
                 .build();
     }
 
-    @After
-    @Override
-    public void destroy() throws Exception {
-        super.destroy();
-        // After this test has finished running, refresh application context and
-        // set the expected 'default' versioned handle provider back to ensure other tests don't fail
-        DSpaceServicesFactory.getInstance().getServiceManager().getApplicationContext().refresh();
-    }
-
-    private void registerProvider(Class type) {
-        // Register our new provider
-        IdentifierProvider identifierProvider =
-                (IdentifierProvider) DSpaceServicesFactory.getInstance().getServiceManager()
-                        .getServiceByName(type.getName(), type);
-        if (identifierProvider == null) {
-            DSpaceServicesFactory.getInstance().getServiceManager().registerServiceClass(type.getName(), type);
-            identifierProvider = (IdentifierProvider) DSpaceServicesFactory.getInstance().getServiceManager()
-                    .getServiceByName(type.getName(), type);
-        }
-
-        // Overwrite the identifier-service's providers with the new one to ensure only this provider is used
-        identifierService = DSpaceServicesFactory.getInstance().getServiceManager()
-                .getServicesByType(IdentifierServiceImpl.class).get(0);
-        identifierService.setProviders(new ArrayList<>());
-        identifierService.setProviders(List.of(identifierProvider));
-    }
-
     private void createVersions() throws SQLException, AuthorizeException {
         itemV1 = ItemBuilder.createItem(context, collection)
                 .withTitle("First version")
@@ -96,7 +55,6 @@ private void createVersions() throws SQLException, AuthorizeException {
 
     @Test
     public void testDefaultVersionedHandleProvider() throws Exception {
-        registerProvider(VersionedHandleIdentifierProvider.class);
         createVersions();
 
         // Confirm the original item only has its original handle
@@ -125,6 +83,11 @@ public void testCanonicalVersionedHandleProvider() throws Exception {
         assertEquals(firstHandle, itemV3.getHandle());
         assertEquals(2, itemV3.getHandles().size());
         containsHandle(itemV3, firstHandle + ".3");
+
+        // Unregister this non-default provider
+        unregisterProvider(VersionedHandleIdentifierProviderWithCanonicalHandles.class);
+        // Re-register the default provider (for later tests)
+        registerProvider(VersionedHandleIdentifierProvider.class);
     }
 
     private void containsHandle(Item item, String handle) {