Enh: Allow validator factory to abstain

[cstamas]

Changes:

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

Fixes: #2007

[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]

nit: begin with lower case the
no periods at end

per Oracle guidelines

[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:

1. 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.

2. 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

[gnodet]

Stale Javadoc: "returned instances (even {@code null}s) are cached and reused" contradicts the @return on line 44 which says "never {@code null}", and the implementation wraps the call in requireNonNull. The "even nulls" text looks like a leftover from when null was the abstention signal.

Suggested change

	/**
	* Creates a new validator for the session.
	* Creates a new validator for the session, or {@link #NOOP} to abstain from validation. Factory is consulted
	* once per session (if cache present in session) and returned instances (even {@code null}s) are cached and reused
	* Creates a new validator for the session, or {@link #NOOP} to abstain from validation. Factory is consulted
	* once per session (if cache present in session) and returned instances are cached and reused
	* across single session.

[gnodet]

Nit: the caching path guards with requireNonNull but this non-caching fallback does not. If a factory violates the contract and returns null here, the error surfaces as a confusing NPE downstream rather than a clear fail-fast.

Suggested change

	session.getCache().computeIfAbsent(session, SESSION_VALIDATORS, ConcurrentHashMap::new))
	.computeIfAbsent(name, key -> requireNonNull(factory.newInstance(session)));
	} else {
	} else {
	return requireNonNull(factory.newInstance(session));
	}