Show hg binary verification error messages (#1637)

Show hg verification error messages on global hg config page if trying to save invalid hg binary.

gradle/changelog/hg_verification_error_messages.yaml

@@ -0,0 +1,2 @@
+- type: added
+  description: Show hg binary verification error messages ([#1637](https://github.com/scm-manager/scm-manager/pull/1637))

scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/api/v2/resources/HgConfigResource.java

@@ -35,6 +35,7 @@ import io.swagger.v3.oas.annotations.tags.Tag;
 import sonia.scm.config.ConfigurationPermissions;
 import sonia.scm.repository.HgGlobalConfig;
 import sonia.scm.repository.HgRepositoryHandler;
+import sonia.scm.repository.HgVerifier;
 import sonia.scm.web.HgVndMediaType;
 import sonia.scm.web.VndMediaType;
 
@@ -48,6 +49,8 @@ import javax.ws.rs.Path;
 import javax.ws.rs.Produces;
 import javax.ws.rs.core.Response;
 
+import static sonia.scm.ScmConstraintViolationException.Builder.doThrow;
+
 /**
  * RESTful Web Service Resource to manage the configuration of the hg plugin.
  */
@@ -144,6 +147,7 @@ public class HgConfigResource {
     responseCode = "204",
     description = "update success"
   )
+  @ApiResponse(responseCode = "400", description = "invalid configuration")
   @ApiResponse(responseCode = "401", description = "not authenticated / invalid credentials")
   @ApiResponse(responseCode = "403", description = "not authorized, the current user does not have the \"configuration:write:hg\" privilege")
   @ApiResponse(
@@ -154,11 +158,16 @@ public class HgConfigResource {
       schema = @Schema(implementation = ErrorDto.class)
     ))
   public Response update(@Valid HgGlobalGlobalConfigDto configDto) {
-
     HgGlobalConfig config = dtoToConfigMapper.map(configDto);
-
     ConfigurationPermissions.write(config).check();
 
+    if (config.getHgBinary() != null) {
+      HgVerifier.HgVerifyStatus verifyStatus = new HgVerifier().verify(config.getHgBinary());
+      doThrow()
+        .violation(verifyStatus.getDescription())
+        .when(verifyStatus != HgVerifier.HgVerifyStatus.VALID);
+    }
+
     repositoryHandler.setConfig(config);
     repositoryHandler.storeConfig();
 

scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/HgRepositoryHandler.java

@@ -24,8 +24,6 @@
 
 package sonia.scm.repository;
 
-//~--- non-JDK imports --------------------------------------------------------
-
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
 import org.slf4j.Logger;

scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/HgVerifier.java

@@ -29,6 +29,9 @@ import org.slf4j.LoggerFactory;
 import sonia.scm.io.SimpleCommand;
 import sonia.scm.io.SimpleCommandResult;
 
+import javax.xml.bind.annotation.XmlEnum;
+import javax.xml.bind.annotation.XmlEnumValue;
+import javax.xml.bind.annotation.XmlType;
 import java.io.IOException;
 import java.nio.file.Files;
 import java.nio.file.Path;
@@ -48,23 +51,31 @@ public class HgVerifier {
   }
 
   public boolean isValid(HgGlobalConfig config) {
-    return isValid(config.getHgBinary());
+    return verify(config) == HgVerifyStatus.VALID;
   }
 
-  public boolean isValid(String hg) {
+  public boolean isValid(Path path) {
-    return isValid(Paths.get(hg));
+    return verify(path) == HgVerifyStatus.VALID;
   }
 
-  public boolean isValid(Path hg) {
+  public HgVerifyStatus verify(HgGlobalConfig config) {
+    return verify(config.getHgBinary());
+  }
+
+  public HgVerifyStatus verify(String hg) {
+    return verify(Paths.get(hg));
+  }
+
+  public HgVerifyStatus verify(Path hg) {
     LOG.trace("check if hg binary {} is valid", hg);
     if (!Files.isRegularFile(hg)) {
       LOG.warn("{} is not a regular file", hg);
-      return false;
+      return HgVerifyStatus.NOT_REGULAR_FILE;
     }
 
     if (!Files.isExecutable(hg)) {
       LOG.warn("{} is not executable", hg);
-      return false;
+      return HgVerifyStatus.NOT_EXECUTABLE;
     }
 
     try {
@@ -72,31 +83,31 @@ public class HgVerifier {
       return isVersionValid(hg, version);
     } catch (IOException ex) {
       LOG.warn("failed to resolve version of {}: ", hg, ex);
-      return false;
+      return HgVerifyStatus.COULD_NOT_RESOLVE_VERSION;
     } catch (InterruptedException ex) {
       Thread.currentThread().interrupt();
       LOG.warn("failed to resolve version of {}: ", hg, ex);
-      return false;
+      return HgVerifyStatus.COULD_NOT_RESOLVE_VERSION;
     }
   }
 
-  private boolean isVersionValid(Path hg, String version) {
+  private HgVerifyStatus isVersionValid(Path hg, String version) {
     String[] parts = version.split("\\.");
     if (parts.length < 2) {
       LOG.warn("{} returned invalid version: {}", hg, version);
-      return false;
+      return HgVerifyStatus.INVALID_VERSION;
     }
     try {
       int major = Integer.parseInt(parts[0]);
       if (major < 4) {
         LOG.warn("{} is too old, we need at least mercurial 4.x", hg);
-        return false;
+        return HgVerifyStatus.VERSION_TOO_OLD;
       }
     } catch (NumberFormatException ex) {
       LOG.warn("{} returned invalid version {}", hg, version);
-      return false;
+      return HgVerifyStatus.INVALID_VERSION;
     }
-    return true;
+    return HgVerifyStatus.VALID;
   }
 
   private VersionResolver defaultVersionResolver() {
@@ -115,4 +126,22 @@ public class HgVerifier {
     String resolveVersion(Path hg) throws IOException, InterruptedException;
   }
 
+  public enum HgVerifyStatus {
+    VALID("hg binary is valid"),
+    NOT_REGULAR_FILE("hg binary is not a regular file"),
+    NOT_EXECUTABLE("hg binary is not executable"),
+    INVALID_VERSION("hg binary returned invalid version"),
+    VERSION_TOO_OLD("hg binary version is too old, we need at least 4.x"),
+    COULD_NOT_RESOLVE_VERSION("failed to resolve version of hg binary");
+
+    private final String description;
+
+    HgVerifyStatus(String description) {
+      this.description = description;
+    }
+
+    public String getDescription() {
+      return description;
+    }
+  }
 }

scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/api/v2/resources/HgConfigResourceTest.java

@@ -155,6 +155,13 @@ public class HgConfigResourceTest {
     assertEquals(HttpServletResponse.SC_NO_CONTENT, response.getStatus());
   }
 
+  @Test
+  @SubjectAware(username = "writeOnly")
+  public void shouldNotUpdateConfigForInvalidBinary() throws URISyntaxException {
+    MockHttpResponse response = put("{\"hgBinary\":\"3.2.1\"}");
+    assertEquals(400, response.getStatus());
+  }
+
   @Test
   @SubjectAware(username = "readOnly")
   public void shouldNotUpdateConfigWhenNotAuthorized() throws URISyntaxException, UnsupportedEncodingException {
@@ -172,9 +179,13 @@ public class HgConfigResourceTest {
   }
 
   private MockHttpResponse put() throws URISyntaxException {
+    return put("{\"disabled\":true}");
+  }
+
+  private MockHttpResponse put(String config) throws URISyntaxException {
     MockHttpRequest request = MockHttpRequest.put("/" + HgConfigResource.HG_CONFIG_PATH_V2)
                                              .contentType(HgVndMediaType.CONFIG)
-                                             .content("{\"disabled\":true}".getBytes());
+                                             .content(config.getBytes());
 
     MockHttpResponse response = new MockHttpResponse();
     dispatcher.invoke(request, response);

scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/repository/HgVerifierTest.java

@@ -51,22 +51,22 @@ class HgVerifierTest {
     private final HgVerifier verifier = new HgVerifier();
 
     @Test
-    void shouldReturnFalseIfFileDoesNotExists(@TempDir Path directory) {
+    void shouldReturnNotRegularFileIfFileDoesNotExists(@TempDir Path directory) {
       Path hg = directory.resolve("hg");
 
-      boolean isValid = verify(hg);
+      HgVerifier.HgVerifyStatus verifyStatus = verify(hg);
 
-      assertThat(isValid).isFalse();
+      assertThat(verifyStatus).isEqualTo(HgVerifier.HgVerifyStatus.NOT_REGULAR_FILE);
     }
 
     @Test
-    void shouldReturnFalseIfFileIsADirectory(@TempDir Path directory) throws IOException {
+    void shouldReturnNotRegularFileIfFileIsADirectory(@TempDir Path directory) throws IOException {
       Path hg = directory.resolve("hg");
       Files.createDirectories(hg);
 
-      boolean isValid = verify(hg);
+      HgVerifier.HgVerifyStatus verifyStatus = verify(hg);
 
-      assertThat(isValid).isFalse();
+      assertThat(verifyStatus).isEqualTo(HgVerifier.HgVerifyStatus.NOT_REGULAR_FILE);
     }
 
     @Test
@@ -74,21 +74,21 @@ class HgVerifierTest {
       Path hg = directory.resolve("hg");
       Files.createFile(hg);
 
-      boolean isValid = verify(hg);
+      HgVerifier.HgVerifyStatus verifyStatus = verify(hg);
 
-      assertThat(isValid).isFalse();
+      assertThat(verifyStatus).isEqualTo(HgVerifier.HgVerifyStatus.NOT_EXECUTABLE);
     }
 
     @Test
-    void shouldReturnTrueForIfMercurialIsAvailable() {
+    void shouldReturnValidIfMercurialIsAvailable() {
       Path hg = findHg();
 
       // skip test if we could not find mercurial
       Assumptions.assumeTrue(hg != null, "skip test, because we could not find mercurial on path");
 
-      boolean isValid = verify(hg);
+      HgVerifier.HgVerifyStatus verifyStatus = verify(hg);
 
-      assertThat(isValid).isTrue();
+      assertThat(verifyStatus).isEqualTo(HgVerifier.HgVerifyStatus.VALID);
     }
 
     private Path findHg() {
@@ -106,48 +106,59 @@ class HgVerifierTest {
       return null;
     }
 
-    private boolean verify(Path hg) {
+    private HgVerifier.HgVerifyStatus verify(Path hg) {
       HgGlobalConfig config = new HgGlobalConfig();
       config.setHgBinary(hg.toString());
-      return verifier.isValid(config);
+      return verifier.verify(config);
     }
 
   }
 
   @ParameterizedTest
-  @ValueSource(strings = { "3-2-1", "x.y.z", "3.2.0" })
+  @ValueSource(strings = { "3-2-1", "x.y.z" })
-  void shouldReturnFalseForInvalidVersions(String version, @TempDir Path directory) throws IOException {
+  void shouldReturnInvalidVersions(String version, @TempDir Path directory) throws IOException {
     HgVerifier verifier = new HgVerifier(hg -> version);
 
     Path hg = createHg(directory);
 
-    boolean isValid = verifier.isValid(hg);
+    HgVerifier.HgVerifyStatus verifyStatus = verifier.verify(hg);
 
-    assertThat(isValid).isFalse();
+    assertThat(verifyStatus).isEqualTo(HgVerifier.HgVerifyStatus.INVALID_VERSION);
   }
 
   @Test
-  void shouldReturnFalseOnIOException(@TempDir Path directory) throws IOException {
+  void shouldReturnTooOldVersion(@TempDir Path directory) throws IOException {
+    HgVerifier verifier = new HgVerifier(hg -> "3.2.1");
+
+    Path hg = createHg(directory);
+
+    HgVerifier.HgVerifyStatus verifyStatus = verifier.verify(hg);
+
+    assertThat(verifyStatus).isEqualTo(HgVerifier.HgVerifyStatus.VERSION_TOO_OLD);
+  }
+
+  @Test
+  void shouldReturnCouldNotResolveOnIOException(@TempDir Path directory) throws IOException {
     HgVerifier verifier = new HgVerifier(hg -> {
       throw new IOException("failed");
     });
 
     Path hg = createHg(directory);
 
-    boolean isValid = verifier.isValid(hg);
+    HgVerifier.HgVerifyStatus verifyStatus = verifier.verify(hg);
 
-    assertThat(isValid).isFalse();
+    assertThat(verifyStatus).isEqualTo(HgVerifier.HgVerifyStatus.COULD_NOT_RESOLVE_VERSION);
   }
 
   @Test
-  void shouldReturnTrue(@TempDir Path directory) throws IOException {
+  void shouldReturnValid(@TempDir Path directory) throws IOException {
     HgVerifier verifier = new HgVerifier(hg -> "4.2.0");
 
     Path hg = createHg(directory);
 
-    boolean isValid = verifier.isValid(hg);
+    HgVerifier.HgVerifyStatus verifyStatus = verifier.verify(hg);
 
-    assertThat(isValid).isTrue();
+    assertThat(verifyStatus).isEqualTo(HgVerifier.HgVerifyStatus.VALID);
   }
 
   @Nonnull

scm-ui/ui-components/src/config/Configuration.tsx

@@ -135,7 +135,8 @@ class Configuration extends React.Component<Props, State> {
     event.preventDefault();
 
     this.setState({
-      modifying: true
+      modifying: true,
+      error: undefined
     });
 
     const { modifiedConfiguration } = this.state;
@@ -182,9 +183,7 @@ class Configuration extends React.Component<Props, State> {
     const { t } = this.props;
     const { fetching, error, configuration, modifying, valid } = this.state;
 
-    if (error) {
+    if (fetching || !configuration) {
-      return <ErrorNotification error={error} />;
-    } else if (fetching || !configuration) {
       return <Loading />;
     } else {
       const readOnly = this.isReadOnly();
@@ -200,6 +199,12 @@ class Configuration extends React.Component<Props, State> {
           {this.renderConfigChangedNotification()}
           <form onSubmit={this.modifyConfiguration}>
             {this.props.render(renderProps)}
+            {error && (
+              <>
+                <hr />
+                <ErrorNotification error={error} />
+              </>
+            )}
             <hr />
             <Level
               right={<SubmitButton label={t("config.form.submit")} disabled={!valid || readOnly} loading={modifying} />}