gnodet-bot commented on code in PR #13081:
URL: https://github.com/apache/maven/pull/13081#discussion_r4069495884
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelBuilder.java:
##########
@@ -2505,21 +2493,115 @@ private DependencyManagement
loadDependencyManagement(Dependency dependency, Col
.build();
}
+ private Model loadImportModel(Dependency dependency, ImportContext
context) {
+ Collection<String> importIds = context.importIds;
+ String groupId = dependency.getGroupId();
+ String artifactId = dependency.getArtifactId();
+ String version = dependency.getVersion();
+ String imported = groupId + ':' + artifactId + ':' + version;
+
+ if (importIds.contains(imported)) {
+ StringBuilder message = new StringBuilder("The import POMs
form a cycle: ");
+ for (String modelId : importIds) {
+ message.append(modelId).append(" -> ");
+ }
+ message.append(imported);
+ if (context.cycleIncludesRelocation(imported)) {
+ context.reportRelocationProblem(message.toString(),
dependency.getLocation(""), null);
+ } else {
+ add(Severity.ERROR, Version.BASE, message.toString());
+ }
+ return null;
+ }
+
+ ImportModelCacheEntry cached =
+ cache(repositories, groupId, artifactId, version, null,
IMPORT, ImportModelCacheEntry::new);
+ ImportedModel importedModel = cached.model;
+ if (importedModel == null) {
+ boolean locked = cached.lock.tryLock();
+ if (!locked && context.relocationSources.isEmpty()) {
+ cached.lock.lock();
+ locked = true;
+ }
+ try {
+ importedModel = cached.model;
+ if (importedModel == null) {
+ // A relocation may lead back to an import being built
by another thread.
+ // Only these paths avoid waiting; ordinary imports
still share one in-flight build.
+ importedModel = doLoadDependencyManagement(dependency,
groupId, artifactId, version, context);
+ if (locked && importedModel != null) {
+ cached.model = importedModel;
+ }
+ }
+ } finally {
+ if (locked) {
+ cached.lock.unlock();
+ }
+ }
+ }
+ if (importedModel == null) {
+ return null;
+ }
+
+ Model importModel = importedModel.model();
+ Relocation relocation = importModel.getDistributionManagement() !=
null
+ ? importModel.getDistributionManagement().getRelocation()
+ : null;
+ if (relocation != null) {
+ if (!validateRelocationCoordinate(relocation.getGroupId(),
"groupId", dependency, context)
+ ||
!validateRelocationCoordinate(relocation.getArtifactId(), "artifactId",
dependency, context)
+ ||
!validateRelocationCoordinate(relocation.getVersion(), "version", dependency,
context)) {
+ return null;
+ }
+ Dependency.Builder relocated =
Dependency.newBuilder(dependency).version(importedModel.version());
+ if (relocation.getGroupId() != null &&
!relocation.getGroupId().isEmpty()) {
+ relocated.groupId(relocation.getGroupId());
+ }
+ if (relocation.getArtifactId() != null
+ && !relocation.getArtifactId().isEmpty()) {
+ relocated.artifactId(relocation.getArtifactId());
+ }
+ if (relocation.getVersion() != null &&
!relocation.getVersion().isEmpty()) {
+ relocated.version(relocation.getVersion());
+ }
+ Dependency relocatedDependency = relocated.build();
+ String message = "The import POM " + imported + " has been
relocated to "
+ + relocatedDependency.getGroupId() + ':' +
relocatedDependency.getArtifactId() + ':'
+ + relocatedDependency.getVersion();
+ if (relocation.getMessage() != null) {
+ message += ": " + relocation.getMessage();
+ }
+ add(Severity.WARNING, Version.BASE, message,
dependency.getLocation(""));
Review Comment:
Same issue as in the compat builder: the WARNING is emitted unconditionally,
including for transitive relocation hops. `ImportContext.relocationSources` is
already the right signal — it is non-empty exactly when we are inside a
transitive hop. Use it.
```suggestion
String message = "The import POM " + imported + " has been
relocated to "
+ relocatedDependency.getGroupId() + ':' +
relocatedDependency.getArtifactId() + ':'
+ relocatedDependency.getVersion();
if (relocation.getMessage() != null) {
message += ": " + relocation.getMessage();
}
// Only warn when actionable: the user declared this import
directly and can update
// the coordinates. Transitive hops (relocationSources
non-empty) are not actionable
// — log at DEBUG to match
DistributionManagementArtifactRelocationSource behaviour.
if (context.relocationSources().isEmpty()) {
add(Severity.WARNING, Version.BASE, message,
dependency.getLocation(""));
} else {
LOGGER.debug(message);
}
```
##########
compat/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java:
##########
@@ -1187,136 +1182,227 @@ private void importDependencyManagement(
it.remove();
- String groupId = dependency.getGroupId();
- String artifactId = dependency.getArtifactId();
- String version = dependency.getVersion();
-
- if (groupId == null || groupId.length() <= 0) {
- problems.add(new ModelProblemCollectorRequest(Severity.ERROR,
Version.BASE)
-
.setMessage("'dependencyManagement.dependencies.dependency.groupId' for "
- + dependency.getManagementKey() + " is
missing.")
- .setLocation(dependency.getLocation("")));
- continue;
- }
- if (artifactId == null || artifactId.length() <= 0) {
- problems.add(new ModelProblemCollectorRequest(Severity.ERROR,
Version.BASE)
-
.setMessage("'dependencyManagement.dependencies.dependency.artifactId' for "
- + dependency.getManagementKey() + " is
missing.")
- .setLocation(dependency.getLocation("")));
- continue;
- }
- if (version == null || version.length() <= 0) {
- problems.add(new ModelProblemCollectorRequest(Severity.ERROR,
Version.BASE)
-
.setMessage("'dependencyManagement.dependencies.dependency.version' for "
- + dependency.getManagementKey() + " is
missing.")
- .setLocation(dependency.getLocation("")));
- continue;
+ DependencyManagement importedManagement =
+ loadDependencyManagement(dependency, model, request,
problems, importIds);
+ if (importedManagement != null) {
+ if (importedManagements == null) {
+ importedManagements = new ArrayList<>();
+ }
+ importedManagements.add(importedManagement);
}
+ }
- String imported = groupId + ':' + artifactId + ':' + version;
+ importIds.remove(importing);
- if (importIds.contains(imported)) {
- StringBuilder message =
- new StringBuilder("The dependencies of type=pom and
with scope=import form a cycle: ");
- for (String modelId : importIds) {
- message.append(modelId);
- message.append(" -> ");
- }
- message.append(imported);
- problems.add(
- new ModelProblemCollectorRequest(Severity.ERROR,
Version.BASE).setMessage(message.toString()));
+ dependencyManagementImporter.importManagement(model,
importedManagements, request, problems);
+ }
- continue;
+ private DependencyManagement loadDependencyManagement(
+ Dependency dependency,
+ Model model,
+ ModelBuildingRequest request,
+ DefaultModelProblemCollector problems,
+ Collection<String> importIds) {
+ String groupId = dependency.getGroupId();
+ String artifactId = dependency.getArtifactId();
+ String version = dependency.getVersion();
+
+ if (groupId == null || groupId.length() <= 0) {
+ problems.add(new ModelProblemCollectorRequest(Severity.ERROR,
Version.BASE)
+
.setMessage("'dependencyManagement.dependencies.dependency.groupId' for "
+ + dependency.getManagementKey() + " is missing.")
+ .setLocation(dependency.getLocation("")));
+ return null;
+ }
+ if (artifactId == null || artifactId.length() <= 0) {
+ problems.add(new ModelProblemCollectorRequest(Severity.ERROR,
Version.BASE)
+
.setMessage("'dependencyManagement.dependencies.dependency.artifactId' for "
+ + dependency.getManagementKey() + " is missing.")
+ .setLocation(dependency.getLocation("")));
+ return null;
+ }
+ if (version == null || version.length() <= 0) {
+ problems.add(new ModelProblemCollectorRequest(Severity.ERROR,
Version.BASE)
+
.setMessage("'dependencyManagement.dependencies.dependency.version' for "
+ + dependency.getManagementKey() + " is missing.")
+ .setLocation(dependency.getLocation("")));
+ return null;
+ }
+
+ String imported = groupId + ':' + artifactId + ':' + version;
+
+ if (importIds.contains(imported)) {
+ StringBuilder message = new StringBuilder("The import POMs form a
cycle: ");
+ for (String modelId : importIds) {
+ message.append(modelId);
+ message.append(" -> ");
}
+ message.append(imported);
+ problems.add(new ModelProblemCollectorRequest(Severity.ERROR,
Version.BASE).setMessage(message.toString()));
+
+ return null;
+ }
- DependencyManagement importMgmt =
- getCache(request.getModelCache(), groupId, artifactId,
version, ModelCacheTag.IMPORT);
+ DependencyManagement importedManagement =
+ getCache(request.getModelCache(), groupId, artifactId,
version, ModelCacheTag.IMPORT);
- if (importMgmt == null) {
- if (workspaceResolver == null && modelResolver == null) {
- throw new NullPointerException(String.format(
- "request.workspaceModelResolver and
request.modelResolver cannot be null"
- + " (parent POM %s and POM %s)",
- ModelProblemUtils.toId(groupId, artifactId,
version),
- ModelProblemUtils.toSourceHint(model)));
- }
+ if (importedManagement == null) {
+ Model importModel = resolveImportModel(dependency, model, request,
problems, importIds);
+ if (importModel == null) {
+ return null;
+ }
- Model importModel = null;
- if (workspaceResolver != null) {
- try {
- importModel =
workspaceResolver.resolveEffectiveModel(groupId, artifactId, version);
- } catch (UnresolvableModelException e) {
- problems.add(new
ModelProblemCollectorRequest(Severity.FATAL, Version.BASE)
- .setMessage(e.getMessage())
- .setException(e));
- continue;
- }
+ Relocation relocation = importModel.getDistributionManagement() !=
null
+ ? importModel.getDistributionManagement().getRelocation()
+ : null;
+ if (relocation != null) {
+ Dependency relocated = dependency.clone();
+ if (!validateRelocationCoordinate(relocation.getGroupId(),
"groupId", dependency, problems)
+ ||
!validateRelocationCoordinate(relocation.getArtifactId(), "artifactId",
dependency, problems)
+ ||
!validateRelocationCoordinate(relocation.getVersion(), "version", dependency,
problems)) {
+ return null;
}
-
- // no workspace resolver or workspace resolver returned null
(i.e. model not in workspace)
- if (importModel == null) {
- final ModelSource importSource;
+ if (relocation.getGroupId() != null &&
!relocation.getGroupId().isEmpty()) {
+ relocated.setGroupId(relocation.getGroupId());
+ }
+ if (relocation.getArtifactId() != null
+ && !relocation.getArtifactId().isEmpty()) {
+ relocated.setArtifactId(relocation.getArtifactId());
+ }
+ if (relocation.getVersion() != null &&
!relocation.getVersion().isEmpty()) {
+ relocated.setVersion(relocation.getVersion());
+ }
+ String message = "The import POM " + imported + " has been
relocated to " + relocated.getGroupId() + ':'
+ + relocated.getArtifactId() + ':' +
relocated.getVersion();
+ if (relocation.getMessage() != null) {
+ message += ": " + relocation.getMessage();
+ }
+ problems.add(new
ModelProblemCollectorRequest(Severity.WARNING, Version.BASE)
+ .setMessage(message)
+ .setLocation(dependency.getLocation("")));
Review Comment:
The warning is emitted unconditionally — including for transitive relocation
hops where the user has nothing to update. Only emit `WARNING` when it is
actually actionable (direct import). Transitive hops should be `DEBUG`.
`ImportContext` doesn't exist in the compat builder, but you can check
whether `importIds` is at its initial depth — or simply replicate the
`relocationSources`-based check. The simplest approach: add a `Logger` field
and split on whether this is a direct or transitive call.
```suggestion
String message = "The import POM " + imported + " has been
relocated to " + relocated.getGroupId() + ':'
+ relocated.getArtifactId() + ':' +
relocated.getVersion();
if (relocation.getMessage() != null) {
message += ": " + relocation.getMessage();
}
// Only warn when the user directly declared this import and
can act on it.
// Transitive relocation hops (a BOM re-importing a
relocated BOM) are not
// actionable by the user — log at DEBUG to match artifact
relocation behaviour.
if (importIds.isEmpty()) {
problems.add(new
ModelProblemCollectorRequest(Severity.WARNING, Version.BASE)
.setMessage(message)
.setLocation(dependency.getLocation("")));
} else {
LOGGER.debug(message);
}
```
--
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]
