From 7e80b46812aaf999a73cc2c89265fe0e05860e46 Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Wed, 22 Jul 2026 15:28:55 +0000 Subject: [PATCH 1/5] Extend re-entrancy detection to cover broken trace chains and skip managed dependency validation The trace-based re-entrancy detection added in PR #1957 does not cover all code paths. Maven 4's RequestTraceHelper converts between Maven API traces and resolver RequestTrace objects, creating a fresh trace chain that loses the REPOSITORY_SYSTEM_CALL marker. This causes MavenValidator to reject re-entrant calls that carry uninterpolated property expressions. This commit addresses the gap with two complementary fixes: 1. Session-scoped re-entrancy detection: Each public RepositorySystem method now maintains a depth counter in SessionData via enterSessionScope()/ exitSessionScope(). When the counter is > 0 on entry, the call is detected as re-entrant regardless of whether the RequestTrace chain carries the marker. This catches cases where consumers rebuild the trace chain from a different tracing system. 2. Skip managed dependency validation in validateCollectRequest(): Managed dependencies are declarative constraints that only take effect when a matching dependency is encountered during collection. Validating them eagerly rejects valid builds where a BOM imports managed dependencies with uninterpolated property expressions (e.g. ${osgi.version}) that are never actually used. If a managed dependency IS matched and its coordinates are invalid, the error surfaces during version/artifact resolution. Together, these changes allow Maven PR #12481 to remove the consumer-side workarounds for uninterpolated expressions in DefaultArtifactDescriptorReader, ArtifactDescriptorReaderDelegate, DefaultProjectDependenciesResolver, and DefaultDependencyResolver. Co-Authored-By: Claude Opus 4.6 --- .../impl/DefaultRepositorySystem.java | 260 +++++++++++++----- .../DefaultRepositorySystemValidator.java | 14 +- ...DefaultRepositorySystemReentrancyTest.java | 168 ++++++++++- 3 files changed, 363 insertions(+), 79 deletions(-) diff --git a/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultRepositorySystem.java b/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultRepositorySystem.java index 915df50f63..71cc7b3425 100644 --- a/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultRepositorySystem.java +++ b/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultRepositorySystem.java @@ -220,55 +220,88 @@ public DefaultRepositorySystem( public VersionResult resolveVersion(RepositorySystemSession session, VersionRequest request) throws VersionResolutionException { requireNonNull(request, "request cannot be null"); - if (!isReentrant(request.getTrace())) { + Runnable exitGuard = null; + if (!isReentrant(request.getTrace(), session)) { validateSession(session); repositorySystemValidator.validateVersionRequest(session, request); request.setTrace(stampReentrancyMarker(request.getTrace())); + exitGuard = enterSessionScope(session); + } + try { + return versionResolver.resolveVersion(session, request); + } finally { + if (exitGuard != null) { + exitGuard.run(); + } } - return versionResolver.resolveVersion(session, request); } @Override public VersionRangeResult resolveVersionRange(RepositorySystemSession session, VersionRangeRequest request) throws VersionRangeResolutionException { requireNonNull(request, "request cannot be null"); - if (!isReentrant(request.getTrace())) { + Runnable exitGuard = null; + if (!isReentrant(request.getTrace(), session)) { validateSession(session); repositorySystemValidator.validateVersionRangeRequest(session, request); request.setTrace(stampReentrancyMarker(request.getTrace())); + exitGuard = enterSessionScope(session); + } + try { + return versionRangeResolver.resolveVersionRange(session, request); + } finally { + if (exitGuard != null) { + exitGuard.run(); + } } - return versionRangeResolver.resolveVersionRange(session, request); } @Override public ArtifactDescriptorResult readArtifactDescriptor( RepositorySystemSession session, ArtifactDescriptorRequest request) throws ArtifactDescriptorException { requireNonNull(request, "request cannot be null"); - boolean outermost = !isReentrant(request.getTrace()); + boolean outermost = !isReentrant(request.getTrace(), session); + Runnable exitGuard = null; if (outermost) { validateSession(session); repositorySystemValidator.validateArtifactDescriptorRequest(session, request); request.setTrace(stampReentrancyMarker(request.getTrace())); + exitGuard = enterSessionScope(session); } - ArtifactDescriptorResult descriptorResult = artifactDescriptorReader.readArtifactDescriptor(session, request); - if (outermost) { - for (ArtifactDecorator decorator : Utils.getArtifactDecorators(session, artifactDecoratorFactories)) { - descriptorResult.setArtifact(decorator.decorateArtifact(descriptorResult)); + try { + ArtifactDescriptorResult descriptorResult = + artifactDescriptorReader.readArtifactDescriptor(session, request); + if (outermost) { + for (ArtifactDecorator decorator : Utils.getArtifactDecorators(session, artifactDecoratorFactories)) { + descriptorResult.setArtifact(decorator.decorateArtifact(descriptorResult)); + } + } + return descriptorResult; + } finally { + if (exitGuard != null) { + exitGuard.run(); } } - return descriptorResult; } @Override public ArtifactResult resolveArtifact(RepositorySystemSession session, ArtifactRequest request) throws ArtifactResolutionException { requireNonNull(request, "request cannot be null"); - if (!isReentrant(request.getTrace())) { + Runnable exitGuard = null; + if (!isReentrant(request.getTrace(), session)) { validateSession(session); repositorySystemValidator.validateArtifactRequests(session, Collections.singleton(request)); request.setTrace(stampReentrancyMarker(request.getTrace())); + exitGuard = enterSessionScope(session); + } + try { + return artifactResolver.resolveArtifact(session, request); + } finally { + if (exitGuard != null) { + exitGuard.run(); + } } - return artifactResolver.resolveArtifact(session, request); } @Override @@ -282,14 +315,22 @@ public List resolveArtifacts( .filter(Objects::nonNull) .findFirst() .orElse(null); - if (!isReentrant(firstTrace)) { + Runnable exitGuard = null; + if (!isReentrant(firstTrace, session)) { validateSession(session); repositorySystemValidator.validateArtifactRequests(session, requests); for (ArtifactRequest request : requests) { request.setTrace(stampReentrancyMarker(request.getTrace())); } + exitGuard = enterSessionScope(session); + } + try { + return artifactResolver.resolveArtifacts(session, requests); + } finally { + if (exitGuard != null) { + exitGuard.run(); + } } - return artifactResolver.resolveArtifacts(session, requests); } @Override @@ -302,96 +343,120 @@ public List resolveMetadata( .filter(Objects::nonNull) .findFirst() .orElse(null); - if (!isReentrant(firstTrace)) { + Runnable exitGuard = null; + if (!isReentrant(firstTrace, session)) { validateSession(session); repositorySystemValidator.validateMetadataRequests(session, requests); for (MetadataRequest request : requests) { request.setTrace(stampReentrancyMarker(request.getTrace())); } + exitGuard = enterSessionScope(session); + } + try { + return metadataResolver.resolveMetadata(session, requests); + } finally { + if (exitGuard != null) { + exitGuard.run(); + } } - return metadataResolver.resolveMetadata(session, requests); } @Override public CollectResult collectDependencies(RepositorySystemSession session, CollectRequest request) throws DependencyCollectionException { requireNonNull(request, "request cannot be null"); - if (!isReentrant(request.getTrace())) { + Runnable exitGuard = null; + if (!isReentrant(request.getTrace(), session)) { validateSession(session); repositorySystemValidator.validateCollectRequest(session, request); request.setTrace(stampReentrancyMarker(request.getTrace())); + exitGuard = enterSessionScope(session); + } + try { + return dependencyCollector.collectDependencies(session, request); + } finally { + if (exitGuard != null) { + exitGuard.run(); + } } - return dependencyCollector.collectDependencies(session, request); } @Override public DependencyResult resolveDependencies(RepositorySystemSession session, DependencyRequest request) throws DependencyResolutionException { requireNonNull(request, "request cannot be null"); - if (!isReentrant(request.getTrace())) { + Runnable exitGuard = null; + if (!isReentrant(request.getTrace(), session)) { validateSession(session); repositorySystemValidator.validateDependencyRequest(session, request); request.setTrace(stampReentrancyMarker(request.getTrace())); + exitGuard = enterSessionScope(session); } - RequestTrace trace = RequestTrace.newChild(request.getTrace(), request); - - DependencyResult result = new DependencyResult(request); - - DependencyCollectionException dce = null; - ArtifactResolutionException are = null; + try { + RequestTrace trace = RequestTrace.newChild(request.getTrace(), request); + + DependencyResult result = new DependencyResult(request); + + DependencyCollectionException dce = null; + ArtifactResolutionException are = null; + + if (request.getRoot() != null) { + result.setRoot(request.getRoot()); + } else if (request.getCollectRequest() != null) { + CollectResult collectResult; + try { + request.getCollectRequest().setTrace(trace); + collectResult = dependencyCollector.collectDependencies(session, request.getCollectRequest()); + } catch (DependencyCollectionException e) { + dce = e; + collectResult = e.getResult(); + } + result.setRoot(collectResult.getRoot()); + result.setCycles(collectResult.getCycles()); + result.setCollectExceptions(collectResult.getExceptions()); + } else { + throw new NullPointerException("dependency node and collect request cannot be null"); + } - if (request.getRoot() != null) { - result.setRoot(request.getRoot()); - } else if (request.getCollectRequest() != null) { - CollectResult collectResult; + final List dependencyNodes = + doFlattenDependencyNodes(session, result.getRoot(), request.getFilter()); + + final List requests = dependencyNodes.stream() + .map(n -> { + if (n.getDependency() != null) { + ArtifactRequest artifactRequest = new ArtifactRequest(n); + artifactRequest.setTrace(trace); + return artifactRequest; + } else { + return null; + } + }) + .filter(Objects::nonNull) + .collect(Collectors.toList()); + List results; try { - request.getCollectRequest().setTrace(trace); - collectResult = dependencyCollector.collectDependencies(session, request.getCollectRequest()); - } catch (DependencyCollectionException e) { - dce = e; - collectResult = e.getResult(); + results = artifactResolver.resolveArtifacts(session, requests); + } catch (ArtifactResolutionException e) { + are = e; + results = e.getResults(); } - result.setRoot(collectResult.getRoot()); - result.setCycles(collectResult.getCycles()); - result.setCollectExceptions(collectResult.getExceptions()); - } else { - throw new NullPointerException("dependency node and collect request cannot be null"); - } + result.setDependencyNodeResults(dependencyNodes); + result.setArtifactResults(results); - final List dependencyNodes = - doFlattenDependencyNodes(session, result.getRoot(), request.getFilter()); - - final List requests = dependencyNodes.stream() - .map(n -> { - if (n.getDependency() != null) { - ArtifactRequest artifactRequest = new ArtifactRequest(n); - artifactRequest.setTrace(trace); - return artifactRequest; - } else { - return null; - } - }) - .filter(Objects::nonNull) - .collect(Collectors.toList()); - List results; - try { - results = artifactResolver.resolveArtifacts(session, requests); - } catch (ArtifactResolutionException e) { - are = e; - results = e.getResults(); - } - result.setDependencyNodeResults(dependencyNodes); - result.setArtifactResults(results); + updateNodesWithResolvedArtifacts(results); - updateNodesWithResolvedArtifacts(results); + if (dce != null) { + throw new DependencyResolutionException(result, dce); + } else if (are != null) { + throw new DependencyResolutionException(result, are); + } - if (dce != null) { - throw new DependencyResolutionException(result, dce); - } else if (are != null) { - throw new DependencyResolutionException(result, are); + return result; + } finally { + if (exitGuard != null) { + exitGuard.run(); + } } - - return result; } @Override @@ -557,6 +622,24 @@ public void shutdown() { } } + /** + * Session data key for the re-entrancy depth counter. This supplements the + * {@link RequestTrace}-based detection for consumers that rebuild the trace chain + * from a different tracing system (e.g. Maven 4's {@code RequestTraceHelper} converts + * between Maven API traces and resolver traces, losing the + * {@link #REPOSITORY_SYSTEM_CALL} marker). + *

+ * The value stored under this key is an {@link AtomicInteger} tracking how many + * {@code RepositorySystem} public methods are currently on the call stack for this + * session. A value > 0 on entry means the call is re-entrant. + */ + private static final Object SESSION_REENTRY_DEPTH_KEY = new Object() { + @Override + public String toString() { + return "RepositorySystem.reentryDepth"; + } + }; + /** * Stamps the {@link #REPOSITORY_SYSTEM_CALL} re-entrancy marker into the trace chain * while preserving the original trace tip data. The marker is inserted below @@ -587,6 +670,41 @@ private static boolean isReentrant(RequestTrace trace) { return false; } + /** + * Combined re-entrancy check using both {@link RequestTrace} ancestry and session-scoped + * depth tracking. Either mechanism detecting re-entrancy is sufficient to skip validation. + *

+ * The trace-based check is the primary mechanism and works when callers properly propagate + * traces. The session-based check is a fallback for callers that rebuild the trace chain + * from a different tracing system (e.g. Maven 4's trace conversion loses the resolver's + * re-entrancy marker). + * + * @param trace the current request trace (may be {@code null}) + * @param session the current repository system session + * @return {@code true} if this is a re-entrant call, {@code false} if it is the outermost call + */ + private static boolean isReentrant(RequestTrace trace, RepositorySystemSession session) { + return isReentrant(trace) || getReentryDepth(session).get() > 0; + } + + /** + * Increments the session-scoped re-entrancy depth counter. Must be called on every outermost + * entry into a public {@code RepositorySystem} method, and the returned {@link Runnable} must + * be invoked in a {@code finally} block to decrement the counter on exit. + * + * @param session the current repository system session + * @return a {@link Runnable} that decrements the depth counter when invoked + */ + private static Runnable enterSessionScope(RepositorySystemSession session) { + AtomicInteger depth = getReentryDepth(session); + depth.incrementAndGet(); + return depth::decrementAndGet; + } + + private static AtomicInteger getReentryDepth(RepositorySystemSession session) { + return (AtomicInteger) session.getData().computeIfAbsent(SESSION_REENTRY_DEPTH_KEY, () -> new AtomicInteger(0)); + } + private void validateSession(RepositorySystemSession session) { requireNonNull(session, "repository system session cannot be null"); invalidSession(session.getLocalRepositoryManager(), "local repository manager"); diff --git a/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemValidator.java b/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemValidator.java index 15c85f20da..109ccf5f08 100644 --- a/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemValidator.java +++ b/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemValidator.java @@ -202,13 +202,13 @@ public void validateCollectRequest(RepositorySystemSession session, CollectReque exceptions.add(e); } } - for (Dependency managedDependency : request.getManagedDependencies()) { - try { - validator.validateDependency(managedDependency); - } catch (Exception e) { - exceptions.add(e); - } - } + // Managed dependencies are intentionally NOT validated here. They are declarative + // constraints (version/scope/exclusion overrides) that only take effect when a + // matching dependency is encountered during collection. Validating them eagerly + // rejects valid builds where a BOM imports managed dependencies with uninterpolated + // property expressions (e.g. ${osgi.version}) 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. for (RemoteRepository repository : request.getRepositories()) { try { validator.validateRemoteRepository(repository); diff --git a/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemReentrancyTest.java b/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemReentrancyTest.java index 6aa9e10de8..eff6a51da6 100644 --- a/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemReentrancyTest.java +++ b/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemReentrancyTest.java @@ -21,11 +21,15 @@ import java.util.Collections; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; import org.eclipse.aether.DefaultRepositorySystemSession; import org.eclipse.aether.RepositorySystemSession; import org.eclipse.aether.RequestTrace; import org.eclipse.aether.artifact.DefaultArtifact; +import org.eclipse.aether.collection.CollectRequest; +import org.eclipse.aether.collection.CollectResult; +import org.eclipse.aether.graph.Dependency; import org.eclipse.aether.impl.ArtifactResolver; import org.eclipse.aether.impl.DependencyCollector; import org.eclipse.aether.impl.Deployer; @@ -57,7 +61,7 @@ public class DefaultRepositorySystemReentrancyTest { /** - * A validator that rejects any artifact whose version contains "${". + * A validator that rejects any artifact or dependency whose version contains "${". * This simulates Maven's MavenValidator rejecting uninterpolated expressions. */ private static final ValidatorFactory EXPRESSION_REJECTING_VALIDATOR_FACTORY = session -> new Validator() { @@ -67,6 +71,11 @@ public void validateArtifact(org.eclipse.aether.artifact.Artifact artifact) { throw new IllegalArgumentException("Uninterpolated expression in version: " + artifact.getVersion()); } } + + @Override + public void validateDependency(Dependency dependency) { + validateArtifact(dependency.getArtifact()); + } }; private DefaultRepositorySystem system; @@ -303,4 +312,161 @@ void outerCallWithNullTraceStillStampsMarker() throws Exception { system.resolveVersionRange(session, innerRequest); assertEquals(countBefore, validationCount.get(), "Re-entrant call should skip validation"); } + + @Test + void sessionScopedDetectionSkipsValidationWhenTraceChainIsBroken() throws Exception { + // Simulates Maven 4's flow where the model builder converts between Maven API traces + // and resolver traces via RequestTraceHelper, losing the REPOSITORY_SYSTEM_CALL marker. + // + // The DependencyCollector, called from within collectDependencies, re-enters + // RepositorySystem.resolveVersionRange with a FRESH trace (no marker in ancestry) + // and an uninterpolated expression like ${project.version}. Without session-scoped + // detection, the validator would reject the expression; with it, the call is + // detected as re-entrant and validation is skipped. + AtomicReference systemRef = new AtomicReference<>(); + + DependencyCollector reentrantCollector = (s, request) -> { + // Inside collectDependencies, simulate a re-entrant call with a FRESH trace + // (no marker in ancestry — this is the broken path) and an uninterpolated + // expression. If session-scoped detection fails, the validator rejects this. + VersionRangeRequest innerRequest = new VersionRangeRequest( + new DefaultArtifact("g:inner:${project.version}"), Collections.emptyList(), null); + // Fresh trace — no REPOSITORY_SYSTEM_CALL marker (simulates Maven's trace conversion) + innerRequest.setTrace(RequestTrace.newChild(null, "MavenModelResolver")); + try { + systemRef.get().resolveVersionRange(s, innerRequest); + } catch (Exception e) { + fail("Re-entrant call with broken trace chain should succeed via session-scoped detection: " + e); + } + return new CollectResult(request); + }; + + DefaultRepositorySystem strictSystem = new DefaultRepositorySystem( + new StubVersionResolver(), + new StubVersionRangeResolver(), + mock(ArtifactResolver.class), + mock(MetadataResolver.class), + new StubArtifactDescriptorReader(), + reentrantCollector, + mock(Installer.class), + mock(Deployer.class), + mock(LocalRepositoryProvider.class), + new StubSyncContextFactory(), + new DefaultRemoteRepositoryManager( + new DefaultUpdatePolicyAnalyzer(), + new DefaultChecksumPolicyProvider(), + new DefaultRepositoryKeyFunctionFactory()), + new DefaultRepositorySystemLifecycle(), + Collections.emptyMap(), + new DefaultRepositorySystemValidator( + Collections.singletonList(EXPRESSION_REJECTING_VALIDATOR_FACTORY))); + systemRef.set(strictSystem); + + // The outermost collectDependencies call should succeed, and the inner + // resolveVersionRange call (with broken trace and uninterpolated expression) + // should be detected as re-entrant via the session-scoped depth counter. + CollectRequest collectRequest = new CollectRequest(); + collectRequest.setRootArtifact(new DefaultArtifact("g:root:1.0")); + + assertDoesNotThrow( + () -> strictSystem.collectDependencies(session, collectRequest), + "Inner call with broken trace chain should succeed via session-scoped detection"); + } + + @Test + void collectDependenciesAcceptsManagedDepsWithUninterpolatedExpressions() throws Exception { + // Reproduces the MavenITgh12305 scenario: a BOM imports managed dependencies + // with uninterpolated expressions like ${osgi.version}. These managed dependencies + // should be accepted because they are declarative constraints that may never be used. + DependencyCollector passThroughCollector = (s, request) -> new CollectResult(request); + + DefaultRepositorySystem strictSystem = new DefaultRepositorySystem( + new StubVersionResolver(), + new StubVersionRangeResolver(), + mock(ArtifactResolver.class), + mock(MetadataResolver.class), + new StubArtifactDescriptorReader(), + passThroughCollector, + mock(Installer.class), + mock(Deployer.class), + mock(LocalRepositoryProvider.class), + new StubSyncContextFactory(), + new DefaultRemoteRepositoryManager( + new DefaultUpdatePolicyAnalyzer(), + new DefaultChecksumPolicyProvider(), + new DefaultRepositoryKeyFunctionFactory()), + new DefaultRepositorySystemLifecycle(), + Collections.emptyMap(), + new DefaultRepositorySystemValidator( + Collections.singletonList(EXPRESSION_REJECTING_VALIDATOR_FACTORY))); + + // Build a CollectRequest with a managed dependency that has an uninterpolated version + // (like ${osgi.version} from a BOM import) + CollectRequest collectRequest = new CollectRequest(); + collectRequest.setRootArtifact(new DefaultArtifact("g:project:1.0")); + collectRequest.addManagedDependency( + new Dependency(new DefaultArtifact("org.example:lib-with-undefined-version:${undefined.version}"), "provided")); + + // This should succeed — managed dependencies are not validated because they are + // declarative constraints, not actual resolution targets + assertDoesNotThrow( + () -> strictSystem.collectDependencies(session, collectRequest), + "Managed dependencies with uninterpolated expressions should be accepted"); + } + + @Test + void collectDependenciesStillRejectsInvalidDirectDependencies() throws Exception { + // Verify that direct dependencies (not managed) ARE still validated + DependencyCollector passThroughCollector = (s, request) -> new CollectResult(request); + + DefaultRepositorySystem strictSystem = new DefaultRepositorySystem( + new StubVersionResolver(), + new StubVersionRangeResolver(), + mock(ArtifactResolver.class), + mock(MetadataResolver.class), + new StubArtifactDescriptorReader(), + passThroughCollector, + mock(Installer.class), + mock(Deployer.class), + mock(LocalRepositoryProvider.class), + new StubSyncContextFactory(), + new DefaultRemoteRepositoryManager( + new DefaultUpdatePolicyAnalyzer(), + new DefaultChecksumPolicyProvider(), + new DefaultRepositoryKeyFunctionFactory()), + new DefaultRepositorySystemLifecycle(), + Collections.emptyMap(), + new DefaultRepositorySystemValidator( + Collections.singletonList(EXPRESSION_REJECTING_VALIDATOR_FACTORY))); + + // Direct dependency with uninterpolated expression should still be rejected + CollectRequest collectRequest = new CollectRequest(); + collectRequest.setRootArtifact(new DefaultArtifact("g:project:1.0")); + collectRequest.addDependency( + new Dependency(new DefaultArtifact("org.example:lib:${undefined.version}"), "compile")); + + assertThrows( + IllegalArgumentException.class, + () -> strictSystem.collectDependencies(session, collectRequest), + "Direct dependencies with uninterpolated expressions should still be rejected"); + } + + @Test + void sessionScopedDepthIsProperlyDecrementedOnExit() throws Exception { + // Verify that the session-scoped depth counter is properly decremented after + // a RepositorySystem call completes, so independent calls are still validated + VersionRangeRequest request1 = + new VersionRangeRequest(new DefaultArtifact("g:a:1.0"), Collections.emptyList(), null); + VersionRangeRequest request2 = + new VersionRangeRequest(new DefaultArtifact("g:b:2.0"), Collections.emptyList(), null); + + system.resolveVersionRange(session, request1); + int countAfterFirst = validationCount.get(); + assertEquals(1, countAfterFirst, "First call should validate"); + + // Second independent call (no shared trace) should also validate + // because the depth counter should have been decremented on exit + system.resolveVersionRange(session, request2); + assertEquals(2, validationCount.get(), "Second independent call should also validate"); + } } From 7e400b231c2806d82354227740bd0c0423d29ad2 Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Wed, 22 Jul 2026 15:37:28 +0000 Subject: [PATCH 2/5] fix spotless formatting violation in re-entrancy test Co-Authored-By: Claude Opus 4.6 --- .../internal/impl/DefaultRepositorySystemReentrancyTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemReentrancyTest.java b/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemReentrancyTest.java index eff6a51da6..1cb3da4519 100644 --- a/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemReentrancyTest.java +++ b/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemReentrancyTest.java @@ -404,8 +404,8 @@ void collectDependenciesAcceptsManagedDepsWithUninterpolatedExpressions() throws // (like ${osgi.version} from a BOM import) CollectRequest collectRequest = new CollectRequest(); collectRequest.setRootArtifact(new DefaultArtifact("g:project:1.0")); - collectRequest.addManagedDependency( - new Dependency(new DefaultArtifact("org.example:lib-with-undefined-version:${undefined.version}"), "provided")); + collectRequest.addManagedDependency(new Dependency( + new DefaultArtifact("org.example:lib-with-undefined-version:${undefined.version}"), "provided")); // This should succeed — managed dependencies are not validated because they are // declarative constraints, not actual resolution targets From 74005921143b0ba0178bb14c09b4fbb804464ae8 Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Thu, 23 Jul 2026 14:30:34 +0200 Subject: [PATCH 3/5] Restore managed dependency validation per review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../DefaultRepositorySystemValidator.java | 14 +++---- ...DefaultRepositorySystemReentrancyTest.java | 41 ------------------- 2 files changed, 7 insertions(+), 48 deletions(-) diff --git a/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemValidator.java b/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemValidator.java index 109ccf5f08..15c85f20da 100644 --- a/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemValidator.java +++ b/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemValidator.java @@ -202,13 +202,13 @@ public void validateCollectRequest(RepositorySystemSession session, CollectReque exceptions.add(e); } } - // Managed dependencies are intentionally NOT validated here. They are declarative - // constraints (version/scope/exclusion overrides) that only take effect when a - // matching dependency is encountered during collection. Validating them eagerly - // rejects valid builds where a BOM imports managed dependencies with uninterpolated - // property expressions (e.g. ${osgi.version}) 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. + for (Dependency managedDependency : request.getManagedDependencies()) { + try { + validator.validateDependency(managedDependency); + } catch (Exception e) { + exceptions.add(e); + } + } for (RemoteRepository repository : request.getRepositories()) { try { validator.validateRemoteRepository(repository); diff --git a/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemReentrancyTest.java b/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemReentrancyTest.java index 1cb3da4519..aadbb48276 100644 --- a/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemReentrancyTest.java +++ b/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemReentrancyTest.java @@ -373,47 +373,6 @@ void sessionScopedDetectionSkipsValidationWhenTraceChainIsBroken() throws Except "Inner call with broken trace chain should succeed via session-scoped detection"); } - @Test - void collectDependenciesAcceptsManagedDepsWithUninterpolatedExpressions() throws Exception { - // Reproduces the MavenITgh12305 scenario: a BOM imports managed dependencies - // with uninterpolated expressions like ${osgi.version}. These managed dependencies - // should be accepted because they are declarative constraints that may never be used. - DependencyCollector passThroughCollector = (s, request) -> new CollectResult(request); - - DefaultRepositorySystem strictSystem = new DefaultRepositorySystem( - new StubVersionResolver(), - new StubVersionRangeResolver(), - mock(ArtifactResolver.class), - mock(MetadataResolver.class), - new StubArtifactDescriptorReader(), - passThroughCollector, - mock(Installer.class), - mock(Deployer.class), - mock(LocalRepositoryProvider.class), - new StubSyncContextFactory(), - new DefaultRemoteRepositoryManager( - new DefaultUpdatePolicyAnalyzer(), - new DefaultChecksumPolicyProvider(), - new DefaultRepositoryKeyFunctionFactory()), - new DefaultRepositorySystemLifecycle(), - Collections.emptyMap(), - new DefaultRepositorySystemValidator( - Collections.singletonList(EXPRESSION_REJECTING_VALIDATOR_FACTORY))); - - // Build a CollectRequest with a managed dependency that has an uninterpolated version - // (like ${osgi.version} from a BOM import) - CollectRequest collectRequest = new CollectRequest(); - collectRequest.setRootArtifact(new DefaultArtifact("g:project:1.0")); - collectRequest.addManagedDependency(new Dependency( - new DefaultArtifact("org.example:lib-with-undefined-version:${undefined.version}"), "provided")); - - // This should succeed — managed dependencies are not validated because they are - // declarative constraints, not actual resolution targets - assertDoesNotThrow( - () -> strictSystem.collectDependencies(session, collectRequest), - "Managed dependencies with uninterpolated expressions should be accepted"); - } - @Test void collectDependenciesStillRejectsInvalidDirectDependencies() throws Exception { // Verify that direct dependencies (not managed) ARE still validated From 48c7a292b7c3b72bab4f0e3fa90545dba4c1ef05 Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Tue, 28 Jul 2026 10:01:09 +0200 Subject: [PATCH 4/5] Use thread-scoped depth counter instead of session-scoped Address review feedback: re-entrancy is per-call-stack, so a ThreadLocal is the correct semantic. The previous AtomicInteger in SessionData was shared across threads, causing false positives (skipped validation) in parallel builds where multiple threads share a single session. Also fix fail() call in test to preserve the exception stack trace via fail(msg, cause) instead of fail(msg + e), and add a test verifying the depth counter is properly decremented when the delegate throws. Co-Authored-By: Claude Opus 4.6 --- .../impl/DefaultRepositorySystem.java | 45 ++++++-------- ...DefaultRepositorySystemReentrancyTest.java | 60 ++++++++++++++++--- 2 files changed, 70 insertions(+), 35 deletions(-) diff --git a/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultRepositorySystem.java b/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultRepositorySystem.java index 71cc7b3425..45336315ca 100644 --- a/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultRepositorySystem.java +++ b/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultRepositorySystem.java @@ -623,22 +623,17 @@ public void shutdown() { } /** - * Session data key for the re-entrancy depth counter. This supplements the - * {@link RequestTrace}-based detection for consumers that rebuild the trace chain - * from a different tracing system (e.g. Maven 4's {@code RequestTraceHelper} converts - * between Maven API traces and resolver traces, losing the - * {@link #REPOSITORY_SYSTEM_CALL} marker). + * Thread-scoped re-entrancy depth counter. This supplements the {@link RequestTrace}-based + * detection for consumers that rebuild the trace chain from a different tracing system + * (e.g. Maven 4's {@code RequestTraceHelper} converts between Maven API traces and + * resolver traces, losing the {@link #REPOSITORY_SYSTEM_CALL} marker). *

- * The value stored under this key is an {@link AtomicInteger} tracking how many - * {@code RepositorySystem} public methods are currently on the call stack for this - * session. A value > 0 on entry means the call is re-entrant. + * Re-entrancy is inherently per-call-stack (per-thread), so a {@code ThreadLocal} is the + * correct semantic. A value > 0 on entry means the current thread is already inside + * a {@code RepositorySystem} public method. Unlike a session-scoped counter, this avoids + * false positives in parallel builds where multiple threads share a single session. */ - private static final Object SESSION_REENTRY_DEPTH_KEY = new Object() { - @Override - public String toString() { - return "RepositorySystem.reentryDepth"; - } - }; + private static final ThreadLocal REENTRY_DEPTH = ThreadLocal.withInitial(() -> new int[] {0}); /** * Stamps the {@link #REPOSITORY_SYSTEM_CALL} re-entrancy marker into the trace chain @@ -671,38 +666,34 @@ private static boolean isReentrant(RequestTrace trace) { } /** - * Combined re-entrancy check using both {@link RequestTrace} ancestry and session-scoped + * Combined re-entrancy check using both {@link RequestTrace} ancestry and thread-scoped * depth tracking. Either mechanism detecting re-entrancy is sufficient to skip validation. *

* The trace-based check is the primary mechanism and works when callers properly propagate - * traces. The session-based check is a fallback for callers that rebuild the trace chain + * traces. The thread-scoped check is a fallback for callers that rebuild the trace chain * from a different tracing system (e.g. Maven 4's trace conversion loses the resolver's * re-entrancy marker). * * @param trace the current request trace (may be {@code null}) - * @param session the current repository system session + * @param session the current repository system session (unused, kept for signature consistency) * @return {@code true} if this is a re-entrant call, {@code false} if it is the outermost call */ private static boolean isReentrant(RequestTrace trace, RepositorySystemSession session) { - return isReentrant(trace) || getReentryDepth(session).get() > 0; + return isReentrant(trace) || REENTRY_DEPTH.get()[0] > 0; } /** - * Increments the session-scoped re-entrancy depth counter. Must be called on every outermost + * Increments the thread-scoped re-entrancy depth counter. Must be called on every outermost * entry into a public {@code RepositorySystem} method, and the returned {@link Runnable} must * be invoked in a {@code finally} block to decrement the counter on exit. * - * @param session the current repository system session + * @param session the current repository system session (unused, kept for signature consistency) * @return a {@link Runnable} that decrements the depth counter when invoked */ private static Runnable enterSessionScope(RepositorySystemSession session) { - AtomicInteger depth = getReentryDepth(session); - depth.incrementAndGet(); - return depth::decrementAndGet; - } - - private static AtomicInteger getReentryDepth(RepositorySystemSession session) { - return (AtomicInteger) session.getData().computeIfAbsent(SESSION_REENTRY_DEPTH_KEY, () -> new AtomicInteger(0)); + int[] depth = REENTRY_DEPTH.get(); + depth[0]++; + return () -> depth[0]--; } private void validateSession(RepositorySystemSession session) { diff --git a/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemReentrancyTest.java b/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemReentrancyTest.java index aadbb48276..3787d2a60c 100644 --- a/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemReentrancyTest.java +++ b/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemReentrancyTest.java @@ -314,13 +314,13 @@ void outerCallWithNullTraceStillStampsMarker() throws Exception { } @Test - void sessionScopedDetectionSkipsValidationWhenTraceChainIsBroken() throws Exception { + void threadScopedDetectionSkipsValidationWhenTraceChainIsBroken() throws Exception { // Simulates Maven 4's flow where the model builder converts between Maven API traces // and resolver traces via RequestTraceHelper, losing the REPOSITORY_SYSTEM_CALL marker. // // The DependencyCollector, called from within collectDependencies, re-enters // RepositorySystem.resolveVersionRange with a FRESH trace (no marker in ancestry) - // and an uninterpolated expression like ${project.version}. Without session-scoped + // and an uninterpolated expression like ${project.version}. Without thread-scoped // detection, the validator would reject the expression; with it, the call is // detected as re-entrant and validation is skipped. AtomicReference systemRef = new AtomicReference<>(); @@ -328,7 +328,7 @@ void sessionScopedDetectionSkipsValidationWhenTraceChainIsBroken() throws Except DependencyCollector reentrantCollector = (s, request) -> { // Inside collectDependencies, simulate a re-entrant call with a FRESH trace // (no marker in ancestry — this is the broken path) and an uninterpolated - // expression. If session-scoped detection fails, the validator rejects this. + // expression. If thread-scoped detection fails, the validator rejects this. VersionRangeRequest innerRequest = new VersionRangeRequest( new DefaultArtifact("g:inner:${project.version}"), Collections.emptyList(), null); // Fresh trace — no REPOSITORY_SYSTEM_CALL marker (simulates Maven's trace conversion) @@ -336,7 +336,7 @@ void sessionScopedDetectionSkipsValidationWhenTraceChainIsBroken() throws Except try { systemRef.get().resolveVersionRange(s, innerRequest); } catch (Exception e) { - fail("Re-entrant call with broken trace chain should succeed via session-scoped detection: " + e); + fail("Re-entrant call with broken trace chain should succeed via thread-scoped detection", e); } return new CollectResult(request); }; @@ -364,13 +364,13 @@ void sessionScopedDetectionSkipsValidationWhenTraceChainIsBroken() throws Except // The outermost collectDependencies call should succeed, and the inner // resolveVersionRange call (with broken trace and uninterpolated expression) - // should be detected as re-entrant via the session-scoped depth counter. + // should be detected as re-entrant via the thread-scoped depth counter. CollectRequest collectRequest = new CollectRequest(); collectRequest.setRootArtifact(new DefaultArtifact("g:root:1.0")); assertDoesNotThrow( () -> strictSystem.collectDependencies(session, collectRequest), - "Inner call with broken trace chain should succeed via session-scoped detection"); + "Inner call with broken trace chain should succeed via thread-scoped detection"); } @Test @@ -411,8 +411,8 @@ void collectDependenciesStillRejectsInvalidDirectDependencies() throws Exception } @Test - void sessionScopedDepthIsProperlyDecrementedOnExit() throws Exception { - // Verify that the session-scoped depth counter is properly decremented after + void threadScopedDepthIsProperlyDecrementedOnExit() throws Exception { + // Verify that the thread-scoped depth counter is properly decremented after // a RepositorySystem call completes, so independent calls are still validated VersionRangeRequest request1 = new VersionRangeRequest(new DefaultArtifact("g:a:1.0"), Collections.emptyList(), null); @@ -428,4 +428,48 @@ void sessionScopedDepthIsProperlyDecrementedOnExit() throws Exception { system.resolveVersionRange(session, request2); assertEquals(2, validationCount.get(), "Second independent call should also validate"); } + + @Test + void threadScopedDepthIsDecrementedWhenDelegateThrows() throws Exception { + // Verify that the depth counter is properly cleaned up even when the delegate + // throws an exception, ensuring subsequent outermost calls are still validated. + DependencyCollector throwingCollector = (s, request) -> { + throw new RuntimeException("Simulated delegate failure"); + }; + + DefaultRepositorySystem throwingSystem = new DefaultRepositorySystem( + new StubVersionResolver(), + new StubVersionRangeResolver(), + mock(ArtifactResolver.class), + mock(MetadataResolver.class), + new StubArtifactDescriptorReader(), + throwingCollector, + mock(Installer.class), + mock(Deployer.class), + mock(LocalRepositoryProvider.class), + new StubSyncContextFactory(), + new DefaultRemoteRepositoryManager( + new DefaultUpdatePolicyAnalyzer(), + new DefaultChecksumPolicyProvider(), + new DefaultRepositoryKeyFunctionFactory()), + new DefaultRepositorySystemLifecycle(), + Collections.emptyMap(), + new DefaultRepositorySystemValidator( + Collections.singletonList(EXPRESSION_REJECTING_VALIDATOR_FACTORY))); + + // First call: collectDependencies should throw because the collector throws + CollectRequest collectRequest = new CollectRequest(); + collectRequest.setRootArtifact(new DefaultArtifact("g:root:1.0")); + assertThrows(RuntimeException.class, () -> throwingSystem.collectDependencies(session, collectRequest)); + + // Second call: resolveVersionRange should still validate (depth counter was reset + // by the try-finally guard despite the exception). If the depth counter leaked, + // this call would skip validation and accept the uninterpolated expression. + VersionRangeRequest request = new VersionRangeRequest( + new DefaultArtifact("g:bad:${unresolved}"), Collections.emptyList(), null); + assertThrows( + IllegalArgumentException.class, + () -> throwingSystem.resolveVersionRange(session, request), + "Depth counter should be reset after exception — validation must still run"); + } } From 94d9fda5b7467f23f38a50c3d943f648d7d13fb4 Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Tue, 28 Jul 2026 10:49:58 +0200 Subject: [PATCH 5/5] fix: spotless formatting violation in test Co-Authored-By: Claude Opus 4.6 --- .../internal/impl/DefaultRepositorySystemReentrancyTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemReentrancyTest.java b/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemReentrancyTest.java index 3787d2a60c..8e39d74659 100644 --- a/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemReentrancyTest.java +++ b/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemReentrancyTest.java @@ -465,8 +465,8 @@ void threadScopedDepthIsDecrementedWhenDelegateThrows() throws Exception { // Second call: resolveVersionRange should still validate (depth counter was reset // by the try-finally guard despite the exception). If the depth counter leaked, // this call would skip validation and accept the uninterpolated expression. - VersionRangeRequest request = new VersionRangeRequest( - new DefaultArtifact("g:bad:${unresolved}"), Collections.emptyList(), null); + VersionRangeRequest request = + new VersionRangeRequest(new DefaultArtifact("g:bad:${unresolved}"), Collections.emptyList(), null); assertThrows( IllegalArgumentException.class, () -> throwingSystem.resolveVersionRange(session, request),