gnodet commented on code in PR #2008:
URL: https://github.com/apache/maven-resolver/pull/2008#discussion_r3641756950
##########
maven-resolver-spi/src/main/java/org/eclipse/aether/spi/validator/ValidatorFactory.java:
##########
@@ -26,11 +26,21 @@
* @since 2.0.8
*/
public interface ValidatorFactory {
+
+ /**
+ * The no-op (does not validate anything) instance of validator.
+ *
+ * @since 2.0.22
+ */
+ Validator NOOP = new Validator() {};
+
/**
- * 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
Review Comment:
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.
```suggestion
* 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.
```
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemValidator.java:
##########
@@ -65,22 +69,35 @@ private void mayThrow(List<Exception> exceptions, String
message) {
}
}
+ @SuppressWarnings("unchecked")
+ private Validator newInstance(RepositorySystemSession session, String
name, ValidatorFactory factory) {
+ if (session.getCache() != null) {
+ return ((ConcurrentHashMap<String, Validator>)
+ session.getCache().computeIfAbsent(session,
SESSION_VALIDATORS, ConcurrentHashMap::new))
+ .computeIfAbsent(name, key ->
requireNonNull(factory.newInstance(session)));
+ } else {
Review Comment:
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.
```suggestion
} else {
return requireNonNull(factory.newInstance(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]
