fix plugin id

Author: bullet-tooth

exonum-java-binding/core/src/main/java/com/exonum/binding/core/runtime/JavaArtifactNames.java

@@ -16,43 +16,17 @@
 
 package com.exonum.binding.core.runtime;
 
-import static com.google.common.base.Preconditions.checkArgument;
-
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
 final class JavaArtifactNames {
-
-  private static final String DELIMITER = ":";
-  private static final Pattern FORBIDDEN_CHARS_PATTERN = Pattern.compile("[\\s:]");
-  private static final int KEEP_EMPTY = -1;
+  private static final Pattern FORBIDDEN_CHARS_PATTERN = Pattern.compile("[\\s]");
 
   /**
-   * Checks that a Java artifact name is in format "groupId/artifactId:version".
-   *
-   * @param name a Java artifact name
-   * @return an array with two elements: artifact name and artifact version
-   * @throws IllegalArgumentException if the name format is not correct
+   * Validates artifact id for forbidden characters.
+   * @throws IllegalArgumentException if any forbidden characters found
    */
-  static String[] checkPluginArtifact(String name) {
-    String[] coordinates = name.split(DELIMITER, KEEP_EMPTY);
-    checkArgument(coordinates.length == 2,
-        "Invalid artifact name (%s), must have 'groupId/artifactId:version' format",
-        name);
-    for (String c : coordinates) {
-      checkNoForbiddenChars(c);
-    }
-    return coordinates;
-  }
-
-  /**
-   * Returns plugin artifact id for the given service artifact.
-   */
-  static String getPluginArtifactId(ServiceArtifactId artifactId) {
-    return artifactId.getName() + DELIMITER + artifactId.getVersion();
-  }
-
-  private static void checkNoForbiddenChars(String s) {
+  static void checkNoForbiddenChars(String s) {
     Matcher matcher = FORBIDDEN_CHARS_PATTERN.matcher(s);
 
     if (matcher.find()) {

exonum-java-binding/core/src/main/java/com/exonum/binding/core/runtime/Pf4jServiceLoader.java

@@ -119,7 +119,7 @@ private void startPlugin(String pluginId) throws ServiceLoadingException {
 
   /** Loads the service definition from the already loaded plugin with the given id. */
   private LoadedServiceDefinition loadDefinition(String pluginId) throws ServiceLoadingException {
-    ServiceArtifactId artifactId = extractServiceId(pluginId);
+    ServiceArtifactId artifactId = ServiceArtifactId.parseFrom(pluginId);
     Supplier<ServiceModule> serviceModuleSupplier = findServiceModuleSupplier(pluginId);
     LoadedServiceDefinition serviceDefinition =
         LoadedServiceDefinition.newInstance(artifactId, serviceModuleSupplier);
@@ -129,20 +129,6 @@ private LoadedServiceDefinition loadDefinition(String pluginId) throws ServiceLo
     return serviceDefinition;
   }
 
-  private static ServiceArtifactId extractServiceId(String pluginId)
-      throws ServiceLoadingException {
-    try {
-      String[] result = JavaArtifactNames.checkPluginArtifact(pluginId);
-      String name = result[0];
-      String version = result[1];
-      return ServiceArtifactId.newJavaId(name, version);
-    } catch (IllegalArgumentException e) {
-      String message = String.format(
-          "Invalid plugin id (%s) is specified in service artifact metadata", pluginId);
-      throw new ServiceLoadingException(message, e);
-    }
-  }
-
   private Supplier<ServiceModule> findServiceModuleSupplier(String pluginId)
       throws ServiceLoadingException {
     List<Class<? extends ServiceModule>> extensionClasses = pluginManager
@@ -192,8 +178,7 @@ public Optional<LoadedServiceDefinition> findService(ServiceArtifactId artifactI
   @Override
   public void unloadService(ServiceArtifactId artifactId) {
     checkArgument(loadedServices.containsKey(artifactId), "No such artifactId: %s", artifactId);
-
-    String pluginId = JavaArtifactNames.getPluginArtifactId(artifactId);
+    String pluginId = artifactId.toString();
     try {
       boolean stopped = pluginManager.unloadPlugin(pluginId);
       // The docs don't say why it may fail to stop the plugin.
@@ -213,7 +198,7 @@ public void unloadAll() {
     // Unload the plugins
     List<Exception> errors = new ArrayList<>();
     for (ServiceArtifactId artifactId : loadedServices.keySet()) {
-      String pluginId = JavaArtifactNames.getPluginArtifactId(artifactId);
+      String pluginId = artifactId.toString();
       try {
         unloadPlugin(pluginId);
       } catch (Exception e) {

exonum-java-binding/core/src/main/java/com/exonum/binding/core/runtime/ServiceArtifactId.java

@@ -34,7 +34,7 @@
 public abstract class ServiceArtifactId {
 
   private static final String DELIMITER = ":";
-  private static final int NUM_FIELDS = 3;
+  private static final int KEEP_EMPTY = -1;
 
   /**
    * Returns the runtime id in which the service shall be deployed.
@@ -52,15 +52,16 @@ public abstract class ServiceArtifactId {
   public abstract String getVersion();
 
   /**
-   * Parses a service id in format "runtimeId:serviceName" as {@link #toString()} produces.
+   * Parses a service id in format "runtimeId:serviceName:version" as {@link #toString()} produces.
    *
    * @param serviceArtifactId a string in format "runtimeId:serviceName:version". Whitespace
    *     characters, including preceding and trailing, are not allowed
    * @return a ServiceArtifactId with the given coordinates
    * @throws IllegalArgumentException if the format is not correct
    */
   public static ServiceArtifactId parseFrom(String serviceArtifactId) {
-    String[] coordinates = serviceArtifactId.split(DELIMITER, NUM_FIELDS);
+    JavaArtifactNames.checkNoForbiddenChars(serviceArtifactId);
+    String[] coordinates = serviceArtifactId.split(DELIMITER, KEEP_EMPTY);
     checkArgument(coordinates.length == 3,
         "Invalid artifact id (%s), must have 'runtimeId:groupId/artifactId:version' format",
         serviceArtifactId);
@@ -92,7 +93,7 @@ public static ServiceArtifactId valueOf(int runtimeId, String name, String versi
   }
 
   /**
-   * Returns an artifact id in the following format: "runtimeId:serviceName".
+   * Returns an artifact id in the following format: "runtimeId:serviceName:version".
    */
   @Override
   public final String toString() {

exonum-java-binding/core/src/test/java/com/exonum/binding/core/runtime/JavaArtifactNamesTest.java

@@ -16,34 +16,22 @@
 
 package com.exonum.binding.core.runtime;
 
+import static com.exonum.binding.core.runtime.JavaArtifactNames.checkNoForbiddenChars;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 
 import org.junit.jupiter.api.Test;
-import org.junit.jupiter.params.ParameterizedTest;
-import org.junit.jupiter.params.provider.ValueSource;
 
 class JavaArtifactNamesTest {
 
   @Test
   void checkValidName() {
     String name = "com.acme/foo:1.0";
-    JavaArtifactNames.checkPluginArtifact(name);
+    checkNoForbiddenChars(name);
   }
 
-  @ParameterizedTest
-  @ValueSource(strings = {
-      "",
-      "too-few:components:1.0",
-      "com.acme:foo-service:0.1.0:extra-component",
-      " : : ",
-      "com acme:foo:1.0",
-      "com.acme:foo service:1.0",
-      "com.acme:foo-service:1 0",
-      "com.acme:foo-service: 1.0",
-      "com.acme:foo-service:1.0 ",
-  })
-  void checkInvalidName(String pluginId) {
-    assertThrows(IllegalArgumentException.class,
-        () -> JavaArtifactNames.checkPluginArtifact(pluginId));
+  @Test
+  void checkInvalidName() {
+    String name = "com.acme foo:1.0";
+    assertThrows(IllegalArgumentException.class, () -> checkNoForbiddenChars(name));
   }
 }

exonum-java-binding/core/src/test/java/com/exonum/binding/core/runtime/Pf4jServiceLoaderIntegrationTestable.java

@@ -56,8 +56,8 @@
 
 abstract class Pf4jServiceLoaderIntegrationTestable {
 
-  private static final String PLUGIN_ID = "com.acme/foo-service:1.0.1";
-  private static final String PLUGIN_ID_2 = "org.acme/bar-service:3.2.1";
+  private static final String PLUGIN_ID = "1:com.acme/foo-service:1.0.1";
+  private static final String PLUGIN_ID_2 = "1:org.acme/bar-service:3.2.1";
   private static final Map<String, Class<?>> TEST_DEPENDENCY_REFERENCE_CLASSES = ImmutableMap.of(
       "exonum-java-binding", Service.class,
       "vertx", Vertx.class,
@@ -95,7 +95,7 @@ void canLoadService() throws ServiceLoadingException, IOException {
 
     // Check the definition
     ServiceArtifactId serviceId = serviceDefinition.getId();
-    ServiceArtifactId expectedId = getServiceArtifact(pluginId);
+    ServiceArtifactId expectedId = ServiceArtifactId.parseFrom(pluginId);
     assertThat(serviceId).isEqualTo(expectedId);
     Supplier<ServiceModule> moduleSupplier = serviceDefinition.getModuleSupplier();
     ServiceModule module = moduleSupplier.get();
@@ -123,7 +123,7 @@ void cannotLoadIfPluginManagerFailsToLoad() throws IOException {
     assertThat(e).hasMessageContaining("Failed to load the service from");
 
     // Check the definition is inaccessible
-    ServiceArtifactId serviceId = getServiceArtifact(PLUGIN_ID);
+    ServiceArtifactId serviceId = ServiceArtifactId.parseFrom(PLUGIN_ID);
     assertThat(serviceLoader.findService(serviceId)).isEmpty();
   }
 
@@ -153,7 +153,7 @@ void cannotLoadIfPluginFailedToStart() throws IOException {
     assertThat(e).hasMessageContaining("Failed to start the plugin");
 
     // Check the definition is inaccessible
-    ServiceArtifactId serviceId = getServiceArtifact(pluginId);
+    ServiceArtifactId serviceId = ServiceArtifactId.parseFrom(pluginId);
     assertThat(serviceLoader.findService(serviceId)).isEmpty();
 
     // Check it is unloaded if failed to start
@@ -163,7 +163,7 @@ void cannotLoadIfPluginFailedToStart() throws IOException {
   @ParameterizedTest
   @ValueSource(strings = {
       "foo-service",
-      "com.acme:foo-service:1.0:extra-coordinate",
+      "1:com.acme/foo-service:1.0:extra-coordinate",
   })
   void cannotLoadIfInvalidPluginIdInMetadata(String invalidPluginId) throws IOException {
     anArtifact()
@@ -173,8 +173,9 @@ void cannotLoadIfInvalidPluginIdInMetadata(String invalidPluginId) throws IOExce
     // Try to load the service
     Exception e = assertThrows(ServiceLoadingException.class,
         () -> serviceLoader.loadService(artifactLocation));
-    assertThat(e).hasMessageContaining("Invalid plugin id");
+    assertThat(e).hasMessageContaining("Failed to load plugin");
     assertThat(e).hasMessageContaining(invalidPluginId);
+    assertThat(e.getCause()).hasMessageContaining("Invalid artifact id");
 
     // Check it is unloaded if failed to start
     verify(pluginManager).unloadPlugin(invalidPluginId);
@@ -253,14 +254,14 @@ void canLoadUnloadService() throws Exception {
 
   @Test
   void unloadServiceNonLoaded() {
-    ServiceArtifactId unknownPluginId = getServiceArtifact(PLUGIN_ID);
+    ServiceArtifactId unknownPluginId = ServiceArtifactId.parseFrom(PLUGIN_ID);
     assertThrows(IllegalArgumentException.class,
         () -> serviceLoader.unloadService(unknownPluginId));
   }
 
   @Test
   void findServiceNonLoaded() {
-    ServiceArtifactId unknownPluginId = getServiceArtifact(PLUGIN_ID);
+    ServiceArtifactId unknownPluginId = ServiceArtifactId.parseFrom(PLUGIN_ID);
     Optional<?> serviceDefinition = serviceLoader.findService(unknownPluginId);
     assertThat(serviceDefinition).isEmpty();
   }
@@ -304,16 +305,11 @@ private static ServiceArtifactBuilder anArtifact() {
   private void verifyUnloaded(String pluginId) {
     verify(pluginManager).unloadPlugin(pluginId);
 
-    ServiceArtifactId serviceId = getServiceArtifact(pluginId);
+    ServiceArtifactId serviceId = ServiceArtifactId.parseFrom(pluginId);
     assertThat(serviceLoader.findService(serviceId)).isEmpty();
   }
 
   private static void assertNamesEqual(Class<?> actual, Class<?> expected) {
     assertThat(actual.getName()).isEqualTo(expected.getName());
   }
-
-  private ServiceArtifactId getServiceArtifact(String pluginId) {
-    String[] result = JavaArtifactNames.checkPluginArtifact(pluginId);
-    return ServiceArtifactId.newJavaId(result[0], result[1]);
-  }
 }

exonum-java-binding/core/src/test/java/com/exonum/binding/core/runtime/ServiceArtifactIdTest.java

@@ -17,6 +17,7 @@
 package com.exonum.binding.core.runtime;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
@@ -28,7 +29,7 @@ class ServiceArtifactIdTest {
   @ValueSource(strings = {
       "0:land-registry:v1",
       "1:com.acme/foo-service:1.1.1-beta1",
-      "100500:foo:bar:",
+      "100500:foo:bar",
   })
   void parseFromRoundtrip(String serviceId) {
     ServiceArtifactId parsedId = ServiceArtifactId.parseFrom(serviceId);
@@ -65,4 +66,21 @@ void toStringTest() {
 
     assertThat(id.toString()).isEqualTo("0:full-name:0.1");
   }
+
+  @ParameterizedTest
+  @ValueSource(strings = {
+      "",
+      "too-few:components:1.0",
+      "com.acme:foo-service:0.1.0:extra-component",
+      " : : ",
+      "com acme:foo:1.0",
+      "com.acme:foo service:1.0",
+      "com.acme:foo-service:1 0",
+      "com.acme:foo-service: 1.0",
+      "com.acme:foo-service:1.0 ",
+  })
+  void checkInvalidName(String artifactId) {
+    assertThrows(IllegalArgumentException.class,
+        () -> ServiceArtifactId.parseFrom(artifactId));
+  }
 }