Refactor plugin manager

CHANGELOG.md

@@ -8,6 +8,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 See [Release](https://github.com/itsallcode/white-rabbit/releases/tag/v1.7.0) / [Milestone](https://github.com/itsallcode/white-rabbit/milestone/9?closed=1)
 
+### Breaking change
+
+* [#192](https://github.com/itsallcode/white-rabbit/pull/192): Simplify the `Plugin` interface by returning an empty `Optional` instead of null or throwing an exception.
+  * This requires adapting third party plugins and rebuilding plugins installed to `~/.whiterabbit/plugins/`.
+
+### Fixed
+
+* [#191](https://github.com/itsallcode/white-rabbit/pull/191): Starting WR with all plugins enabled failed with exception `IllegalStateException: Found multiple plugins supporting org.itsallcode.whiterabbit.api.features.MonthDataStorage`.
+
 ### Added
 
 * [#158](https://github.com/itsallcode/white-rabbit/issues/158): PMSmart plugin: Support optional configuration  `pmsmart.transfer.comments` to skip transfer of comments.

api/src/main/java/org/itsallcode/whiterabbit/api/AbstractPlugin.java

@@ -1,5 +1,7 @@
 package org.itsallcode.whiterabbit.api;
 
+import java.util.Optional;
+
 import org.itsallcode.whiterabbit.api.features.PluginFeature;
 
 public abstract class AbstractPlugin<S extends PluginFeature> implements Plugin
@@ -35,18 +37,18 @@ public void close()
     @Override
     public boolean supports(Class<? extends PluginFeature> featureType)
     {
-        return featureType.isAssignableFrom(featureType);
+        return this.featureType.isAssignableFrom(featureType);
     }
 
     protected abstract S createInstance();
 
     @Override
-    public <T extends PluginFeature> T getFeature(Class<T> featureType)
+    public <T extends PluginFeature> Optional<T> getFeature(Class<T> featureType)
     {
-        if (featureType.isAssignableFrom(this.featureType))
+        if (this.supports(featureType))
         {
-            return featureType.cast(createInstance());
+            return Optional.of(featureType.cast(createInstance()));
         }
-        throw new IllegalArgumentException("Feature " + featureType.getName() + " not supported by plugin " + getId());
+        return Optional.empty();
     }
 }

api/src/main/java/org/itsallcode/whiterabbit/api/Plugin.java

@@ -1,5 +1,8 @@
 package org.itsallcode.whiterabbit.api;
 
+import java.util.Optional;
+
+import org.itsallcode.whiterabbit.api.features.Holidays;
 import org.itsallcode.whiterabbit.api.features.MonthDataStorage;
 import org.itsallcode.whiterabbit.api.features.PluginFeature;
 import org.itsallcode.whiterabbit.api.features.ProjectReportExporter;
@@ -14,6 +17,7 @@
  * <ul>
  * <li>{@link ProjectReportExporter}</li>
  * <li>{@link MonthDataStorage}</li>
+ * <li>{@link Holidays}</li>
  * </ul>
  */
 public interface Plugin
@@ -51,10 +55,10 @@ public interface Plugin
      *            the feature type.
      * @param <T>
      *            the type of the feature.
-     * @return the instance of the given feature. May return <code>null</code>
-     *         or throw an exception if the feature is not supported.
+     * @return the instance of the given feature or an empty {@link Optional} if
+     *         the feature is not supported.
      */
-    <T extends PluginFeature> T getFeature(Class<T> featureType);
+    <T extends PluginFeature> Optional<T> getFeature(Class<T> featureType);
 
     /**
      * Called before closing the plugin. The plugin should cleanup any resources

api/src/test/java/org/itsallcode/whiterabbit/api/AbstractPluginTest.java

@@ -0,0 +1,81 @@
+package org.itsallcode.whiterabbit.api;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import org.itsallcode.whiterabbit.api.features.PluginFeature;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@ExtendWith(MockitoExtension.class)
+class AbstractPluginTest
+{
+    private static final String PLUGIN_ID = "testing-plugin";
+
+    @Mock
+    SupportedFeature supportedFeatureMock;
+
+    private TestingAbstractPlugin plugin;
+
+    @BeforeEach
+    void setUp()
+    {
+        plugin = new TestingAbstractPlugin();
+    }
+
+    @Test
+    void supportsReturnsTrueForSupportedFeature()
+    {
+        assertThat(plugin.supports(SupportedFeature.class)).isTrue();
+    }
+
+    @Test
+    void supportsReturnsFalseForUnsupportedFeature()
+    {
+        assertThat(plugin.supports(UnsupportedFeature.class)).isFalse();
+    }
+
+    @Test
+    void gettingUnsupportedFeatureReturnsEmptyOptional()
+    {
+        assertThat(plugin.getFeature(UnsupportedFeature.class)).isEmpty();
+    }
+
+    @Test
+    void gettingSupportedFeatureWorks()
+    {
+        assertThat(plugin.getFeature(SupportedFeature.class)).isPresent().containsSame(supportedFeatureMock);
+    }
+
+    @Test
+    void getPluginId()
+    {
+        assertThat(plugin.getId()).isEqualTo(PLUGIN_ID);
+    }
+
+    private interface SupportedFeature extends PluginFeature
+    {
+
+    }
+
+    private interface UnsupportedFeature extends PluginFeature
+    {
+
+    }
+
+    private class TestingAbstractPlugin extends AbstractPlugin<SupportedFeature>
+    {
+        protected TestingAbstractPlugin()
+        {
+            super(PLUGIN_ID, SupportedFeature.class);
+        }
+
+        @Override
+        protected SupportedFeature createInstance()
+        {
+            return supportedFeatureMock;
+        }
+    }
+}

jfxui/src/main/java/org/itsallcode/whiterabbit/jfxui/table/days/DayRecordPropertyAdapter.java

@@ -88,7 +88,7 @@ public void setTableRow(TableRow<DayRecordPropertyAdapter> newTableRow)
         this.tableRow = newTableRow;
         if (date.get() != null && tableRow.getIndex() + 1 != date.get().getDayOfMonth())
         {
-            LOG.warn("Trying to set invalid row #{} for date {}.", tableRow.getIndex(), date.get());
+            LOG.trace("Trying to set invalid row #{} for date {}.", tableRow.getIndex(), date.get());
             return;
         }
         LOG.trace("Setting table row #{} for day {}", newTableRow.getIndex(), date.get());

jfxui/src/main/java/org/itsallcode/whiterabbit/jfxui/ui/ProjectReportViewer.java

@@ -8,6 +8,7 @@
 
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
+import org.itsallcode.whiterabbit.api.features.ProjectReportExporter;
 import org.itsallcode.whiterabbit.api.model.DayType;
 import org.itsallcode.whiterabbit.api.model.ProjectReport;
 import org.itsallcode.whiterabbit.api.model.ProjectReportActivity;
@@ -21,9 +22,13 @@
 import org.itsallcode.whiterabbit.jfxui.ui.widget.ReportWindow;
 import org.itsallcode.whiterabbit.jfxui.uistate.UiStateService;
 import org.itsallcode.whiterabbit.logic.service.AppService;
+import org.itsallcode.whiterabbit.logic.service.plugin.AppPlugin;
 import org.itsallcode.whiterabbit.logic.service.project.ProjectImpl;
 
+import javafx.event.ActionEvent;
+import javafx.event.EventHandler;
 import javafx.scene.Node;
+import javafx.scene.control.Button;
 import javafx.scene.control.TreeItem;
 import javafx.scene.control.TreeTableView;
 import javafx.stage.Stage;
@@ -63,15 +68,23 @@ public void show()
 
     private Node[] getExportButtons()
     {
-        return appService.pluginManager().getProjectReportExporterPlugins().stream()
-                .map(pluginId -> UiWidget.button(pluginId + "-export-button", "Export to " + pluginId,
-                        e -> exportReport(pluginId)))
+        return appService.pluginManager().findPluginsSupporting(ProjectReportExporter.class).stream()
+                .map(this::createExportButton)
                 .toArray(Node[]::new);
     }
 
-    private void exportReport(String pluginId)
+    private Button createExportButton(AppPlugin plugin)
     {
-        final var projectReportExporter = appService.pluginManager().getProjectReportExporter(pluginId);
+        final String pluginId = plugin.getId();
+        final EventHandler<ActionEvent> action = e -> exportReport(plugin);
+        return UiWidget.button(pluginId + "-export-button", "Export to " + pluginId, action);
+    }
+
+    private void exportReport(AppPlugin plugin)
+    {
+        final var projectReportExporter = plugin.getFeature(ProjectReportExporter.class)
+                .orElseThrow(() -> new IllegalStateException(
+                        "Plugin " + plugin + " does not support " + ProjectReportExporter.class));
         final DialogProgressMonitor progressMonitor = ProgressDialog.show(primaryStage, "Exporting project report...");
         appService.scheduler().schedule(Duration.ZERO, () -> {
             projectReportExporter.export(report, progressMonitor);

jfxui/src/uiTest/java/org/itsallcode/whiterabbit/jfxui/testutil/UiTestDummyPlugin.java

@@ -2,6 +2,8 @@
 
 import static org.mockito.Mockito.mock;
 
+import java.util.Optional;
+
 import org.itsallcode.whiterabbit.api.Plugin;
 import org.itsallcode.whiterabbit.api.PluginConfiguration;
 import org.itsallcode.whiterabbit.api.features.PluginFeature;
@@ -29,14 +31,13 @@ public boolean supports(Class<? extends PluginFeature> featureType)
     }
 
     @Override
-    public <T extends PluginFeature> T getFeature(Class<T> featureType)
+    public <T extends PluginFeature> Optional<T> getFeature(Class<T> featureType)
     {
-        return mock(featureType);
+        return Optional.of(mock(featureType));
     }
 
     @Override
     public void close()
     {
-
     }
 }

logic/src/main/java/org/itsallcode/whiterabbit/logic/service/AppService.java

@@ -122,7 +122,7 @@ public static AppService create(final Config config, Clock clock, ScheduledExecu
 
     private static MonthDataStorage createMonthDataStorage(final Config config, final PluginManager pluginManager)
     {
-        final Optional<MonthDataStorage> dataStorage = pluginManager.getMonthDataStorage();
+        final Optional<MonthDataStorage> dataStorage = pluginManager.getUniqueFeature(MonthDataStorage.class);
         if (dataStorage.isPresent())
         {
             LOG.info("Using storage plugin {}", dataStorage.get().getClass().getName());

logic/src/main/java/org/itsallcode/whiterabbit/logic/service/plugin/AppPlugin.java

@@ -1,6 +1,7 @@
 package org.itsallcode.whiterabbit.logic.service.plugin;
 
 import java.util.Collection;
+import java.util.Optional;
 
 import org.itsallcode.whiterabbit.api.features.Holidays;
 import org.itsallcode.whiterabbit.api.features.MonthDataStorage;
@@ -13,6 +14,8 @@ public interface AppPlugin
 
     Collection<AppPluginFeature> getFeatures();
 
+    <T extends PluginFeature> Optional<T> getFeature(Class<T> featureType);
+
     AppPluginOrigin getOrigin();
 
     public enum AppPluginFeature

logic/src/main/java/org/itsallcode/whiterabbit/logic/service/plugin/PluginWrapper.java → logic/src/main/java/org/itsallcode/whiterabbit/logic/service/plugin/AppPluginImpl.java

@@ -14,24 +14,24 @@
 import org.itsallcode.whiterabbit.api.features.PluginFeature;
 import org.itsallcode.whiterabbit.logic.Config;
 
-class PluginWrapper implements AppPlugin
+class AppPluginImpl implements AppPlugin
 {
-    private static final Logger LOG = LogManager.getLogger(PluginWrapper.class);
+    private static final Logger LOG = LogManager.getLogger(AppPluginImpl.class);
 
     private final AppPluginOrigin origin;
     private final Plugin plugin;
     private final Config config;
 
-    private PluginWrapper(Config config, AppPluginOrigin origin, Plugin plugin)
+    private AppPluginImpl(Config config, AppPluginOrigin origin, Plugin plugin)
     {
         this.config = config;
         this.origin = origin;
         this.plugin = plugin;
     }
 
-    public static PluginWrapper create(Config config, AppPluginOrigin origin, Plugin plugin)
+    public static AppPluginImpl create(Config config, AppPluginOrigin origin, Plugin plugin)
     {
-        return new PluginWrapper(config, origin, plugin);
+        return new AppPluginImpl(config, origin, plugin);
     }
 
     void init()
@@ -72,7 +72,7 @@ public boolean supports(Class<? extends PluginFeature> featureType)
         }
     }
 
-    <T extends PluginFeature> T getFeature(Class<T> featureType)
+    public <T extends PluginFeature> Optional<T> getFeature(Class<T> featureType)
     {
         return plugin.getFeature(featureType);
     }

logic/src/main/java/org/itsallcode/whiterabbit/logic/service/plugin/PluginManager.java

@@ -8,9 +8,7 @@
 
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
-import org.itsallcode.whiterabbit.api.features.MonthDataStorage;
 import org.itsallcode.whiterabbit.api.features.PluginFeature;
-import org.itsallcode.whiterabbit.api.features.ProjectReportExporter;
 import org.itsallcode.whiterabbit.logic.Config;
 
 public class PluginManager
@@ -34,20 +32,16 @@ public static PluginManager create(Config config)
     public <T extends PluginFeature> List<T> getAllFeatures(Class<T> featureType)
     {
         return findPluginsSupporting(featureType).stream()
-                .map(pluginId -> getFeature(pluginId, featureType))
+                .map(plugin -> plugin.getFeature(featureType))
+                .filter(Optional::isPresent)
+                .map(Optional::get)
                 .collect(toList());
     }
 
-    public List<String> getProjectReportExporterPlugins()
-    {
-        return findPluginsSupporting(ProjectReportExporter.class);
-    }
-
-    private List<String> findPluginsSupporting(Class<? extends PluginFeature> featureType)
+    public List<AppPlugin> findPluginsSupporting(Class<? extends PluginFeature> featureType)
     {
         return pluginRegistry.getAllPlugins().stream()
                 .filter(plugin -> plugin.supports(featureType))
-                .map(PluginWrapper::getId)
                 .collect(toList());
     }
 
@@ -57,43 +51,19 @@ public Collection<? extends AppPlugin> getAllPlugins()
         return pluginRegistry.getAllPlugins();
     }
 
-    public ProjectReportExporter getProjectReportExporter(String id)
-    {
-        return getFeature(id, ProjectReportExporter.class);
-    }
-
-    public Optional<MonthDataStorage> getMonthDataStorage()
-    {
-        return getUniqueFeature(MonthDataStorage.class);
-    }
-
-    private Optional<MonthDataStorage> getUniqueFeature(Class<MonthDataStorage> featureType)
+    public <T extends PluginFeature> Optional<T> getUniqueFeature(Class<T> featureType)
     {
-        final List<String> pluginIds = findPluginsSupporting(featureType);
-        if (pluginIds.isEmpty())
+        final List<AppPlugin> plugins = findPluginsSupporting(featureType);
+        if (plugins.isEmpty())
         {
             return Optional.empty();
         }
-        if (pluginIds.size() > 1)
+        if (plugins.size() > 1)
         {
             throw new IllegalStateException("Found multiple plugins supporting " + featureType.getName()
-                    + ": " + pluginIds + ". Please add only one storage plugin to the classpath.");
-        }
-        return Optional.of(getFeature(pluginIds.get(0), featureType));
-    }
-
-    private <T extends PluginFeature> T getFeature(String id, final Class<T> featureType)
-    {
-        final PluginWrapper plugin = pluginRegistry.getPlugin(id);
-        if (plugin == null)
-        {
-            throw new IllegalStateException("Plugin '" + id + "' not found");
-        }
-        if (!plugin.supports(featureType))
-        {
-            throw new IllegalStateException("Plugin '" + id + "' does not support feature " + featureType.getName());
+                    + ": " + plugins + ". Please add only one storage plugin to the classpath.");
         }
-        return plugin.getFeature(featureType);
+        return plugins.get(0).getFeature(featureType);
     }
 
     public void close()