Copilot commented on code in PR #2188:
URL: https://github.com/apache/maven-resolver/pull/2188#discussion_r4327305334
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/filter/PrefixesRemoteRepositoryFilterSource.java:
##########
@@ -364,24 +363,32 @@ private CachedPrefixes cachePrefixes(
private static final PrefixTree ENABLED_NO_INPUT = new
PrefixTree("enabled-no-input");
private static final PrefixTree BROKEN = new PrefixTree("broken");
+ private enum VerificationOutcome {
+ SERVED,
+ ABSENT,
+ UNKNOWN
+ }
+
/**
* The cached per remote repository prefixes state: the effective {@link
PrefixTree}, whether it was
* auto-discovered (as only auto-discovered prefixes are subject to denied
path verification, see
- * {@link #CONFIG_PROP_VERIFY_DENIED}), whether verification happened
already, and the denied path (if any)
- * that verification proved the remote repository actually serves.
+ * {@link #CONFIG_PROP_VERIFY_DENIED}), per-path caching of paths verified
as served, and per-path caching
+ * of definitively absent paths, with verification outcomes maintained
independently for each path.
*/
private static final class CachedPrefixes {
private static final CachedPrefixes DISABLED_PREFIXES = new
CachedPrefixes(DISABLED, false);
private static final CachedPrefixes NO_INPUT_PREFIXES = new
CachedPrefixes(ENABLED_NO_INPUT, false);
private volatile PrefixTree prefixTree;
private final boolean autoDiscovered;
- private final AtomicBoolean verifyClaimed = new AtomicBoolean(false);
- private volatile String verifiedServedPath;
+ private final Set<String> verifiedServedPaths;
+ private final Set<String> verifiedAbsentPaths;
private CachedPrefixes(PrefixTree prefixTree, boolean autoDiscovered) {
this.prefixTree = prefixTree;
this.autoDiscovered = autoDiscovered;
+ this.verifiedServedPaths = autoDiscovered ?
ConcurrentHashMap.newKeySet() : Collections.emptySet();
+ this.verifiedAbsentPaths = autoDiscovered ?
ConcurrentHashMap.newKeySet() : Collections.emptySet();
}
Review Comment:
The per-session `verifiedServedPaths` / `verifiedAbsentPaths` sets can grow
without bound for repositories that receive many distinct denied-path requests
(intentional or accidental), increasing memory usage for the lifetime of the
session. If this can be reached via untrusted coordinates/paths, consider
adding an upper bound (e.g., size cap with eviction) or alternative storage
(e.g., bounded LRU) to prevent unbounded growth while preserving most of the
caching benefit.
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/filter/PrefixesRemoteRepositoryFilterSource.java:
##########
@@ -392,21 +399,24 @@ private boolean autoDiscovered() {
return autoDiscovered;
}
- private boolean claimVerification() {
- return verifyClaimed.compareAndSet(false, true);
- }
-
private void drop() {
this.prefixTree = BROKEN;
}
private void allowVerifiedServedPath(String path) {
- this.verifiedServedPath = path;
+ this.verifiedServedPaths.add(path);
}
private boolean isVerifiedServedPath(String path) {
- String verified = this.verifiedServedPath;
- return verified != null && verified.equals(path);
+ return this.verifiedServedPaths.contains(path);
+ }
+
+ private void recordVerifiedAbsentPath(String path) {
+ this.verifiedAbsentPaths.add(path);
+ }
Review Comment:
Verification normalizes `probePath` by stripping leading slashes, but
caching uses the original `path` string as the key. This means logically
equivalent paths like `/org/foo/...` vs `org/foo/...` will be cached
separately, potentially causing redundant probes and inconsistent cache hits
depending on `RepositoryLayout` behavior. Consider normalizing the cache key
the same way as `probePath` (while still preserving the original string for
user-facing reasoning messages if desired).
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/filter/PrefixesRemoteRepositoryFilterSource.java:
##########
@@ -392,21 +399,24 @@ private boolean autoDiscovered() {
return autoDiscovered;
}
- private boolean claimVerification() {
- return verifyClaimed.compareAndSet(false, true);
- }
-
private void drop() {
this.prefixTree = BROKEN;
}
private void allowVerifiedServedPath(String path) {
- this.verifiedServedPath = path;
+ this.verifiedServedPaths.add(path);
Review Comment:
Verification normalizes `probePath` by stripping leading slashes, but
caching uses the original `path` string as the key. This means logically
equivalent paths like `/org/foo/...` vs `org/foo/...` will be cached
separately, potentially causing redundant probes and inconsistent cache hits
depending on `RepositoryLayout` behavior. Consider normalizing the cache key
the same way as `probePath` (while still preserving the original string for
user-facing reasoning messages if desired).
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/filter/PrefixesRemoteRepositoryFilterSource.java:
##########
@@ -628,21 +691,34 @@ private boolean
isVerifyDeniedDropsTreeEnabled(RemoteRepository repository) {
/**
* Checks whether the remote repository actually serves given path,
using a lightweight existence check
- * (the transporter sits below the filtering connector, so no
recursion can happen). Any failure (path
- * not present, transport problem) yields {@code false}: the prefixes
verdict is overridden (or, with
- * {@link #CONFIG_PROP_VERIFY_DENIED_DROPS_TREE}, the whole file
dropped) only when the remote repository
- * provably serves the denied path.
+ * (the transporter sits below the filtering connector, so no
recursion can happen). Only definitive
+ * {@link Transporter#ERROR_NOT_FOUND} is recorded as absent so
transient transport or network failures
+ * do not permanently poison the negative cache. Similarly, URI
construction failure does not establish
+ * remote absence and yields {@link VerificationOutcome#UNKNOWN}.
*/
- private boolean remoteRepositoryServesPath(RemoteRepository
repository, String path) {
- try (Transporter transporter =
transporterProvider.newTransporter(session, repository)) {
- transporter.peek(new PeekTask(new URI(null, null, path,
null)));
- return true;
+ private VerificationOutcome
remoteRepositoryServesPath(RemoteRepository repository, String path) {
+ Transporter transporter = null;
+ try {
+ String probePath = path;
+ while (probePath.startsWith("/")) {
+ probePath = probePath.substring(1);
+ }
+ transporter = transporterProvider.newTransporter(session,
repository);
+ transporter.peek(new PeekTask(new URI(null, null, probePath,
null)));
+ return VerificationOutcome.SERVED;
} catch (URISyntaxException e) {
logger.debug("Cannot construct URI for denied path {} of {}",
path, repository, e);
- return false;
+ return VerificationOutcome.UNKNOWN;
} catch (Exception e) {
logger.debug("Verification of denied path {} against {}
failed", path, repository, e);
- return false;
+ if (transporter != null && transporter.classify(e) ==
Transporter.ERROR_NOT_FOUND) {
+ return VerificationOutcome.ABSENT;
+ }
+ return VerificationOutcome.UNKNOWN;
+ } finally {
+ if (transporter != null) {
+ transporter.close();
+ }
}
}
Review Comment:
`transporter.close()` in the `finally` block can throw, which would override
the intended `VerificationOutcome` (e.g., turning a clean ABSENT/UNKNOWN
classification into an exception escaping the filter). Consider restoring
try-with-resources semantics, or wrapping `transporter.close()` in its own
try/catch (optionally logging at debug) to ensure close failures don’t affect
filtering decisions.
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/filter/PrefixesRemoteRepositoryFilterSource.java:
##########
@@ -628,21 +691,34 @@ private boolean
isVerifyDeniedDropsTreeEnabled(RemoteRepository repository) {
/**
* Checks whether the remote repository actually serves given path,
using a lightweight existence check
- * (the transporter sits below the filtering connector, so no
recursion can happen). Any failure (path
- * not present, transport problem) yields {@code false}: the prefixes
verdict is overridden (or, with
- * {@link #CONFIG_PROP_VERIFY_DENIED_DROPS_TREE}, the whole file
dropped) only when the remote repository
- * provably serves the denied path.
+ * (the transporter sits below the filtering connector, so no
recursion can happen). Only definitive
+ * {@link Transporter#ERROR_NOT_FOUND} is recorded as absent so
transient transport or network failures
+ * do not permanently poison the negative cache. Similarly, URI
construction failure does not establish
+ * remote absence and yields {@link VerificationOutcome#UNKNOWN}.
*/
- private boolean remoteRepositoryServesPath(RemoteRepository
repository, String path) {
- try (Transporter transporter =
transporterProvider.newTransporter(session, repository)) {
- transporter.peek(new PeekTask(new URI(null, null, path,
null)));
- return true;
+ private VerificationOutcome
remoteRepositoryServesPath(RemoteRepository repository, String path) {
+ Transporter transporter = null;
+ try {
+ String probePath = path;
+ while (probePath.startsWith("/")) {
+ probePath = probePath.substring(1);
+ }
Review Comment:
Verification normalizes `probePath` by stripping leading slashes, but
caching uses the original `path` string as the key. This means logically
equivalent paths like `/org/foo/...` vs `org/foo/...` will be cached
separately, potentially causing redundant probes and inconsistent cache hits
depending on `RepositoryLayout` behavior. Consider normalizing the cache key
the same way as `probePath` (while still preserving the original string for
user-facing reasoning messages if desired).
--
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]
