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