gnodet commented on code in PR #11904:
URL: https://github.com/apache/maven/pull/11904#discussion_r3284342495
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelNormalizer.java:
##########
@@ -76,15 +81,20 @@ public Model mergeDuplicates(Model model,
ModelBuilderRequest request, ModelProb
* the way 2.x works. When we're in strict mode, the removal of
duplicates just saves other merging steps from
* aftereffects and bogus error messages.
*/
+ // Expand id attributes on dependencies (and their exclusions), then
deduplicate
List<Dependency> dependencies = model.getDependencies();
- Map<String, Dependency> normalized = new
LinkedHashMap<>(dependencies.size() * 2);
+ List<Dependency> expanded = injectList(dependencies,
this::expandDependencyId);
+ if (expanded != null) {
Review Comment:
This only expands `id` on top-level dependencies. Dependency management
dependencies (`model.getDependencyManagement().getDependencies()`) and
profile-scoped dependencies are also validated for `id` attribute conflicts
(see `DefaultModelValidator` lines 536-596), but they won't be expanded here.
Should the expansion also apply to:
- `model.getDependencyManagement().getDependencies()`
- `profile.getDependencies()`
- `profile.getDependencyManagement().getDependencies()`
Without this, a user writing `<dependency id="g:a:v"/>` in
`<dependencyManagement>` will pass validation but the GAV fields will never be
populated.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelNormalizer.java:
##########
@@ -144,4 +154,92 @@ private <T> List<T> injectList(List<T> list,
UnaryOperator<T> modifer) {
}
return newList;
}
+
+ /**
+ * Expands the {@code id} attribute on a dependency into its component
fields.
+ * The id format is {@code groupId:artifactId:version}.
+ */
+ Dependency expandDependencyId(Dependency d) {
+ String id = d.getId();
+ if (id == null || id.isEmpty()) {
+ // No id attribute, but still expand exclusion ids
+ List<Exclusion> expanded = injectList(d.getExclusions(),
this::expandExclusionId);
+ return expanded != null ? d.withExclusions(expanded) : d;
+ }
+ String[] parts = id.split(":");
+ if (parts.length != 3) {
+ // Invalid format — will be caught by the validator
+ return d;
+ }
+ Dependency.Builder builder = Dependency.newBuilder(d);
+ if (isBlank(d.getGroupId())) {
+ builder.groupId(parts[0]);
+ }
+ if (isBlank(d.getArtifactId())) {
+ builder.artifactId(parts[1]);
+ }
+ if (isBlank(d.getVersion())) {
+ builder.version(parts[2]);
+ }
+ List<Exclusion> expanded = injectList(d.getExclusions(),
this::expandExclusionId);
+ if (expanded != null) {
+ builder.exclusions(expanded);
+ }
+ return builder.build();
+ }
+
+ /**
+ * Expands the {@code id} attribute on an exclusion into its component
fields.
+ * The id format is {@code groupId:artifactId}.
+ */
+ Exclusion expandExclusionId(Exclusion e) {
+ String id = e.getId();
+ if (id == null || id.isEmpty()) {
+ return e;
+ }
+ String[] parts = id.split(":");
+ if (parts.length != 2) {
+ // Invalid format — will be caught by the validator
+ return e;
+ }
+ Exclusion.Builder builder = Exclusion.newBuilder(e);
+ if (isBlank(e.getGroupId())) {
+ builder.groupId(parts[0]);
+ }
+ if (isBlank(e.getArtifactId())) {
+ builder.artifactId(parts[1]);
+ }
+ return builder.build();
+ }
+
+ /**
+ * Expands the {@code id} (XML attribute) / {@code gav} (Java field) on a
mixin
+ * into its component fields. The format is {@code
groupId:artifactId:version}.
+ */
+ Mixin expandMixinGav(Mixin m) {
+ String gav = m.getGav();
+ if (gav == null || gav.isEmpty()) {
+ return m;
+ }
+ String[] parts = gav.split(":");
+ if (parts.length != 3) {
+ // Invalid format — will be caught by the validator
+ return m;
+ }
+ Mixin.Builder builder = Mixin.newBuilder(m);
+ if (isBlank(m.getGroupId())) {
+ builder.groupId(parts[0]);
+ }
+ if (isBlank(m.getArtifactId())) {
+ builder.artifactId(parts[1]);
+ }
+ if (isBlank(m.getVersion())) {
+ builder.version(parts[2]);
+ }
+ return builder.build();
+ }
+
Review Comment:
Nit: this method name is misleading -- `String.isBlank()` in Java checks for
whitespace-only strings, but this only checks null/empty. Consider renaming to
`isNullOrEmpty` to avoid confusion, or use `s == null || s.isBlank()` to also
handle whitespace-only values in coordinates.
```suggestion
private static boolean isNullOrEmpty(String s) {
return s == null || s.isEmpty();
}
```
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelNormalizer.java:
##########
@@ -144,4 +154,92 @@ private <T> List<T> injectList(List<T> list,
UnaryOperator<T> modifer) {
}
return newList;
}
+
+ /**
+ * Expands the {@code id} attribute on a dependency into its component
fields.
+ * The id format is {@code groupId:artifactId:version}.
+ */
+ Dependency expandDependencyId(Dependency d) {
+ String id = d.getId();
+ if (id == null || id.isEmpty()) {
+ // No id attribute, but still expand exclusion ids
+ List<Exclusion> expanded = injectList(d.getExclusions(),
this::expandExclusionId);
+ return expanded != null ? d.withExclusions(expanded) : d;
+ }
+ String[] parts = id.split(":");
Review Comment:
The normalizer silently ignores the `id` when the format is wrong
(`parts.length != 3`), deferring to the validator. But if the validator and
normalizer both split on `:`, and the user writes `g:a:v:extra`, the split
produces 4 parts -- the normalizer skips it, and the validator also rejects it.
This is fine.
However, consider that `String.split(":")` has edge cases: trailing empty
strings are removed by default. For example, `"g:a:"` splits to `["g", "a"]`
(length 2, not 3). This means a dependency `id="g:a:"` would silently fail
normalization AND pass the `parts.length != 3` check in the validator --
resulting in a format error, which is correct. Just worth being aware of the
edge case.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java:
##########
@@ -1138,7 +1187,22 @@ private void validate20RawDependencies(
Map<String, Dependency> index = new HashMap<>();
for (Dependency dependency : dependencies) {
- String key = dependency.getManagementKey();
+ // When 'id' attribute is set, use it for the key since
groupId/artifactId
+ // have not yet been expanded (normalization runs after validation)
+ String key;
+ if (dependency.getId() != null && !dependency.getId().isEmpty()) {
+ key = dependency.getId();
+ } else {
+ key = dependency.getManagementKey();
Review Comment:
Good approach using `dependency.getId()` as the dedup key when set, since
normalization hasn't run yet at this point. However, two dependencies with the
same `id` attribute but different scopes/classifiers would collide here (since
the key is just the raw `id` string, not the management key). After
normalization, they'd have distinct management keys (which includes type and
classifier). Is this the intended behavior?
--
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]
