Use a Set for serverPlugins instead of a de-duplicating adder

Addresses review feedback on #397. Rather than routing the eight
registrations through a helper that scans the List, serverPlugins is now
a Set so the de-duplication is the natural behaviour of the collection
and the registration calls stay as they were.

LinkedHashSet rather than HashSet: initPlugins loads plugins in a
deliberate order (descending plugin weight), and that order is preserved
when starting and stopping them.

getServerPlugins still returns a List so the ExtensionController
signature is unchanged for extensions.

The unit tests added in the previous commit are removed. They exercised
the helper directly, and with a Set the invariant is enforced by the
collection type rather than by logic of our own.

Signed-off-by: Giovanni Giannola <giogiannola@globalesm.com>

Author: ggiannola

server/src/main/java/com/mirth/connect/server/controllers/DefaultExtensionController.java

@@ -84,7 +84,14 @@ public class DefaultExtensionController extends ExtensionController {
 
     // these are plugins for specific extension points, keyed by plugin name
     // (not path)
-    private List<ServerPlugin> serverPlugins = new ArrayList<ServerPlugin>();
+    /*
+     * A plugin class may implement several of the plugin type interfaces, in which case it is
+     * registered once for each interface it implements. A Set holds a single entry per plugin,
+     * so start() and stop() are invoked once per plugin rather than once per interface.
+     * LinkedHashSet because initPlugins loads plugins in a deliberate order (by plugin weight)
+     * and that order is preserved when they are started and stopped.
+     */
+    private Set<ServerPlugin> serverPlugins = new LinkedHashSet<ServerPlugin>();
     private Map<String, ServicePlugin> servicePlugins = new LinkedHashMap<String, ServicePlugin>();
     private Map<String, ChannelPlugin> channelPlugins = new LinkedHashMap<String, ChannelPlugin>();
     private Map<String, CodeTemplateServerPlugin> codeTemplateServerPlugins = new LinkedHashMap<String, CodeTemplateServerPlugin>();
@@ -240,42 +247,42 @@ public void initPlugins() {
                          */
                         servicePlugin.init(currentProperties);
                         servicePlugins.put(servicePlugin.getPluginPointName(), servicePlugin);
-                        addServerPlugin(servicePlugin);
+                        serverPlugins.add(servicePlugin);
                         logger.debug("sucessfully loaded server plugin: " + serverPlugin.getPluginPointName());
                     }
 
                     if (serverPlugin instanceof ChannelPlugin) {
                         ChannelPlugin channelPlugin = (ChannelPlugin) serverPlugin;
                         channelPlugins.put(channelPlugin.getPluginPointName(), channelPlugin);
-                        addServerPlugin(channelPlugin);
+                        serverPlugins.add(channelPlugin);
                         logger.debug("sucessfully loaded server channel plugin: " + serverPlugin.getPluginPointName());
                     }
 
                     if (serverPlugin instanceof CodeTemplateServerPlugin) {
                         CodeTemplateServerPlugin codeTemplateServerPlugin = (CodeTemplateServerPlugin) serverPlugin;
                         codeTemplateServerPlugins.put(codeTemplateServerPlugin.getPluginPointName(), codeTemplateServerPlugin);
-                        addServerPlugin(codeTemplateServerPlugin);
+                        serverPlugins.add(codeTemplateServerPlugin);
                         logger.debug("sucessfully loaded server code template plugin: " + serverPlugin.getPluginPointName());
                     }
 
                     if (serverPlugin instanceof DataTypeServerPlugin) {
                         DataTypeServerPlugin dataTypePlugin = (DataTypeServerPlugin) serverPlugin;
                         dataTypePlugins.put(dataTypePlugin.getPluginPointName(), dataTypePlugin);
-                        addServerPlugin(dataTypePlugin);
+                        serverPlugins.add(dataTypePlugin);
                         logger.debug("sucessfully loaded server data type plugin: " + serverPlugin.getPluginPointName());
                     }
 
                     if (serverPlugin instanceof ResourcePlugin) {
                         ResourcePlugin resourcePlugin = (ResourcePlugin) serverPlugin;
                         resourcePlugins.put(resourcePlugin.getPluginPointName(), resourcePlugin);
-                        addServerPlugin(resourcePlugin);
+                        serverPlugins.add(resourcePlugin);
                         logger.debug("Successfully loaded resource plugin: " + resourcePlugin.getPluginPointName());
                     }
 
                     if (serverPlugin instanceof TransmissionModeProvider) {
                         TransmissionModeProvider transmissionModeProvider = (TransmissionModeProvider) serverPlugin;
                         transmissionModeProviders.put(transmissionModeProvider.getPluginPointName(), transmissionModeProvider);
-                        addServerPlugin(transmissionModeProvider);
+                        serverPlugins.add(transmissionModeProvider);
                         logger.debug("Successfully loaded transmission mode provider plugin: " + transmissionModeProvider.getPluginPointName());
                     }
 
@@ -287,7 +294,7 @@ public void initPlugins() {
                         }
 
                         this.authorizationPlugin = authorizationPlugin;
-                        addServerPlugin(authorizationPlugin);
+                        serverPlugins.add(authorizationPlugin);
                         logger.debug("sucessfully loaded server authorization plugin: " + serverPlugin.getPluginPointName());
                     }
 
@@ -299,7 +306,7 @@ public void initPlugins() {
                         }
 
                         this.multiFactorAuthenticationPlugin = multiFactorAuthenticationPlugin;
-                        addServerPlugin(multiFactorAuthenticationPlugin);
+                        serverPlugins.add(multiFactorAuthenticationPlugin);
                         logger.debug("sucessfully loaded server multi-factor authentication plugin: " + serverPlugin.getPluginPointName());
                     }
                 } catch (Exception e) {
@@ -309,32 +316,6 @@ public void initPlugins() {
         }
     }
 
-    /**
-     * Registers a plugin in the list used to start and stop all server plugins.
-     * <p>
-     * A single plugin class may implement more than one of the plugin type interfaces (for example
-     * both {@link ServicePlugin} and {@link ChannelPlugin}). Such a plugin is registered against
-     * each type it implements, so this method guards against adding the same instance more than
-     * once. Without the guard, {@link #startPlugins()} and {@link #stopPlugins()} would invoke
-     * {@link ServerPlugin#start()} and {@link ServerPlugin#stop()} once per implemented interface
-     * rather than once per plugin.
-     * <p>
-     * Instances are compared by identity on purpose. Two distinct plugin instances must both be
-     * registered even if the plugin class considers them equal.
-     * <p>
-     * Package private so it can be exercised directly by unit tests without going through the full
-     * extension loading process.
-     */
-    void addServerPlugin(ServerPlugin serverPlugin) {
-        for (ServerPlugin registeredPlugin : serverPlugins) {
-            if (registeredPlugin == serverPlugin) {
-                return;
-            }
-        }
-
-        serverPlugins.add(serverPlugin);
-    }
-
     /* These are the maps for the different types of plugins */
     /* ********************************************************************** */
 
@@ -742,7 +723,8 @@ public List<String> getClientLibraries() {
     }
 
     public List<ServerPlugin> getServerPlugins() {
-        return serverPlugins;
+        // Copied into a List so the ExtensionController signature stays unchanged for extensions.
+        return new ArrayList<ServerPlugin>(serverPlugins);
     }
 
     void extractZipEntry(ZipEntry entry, File installTempDir, ZipFile zipFile) throws IOException {

server/src/test/java/com/mirth/connect/server/controllers/DefaultExtensionControllerTest.java

@@ -9,12 +9,9 @@
 
 package com.mirth.connect.server.controllers;
 
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertTrue;
 
 import java.io.File;
-import java.util.List;
 import java.util.zip.ZipEntry;
 import java.util.zip.ZipException;
 import java.util.zip.ZipFile;
@@ -23,7 +20,6 @@
 import org.junit.Before;
 import org.junit.Test;
 
-import com.mirth.connect.plugins.ServerPlugin;
 import com.mirth.connect.util.ZipTestUtils;
 
 public class DefaultExtensionControllerTest {
@@ -76,98 +72,4 @@ public void cleanupTestFolder() {
     private ZipFile createTempZipFile(String fileName) throws Exception {
         return new ZipFile(ZipTestUtils.createTempZipFile(fileName));
     }
-
-    /*
-     * A plugin class may implement several of the plugin type interfaces (ServicePlugin,
-     * ChannelPlugin, and so on). initPlugins registers the instance once per interface it
-     * implements, so registration must ignore an instance that is already registered. Otherwise
-     * start() and stop() get invoked once per implemented interface instead of once per plugin.
-     */
-    @Test
-    public void testAddServerPluginIgnoresDuplicateRegistrationOfSameInstance() {
-        DefaultExtensionController extensionController = new DefaultExtensionController();
-        CountingServerPlugin plugin = new CountingServerPlugin("multi-type plugin");
-
-        extensionController.addServerPlugin(plugin);
-        extensionController.addServerPlugin(plugin);
-        extensionController.addServerPlugin(plugin);
-
-        List<ServerPlugin> registeredPlugins = extensionController.getServerPlugins();
-        assertEquals(1, registeredPlugins.size());
-        assertSame(plugin, registeredPlugins.get(0));
-    }
-
-    @Test
-    public void testStopPluginsStopsPluginRegisteredForMultipleTypesOnce() {
-        DefaultExtensionController extensionController = new DefaultExtensionController();
-        CountingServerPlugin plugin = new CountingServerPlugin("multi-type plugin");
-
-        extensionController.addServerPlugin(plugin);
-        extensionController.addServerPlugin(plugin);
-
-        extensionController.stopPlugins();
-
-        assertEquals(1, plugin.stopCount);
-    }
-
-    /*
-     * Registration deliberately compares instances by identity, so two separate plugin instances
-     * are both registered and both stopped even when the plugin class reports them as equal.
-     */
-    @Test
-    public void testAddServerPluginRegistersDistinctButEqualInstancesSeparately() {
-        DefaultExtensionController extensionController = new DefaultExtensionController();
-        AlwaysEqualServerPlugin firstPlugin = new AlwaysEqualServerPlugin();
-        AlwaysEqualServerPlugin secondPlugin = new AlwaysEqualServerPlugin();
-
-        extensionController.addServerPlugin(firstPlugin);
-        extensionController.addServerPlugin(secondPlugin);
-
-        assertEquals(2, extensionController.getServerPlugins().size());
-
-        extensionController.stopPlugins();
-
-        assertEquals(1, firstPlugin.stopCount);
-        assertEquals(1, secondPlugin.stopCount);
-    }
-
-    private static class CountingServerPlugin implements ServerPlugin {
-        private final String pluginPointName;
-        int stopCount;
-
-        private CountingServerPlugin(String pluginPointName) {
-            this.pluginPointName = pluginPointName;
-        }
-
-        @Override
-        public String getPluginPointName() {
-            return pluginPointName;
-        }
-
-        @Override
-        public void start() {
-            // Not exercised here: startPlugins() also reaches into ControllerFactory.
-        }
-
-        @Override
-        public void stop() {
-            stopCount++;
-        }
-    }
-
-    private static class AlwaysEqualServerPlugin extends CountingServerPlugin {
-        private AlwaysEqualServerPlugin() {
-            super("always equal plugin");
-        }
-
-        @Override
-        public boolean equals(Object other) {
-            return other instanceof AlwaysEqualServerPlugin;
-        }
-
-        @Override
-        public int hashCode() {
-            return AlwaysEqualServerPlugin.class.hashCode();
-        }
-    }
 }