[MNG-6797] Remember if Maven model problems were encountered.

[wilx]

Retain the model problems discovered during project collection in MavenSession.getModelProblems().

Plugins can inspect the problem details and reject builds with model warnings, as in the proposed Enforcer rule.

Integration tests: apache/maven-integration-testing#442. All four scenarios (clean models, warnings, quiet logging, and reactor-child problems) passed against core commit ab914f59e3.

See also MNG-6797.

Following this checklist to help us incorporate your
contribution quickly and easily:

• Make sure there is a JIRA issue filed
  for the change (usually before you start working on it). Trivial changes like typos do not
  require a JIRA issue. Your pull request should address just this issue, without
  pulling in other changes.
• Each commit in the pull request should have a meaningful subject line and body.
• Format the pull request title like [MNG-XXX] - Fixes bug in ApproximateQuantiles,
  where you replace MNG-XXX with the appropriate JIRA issue. Best practice
  is to use the JIRA issue title in the pull request title and in the first line of the
  commit message.
• Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• Run mvn clean verify to make sure basic checks pass. A more thorough check will
  be performed on your pull request automatically.
• You have run the Core IT successfully.

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

• I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004

• In any other case, please file an Apache Individual Contributor License Agreement.

[michael-o]

As fas as I understand you want to memorize state. Can you provide an IT why this is necessary/benefit?

I also fail to see how problems are detected, you only changed the signature of the method.

[michael-o]

That really hurts because the plural implies that you have a collection of problem, and not a boolean value. Moreover, third person singular with a plural reads horribly.

[wilx]

As fas as I understand you want to memorize state. Can you provide an IT why this is necessary/benefit?

IME as someone who maintains Maven projects, people often introduce issues like that and they do not care to fix them unless their build breaks. Remembering in MavenSession that there were problems with the Maven model will allow me to write Maven Enforcer plugin rule that will read that and will fail the build.

I also fail to see how problems are detected, you only changed the signature of the method.

See org.apache.maven.graph.DefaultGraphBuilder#collectProjects. It prints the collected Maven model problems.

That really hurts because the plural implies that you have a collection of problem, and not a boolean value. Moreover, third person singular with a plural reads horribly.

I am open to suggestions for better name. I have only propagated the problems variable name upwards. One thing, I would like to keep the isX or getX for the getter as that would allow me, I believe, to use something like this ReflectionValueExtractor.evaluate("problems", session, false); to retrieve it in the Enforce rule easily, I hope.

[jira-importer]

Resolve #8485

[gnodet]

AI Code Review — PR #298

Verdict: ⚠️ Critically stale — needs complete rewrite

This PR adds a problems boolean to MavenSession to track whether model problems were encountered during project collection (MNG-6797). The intent is to let plugins like the Enforcer fail builds on model warnings.

However, the PR is nearly 7 years old and the codebase has been fundamentally restructured since it was opened.

Findings

#	Severity	File	Details
1	🔴 High	DefaultGraphBuilder.java	Method deleted — The collectProjects(List, List, MavenExecutionRequest) method this PR modifies no longer exists on master. It was refactored in MNG-7646 to delegate to strategy classes (PomlessCollectionStrategy, MultiModuleCollectionStrategy, RequestPomCollectionStrategy). The PR cannot merge.
2	🔴 High	MavenSession.java	File moved — Path changed from maven-core/ to impl/maven-core/ by MNG-8346. Merge state is CONFLICTING.
3	🟡 Medium	MavenSession.java:270	Naming — isProblems() is grammatically awkward. The Java convention for presence semantics is hasProblems() or hasModelProblems().

Recommendation

If MNG-6797 is still relevant:

1. Check whether the issue has been addressed by Maven 4's structured problem reporting (ProblemCollector<BuilderProblem>, ModelBuilderException)
2. If not, a fresh implementation would need to integrate with the current strategy-based project collection and possibly leverage the Maven 4 problem APIs
3. The original reviewer (michael-o) requested an integration test, which was never provided

The PR in its current form cannot be merged and would require a complete rewrite.

---

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of Guillaume Nodet

[gnodet]

Code Review

This nearly 7-year-old PR (opened November 2019) cannot merge in its current form. The target code has been both relocated and fundamentally refactored.

1. Target code no longer exists (high)

The collectProjects(List<MavenProject>, List<File>, MavenExecutionRequest) method in DefaultGraphBuilder was refactored in MNG-7646 — project collection now delegates to strategy classes (PomlessCollectionStrategy, MultiModuleCollectionStrategy, RequestPomCollectionStrategy). The code this PR modifies is gone.

2. Files relocated (high)

Both files moved from maven-core/ to impl/maven-core/ by MNG-8346. Merge state is CONFLICTING.

3. Naming issue unaddressed (medium)

isProblems() was flagged by reviewer michael-o as reading awkwardly. hasModelProblems() or hasProblems() would follow Java bean conventions. Acknowledged but never fixed.

4. Missing integration test (medium)

michael-o requested an IT demonstrating the benefit. Never provided.

Recommendation

If MNG-6797 is still relevant, a fresh implementation is needed against the current strategy-based architecture, ideally leveraging Maven 4's structured problem reporting (ProblemCollector, ModelBuilderException) rather than a bare boolean flag on MavenSession.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet

[wilx]

I forgot I even did this. Rebased.

[wilx]

https://github.com/wilx/maven/actions/runs/30470388455/job/90640967247 looks like infrastructure problem.

[elharo]

Ping @michael-o Current thoughts about this one?

[michael-o]

The rebase fine. @wilx Can you please briefly summarize how you want to put this into use?

[wilx]

The rebase fine. @wilx Can you please briefly summarize how you want to put this into use?

I want to create a Maven Enforcer rule that will inspect the presence of the model problems and will fail the build if there are problems.

[michael-o]

The rebase fine. @wilx Can you please briefly summarize how you want to put this into use?

I want to create a Maven Enforcer rule that will inspect the presence of the model problems and will fail the build if there are problems.

Sounds reasonable.

[elharo]

a boolean flag seems insufficient for this purpose. The model problems themselves should be exposed to plugins that want to read them. I'd prefer to avoid half-measures. Is there a more complete approach possible here?

[wilx]

a boolean flag seems insufficient for this purpose. The model problems themselves should be exposed to plugins that want to read them. I'd prefer to avoid half-measures. Is there a more complete approach possible here?

A boolean is sufficient to implement the Enforcer rule.

I do not have a use case for it but I agree that having access to the problem items might be useful. However, because I want to be able to use the Enforcer rules both with 3.10.x and 4.x Maven, there are some design problems:

• Maven has org.apache.maven.model.building.ModelProblem. But the interface and related classes are deprecated in 4.x. I am sure introducing more uses of deprecated API is no go for 4.x branches.
• OTOH, porting the 4.x problem gathering code to 3.x also seems like something that would be rejected. It is a lot of code.
• Introducing 3rd common representation is recreation of the XKCD meme.

What else could we do instead?

[elharo]

Is org.apache.maven.model.building.ModelProblem not deprecated in 3.x? If so this might need different code in 3.x and 4.x, not a simple backport. What is the replacement for ModelProblem in 4.x?

[gnodet-bot]

Code Review — #298

The PR correctly targets maven-3.10.x where both MavenSession.java and DefaultGraphBuilder.java still live under maven-core/ and collectProjects() still exists in its current form — so prior reviews claiming the code was gone were wrong (those reviewed against master, not the 3.10.x target branch). The approach here is sound in principle.

Two issues need to be addressed before merge:

1. Pomless build path never sets modelProblems (medium)

getProjectsForMavenReactor() has two code paths. The PR only instruments the POM path:

// POM path — correctly instrumented
List<File> files = Arrays.asList(request.getPom().getAbsoluteFile());
boolean problems = collectProjects(projects, files, request);
session.setModelProblems(problems);
return projects;

The pomless early-return path (request.getPom() == null) falls through without ever calling setModelProblems(), leaving the session field at false. In practice this is a niche edge case (pomless builds use a bundled standalone POM), but if that standalone project ever has problems they'd be silently missed. Add session.setModelProblems(false) before the early return projects;, or better, hoist the setModelProblems() call to the common return point after restructuring to avoid two returns.

2. Design: setModelProblems(false) can reset a true value (low — fixable with ||=)

setModelProblems(boolean) is a direct setter — it can overwrite a previously-set true if called multiple times. In the current call graph this is safe (one call per session build), but the API is misleading. Prefer an additive setter:

public void addModelProblems() {
    this.modelProblems = true;
}

or at minimum document the single-call contract explicitly. As-is, a plugin that calls session.setModelProblems(false) by mistake can silently clear the flag.

Existing review findings — status check

• michael-o (CHANGES_REQUESTED): requested an integration test. The added unit test covers the flag-setting logic adequately for this change. Whether a full IT is still required is a policy call for the committers, but the unit test is a meaningful improvement over nothing.
• elharo (CHANGES_REQUESTED): "a boolean flag seems insufficient — expose the model problems themselves." Still unaddressed. The author's argument (API compatibility between 3.x ModelProblem and deprecated 4.x equivalents) is valid. The boolean approach works for the stated Enforcer use case. Committers should decide whether this is a blocker.

What's correct in this PR

• The fix to decouple problems = true from logger.isWarnEnabled() in collectProjects() is a genuine correctness improvement from the original code — problems were previously silently missed when warn logging was disabled.
• Javadoc on the new methods is accurate and well-formed.
• @since 3.10.0 tags are appropriate.
• The unit test covers both the positive and negative cases.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

⚠️ Pomless path skipped: the early-return branch for request.getPom() == null (around line 333) never calls setModelProblems(), leaving the session at false even if the standalone project has problems. This line is only reached when a POM file is present.

Add session.setModelProblems(false) before the early return projects; in the pomless branch, or restructure to have a single return point so setModelProblems() is always called.

[gnodet-bot]

⚠️ Resettable flag: setModelProblems(boolean) accepts false, meaning it can clear a previously-set true. The API contract should either be documented as "call at most once per build" or changed to an additive setter:

Suggested change

	public RepositorySystemSession getRepositorySession() {
	public void setModelProblems() {
	this.modelProblems = true;
	}

This makes the flag monotonic and prevents accidental resets by callers passing false.

[wilx]

I have a change that I forgot to push that exposes the list of the problems. However, the code and data structures are different in 4.x and 3.x. So this would require split of or at least knowledge by the Enforcer rule to accommodate both.

[gnodet-bot]

Re-review — #298 (commit ab914f5)

The upgrade from boolean to List<ModelProblem> directly addresses @elharo's CHANGES_REQUESTED concern — the full problem list is now exposed, not just a flag. That is a meaningful improvement and closes the most significant blocker from the previous round.

Two issues remain:

1. Pomless build path still never calls setModelProblems() (medium — same gap as before)

From my previous review: the request.getPom() == null early-return branch in getProjectsForMavenReactor() returns without calling setModelProblems(), leaving modelProblems as Collections.emptyList() even if the standalone project has problems.

The new commits did not touch that branch. The fix is straightforward:

// We have no POM file.
//
if (request.getPom() == null) {
    ModelSource modelSource = new UrlModelSource(DefaultMaven.class.getResource("project/standalone.xml"));
    MavenProject project = projectBuilder
            .build(modelSource, request.getProjectBuildingRequest())
            .getProject();
    project.setExecutionRoot(true);
    projects.add(project);
    request.setProjectPresent(false);
    session.setModelProblems(Collections.emptyList()); // explicit — pomless builds have no model problems
    return projects;
}

(Or restructure to a single return point.)

2. Test gap: pomless path not covered (low)

The three new tests in DefaultGraphBuilderTest all go through the POM path (request.setPom(new File("pom.xml"))). None exercises request.getPom() == null. Given that finding #1 is specifically about that branch, a test reproducing it would be valuable.

Prior findings — status

• Previous gnodet-bot finding 1 (pomless path): not addressed — re-raised above.
• Previous gnodet-bot finding 2 (resettable setter): resolved by the List<ModelProblem> API change. Passing null now throws NPE (documented), and the defensive copy prevents external mutation. ✅
• @elharo (CHANGES_REQUESTED) — "boolean flag insufficient": addressed by exposing the full List<ModelProblem>. ✅
• @michael-o (CHANGES_REQUESTED) — requested an integration test. The new unit tests are a meaningful addition; whether a full Maven IT is still required is a committer policy decision.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

⚠️ Pomless path above this line still never calls setModelProblems().

The request.getPom() == null branch (lines ~333–341) returns early without setting model problems on the session. Since modelProblems defaults to Collections.emptyList(), a pomless build that encounters problems silently exposes an empty list. Add session.setModelProblems(Collections.emptyList()) before that early return projects; (an empty list is correct for the standalone POM case — it only needs to be an explicit call to make the intent clear and to make the contract testable).

[wilx]

Do we need this? Nobody can use Enforcer from the default POM in POMless execution, AFAIK.

[gnodet-bot]

You're right — withdrawing that finding. In a pomless execution, the build uses Maven's internal standalone POM (DefaultMaven.class.getResource("project/standalone.xml")), which is a fixed resource. Since Enforcer rules must be configured in the project POM, there is no way to configure an Enforcer rule in a pomless build. The pomless path can never trigger the proposed Enforcer rule, making an explicit setModelProblems() call there purely academic. The remaining open issue (from my last review) is the test coverage gap for that branch, but given the use case doesn't exist, even that is low priority.

This comment was generated by an AI agent, Hermès on behalf of @gnodet.

[wilx]

Prototype rule: apache/maven-enforcer#1030

[cstamas]

Whoever holds "req for change" on this PR, pls re-review. I scheduled it for 3.10.

[gnodet]

We'd need a similar PR for the Maven 4 API.