fix #2213 refactoring

src/main/java/org/codelibs/fess/app/web/admin/plugin/AdminPluginAction.java

@@ -54,7 +54,7 @@ public class AdminPluginAction extends FessAdminAction {
     public HtmlResponse delete(final DeleteForm form) {
         validate(form, messages -> {}, () -> asHtml(path_AdminPlugin_AdminPluginJsp));
         verifyToken(() -> asHtml(path_AdminPlugin_AdminPluginJsp));
-        final Artifact artifact = new Artifact(form.name, form.version, null);
+        final Artifact artifact = new Artifact(form.name, form.version);
         deleteArtifact(artifact);
         saveInfo(messages -> messages.addSuccessDeletePlugin(GLOBAL, artifact.getFileName()));
         return redirect(getClass());
@@ -65,6 +65,9 @@ public class AdminPluginAction extends FessAdminAction {
         validate(form, messages -> {}, () -> asHtml(path_AdminPlugin_AdminPluginInstallpluginJsp));
         verifyToken(() -> asHtml(path_AdminPlugin_AdminPluginInstallpluginJsp));
         final Artifact artifact = getArtifactFromInstallForm(form);
+        if (artifact == null) {
+            throwValidationError(messages -> messages.addErrorsCrudCouldNotFindCrudTable(GLOBAL, form.id), () -> asListHtml());
+        }
         installArtifact(artifact);
         saveInfo(messages -> messages.addSuccessInstallPlugin(GLOBAL, artifact.getFileName()));
         return redirect(getClass());
@@ -73,9 +76,9 @@ public class AdminPluginAction extends FessAdminAction {
     @Execute
     public HtmlResponse installplugin() {
         saveToken();
-        return asHtml(path_AdminPlugin_AdminPluginInstallpluginJsp).renderWith(data -> {
-            RenderDataUtil.register(data, "availableArtifactItems", getAllAvailableArtifacts());
-        }).useForm(InstallForm.class, op -> op.setup(form -> {}));
+        return asHtml(path_AdminPlugin_AdminPluginInstallpluginJsp).renderWith(
+                data -> RenderDataUtil.register(data, "availableArtifactItems", getAllAvailableArtifacts())).useForm(InstallForm.class,
+                op -> op.setup(form -> {}));
     }
 
     private HtmlResponse asListHtml() {
@@ -84,20 +87,20 @@ public class AdminPluginAction extends FessAdminAction {
     }
 
     public static List<Map<String, String>> getAllAvailableArtifacts() {
-        final PluginHelper pluginHelper = ComponentUtil.getComponent(PluginHelper.class);
+        final PluginHelper pluginHelper = ComponentUtil.getPluginHelper();
         final List<Map<String, String>> result = new ArrayList<>();
         for (final PluginHelper.ArtifactType artifactType : PluginHelper.ArtifactType.values()) {
-            result.addAll(Arrays.stream(pluginHelper.getAvailableArtifacts(artifactType)).map(artifact -> beanToMap(artifact))
+            result.addAll(Arrays.stream(pluginHelper.getAvailableArtifacts(artifactType)).map(AdminPluginAction::beanToMap)
                     .collect(Collectors.toList()));
         }
         return result;
     }
 
     public static List<Map<String, String>> getAllInstalledArtifacts() {
-        final PluginHelper pluginHelper = ComponentUtil.getComponent(PluginHelper.class);
+        final PluginHelper pluginHelper = ComponentUtil.getPluginHelper();
         final List<Map<String, String>> result = new ArrayList<>();
         for (final PluginHelper.ArtifactType artifactType : PluginHelper.ArtifactType.values()) {
-            result.addAll(Arrays.stream(pluginHelper.getInstalledArtifacts(artifactType)).map(artifact -> beanToMap(artifact))
+            result.addAll(Arrays.stream(pluginHelper.getInstalledArtifacts(artifactType)).map(AdminPluginAction::beanToMap)
                     .collect(Collectors.toList()));
         }
         return result;
@@ -105,7 +108,8 @@ public class AdminPluginAction extends FessAdminAction {
 
     public static Map<String, String> beanToMap(final Artifact artifact) {
         final Map<String, String> item = new HashMap<>();
-        item.put("type", ArtifactType.getType(artifact).getId());
+        item.put("type", ArtifactType.getType(artifact.getName()).getId());
+        item.put("id", artifact.getName() + ":" + artifact.getVersion());
         item.put("name", artifact.getName());
         item.put("version", artifact.getVersion());
         item.put("url", artifact.getUrl());
@@ -113,14 +117,14 @@ public class AdminPluginAction extends FessAdminAction {
     }
 
     private Artifact getArtifactFromInstallForm(final InstallForm form) {
-        final String[] values = form.selectedArtifact.split("\\|");
-        return new Artifact(values[0], values[1], values[2]);
+        final String[] values = form.id.split(":");
+        return ComponentUtil.getPluginHelper().getArtifact(values[0], values[1]);
     }
 
     public static void installArtifact(final Artifact artifact) {
-        final PluginHelper pluginHelper = ComponentUtil.getComponent(PluginHelper.class);
         new Thread(() -> {
-            final Artifact[] artifacts = pluginHelper.getInstalledArtifacts(ArtifactType.getType(artifact));
+            final PluginHelper pluginHelper = ComponentUtil.getPluginHelper();
+            final Artifact[] artifacts = pluginHelper.getInstalledArtifacts(ArtifactType.getType(artifact.getName()));
             try {
                 pluginHelper.installArtifact(artifact);
             } catch (final Exception e) {
@@ -139,10 +143,9 @@ public class AdminPluginAction extends FessAdminAction {
     }
 
     public static void deleteArtifact(final Artifact artifact) {
-        final PluginHelper pluginHelper = ComponentUtil.getComponent(PluginHelper.class);
         new Thread(() -> {
             try {
-                pluginHelper.deleteInstalledArtifact(artifact);
+                ComponentUtil.getPluginHelper().deleteInstalledArtifact(artifact);
             } catch (final Exception e) {
                 logger.warn("Failed to delete " + artifact.getFileName(), e);
             }

src/main/java/org/codelibs/fess/app/web/admin/plugin/InstallForm.java

@@ -23,6 +23,6 @@ public class InstallForm {
 
     @Required
     @Size(max = 400)
-    public String selectedArtifact;
+    public String id;
 
 }

src/main/java/org/codelibs/fess/app/web/api/admin/plugin/ApiAdminPluginAction.java

@@ -19,11 +19,10 @@ import static org.codelibs.fess.app.web.admin.plugin.AdminPluginAction.getAllIns
 import static org.codelibs.fess.app.web.admin.plugin.AdminPluginAction.getAllAvailableArtifacts;
 import static org.codelibs.fess.app.web.admin.plugin.AdminPluginAction.installArtifact;
 import static org.codelibs.fess.app.web.admin.plugin.AdminPluginAction.deleteArtifact;
-import static org.codelibs.fess.helper.PluginHelper.getArtifactFromFileName;
-import static org.codelibs.fess.helper.PluginHelper.getArtifactTypeFromFileName;
 
 import org.codelibs.fess.app.web.api.ApiResult;
 import org.codelibs.fess.helper.PluginHelper.Artifact;
+import org.codelibs.fess.util.ComponentUtil;
 import org.codelibs.fess.app.web.api.admin.FessApiAdminAction;
 import org.lastaflute.web.Execute;
 import org.lastaflute.web.response.JsonResponse;
@@ -51,10 +50,9 @@ public class ApiAdminPluginAction extends FessApiAdminAction {
     @Execute
     public JsonResponse<ApiResult> put$index(final InstallBody body) {
         validateApi(body, messages -> {});
-        final Artifact artifact = new Artifact(body.name, body.version, body.url);
-        if (!isValidBody(artifact)) {
-            return asJson(new ApiResult.ApiErrorResponse().message("invalid name, version, or url").status(ApiResult.Status.BAD_REQUEST)
-                    .result());
+        final Artifact artifact = ComponentUtil.getPluginHelper().getArtifact(body.name, body.version);
+        if (artifact == null) {
+            return asJson(new ApiResult.ApiErrorResponse().message("invalid name or version").status(ApiResult.Status.BAD_REQUEST).result());
         }
         installArtifact(artifact);
         return asJson(new ApiResult.ApiResponse().status(ApiResult.Status.OK).result());
@@ -64,14 +62,8 @@ public class ApiAdminPluginAction extends FessApiAdminAction {
     @Execute
     public JsonResponse<ApiResult> delete$index(final DeleteBody body) {
         validateApi(body, messages -> {});
-        deleteArtifact(new Artifact(body.name, body.version, null));
+        deleteArtifact(new Artifact(body.name, body.version));
         return asJson(new ApiResult.ApiResponse().status(ApiResult.Status.OK).result());
     }
 
-    protected Boolean isValidBody(final Artifact artifact) {
-        final String[] tokens = artifact.getUrl().split("/");
-        final String filenameFromUrl = tokens[tokens.length - 1];
-        return getArtifactFromFileName(getArtifactTypeFromFileName(filenameFromUrl), filenameFromUrl).getFileName().equals(
-                artifact.getFileName());
-    }
 }

src/main/java/org/codelibs/fess/app/web/api/admin/plugin/DeleteBody.java

@@ -1,15 +1,9 @@
 package org.codelibs.fess.app.web.api.admin.plugin;
 
+import org.codelibs.fess.app.web.admin.plugin.DeleteForm;
 import org.lastaflute.web.validation.Required;
 
 import javax.validation.constraints.Size;
 
-public class DeleteBody {
-    @Required
-    @Size(max = 100)
-    public String name;
-
-    @Required
-    @Size(max = 100)
-    public String version;
+public class DeleteBody extends DeleteForm {
 }

src/main/java/org/codelibs/fess/app/web/api/admin/plugin/InstallBody.java

@@ -12,8 +12,4 @@ public class InstallBody {
     @Required
     @Size(max = 100)
     public String version;
-
-    @Required
-    @Size(max = 200)
-    public String url;
 }

src/main/java/org/codelibs/fess/helper/PluginHelper.java

@@ -79,7 +79,7 @@ public class PluginHelper {
 
     protected String[] getRepositories() {
         return split(ComponentUtil.getFessConfig().getPluginRepositories(), ",").get(
-                stream -> stream.map(s -> s.trim()).toArray(n -> new String[n]));
+                stream -> stream.map(String::trim).toArray(n -> new String[n]));
     }
 
     protected List<Artifact> processRepository(final ArtifactType artifactType, final String url) {
@@ -172,16 +172,12 @@ public class PluginHelper {
         return list.toArray(new Artifact[list.size()]);
     }
 
-    public static Artifact getArtifactFromFileName(final ArtifactType artifactType, final String fileName) {
+    protected Artifact getArtifactFromFileName(final ArtifactType artifactType, final String fileName) {
         final String convertedFileName = fileName.substring(artifactType.getId().length() + 1, fileName.lastIndexOf('.'));
         final int firstIndexOfDash = convertedFileName.indexOf('-');
         final String artifactName = artifactType.getId() + "-" + convertedFileName.substring(0, firstIndexOfDash);
         final String artifactVersion = convertedFileName.substring(firstIndexOfDash + 1);
-        return new Artifact(artifactName, artifactVersion, null);
-    }
-
-    public static ArtifactType getArtifactTypeFromFileName(final String filename) {
-        return ArtifactType.getType(new Artifact(filename, null, null));
+        return new Artifact(artifactName, artifactVersion);
     }
 
     public void installArtifact(final Artifact artifact) {
@@ -212,6 +208,18 @@ public class PluginHelper {
         }
     }
 
+    public Artifact getArtifact(String name, String version) {
+        if (StringUtil.isBlank(name) || StringUtil.isBlank(version)) {
+            return null;
+        }
+        for (final Artifact artifact : getAvailableArtifacts(ArtifactType.getType(name))) {
+            if (name.equals(artifact.getName()) && version.equals(artifact.getVersion())) {
+                return artifact;
+            }
+        }
+        return null;
+    }
+
     protected Path getPluginPath() {
         return Paths.get(ComponentUtil.getComponent(ServletContext.class).getRealPath("/WEB-INF/plugin"));
     }
@@ -227,6 +235,10 @@ public class PluginHelper {
             this.url = url;
         }
 
+        public Artifact(final String name, final String version) {
+            this(name, version, null);
+        }
+
         public String getName() {
             return name;
         }
@@ -262,11 +274,12 @@ public class PluginHelper {
             return id;
         }
 
-        public static ArtifactType getType(final Artifact artifact) {
-            if (artifact.getName().startsWith(DATA_STORE.getId())) {
+        public static ArtifactType getType(final String name) {
+            if (name.startsWith(DATA_STORE.getId())) {
                 return DATA_STORE;
             }
             return UNKNOWN;
         }
     }
+
 }

src/main/java/org/codelibs/fess/util/ComponentUtil.java

@@ -90,7 +90,7 @@ public final class ComponentUtil {
 
     private static Map<String, Object> componentMap = new HashMap<>();
 
-    private static final String PLUGIN_HELPER = "PluginHelper";
+    private static final String PLUGIN_HELPER = "pluginHelper";
 
     private static final String LANGUAGE_HELPER = "languageHelper";
 

src/main/webapp/WEB-INF/view/admin/plugin/admin_plugin_installplugin.jsp

@@ -39,10 +39,10 @@
                             <div class="box-body">
                                 <div class="form-group">
                                     <la:errors property="selectedArtifact" />
-                                    <la:select styleId="TODO" property="selectedArtifact" styleClass="form-control">
+                                    <la:select styleId="artifacts" property="id" styleClass="form-control">
                                         <c:forEach var="item" varStatus="s"
                                                    items="${availableArtifactItems}">
-                                            <la:option value="${f:h(item.name)}|${f:h(item.version)}|${f:h(item.url)}">${f:h(item.name)}-${f:h(item.version)}</la:option>
+                                            <la:option value="${f:h(item.id)}">${f:h(item.name)}-${f:h(item.version)}</la:option>
                                         </c:forEach>
                                     </la:select>
                                 </div>

src/test/java/org/codelibs/fess/it/admin/PluginTests.java

@@ -15,26 +15,23 @@
  */
 package org.codelibs.fess.it.admin;
 
-import static org.codelibs.fess.helper.PluginHelper.Artifact;
-
-import static org.junit.jupiter.api.Assertions.fail;
+import static org.hamcrest.Matchers.equalTo;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
 
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 
-import java.util.concurrent.CountDownLatch;
-
-import static org.hamcrest.Matchers.equalTo;
-import static org.junit.Assert.assertTrue;
-
-import io.restassured.response.Response;
+import org.codelibs.fess.helper.PluginHelper.Artifact;
 import org.codelibs.fess.it.CrudTestBase;
-import org.junit.jupiter.api.Tag;
 import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Tag;
 import org.junit.jupiter.api.Test;
 
+import io.restassured.response.Response;
+
 @Tag("it")
 public class PluginTests extends CrudTestBase {
 
@@ -141,47 +138,42 @@ public class PluginTests extends CrudTestBase {
         // Install
         {
             checkPutMethod(targetMap, getInstallEndpointSuffix()).then().body("response.status", equalTo(0));
-            final CountDownLatch latch = new CountDownLatch(1);
-            try {
-                Thread.sleep(5000);
-                latch.countDown();
-            } catch (final Exception e) {
-                try {
-                    fail(e.getMessage());
-                } finally {
-                    latch.countDown();
+
+            for (int i = 0; i < 60; i++) {
+                final List<Map<String, Object>> installed =
+                        checkGetMethod(Collections.emptyMap(), getInstalledEndpointSuffix() + "/").body().jsonPath()
+                                .get("response.plugins");
+                boolean exists =
+                        installed.stream().map(o -> getArtifactFromMap(o))
+                                .anyMatch(a -> a.getName().equals(target.getName()) && a.getVersion().equals(target.getVersion()));
+                if (!exists) {
+                    Thread.sleep(500);
+                    continue;
                 }
+                assertTrue(exists);
             }
-            latch.await();
-            final List<Map<String, Object>> installed =
-                    checkGetMethod(Collections.emptyMap(), getInstalledEndpointSuffix() + "/").body().jsonPath().get("response.plugins");
-            assertTrue(installed.stream().map(o -> getArtifactFromMap(o))
-                    .anyMatch(a -> a.getName().equals(target.getName()) && a.getVersion().equals(target.getVersion())));
         }
         // Delete
         {
             checkDeleteMethod(targetMap).then().body("response.status", equalTo(0));
-            final CountDownLatch latch = new CountDownLatch(1);
-            try {
-                Thread.sleep(5000);
-                latch.countDown();
-            } catch (final Exception e) {
-                try {
-                    fail(e.getMessage());
-                } finally {
-                    latch.countDown();
+
+            for (int i = 0; i < 60; i++) {
+                final List<Map<String, Object>> installed =
+                        checkGetMethod(Collections.emptyMap(), getInstalledEndpointSuffix() + "/").body().jsonPath()
+                                .get("response.plugins");
+                boolean exists =
+                        installed.stream().map(o -> getArtifactFromMap(o))
+                                .anyMatch(a -> a.getName().equals(target.getName()) && a.getVersion().equals(target.getVersion()));
+                if (exists) {
+                    Thread.sleep(500);
+                    continue;
                 }
+                assertFalse(exists);
             }
-            latch.await();
-            final List<Map<String, Object>> installed =
-                    checkGetMethod(Collections.emptyMap(), getInstalledEndpointSuffix() + "/").body().jsonPath().get("response.plugins");
-            assertTrue(!installed.stream().map(o -> getArtifactFromMap(o))
-                    .anyMatch(a -> a.getName().equals(target.getName()) && a.getVersion().equals(target.getVersion())));
         }
     }
 
     protected Artifact getArtifactFromMap(final Map<String, Object> item) {
-        return new Artifact(String.valueOf(item.get("name").toString()), String.valueOf(item.get("version")), String.valueOf(item
-                .get("url")));
+        return new Artifact((String) item.get("name"), (String) item.get("version"));
     }
 }