[PR] Fix #10442: mvnval, a tool to validate POMs without building them [maven]

["ikxeno (via GitHub)" <gi...@apache.org>]

ikxeno opened a new pull request, #13276:
URL: https://github.com/apache/maven/pull/13276

   Two commits, reviewable apart:
   
   1. `buildRaw` on `ModelBuilder.ModelBuilderSession`, with its own tests.
   2. The `mvnval` tool, which uses it.
   
   ```
   $ mvnval pom.xml
   pom.xml:
     ERROR 'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique:
           junit:junit:jar -> version 4.13.2 vs 4.12 @ line 12, column 5
   ```
   
   Takes any number of POMs, defaults to `./pom.xml`, writes text or `--format json`.
   Exit 0 when nothing was reported, 1 when anything was rejected, 2 on bad usage,
   3 when only warnings were. Reachable as `mvnval` and as `mvn --val`.
   
   I ran it over 2064 real POMs while writing this: 406 in this repo, 1638 IT
   resources, and 20 published ones from Central. It found ten duplicate dependency
   declarations in `io.netty:netty-all:4.1.115.Final` (lines 848, 1062 and 1111 all
   declare `netty-transport-native-epoll:linux-x86_64`), and it named the deliberately
   broken fixtures under `src/test/resources-project-builder` for what they are.
   
   ## Why not `mvn -f pom.xml validate`
   
   Because that needs a project you can build. It resolves the parent, so it wants
   the network and a populated local repository, and it stops at the first thing it
   cannot resolve. `mvnval` reads files and nothing else. That makes it usable on a
   POM whose parent is not published yet, and in a pre-commit hook or a CI gate with
   no repository at all.
   
   ## Why a new API method
   
   `buildRawModel(request)` already exists, and it is not enough: it returns the
   `Model` and drops the problem collector, so the warnings never reach the caller.
   `build()` keeps the problems but throws as soon as validation reports an error, so
   a POM with one error and four warnings tells you about the error only. This tool
   needs the model read and the problems kept.
   
   `default`, not abstract, so anything implementing `ModelBuilderSession` against
   4.0.0 still compiles. There is one implementor in tree and no japicmp gate on
   `maven-api-core`, so this is about third parties, not about the build.
   
   ## What the tool cannot see
   
   Validation stops at the raw model, so the reachable checks are a subset. A
   dependency whose version comes from the parent's `dependencyManagement` is only
   caught in `validateEffectiveModel`. That is what a verdict depending on the files
   alone costs. `--help` says so, so nobody has to find out the hard way.
   
   ## Decisions worth arguing about
   
   **The reactor is mapped before the POM is read.** A 4.1.0 subproject may leave out
   its parent version, and it is taken from the parent's file model. Without the walk
   a perfectly valid subproject fails with `'parent.version' is missing`.
   
   **A reactor module that fails to load is logged, not reported.** `loadFilePom`
   reports that failure against the caller's result, so before this change
   `mvnval a/pom.xml` said `a/pom.xml` was broken when the culprit was `b/pom.xml`,
   and the `source` field named `a/pom.xml` too. The flag is scoped to `buildRaw`.
   `build()` is untouched and still fails on a broken module, because a build that
   silently skipped one would be worse.
   
   **Warnings exit 3, not 2.** The CLI returns 2 for a tool that failed in a way it
   does not handle, which is the same 2 the siblings use for `BAD_OPERATION`. A gate
   has to be able to tell "this POM has warnings" from "mvnval broke", so warnings
   got a code the framework does not produce.
   
   **One problem reported twice is collapsed in the tool.** The parent version check
   sits in both `validateFileModel` and `validateRawModel`, so the model builder hands
   the same finding over twice. I collapsed it in `Report` rather than touching the
   validator, since changing that changes `mvn`.
   
   **A fresh `ModelBuilderSession` per POM.** Sharing one across files would share
   `dag` and `mappedSources`, and two roots holding the same groupId:artifactId then
   trip `No unique Source for ...`. Slower, but it cannot report on the wrong file.
   
   **JSON assembled by hand.** No JSON library is on the CLI classpath and I would
   rather not put one there for one output format. Non-ASCII is escaped as `\uXXXX`
   because the writer's charset is the console's, which on Windows is routinely not
   UTF-8, and a path can carry an unpaired surrogate that no charset encodes.
   
   **A standalone `Session`, built in `createSession()`.** `LookupInvoker` only ever
   builds a `ProtoSession`, and `ModelBuilderRequest` needs a full `Session`. `mvnup`
   has the same problem and solves it in `MvnupSessionHolder`. A bare session is
   enough here because nothing is resolved and no transport is configured.
   
   **Not an `mvnsh` built-in yet.** The registry builds its completer from
   `lookupMap(Goal.class)` and `mvnval` has no goals, so wiring it up means changing
   `BuiltinShellCommandRegistryFactory`, not this tool. Happy to do it in a follow-up
   if you would rather it landed together.
   
   **`THREAT_MODEL.md` gets a row.** It reads operator POMs and emits their content
   into a machine-readable document. It resolves no parent, opens no connection and
   writes nothing back, and the row says exactly that.
   
   ## Checked
   
   `mvn verify` on `maven-api-cli`, `maven-api-core`, `maven-impl` and `maven-cli`,
   plus the first commit alone in a separate worktree. Checkstyle clean, javadoc clean
   under doclint. `MavenITgh10442MvnvalValidatePomTest` passes, 3 tests. Distribution
   built and the tool exercised from the unpacked zip, including paths with spaces and
   with Cyrillic, which round-trip through the JSON escaping unchanged.
   
   Two ITs fail on my machine for reasons this branch does not touch, and I have not
   checked them against `master`: `MavenITmng4428FollowHttpRedirectTest.testitHttpsToHttps`,
   which is HTTPS transport, and `MavenITmng7045DropUselessAndOutdatedCdiApiTest`, where
   Groovy 4.0.28 dies with "BUG! exception in phase 'semantic analysis'" on JDK 25 after
   the model has already built.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MNG-8650] mvnval, a tool to validate POMs without building them [maven]

["ikxeno (via GitHub)" <gi...@apache.org>]

ikxeno commented on code in PR #13276:
URL: https://github.com/apache/maven/pull/13276#discussion_r4116409625


##########
apache-maven/src/assembly/maven/bin/mvnval.cmd:
##########
@@ -0,0 +1,39 @@
+@REM Licensed to the Apache Software Foundation (ASF) under one
+@REM or more contributor license agreements.  See the NOTICE file
+@REM distributed with this work for additional information
+@REM regarding copyright ownership.  The ASF licenses this file
+@REM to you under the Apache License, Version 2.0 (the
+@REM "License"); you may not use this file except in compliance
+@REM with the License.  You may obtain a copy of the License at
+@REM
+@REM    http://www.apache.org/licenses/LICENSE-2.0
+@REM
+@REM Unless required by applicable law or agreed to in writing,
+@REM software distributed under the License is distributed on an
+@REM "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+@REM KIND, either express or implied.  See the License for the
+@REM specific language governing permissions and limitations
+@REM under the License.
+
+@REM -----------------------------------------------------------------------------
+@REM Apache Maven POM Validation Script
+@REM
+@REM Environment Variable Prerequisites
+@REM
+@REM   JAVA_HOME           (Optional) Points to a Java installation.
+@REM   MAVEN_BATCH_ECHO    (Optional) Set to 'on' to enable the echoing of the batch commands.

Review Comment:
     `on` is what the other launchers say and what the code compares against:
   ```
     mvn.cmd:25     @REM   MAVEN_BATCH_ECHO  (Optional) Set to 'on' to enable ...
     
     mvnenc.cmd:24  @REM   MAVEN_BATCH_ECHO    (Optional) Set to 'on' to enable ...
     
     mvnval.cmd:35  @if "%MAVEN_BATCH_ECHO%"=="on" echo %MAVEN_BATCH_ECHO%
     ```
   
     I'd rather not make mvnval the one file that differs. Happy to move the whole set to true in a separate PR if that's what you want.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MNG-8650] mvnval, a tool to validate POMs without building them [maven]

["ikxeno (via GitHub)" <gi...@apache.org>]

ikxeno commented on code in PR #13276:
URL: https://github.com/apache/maven/pull/13276#discussion_r4116397507


##########
impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnval/CommonsCliValidateOptionsTest.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0

Review Comment:
   Every Java file in the tree uses the http form:
   ```
   $ grep -rl 'http://www.apache.org/licenses/LICENSE-2.0' --include='.java' | wc -l
   1322
   
   $ grep -rl 'https://www.apache.org/licenses/LICENSE-2.0' --include='.java' | wc -l
   0
   ```
   So the new files match their neighbours rather than starting a second style. If the project wants https, that's a tree-wide change and I'll do it as its own PR. Tell me which you prefer and I'll follow it here.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MNG-8650] mvnval, a tool to validate POMs without building them [maven]

["gnodet (via GitHub)" <gi...@apache.org>]

gnodet commented on PR #13276:
URL: https://github.com/apache/maven/pull/13276#issuecomment-5874820199

   Thanks for addressing the rename, the cache cleanup, and the shared session — those are all improvements.
   
   On the network question: I think the default should be `--mode effective` (full effective model validation, downloading parents into the local repository as needed), with `--mode raw` as an explicit opt-out for the offline / pre-commit case. The raw mode is useful, but it's the more limited option — defaulting to it means users get incomplete validation silently.
   
   For the local repository, the default should be `~/.m2/repository` as usual, with two options:
   - `--local-repository <path>` / `-Dmaven.repo.local` for an explicit path
   - `--temp-local-repository` for a throwaway directory deleted on exit (the CI isolation case)
   
   `ApiRunner.createSession(injectorConsumer, localRepo)` already accepts the path — this is purely wiring on the `mvnval` side, no new API needed.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MNG-8650] mvnval, a tool to validate POMs without building them [maven]

["gnodet-bot (via GitHub)" <gi...@apache.org>]

gnodet-bot commented on code in PR #13276:
URL: https://github.com/apache/maven/pull/13276#discussion_r4212141270


##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnval/ValidateInvoker.java:
##########
@@ -0,0 +1,736 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.cling.invoker.mvnval;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.Comparator;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.function.Consumer;
+import java.util.stream.Stream;
+
+import org.apache.maven.api.Constants;
+import org.apache.maven.api.RemoteRepository;
+import org.apache.maven.api.Session;
+import org.apache.maven.api.annotations.Nullable;
+import org.apache.maven.api.cli.InvokerException;
+import org.apache.maven.api.cli.InvokerRequest;
+import org.apache.maven.api.cli.mvnval.ValidateOptions;
+import org.apache.maven.api.di.Named;
+import org.apache.maven.api.di.Provides;
+import org.apache.maven.api.model.Model;
+import org.apache.maven.api.model.Profile;
+import org.apache.maven.api.model.Repository;
+import org.apache.maven.api.model.RepositoryPolicy;
+import org.apache.maven.api.services.Lookup;
+import org.apache.maven.api.services.ModelBuilder;
+import org.apache.maven.api.services.ModelBuilder.ModelBuilderSession;
+import org.apache.maven.api.services.ModelBuilderException;
+import org.apache.maven.api.services.ModelBuilderRequest;
+import org.apache.maven.api.services.ModelBuilderRequest.RequestType;
+import org.apache.maven.api.services.ModelBuilderResult;
+import org.apache.maven.api.services.RepositoryFactory;
+import org.apache.maven.api.services.SettingsBuilder;
+import org.apache.maven.api.services.Sources;
+import org.apache.maven.cling.invoker.LookupContext;
+import org.apache.maven.cling.invoker.LookupInvoker;
+import org.apache.maven.cling.logging.Slf4jConfiguration;
+import org.apache.maven.impl.InternalSession;
+import org.apache.maven.impl.standalone.ApiRunner;
+import org.apache.maven.jline.MessageUtils;
+import org.eclipse.aether.DefaultRepositorySystemSession;
+import org.eclipse.aether.spi.connector.transport.TransporterFactory;
+import org.eclipse.aether.spi.connector.transport.http.ChecksumExtractor;
+import org.eclipse.aether.spi.io.PathProcessor;
+import org.eclipse.aether.transport.apache.ApacheTransporterFactory;
+import org.eclipse.aether.transport.file.FileTransporterFactory;
+import org.jline.reader.UserInterruptException;
+import org.jline.terminal.Terminal;
+
+/**
+ * Validates POM files without building them.
+ * <p>
+ * Two modes, and one contains the other. {@code raw} stops at
+ * {@link ModelBuilder.ModelBuilderSession#validate}, reaching no repository at all: it works
+ * offline and on a POM whose parent is not published yet, but it cannot see a problem that
+ * inheritance introduces, such as a dependency whose version comes from the parent's
+ * {@code dependencyManagement}. {@code effective}, the default, runs that pass and then builds
+ * the effective model as well, so parents and imported boms are resolved and the checks needing
+ * them run too. Both passes, because the effective build says nothing about the reactor around a
+ * POM: on its own it passes a project whose declared subproject is not on disk, which
+ * {@code mvn} refuses to read.
+ * <p>
+ * Nothing is ever written back to a POM. In {@code effective} mode what is resolved lands in the
+ * local repository, as it does for every Maven tool. {@code --local-repository} aims that
+ * somewhere else and {@code --temp-local-repository} at a directory made for the run and deleted
+ * when it ends, for a gate that wants the configured repository left as it was.
+ * <p>
+ * It stands up no container, so the directory holding a POM cannot make this process load
+ * anything through {@code .mvn/extensions.xml}, {@code maven.ext.class.path} or
+ * {@code .mvn/settings.xml}. The user's own settings are another matter: resolution reads them
+ * for mirrors, proxies and credentials, as it has to. The JVM is outside this:
+ * {@code bin/mvn} passes {@code .mvn/jvm.config} to the launcher before this process starts.
+ * <p>
+ * {@code -o} works in {@code effective} mode: resolution hits the local repository only and
+ * fails fast when a parent or BOM is absent. Combined with {@code --temp-local-repository} it
+ * proves that the POMs being validated are fully self-contained.
+ * <p>
+ * When more than one POM is given, {@code effective} mode pre-scans them all to build a bundle
+ * workspace: a parent that is named in the bundle is resolved from disk rather than from a
+ * repository, so the verdict does not depend on publication order and the run does not download
+ * what is already on disk.
+ */
+public class ValidateInvoker extends LookupInvoker<ValidateContext> {
+
+    /** Where Central lives, for the one case where no settings file names a repository. */
+    private static final String CENTRAL_URL = "https://repo.maven.apache.org/maven2";
+
+    public static final int OK = 0;
+    public static final int ERROR = 1;
+
+    /** Bad user input. Same meaning as in {@code mvnenc} and {@code mvnup}. */
+    public static final int BAD_OPERATION = 2;
+
+    /**
+     * Interrupted. Same meaning as in {@code mvnenc} and {@code mvnup}, and reachable only with a
+     * terminal attached: without one the signal reaches the JVM instead and the exit code is its.
+     * Left out of {@code --help} for that reason.
+     */
+    public static final int CANCELED = 3;
+
+    /**
+     * Nothing was rejected, but something was reported. Numbered 4 rather than reusing a lower
+     * code: 0 to 3 mean the same thing across the Maven 4 tools, and a gate must be able to tell
+     * "this POM has warnings" from "mvnval broke" or "somebody hit Ctrl+C".
+     */
+    public static final int WARNINGS = 4;
+
+    public ValidateInvoker(Lookup protoLookup, @Nullable Consumer<LookupContext> contextConsumer) {
+        super(protoLookup, contextConsumer);
+    }
+
+    @Override
+    protected ValidateContext createContext(InvokerRequest invokerRequest) {
+        return new ValidateContext(
+                invokerRequest, (ValidateOptions) invokerRequest.options().orElse(null));
+    }
+
+    @Override
+    protected int execute(ValidateContext context) throws Exception {
+        Thread validationThread = Thread.currentThread();
+        AtomicBoolean cancellationRequested = new AtomicBoolean();
+        Terminal.SignalHandler previousHandler = context.terminal.handle(Terminal.Signal.INT, signal -> {
+            cancellationRequested.set(true);
+            validationThread.interrupt();
+        });
+        try {
+            OutputFormat format;
+            ValidationMode mode;
+            try {
+                // Checked here rather than while parsing: the parser knows the option names,
+                // not the values, and a value it did accept can still become anything once
+                // options are interpolated.
+                format = context.options().format().map(OutputFormat::parse).orElse(OutputFormat.TEXT);
+                mode = context.options().mode().map(ValidationMode::parse).orElse(ValidationMode.EFFECTIVE);
+            } catch (IllegalArgumentException e) {
+                context.logger.error(e.getMessage());
+                return BAD_OPERATION;
+            }
+            // -s/-ps/-is redirect the settings files that ApiRunner reads at session-creation time.
+            // Honouring them would require threading the alternate path into ApiRunner before it
+            // starts; that is a larger change than belongs here, so they are still refused.
+            // -o is different: it applies to the resolver session, which createSession() returns,
+            // and can be applied there after the session is built.  Raw mode resolves nothing, so
+            // none of these apply there.
+            if (mode != ValidationMode.RAW) {
+                String refused = refusedSettingsOption(context.options());
+                if (refused != null) {
+                    context.logger.error(refused
+                            + " cannot redirect the settings file mvnval reads."
+                            + " Use the default settings, or copy the relevant settings to the default location.");
+                    return BAD_OPERATION;
+                }
+            }
+
+            if (context.options().localRepository().isPresent()
+                    && context.options().tempLocalRepository().orElse(false)) {
+                context.logger.error(
+                        "--local-repository and --temp-local-repository name different directories; give one.");
+                return BAD_OPERATION;
+            }
+            String unusableRepository = unusableRepository(context);
+            if (unusableRepository != null) {
+                context.logger.error(unusableRepository);
+                return BAD_OPERATION;
+            }
+            Path localRepository = null;
+            // Only in effective mode: raw resolves nothing, so making a directory there could
+            // lose a run that would never have written to it.
+            if (mode != ValidationMode.RAW) {
+                try {
+                    localRepository = localRepository(context);
+                } catch (IOException e) {
+                    // Not ERROR: no POM was rejected, the run could not be set up.
+                    context.logger.error("Could not create a temporary local repository: " + e);
+                    return BAD_OPERATION;
+                }
+            }
+            Thread cleanup = temporary(context, localRepository) ? cleanupHook(localRepository, context) : null;
+            try {
+                List<Report> reports =
+                        validateAll(context, createSession(localRepository), mode, cancellationRequested);
+                if (cancellationRequested.get() || Thread.currentThread().isInterrupted()) {
+                    return canceled(context);
+                }
+                // determineWriter, not context.writer: on a real run that field is still empty
+                // and this is what fills it.
+                int contextLines = context.options().context().orElse(2);
+                format.report(
+                        reports,
+                        context.cwd.get(),
+                        determineWriter(context),
+                        contextLines,
+                        MessageUtils.isColorEnabled());
+                return exitCode(reports);
+            } finally {
+                if (cleanup != null) {
+                    // Delete first, deregister second. The other order leaves a window where a
+                    // signal arriving after the hook is gone kills the process before the
+                    // deletion runs.
+                    deleteRecursively(localRepository, context);
+                    removeHook(cleanup);
+                }
+            }
+        } catch (UnsupportedOperationException e) {
+            // The ModelBuilder in this container has not written validate. Nothing was wrong with
+            // any POM, so this must not be ERROR, the code a gate reads as "a POM was rejected".
+            context.logger.error("This Maven installation cannot validate POMs: " + e.getMessage());
+            return BAD_OPERATION;
+        } catch (UserInterruptException e) {
+            return canceled(context);
+        } catch (Exception e) {
+            if (cancellationRequested.get() || Thread.currentThread().isInterrupted()) {
+                return canceled(context);
+            }
+            // Without this an unexpected failure escapes to the CLI and becomes 2, which this tool
+            // documents as bad usage. Both siblings catch here too.
+            if (context.options().showErrors().orElse(false)) {
+                context.logger.error(e.getMessage(), e);
+            } else {
+                context.logger.error(e.getMessage());
+            }
+            return ERROR;
+        } finally {
+            context.terminal.handle(Terminal.Signal.INT, previousHandler);
+            if (cancellationRequested.get()) {
+                Thread.interrupted();
+            }
+        }
+    }
+
+    private static int canceled(ValidateContext context) {
+        Thread.interrupted();
+        context.logger.error("Validation canceled by user.");
+        return CANCELED;
+    }
+
+    /**
+     * Turns an unparseable command line into {@link #BAD_OPERATION}.
+     * <p>
+     * The base exits 1 for a bad argument, which is the code this tool uses for a POM it rejected.
+     * A gate has to be able to tell a typo in its own script from a POM that failed validation.
+     */
+    @Override
+    protected void validate(ValidateContext context) throws Exception {
+        try {
+            super.validate(context);
+        } catch (InvokerException.ExitException e) {
+            throw e.getExitCode() == ERROR ? new InvokerException.ExitException(BAD_OPERATION) : e;
+        }
+    }
+
+    /** Whether stdout carries a document this run must not write anything else to. */
+    private static boolean json(ValidateContext context) {
+        return context.options()
+                .format()
+                .filter(OutputFormat.JSON.name()::equalsIgnoreCase)
+                .isPresent();
+    }
+
+    /**
+     * Keeps the version banner out of the document under {@code --format json}.
+     * <p>
+     * The base prints it whenever {@code effectiveVerbose()} holds, which a CI runner setting
+     * {@code RUNNER_DEBUG=1} makes true, so re-running a job with debug logging turned the
+     * document into something no parser accepts. Asking for {@code -V} still prints it, since
+     * that is what the flag is for.
+     */
+    @Override
+    protected void preCommands(ValidateContext context) throws Exception {
+        if (json(context) && !context.options().showVersion().orElse(false)) {
+            return;
+        }
+        super.preCommands(context);
+    }
+
+    /**
+     * Suppresses resolver noise from the output unless the caller asked for it.
+     * <p>
+     * The resolver writes an {@code [INFO]} line the first time it reads a repository's prefix
+     * file. Under {@code --format json} that line breaks the document; in text mode it clutters
+     * output that is meant to list only POM problems. Neither case benefits from seeing it.
+     * The level is forced to {@code ERROR} — the same effect as {@code -q} — unless {@code -X}
+     * or {@code -e} is given, in which case the full log is what the caller wants.
+     * <p>
+     * The property must be set <em>before</em> calling {@link Slf4jConfiguration#setRootLoggerLevel}
+     * because that method itself logs at {@code INFO} when it overrides a value already set (e.g.
+     * a CI runner running with {@code DEBUG}), which would be the very noise we are suppressing.
+     */
+    @Override
+    protected void configureLogging(ValidateContext context) throws Exception {
+        super.configureLogging(context);
+        if (!context.options().verbose().orElse(false)
+                && !context.options().showErrors().orElse(false)) {
+            context.loggerLevel = Slf4jConfiguration.Level.ERROR;
+            System.setProperty(Constants.MAVEN_LOGGER_DEFAULT_LOG_LEVEL, "error");
+            context.slf4jConfiguration.setRootLoggerLevel(context.loggerLevel);
+        }
+    }
+
+    // The next five steps stand up dependency injection and read configuration. Nothing here uses
+    // either: the model builder comes from createSession(). Running them would let the directory
+    // holding the POM decide what this process loads, via .mvn/extensions.xml, .mvn/settings.xml,
+    // or maven.ext.class.path in .mvn/maven-user.properties. Refused one at a time rather than by
+    // replacing doInvoke, so a step added to the base class later still runs.
+
+    /** No container: nothing is looked up, and no extension or contributed property is loaded. */
+    @Override
+    protected void container(ValidateContext context) {}
+
+    /** No container, so no {@code PropertyContributor} to run. */
+    @Override
+    protected void postContainer(ValidateContext context) {}
+
+    /** No container to look up from. */
+    @Override
+    protected void lookup(ValidateContext context) {}
+
+    /** No {@code EventSpy} dispatch: this tool emits no build events. */
+    @Override
+    protected void init(ValidateContext context) {}
+
+    /**
+     * No settings step: the model builder runs on the session from {@link #createSession(Path)},
+     * which reads the user's settings itself when it has to resolve.
+     */
+    @Override
+    protected void settings(ValidateContext context) {}
+
+    /**
+     * Creates the session the model builder runs on. Made here because {@code LookupInvoker} only
+     * ever builds a {@code ProtoSession} while {@link ModelBuilderRequest} needs a full
+     * {@link Session}. Left {@code protected} as a seam: building one is the expensive part of
+     * every test in this package, and the tests substitute a shared one.
+     *
+     * @return the session, never {@code null}
+     */
+    protected Session createSession(@Nullable Path localRepository) {
+        Session session = ApiRunner.createSession(injector -> injector.bindImplicit(TransporterFactoryConfig.class));
+        if (localRepository != null) {
+            // Set on the session that came back, not passed to createSession, which prefers
+            // settings.getLocalRepository() over its argument and may in any case hand back a
+            // session another tool built. See derivedRepositories for why that happens.
+            session = session.withLocalRepository(session.createLocalRepository(localRepository));
+        }
+        return session.withRemoteRepositories(derivedRepositories(session));
+    }
+
+    /**
+     * Applies {@code --offline} to a session built by {@link #createSession}.
+     * <p>
+     * {@code ApiRunner.createSession} applies offline from the settings file.  This supplements
+     * that: when the caller passes {@code -o} on the command line, the session is made offline
+     * regardless of what the settings say, by cloning the underlying resolver session with offline
+     * set to true.
+     */
+    private static Session withOffline(Session session) {
+        // withLocalRepository(same repo) forces AbstractSession to clone the underlying resolver
+        // session (DefaultRepositorySystemSession) into a fresh one held only by the new session
+        // object.  That fresh clone is safe to mutate without affecting the caller's session.
+        Session copy = session.withLocalRepository(session.getLocalRepository());
+        DefaultRepositorySystemSession rsession =
+                (DefaultRepositorySystemSession) InternalSession.from(copy).getSession();
+        rsession.setOffline(true);
+        return copy;
+    }

Review Comment:
   ⚠️ ** — clone never happens; the Javadoc is wrong**
   
    calls , which short-circuits at:
   ```java
   if (session.getLocalRepository() != null
           && Objects.equals(session.getLocalRepository().getBasePath(), localRepository.path())) {
       return this;  // same path → no clone
   }
   ```
   Since both sides resolve to the same base path, .  returns the original , and  mutates it in-place. The comment claiming "That fresh clone is safe to mutate without affecting the caller's session" is false: there is no clone.
   
   This works today only because  immediately reassigns  and never touches the original reference again. Any future refactor that passes the same session object to two independent code paths (e.g. a batch validation that reuses a session across calls) will silently find the second path offline.
   
   Fix: either drop the dead clone attempt and document the in-place mutation honestly, or use a method that actually produces a copy:
   ```suggestion
       private static Session withOffline(Session session) {
           // AbstractSession.withLocalRepository short-circuits when the path is unchanged
           // (it returns this). Cast directly to the mutable impl and mutate in-place.
           // Safe only because the caller (validateAll) does not retain a reference to the
           // original session after calling this method — the reassignment is the contract.
           DefaultRepositorySystemSession rsession =
                   (DefaultRepositorySystemSession) InternalSession.from(session).getSession();
           rsession.setOffline(true);
           return session;
       }
   ```



##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnval/ValidateInvoker.java:
##########
@@ -0,0 +1,736 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.cling.invoker.mvnval;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.Comparator;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.function.Consumer;
+import java.util.stream.Stream;
+
+import org.apache.maven.api.Constants;
+import org.apache.maven.api.RemoteRepository;
+import org.apache.maven.api.Session;
+import org.apache.maven.api.annotations.Nullable;
+import org.apache.maven.api.cli.InvokerException;
+import org.apache.maven.api.cli.InvokerRequest;
+import org.apache.maven.api.cli.mvnval.ValidateOptions;
+import org.apache.maven.api.di.Named;
+import org.apache.maven.api.di.Provides;
+import org.apache.maven.api.model.Model;
+import org.apache.maven.api.model.Profile;
+import org.apache.maven.api.model.Repository;
+import org.apache.maven.api.model.RepositoryPolicy;
+import org.apache.maven.api.services.Lookup;
+import org.apache.maven.api.services.ModelBuilder;
+import org.apache.maven.api.services.ModelBuilder.ModelBuilderSession;
+import org.apache.maven.api.services.ModelBuilderException;
+import org.apache.maven.api.services.ModelBuilderRequest;
+import org.apache.maven.api.services.ModelBuilderRequest.RequestType;
+import org.apache.maven.api.services.ModelBuilderResult;
+import org.apache.maven.api.services.RepositoryFactory;
+import org.apache.maven.api.services.SettingsBuilder;
+import org.apache.maven.api.services.Sources;
+import org.apache.maven.cling.invoker.LookupContext;
+import org.apache.maven.cling.invoker.LookupInvoker;
+import org.apache.maven.cling.logging.Slf4jConfiguration;
+import org.apache.maven.impl.InternalSession;
+import org.apache.maven.impl.standalone.ApiRunner;
+import org.apache.maven.jline.MessageUtils;
+import org.eclipse.aether.DefaultRepositorySystemSession;
+import org.eclipse.aether.spi.connector.transport.TransporterFactory;
+import org.eclipse.aether.spi.connector.transport.http.ChecksumExtractor;
+import org.eclipse.aether.spi.io.PathProcessor;
+import org.eclipse.aether.transport.apache.ApacheTransporterFactory;
+import org.eclipse.aether.transport.file.FileTransporterFactory;
+import org.jline.reader.UserInterruptException;
+import org.jline.terminal.Terminal;
+
+/**
+ * Validates POM files without building them.
+ * <p>
+ * Two modes, and one contains the other. {@code raw} stops at
+ * {@link ModelBuilder.ModelBuilderSession#validate}, reaching no repository at all: it works
+ * offline and on a POM whose parent is not published yet, but it cannot see a problem that
+ * inheritance introduces, such as a dependency whose version comes from the parent's
+ * {@code dependencyManagement}. {@code effective}, the default, runs that pass and then builds
+ * the effective model as well, so parents and imported boms are resolved and the checks needing
+ * them run too. Both passes, because the effective build says nothing about the reactor around a
+ * POM: on its own it passes a project whose declared subproject is not on disk, which
+ * {@code mvn} refuses to read.
+ * <p>
+ * Nothing is ever written back to a POM. In {@code effective} mode what is resolved lands in the
+ * local repository, as it does for every Maven tool. {@code --local-repository} aims that
+ * somewhere else and {@code --temp-local-repository} at a directory made for the run and deleted
+ * when it ends, for a gate that wants the configured repository left as it was.
+ * <p>
+ * It stands up no container, so the directory holding a POM cannot make this process load
+ * anything through {@code .mvn/extensions.xml}, {@code maven.ext.class.path} or
+ * {@code .mvn/settings.xml}. The user's own settings are another matter: resolution reads them
+ * for mirrors, proxies and credentials, as it has to. The JVM is outside this:
+ * {@code bin/mvn} passes {@code .mvn/jvm.config} to the launcher before this process starts.
+ * <p>
+ * {@code -o} works in {@code effective} mode: resolution hits the local repository only and
+ * fails fast when a parent or BOM is absent. Combined with {@code --temp-local-repository} it
+ * proves that the POMs being validated are fully self-contained.
+ * <p>
+ * When more than one POM is given, {@code effective} mode pre-scans them all to build a bundle
+ * workspace: a parent that is named in the bundle is resolved from disk rather than from a
+ * repository, so the verdict does not depend on publication order and the run does not download
+ * what is already on disk.
+ */
+public class ValidateInvoker extends LookupInvoker<ValidateContext> {
+
+    /** Where Central lives, for the one case where no settings file names a repository. */
+    private static final String CENTRAL_URL = "https://repo.maven.apache.org/maven2";
+
+    public static final int OK = 0;
+    public static final int ERROR = 1;
+
+    /** Bad user input. Same meaning as in {@code mvnenc} and {@code mvnup}. */
+    public static final int BAD_OPERATION = 2;
+
+    /**
+     * Interrupted. Same meaning as in {@code mvnenc} and {@code mvnup}, and reachable only with a
+     * terminal attached: without one the signal reaches the JVM instead and the exit code is its.
+     * Left out of {@code --help} for that reason.
+     */
+    public static final int CANCELED = 3;
+
+    /**
+     * Nothing was rejected, but something was reported. Numbered 4 rather than reusing a lower
+     * code: 0 to 3 mean the same thing across the Maven 4 tools, and a gate must be able to tell
+     * "this POM has warnings" from "mvnval broke" or "somebody hit Ctrl+C".
+     */
+    public static final int WARNINGS = 4;
+
+    public ValidateInvoker(Lookup protoLookup, @Nullable Consumer<LookupContext> contextConsumer) {
+        super(protoLookup, contextConsumer);
+    }
+
+    @Override
+    protected ValidateContext createContext(InvokerRequest invokerRequest) {
+        return new ValidateContext(
+                invokerRequest, (ValidateOptions) invokerRequest.options().orElse(null));
+    }
+
+    @Override
+    protected int execute(ValidateContext context) throws Exception {
+        Thread validationThread = Thread.currentThread();
+        AtomicBoolean cancellationRequested = new AtomicBoolean();
+        Terminal.SignalHandler previousHandler = context.terminal.handle(Terminal.Signal.INT, signal -> {
+            cancellationRequested.set(true);
+            validationThread.interrupt();
+        });
+        try {
+            OutputFormat format;
+            ValidationMode mode;
+            try {
+                // Checked here rather than while parsing: the parser knows the option names,
+                // not the values, and a value it did accept can still become anything once
+                // options are interpolated.
+                format = context.options().format().map(OutputFormat::parse).orElse(OutputFormat.TEXT);
+                mode = context.options().mode().map(ValidationMode::parse).orElse(ValidationMode.EFFECTIVE);
+            } catch (IllegalArgumentException e) {
+                context.logger.error(e.getMessage());
+                return BAD_OPERATION;
+            }
+            // -s/-ps/-is redirect the settings files that ApiRunner reads at session-creation time.
+            // Honouring them would require threading the alternate path into ApiRunner before it
+            // starts; that is a larger change than belongs here, so they are still refused.
+            // -o is different: it applies to the resolver session, which createSession() returns,
+            // and can be applied there after the session is built.  Raw mode resolves nothing, so
+            // none of these apply there.
+            if (mode != ValidationMode.RAW) {
+                String refused = refusedSettingsOption(context.options());
+                if (refused != null) {
+                    context.logger.error(refused
+                            + " cannot redirect the settings file mvnval reads."
+                            + " Use the default settings, or copy the relevant settings to the default location.");
+                    return BAD_OPERATION;
+                }
+            }
+
+            if (context.options().localRepository().isPresent()
+                    && context.options().tempLocalRepository().orElse(false)) {
+                context.logger.error(
+                        "--local-repository and --temp-local-repository name different directories; give one.");
+                return BAD_OPERATION;
+            }
+            String unusableRepository = unusableRepository(context);
+            if (unusableRepository != null) {
+                context.logger.error(unusableRepository);
+                return BAD_OPERATION;
+            }
+            Path localRepository = null;
+            // Only in effective mode: raw resolves nothing, so making a directory there could
+            // lose a run that would never have written to it.
+            if (mode != ValidationMode.RAW) {
+                try {
+                    localRepository = localRepository(context);
+                } catch (IOException e) {
+                    // Not ERROR: no POM was rejected, the run could not be set up.
+                    context.logger.error("Could not create a temporary local repository: " + e);
+                    return BAD_OPERATION;
+                }
+            }
+            Thread cleanup = temporary(context, localRepository) ? cleanupHook(localRepository, context) : null;
+            try {
+                List<Report> reports =
+                        validateAll(context, createSession(localRepository), mode, cancellationRequested);
+                if (cancellationRequested.get() || Thread.currentThread().isInterrupted()) {
+                    return canceled(context);
+                }
+                // determineWriter, not context.writer: on a real run that field is still empty
+                // and this is what fills it.
+                int contextLines = context.options().context().orElse(2);
+                format.report(
+                        reports,
+                        context.cwd.get(),
+                        determineWriter(context),
+                        contextLines,
+                        MessageUtils.isColorEnabled());
+                return exitCode(reports);
+            } finally {
+                if (cleanup != null) {
+                    // Delete first, deregister second. The other order leaves a window where a
+                    // signal arriving after the hook is gone kills the process before the
+                    // deletion runs.
+                    deleteRecursively(localRepository, context);
+                    removeHook(cleanup);
+                }
+            }
+        } catch (UnsupportedOperationException e) {
+            // The ModelBuilder in this container has not written validate. Nothing was wrong with
+            // any POM, so this must not be ERROR, the code a gate reads as "a POM was rejected".
+            context.logger.error("This Maven installation cannot validate POMs: " + e.getMessage());
+            return BAD_OPERATION;
+        } catch (UserInterruptException e) {
+            return canceled(context);
+        } catch (Exception e) {
+            if (cancellationRequested.get() || Thread.currentThread().isInterrupted()) {
+                return canceled(context);
+            }
+            // Without this an unexpected failure escapes to the CLI and becomes 2, which this tool
+            // documents as bad usage. Both siblings catch here too.
+            if (context.options().showErrors().orElse(false)) {
+                context.logger.error(e.getMessage(), e);
+            } else {
+                context.logger.error(e.getMessage());
+            }
+            return ERROR;
+        } finally {
+            context.terminal.handle(Terminal.Signal.INT, previousHandler);
+            if (cancellationRequested.get()) {
+                Thread.interrupted();
+            }
+        }
+    }
+
+    private static int canceled(ValidateContext context) {
+        Thread.interrupted();
+        context.logger.error("Validation canceled by user.");
+        return CANCELED;
+    }
+
+    /**
+     * Turns an unparseable command line into {@link #BAD_OPERATION}.
+     * <p>
+     * The base exits 1 for a bad argument, which is the code this tool uses for a POM it rejected.
+     * A gate has to be able to tell a typo in its own script from a POM that failed validation.
+     */
+    @Override
+    protected void validate(ValidateContext context) throws Exception {
+        try {
+            super.validate(context);
+        } catch (InvokerException.ExitException e) {
+            throw e.getExitCode() == ERROR ? new InvokerException.ExitException(BAD_OPERATION) : e;
+        }
+    }
+
+    /** Whether stdout carries a document this run must not write anything else to. */
+    private static boolean json(ValidateContext context) {
+        return context.options()
+                .format()
+                .filter(OutputFormat.JSON.name()::equalsIgnoreCase)
+                .isPresent();
+    }
+
+    /**
+     * Keeps the version banner out of the document under {@code --format json}.
+     * <p>
+     * The base prints it whenever {@code effectiveVerbose()} holds, which a CI runner setting
+     * {@code RUNNER_DEBUG=1} makes true, so re-running a job with debug logging turned the
+     * document into something no parser accepts. Asking for {@code -V} still prints it, since
+     * that is what the flag is for.
+     */
+    @Override
+    protected void preCommands(ValidateContext context) throws Exception {
+        if (json(context) && !context.options().showVersion().orElse(false)) {
+            return;
+        }
+        super.preCommands(context);
+    }
+
+    /**
+     * Suppresses resolver noise from the output unless the caller asked for it.
+     * <p>
+     * The resolver writes an {@code [INFO]} line the first time it reads a repository's prefix
+     * file. Under {@code --format json} that line breaks the document; in text mode it clutters
+     * output that is meant to list only POM problems. Neither case benefits from seeing it.
+     * The level is forced to {@code ERROR} — the same effect as {@code -q} — unless {@code -X}
+     * or {@code -e} is given, in which case the full log is what the caller wants.
+     * <p>
+     * The property must be set <em>before</em> calling {@link Slf4jConfiguration#setRootLoggerLevel}
+     * because that method itself logs at {@code INFO} when it overrides a value already set (e.g.
+     * a CI runner running with {@code DEBUG}), which would be the very noise we are suppressing.
+     */
+    @Override
+    protected void configureLogging(ValidateContext context) throws Exception {
+        super.configureLogging(context);
+        if (!context.options().verbose().orElse(false)
+                && !context.options().showErrors().orElse(false)) {
+            context.loggerLevel = Slf4jConfiguration.Level.ERROR;
+            System.setProperty(Constants.MAVEN_LOGGER_DEFAULT_LOG_LEVEL, "error");
+            context.slf4jConfiguration.setRootLoggerLevel(context.loggerLevel);
+        }
+    }
+
+    // The next five steps stand up dependency injection and read configuration. Nothing here uses
+    // either: the model builder comes from createSession(). Running them would let the directory
+    // holding the POM decide what this process loads, via .mvn/extensions.xml, .mvn/settings.xml,
+    // or maven.ext.class.path in .mvn/maven-user.properties. Refused one at a time rather than by
+    // replacing doInvoke, so a step added to the base class later still runs.
+
+    /** No container: nothing is looked up, and no extension or contributed property is loaded. */
+    @Override
+    protected void container(ValidateContext context) {}
+
+    /** No container, so no {@code PropertyContributor} to run. */
+    @Override
+    protected void postContainer(ValidateContext context) {}
+
+    /** No container to look up from. */
+    @Override
+    protected void lookup(ValidateContext context) {}
+
+    /** No {@code EventSpy} dispatch: this tool emits no build events. */
+    @Override
+    protected void init(ValidateContext context) {}
+
+    /**
+     * No settings step: the model builder runs on the session from {@link #createSession(Path)},
+     * which reads the user's settings itself when it has to resolve.
+     */
+    @Override
+    protected void settings(ValidateContext context) {}
+
+    /**
+     * Creates the session the model builder runs on. Made here because {@code LookupInvoker} only
+     * ever builds a {@code ProtoSession} while {@link ModelBuilderRequest} needs a full
+     * {@link Session}. Left {@code protected} as a seam: building one is the expensive part of
+     * every test in this package, and the tests substitute a shared one.
+     *
+     * @return the session, never {@code null}
+     */
+    protected Session createSession(@Nullable Path localRepository) {
+        Session session = ApiRunner.createSession(injector -> injector.bindImplicit(TransporterFactoryConfig.class));
+        if (localRepository != null) {
+            // Set on the session that came back, not passed to createSession, which prefers
+            // settings.getLocalRepository() over its argument and may in any case hand back a
+            // session another tool built. See derivedRepositories for why that happens.
+            session = session.withLocalRepository(session.createLocalRepository(localRepository));
+        }
+        return session.withRemoteRepositories(derivedRepositories(session));
+    }
+
+    /**
+     * Applies {@code --offline} to a session built by {@link #createSession}.
+     * <p>
+     * {@code ApiRunner.createSession} applies offline from the settings file.  This supplements
+     * that: when the caller passes {@code -o} on the command line, the session is made offline
+     * regardless of what the settings say, by cloning the underlying resolver session with offline
+     * set to true.
+     */
+    private static Session withOffline(Session session) {
+        // withLocalRepository(same repo) forces AbstractSession to clone the underlying resolver
+        // session (DefaultRepositorySystemSession) into a fresh one held only by the new session
+        // object.  That fresh clone is safe to mutate without affecting the caller's session.
+        Session copy = session.withLocalRepository(session.getLocalRepository());
+        DefaultRepositorySystemSession rsession =
+                (DefaultRepositorySystemSession) InternalSession.from(copy).getSession();
+        rsession.setOffline(true);
+        return copy;
+    }
+
+    /**
+     * Installs a {@link BundleWorkspaceReader} on the resolver session so that parent and BOM
+     * lookups for POMs in the bundle are answered from disk rather than from a repository.
+     * <p>
+     * The workspace reader is set on a clone of the resolver session: it does not mutate the
+     * original session, which the tests may reuse across runs.
+     */
+    private static Session withWorkspaceReader(Session session, BundleWorkspaceReader reader) {
+        Session copy = session.withLocalRepository(session.getLocalRepository());
+        DefaultRepositorySystemSession rsession =
+                (DefaultRepositorySystemSession) InternalSession.from(copy).getSession();
+        rsession.setWorkspaceReader(reader);
+        return copy;
+    }

Review Comment:
   ⚠️ ** — same broken-clone pattern, stronger safety claim**
   
   Same root cause as :  returns , so the workspace reader is installed on the original session's . The Javadoc says _"it does not mutate the original session, which the tests may reuse across runs"_ — but it does mutate it. The  in  creates a fresh session per test, so the test-isolation concern is mitigated in the current test suite, but the Javadoc guarantee is false.
   
   Fix consistently with :
   ```suggestion
       private static Session withWorkspaceReader(Session session, BundleWorkspaceReader reader) {
           // AbstractSession.withLocalRepository short-circuits when the path is unchanged
           // (returns this). Mutate in-place — safe only because validateAll reassigns
           // session immediately and does not retain the pre-mutation reference.
           DefaultRepositorySystemSession rsession =
                   (DefaultRepositorySystemSession) InternalSession.from(session).getSession();
           rsession.setWorkspaceReader(reader);
           return session;
       }
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MNG-8650] mvnval, a tool to validate POMs without building them [maven]

["gnodet-bot (via GitHub)" <gi...@apache.org>]

gnodet-bot commented on code in PR #13276:
URL: https://github.com/apache/maven/pull/13276#discussion_r4212150016


##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnval/ValidateInvoker.java:
##########
@@ -0,0 +1,736 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.cling.invoker.mvnval;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.Comparator;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.function.Consumer;
+import java.util.stream.Stream;
+
+import org.apache.maven.api.Constants;
+import org.apache.maven.api.RemoteRepository;
+import org.apache.maven.api.Session;
+import org.apache.maven.api.annotations.Nullable;
+import org.apache.maven.api.cli.InvokerException;
+import org.apache.maven.api.cli.InvokerRequest;
+import org.apache.maven.api.cli.mvnval.ValidateOptions;
+import org.apache.maven.api.di.Named;
+import org.apache.maven.api.di.Provides;
+import org.apache.maven.api.model.Model;
+import org.apache.maven.api.model.Profile;
+import org.apache.maven.api.model.Repository;
+import org.apache.maven.api.model.RepositoryPolicy;
+import org.apache.maven.api.services.Lookup;
+import org.apache.maven.api.services.ModelBuilder;
+import org.apache.maven.api.services.ModelBuilder.ModelBuilderSession;
+import org.apache.maven.api.services.ModelBuilderException;
+import org.apache.maven.api.services.ModelBuilderRequest;
+import org.apache.maven.api.services.ModelBuilderRequest.RequestType;
+import org.apache.maven.api.services.ModelBuilderResult;
+import org.apache.maven.api.services.RepositoryFactory;
+import org.apache.maven.api.services.SettingsBuilder;
+import org.apache.maven.api.services.Sources;
+import org.apache.maven.cling.invoker.LookupContext;
+import org.apache.maven.cling.invoker.LookupInvoker;
+import org.apache.maven.cling.logging.Slf4jConfiguration;
+import org.apache.maven.impl.InternalSession;
+import org.apache.maven.impl.standalone.ApiRunner;
+import org.apache.maven.jline.MessageUtils;
+import org.eclipse.aether.DefaultRepositorySystemSession;
+import org.eclipse.aether.spi.connector.transport.TransporterFactory;
+import org.eclipse.aether.spi.connector.transport.http.ChecksumExtractor;
+import org.eclipse.aether.spi.io.PathProcessor;
+import org.eclipse.aether.transport.apache.ApacheTransporterFactory;
+import org.eclipse.aether.transport.file.FileTransporterFactory;
+import org.jline.reader.UserInterruptException;
+import org.jline.terminal.Terminal;
+
+/**
+ * Validates POM files without building them.
+ * <p>
+ * Two modes, and one contains the other. {@code raw} stops at
+ * {@link ModelBuilder.ModelBuilderSession#validate}, reaching no repository at all: it works
+ * offline and on a POM whose parent is not published yet, but it cannot see a problem that
+ * inheritance introduces, such as a dependency whose version comes from the parent's
+ * {@code dependencyManagement}. {@code effective}, the default, runs that pass and then builds
+ * the effective model as well, so parents and imported boms are resolved and the checks needing
+ * them run too. Both passes, because the effective build says nothing about the reactor around a
+ * POM: on its own it passes a project whose declared subproject is not on disk, which
+ * {@code mvn} refuses to read.
+ * <p>
+ * Nothing is ever written back to a POM. In {@code effective} mode what is resolved lands in the
+ * local repository, as it does for every Maven tool. {@code --local-repository} aims that
+ * somewhere else and {@code --temp-local-repository} at a directory made for the run and deleted
+ * when it ends, for a gate that wants the configured repository left as it was.
+ * <p>
+ * It stands up no container, so the directory holding a POM cannot make this process load
+ * anything through {@code .mvn/extensions.xml}, {@code maven.ext.class.path} or
+ * {@code .mvn/settings.xml}. The user's own settings are another matter: resolution reads them
+ * for mirrors, proxies and credentials, as it has to. The JVM is outside this:
+ * {@code bin/mvn} passes {@code .mvn/jvm.config} to the launcher before this process starts.
+ * <p>
+ * {@code -o} works in {@code effective} mode: resolution hits the local repository only and
+ * fails fast when a parent or BOM is absent. Combined with {@code --temp-local-repository} it
+ * proves that the POMs being validated are fully self-contained.
+ * <p>
+ * When more than one POM is given, {@code effective} mode pre-scans them all to build a bundle
+ * workspace: a parent that is named in the bundle is resolved from disk rather than from a
+ * repository, so the verdict does not depend on publication order and the run does not download
+ * what is already on disk.
+ */
+public class ValidateInvoker extends LookupInvoker<ValidateContext> {
+
+    /** Where Central lives, for the one case where no settings file names a repository. */
+    private static final String CENTRAL_URL = "https://repo.maven.apache.org/maven2";
+
+    public static final int OK = 0;
+    public static final int ERROR = 1;
+
+    /** Bad user input. Same meaning as in {@code mvnenc} and {@code mvnup}. */
+    public static final int BAD_OPERATION = 2;
+
+    /**
+     * Interrupted. Same meaning as in {@code mvnenc} and {@code mvnup}, and reachable only with a
+     * terminal attached: without one the signal reaches the JVM instead and the exit code is its.
+     * Left out of {@code --help} for that reason.
+     */
+    public static final int CANCELED = 3;
+
+    /**
+     * Nothing was rejected, but something was reported. Numbered 4 rather than reusing a lower
+     * code: 0 to 3 mean the same thing across the Maven 4 tools, and a gate must be able to tell
+     * "this POM has warnings" from "mvnval broke" or "somebody hit Ctrl+C".
+     */
+    public static final int WARNINGS = 4;
+
+    public ValidateInvoker(Lookup protoLookup, @Nullable Consumer<LookupContext> contextConsumer) {
+        super(protoLookup, contextConsumer);
+    }
+
+    @Override
+    protected ValidateContext createContext(InvokerRequest invokerRequest) {
+        return new ValidateContext(
+                invokerRequest, (ValidateOptions) invokerRequest.options().orElse(null));
+    }
+
+    @Override
+    protected int execute(ValidateContext context) throws Exception {
+        Thread validationThread = Thread.currentThread();
+        AtomicBoolean cancellationRequested = new AtomicBoolean();
+        Terminal.SignalHandler previousHandler = context.terminal.handle(Terminal.Signal.INT, signal -> {
+            cancellationRequested.set(true);
+            validationThread.interrupt();
+        });
+        try {
+            OutputFormat format;
+            ValidationMode mode;
+            try {
+                // Checked here rather than while parsing: the parser knows the option names,
+                // not the values, and a value it did accept can still become anything once
+                // options are interpolated.
+                format = context.options().format().map(OutputFormat::parse).orElse(OutputFormat.TEXT);
+                mode = context.options().mode().map(ValidationMode::parse).orElse(ValidationMode.EFFECTIVE);
+            } catch (IllegalArgumentException e) {
+                context.logger.error(e.getMessage());
+                return BAD_OPERATION;
+            }
+            // -s/-ps/-is redirect the settings files that ApiRunner reads at session-creation time.
+            // Honouring them would require threading the alternate path into ApiRunner before it
+            // starts; that is a larger change than belongs here, so they are still refused.
+            // -o is different: it applies to the resolver session, which createSession() returns,
+            // and can be applied there after the session is built.  Raw mode resolves nothing, so
+            // none of these apply there.
+            if (mode != ValidationMode.RAW) {
+                String refused = refusedSettingsOption(context.options());
+                if (refused != null) {
+                    context.logger.error(refused
+                            + " cannot redirect the settings file mvnval reads."
+                            + " Use the default settings, or copy the relevant settings to the default location.");
+                    return BAD_OPERATION;
+                }
+            }
+
+            if (context.options().localRepository().isPresent()
+                    && context.options().tempLocalRepository().orElse(false)) {
+                context.logger.error(
+                        "--local-repository and --temp-local-repository name different directories; give one.");
+                return BAD_OPERATION;
+            }
+            String unusableRepository = unusableRepository(context);
+            if (unusableRepository != null) {
+                context.logger.error(unusableRepository);
+                return BAD_OPERATION;
+            }
+            Path localRepository = null;
+            // Only in effective mode: raw resolves nothing, so making a directory there could
+            // lose a run that would never have written to it.
+            if (mode != ValidationMode.RAW) {
+                try {
+                    localRepository = localRepository(context);
+                } catch (IOException e) {
+                    // Not ERROR: no POM was rejected, the run could not be set up.
+                    context.logger.error("Could not create a temporary local repository: " + e);
+                    return BAD_OPERATION;
+                }
+            }
+            Thread cleanup = temporary(context, localRepository) ? cleanupHook(localRepository, context) : null;
+            try {
+                List<Report> reports =
+                        validateAll(context, createSession(localRepository), mode, cancellationRequested);
+                if (cancellationRequested.get() || Thread.currentThread().isInterrupted()) {
+                    return canceled(context);
+                }
+                // determineWriter, not context.writer: on a real run that field is still empty
+                // and this is what fills it.
+                int contextLines = context.options().context().orElse(2);
+                format.report(
+                        reports,
+                        context.cwd.get(),
+                        determineWriter(context),
+                        contextLines,
+                        MessageUtils.isColorEnabled());
+                return exitCode(reports);
+            } finally {
+                if (cleanup != null) {
+                    // Delete first, deregister second. The other order leaves a window where a
+                    // signal arriving after the hook is gone kills the process before the
+                    // deletion runs.
+                    deleteRecursively(localRepository, context);
+                    removeHook(cleanup);
+                }
+            }
+        } catch (UnsupportedOperationException e) {
+            // The ModelBuilder in this container has not written validate. Nothing was wrong with
+            // any POM, so this must not be ERROR, the code a gate reads as "a POM was rejected".
+            context.logger.error("This Maven installation cannot validate POMs: " + e.getMessage());
+            return BAD_OPERATION;
+        } catch (UserInterruptException e) {
+            return canceled(context);
+        } catch (Exception e) {
+            if (cancellationRequested.get() || Thread.currentThread().isInterrupted()) {
+                return canceled(context);
+            }
+            // Without this an unexpected failure escapes to the CLI and becomes 2, which this tool
+            // documents as bad usage. Both siblings catch here too.
+            if (context.options().showErrors().orElse(false)) {
+                context.logger.error(e.getMessage(), e);
+            } else {
+                context.logger.error(e.getMessage());
+            }
+            return ERROR;
+        } finally {
+            context.terminal.handle(Terminal.Signal.INT, previousHandler);
+            if (cancellationRequested.get()) {
+                Thread.interrupted();
+            }
+        }
+    }
+
+    private static int canceled(ValidateContext context) {
+        Thread.interrupted();
+        context.logger.error("Validation canceled by user.");
+        return CANCELED;
+    }
+
+    /**
+     * Turns an unparseable command line into {@link #BAD_OPERATION}.
+     * <p>
+     * The base exits 1 for a bad argument, which is the code this tool uses for a POM it rejected.
+     * A gate has to be able to tell a typo in its own script from a POM that failed validation.
+     */
+    @Override
+    protected void validate(ValidateContext context) throws Exception {
+        try {
+            super.validate(context);
+        } catch (InvokerException.ExitException e) {
+            throw e.getExitCode() == ERROR ? new InvokerException.ExitException(BAD_OPERATION) : e;
+        }
+    }
+
+    /** Whether stdout carries a document this run must not write anything else to. */
+    private static boolean json(ValidateContext context) {
+        return context.options()
+                .format()
+                .filter(OutputFormat.JSON.name()::equalsIgnoreCase)
+                .isPresent();
+    }
+
+    /**
+     * Keeps the version banner out of the document under {@code --format json}.
+     * <p>
+     * The base prints it whenever {@code effectiveVerbose()} holds, which a CI runner setting
+     * {@code RUNNER_DEBUG=1} makes true, so re-running a job with debug logging turned the
+     * document into something no parser accepts. Asking for {@code -V} still prints it, since
+     * that is what the flag is for.
+     */
+    @Override
+    protected void preCommands(ValidateContext context) throws Exception {
+        if (json(context) && !context.options().showVersion().orElse(false)) {
+            return;
+        }
+        super.preCommands(context);
+    }
+
+    /**
+     * Suppresses resolver noise from the output unless the caller asked for it.
+     * <p>
+     * The resolver writes an {@code [INFO]} line the first time it reads a repository's prefix
+     * file. Under {@code --format json} that line breaks the document; in text mode it clutters
+     * output that is meant to list only POM problems. Neither case benefits from seeing it.
+     * The level is forced to {@code ERROR} — the same effect as {@code -q} — unless {@code -X}
+     * or {@code -e} is given, in which case the full log is what the caller wants.
+     * <p>
+     * The property must be set <em>before</em> calling {@link Slf4jConfiguration#setRootLoggerLevel}
+     * because that method itself logs at {@code INFO} when it overrides a value already set (e.g.
+     * a CI runner running with {@code DEBUG}), which would be the very noise we are suppressing.
+     */
+    @Override
+    protected void configureLogging(ValidateContext context) throws Exception {
+        super.configureLogging(context);
+        if (!context.options().verbose().orElse(false)
+                && !context.options().showErrors().orElse(false)) {
+            context.loggerLevel = Slf4jConfiguration.Level.ERROR;
+            System.setProperty(Constants.MAVEN_LOGGER_DEFAULT_LOG_LEVEL, "error");
+            context.slf4jConfiguration.setRootLoggerLevel(context.loggerLevel);
+        }
+    }
+
+    // The next five steps stand up dependency injection and read configuration. Nothing here uses
+    // either: the model builder comes from createSession(). Running them would let the directory
+    // holding the POM decide what this process loads, via .mvn/extensions.xml, .mvn/settings.xml,
+    // or maven.ext.class.path in .mvn/maven-user.properties. Refused one at a time rather than by
+    // replacing doInvoke, so a step added to the base class later still runs.
+
+    /** No container: nothing is looked up, and no extension or contributed property is loaded. */
+    @Override
+    protected void container(ValidateContext context) {}
+
+    /** No container, so no {@code PropertyContributor} to run. */
+    @Override
+    protected void postContainer(ValidateContext context) {}
+
+    /** No container to look up from. */
+    @Override
+    protected void lookup(ValidateContext context) {}
+
+    /** No {@code EventSpy} dispatch: this tool emits no build events. */
+    @Override
+    protected void init(ValidateContext context) {}
+
+    /**
+     * No settings step: the model builder runs on the session from {@link #createSession(Path)},
+     * which reads the user's settings itself when it has to resolve.
+     */
+    @Override
+    protected void settings(ValidateContext context) {}
+
+    /**
+     * Creates the session the model builder runs on. Made here because {@code LookupInvoker} only
+     * ever builds a {@code ProtoSession} while {@link ModelBuilderRequest} needs a full
+     * {@link Session}. Left {@code protected} as a seam: building one is the expensive part of
+     * every test in this package, and the tests substitute a shared one.
+     *
+     * @return the session, never {@code null}
+     */
+    protected Session createSession(@Nullable Path localRepository) {
+        Session session = ApiRunner.createSession(injector -> injector.bindImplicit(TransporterFactoryConfig.class));
+        if (localRepository != null) {
+            // Set on the session that came back, not passed to createSession, which prefers
+            // settings.getLocalRepository() over its argument and may in any case hand back a
+            // session another tool built. See derivedRepositories for why that happens.
+            session = session.withLocalRepository(session.createLocalRepository(localRepository));
+        }
+        return session.withRemoteRepositories(derivedRepositories(session));
+    }
+
+    /**
+     * Applies {@code --offline} to a session built by {@link #createSession}.
+     * <p>
+     * {@code ApiRunner.createSession} applies offline from the settings file.  This supplements
+     * that: when the caller passes {@code -o} on the command line, the session is made offline
+     * regardless of what the settings say, by cloning the underlying resolver session with offline
+     * set to true.
+     */
+    private static Session withOffline(Session session) {
+        // withLocalRepository(same repo) forces AbstractSession to clone the underlying resolver
+        // session (DefaultRepositorySystemSession) into a fresh one held only by the new session
+        // object.  That fresh clone is safe to mutate without affecting the caller's session.
+        Session copy = session.withLocalRepository(session.getLocalRepository());
+        DefaultRepositorySystemSession rsession =
+                (DefaultRepositorySystemSession) InternalSession.from(copy).getSession();
+        rsession.setOffline(true);
+        return copy;
+    }

Review Comment:
   ⚠️ **`withOffline` — clone never happens; the Javadoc is wrong**
   
   `session.withLocalRepository(session.getLocalRepository())` calls `AbstractSession.withLocalRepository()`, which short-circuits at:
   ```java
   if (session.getLocalRepository() != null
           && Objects.equals(session.getLocalRepository().getBasePath(), localRepository.path())) {
       return this;  // same path → no clone
   }
   ```
   Since both sides resolve to the same base path, `copy == session`. `InternalSession.from(copy).getSession()` returns the original `DefaultRepositorySystemSession`, and `rsession.setOffline(true)` mutates it in-place. The comment claiming "That fresh clone is safe to mutate without affecting the caller's session" is false: there is no clone.
   
   This works today only because `validateAll` immediately reassigns `session = withOffline(session)` and never touches the original reference again. Any future refactor that passes the same session to two independent code paths will silently find the second path offline.
   
   Fix — drop the dead clone attempt and document the actual invariant:
   ```suggestion
       private static Session withOffline(Session session) {
           // AbstractSession.withLocalRepository short-circuits when the path is unchanged
           // (returns this). Mutate the underlying DefaultRepositorySystemSession in-place.
           // Safe only because validateAll reassigns session immediately and does not retain
           // the pre-mutation reference.
           DefaultRepositorySystemSession rsession =
                   (DefaultRepositorySystemSession) InternalSession.from(session).getSession();
           rsession.setOffline(true);
           return session;
       }
   ```



##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnval/ValidateInvoker.java:
##########
@@ -0,0 +1,736 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.cling.invoker.mvnval;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.Comparator;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.function.Consumer;
+import java.util.stream.Stream;
+
+import org.apache.maven.api.Constants;
+import org.apache.maven.api.RemoteRepository;
+import org.apache.maven.api.Session;
+import org.apache.maven.api.annotations.Nullable;
+import org.apache.maven.api.cli.InvokerException;
+import org.apache.maven.api.cli.InvokerRequest;
+import org.apache.maven.api.cli.mvnval.ValidateOptions;
+import org.apache.maven.api.di.Named;
+import org.apache.maven.api.di.Provides;
+import org.apache.maven.api.model.Model;
+import org.apache.maven.api.model.Profile;
+import org.apache.maven.api.model.Repository;
+import org.apache.maven.api.model.RepositoryPolicy;
+import org.apache.maven.api.services.Lookup;
+import org.apache.maven.api.services.ModelBuilder;
+import org.apache.maven.api.services.ModelBuilder.ModelBuilderSession;
+import org.apache.maven.api.services.ModelBuilderException;
+import org.apache.maven.api.services.ModelBuilderRequest;
+import org.apache.maven.api.services.ModelBuilderRequest.RequestType;
+import org.apache.maven.api.services.ModelBuilderResult;
+import org.apache.maven.api.services.RepositoryFactory;
+import org.apache.maven.api.services.SettingsBuilder;
+import org.apache.maven.api.services.Sources;
+import org.apache.maven.cling.invoker.LookupContext;
+import org.apache.maven.cling.invoker.LookupInvoker;
+import org.apache.maven.cling.logging.Slf4jConfiguration;
+import org.apache.maven.impl.InternalSession;
+import org.apache.maven.impl.standalone.ApiRunner;
+import org.apache.maven.jline.MessageUtils;
+import org.eclipse.aether.DefaultRepositorySystemSession;
+import org.eclipse.aether.spi.connector.transport.TransporterFactory;
+import org.eclipse.aether.spi.connector.transport.http.ChecksumExtractor;
+import org.eclipse.aether.spi.io.PathProcessor;
+import org.eclipse.aether.transport.apache.ApacheTransporterFactory;
+import org.eclipse.aether.transport.file.FileTransporterFactory;
+import org.jline.reader.UserInterruptException;
+import org.jline.terminal.Terminal;
+
+/**
+ * Validates POM files without building them.
+ * <p>
+ * Two modes, and one contains the other. {@code raw} stops at
+ * {@link ModelBuilder.ModelBuilderSession#validate}, reaching no repository at all: it works
+ * offline and on a POM whose parent is not published yet, but it cannot see a problem that
+ * inheritance introduces, such as a dependency whose version comes from the parent's
+ * {@code dependencyManagement}. {@code effective}, the default, runs that pass and then builds
+ * the effective model as well, so parents and imported boms are resolved and the checks needing
+ * them run too. Both passes, because the effective build says nothing about the reactor around a
+ * POM: on its own it passes a project whose declared subproject is not on disk, which
+ * {@code mvn} refuses to read.
+ * <p>
+ * Nothing is ever written back to a POM. In {@code effective} mode what is resolved lands in the
+ * local repository, as it does for every Maven tool. {@code --local-repository} aims that
+ * somewhere else and {@code --temp-local-repository} at a directory made for the run and deleted
+ * when it ends, for a gate that wants the configured repository left as it was.
+ * <p>
+ * It stands up no container, so the directory holding a POM cannot make this process load
+ * anything through {@code .mvn/extensions.xml}, {@code maven.ext.class.path} or
+ * {@code .mvn/settings.xml}. The user's own settings are another matter: resolution reads them
+ * for mirrors, proxies and credentials, as it has to. The JVM is outside this:
+ * {@code bin/mvn} passes {@code .mvn/jvm.config} to the launcher before this process starts.
+ * <p>
+ * {@code -o} works in {@code effective} mode: resolution hits the local repository only and
+ * fails fast when a parent or BOM is absent. Combined with {@code --temp-local-repository} it
+ * proves that the POMs being validated are fully self-contained.
+ * <p>
+ * When more than one POM is given, {@code effective} mode pre-scans them all to build a bundle
+ * workspace: a parent that is named in the bundle is resolved from disk rather than from a
+ * repository, so the verdict does not depend on publication order and the run does not download
+ * what is already on disk.
+ */
+public class ValidateInvoker extends LookupInvoker<ValidateContext> {
+
+    /** Where Central lives, for the one case where no settings file names a repository. */
+    private static final String CENTRAL_URL = "https://repo.maven.apache.org/maven2";
+
+    public static final int OK = 0;
+    public static final int ERROR = 1;
+
+    /** Bad user input. Same meaning as in {@code mvnenc} and {@code mvnup}. */
+    public static final int BAD_OPERATION = 2;
+
+    /**
+     * Interrupted. Same meaning as in {@code mvnenc} and {@code mvnup}, and reachable only with a
+     * terminal attached: without one the signal reaches the JVM instead and the exit code is its.
+     * Left out of {@code --help} for that reason.
+     */
+    public static final int CANCELED = 3;
+
+    /**
+     * Nothing was rejected, but something was reported. Numbered 4 rather than reusing a lower
+     * code: 0 to 3 mean the same thing across the Maven 4 tools, and a gate must be able to tell
+     * "this POM has warnings" from "mvnval broke" or "somebody hit Ctrl+C".
+     */
+    public static final int WARNINGS = 4;
+
+    public ValidateInvoker(Lookup protoLookup, @Nullable Consumer<LookupContext> contextConsumer) {
+        super(protoLookup, contextConsumer);
+    }
+
+    @Override
+    protected ValidateContext createContext(InvokerRequest invokerRequest) {
+        return new ValidateContext(
+                invokerRequest, (ValidateOptions) invokerRequest.options().orElse(null));
+    }
+
+    @Override
+    protected int execute(ValidateContext context) throws Exception {
+        Thread validationThread = Thread.currentThread();
+        AtomicBoolean cancellationRequested = new AtomicBoolean();
+        Terminal.SignalHandler previousHandler = context.terminal.handle(Terminal.Signal.INT, signal -> {
+            cancellationRequested.set(true);
+            validationThread.interrupt();
+        });
+        try {
+            OutputFormat format;
+            ValidationMode mode;
+            try {
+                // Checked here rather than while parsing: the parser knows the option names,
+                // not the values, and a value it did accept can still become anything once
+                // options are interpolated.
+                format = context.options().format().map(OutputFormat::parse).orElse(OutputFormat.TEXT);
+                mode = context.options().mode().map(ValidationMode::parse).orElse(ValidationMode.EFFECTIVE);
+            } catch (IllegalArgumentException e) {
+                context.logger.error(e.getMessage());
+                return BAD_OPERATION;
+            }
+            // -s/-ps/-is redirect the settings files that ApiRunner reads at session-creation time.
+            // Honouring them would require threading the alternate path into ApiRunner before it
+            // starts; that is a larger change than belongs here, so they are still refused.
+            // -o is different: it applies to the resolver session, which createSession() returns,
+            // and can be applied there after the session is built.  Raw mode resolves nothing, so
+            // none of these apply there.
+            if (mode != ValidationMode.RAW) {
+                String refused = refusedSettingsOption(context.options());
+                if (refused != null) {
+                    context.logger.error(refused
+                            + " cannot redirect the settings file mvnval reads."
+                            + " Use the default settings, or copy the relevant settings to the default location.");
+                    return BAD_OPERATION;
+                }
+            }
+
+            if (context.options().localRepository().isPresent()
+                    && context.options().tempLocalRepository().orElse(false)) {
+                context.logger.error(
+                        "--local-repository and --temp-local-repository name different directories; give one.");
+                return BAD_OPERATION;
+            }
+            String unusableRepository = unusableRepository(context);
+            if (unusableRepository != null) {
+                context.logger.error(unusableRepository);
+                return BAD_OPERATION;
+            }
+            Path localRepository = null;
+            // Only in effective mode: raw resolves nothing, so making a directory there could
+            // lose a run that would never have written to it.
+            if (mode != ValidationMode.RAW) {
+                try {
+                    localRepository = localRepository(context);
+                } catch (IOException e) {
+                    // Not ERROR: no POM was rejected, the run could not be set up.
+                    context.logger.error("Could not create a temporary local repository: " + e);
+                    return BAD_OPERATION;
+                }
+            }
+            Thread cleanup = temporary(context, localRepository) ? cleanupHook(localRepository, context) : null;
+            try {
+                List<Report> reports =
+                        validateAll(context, createSession(localRepository), mode, cancellationRequested);
+                if (cancellationRequested.get() || Thread.currentThread().isInterrupted()) {
+                    return canceled(context);
+                }
+                // determineWriter, not context.writer: on a real run that field is still empty
+                // and this is what fills it.
+                int contextLines = context.options().context().orElse(2);
+                format.report(
+                        reports,
+                        context.cwd.get(),
+                        determineWriter(context),
+                        contextLines,
+                        MessageUtils.isColorEnabled());
+                return exitCode(reports);
+            } finally {
+                if (cleanup != null) {
+                    // Delete first, deregister second. The other order leaves a window where a
+                    // signal arriving after the hook is gone kills the process before the
+                    // deletion runs.
+                    deleteRecursively(localRepository, context);
+                    removeHook(cleanup);
+                }
+            }
+        } catch (UnsupportedOperationException e) {
+            // The ModelBuilder in this container has not written validate. Nothing was wrong with
+            // any POM, so this must not be ERROR, the code a gate reads as "a POM was rejected".
+            context.logger.error("This Maven installation cannot validate POMs: " + e.getMessage());
+            return BAD_OPERATION;
+        } catch (UserInterruptException e) {
+            return canceled(context);
+        } catch (Exception e) {
+            if (cancellationRequested.get() || Thread.currentThread().isInterrupted()) {
+                return canceled(context);
+            }
+            // Without this an unexpected failure escapes to the CLI and becomes 2, which this tool
+            // documents as bad usage. Both siblings catch here too.
+            if (context.options().showErrors().orElse(false)) {
+                context.logger.error(e.getMessage(), e);
+            } else {
+                context.logger.error(e.getMessage());
+            }
+            return ERROR;
+        } finally {
+            context.terminal.handle(Terminal.Signal.INT, previousHandler);
+            if (cancellationRequested.get()) {
+                Thread.interrupted();
+            }
+        }
+    }
+
+    private static int canceled(ValidateContext context) {
+        Thread.interrupted();
+        context.logger.error("Validation canceled by user.");
+        return CANCELED;
+    }
+
+    /**
+     * Turns an unparseable command line into {@link #BAD_OPERATION}.
+     * <p>
+     * The base exits 1 for a bad argument, which is the code this tool uses for a POM it rejected.
+     * A gate has to be able to tell a typo in its own script from a POM that failed validation.
+     */
+    @Override
+    protected void validate(ValidateContext context) throws Exception {
+        try {
+            super.validate(context);
+        } catch (InvokerException.ExitException e) {
+            throw e.getExitCode() == ERROR ? new InvokerException.ExitException(BAD_OPERATION) : e;
+        }
+    }
+
+    /** Whether stdout carries a document this run must not write anything else to. */
+    private static boolean json(ValidateContext context) {
+        return context.options()
+                .format()
+                .filter(OutputFormat.JSON.name()::equalsIgnoreCase)
+                .isPresent();
+    }
+
+    /**
+     * Keeps the version banner out of the document under {@code --format json}.
+     * <p>
+     * The base prints it whenever {@code effectiveVerbose()} holds, which a CI runner setting
+     * {@code RUNNER_DEBUG=1} makes true, so re-running a job with debug logging turned the
+     * document into something no parser accepts. Asking for {@code -V} still prints it, since
+     * that is what the flag is for.
+     */
+    @Override
+    protected void preCommands(ValidateContext context) throws Exception {
+        if (json(context) && !context.options().showVersion().orElse(false)) {
+            return;
+        }
+        super.preCommands(context);
+    }
+
+    /**
+     * Suppresses resolver noise from the output unless the caller asked for it.
+     * <p>
+     * The resolver writes an {@code [INFO]} line the first time it reads a repository's prefix
+     * file. Under {@code --format json} that line breaks the document; in text mode it clutters
+     * output that is meant to list only POM problems. Neither case benefits from seeing it.
+     * The level is forced to {@code ERROR} — the same effect as {@code -q} — unless {@code -X}
+     * or {@code -e} is given, in which case the full log is what the caller wants.
+     * <p>
+     * The property must be set <em>before</em> calling {@link Slf4jConfiguration#setRootLoggerLevel}
+     * because that method itself logs at {@code INFO} when it overrides a value already set (e.g.
+     * a CI runner running with {@code DEBUG}), which would be the very noise we are suppressing.
+     */
+    @Override
+    protected void configureLogging(ValidateContext context) throws Exception {
+        super.configureLogging(context);
+        if (!context.options().verbose().orElse(false)
+                && !context.options().showErrors().orElse(false)) {
+            context.loggerLevel = Slf4jConfiguration.Level.ERROR;
+            System.setProperty(Constants.MAVEN_LOGGER_DEFAULT_LOG_LEVEL, "error");
+            context.slf4jConfiguration.setRootLoggerLevel(context.loggerLevel);
+        }
+    }
+
+    // The next five steps stand up dependency injection and read configuration. Nothing here uses
+    // either: the model builder comes from createSession(). Running them would let the directory
+    // holding the POM decide what this process loads, via .mvn/extensions.xml, .mvn/settings.xml,
+    // or maven.ext.class.path in .mvn/maven-user.properties. Refused one at a time rather than by
+    // replacing doInvoke, so a step added to the base class later still runs.
+
+    /** No container: nothing is looked up, and no extension or contributed property is loaded. */
+    @Override
+    protected void container(ValidateContext context) {}
+
+    /** No container, so no {@code PropertyContributor} to run. */
+    @Override
+    protected void postContainer(ValidateContext context) {}
+
+    /** No container to look up from. */
+    @Override
+    protected void lookup(ValidateContext context) {}
+
+    /** No {@code EventSpy} dispatch: this tool emits no build events. */
+    @Override
+    protected void init(ValidateContext context) {}
+
+    /**
+     * No settings step: the model builder runs on the session from {@link #createSession(Path)},
+     * which reads the user's settings itself when it has to resolve.
+     */
+    @Override
+    protected void settings(ValidateContext context) {}
+
+    /**
+     * Creates the session the model builder runs on. Made here because {@code LookupInvoker} only
+     * ever builds a {@code ProtoSession} while {@link ModelBuilderRequest} needs a full
+     * {@link Session}. Left {@code protected} as a seam: building one is the expensive part of
+     * every test in this package, and the tests substitute a shared one.
+     *
+     * @return the session, never {@code null}
+     */
+    protected Session createSession(@Nullable Path localRepository) {
+        Session session = ApiRunner.createSession(injector -> injector.bindImplicit(TransporterFactoryConfig.class));
+        if (localRepository != null) {
+            // Set on the session that came back, not passed to createSession, which prefers
+            // settings.getLocalRepository() over its argument and may in any case hand back a
+            // session another tool built. See derivedRepositories for why that happens.
+            session = session.withLocalRepository(session.createLocalRepository(localRepository));
+        }
+        return session.withRemoteRepositories(derivedRepositories(session));
+    }
+
+    /**
+     * Applies {@code --offline} to a session built by {@link #createSession}.
+     * <p>
+     * {@code ApiRunner.createSession} applies offline from the settings file.  This supplements
+     * that: when the caller passes {@code -o} on the command line, the session is made offline
+     * regardless of what the settings say, by cloning the underlying resolver session with offline
+     * set to true.
+     */
+    private static Session withOffline(Session session) {
+        // withLocalRepository(same repo) forces AbstractSession to clone the underlying resolver
+        // session (DefaultRepositorySystemSession) into a fresh one held only by the new session
+        // object.  That fresh clone is safe to mutate without affecting the caller's session.
+        Session copy = session.withLocalRepository(session.getLocalRepository());
+        DefaultRepositorySystemSession rsession =
+                (DefaultRepositorySystemSession) InternalSession.from(copy).getSession();
+        rsession.setOffline(true);
+        return copy;
+    }
+
+    /**
+     * Installs a {@link BundleWorkspaceReader} on the resolver session so that parent and BOM
+     * lookups for POMs in the bundle are answered from disk rather than from a repository.
+     * <p>
+     * The workspace reader is set on a clone of the resolver session: it does not mutate the
+     * original session, which the tests may reuse across runs.
+     */
+    private static Session withWorkspaceReader(Session session, BundleWorkspaceReader reader) {
+        Session copy = session.withLocalRepository(session.getLocalRepository());
+        DefaultRepositorySystemSession rsession =
+                (DefaultRepositorySystemSession) InternalSession.from(copy).getSession();
+        rsession.setWorkspaceReader(reader);
+        return copy;
+    }

Review Comment:
   ⚠️ **`withWorkspaceReader` — same broken-clone pattern, stronger safety claim**
   
   Same root cause as `withOffline`: `withLocalRepository(session.getLocalRepository())` returns `this`, so the workspace reader is installed on the original session's `DefaultRepositorySystemSession`. The Javadoc says _"it does not mutate the original session, which the tests may reuse across runs"_ — but it does.
   
   The `@BeforeEach` in `ValidateInvokerTest` creates a fresh session per test, so the test-isolation concern is mitigated today, but the Javadoc guarantee is false. Fix consistently with `withOffline`:
   ```suggestion
       private static Session withWorkspaceReader(Session session, BundleWorkspaceReader reader) {
           // AbstractSession.withLocalRepository short-circuits when the path is unchanged
           // (returns this). Mutate the underlying DefaultRepositorySystemSession in-place.
           // Safe only because validateAll reassigns session immediately and does not retain
           // the pre-mutation reference.
           DefaultRepositorySystemSession rsession =
                   (DefaultRepositorySystemSession) InternalSession.from(session).getSession();
           rsession.setWorkspaceReader(reader);
           return session;
       }
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MNG-8650] mvnval, a tool to validate POMs without building them [maven]

["gnodet (via GitHub)" <gi...@apache.org>]

gnodet commented on PR #13276:
URL: https://github.com/apache/maven/pull/13276#issuecomment-5865425772

   Thanks for this — the use case is clearly stated and the implementation is clean. I have a few concerns about the API addition before this goes in.
   
   ## `buildRaw` encodes a limitation that belongs to the caller, not the API
   
   The new `ModelBuilderSession.buildRaw()` is essentially "run the model builder pipeline but collect problems instead of throwing on the first validation error". That's a useful and legitimate contract. But the name encodes *how far the pipeline runs* (raw = file + raw model phase only) as if that were fixed by the method, when it's already controlled by `RequestType` on the `ModelBuilderRequest`.
   
   I'd suggest renaming it to `validate`:
   
   ```java
   /**
    * Validates the model described by the request, collecting all problems
    * without throwing on validation errors. The request's {@link ModelBuilderRequest.RequestType}
    * controls how far the pipeline runs. Only throws when the model cannot be read at all.
    *
    * @since 4.1.0
    */
   default ModelBuilderResult validate(ModelBuilderRequest request) throws ModelBuilderException {
       ...
   }
   ```
   
   Same implementation for now — the raw case stays as-is. But the name doesn't bake in the limitation, and we don't need a second API addition later when someone wants effective-model validation.
   
   ## Effective-model validation is already achievable — without any new API
   
   The PR description notes that validation stops at the raw model, and that a dependency whose version comes from the parent's `dependencyManagement` is only caught in `validateEffectiveModel`. That's true today, but it doesn't require any new API to fix. The `ModelResolver` SPI is the customization point: an implementation that resolves parents (from the batch being validated, or from Central into a temporary local repository) lets the model builder do full effective-model validation without touching the caller's local repo. The use case maps directly onto the `ApiRunner` customizer:
   
   ```java
   ApiRunner.builder()
       .customizer(container -> container.addBean(ModelResolver.class,
           new CentralBackedResolver(tempLocalRepo)))
       .build()
       .run(session -> session.getService(ModelBuilder.class)
           .createSession()
           .validate(requestWithBuildProjectType));
   ```
   
   `mappedSources` inside `DefaultModelBuilder` already does the reactor equivalent of this. A `validate()` method that honours `RequestType` gives callers a clean upgrade path from "raw only" to "effective model from batch" without any further API changes.
   
   ## The `clearRequestScopedCache` in `buildRaw` is a code smell
   
   The `finally` block in `buildRaw` explicitly clears the request-scoped cache because the method knows it shouldn't share session state with a concurrent `build()` call on the same `ModelBuilderSession`. But the right answer isn't special-casing the cleanup: it's using a fresh `ModelBuilderSession` per validation job, which is what `createSession()` is for. A `validate()` method that documents "use a dedicated session" doesn't need this workaround at all — each `createSession()` + `validate()` is naturally isolated and the cache lifecycle is just the session lifecycle.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MNG-8650] mvnval, a tool to validate POMs without building them [maven]

["rmannibucau (via GitHub)" <gi...@apache.org>]

rmannibucau commented on code in PR #13276:
URL: https://github.com/apache/maven/pull/13276#discussion_r4119989548


##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnval/JsonReporter.java:
##########
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.cling.invoker.mvnval;
+
+import java.util.List;
+import java.util.function.Consumer;
+import java.util.stream.Collectors;
+
+import org.apache.maven.api.annotations.Nullable;
+import org.apache.maven.api.services.ModelProblem;
+
+/**
+ * Renders validation results as a JSON array, one object per validated file. Assembled by hand
+ * so the tool needs no dependency the distribution does not already carry.
+ */
+class JsonReporter implements Reporter {
+
+    private static final char[] HEX = "0123456789abcdef".toCharArray();
+
+    private static final char FIRST_PRINTABLE_ASCII = 0x20;
+    private static final char LAST_PRINTABLE_ASCII = 0x7e;
+
+    @Override
+    public void report(List<Report> reports, Consumer<String> out) {
+        out.accept(reports.stream().map(JsonReporter::toJson).collect(Collectors.joining(",", "[", "]")));
+    }
+
+    private static String toJson(Report report) {
+        StringBuilder sb =
+                new StringBuilder("{\"pom\":").append(quote(report.pom().toString()));
+        if (report.failure() != null) {
+            sb.append(",\"failure\":").append(quote(report.failure()));
+        }
+        return sb.append(",\"problems\":")
+                .append(report.problems().stream()
+                        .map(problem -> toJson(problem, report))
+                        .collect(Collectors.joining(",", "[", "]")))
+                .append('}')
+                .toString();
+    }
+
+    private static String toJson(ModelProblem problem, Report report) {
+        StringBuilder sb = new StringBuilder("{\"severity\":")
+                .append(quote(problem.getSeverity().name()))
+                .append(",\"message\":")
+                .append(quote(problem.getMessage()));
+        // Absent values are left out, not written as the -1 the model builder uses for "no location".
+        String source = report.sourceOf(problem);
+        if (source != null) {
+            sb.append(",\"source\":").append(quote(source));
+        }
+        if (problem.getLineNumber() > 0) {
+            sb.append(",\"line\":").append(problem.getLineNumber());
+        }
+        if (problem.getColumnNumber() > 0) {
+            sb.append(",\"column\":").append(problem.getColumnNumber());
+        }
+        return sb.append('}').toString();
+    }
+
+    private static String quote(@Nullable String value) {

Review Comment:
   tip: you can lazy instantite the stringbuilder so you can return value when not needing any encoding, see https://github.com/apache/tomcat/blob/main/java/org/apache/juli/JsonFormatter.java#L147 for an example



##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnval/JsonReporter.java:
##########
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.cling.invoker.mvnval;
+
+import java.util.List;
+import java.util.function.Consumer;
+import java.util.stream.Collectors;
+
+import org.apache.maven.api.annotations.Nullable;
+import org.apache.maven.api.services.ModelProblem;
+
+/**
+ * Renders validation results as a JSON array, one object per validated file. Assembled by hand
+ * so the tool needs no dependency the distribution does not already carry.
+ */
+class JsonReporter implements Reporter {
+
+    private static final char[] HEX = "0123456789abcdef".toCharArray();
+
+    private static final char FIRST_PRINTABLE_ASCII = 0x20;
+    private static final char LAST_PRINTABLE_ASCII = 0x7e;
+
+    @Override
+    public void report(List<Report> reports, Consumer<String> out) {
+        out.accept(reports.stream().map(JsonReporter::toJson).collect(Collectors.joining(",", "[", "]")));
+    }
+
+    private static String toJson(Report report) {
+        StringBuilder sb =
+                new StringBuilder("{\"pom\":").append(quote(report.pom().toString()));
+        if (report.failure() != null) {
+            sb.append(",\"failure\":").append(quote(report.failure()));
+        }
+        return sb.append(",\"problems\":")
+                .append(report.problems().stream()
+                        .map(problem -> toJson(problem, report))
+                        .collect(Collectors.joining(",", "[", "]")))
+                .append('}')
+                .toString();
+    }
+
+    private static String toJson(ModelProblem problem, Report report) {
+        StringBuilder sb = new StringBuilder("{\"severity\":")
+                .append(quote(problem.getSeverity().name()))
+                .append(",\"message\":")
+                .append(quote(problem.getMessage()));
+        // Absent values are left out, not written as the -1 the model builder uses for "no location".
+        String source = report.sourceOf(problem);
+        if (source != null) {
+            sb.append(",\"source\":").append(quote(source));
+        }
+        if (problem.getLineNumber() > 0) {
+            sb.append(",\"line\":").append(problem.getLineNumber());
+        }
+        if (problem.getColumnNumber() > 0) {
+            sb.append(",\"column\":").append(problem.getColumnNumber());
+        }
+        return sb.append('}').toString();
+    }
+
+    private static String quote(@Nullable String value) {

Review Comment:
   tip: you can lazy instantiate the stringbuilder so you can return value when not needing any encoding, see https://github.com/apache/tomcat/blob/main/java/org/apache/juli/JsonFormatter.java#L147 for an example



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MNG-8650] mvnval, a tool to validate POMs without building them [maven]

["gnodet-bot (via GitHub)" <gi...@apache.org>]

gnodet-bot commented on code in PR #13276:
URL: https://github.com/apache/maven/pull/13276#discussion_r4136286109


##########
impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnval/ValidateInvokerTest.java:
##########
@@ -0,0 +1,789 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.cling.invoker.mvnval;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.Set;
+
+import org.apache.maven.api.Session;
+import org.apache.maven.api.cli.InvokerException;
+import org.apache.maven.api.cli.mvnval.ValidateOptions;
+import org.apache.maven.api.services.ModelBuilder;
+import org.apache.maven.cling.invoker.ProtoLookup;
+import org.apache.maven.impl.standalone.ApiRunner;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Nested;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+/**
+ * Unit tests for the {@link ValidateInvoker} class.
+ * Tests that the verdict depends on the file alone, that a file which cannot be read does not
+ * lose the verdict on the others, and that the exit code follows the reports.
+ */
+@DisplayName("ValidateInvoker")
+class ValidateInvokerTest {
+
+    private static final String DUPLICATE_DEPENDENCY = """
+            <project xmlns="http://maven.apache.org/POM/4.0.0">
+              <modelVersion>4.0.0</modelVersion>
+              %s
+              <groupId>org.test</groupId>
+              <artifactId>test</artifactId>
+              <version>1.0</version>
+              <dependencies>
+                <dependency>
+                  <groupId>junit</groupId><artifactId>junit</artifactId><version>4.13.2</version>
+                </dependency>
+                <dependency>
+                  <groupId>junit</groupId><artifactId>junit</artifactId><version>4.12</version>
+                </dependency>
+              </dependencies>
+            </project>
+            """;
+
+    private static final String UNRESOLVABLE_PARENT = """
+            <parent>
+                <groupId>org.nowhere</groupId>
+                <artifactId>does-not-exist</artifactId>
+                <version>999.0.0</version>
+              </parent>""";
+
+    @TempDir
+    Path tempDir;
+
+    private List<String> output;
+
+    /** Every path handed to the seam, so a test can assert on the one the run actually made. */
+    private List<Path> aimed;
+
+    private ValidateInvoker invoker;
+
+    @BeforeEach
+    void setUp() {
+        output = new ArrayList<>();
+        // One session for the whole test: building one is the expensive part of every test here.
+        Session shared = ApiRunner.createSession();
+        aimed = new ArrayList<>();
+        invoker = new ValidateInvoker(ProtoLookup.builder().build(), null) {
+            @Override
+            protected Session createSession(Path localRepository) {
+                // withLocalRepository is the line production uses, so an aimed repository is
+                // exercised rather than stubbed. The transports and the settings-derived
+                // repository list production adds are not: this session has neither.
+                aimed.add(localRepository);
+                return localRepository == null
+                        ? shared
+                        : shared.withLocalRepository(shared.createLocalRepository(localRepository));
+            }
+        };
+    }
+
+    private Path writePom(String name, String parent) throws Exception {
+        Path pom = Files.createDirectories(tempDir.resolve(name)).resolve("pom.xml");
+        Files.writeString(pom, String.format(DUPLICATE_DEPENDENCY, parent));
+        return pom;
+    }
+
+    private int run(List<String> poms) throws Exception {
+        return run(poms, Optional.empty());
+    }
+
+    private int run(List<String> poms, Optional<String> format) throws Exception {
+        return run(poms, format, Optional.of("raw"));
+    }
+
+    /** Defaults to raw: the tests using this helper read files only. */
+    private int run(List<String> poms, Optional<String> format, Optional<String> mode) throws Exception {
+        ValidateOptions options = mock(ValidateOptions.class);
+        when(options.format()).thenReturn(format);
+        when(options.mode()).thenReturn(mode);
+        when(options.poms()).thenReturn(poms.isEmpty() ? Optional.empty() : Optional.of(poms));
+
+        ValidateContext context = TestUtils.createMockContext(tempDir, options);
+        context.writer = output::add;
+        return invoker.execute(context);
+    }
+
+    @Nested
+    @DisplayName("Wiring")
+    class WiringTests {
+
+        @Test
+        @DisplayName("should register the transports resolution needs")
+        void shouldRegisterTransports() throws Exception {
+            // The block whose absence produces "No transporter factories registered". Proved over
+            // file://, which needs FileTransporterFactory and no network. Every other test here
+            // replaces createSession, so without this the block could be deleted and stay green.
+            Path repository = Files.createDirectories(tempDir.resolve("served/org/test/far/1.0"));
+            String parent = """
+                    <project xmlns="http://maven.apache.org/POM/4.0.0">
+                      <modelVersion>4.0.0</modelVersion>
+                      <groupId>org.test</groupId><artifactId>far</artifactId>
+                      <version>1.0</version><packaging>pom</packaging>
+                    </project>""";
+            Files.writeString(repository.resolve("far-1.0.pom"), parent);
+            // A repository without checksums is refused before the content is looked at, so the
+            // transport would never be exercised.
+            Files.writeString(repository.resolve("far-1.0.pom.sha1"), sha1(parent));
+            Path pom = Files.createDirectories(tempDir.resolve("fetcher")).resolve("pom.xml");
+            Files.writeString(pom, """
+                    <project xmlns="http://maven.apache.org/POM/4.0.0">
+                      <modelVersion>4.0.0</modelVersion>
+                      <parent>
+                        <groupId>org.test</groupId><artifactId>far</artifactId>
+                        <version>1.0</version><relativePath/>
+                      </parent>
+                      <artifactId>fetcher</artifactId>
+                      <repositories>
+                        <repository><id>served</id><url>%s</url></repository>
+                      </repositories>
+                    </project>""".formatted(tempDir.resolve("served").toUri()));
+
+            ValidateOptions options = mock(ValidateOptions.class);
+            when(options.mode()).thenReturn(Optional.of("effective"));
+            when(options.localRepository())
+                    .thenReturn(Optional.of(tempDir.resolve("into").toString()));
+            when(options.poms()).thenReturn(Optional.of(List.of(pom.toString())));
+            ValidateContext context = TestUtils.createMockContext(tempDir, options);
+            context.writer = output::add;
+
+            int exitCode = new ValidateInvoker(ProtoLookup.builder().build(), null).execute(context);
+
+            assertEquals(ValidateInvoker.OK, exitCode, output.toString());
+            assertTrue(
+                    Files.exists(tempDir.resolve("into/org/test/far/1.0/far-1.0.pom")),
+                    "the parent has to arrive through a transport: " + output);
+        }
+
+        @Test
+        @DisplayName("should exit on bad usage when the model builder cannot validate")
+        void shouldRefuseAModelBuilderThatCannotValidate() throws Exception {
+            // validate() is a default that throws, so an out-of-tree ModelBuilder may not have it.
+            // Unreachable from the shipped CLI, which is why it is worth pinning here: nothing was
+            // wrong with the POM, so this must not be the code that means a POM was rejected.
+            ValidateInvoker refusing = new ValidateInvoker(ProtoLookup.builder().build(), null) {
+                @Override
+                protected Session createSession(Path localRepository) {
+                    Session session = mock(Session.class);
+                    ModelBuilder builder = mock(ModelBuilder.class);
+                    ModelBuilder.ModelBuilderSession builderSession = mock(ModelBuilder.ModelBuilderSession.class);
+                    when(session.getService(ModelBuilder.class)).thenReturn(builder);
+                    when(builder.newSession()).thenReturn(builderSession);
+                    when(builderSession.validate(any()))
+                            .thenThrow(new UnsupportedOperationException("does not support validating a model"));
+                    return session;
+                }
+            };
+            ValidateOptions options = mock(ValidateOptions.class);
+            when(options.mode()).thenReturn(Optional.of("raw"));
+            when(options.poms())
+                    .thenReturn(Optional.of(List.of(writePom("unsupported", "").toString())));
+            ValidateContext context = TestUtils.createMockContext(tempDir, options);
+            context.writer = output::add;
+
+            assertEquals(ValidateInvoker.BAD_OPERATION, refusing.execute(context));
+        }
+    }
+
+    private static String sha1(String content) throws Exception {
+        byte[] digest = java.security.MessageDigest.getInstance("SHA-1").digest(content.getBytes(UTF_8));
+        StringBuilder hex = new StringBuilder(digest.length * 2);
+        for (byte b : digest) {
+            hex.append(String.format("%02x", b));
+        }
+        return hex.toString();
+    }
+
+    @Nested
+    @DisplayName("Determinism")
+    class DeterminismTests {
+
+        @Test
+        @DisplayName("should reach the same verdict whether or not the parent can be resolved")
+        void shouldReachSameVerdictRegardlessOfParent() throws Exception {
+            Path withParent = writePom("with-parent", UNRESOLVABLE_PARENT);
+            Path withoutParent = writePom("without-parent", "");
+
+            int exitWith = run(List.of(withParent.toString()));
+            List<String> linesWith = List.copyOf(output);
+            output.clear();
+            int exitWithout = run(List.of(withoutParent.toString()));
+
+            assertEquals(exitWith, exitWithout, "an unresolvable parent must not change the exit code");
+            assertTrue(
+                    linesWith.stream().anyMatch(l -> l.contains("must be unique")),
+                    "the duplicate dependency should be reported even with an unresolvable parent: " + linesWith);
+            assertTrue(
+                    output.stream().anyMatch(l -> l.contains("must be unique")),
+                    "and also without a parent: " + output);
+        }
+
+        @Test
+        @DisplayName("should print a path under the working directory as the caller typed it")
+        void shouldPrintPathsRelativeToTheWorkingDirectory() throws Exception {
+            Path pom = writePom("near", "");
+            Path outside = Files.createDirectories(
+                            Files.createTempDirectory("mvnval-outside-").resolve("far"))
+                    .resolve("pom.xml");
+            Files.writeString(outside, String.format(DUPLICATE_DEPENDENCY, ""));

Review Comment:
   💡 **Minor:** `Files.createTempDirectory("mvnval-outside-")` creates a directory in the system temp dir that is never cleaned up — `@TempDir` only manages `tempDir`. On a CI server running this test repeatedly, these accumulate.
   
   Either nest it under the existing `@TempDir`:
   ```suggestion
               Path outside = Files.createDirectories(
                               tempDir.resolve("outside").resolve("far"))
                       .resolve("pom.xml");
   ```
   Or, if the intent is that the path genuinely sits outside `tempDir` (to exercise the absolute-path branch), a `try/finally` with `deleteRecursively` would work — though the suggestion above is simpler since `tempDir` is already outside the mock `cwd` passed to the invoker.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MNG-8650] mvnval, a tool to validate POMs without building them [maven]

["gnodet (via GitHub)" <gi...@apache.org>]

gnodet commented on PR #13276:
URL: https://github.com/apache/maven/pull/13276#issuecomment-6047038797

   > ### ⚠️ License declaration missing
   > The pull request description must contain at least one of the following lines with a **checked** box (`[x]`):
   > 
   > ```
   > - [ ] I hereby declare this contribution to be licensed under the [Apache License Version 2.0, January 2004](https://www.apache.org/licenses/LICENSE-2.0)
   > - [ ] I hereby declare that I have filed an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
   > ```
   > 
   > This comment will be hidden automatically once the description is fixed.
   
   @ikxeno you need to add the first line with a checked box


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MNG-8650] mvnval, a tool to validate POMs without building them [maven]

["ikxeno (via GitHub)" <gi...@apache.org>]

ikxeno commented on code in PR #13276:
URL: https://github.com/apache/maven/pull/13276#discussion_r4136384148


##########
impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnval/ValidateInvokerTest.java:
##########
@@ -0,0 +1,789 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.cling.invoker.mvnval;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.Set;
+
+import org.apache.maven.api.Session;
+import org.apache.maven.api.cli.InvokerException;
+import org.apache.maven.api.cli.mvnval.ValidateOptions;
+import org.apache.maven.api.services.ModelBuilder;
+import org.apache.maven.cling.invoker.ProtoLookup;
+import org.apache.maven.impl.standalone.ApiRunner;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Nested;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+/**
+ * Unit tests for the {@link ValidateInvoker} class.
+ * Tests that the verdict depends on the file alone, that a file which cannot be read does not
+ * lose the verdict on the others, and that the exit code follows the reports.
+ */
+@DisplayName("ValidateInvoker")
+class ValidateInvokerTest {
+
+    private static final String DUPLICATE_DEPENDENCY = """
+            <project xmlns="http://maven.apache.org/POM/4.0.0">
+              <modelVersion>4.0.0</modelVersion>
+              %s
+              <groupId>org.test</groupId>
+              <artifactId>test</artifactId>
+              <version>1.0</version>
+              <dependencies>
+                <dependency>
+                  <groupId>junit</groupId><artifactId>junit</artifactId><version>4.13.2</version>
+                </dependency>
+                <dependency>
+                  <groupId>junit</groupId><artifactId>junit</artifactId><version>4.12</version>
+                </dependency>
+              </dependencies>
+            </project>
+            """;
+
+    private static final String UNRESOLVABLE_PARENT = """
+            <parent>
+                <groupId>org.nowhere</groupId>
+                <artifactId>does-not-exist</artifactId>
+                <version>999.0.0</version>
+              </parent>""";
+
+    @TempDir
+    Path tempDir;
+
+    private List<String> output;
+
+    /** Every path handed to the seam, so a test can assert on the one the run actually made. */
+    private List<Path> aimed;
+
+    private ValidateInvoker invoker;
+
+    @BeforeEach
+    void setUp() {
+        output = new ArrayList<>();
+        // One session for the whole test: building one is the expensive part of every test here.
+        Session shared = ApiRunner.createSession();
+        aimed = new ArrayList<>();
+        invoker = new ValidateInvoker(ProtoLookup.builder().build(), null) {
+            @Override
+            protected Session createSession(Path localRepository) {
+                // withLocalRepository is the line production uses, so an aimed repository is
+                // exercised rather than stubbed. The transports and the settings-derived
+                // repository list production adds are not: this session has neither.
+                aimed.add(localRepository);
+                return localRepository == null
+                        ? shared
+                        : shared.withLocalRepository(shared.createLocalRepository(localRepository));
+            }
+        };
+    }
+
+    private Path writePom(String name, String parent) throws Exception {
+        Path pom = Files.createDirectories(tempDir.resolve(name)).resolve("pom.xml");
+        Files.writeString(pom, String.format(DUPLICATE_DEPENDENCY, parent));
+        return pom;
+    }
+
+    private int run(List<String> poms) throws Exception {
+        return run(poms, Optional.empty());
+    }
+
+    private int run(List<String> poms, Optional<String> format) throws Exception {
+        return run(poms, format, Optional.of("raw"));
+    }
+
+    /** Defaults to raw: the tests using this helper read files only. */
+    private int run(List<String> poms, Optional<String> format, Optional<String> mode) throws Exception {
+        ValidateOptions options = mock(ValidateOptions.class);
+        when(options.format()).thenReturn(format);
+        when(options.mode()).thenReturn(mode);
+        when(options.poms()).thenReturn(poms.isEmpty() ? Optional.empty() : Optional.of(poms));
+
+        ValidateContext context = TestUtils.createMockContext(tempDir, options);
+        context.writer = output::add;
+        return invoker.execute(context);
+    }
+
+    @Nested
+    @DisplayName("Wiring")
+    class WiringTests {
+
+        @Test
+        @DisplayName("should register the transports resolution needs")
+        void shouldRegisterTransports() throws Exception {
+            // The block whose absence produces "No transporter factories registered". Proved over
+            // file://, which needs FileTransporterFactory and no network. Every other test here
+            // replaces createSession, so without this the block could be deleted and stay green.
+            Path repository = Files.createDirectories(tempDir.resolve("served/org/test/far/1.0"));
+            String parent = """
+                    <project xmlns="http://maven.apache.org/POM/4.0.0">
+                      <modelVersion>4.0.0</modelVersion>
+                      <groupId>org.test</groupId><artifactId>far</artifactId>
+                      <version>1.0</version><packaging>pom</packaging>
+                    </project>""";
+            Files.writeString(repository.resolve("far-1.0.pom"), parent);
+            // A repository without checksums is refused before the content is looked at, so the
+            // transport would never be exercised.
+            Files.writeString(repository.resolve("far-1.0.pom.sha1"), sha1(parent));
+            Path pom = Files.createDirectories(tempDir.resolve("fetcher")).resolve("pom.xml");
+            Files.writeString(pom, """
+                    <project xmlns="http://maven.apache.org/POM/4.0.0">
+                      <modelVersion>4.0.0</modelVersion>
+                      <parent>
+                        <groupId>org.test</groupId><artifactId>far</artifactId>
+                        <version>1.0</version><relativePath/>
+                      </parent>
+                      <artifactId>fetcher</artifactId>
+                      <repositories>
+                        <repository><id>served</id><url>%s</url></repository>
+                      </repositories>
+                    </project>""".formatted(tempDir.resolve("served").toUri()));
+
+            ValidateOptions options = mock(ValidateOptions.class);
+            when(options.mode()).thenReturn(Optional.of("effective"));
+            when(options.localRepository())
+                    .thenReturn(Optional.of(tempDir.resolve("into").toString()));
+            when(options.poms()).thenReturn(Optional.of(List.of(pom.toString())));
+            ValidateContext context = TestUtils.createMockContext(tempDir, options);
+            context.writer = output::add;
+
+            int exitCode = new ValidateInvoker(ProtoLookup.builder().build(), null).execute(context);
+
+            assertEquals(ValidateInvoker.OK, exitCode, output.toString());
+            assertTrue(
+                    Files.exists(tempDir.resolve("into/org/test/far/1.0/far-1.0.pom")),
+                    "the parent has to arrive through a transport: " + output);
+        }
+
+        @Test
+        @DisplayName("should exit on bad usage when the model builder cannot validate")
+        void shouldRefuseAModelBuilderThatCannotValidate() throws Exception {
+            // validate() is a default that throws, so an out-of-tree ModelBuilder may not have it.
+            // Unreachable from the shipped CLI, which is why it is worth pinning here: nothing was
+            // wrong with the POM, so this must not be the code that means a POM was rejected.
+            ValidateInvoker refusing = new ValidateInvoker(ProtoLookup.builder().build(), null) {
+                @Override
+                protected Session createSession(Path localRepository) {
+                    Session session = mock(Session.class);
+                    ModelBuilder builder = mock(ModelBuilder.class);
+                    ModelBuilder.ModelBuilderSession builderSession = mock(ModelBuilder.ModelBuilderSession.class);
+                    when(session.getService(ModelBuilder.class)).thenReturn(builder);
+                    when(builder.newSession()).thenReturn(builderSession);
+                    when(builderSession.validate(any()))
+                            .thenThrow(new UnsupportedOperationException("does not support validating a model"));
+                    return session;
+                }
+            };
+            ValidateOptions options = mock(ValidateOptions.class);
+            when(options.mode()).thenReturn(Optional.of("raw"));
+            when(options.poms())
+                    .thenReturn(Optional.of(List.of(writePom("unsupported", "").toString())));
+            ValidateContext context = TestUtils.createMockContext(tempDir, options);
+            context.writer = output::add;
+
+            assertEquals(ValidateInvoker.BAD_OPERATION, refusing.execute(context));
+        }
+    }
+
+    private static String sha1(String content) throws Exception {
+        byte[] digest = java.security.MessageDigest.getInstance("SHA-1").digest(content.getBytes(UTF_8));
+        StringBuilder hex = new StringBuilder(digest.length * 2);
+        for (byte b : digest) {
+            hex.append(String.format("%02x", b));
+        }
+        return hex.toString();
+    }
+
+    @Nested
+    @DisplayName("Determinism")
+    class DeterminismTests {
+
+        @Test
+        @DisplayName("should reach the same verdict whether or not the parent can be resolved")
+        void shouldReachSameVerdictRegardlessOfParent() throws Exception {
+            Path withParent = writePom("with-parent", UNRESOLVABLE_PARENT);
+            Path withoutParent = writePom("without-parent", "");
+
+            int exitWith = run(List.of(withParent.toString()));
+            List<String> linesWith = List.copyOf(output);
+            output.clear();
+            int exitWithout = run(List.of(withoutParent.toString()));
+
+            assertEquals(exitWith, exitWithout, "an unresolvable parent must not change the exit code");
+            assertTrue(
+                    linesWith.stream().anyMatch(l -> l.contains("must be unique")),
+                    "the duplicate dependency should be reported even with an unresolvable parent: " + linesWith);
+            assertTrue(
+                    output.stream().anyMatch(l -> l.contains("must be unique")),
+                    "and also without a parent: " + output);
+        }
+
+        @Test
+        @DisplayName("should print a path under the working directory as the caller typed it")
+        void shouldPrintPathsRelativeToTheWorkingDirectory() throws Exception {
+            Path pom = writePom("near", "");
+            Path outside = Files.createDirectories(
+                            Files.createTempDirectory("mvnval-outside-").resolve("far"))
+                    .resolve("pom.xml");
+            Files.writeString(outside, String.format(DUPLICATE_DEPENDENCY, ""));

Review Comment:
   Done



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MNG-8650] mvnval, a tool to validate POMs without building them [maven]

["ikxeno (via GitHub)" <gi...@apache.org>]

ikxeno commented on PR #13276:
URL: https://github.com/apache/maven/pull/13276#issuecomment-5891628016

   @gnodet 
   
   Default is `--mode effective` now, `--mode raw` the opt-out.
   
   Effective runs the raw pass first. `build()` with `BUILD_EFFECTIVE` says nothing
   about the reactor, so on its own it passed a project whose `<modules>` points at
   a directory that isn't there. No second read, the file and raw models are cached
   per source (`DefaultModelBuilder:1812`, `:2168`).
   
   `--local-repository`, `--temp-local-repository` and `maven.repo.local` all work.
   I set the path with `Session.withLocalRepository` on the session that comes back
   rather than passing it to `ApiRunner.createSession`, because two things can drop
   that argument: `settings.getLocalRepository()` wins over it at `ApiRunner:594`,
   and the injector can hand back another tool's pl:333`
   binds a qualified `@Provides` under the unqualified key as well, so mvnup's can
   answer `getInstance(Session.class)`. Neither f is what
   made it worth pinning. Same reason I re-derive the remote repositories from the
   settings. File that injector one separately?
   
   The shared session I undid. It makes the verdir:
   derived sessions share `mappedSources`, so a GA read from an earlier POM answers
   a later one's parent lookup. Same two files, oan error
   about the wrong parent. There's a test now. Every pass walks the reactor from the
   root again, so sharing saved no reading either
   
   Your javadoc sketch has `RequestType` controllnever
   reads it. I wrote what's true instead. Want it to actually dispatch?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MNG-8650] mvnval, a tool to validate POMs without building them [maven]

["gnodet-bot (via GitHub)" <gi...@apache.org>]

gnodet-bot commented on code in PR #13276:
URL: https://github.com/apache/maven/pull/13276#discussion_r4212998409


##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnval/ValidateInvoker.java:
##########
@@ -0,0 +1,736 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.cling.invoker.mvnval;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.Comparator;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.function.Consumer;
+import java.util.stream.Stream;
+
+import org.apache.maven.api.Constants;
+import org.apache.maven.api.RemoteRepository;
+import org.apache.maven.api.Session;
+import org.apache.maven.api.annotations.Nullable;
+import org.apache.maven.api.cli.InvokerException;
+import org.apache.maven.api.cli.InvokerRequest;
+import org.apache.maven.api.cli.mvnval.ValidateOptions;
+import org.apache.maven.api.di.Named;
+import org.apache.maven.api.di.Provides;
+import org.apache.maven.api.model.Model;
+import org.apache.maven.api.model.Profile;
+import org.apache.maven.api.model.Repository;
+import org.apache.maven.api.model.RepositoryPolicy;
+import org.apache.maven.api.services.Lookup;
+import org.apache.maven.api.services.ModelBuilder;
+import org.apache.maven.api.services.ModelBuilder.ModelBuilderSession;
+import org.apache.maven.api.services.ModelBuilderException;
+import org.apache.maven.api.services.ModelBuilderRequest;
+import org.apache.maven.api.services.ModelBuilderRequest.RequestType;
+import org.apache.maven.api.services.ModelBuilderResult;
+import org.apache.maven.api.services.RepositoryFactory;
+import org.apache.maven.api.services.SettingsBuilder;
+import org.apache.maven.api.services.Sources;
+import org.apache.maven.cling.invoker.LookupContext;
+import org.apache.maven.cling.invoker.LookupInvoker;
+import org.apache.maven.cling.logging.Slf4jConfiguration;
+import org.apache.maven.impl.InternalSession;
+import org.apache.maven.impl.standalone.ApiRunner;
+import org.apache.maven.jline.MessageUtils;
+import org.eclipse.aether.DefaultRepositorySystemSession;
+import org.eclipse.aether.spi.connector.transport.TransporterFactory;
+import org.eclipse.aether.spi.connector.transport.http.ChecksumExtractor;
+import org.eclipse.aether.spi.io.PathProcessor;
+import org.eclipse.aether.transport.apache.ApacheTransporterFactory;
+import org.eclipse.aether.transport.file.FileTransporterFactory;
+import org.jline.reader.UserInterruptException;
+import org.jline.terminal.Terminal;
+
+/**
+ * Validates POM files without building them.
+ * <p>
+ * Two modes, and one contains the other. {@code raw} stops at
+ * {@link ModelBuilder.ModelBuilderSession#validate}, reaching no repository at all: it works
+ * offline and on a POM whose parent is not published yet, but it cannot see a problem that
+ * inheritance introduces, such as a dependency whose version comes from the parent's
+ * {@code dependencyManagement}. {@code effective}, the default, runs that pass and then builds
+ * the effective model as well, so parents and imported boms are resolved and the checks needing
+ * them run too. Both passes, because the effective build says nothing about the reactor around a
+ * POM: on its own it passes a project whose declared subproject is not on disk, which
+ * {@code mvn} refuses to read.
+ * <p>
+ * Nothing is ever written back to a POM. In {@code effective} mode what is resolved lands in the
+ * local repository, as it does for every Maven tool. {@code --local-repository} aims that
+ * somewhere else and {@code --temp-local-repository} at a directory made for the run and deleted
+ * when it ends, for a gate that wants the configured repository left as it was.
+ * <p>
+ * It stands up no container, so the directory holding a POM cannot make this process load
+ * anything through {@code .mvn/extensions.xml}, {@code maven.ext.class.path} or
+ * {@code .mvn/settings.xml}. The user's own settings are another matter: resolution reads them
+ * for mirrors, proxies and credentials, as it has to. The JVM is outside this:
+ * {@code bin/mvn} passes {@code .mvn/jvm.config} to the launcher before this process starts.
+ * <p>
+ * {@code -o} works in {@code effective} mode: resolution hits the local repository only and
+ * fails fast when a parent or BOM is absent. Combined with {@code --temp-local-repository} it
+ * proves that the POMs being validated are fully self-contained.
+ * <p>
+ * When more than one POM is given, {@code effective} mode pre-scans them all to build a bundle
+ * workspace: a parent that is named in the bundle is resolved from disk rather than from a
+ * repository, so the verdict does not depend on publication order and the run does not download
+ * what is already on disk.
+ */
+public class ValidateInvoker extends LookupInvoker<ValidateContext> {
+
+    /** Where Central lives, for the one case where no settings file names a repository. */
+    private static final String CENTRAL_URL = "https://repo.maven.apache.org/maven2";
+
+    public static final int OK = 0;
+    public static final int ERROR = 1;
+
+    /** Bad user input. Same meaning as in {@code mvnenc} and {@code mvnup}. */
+    public static final int BAD_OPERATION = 2;
+
+    /**
+     * Interrupted. Same meaning as in {@code mvnenc} and {@code mvnup}, and reachable only with a
+     * terminal attached: without one the signal reaches the JVM instead and the exit code is its.
+     * Left out of {@code --help} for that reason.
+     */
+    public static final int CANCELED = 3;
+
+    /**
+     * Nothing was rejected, but something was reported. Numbered 4 rather than reusing a lower
+     * code: 0 to 3 mean the same thing across the Maven 4 tools, and a gate must be able to tell
+     * "this POM has warnings" from "mvnval broke" or "somebody hit Ctrl+C".
+     */
+    public static final int WARNINGS = 4;
+
+    public ValidateInvoker(Lookup protoLookup, @Nullable Consumer<LookupContext> contextConsumer) {
+        super(protoLookup, contextConsumer);
+    }
+
+    @Override
+    protected ValidateContext createContext(InvokerRequest invokerRequest) {
+        return new ValidateContext(
+                invokerRequest, (ValidateOptions) invokerRequest.options().orElse(null));
+    }
+
+    @Override
+    protected int execute(ValidateContext context) throws Exception {
+        Thread validationThread = Thread.currentThread();
+        AtomicBoolean cancellationRequested = new AtomicBoolean();
+        Terminal.SignalHandler previousHandler = context.terminal.handle(Terminal.Signal.INT, signal -> {
+            cancellationRequested.set(true);
+            validationThread.interrupt();
+        });
+        try {
+            OutputFormat format;
+            ValidationMode mode;
+            try {
+                // Checked here rather than while parsing: the parser knows the option names,
+                // not the values, and a value it did accept can still become anything once
+                // options are interpolated.
+                format = context.options().format().map(OutputFormat::parse).orElse(OutputFormat.TEXT);
+                mode = context.options().mode().map(ValidationMode::parse).orElse(ValidationMode.EFFECTIVE);
+            } catch (IllegalArgumentException e) {
+                context.logger.error(e.getMessage());
+                return BAD_OPERATION;
+            }
+            // -s/-ps/-is redirect the settings files that ApiRunner reads at session-creation time.
+            // Honouring them would require threading the alternate path into ApiRunner before it
+            // starts; that is a larger change than belongs here, so they are still refused.
+            // -o is different: it applies to the resolver session, which createSession() returns,
+            // and can be applied there after the session is built.  Raw mode resolves nothing, so
+            // none of these apply there.
+            if (mode != ValidationMode.RAW) {
+                String refused = refusedSettingsOption(context.options());
+                if (refused != null) {
+                    context.logger.error(refused
+                            + " cannot redirect the settings file mvnval reads."
+                            + " Use the default settings, or copy the relevant settings to the default location.");
+                    return BAD_OPERATION;
+                }
+            }
+
+            if (context.options().localRepository().isPresent()
+                    && context.options().tempLocalRepository().orElse(false)) {
+                context.logger.error(
+                        "--local-repository and --temp-local-repository name different directories; give one.");
+                return BAD_OPERATION;
+            }
+            String unusableRepository = unusableRepository(context);
+            if (unusableRepository != null) {
+                context.logger.error(unusableRepository);
+                return BAD_OPERATION;
+            }
+            Path localRepository = null;
+            // Only in effective mode: raw resolves nothing, so making a directory there could
+            // lose a run that would never have written to it.
+            if (mode != ValidationMode.RAW) {
+                try {
+                    localRepository = localRepository(context);
+                } catch (IOException e) {
+                    // Not ERROR: no POM was rejected, the run could not be set up.
+                    context.logger.error("Could not create a temporary local repository: " + e);
+                    return BAD_OPERATION;
+                }
+            }
+            Thread cleanup = temporary(context, localRepository) ? cleanupHook(localRepository, context) : null;
+            try {
+                List<Report> reports =
+                        validateAll(context, createSession(localRepository), mode, cancellationRequested);
+                if (cancellationRequested.get() || Thread.currentThread().isInterrupted()) {
+                    return canceled(context);
+                }
+                // determineWriter, not context.writer: on a real run that field is still empty
+                // and this is what fills it.
+                int contextLines = context.options().context().orElse(2);
+                format.report(
+                        reports,
+                        context.cwd.get(),
+                        determineWriter(context),
+                        contextLines,
+                        MessageUtils.isColorEnabled());
+                return exitCode(reports);
+            } finally {
+                if (cleanup != null) {
+                    // Delete first, deregister second. The other order leaves a window where a
+                    // signal arriving after the hook is gone kills the process before the
+                    // deletion runs.
+                    deleteRecursively(localRepository, context);
+                    removeHook(cleanup);
+                }
+            }
+        } catch (UnsupportedOperationException e) {
+            // The ModelBuilder in this container has not written validate. Nothing was wrong with
+            // any POM, so this must not be ERROR, the code a gate reads as "a POM was rejected".
+            context.logger.error("This Maven installation cannot validate POMs: " + e.getMessage());
+            return BAD_OPERATION;
+        } catch (UserInterruptException e) {
+            return canceled(context);
+        } catch (Exception e) {
+            if (cancellationRequested.get() || Thread.currentThread().isInterrupted()) {
+                return canceled(context);
+            }
+            // Without this an unexpected failure escapes to the CLI and becomes 2, which this tool
+            // documents as bad usage. Both siblings catch here too.
+            if (context.options().showErrors().orElse(false)) {
+                context.logger.error(e.getMessage(), e);
+            } else {
+                context.logger.error(e.getMessage());
+            }
+            return ERROR;
+        } finally {
+            context.terminal.handle(Terminal.Signal.INT, previousHandler);
+            if (cancellationRequested.get()) {
+                Thread.interrupted();
+            }
+        }
+    }
+
+    private static int canceled(ValidateContext context) {
+        Thread.interrupted();
+        context.logger.error("Validation canceled by user.");
+        return CANCELED;
+    }
+
+    /**
+     * Turns an unparseable command line into {@link #BAD_OPERATION}.
+     * <p>
+     * The base exits 1 for a bad argument, which is the code this tool uses for a POM it rejected.
+     * A gate has to be able to tell a typo in its own script from a POM that failed validation.
+     */
+    @Override
+    protected void validate(ValidateContext context) throws Exception {
+        try {
+            super.validate(context);
+        } catch (InvokerException.ExitException e) {
+            throw e.getExitCode() == ERROR ? new InvokerException.ExitException(BAD_OPERATION) : e;
+        }
+    }
+
+    /** Whether stdout carries a document this run must not write anything else to. */
+    private static boolean json(ValidateContext context) {
+        return context.options()
+                .format()
+                .filter(OutputFormat.JSON.name()::equalsIgnoreCase)
+                .isPresent();
+    }
+
+    /**
+     * Keeps the version banner out of the document under {@code --format json}.
+     * <p>
+     * The base prints it whenever {@code effectiveVerbose()} holds, which a CI runner setting
+     * {@code RUNNER_DEBUG=1} makes true, so re-running a job with debug logging turned the
+     * document into something no parser accepts. Asking for {@code -V} still prints it, since
+     * that is what the flag is for.
+     */
+    @Override
+    protected void preCommands(ValidateContext context) throws Exception {
+        if (json(context) && !context.options().showVersion().orElse(false)) {
+            return;
+        }
+        super.preCommands(context);
+    }
+
+    /**
+     * Suppresses resolver noise from the output unless the caller asked for it.
+     * <p>
+     * The resolver writes an {@code [INFO]} line the first time it reads a repository's prefix
+     * file. Under {@code --format json} that line breaks the document; in text mode it clutters
+     * output that is meant to list only POM problems. Neither case benefits from seeing it.
+     * The level is forced to {@code ERROR} — the same effect as {@code -q} — unless {@code -X}
+     * or {@code -e} is given, in which case the full log is what the caller wants.
+     * <p>
+     * The property must be set <em>before</em> calling {@link Slf4jConfiguration#setRootLoggerLevel}
+     * because that method itself logs at {@code INFO} when it overrides a value already set (e.g.
+     * a CI runner running with {@code DEBUG}), which would be the very noise we are suppressing.
+     */
+    @Override
+    protected void configureLogging(ValidateContext context) throws Exception {
+        super.configureLogging(context);
+        if (!context.options().verbose().orElse(false)
+                && !context.options().showErrors().orElse(false)) {
+            context.loggerLevel = Slf4jConfiguration.Level.ERROR;
+            System.setProperty(Constants.MAVEN_LOGGER_DEFAULT_LOG_LEVEL, "error");
+            context.slf4jConfiguration.setRootLoggerLevel(context.loggerLevel);
+        }
+    }
+
+    // The next five steps stand up dependency injection and read configuration. Nothing here uses
+    // either: the model builder comes from createSession(). Running them would let the directory
+    // holding the POM decide what this process loads, via .mvn/extensions.xml, .mvn/settings.xml,
+    // or maven.ext.class.path in .mvn/maven-user.properties. Refused one at a time rather than by
+    // replacing doInvoke, so a step added to the base class later still runs.
+
+    /** No container: nothing is looked up, and no extension or contributed property is loaded. */
+    @Override
+    protected void container(ValidateContext context) {}
+
+    /** No container, so no {@code PropertyContributor} to run. */
+    @Override
+    protected void postContainer(ValidateContext context) {}
+
+    /** No container to look up from. */
+    @Override
+    protected void lookup(ValidateContext context) {}
+
+    /** No {@code EventSpy} dispatch: this tool emits no build events. */
+    @Override
+    protected void init(ValidateContext context) {}
+
+    /**
+     * No settings step: the model builder runs on the session from {@link #createSession(Path)},
+     * which reads the user's settings itself when it has to resolve.
+     */
+    @Override
+    protected void settings(ValidateContext context) {}
+
+    /**
+     * Creates the session the model builder runs on. Made here because {@code LookupInvoker} only
+     * ever builds a {@code ProtoSession} while {@link ModelBuilderRequest} needs a full
+     * {@link Session}. Left {@code protected} as a seam: building one is the expensive part of
+     * every test in this package, and the tests substitute a shared one.
+     *
+     * @return the session, never {@code null}
+     */
+    protected Session createSession(@Nullable Path localRepository) {
+        Session session = ApiRunner.createSession(injector -> injector.bindImplicit(TransporterFactoryConfig.class));
+        if (localRepository != null) {
+            // Set on the session that came back, not passed to createSession, which prefers
+            // settings.getLocalRepository() over its argument and may in any case hand back a
+            // session another tool built. See derivedRepositories for why that happens.
+            session = session.withLocalRepository(session.createLocalRepository(localRepository));
+        }
+        return session.withRemoteRepositories(derivedRepositories(session));
+    }
+
+    /**
+     * Applies {@code --offline} to a session built by {@link #createSession}.
+     * <p>
+     * {@code ApiRunner.createSession} applies offline from the settings file.  This supplements
+     * that: when the caller passes {@code -o} on the command line, the session is made offline
+     * regardless of what the settings say, by cloning the underlying resolver session with offline

Review Comment:
   💡 **Stale Javadoc** — the phrase "by cloning the underlying resolver session with offline set to true" no longer matches the implementation. The method now mutates in-place rather than cloning.
   
   ```suggestion
        * regardless of what the settings say, by setting offline to true on the underlying resolver session.
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MNG-8650] mvnval, a tool to validate POMs without building them [maven]

["gnodet (via GitHub)" <gi...@apache.org>]

gnodet commented on PR #13276:
URL: https://github.com/apache/maven/pull/13276#issuecomment-6048695106

   The `pr-check / license-declaration` CI step is failing because the PR description does not contain a checked license declaration.
   
   Please add one of the following lines (with the box checked `[x]`) to the PR description:
   
   - [x] I hereby declare this contribution to be licensed under the [Apache License Version 2.0, January 2004](https://www.apache.org/licenses/LICENSE-2.0)
   
   or
   
   - [x] I hereby declare that I have filed an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
   
   Once the description is updated, the check will re-run and clear automatically.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MNG-8650] mvnval, a tool to validate POMs without building them [maven]

["Bukama (via GitHub)" <gi...@apache.org>]

Bukama commented on PR #13276:
URL: https://github.com/apache/maven/pull/13276#issuecomment-6054023009

   Hi,
   I leave the technical parts of this to @gnodet. But if this gets merged, I encourage you to also create a PR on maven-site and add a small page + menu entry in the tool section. Thank you. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MNG-8650] mvnval, a tool to validate POMs without building them [maven]

["gnodet (via GitHub)" <gi...@apache.org>]

gnodet commented on PR #13276:
URL: https://github.com/apache/maven/pull/13276#issuecomment-6046970501

   I pushed three improvements on top of this PR:
   
   **1. Suppress resolver noise in text mode**
   
   `configureLogging` now forces resolver logging to ERROR level in both text and JSON modes (unless `-X`/`-e` is passed). Previously only JSON mode silenced it; text output was cluttered with lines like `[INFO] Loaded 24013 auto-discovered prefixes from ...` that have nothing to do with POM validation.
   
   **2. Extract line/column from `XmlReaderException`**
   
   `DefaultModelBuilder.add()` now falls back to `XmlReaderException.getLocation()` when no `InputLocation` is present on the problem. Parser failures like `Duplicated tag: 'relativePath'` carry the location in the exception but it was never consulted, so the FATAL problem was reported without any line or column number.
   
   **3. XML source context display (`-C` / `--context`)**
   
   `TextReporter` now shows N lines of surrounding XML source around each problem location (default 2, configurable via `-C N` / `--context N`, disabled with `-C 0`). The problem line is marked with `>` and rendered in bold. Severity labels are colored (red for ERROR/FATAL, yellow for WARNING) when the terminal supports ANSI color — using `MessageUtils.isColorEnabled()` directly rather than `context.coloredOutput`, which stays `null` in auto mode until JLine resolves it from the terminal type.
   
   Example output:
   ```
   /path/to/pom.xml:
     FATAL Non-parseable POM: Duplicated tag: 'relativePath' @ line 6, column 5
       5 |     <relativePath/>
     > 6 |     <relativePath/>
       7 |   </parent>
   ```


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MNG-8650] mvnval, a tool to validate POMs without building them [maven]

["ikxeno (via GitHub)" <gi...@apache.org>]

ikxeno commented on code in PR #13276:
URL: https://github.com/apache/maven/pull/13276#discussion_r4116427620


##########
THREAT_MODEL.md:
##########
@@ -166,6 +166,7 @@ Maven's inputs split cleanly into **operator-supplied (trusted)** and **reposito
 | **Consumer-POM transform input** (`mvn deploy`) | **4.x only** | operator project → published | source **no**; the *published* result is a new surface for downstream | verify what you publish matches intent *(inferred, Q7)* |
 | **`mvnup` rewrite input** | **4.x only** | operator `pom.xml` | **no** — trusted, but written back in place | review the diff `mvnup` produces *(inferred, Q8)* |
 | **`mvnenc` secrets / vault** | **4.x only** | operator | **no** — trusted | manage the master key/vault (§10) *(inferred, Q9)* |
+| **`mvnval` validation input** | **4.x only** | operator `pom.xml` | **no** — trusted, read-only | reads POMs and reports on them; resolves no parent, reaches no network, writes nothing back |

Review Comment:
   It wasn't tested, which is why it was wrong. Two channels were open.
   
   .mvn/extensions.xml was resolved from central before validation started. With that fixed, maven.ext.class.path in .mvn/maven-user.properties still put a jar on the container classpath:
   
   ```
   $ mvnval -X pom.xml | grep evil
   [DEBUG]   included '/tmp/evil.jar'
   ```
   
   So instead of closing channels one by one, the tool now boots no container. It used nothing from it, and that also drops .mvn/settings.xml beside the POM. There's a test for it.
   
   .mvn/jvm.config it can't refuse, since bin/mvn reads it before any Java runs. The row says so now.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MNG-8650] mvnval, a tool to validate POMs without building them [maven]

["ikxeno (via GitHub)" <gi...@apache.org>]

ikxeno commented on code in PR #13276:
URL: https://github.com/apache/maven/pull/13276#discussion_r4116976760


##########
impl/maven-cli/src/main/java/org/apache/maven/cling/MavenValCling.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0

Review Comment:
   The same as below



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MNG-8650] mvnval, a tool to validate POMs without building them [maven]

["ikxeno (via GitHub)" <gi...@apache.org>]

ikxeno commented on code in PR #13276:
URL: https://github.com/apache/maven/pull/13276#discussion_r4116922519


##########
impl/maven-cli/src/main/java/org/apache/maven/cling/MavenValCling.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.cling;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+
+import org.apache.maven.api.annotations.Nullable;
+import org.apache.maven.api.cli.Invoker;
+import org.apache.maven.api.cli.Parser;
+import org.apache.maven.api.cli.ParserRequest;
+import org.apache.maven.cling.invoker.ProtoLookup;
+import org.apache.maven.cling.invoker.mvnval.ValidateInvoker;
+import org.apache.maven.cling.invoker.mvnval.ValidateParser;
+import org.codehaus.plexus.classworlds.ClassWorld;
+
+/**
+ * Maven POM validation CLI "new-gen".
+ */
+public class MavenValCling extends ClingSupport {
+    /**
+     * "Normal" Java entry point. Note: Maven uses ClassWorld Launcher and this entry point is NOT used under normal
+     * circumstances.
+     */
+    public static void main(String[] args) throws IOException {
+        int exitCode = new MavenValCling().run(args, null, null, null, false);
+        System.exit(exitCode);
+    }
+
+    /**
+     * ClassWorld Launcher "enhanced" entry point: returning exitCode and accepts Class World.
+     */
+    public static int main(String[] args, ClassWorld world) throws IOException {
+        return new MavenValCling(world).run(args, null, null, null, false);
+    }
+
+    /**
+     * ClassWorld Launcher "embedded" entry point: returning exitCode and accepts Class World and streams.
+     */
+    public static int main(
+            String[] args,
+            ClassWorld world,
+            @Nullable InputStream stdIn,
+            @Nullable OutputStream stdOut,
+            @Nullable OutputStream stdErr)
+            throws IOException {
+        return new MavenValCling(world).run(args, stdIn, stdOut, stdErr, true);
+    }
+
+    public MavenValCling() {
+        super();
+    }
+
+    public MavenValCling(ClassWorld classWorld) {
+        super(classWorld);
+    }
+
+    @Override
+    protected Invoker createInvoker() {
+        return new ValidateInvoker(
+                ProtoLookup.builder().addMapping(ClassWorld.class, classWorld).build(), null);

Review Comment:
   Right, and worse: `maven.ext.class.path` in `.mvn/maven-user.properties` was read too, so 6781eef stands up no container at all.



##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnval/CommonsCliValidateOptions.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.cling.invoker.mvnval;
+
+import java.util.List;
+import java.util.Optional;
+import java.util.function.Consumer;
+
+import org.apache.commons.cli.CommandLine;
+import org.apache.commons.cli.Option;
+import org.apache.commons.cli.ParseException;
+import org.apache.maven.api.annotations.Nonnull;
+import org.apache.maven.api.cli.Options;
+import org.apache.maven.api.cli.ParserRequest;
+import org.apache.maven.api.cli.mvnval.ValidateOptions;
+import org.apache.maven.cling.invoker.CommonsCliOptions;
+
+/**
+ * Implementation of {@link ValidateOptions} (base + mvnval).
+ */
+public class CommonsCliValidateOptions extends CommonsCliOptions implements ValidateOptions {
+
+    public static CommonsCliValidateOptions parse(String[] args) throws ParseException {
+        CLIManager cliManager = new CLIManager();
+        CommonsCliValidateOptions options =
+                new CommonsCliValidateOptions(Options.SOURCE_CLI, cliManager, cliManager.parse(args));
+        // Reject a bad --format here, so the user gets a usage error rather than a failure
+        // after the container has been built.
+        try {
+            options.format().ifPresent(OutputFormat::parse);
+        } catch (IllegalArgumentException e) {
+            throw new ParseException(e.getMessage());
+        }
+        return options;
+    }

Review Comment:
   Correct, the check moved into the invoker and `mvnval --format bogus` exits 2 now.



##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnval/ValidateParser.java:
##########
@@ -0,0 +1,35 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.cling.invoker.mvnval;
+
+import org.apache.commons.cli.ParseException;
+import org.apache.maven.api.cli.Options;
+import org.apache.maven.cling.invoker.BaseParser;
+
+public class ValidateParser extends BaseParser {
+
+    @Override
+    protected Options parseCliOptions(LocalContext context) {
+        try {
+            return CommonsCliValidateOptions.parse(context.parserRequest.args().toArray(new String[0]));
+        } catch (ParseException e) {
+            throw new IllegalArgumentException("Failed to parse command line options: " + e.getMessage(), e);

Review Comment:
   Same one, fixed in 1bee3a8.



##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/ModelBuilder.java:
##########
@@ -38,6 +38,33 @@ public interface ModelBuilder extends Service {
     interface ModelBuilderSession {
 
         ModelBuilderResult build(ModelBuilderRequest request) throws ModelBuilderException;
+
+        /**
+         * Reads and validates the raw model, without inheritance, interpolation or profile injection.
+         * <p>
+         * Unlike {@link #build(ModelBuilderRequest)} this does not throw when validation reports
+         * errors: the result carries errors and warnings alike, so a caller that only wants to
+         * report on a POM can see warnings that come with no error. A problem severe enough that
+         * the model cannot be read at all is still thrown.
+         * <p>
+         * Reading one raw model can need others: a subproject may leave out its parent version,
+         * which is then taken from the parent's own file model. An implementation may therefore
+         * read files besides the one the request names, up to the project root. Nothing is
+         * resolved from a repository and nothing is downloaded.
+         * <p>
+         * Only the source, the file model, the raw model and the problems are populated on the
+         * result. The accessors that describe a fully built model, such as
+         * {@link ModelBuilderResult#getEffectiveModel()}, are not.
+         *
+         * @param request the request containing the parameters for reading the model
+         * @return the result, carrying the raw model and the problems collected
+         * @throws ModelBuilderException if the model cannot be read at all
+         * @throws UnsupportedOperationException if this implementation does not support it
+         * @since 4.1.0
+         */
+        default ModelBuilderResult buildRaw(ModelBuilderRequest request) throws ModelBuilderException {
+            throw new UnsupportedOperationException(getClass().getName() + " does not support reading a raw model");
+        }

Review Comment:
   A default only breaks implementors that call it, an abstract method breaks all of them, which is why it is a default.



##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnval/ValidateInvoker.java:
##########
@@ -0,0 +1,197 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.cling.invoker.mvnval;
+
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.function.Consumer;
+
+import org.apache.maven.api.Session;
+import org.apache.maven.api.annotations.Nullable;
+import org.apache.maven.api.cli.InvokerRequest;
+import org.apache.maven.api.cli.mvnval.ValidateOptions;
+import org.apache.maven.api.services.Lookup;
+import org.apache.maven.api.services.ModelBuilder;
+import org.apache.maven.api.services.ModelBuilderException;
+import org.apache.maven.api.services.ModelBuilderRequest;
+import org.apache.maven.api.services.ModelBuilderResult;
+import org.apache.maven.api.services.ModelProblem;
+import org.apache.maven.api.services.Sources;
+import org.apache.maven.cling.invoker.CoreExtensionSelector;
+import org.apache.maven.cling.invoker.LookupContext;
+import org.apache.maven.cling.invoker.LookupInvoker;
+import org.apache.maven.impl.standalone.ApiRunner;
+
+/**
+ * Validates POM files without building them.
+ * <p>
+ * Validation stops after {@link ModelBuilder.ModelBuilderSession#buildRaw}, so no parent is
+ * resolved and the verdict depends on the files alone, never on the network. The problems it can
+ * reach are therefore a subset: a dependency whose version comes from the parent's
+ * {@code dependencyManagement} is only checked later, in {@code validateEffectiveModel}.
+ */
+public class ValidateInvoker extends LookupInvoker<ValidateContext> {
+
+    public static final int OK = 0;
+    public static final int ERROR = 1;
+
+    /** Bad user input, as in {@code mvnenc} and {@code mvnup}. */
+    public static final int BAD_OPERATION = 2;
+
+    /**
+     * Nothing was rejected, but something was reported. Deliberately not 2: the CLI itself exits
+     * with 2 when a tool fails in a way it does not handle, and a gate must be able to tell "this
+     * POM has warnings" from "mvnval broke".
+     */
+    public static final int WARNINGS = 3;
+
+    public ValidateInvoker(Lookup protoLookup, @Nullable Consumer<LookupContext> contextConsumer) {
+        super(protoLookup, contextConsumer);
+    }
+
+    @Override
+    protected ValidateContext createContext(InvokerRequest invokerRequest) {
+        return new ValidateContext(
+                invokerRequest, (ValidateOptions) invokerRequest.options().orElse(null));
+    }
+
+    @Override
+    protected int execute(ValidateContext context) throws Exception {
+        OutputFormat format;
+        try {
+            // The parser checks --format too, but options are interpolated after parsing, so
+            // ${...} can still turn into anything by the time it gets here.
+            format = context.options().format().map(OutputFormat::parse).orElse(OutputFormat.TEXT);
+        } catch (IllegalArgumentException e) {
+            context.logger.error(e.getMessage());
+            return BAD_OPERATION;
+        }
+
+        List<Report> reports = validate(resolvePoms(context), createSession());

Review Comment:
   The comment was wrong and has been reworded.



##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelBuilder.java:
##########
@@ -1053,14 +1097,17 @@ private void loadFromRoot(Path root, Path top) {
                 loadFilePom(executor, top, root, Set.of(), r);
             }
             if (result.getFileModel() == null && !Objects.equals(top, root)) {
-                logger.warn(
-                        "The top project ({}) cannot be found in the reactor from root project ({}). "
-                                + "Make sure the root directory is correct (a missing '.mvn' directory in the root "
-                                + "project is the most common cause) and the project is correctly included "
-                                + "in the reactor (missing activated profiles, command line options, etc.). For this "
-                                + "build, the top project will be used as the root project.",
-                        top,
-                        root);
+                // Only a warning when a build is what was asked for. A caller reading one POM has
+                // no reactor to get wrong, and the retry below is routine there, not a misconfiguration.
+                (reportReactorLoadFailures ? logger.atWarn() : logger.atDebug())
+                        .log(
+                                "The top project ({}) cannot be found in the reactor from root project ({}). "
+                                        + "Make sure the root directory is correct (a missing '.mvn' directory in the root "
+                                        + "project is the most common cause) and the project is correctly included "
+                                        + "in the reactor (missing activated profiles, command line options, etc.). For this "
+                                        + "build, the top project will be used as the root project.",

Review Comment:
   Fair, `derive()` copies the flag now.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MNG-8650] mvnval, a tool to validate POMs without building them [maven]

["elharo (via GitHub)" <gi...@apache.org>]

elharo commented on code in PR #13276:
URL: https://github.com/apache/maven/pull/13276#discussion_r4115184947


##########
apache-maven/src/assembly/maven/bin/mvnval.cmd:
##########
@@ -0,0 +1,39 @@
+@REM Licensed to the Apache Software Foundation (ASF) under one
+@REM or more contributor license agreements.  See the NOTICE file
+@REM distributed with this work for additional information
+@REM regarding copyright ownership.  The ASF licenses this file
+@REM to you under the Apache License, Version 2.0 (the
+@REM "License"); you may not use this file except in compliance
+@REM with the License.  You may obtain a copy of the License at
+@REM
+@REM    http://www.apache.org/licenses/LICENSE-2.0
+@REM
+@REM Unless required by applicable law or agreed to in writing,
+@REM software distributed under the License is distributed on an
+@REM "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+@REM KIND, either express or implied.  See the License for the
+@REM specific language governing permissions and limitations
+@REM under the License.
+
+@REM -----------------------------------------------------------------------------
+@REM Apache Maven POM Validation Script
+@REM
+@REM Environment Variable Prerequisites
+@REM
+@REM   JAVA_HOME           (Optional) Points to a Java installation.
+@REM   MAVEN_BATCH_ECHO    (Optional) Set to 'on' to enable the echoing of the batch commands.

Review Comment:
   Is on conventional? I'd expect true



##########
impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnval/CommonsCliValidateOptionsTest.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0

Review Comment:
   https here and elsewhere



##########
impl/maven-cli/src/main/java/org/apache/maven/cling/MavenValCling.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0

Review Comment:
   https



##########
THREAT_MODEL.md:
##########
@@ -166,6 +166,7 @@ Maven's inputs split cleanly into **operator-supplied (trusted)** and **reposito
 | **Consumer-POM transform input** (`mvn deploy`) | **4.x only** | operator project → published | source **no**; the *published* result is a new surface for downstream | verify what you publish matches intent *(inferred, Q7)* |
 | **`mvnup` rewrite input** | **4.x only** | operator `pom.xml` | **no** — trusted, but written back in place | review the diff `mvnup` produces *(inferred, Q8)* |
 | **`mvnenc` secrets / vault** | **4.x only** | operator | **no** — trusted | manage the master key/vault (§10) *(inferred, Q9)* |
+| **`mvnval` validation input** | **4.x only** | operator `pom.xml` | **no** — trusted, read-only | reads POMs and reports on them; resolves no parent, reaches no network, writes nothing back |

Review Comment:
   It really doesn't touch the network? Is this tested somehow?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MNG-8650] mvnval, a tool to validate POMs without building them [maven]

["github-actions[bot] (via GitHub)" <gi...@apache.org>]

github-actions[bot] commented on PR #13276:
URL: https://github.com/apache/maven/pull/13276#issuecomment-6046970097

   <!-- license-declaration-check -->
   ### ⚠️ License declaration missing
   
   The pull request description must contain at least one of the following lines with a **checked** box (`[x]`):
   
   ```markdown
   - [ ] I hereby declare this contribution to be licensed under the [Apache License Version 2.0, January 2004](https://www.apache.org/licenses/LICENSE-2.0)
   - [ ] I hereby declare that I have filed an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
   ```
   
   This comment will be hidden automatically once the description is fixed.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MNG-8650] mvnval, a tool to validate POMs without building them [maven]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #13276:
URL: https://github.com/apache/maven/pull/13276#discussion_r4115200364


##########
impl/maven-cli/src/main/java/org/apache/maven/cling/MavenValCling.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.cling;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+
+import org.apache.maven.api.annotations.Nullable;
+import org.apache.maven.api.cli.Invoker;
+import org.apache.maven.api.cli.Parser;
+import org.apache.maven.api.cli.ParserRequest;
+import org.apache.maven.cling.invoker.ProtoLookup;
+import org.apache.maven.cling.invoker.mvnval.ValidateInvoker;
+import org.apache.maven.cling.invoker.mvnval.ValidateParser;
+import org.codehaus.plexus.classworlds.ClassWorld;
+
+/**
+ * Maven POM validation CLI "new-gen".
+ */
+public class MavenValCling extends ClingSupport {
+    /**
+     * "Normal" Java entry point. Note: Maven uses ClassWorld Launcher and this entry point is NOT used under normal
+     * circumstances.
+     */
+    public static void main(String[] args) throws IOException {
+        int exitCode = new MavenValCling().run(args, null, null, null, false);
+        System.exit(exitCode);
+    }
+
+    /**
+     * ClassWorld Launcher "enhanced" entry point: returning exitCode and accepts Class World.
+     */
+    public static int main(String[] args, ClassWorld world) throws IOException {
+        return new MavenValCling(world).run(args, null, null, null, false);
+    }
+
+    /**
+     * ClassWorld Launcher "embedded" entry point: returning exitCode and accepts Class World and streams.
+     */
+    public static int main(
+            String[] args,
+            ClassWorld world,
+            @Nullable InputStream stdIn,
+            @Nullable OutputStream stdOut,
+            @Nullable OutputStream stdErr)
+            throws IOException {
+        return new MavenValCling(world).run(args, stdIn, stdOut, stdErr, true);
+    }
+
+    public MavenValCling() {
+        super();
+    }
+
+    public MavenValCling(ClassWorld classWorld) {
+        super(classWorld);
+    }
+
+    @Override
+    protected Invoker createInvoker() {
+        return new ValidateInvoker(
+                ProtoLookup.builder().addMapping(ClassWorld.class, classWorld).build(), null);

Review Comment:
   This tool still inherits the normal `LookupInvoker` bootstrap, so `createContainerCapsule()` selects project core extensions and `BootstrapCoreExtensionManager.loadCoreExtensions()` resolves and loads them before `execute()` reaches `buildRaw`. A POM under `.mvn/extensions.xml` can therefore trigger repository access and arbitrary extension code, contradicting the documented file-only/no-network behavior and making validation of an untrusted POM unsafe. Use a no-extension bootstrap for `mvnval` (and cover this with a fixture), or explicitly document and accept this behavior.



##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnval/CommonsCliValidateOptions.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.cling.invoker.mvnval;
+
+import java.util.List;
+import java.util.Optional;
+import java.util.function.Consumer;
+
+import org.apache.commons.cli.CommandLine;
+import org.apache.commons.cli.Option;
+import org.apache.commons.cli.ParseException;
+import org.apache.maven.api.annotations.Nonnull;
+import org.apache.maven.api.cli.Options;
+import org.apache.maven.api.cli.ParserRequest;
+import org.apache.maven.api.cli.mvnval.ValidateOptions;
+import org.apache.maven.cling.invoker.CommonsCliOptions;
+
+/**
+ * Implementation of {@link ValidateOptions} (base + mvnval).
+ */
+public class CommonsCliValidateOptions extends CommonsCliOptions implements ValidateOptions {
+
+    public static CommonsCliValidateOptions parse(String[] args) throws ParseException {
+        CLIManager cliManager = new CLIManager();
+        CommonsCliValidateOptions options =
+                new CommonsCliValidateOptions(Options.SOURCE_CLI, cliManager, cliManager.parse(args));
+        // Reject a bad --format here, so the user gets a usage error rather than a failure
+        // after the container has been built.
+        try {
+            options.format().ifPresent(OutputFormat::parse);
+        } catch (IllegalArgumentException e) {
+            throw new ParseException(e.getMessage());
+        }
+        return options;
+    }

Review Comment:
   Because `ValidateParser` throws here, `BaseParser.parseInvocation` marks the request as `parsingFailed`; `LookupInvoker.validate` then exits with code 1 before `ValidateInvoker.execute` can return `BAD_OPERATION` (2). Consequently an actual `mvnval --format xml` is reported with the parser-failure exit code, contrary to the documented “2 on bad usage” contract. Defer format validation to the invoker (which already maps `OutputFormat.parse` failures to 2), or otherwise give this parser a tool-specific bad-usage path.



##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnval/ValidateParser.java:
##########
@@ -0,0 +1,35 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.cling.invoker.mvnval;
+
+import org.apache.commons.cli.ParseException;
+import org.apache.maven.api.cli.Options;
+import org.apache.maven.cling.invoker.BaseParser;
+
+public class ValidateParser extends BaseParser {
+
+    @Override
+    protected Options parseCliOptions(LocalContext context) {
+        try {
+            return CommonsCliValidateOptions.parse(context.parserRequest.args().toArray(new String[0]));
+        } catch (ParseException e) {
+            throw new IllegalArgumentException("Failed to parse command line options: " + e.getMessage(), e);

Review Comment:
   An invalid `--format` is rejected here during parsing, which makes `BaseParser` set `parsingFailed` and `LookupInvoker.validate()` return exit code 1 before `ValidateInvoker.BAD_OPERATION` can run. That violates this tool's documented contract of exit code 2 for bad usage; add a tool-specific parser-error exit path or defer this validation to execution, and test the actual CLI exit code.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MNG-8650] mvnval, a tool to validate POMs without building them [maven]

["ikxeno (via GitHub)" <gi...@apache.org>]

ikxeno commented on PR #13276:
URL: https://github.com/apache/maven/pull/13276#issuecomment-5869689067

   @elharo 
   
   I agree https would be better, and I can't use it here. Spotless checks every Java
   file against [`config/maven-header-plain.txt`][header] from `maven-shared-resources`,
   and line 10 of it is http. `maven-parent:50` [hands that artifact to spotless][parent]
   as a plugin dependency.
   
   Under CI that's `spotless:check` and an https header fails the build; locally the
   same config runs `spotless:apply` and rewrites it back to http before you notice.
   Either way it takes a change to `maven-shared-resources` first, then one sweep over
   all 3237 files. Glad to do both, just not inside this PR.
   
   [header]: https://github.com/apache/maven-shared-resources/blob/maven-shared-resources-6/src/main/resources/config/maven-header-plain.txt#L10
   [parent]: https://github.com/apache/maven-parent/blob/maven-parent-50/pom.xml#L1246-L1248


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MNG-8650] mvnval, a tool to validate POMs without building them [maven]

["ikxeno (via GitHub)" <gi...@apache.org>]

ikxeno commented on PR #13276:
URL: https://github.com/apache/maven/pull/13276#issuecomment-5895036719

   @gnodet, the CLI is ready for review. Here is the output from the unpacked distribution at `eaabc887b0`.
   
   I ran all nine fixture files in both modes, then checked batch output, JSON, invalid arguments, the `mvn --val` entry point and the repository options. The repository probe has `org.apache:apache:35` as a parent and an empty `<relativePath/>`, so effective mode has to resolve that parent. Each block includes the process exit code. Commands run from the sandbox unless a different directory is shown.
   
   Machine-specific paths are shown as `<sandbox>`.
   
   <details>
   <summary><code>mvnval ok/pom.xml</code> (exit 0)</summary>
   
   ```text
   $ mvnval ok/pom.xml
   ok/pom.xml: no problems
   exit 0
   ```
   
   </details>
   
   <details>
   <summary><code>mvnval --mode raw ok/pom.xml</code> (exit 0)</summary>
   
   ```text
   $ mvnval --mode raw ok/pom.xml
   ok/pom.xml: no problems
   exit 0
   ```
   
   </details>
   
   <details>
   <summary><code>mvnval dup/pom.xml</code> (exit 1)</summary>
   
   ```text
   $ mvnval dup/pom.xml
   dup/pom.xml:
     ERROR 'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: junit:junit:jar -> version 4.13.2 vs 4.12 @ line 5, column 1
   exit 1
   ```
   
   </details>
   
   <details>
   <summary><code>mvnval --mode raw dup/pom.xml</code> (exit 1)</summary>
   
   ```text
   $ mvnval --mode raw dup/pom.xml
   dup/pom.xml:
     ERROR 'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: junit:junit:jar -> version 4.13.2 vs 4.12 @ line 5, column 1
   exit 1
   ```
   
   </details>
   
   <details>
   <summary><code>mvnval ghost/pom.xml</code> (exit 1)</summary>
   
   ```text
   $ mvnval ghost/pom.xml
   ghost/pom.xml:
     ERROR Child subproject no-such-dir of <sandbox>/ghost does not exist
   exit 1
   ```
   
   </details>
   
   <details>
   <summary><code>mvnval --mode raw ghost/pom.xml</code> (exit 1)</summary>
   
   ```text
   $ mvnval --mode raw ghost/pom.xml
   ghost/pom.xml:
     ERROR Child subproject no-such-dir of <sandbox>/ghost does not exist
   exit 1
   ```
   
   </details>
   
   <details>
   <summary><code>mvnval ghost/mod/pom.xml</code> (exit 0)</summary>
   
   ```text
   $ mvnval ghost/mod/pom.xml
   ghost/mod/pom.xml: no problems
   exit 0
   ```
   
   </details>
   
   <details>
   <summary><code>mvnval --mode raw ghost/mod/pom.xml</code> (exit 0)</summary>
   
   ```text
   $ mvnval --mode raw ghost/mod/pom.xml
   ghost/mod/pom.xml: no problems
   exit 0
   ```
   
   </details>
   
   <details>
   <summary><code>mvnval broken.xml</code> (exit 1)</summary>
   
   ```text
   $ mvnval broken.xml
   broken.xml:
     FATAL Non-parseable POM <sandbox>/broken.xml: Unable to read model: Unexpected character 'n' (code 110) in prolog; expected '<'
        at [row,col {unknown-source}]: [1,1]
   exit 1
   ```
   
   </details>
   
   <details>
   <summary><code>mvnval --mode raw broken.xml</code> (exit 1)</summary>
   
   ```text
   $ mvnval --mode raw broken.xml
   broken.xml:
     FATAL Non-parseable POM <sandbox>/broken.xml: Unable to read model: Unexpected character 'n' (code 110) in prolog; expected '<'
        at [row,col {unknown-source}]: [1,1]
   exit 1
   ```
   
   </details>
   
   <details>
   <summary><code>mvnval inherit/pom.xml</code> (exit 0)</summary>
   
   ```text
   $ mvnval inherit/pom.xml
   inherit/pom.xml: no problems
   exit 0
   ```
   
   </details>
   
   <details>
   <summary><code>mvnval --mode raw inherit/pom.xml</code> (exit 0)</summary>
   
   ```text
   $ mvnval --mode raw inherit/pom.xml
   inherit/pom.xml: no problems
   exit 0
   ```
   
   </details>
   
   <details>
   <summary><code>mvnval inherit/child/pom.xml</code> (exit 1)</summary>
   
   ```text
   $ mvnval inherit/child/pom.xml
   inherit/child/pom.xml:
     ERROR 'dependencies.dependency.version' for groupId='org.x', artifactId='unmanaged', type='jar' is missing. @ line 4, column 15
   exit 1
   ```
   
   </details>
   
   <details>
   <summary><code>mvnval --mode raw inherit/child/pom.xml</code> (exit 0)</summary>
   
   ```text
   $ mvnval --mode raw inherit/child/pom.xml
   inherit/child/pom.xml: no problems
   exit 0
   ```
   
   </details>
   
   <details>
   <summary><code>mvnval warn/pom.xml</code> (exit 4)</summary>
   
   ```text
   $ mvnval warn/pom.xml
   warn/pom.xml:
     WARNING 'build.plugins.plugin.version' for org.apache.maven.plugins:maven-clean-plugin  is missing. @ line 3, column 17
   exit 4
   ```
   
   </details>
   
   <details>
   <summary><code>mvnval --mode raw warn/pom.xml</code> (exit 0)</summary>
   
   ```text
   $ mvnval --mode raw warn/pom.xml
   warn/pom.xml: no problems
   exit 0
   ```
   
   </details>
   
   <details>
   <summary><code>mvnval netty-all-4.1.115.Final.pom</code> (exit 1)</summary>
   
   ```text
   $ mvnval netty-all-4.1.115.Final.pom
   netty-all-4.1.115.Final.pom:
     ERROR 'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: io.netty:netty-transport-native-epoll:jar:linux-x86_64 -> duplicate declaration of version 4.1.115.Final @ line 1060, column 5
     ERROR 'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: io.netty:netty-transport-native-epoll:jar:linux-aarch_64 -> duplicate declaration of version 4.1.115.Final @ line 1067, column 5
     ERROR 'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: io.netty:netty-transport-native-epoll:jar:linux-riscv64 -> duplicate declaration of version 4.1.115.Final @ line 1074, column 5
     ERROR 'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: io.netty:netty-transport-native-kqueue:jar:osx-x86_64 -> duplicate declaration of version 4.1.115.Final @ line 1081, column 5
     ERROR 'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: io.netty:netty-transport-native-kqueue:jar:osx-aarch_64 -> duplicate declaration of version 4.1.115.Final @ line 1088, column 5
     ERROR 'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: io.netty:netty-resolver-dns-native-macos:jar:osx-x86_64 -> duplicate declaration of version 4.1.115.Final @ line 1095, column 5
     ERROR 'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: io.netty:netty-resolver-dns-native-macos:jar:osx-aarch_64 -> duplicate declaration of version 4.1.115.Final @ line 1102, column 5
     ERROR 'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: io.netty:netty-transport-native-epoll:jar:linux-x86_64 -> duplicate declaration of version 4.1.115.Final @ line 1109, column 5
     ERROR 'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: io.netty:netty-transport-native-epoll:jar:linux-aarch_64 -> duplicate declaration of version 4.1.115.Final @ line 1116, column 5
     ERROR 'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: io.netty:netty-transport-native-epoll:jar:linux-riscv64 -> duplicate declaration of version 4.1.115.Final @ line 1123, column 5
   exit 1
   ```
   
   </details>
   
   <details>
   <summary><code>mvnval --mode raw netty-all-4.1.115.Final.pom</code> (exit 1)</summary>
   
   ```text
   $ mvnval --mode raw netty-all-4.1.115.Final.pom
   netty-all-4.1.115.Final.pom:
     ERROR 'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: io.netty:netty-transport-native-epoll:jar:linux-x86_64 -> duplicate declaration of version 4.1.115.Final @ line 1060, column 5
     ERROR 'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: io.netty:netty-transport-native-epoll:jar:linux-aarch_64 -> duplicate declaration of version 4.1.115.Final @ line 1067, column 5
     ERROR 'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: io.netty:netty-transport-native-epoll:jar:linux-riscv64 -> duplicate declaration of version 4.1.115.Final @ line 1074, column 5
     ERROR 'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: io.netty:netty-transport-native-kqueue:jar:osx-x86_64 -> duplicate declaration of version 4.1.115.Final @ line 1081, column 5
     ERROR 'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: io.netty:netty-transport-native-kqueue:jar:osx-aarch_64 -> duplicate declaration of version 4.1.115.Final @ line 1088, column 5
     ERROR 'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: io.netty:netty-resolver-dns-native-macos:jar:osx-x86_64 -> duplicate declaration of version 4.1.115.Final @ line 1095, column 5
     ERROR 'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: io.netty:netty-resolver-dns-native-macos:jar:osx-aarch_64 -> duplicate declaration of version 4.1.115.Final @ line 1102, column 5
     ERROR 'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: io.netty:netty-transport-native-epoll:jar:linux-x86_64 -> duplicate declaration of version 4.1.115.Final @ line 1109, column 5
     ERROR 'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: io.netty:netty-transport-native-epoll:jar:linux-aarch_64 -> duplicate declaration of version 4.1.115.Final @ line 1116, column 5
     ERROR 'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: io.netty:netty-transport-native-epoll:jar:linux-riscv64 -> duplicate declaration of version 4.1.115.Final @ line 1123, column 5
   exit 1
   ```
   
   </details>
   
   <details>
   <summary><code>mvnval ok/pom.xml dup/pom.xml ghost/pom.xml warn/pom.xml</code> (exit 1)</summary>
   
   ```text
   $ mvnval ok/pom.xml dup/pom.xml ghost/pom.xml warn/pom.xml
   ok/pom.xml: no problems
   dup/pom.xml:
     ERROR 'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: junit:junit:jar -> version 4.13.2 vs 4.12 @ line 5, column 1
   ghost/pom.xml:
     ERROR Child subproject no-such-dir of <sandbox>/ghost does not exist
   warn/pom.xml:
     WARNING 'build.plugins.plugin.version' for org.apache.maven.plugins:maven-clean-plugin  is missing. @ line 3, column 17
   exit 1
   ```
   
   </details>
   
   <details>
   <summary><code>mvnval --mode raw --format json dup/pom.xml</code> (exit 1)</summary>
   
   ```text
   $ mvnval --mode raw --format json dup/pom.xml
   [{"pom":"<sandbox>/dup/pom.xml","problems":[{"severity":"ERROR","message":"'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: junit:junit:jar -> version 4.13.2 vs 4.12","line":5,"column":1}]}]
   exit 1
   ```
   
   </details>
   
   <details>
   <summary><code>RUNNER_DEBUG=1 mvnval --mode raw --format json dup/pom.xml</code> (exit 1)</summary>
   
   ```text
   $ RUNNER_DEBUG=1 mvnval --mode raw --format json dup/pom.xml
   [{"pom":"<sandbox>/dup/pom.xml","problems":[{"severity":"ERROR","message":"'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: junit:junit:jar -> version 4.13.2 vs 4.12","line":5,"column":1}]}]
   exit 1
   ```
   
   </details>
   
   <details>
   <summary><code>mvnval -o inherit/child/pom.xml</code> (exit 2)</summary>
   
   ```text
   $ mvnval -o inherit/child/pom.xml
   [ERROR] --offline (-o) needs --mode raw, which reaches no repository.
   exit 2
   ```
   
   </details>
   
   <details>
   <summary><code>mvnval --mode raw -o inherit/child/pom.xml</code> (exit 0)</summary>
   
   ```text
   $ mvnval --mode raw -o inherit/child/pom.xml
   inherit/child/pom.xml: no problems
   exit 0
   ```
   
   </details>
   
   <details>
   <summary><code>mvnval --mode bogus ok/pom.xml</code> (exit 2)</summary>
   
   ```text
   $ mvnval --mode bogus ok/pom.xml
   [ERROR] Unknown mode 'bogus', expected one of: raw, effective
   exit 2
   ```
   
   </details>
   
   <details>
   <summary><code>mvnval --format yaml ok/pom.xml</code> (exit 2)</summary>
   
   ```text
   $ mvnval --format yaml ok/pom.xml
   [ERROR] Unknown format 'yaml', expected one of: text, json
   exit 2
   ```
   
   </details>
   
   <details>
   <summary><code>mvnval missing-pom.xml</code> (exit 1)</summary>
   
   ```text
   $ mvnval missing-pom.xml
   missing-pom.xml: does not exist
   exit 1
   ```
   
   </details>
   
   <details>
   <summary><code>mvnval ok</code> (exit 1)</summary>
   
   ```text
   $ mvnval ok
   ok: is a directory, not a POM file
   exit 1
   ```
   
   </details>
   
   <details>
   <summary><code>cd ok &amp;&amp; mvnval</code> (exit 0)</summary>
   
   ```text
   $ cd ok && mvnval
   pom.xml: no problems
   exit 0
   ```
   
   </details>
   
   <details>
   <summary><code>mvnval ok/pom.xml ./ok/pom.xml</code> (exit 0)</summary>
   
   ```text
   $ mvnval ok/pom.xml ./ok/pom.xml
   ok/pom.xml: no problems
   exit 0
   ```
   
   </details>
   
   <details>
   <summary><code>mvnval --mode raw --temp-local-repository ok/pom.xml</code> (exit 0)</summary>
   
   ```text
   $ mvnval --mode raw --temp-local-repository ok/pom.xml
   ok/pom.xml: no problems
   exit 0
   ```
   
   </details>
   
   <details>
   <summary><code>mvn --val --mode raw dup/pom.xml</code> (exit 1)</summary>
   
   ```text
   $ mvn --val --mode raw dup/pom.xml
   dup/pom.xml:
     ERROR 'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: junit:junit:jar -> version 4.13.2 vs 4.12 @ line 5, column 1
   exit 1
   ```
   
   </details>
   
   Repository options: the three explicit-path runs each fetched `org/apache/apache/35/apache-35.pom` into the requested directory. For each temporary run, I observed one `mvnval-repo-*` directory while the process was running and none after it exited.
   
   <details>
   <summary><code>mvnval --local-repository repo-probe/cli-repo repo-probe/pom.xml</code> (exit 0)</summary>
   
   ```text
   $ mvnval --local-repository repo-probe/cli-repo repo-probe/pom.xml
   [INFO] Loaded 23932 auto-discovered prefixes for remote repository central (prefixes-central.txt)
   repo-probe/pom.xml: no problems
   exit 0
   ```
   
   Checked: the parent POM was present in the requested repository.
   
   </details>
   
   <details>
   <summary><code>mvnval -Dmaven.repo.local=repo-probe/property-repo repo-probe/pom.xml</code> (exit 0)</summary>
   
   ```text
   $ mvnval -Dmaven.repo.local=repo-probe/property-repo repo-probe/pom.xml
   [INFO] Loaded 23932 auto-discovered prefixes for remote repository central (prefixes-central.txt)
   repo-probe/pom.xml: no problems
   exit 0
   ```
   
   Checked: the parent POM was present in the requested repository.
   
   </details>
   
   <details>
   <summary><code>mvnval -Dmaven.repo.local=repo-probe/unused-property-repo --local-repository repo-probe/priority-repo repo-probe/pom.xml</code> (exit 0)</summary>
   
   ```text
   $ mvnval -Dmaven.repo.local=repo-probe/unused-property-repo --local-repository repo-probe/priority-repo repo-probe/pom.xml
   [INFO] Loaded 23932 auto-discovered prefixes for remote repository central (prefixes-central.txt)
   repo-probe/pom.xml: no problems
   exit 0
   ```
   
   Checked: the parent POM was present in the requested repository.
   
   Checked: the unused repository path was not created.
   
   </details>
   
   <details>
   <summary><code>mvnval --temp-local-repository repo-probe/pom.xml</code> (exit 0)</summary>
   
   ```text
   $ mvnval --temp-local-repository repo-probe/pom.xml
   [INFO] Loaded 23932 auto-discovered prefixes for remote repository central (prefixes-central.txt)
   repo-probe/pom.xml: no problems
   exit 0
   ```
   
   Checked: the temporary directory existed during the run and was gone afterward.
   
   </details>
   
   <details>
   <summary><code>mvnval -Dmaven.repo.local=repo-probe/unused-temp-property-repo --temp-local-repository repo-probe/pom.xml</code> (exit 0)</summary>
   
   ```text
   $ mvnval -Dmaven.repo.local=repo-probe/unused-temp-property-repo --temp-local-repository repo-probe/pom.xml
   [INFO] Loaded 23932 auto-discovered prefixes for remote repository central (prefixes-central.txt)
   repo-probe/pom.xml: no problems
   exit 0
   ```
   
   Checked: the temporary directory existed during the run and was gone afterward.
   
   Checked: the unused repository path was not created.
   
   </details>
   
   <details>
   <summary><code>mvnval --local-repository repo-probe/cli-repo --temp-local-repository repo-probe/pom.xml</code> (exit 2)</summary>
   
   ```text
   $ mvnval --local-repository repo-probe/cli-repo --temp-local-repository repo-probe/pom.xml
   [ERROR] --local-repository and --temp-local-repository name different directories; give one.
   exit 2
   ```
   
   </details>
   
   <details>
   <summary><code>mvnval --mode raw --local-repository repo-probe/raw-ignored-repo repo-probe/pom.xml</code> (exit 0)</summary>
   
   ```text
   $ mvnval --mode raw --local-repository repo-probe/raw-ignored-repo repo-probe/pom.xml
   repo-probe/pom.xml: no problems
   exit 0
   ```
   
   Checked: the unused repository path was not created.
   
   </details>


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MNG-8650] mvnval, a tool to validate POMs without building them [maven]

["gnodet (via GitHub)" <gi...@apache.org>]

gnodet merged PR #13276:
URL: https://github.com/apache/maven/pull/13276


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MNG-8650] mvnval, a tool to validate POMs without building them [maven]

["gnodet (via GitHub)" <gi...@apache.org>]

gnodet commented on PR #13276:
URL: https://github.com/apache/maven/pull/13276#issuecomment-6061586781

   The maven-site documentation is ready: https://github.com/apache/maven-site/pull/1705
   
   Adds:
   - `content/markdown/tools/mvnval.md` — full page covering modes (raw vs effective), options, exit codes, output formats, and integration
   - `content/markdown/tools/index.md` — adds `mvnval` to the tools table


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MNG-8650] mvnval, a tool to validate POMs without building them [maven]

["gnodet-bot (via GitHub)" <gi...@apache.org>]

gnodet-bot commented on code in PR #13276:
URL: https://github.com/apache/maven/pull/13276#discussion_r4116080684


##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/ModelBuilder.java:
##########
@@ -38,6 +38,33 @@ public interface ModelBuilder extends Service {
     interface ModelBuilderSession {
 
         ModelBuilderResult build(ModelBuilderRequest request) throws ModelBuilderException;
+
+        /**
+         * Reads and validates the raw model, without inheritance, interpolation or profile injection.
+         * <p>
+         * Unlike {@link #build(ModelBuilderRequest)} this does not throw when validation reports
+         * errors: the result carries errors and warnings alike, so a caller that only wants to
+         * report on a POM can see warnings that come with no error. A problem severe enough that
+         * the model cannot be read at all is still thrown.
+         * <p>
+         * Reading one raw model can need others: a subproject may leave out its parent version,
+         * which is then taken from the parent's own file model. An implementation may therefore
+         * read files besides the one the request names, up to the project root. Nothing is
+         * resolved from a repository and nothing is downloaded.
+         * <p>
+         * Only the source, the file model, the raw model and the problems are populated on the
+         * result. The accessors that describe a fully built model, such as
+         * {@link ModelBuilderResult#getEffectiveModel()}, are not.
+         *
+         * @param request the request containing the parameters for reading the model
+         * @return the result, carrying the raw model and the problems collected
+         * @throws ModelBuilderException if the model cannot be read at all
+         * @throws UnsupportedOperationException if this implementation does not support it
+         * @since 4.1.0
+         */
+        default ModelBuilderResult buildRaw(ModelBuilderRequest request) throws ModelBuilderException {
+            throw new UnsupportedOperationException(getClass().getName() + " does not support reading a raw model");
+        }

Review Comment:
   💡 **API default throws `UnsupportedOperationException`** — the test `shouldRejectBuildRawWhenAnImplementationDoesNotSupportIt` covers this, but the Javadoc says `@throws UnsupportedOperationException if this implementation does not support it` which is correct. Worth noting: any `ModelBuilderSession` proxy or decorator (e.g. in IDE tooling) that only overrides `build()` will now silently fail on `buildRaw()` with an `UnsupportedOperationException` rather than a compilation error. This is fine for a `@since 4.1.0` addition — it can't break existing code — but consumers should be aware.



##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelBuilder.java:
##########
@@ -1053,14 +1097,17 @@ private void loadFromRoot(Path root, Path top) {
                 loadFilePom(executor, top, root, Set.of(), r);
             }
             if (result.getFileModel() == null && !Objects.equals(top, root)) {
-                logger.warn(
-                        "The top project ({}) cannot be found in the reactor from root project ({}). "
-                                + "Make sure the root directory is correct (a missing '.mvn' directory in the root "
-                                + "project is the most common cause) and the project is correctly included "
-                                + "in the reactor (missing activated profiles, command line options, etc.). For this "
-                                + "build, the top project will be used as the root project.",
-                        top,
-                        root);
+                // Only a warning when a build is what was asked for. A caller reading one POM has
+                // no reactor to get wrong, and the retry below is routine there, not a misconfiguration.
+                (reportReactorLoadFailures ? logger.atWarn() : logger.atDebug())
+                        .log(
+                                "The top project ({}) cannot be found in the reactor from root project ({}). "
+                                        + "Make sure the root directory is correct (a missing '.mvn' directory in the root "
+                                        + "project is the most common cause) and the project is correctly included "
+                                        + "in the reactor (missing activated profiles, command line options, etc.). For this "
+                                        + "build, the top project will be used as the root project.",

Review Comment:
   ⚠️ **`reportReactorLoadFailures` is a mutable field on `ModelBuilderSessionState` but set from `loadReactor` on the calling session** — When `loadReactor` calls `loadFromRoot` which spawns child sessions via `derive()`, the child sessions inherit the initial `reportReactorLoadFailures = true` from the constructor. The field is only set on `this` (the parent session), but the `loadFilePom` catch block at line 1205 checks `reportReactorLoadFailures` on whatever session instance is executing. Since `loadFilePom` is called on the parent session (not derived ones), this works correctly, but the coupling between the field lifecycle and the method call chain is fragile — a refactoring that moves `loadFilePom` to a derived session would silently break the error suppression.



##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnval/ValidateInvoker.java:
##########
@@ -0,0 +1,197 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.cling.invoker.mvnval;
+
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.function.Consumer;
+
+import org.apache.maven.api.Session;
+import org.apache.maven.api.annotations.Nullable;
+import org.apache.maven.api.cli.InvokerRequest;
+import org.apache.maven.api.cli.mvnval.ValidateOptions;
+import org.apache.maven.api.services.Lookup;
+import org.apache.maven.api.services.ModelBuilder;
+import org.apache.maven.api.services.ModelBuilderException;
+import org.apache.maven.api.services.ModelBuilderRequest;
+import org.apache.maven.api.services.ModelBuilderResult;
+import org.apache.maven.api.services.ModelProblem;
+import org.apache.maven.api.services.Sources;
+import org.apache.maven.cling.invoker.CoreExtensionSelector;
+import org.apache.maven.cling.invoker.LookupContext;
+import org.apache.maven.cling.invoker.LookupInvoker;
+import org.apache.maven.impl.standalone.ApiRunner;
+
+/**
+ * Validates POM files without building them.
+ * <p>
+ * Validation stops after {@link ModelBuilder.ModelBuilderSession#buildRaw}, so no parent is
+ * resolved and the verdict depends on the files alone, never on the network. The problems it can
+ * reach are therefore a subset: a dependency whose version comes from the parent's
+ * {@code dependencyManagement} is only checked later, in {@code validateEffectiveModel}.
+ */
+public class ValidateInvoker extends LookupInvoker<ValidateContext> {
+
+    public static final int OK = 0;
+    public static final int ERROR = 1;
+
+    /** Bad user input, as in {@code mvnenc} and {@code mvnup}. */
+    public static final int BAD_OPERATION = 2;
+
+    /**
+     * Nothing was rejected, but something was reported. Deliberately not 2: the CLI itself exits
+     * with 2 when a tool fails in a way it does not handle, and a gate must be able to tell "this
+     * POM has warnings" from "mvnval broke".
+     */
+    public static final int WARNINGS = 3;
+
+    public ValidateInvoker(Lookup protoLookup, @Nullable Consumer<LookupContext> contextConsumer) {
+        super(protoLookup, contextConsumer);
+    }
+
+    @Override
+    protected ValidateContext createContext(InvokerRequest invokerRequest) {
+        return new ValidateContext(
+                invokerRequest, (ValidateOptions) invokerRequest.options().orElse(null));
+    }
+
+    @Override
+    protected int execute(ValidateContext context) throws Exception {
+        OutputFormat format;
+        try {
+            // The parser checks --format too, but options are interpolated after parsing, so
+            // ${...} can still turn into anything by the time it gets here.
+            format = context.options().format().map(OutputFormat::parse).orElse(OutputFormat.TEXT);
+        } catch (IllegalArgumentException e) {
+            context.logger.error(e.getMessage());
+            return BAD_OPERATION;
+        }
+
+        List<Report> reports = validate(resolvePoms(context), createSession());

Review Comment:
   ❓ **`determineWriter` vs `context.writer`** — The comment says `determineWriter, not context.writer: that field is lazy and nothing on this path has created it yet.` But in `ValidateInvokerTest`, the test directly sets `context.writer = output::add` and calls `invoker.execute(context)`. This works because the test bypasses the lazy initialization.
   
   Is there a risk that `determineWriter(context)` returns something different from what the test expects? Looking at the implementation, `determineWriter` returns `context.writer` if non-null, falling back to `System.out::println`. Since the test sets `context.writer` before calling `execute`, the comment is slightly misleading — `determineWriter` will find `context.writer` already set by the test. The comment is accurate for production usage though, where nothing sets `context.writer` before this point.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MNG-8650] mvnval, a tool to validate POMs without building them [maven]

["ikxeno (via GitHub)" <gi...@apache.org>]

ikxeno commented on PR #13276:
URL: https://github.com/apache/maven/pull/13276#issuecomment-5858486367

   > Where will this be documented? In which files/URLs specifically? If they're in this repo, they must be part of this PR. If they're in maven-site there should be a concurrent PR with those changes too.
   
   In this repo the only file documenting the tool set is `api/maven-api-cli/src/site/markdown/index.md`, which this PR updates with an mvnval entry next to `mvnup`. `--help` carries the usage, the exit codes and what the check can't see.
   
   Past that I couldn't find a page for `mvnup` or `mvnenc` on maven.apache.org, so I don't have a sibling to copy. If it belongs in maven-site, say where and I'll open a PR there alongside this one. If you'd rather it lived here, point me at the file.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MNG-8650] mvnval, a tool to validate POMs without building them [maven]

["ikxeno (via GitHub)" <gi...@apache.org>]

ikxeno commented on code in PR #13276:
URL: https://github.com/apache/maven/pull/13276#discussion_r4121722279


##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnval/JsonReporter.java:
##########
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.cling.invoker.mvnval;
+
+import java.util.List;
+import java.util.function.Consumer;
+import java.util.stream.Collectors;
+
+import org.apache.maven.api.annotations.Nullable;
+import org.apache.maven.api.services.ModelProblem;
+
+/**
+ * Renders validation results as a JSON array, one object per validated file. Assembled by hand
+ * so the tool needs no dependency the distribution does not already carry.
+ */
+class JsonReporter implements Reporter {
+
+    private static final char[] HEX = "0123456789abcdef".toCharArray();
+
+    private static final char FIRST_PRINTABLE_ASCII = 0x20;
+    private static final char LAST_PRINTABLE_ASCII = 0x7e;
+
+    @Override
+    public void report(List<Report> reports, Consumer<String> out) {
+        out.accept(reports.stream().map(JsonReporter::toJson).collect(Collectors.joining(",", "[", "]")));
+    }
+
+    private static String toJson(Report report) {
+        StringBuilder sb =
+                new StringBuilder("{\"pom\":").append(quote(report.pom().toString()));
+        if (report.failure() != null) {
+            sb.append(",\"failure\":").append(quote(report.failure()));
+        }
+        return sb.append(",\"problems\":")
+                .append(report.problems().stream()
+                        .map(problem -> toJson(problem, report))
+                        .collect(Collectors.joining(",", "[", "]")))
+                .append('}')
+                .toString();
+    }
+
+    private static String toJson(ModelProblem problem, Report report) {
+        StringBuilder sb = new StringBuilder("{\"severity\":")
+                .append(quote(problem.getSeverity().name()))
+                .append(",\"message\":")
+                .append(quote(problem.getMessage()));
+        // Absent values are left out, not written as the -1 the model builder uses for "no location".
+        String source = report.sourceOf(problem);
+        if (source != null) {
+            sb.append(",\"source\":").append(quote(source));
+        }
+        if (problem.getLineNumber() > 0) {
+            sb.append(",\"line\":").append(problem.getLineNumber());
+        }
+        if (problem.getColumnNumber() > 0) {
+            sb.append(",\"column\":").append(problem.getColumnNumber());
+        }
+        return sb.append('}').toString();
+    }
+
+    private static String quote(@Nullable String value) {

Review Comment:
   Taken, thanks, addbec6. The builder only turns up once a character actually needs replacing now.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MNG-8650] mvnval, a tool to validate POMs without building them [maven]

["ikxeno (via GitHub)" <gi...@apache.org>]

ikxeno commented on PR #13276:
URL: https://github.com/apache/maven/pull/13276#issuecomment-5869748746

   @gnodet 
   
   Two things I'd like you to decide.
   
   Should `validate()` read `RequestType`? `BUILD_PROJECT` raw as now,
   `BUILD_EFFECTIVE` running `validateEffectiveModel` and collecting the problems
   rather than throwing. Then `--depth` is just the request, and there is never a
   second method. I didn't do it on my own because nothing in the enum means "stop at
   the raw model" today, and both branches of `build()` run to the end.
   
   And can `--depth` default to `raw`? With `effective` as the default the offline
   cases die: no repository, and no `settings.xml` either, so no mirrors or proxies.
   There is also this. Once anything resolves, the `<repositories>` of the POM being
   validated are part of where it resolves from, `DefaultModelBuilder` near 699 and
   731. A gate in front of Central would be letting untrusted input pick the URLs it
   fetches. As an opt-in flag I'll add it today.
   
   The rest is in, adf8539 and 6f09770. Renamed, cache clear gone, one session for
   the run.
   
   Two things I got wrong. Sharing a session doesn't trip `No unique Source` when two
   roots share coordinates; I claimed it did in the description, and there is a test
   now. And the parent still isn't read once:
   
   ```
   $ mvnval -X a/pom.xml b/pom.xml c/pom.xml | grep 'Reading file model' | sort | uniq -c
      3 a/pom.xml   3 b/pom.xml   3 c/pom.xml   3 pom.xml
   ```
   
   Same numbers with a session per POM, because every `validate()` walks the reactor
   from the root again. If you want the batch case cheaper, that's the walk, not the
   session.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MNG-8650] mvnval, a tool to validate POMs without building them [maven]

["ikxeno (via GitHub)" <gi...@apache.org>]

ikxeno commented on PR #13276:
URL: https://github.com/apache/maven/pull/13276#issuecomment-6057220803

   @gnodet
   
   Thanks for your work on this. I've pushed `fd9b90e47d` on top of your changes. It fixes the Windows path assertion and explicitly selects `--mode raw` in the three existing mvnval ITs.
   
   Could you please approve the CI run for the updated commit?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org