gnodet-bot commented on code in PR #12714:
URL: https://github.com/apache/maven/pull/12714#discussion_r4046302810
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelBuilder.java:
##########
@@ -1766,13 +1580,9 @@ Model doReadFileModel(Set<Path> activeModelReads) throws
ModelBuilderException {
Path rootDirectory;
boolean rootDirectoryFromSession = false;
setSource(modelSource.getLocation());
- logger.debug("Reading file model from " +
modelSource.getLocation());
+ logger.trace("Reading file model from " +
modelSource.getLocation());
Path sourcePath = modelSource.getPath();
- // Use toAbsolutePath().normalize() for consistent path identity
in activeModelReads.
- // This must match the normalization used in
getEnhancedProperties() guard check
- // to prevent StackOverflowError from path representation
mismatches (GH-12598).
- Path normalizedPath =
- sourcePath != null ?
sourcePath.toAbsolutePath().normalize() : null;
+ Path normalizedPath = sourcePath != null ? sourcePath.normalize()
: null;
Review Comment:
⚠️ **Regression (GH-12598): `sourcePath.normalize()` without
`toAbsolutePath()`**
The base branch used `sourcePath.toAbsolutePath().normalize()` here to
ensure a canonical, absolute path is stored in `activeModelReads`. The comment
that was removed explicitly explained why:
> Use `toAbsolutePath().normalize()` for consistent path identity in
`activeModelReads`. This must match the normalization used in
`getEnhancedProperties()` guard check to prevent StackOverflowError from path
representation mismatches (GH-12598).
With only `.normalize()`, a relative `sourcePath` (e.g. when
`modelSource.getPath()` returns a relative `Path`) will be stored as-is. The
guard in `getEnhancedProperties` at line 751 uses `rootModelPath.normalize()` —
also without `toAbsolutePath()`. If the two paths represent the same file but
one is absolute and one is relative, the `contains()` check returns `false` and
the StackOverflow reappears on projects that triggered GH-12598.
```suggestion
Path normalizedPath = sourcePath != null ?
sourcePath.toAbsolutePath().normalize() : null;
```
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelBuilder.java:
##########
@@ -892,11 +747,8 @@ private Map<String, String> getEnhancedProperties(Model
model, Path rootDirector
// Also skip if the root model is already being read in an
outer call frame
// to prevent StackOverflowError when a project has an
internal parent in a
// subdirectory with CI-friendly ${revision} and a .mvn/
root marker (GH-12301).
- // Use toAbsolutePath().normalize() for the guard check to
handle paths
- // obtained via different representations (e.g., symlinks,
relative segments).
if (isParentWithinRootDirectory(rootModelPath,
rootDirectory)
- && !activeModelReads.contains(
-
rootModelPath.toAbsolutePath().normalize())) {
+ &&
!activeModelReads.contains(rootModelPath.normalize())) {
Review Comment:
⚠️ **Regression (GH-12301/GH-12598): `rootModelPath.normalize()` without
`toAbsolutePath()`**
The base branch guard was `rootModelPath.toAbsolutePath().normalize()` to
match the identity of entries put into `activeModelReads` by `doReadFileModel`.
That comment was deleted here:
> Use `toAbsolutePath().normalize()` for the guard check to handle paths
obtained via different representations (e.g., symlinks, relative segments).
The `rootModelPath` comes from
`modelProcessor.locateExistingPom(rootDirectory)`. If that returns a relative
or symlinked path while the entry in `activeModelReads` is an absolute path (or
vice versa), the cycle guard fires when it shouldn't, or misses when it should
fire.
```suggestion
&&
!activeModelReads.contains(rootModelPath.toAbsolutePath().normalize())) {
```
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelBuilder.java:
##########
@@ -291,91 +289,24 @@ List<RemoteRepository> getExternalRepositories() {
// Contains both GAV coordinates (groupId:artifactId:version) and file
paths
final Set<String> parentChain;
- // Sticky across derive(): true for a session that is itself resolving
a dependency
- // (ModelBuilderRequest.RequestType.CONSUMER_DEPENDENCY), or that was
derived, directly
- // or transitively, from such a session -- for instance a dependency's
own parent POM.
- // Kept separate from request.getRequestType() because a parent lookup
always derives a
- // CONSUMER_PARENT request regardless of what kind of session
triggered it, which would
- // otherwise lose the distinction this flag preserves.
- final boolean externalOrigin;
-
ModelBuilderSessionState(ModelBuilderRequest request) {
this(
request.getSession(),
request,
new DefaultModelBuilderResult(request,
ProblemCollector.create(request.getSession())),
new Graph(),
new ConcurrentHashMap<>(64),
- ConcurrentHashMap.newKeySet(),
- new ConcurrentHashMap<>(),
List.of(),
repos(request),
repos(request),
- new LinkedHashSet<>(),
- isExternalOrigin(request));
+ new LinkedHashSet<>());
}
static List<RemoteRepository> repos(ModelBuilderRequest request) {
- List<RemoteRepository> repos = request.getRepositories() != null
- ? request.getRepositories()
- : request.getSession().getRemoteRepositories();
- return mergeRepositoriesById(repos);
- }
-
- /**
- * Merges repositories that share the same ID by combining their
policies.
- * This handles the case where mirror injection produces multiple
repository
- * entries with the same mirror ID but different snapshot/release
policies
- * (e.g., when both "central" and a profile-defined repo are mirrored
to the
- * same mirror, producing two entries with the mirror's ID but
different policies).
- * Without this merge, policy deduplication in the resolver can drop
the snapshot
- * policy, causing SNAPSHOT parent resolution to fail (MNG-12769).
- */
- private static List<RemoteRepository>
mergeRepositoriesById(List<RemoteRepository> repos) {
- if (repos.size() <= 1) {
- return List.copyOf(repos);
- }
- LinkedHashMap<String, RemoteRepository> byId = new
LinkedHashMap<>();
- boolean hasDuplicates = false;
- for (RemoteRepository repo : repos) {
- RemoteRepository existing = byId.putIfAbsent(repo.getId(),
repo);
- if (existing != null) {
- hasDuplicates = true;
- byId.put(repo.getId(), mergeRepositoryPolicies(existing,
repo));
- }
- }
- return hasDuplicates ? List.copyOf(byId.values()) :
List.copyOf(repos);
- }
-
- /**
- * Merges two repositories with the same ID by combining their
policies.
- * For each policy type (release/snapshot), the result is enabled if
either
- * input has it enabled. URL, proxy, authentication, and other
properties
- * are preserved from the dominant (first) repository.
- */
- private static RemoteRepository
mergeRepositoryPolicies(RemoteRepository dominant, RemoteRepository recessive) {
- if (dominant instanceof DefaultRemoteRepository d && recessive
instanceof DefaultRemoteRepository r) {
- org.eclipse.aether.repository.RemoteRepository dr =
d.getRepository();
- org.eclipse.aether.repository.RemoteRepository rr =
r.getRepository();
-
- boolean mergeSnapshots =
- rr.getPolicy(true).isEnabled() &&
!dr.getPolicy(true).isEnabled();
- boolean mergeReleases =
- rr.getPolicy(false).isEnabled() &&
!dr.getPolicy(false).isEnabled();
-
- if (mergeSnapshots || mergeReleases) {
- org.eclipse.aether.repository.RemoteRepository.Builder
builder =
- new
org.eclipse.aether.repository.RemoteRepository.Builder(dr);
- if (mergeSnapshots) {
- builder.setSnapshotPolicy(rr.getPolicy(true));
- }
- if (mergeReleases) {
- builder.setReleasePolicy(rr.getPolicy(false));
- }
- return new DefaultRemoteRepository(builder.build());
- }
- }
- return dominant;
+ return List.copyOf(
+ request.getRepositories() != null
+ ? request.getRepositories()
Review Comment:
⚠️ **Regression (MNG-12769): `mergeRepositoriesById` removed**
The base branch's `repos()` method called `mergeRepositoriesById()` which
merged duplicate repo entries (same ID, different snapshot/release policies)
that mirror injection can produce. The Javadoc explained:
> Without this merge, policy deduplication in the resolver can drop the
snapshot policy, causing SNAPSHOT parent resolution to fail (MNG-12769).
This PR replaces `repos()` with a plain `List.copyOf(...)`, silently
reintroducing the MNG-12769 bug for projects that use mirrors with split
release/snapshot policies.
The fix should be re-applied here, or moved to a layer below
`ModelBuilderSessionState` if the intent is to clean up its scope.
--
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]
