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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
