[IT] Related fixes

impl/maven-executor/src/main/java/org/apache/maven/cling/executor/ExecutorHelper.java

@@ -26,7 +26,7 @@
 /**
  * Helper class for routing Maven execution based on preferences and/or issued execution requests.
  */
-public interface ExecutorHelper extends ExecutorTool {
+public interface ExecutorHelper {
     /**
      * The modes of execution.
      */

impl/maven-executor/src/main/java/org/apache/maven/cling/executor/internal/HelperImpl.java

@@ -21,15 +21,13 @@
 import java.nio.file.Path;
 import java.util.Collections;
 import java.util.HashMap;
-import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
 
 import org.apache.maven.api.annotations.Nullable;
 import org.apache.maven.api.cli.Executor;
 import org.apache.maven.api.cli.ExecutorException;
 import org.apache.maven.api.cli.ExecutorRequest;
 import org.apache.maven.cling.executor.ExecutorHelper;
-import org.apache.maven.cling.executor.ExecutorTool;
 
 import static java.util.Objects.requireNonNull;
 
@@ -40,7 +38,6 @@ public class HelperImpl implements ExecutorHelper {
     private final Mode defaultMode;
     private final Path installationDirectory;
     private final Path userHomeDirectory;
-    private final ExecutorTool executorTool;
     private final HashMap<Mode, Executor> executors;
 
     private final ConcurrentHashMap<String, String> cache;
@@ -58,7 +55,6 @@ public HelperImpl(
         this.userHomeDirectory = userHomeDirectory != null
                 ? ExecutorRequest.getCanonicalPath(userHomeDirectory)
                 : ExecutorRequest.discoverUserHomeDirectory();
-        this.executorTool = new ToolboxTool(this);
         this.executors = new HashMap<>();
 
         this.executors.put(Mode.EMBEDDED, requireNonNull(embedded, "embedded"));
@@ -89,28 +85,6 @@ public String mavenVersion() {
         });
     }
 
-    @Override
-    public Map<String, String> dump(ExecutorRequest.Builder request) throws ExecutorException {
-        return executorTool.dump(request);
-    }
-
-    @Override
-    public String localRepository(ExecutorRequest.Builder request) throws ExecutorException {
-        return executorTool.localRepository(request);
-    }
-
-    @Override
-    public String artifactPath(ExecutorRequest.Builder request, String gav, String repositoryId)
-            throws ExecutorException {
-        return executorTool.artifactPath(request, gav, repositoryId);
-    }
-
-    @Override
-    public String metadataPath(ExecutorRequest.Builder request, String gav, String repositoryId)
-            throws ExecutorException {
-        return executorTool.metadataPath(request, gav, repositoryId);
-    }
-
     protected Executor getExecutor(Mode mode, ExecutorRequest request) throws ExecutorException {
         return switch (mode) {
             case AUTO -> getExecutorByRequest(request);

impl/maven-executor/src/main/java/org/apache/maven/cling/executor/internal/ToolboxTool.java

@@ -59,7 +59,8 @@ public Map<String, String> dump(ExecutorRequest.Builder executorRequest) throws
         doExecute(builder);
         try {
             Properties properties = new Properties();
-            properties.load(new ByteArrayInputStream(stdout.toByteArray()));
+            properties.load(new ByteArrayInputStream(
+                    validateOutput(false, stdout, stderr).getBytes()));
             return properties.entrySet().stream()
                     .collect(Collectors.toMap(
                             e -> String.valueOf(e.getKey()),
@@ -79,7 +80,7 @@ public String localRepository(ExecutorRequest.Builder executorRequest) throws Ex
                 .stdOut(stdout)
                 .stdErr(stderr);
         doExecute(builder);
-        return shaveStdout(stdout);
+        return validateOutput(true, stdout, stderr);
     }
 
     @Override
@@ -95,7 +96,7 @@ public String artifactPath(ExecutorRequest.Builder executorRequest, String gav,
             builder.argument("-Drepository=" + repositoryId + "::unimportant");
         }
         doExecute(builder);
-        return shaveStdout(stdout);
+        return validateOutput(true, stdout, stderr);
     }
 
     @Override
@@ -111,7 +112,7 @@ public String metadataPath(ExecutorRequest.Builder executorRequest, String gav,
             builder.argument("-Drepository=" + repositoryId + "::unimportant");
         }
         doExecute(builder);
-        return shaveStdout(stdout);
+        return validateOutput(true, stdout, stderr);
     }
 
     private ExecutorRequest.Builder mojo(ExecutorRequest.Builder builder, String mojo) {
@@ -131,7 +132,19 @@ private void doExecute(ExecutorRequest.Builder builder) {
         }
     }
 
-    private String shaveStdout(ByteArrayOutputStream stdout) {
-        return stdout.toString().replace("\n", "").replace("\r", "");
+    /**
+     * Performs "sanity check" for output, making sure no insane values like empty strings are returned.
+     */
+    private String validateOutput(boolean shave, ByteArrayOutputStream stdout, ByteArrayOutputStream stderr) {
+        String result = stdout.toString();
+        if (shave) {
+            result = result.replace("\n", "").replace("\r", "");
+        }
+        // sanity checks: stderr has any OR result is empty string (no method should emit empty string)
+        if (stderr.size() > 0 || result.trim().isEmpty()) {
+            throw new ExecutorException(
+                    "Unexpected stdout[" + stdout.size() + "]=" + stdout + "; stderr[" + stderr.size() + "]=" + stderr);
+        }
+        return result;
     }
 }

impl/maven-executor/src/test/java/org/apache/maven/cling/executor/impl/HelperImplTest.java → impl/maven-executor/src/test/java/org/apache/maven/cling/executor/impl/ToolboxToolTest.java

@@ -27,6 +27,7 @@
 import org.apache.maven.cling.executor.ExecutorHelper;
 import org.apache.maven.cling.executor.MavenExecutorTestSupport;
 import org.apache.maven.cling.executor.internal.HelperImpl;
+import org.apache.maven.cling.executor.internal.ToolboxTool;
 import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Timeout;
 import org.junit.jupiter.api.io.TempDir;
@@ -38,7 +39,7 @@
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
-public class HelperImplTest {
+public class ToolboxToolTest {
     @TempDir
     private static Path userHome;
 
@@ -52,7 +53,7 @@ void dump3(ExecutorHelper.Mode mode) throws Exception {
                 userHome,
                 MavenExecutorTestSupport.EMBEDDED_MAVEN_EXECUTOR,
                 MavenExecutorTestSupport.FORKED_MAVEN_EXECUTOR);
-        Map<String, String> dump = helper.dump(helper.executorRequest());
+        Map<String, String> dump = new ToolboxTool(helper).dump(helper.executorRequest());
         assertEquals(System.getProperty("maven3version"), dump.get("maven.version"));
     }
 
@@ -66,7 +67,7 @@ void dump4(ExecutorHelper.Mode mode) throws Exception {
                 userHome,
                 MavenExecutorTestSupport.EMBEDDED_MAVEN_EXECUTOR,
                 MavenExecutorTestSupport.FORKED_MAVEN_EXECUTOR);
-        Map<String, String> dump = helper.dump(helper.executorRequest());
+        Map<String, String> dump = new ToolboxTool(helper).dump(helper.executorRequest());
         assertEquals(System.getProperty("maven4version"), dump.get("maven.version"));
     }
 
@@ -106,7 +107,7 @@ void localRepository3(ExecutorHelper.Mode mode) {
                 userHome,
                 MavenExecutorTestSupport.EMBEDDED_MAVEN_EXECUTOR,
                 MavenExecutorTestSupport.FORKED_MAVEN_EXECUTOR);
-        String localRepository = helper.localRepository(helper.executorRequest());
+        String localRepository = new ToolboxTool(helper).localRepository(helper.executorRequest());
         Path local = Paths.get(localRepository);
         assertTrue(Files.isDirectory(local));
     }
@@ -122,7 +123,7 @@ void localRepository4(ExecutorHelper.Mode mode) {
                 userHome,
                 MavenExecutorTestSupport.EMBEDDED_MAVEN_EXECUTOR,
                 MavenExecutorTestSupport.FORKED_MAVEN_EXECUTOR);
-        String localRepository = helper.localRepository(helper.executorRequest());
+        String localRepository = new ToolboxTool(helper).localRepository(helper.executorRequest());
         Path local = Paths.get(localRepository);
         assertTrue(Files.isDirectory(local));
     }
@@ -137,7 +138,8 @@ void artifactPath3(ExecutorHelper.Mode mode) {
                 userHome,
                 MavenExecutorTestSupport.EMBEDDED_MAVEN_EXECUTOR,
                 MavenExecutorTestSupport.FORKED_MAVEN_EXECUTOR);
-        String path = helper.artifactPath(helper.executorRequest(), "aopalliance:aopalliance:1.0", "central");
+        String path = new ToolboxTool(helper)
+                .artifactPath(helper.executorRequest(), "aopalliance:aopalliance:1.0", "central");
         // split repository: assert "ends with" as split may introduce prefixes
         assertTrue(
                 path.endsWith("aopalliance" + File.separator + "aopalliance" + File.separator + "1.0" + File.separator
@@ -155,7 +157,8 @@ void artifactPath4(ExecutorHelper.Mode mode) {
                 userHome,
                 MavenExecutorTestSupport.EMBEDDED_MAVEN_EXECUTOR,
                 MavenExecutorTestSupport.FORKED_MAVEN_EXECUTOR);
-        String path = helper.artifactPath(helper.executorRequest(), "aopalliance:aopalliance:1.0", "central");
+        String path = new ToolboxTool(helper)
+                .artifactPath(helper.executorRequest(), "aopalliance:aopalliance:1.0", "central");
         // split repository: assert "ends with" as split may introduce prefixes
         assertTrue(
                 path.endsWith("aopalliance" + File.separator + "aopalliance" + File.separator + "1.0" + File.separator
@@ -173,7 +176,7 @@ void metadataPath3(ExecutorHelper.Mode mode) {
                 userHome,
                 MavenExecutorTestSupport.EMBEDDED_MAVEN_EXECUTOR,
                 MavenExecutorTestSupport.FORKED_MAVEN_EXECUTOR);
-        String path = helper.metadataPath(helper.executorRequest(), "aopalliance", "someremote");
+        String path = new ToolboxTool(helper).metadataPath(helper.executorRequest(), "aopalliance", "someremote");
         // split repository: assert "ends with" as split may introduce prefixes
         assertTrue(path.endsWith("aopalliance" + File.separator + "maven-metadata-someremote.xml"), "path=" + path);
     }
@@ -188,7 +191,7 @@ void metadataPath4(ExecutorHelper.Mode mode) {
                 userHome,
                 MavenExecutorTestSupport.EMBEDDED_MAVEN_EXECUTOR,
                 MavenExecutorTestSupport.FORKED_MAVEN_EXECUTOR);
-        String path = helper.metadataPath(helper.executorRequest(), "aopalliance", "someremote");
+        String path = new ToolboxTool(helper).metadataPath(helper.executorRequest(), "aopalliance", "someremote");
         // split repository: assert "ends with" as split may introduce prefixes
         assertTrue(path.endsWith("aopalliance" + File.separator + "maven-metadata-someremote.xml"), "path=" + path);
     }

its/core-it-suite/src/test/java/org/apache/maven/it/MavenITmng3477DependencyResolutionErrorMessageTest.java

@@ -96,7 +96,7 @@ void connectionProblemsPlugin() throws Exception {
                 new String[] {
                     ".*The following artifacts could not be resolved: org.apache.maven.its.plugins:maven-it-plugin-not-exists:pom:1.2.3 \\(absent\\): "
                             + "Could not transfer artifact org.apache.maven.its.plugins:maven-it-plugin-not-exists:pom:1.2.3 from/to "
-                            + "maven-core-it \\(http://localhost:.*/repo\\): Connection to http://localhost:.*2/repo/ refused.*"
+                            + "central \\(http://localhost:.*/repo\\): Connection to http://localhost:.*2/repo/ refused.*"
                 },
                 "pom-plugin.xml");
     }

its/core-it-suite/src/test/resources-filtered/settings-remote.xml

@@ -49,7 +49,7 @@ plugins/artifacts from test repos so use of these settings should be the excepti
             <enabled>true</enabled>
           </releases>
           <snapshots>
-            <enabled>true</enabled>
+            <enabled>false</enabled>
           </snapshots>
         </repository>
       </repositories>
@@ -62,7 +62,7 @@ plugins/artifacts from test repos so use of these settings should be the excepti
             <enabled>true</enabled>
           </releases>
           <snapshots>
-            <enabled>true</enabled>
+            <enabled>false</enabled>
           </snapshots>
         </pluginRepository>
       </pluginRepositories>

its/core-it-suite/src/test/resources-filtered/settings.xml

@@ -51,7 +51,7 @@ own test repos or can employ the local repository (after bootstrapping). This co
             <enabled>true</enabled>
           </releases>
           <snapshots>
-            <enabled>true</enabled>
+            <enabled>false</enabled>
           </snapshots>
         </pluginRepository>
       </pluginRepositories>

its/core-it-suite/src/test/resources/mng-5175/settings-template.xml

@@ -20,5 +20,46 @@
       <url>http://localhost:@port@/archiva/repository/internal/</url>
     </mirror>
   </mirrors>
+  <!--
+  This IT got Central only from env, then set it mirrorOf* and expected to get snapshot;
+  This is wrong: As IT had no new repo in context, only default ones (from env, Central) and none of them
+  serve snapshots; despite mirrorOf*. Fixing it by adding a fluke snapshot enabled repo below
+  -->
+  <profiles>
+    <profile>
+      <id>maven-core-it-repo</id>
+      <repositories>
+        <repository>
+          <id>maven-core-it</id>
+          <url>http://unimportant</url>
+          <releases>
+            <enabled>true</enabled>
+            <checksumPolicy>ignore</checksumPolicy>
+          </releases>
+          <snapshots>
+            <enabled>true</enabled>
+            <checksumPolicy>ignore</checksumPolicy>
+          </snapshots>
+        </repository>
+      </repositories>
+      <pluginRepositories>
+        <pluginRepository>
+          <id>maven-core-it</id>
+          <url>http://localhost:@port@/repo</url>
+          <releases>
+            <enabled>true</enabled>
+            <checksumPolicy>ignore</checksumPolicy>
+          </releases>
+          <snapshots>
+            <enabled>true</enabled>
+            <checksumPolicy>ignore</checksumPolicy>
+          </snapshots>
+        </pluginRepository>
+      </pluginRepositories>
+    </profile>
+  </profiles>
+  <activeProfiles>
+    <activeProfile>maven-core-it-repo</activeProfile>
+  </activeProfiles>
 </settings>