Enh: Allow validator factory to abstain by cstamas · Pull Request #2008 · apache/maven-resolver

cstamas · 2026-07-22T10:01:50Z

Changes:

allow ValidatorFactory to "abstain" and return NOOP
introduce alt execution path for managed dependencies (use cases varies)
cache validators per session

Fixes: #2007

Also, cache validators across single session. Fixes: apache#2007

Revert the managed dependency validation removal from DefaultRepositorySystemValidator. As cstamas noted, managed deps should remain validatable for non-Maven use cases — the proper separation between direct and managed dependency validation belongs in PR #2008 (validateManagedDependency method). This PR now focuses solely on session-scoped re-entrancy detection via an AtomicInteger depth counter in session data, which covers the broken trace chain scenario independently. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

elharo

Needs tests

Is there a better signal than "null" for abstention that doesn't risk NPEs? Abstention seems equivalent to approval unless something requires some validator to approve?

elharo · 2026-07-23T13:20:28Z

+     * that are never actually used. If a managed dependency IS matched and its coordinates are invalid, the error
+     * will surface naturally during version resolution or artifact resolution.
+     *
+     * @param managedDependency The managed dependency to validate, never {@code null}.


nit: begin with lower case the
no periods at end

per Oracle guidelines

While I agree with this sentiment, this javadoc method is not the single one in this PR not following that, and in this codebase this is not the single one either (in fact, whole codebase follows this style). I'd leave this change to some time, and IMO the change should be done consistently across whole codebase.

Hm, was wrong: lowercase stands but not the "no period at end"?

gnodet

The overall design of the three features (abstain, managed-dependency differentiation, caching) is sound and addresses real needs. The separate validateManagedDependency path correctly avoids false positives from uninterpolated BOM property expressions — well-documented rationale.

Two implementation issues in the caching logic:

Null values not cached: ConcurrentHashMap.computeIfAbsent does not store entries when the mapping function returns null. When a factory abstains (returns null), the factory is re-invoked on every subsequent validation call within the same session — contradicting the Javadoc claim that "even nulls are cached." Fix: use a sentinel object pattern (e.g., private static final Validator ABSENT = new Validator() {};) and unwrap on retrieval.
System.identityHashCode as map key: System.identityHashCode is not guaranteed unique across distinct object instances. Hash collisions cause the second factory's newInstance() to never be called — it silently reuses the first factory's Validator. Since Integer.equals compares by value, two different factory instances with the same identityHashCode map to the same key. Fix: use the factory instance itself as the key (ConcurrentHashMap<ValidatorFactory, Validator>), which relies on reference equality via Object.equals.

Missing test coverage: No tests cover the three new behaviors — factory returning null to abstain, per-session caching, or managed dependencies routed to validateManagedDependency.

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

Claude Code on behalf of gnodet

gnodet

The critical bugs from the previous review are properly fixed — the NOOP sentinel pattern and string-keyed Map are clean solutions. Two minor issues remain.

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

Claude Code on behalf of gnodet

…impl/DefaultRepositorySystemValidator.java Co-authored-by: Guillaume Nodet <gnodet@gmail.com>

elharo · 2026-07-24T10:43:14Z


    @Inject
-    public DefaultRepositorySystemValidator(List<ValidatorFactory> validatorFactories) {
+    public DefaultRepositorySystemValidator(Map<String, ValidatorFactory> validatorFactories) {


public API change

public API defined here https://maven.apache.org/resolver/api-compatibility.html
So is not. Moreover, client code should use DI, and DI protects from changes like these, and they should inject managed beans, not manually (re)construct them.

elharo · 2026-07-24T10:46:16Z

-            for (RemoteRepository repository : request.getRepositories()) {
+        for (Map.Entry<String, ValidatorFactory> entry : validatorFactories.entrySet()) {
+            Validator validator = newInstance(session, entry.getKey(), entry.getValue());
+            if (validator != ValidatorFactory.NOOP) {


do we really need all these if (validator != ValidatorFactory.NOOP) checks? If it's Validator.NOOP then the validator doesn't throw any exceptions, nothing is added to the exceptions list, and we get exactly the same result. It feels cleaner to just treat Validator.NOOP like every other one.

Agreed, this is leftover from null being allowed.

Well, am leaving it as it is not hurting. OTOH, with today's app frameworks we at least avoid busy-looping (and calling no-op method) over thousands of managed deps...

elharo · 2026-07-24T10:49:37Z

 public class DefaultRepositorySystemValidator implements RepositorySystemValidator {
-    private final List<ValidatorFactory> validatorFactories;
+    private static final Object SESSION_VALIDATORS = Keys.of(DefaultRepositorySystemValidator.class, "validators");
+    private final Map<String, ValidatorFactory> validatorFactories;


I'm sure there's a reason but I don't immediately see what the String key is here or why we need it. A comment might be helpful.

See "What is Sisu" here https://eclipse.dev/sisu/org.eclipse.sisu.inject/

This is most common technique to have injected "variations" (or qualified beans) for same (component) contract.

gnodet

All previously identified issues are properly fixed. The NOOP sentinel pattern is a clean solution for the abstain mechanism, the Javadoc is now consistent, and null enforcement is uniform across both code paths. Test coverage is solid.

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

Claude Code on behalf of gnodet

Enh: Allow validator factory to abstain

a9de30f

Also, cache validators across single session. Fixes: apache#2007

cstamas added this to the 2.0.22 milestone Jul 22, 2026

cstamas requested review from gnodet and slawekjaranowski July 22, 2026 10:01

cstamas self-assigned this Jul 22, 2026

cstamas added the enhancement New feature or request label Jul 22, 2026

cstamas requested a review from kwin July 22, 2026 13:12

cstamas marked this pull request as ready for review July 22, 2026 13:12

Introduce new validator method.

c7626b0

cstamas mentioned this pull request Jul 22, 2026

Extend re-entrancy detection for broken trace chains #2009

Open

5 tasks

elharo requested changes Jul 23, 2026

View reviewed changes

gnodet reviewed Jul 23, 2026

View reviewed changes

cstamas added 2 commits July 23, 2026 22:29

Merge remote-tracking branch 'upstream/master' into issue-2007

e5b25d7

Add PR review changes

819997e

cstamas requested review from elharo and gnodet July 23, 2026 21:16

gnodet reviewed Jul 23, 2026

View reviewed changes

Comment thread maven-resolver-spi/src/main/java/org/eclipse/aether/spi/validator/ValidatorFactory.java Outdated

Comment thread ...er-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemValidator.java

cstamas and others added 4 commits July 24, 2026 10:33

Update maven-resolver-impl/src/main/java/org/eclipse/aether/internal/…

3baceec

…impl/DefaultRepositorySystemValidator.java Co-authored-by: Guillaume Nodet <gnodet@gmail.com>

PR comments

e1ef2b0

Tidy up UT

52d819d

Lower case

ddd50e0

cstamas requested a review from gnodet July 24, 2026 09:05

elharo reviewed Jul 24, 2026

View reviewed changes

gnodet approved these changes Jul 24, 2026

View reviewed changes

cstamas merged commit f164aa9 into apache:master Jul 24, 2026
20 checks passed

cstamas deleted the issue-2007 branch July 24, 2026 12:02

Uh oh!

Conversation

cstamas commented Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elharo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cstamas Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cstamas Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cstamas commented Jul 22, 2026 •

edited

Loading

cstamas Jul 24, 2026 •

edited

Loading

cstamas Jul 24, 2026 •

edited

Loading