From 3f1990923fa4afa4deadbf24d4caea49a8fde336 Mon Sep 17 00:00:00 2001 From: Rob Bygrave Date: Wed, 8 Apr 2026 18:08:53 +1200 Subject: [PATCH 01/11] Feat: Add id attribute (gav) support to Dependency, Exclusion, Mixin Based on design discussion on Issue #11500, the proposal is to support `groupId:artifactId:version` format for Dependency, Exclusion and Mixin. This means that we can define dependencies in the form: ```xml ``` Examples: ```xml ``` ```xml ``` ```xml ``` --- api/maven-api-model/src/main/mdo/maven.mdo | 42 +++++ .../impl/model/DefaultModelNormalizer.java | 106 ++++++++++- .../impl/model/DefaultModelObjectPool.java | 4 +- .../impl/model/DefaultModelValidator.java | 162 ++++++++++++++++- .../model/DefaultModelNormalizerTest.java | 165 ++++++++++++++++++ .../impl/model/DefaultModelValidatorTest.java | 42 +++++ .../validation/dependency-id-bad-format.xml | 10 ++ .../validation/dependency-id-conflict.xml | 12 ++ .../poms/validation/dependency-id-valid.xml | 13 ++ .../validation/exclusion-id-bad-format.xml | 14 ++ .../poms/validation/exclusion-id-valid.xml | 14 ++ .../maven/model/v4/MavenStaxReaderTest.java | 68 ++++++++ 12 files changed, 646 insertions(+), 6 deletions(-) create mode 100644 impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelNormalizerTest.java create mode 100644 impl/maven-impl/src/test/resources/poms/validation/dependency-id-bad-format.xml create mode 100644 impl/maven-impl/src/test/resources/poms/validation/dependency-id-conflict.xml create mode 100644 impl/maven-impl/src/test/resources/poms/validation/dependency-id-valid.xml create mode 100644 impl/maven-impl/src/test/resources/poms/validation/exclusion-id-bad-format.xml create mode 100644 impl/maven-impl/src/test/resources/poms/validation/exclusion-id-valid.xml diff --git a/api/maven-api-model/src/main/mdo/maven.mdo b/api/maven-api-model/src/main/mdo/maven.mdo index 74aff7785e54..1a08abffb1d7 100644 --- a/api/maven-api-model/src/main/mdo/maven.mdo +++ b/api/maven-api-model/src/main/mdo/maven.mdo @@ -1204,6 +1204,20 @@ ]]> + + id + 4.2.0+ + + + + String + groupId 3.0.0+ @@ -1440,6 +1454,20 @@ ]]> + + id + 4.2.0+ + + + + String + groupId 4.0.0+ @@ -1863,6 +1891,20 @@ 4.1.0+ Parent + + gav + 4.2.0+ + + + + String + classifier 4.1.0+ diff --git a/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelNormalizer.java b/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelNormalizer.java index f36918f770aa..b903df0b384f 100644 --- a/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelNormalizer.java +++ b/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelNormalizer.java @@ -29,6 +29,8 @@ import org.apache.maven.api.di.Singleton; import org.apache.maven.api.model.Build; import org.apache.maven.api.model.Dependency; +import org.apache.maven.api.model.Exclusion; +import org.apache.maven.api.model.Mixin; import org.apache.maven.api.model.Model; import org.apache.maven.api.model.Plugin; import org.apache.maven.api.services.ModelBuilderRequest; @@ -49,6 +51,9 @@ public class DefaultModelNormalizer implements ModelNormalizer { public Model mergeDuplicates(Model model, ModelBuilderRequest request, ModelProblemCollector problems) { Model.Builder builder = Model.newBuilder(model); + // Expand id attributes on mixins + builder.mixins(injectList(model.getMixins(), this::expandMixinGav)); + Build build = model.getBuild(); if (build != null) { List plugins = build.getPlugins(); @@ -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 dependencies = model.getDependencies(); - Map normalized = new LinkedHashMap<>(dependencies.size() * 2); + List expanded = injectList(dependencies, this::expandDependencyId); + if (expanded != null) { + dependencies = expanded; + } + Map normalizedDeps = new LinkedHashMap<>(dependencies.size() * 2); for (Dependency dependency : dependencies) { - normalized.put(dependency.getManagementKey(), dependency); + normalizedDeps.put(dependency.getManagementKey(), dependency); } - if (dependencies.size() != normalized.size()) { - builder.dependencies(normalized.values()); + if (expanded != null || dependencies.size() != normalizedDeps.size()) { + builder.dependencies(normalizedDeps.values()); } return builder.build(); @@ -144,4 +154,92 @@ private List injectList(List list, UnaryOperator 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 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 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(); + } + + private static boolean isBlank(String s) { + return s == null || s.isEmpty(); + } } diff --git a/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelObjectPool.java b/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelObjectPool.java index bec34fe0fa76..03791aaee4c1 100644 --- a/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelObjectPool.java +++ b/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelObjectPool.java @@ -237,7 +237,8 @@ private static boolean objectsEqual(Object obj1, Object obj2) { */ private static boolean dependenciesEqual( org.apache.maven.api.model.Dependency dep1, org.apache.maven.api.model.Dependency dep2) { - return Objects.equals(dep1.getGroupId(), dep2.getGroupId()) + return Objects.equals(dep1.getId(), dep2.getId()) + && Objects.equals(dep1.getGroupId(), dep2.getGroupId()) && Objects.equals(dep1.getArtifactId(), dep2.getArtifactId()) && Objects.equals(dep1.getVersion(), dep2.getVersion()) && Objects.equals(dep1.getType(), dep2.getType()) @@ -286,6 +287,7 @@ private static int computeHashCode(Object obj) { */ private static int dependencyHashCode(org.apache.maven.api.model.Dependency dep) { return Objects.hash( + dep.getId(), dep.getGroupId(), dep.getArtifactId(), dep.getVersion(), diff --git a/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java b/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java index 72917cd7c635..324d01add40b 100644 --- a/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java +++ b/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java @@ -57,6 +57,7 @@ import org.apache.maven.api.model.Exclusion; import org.apache.maven.api.model.InputLocation; import org.apache.maven.api.model.InputLocationTracker; +import org.apache.maven.api.model.Mixin; import org.apache.maven.api.model.Model; import org.apache.maven.api.model.Parent; import org.apache.maven.api.model.Plugin; @@ -372,6 +373,54 @@ && equals(parent.getArtifactId(), model.getArtifactId())) { // Validate each mixin for (Parent mixin : model.getMixins()) { + // Validate id/gav attribute format and conflict with child elements + if (mixin instanceof Mixin m + && m.getGav() != null + && !m.getGav().isEmpty()) { + String gav = m.getGav(); + String[] parts = gav.split(":"); + if (parts.length != 3) { + addViolation( + problems, + Severity.ERROR, + Version.V42, + "mixins.mixin.id", + null, + "must have the format 'groupId:artifactId:version' but is '" + gav + "'.", + mixin); + } + if (mixin.getGroupId() != null && !mixin.getGroupId().isEmpty()) { + addViolation( + problems, + Severity.ERROR, + Version.V42, + "mixins.mixin.groupId", + null, + "must not be specified when the 'id' attribute is used.", + mixin); + } + if (mixin.getArtifactId() != null && !mixin.getArtifactId().isEmpty()) { + addViolation( + problems, + Severity.ERROR, + Version.V42, + "mixins.mixin.artifactId", + null, + "must not be specified when the 'id' attribute is used.", + mixin); + } + if (mixin.getVersion() != null && !mixin.getVersion().isEmpty()) { + addViolation( + problems, + Severity.ERROR, + Version.V42, + "mixins.mixin.version", + null, + "must not be specified when the 'id' attribute is used.", + mixin); + } + } + if (mixin.getRelativePath() != null && !mixin.getRelativePath().isEmpty() && (mixin.getGroupId() != null && !mixin.getGroupId().isEmpty() @@ -1138,7 +1187,22 @@ private void validate20RawDependencies( Map 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(); + } + + // Validate id attribute format and conflict with child elements + validateDependencyIdAttribute(problems, dependency, prefix + prefix2); + + // Validate id attribute on exclusions + for (Exclusion exclusion : dependency.getExclusions()) { + validateExclusionIdAttribute(problems, exclusion, prefix + prefix2 + "exclusions.exclusion."); + } if ("import".equals(dependency.getScope())) { if (!"pom".equals(dependency.getType())) { @@ -1255,6 +1319,102 @@ private void validate20RawDependencies( } } + /** + * Validates the {@code id} attribute on a dependency element. + * The id must have the format {@code groupId:artifactId:version} and must not + * be specified alongside individual groupId, artifactId, or version child elements. + */ + private void validateDependencyIdAttribute(ModelProblemCollector problems, Dependency dependency, String prefix) { + String id = dependency.getId(); + if (id == null || id.isEmpty()) { + return; + } + String[] parts = id.split(":"); + if (parts.length != 3) { + addViolation( + problems, + Severity.ERROR, + Version.V42, + prefix + "id", + null, + "must have the format 'groupId:artifactId:version' but is '" + id + "'.", + dependency); + } + if (dependency.getGroupId() != null && !dependency.getGroupId().isEmpty()) { + addViolation( + problems, + Severity.ERROR, + Version.V42, + prefix + "groupId", + null, + "must not be specified when the 'id' attribute is used.", + dependency); + } + if (dependency.getArtifactId() != null && !dependency.getArtifactId().isEmpty()) { + addViolation( + problems, + Severity.ERROR, + Version.V42, + prefix + "artifactId", + null, + "must not be specified when the 'id' attribute is used.", + dependency); + } + if (dependency.getVersion() != null && !dependency.getVersion().isEmpty()) { + addViolation( + problems, + Severity.ERROR, + Version.V42, + prefix + "version", + null, + "must not be specified when the 'id' attribute is used.", + dependency); + } + } + + /** + * Validates the {@code id} attribute on an exclusion element. + * The id must have the format {@code groupId:artifactId} and must not + * be specified alongside individual groupId or artifactId child elements. + */ + private void validateExclusionIdAttribute(ModelProblemCollector problems, Exclusion exclusion, String prefix) { + String id = exclusion.getId(); + if (id == null || id.isEmpty()) { + return; + } + String[] parts = id.split(":"); + if (parts.length != 2) { + addViolation( + problems, + Severity.ERROR, + Version.V42, + prefix + "id", + null, + "must have the format 'groupId:artifactId' but is '" + id + "'.", + exclusion); + } + if (exclusion.getGroupId() != null && !exclusion.getGroupId().isEmpty()) { + addViolation( + problems, + Severity.ERROR, + Version.V42, + prefix + "groupId", + null, + "must not be specified when the 'id' attribute is used.", + exclusion); + } + if (exclusion.getArtifactId() != null && !exclusion.getArtifactId().isEmpty()) { + addViolation( + problems, + Severity.ERROR, + Version.V42, + prefix + "artifactId", + null, + "must not be specified when the 'id' attribute is used.", + exclusion); + } + } + private void validate20RawDependenciesSelfReferencing( ModelProblemCollector problems, Model model, List dependencies, String prefix) { // We only check for groupId/artifactId/version/classifier cause if there is another diff --git a/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelNormalizerTest.java b/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelNormalizerTest.java new file mode 100644 index 000000000000..247017ddba09 --- /dev/null +++ b/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelNormalizerTest.java @@ -0,0 +1,165 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.impl.model; + +import org.apache.maven.api.model.Dependency; +import org.apache.maven.api.model.Exclusion; +import org.apache.maven.api.model.Mixin; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; + +class DefaultModelNormalizerTest { + + private final DefaultModelNormalizer normalizer = new DefaultModelNormalizer(); + + @Test + void testExpandDependencyId() { + Dependency dep = + Dependency.newBuilder().id("org.slf4j:slf4j-api:2.0.17").build(); + + Dependency result = normalizer.expandDependencyId(dep); + + assertEquals("org.slf4j", result.getGroupId()); + assertEquals("slf4j-api", result.getArtifactId()); + assertEquals("2.0.17", result.getVersion()); + assertEquals("org.slf4j:slf4j-api:2.0.17", result.getId()); + } + + @Test + void testExpandDependencyIdDoesNotOverrideExistingFields() { + Dependency dep = Dependency.newBuilder() + .id("org.slf4j:slf4j-api:2.0.17") + .groupId("org.override") + .build(); + + Dependency result = normalizer.expandDependencyId(dep); + + // Existing groupId should not be overridden + assertEquals("org.override", result.getGroupId()); + assertEquals("slf4j-api", result.getArtifactId()); + assertEquals("2.0.17", result.getVersion()); + } + + @Test + void testExpandDependencyIdInvalidFormat() { + Dependency dep = Dependency.newBuilder().id("invalid-no-colons").build(); + + Dependency result = normalizer.expandDependencyId(dep); + + // Invalid format — fields not populated, validator will catch this + assertNull(result.getGroupId()); + assertNull(result.getArtifactId()); + } + + @Test + void testExpandDependencyIdNull() { + Dependency dep = Dependency.newBuilder() + .groupId("org.example") + .artifactId("my-lib") + .version("1.0") + .build(); + + Dependency result = normalizer.expandDependencyId(dep); + + // No id attribute, should return unchanged + assertEquals("org.example", result.getGroupId()); + assertEquals("my-lib", result.getArtifactId()); + assertEquals("1.0", result.getVersion()); + } + + @Test + void testExpandExclusionId() { + Exclusion exc = Exclusion.newBuilder().id("com.example:unwanted-lib").build(); + + Exclusion result = normalizer.expandExclusionId(exc); + + assertEquals("com.example", result.getGroupId()); + assertEquals("unwanted-lib", result.getArtifactId()); + } + + @Test + void testExpandExclusionIdWildcard() { + Exclusion exc = Exclusion.newBuilder().id("*:*").build(); + + Exclusion result = normalizer.expandExclusionId(exc); + + assertEquals("*", result.getGroupId()); + assertEquals("*", result.getArtifactId()); + } + + @Test + void testExpandExclusionIdNull() { + Exclusion exc = + Exclusion.newBuilder().groupId("org.example").artifactId("lib").build(); + + Exclusion result = normalizer.expandExclusionId(exc); + + // No id attribute, should return same instance + assertEquals("org.example", result.getGroupId()); + assertEquals("lib", result.getArtifactId()); + } + + @Test + void testExpandMixinGav() { + Mixin mixin = + Mixin.newBuilder().gav("com.example.mixins:java-mixin:1.0.0").build(); + + Mixin result = normalizer.expandMixinGav(mixin); + + assertEquals("com.example.mixins", result.getGroupId()); + assertEquals("java-mixin", result.getArtifactId()); + assertEquals("1.0.0", result.getVersion()); + } + + @Test + void testExpandMixinGavNull() { + Mixin mixin = Mixin.newBuilder() + .groupId("com.example") + .artifactId("my-mixin") + .version("2.0") + .build(); + + Mixin result = normalizer.expandMixinGav(mixin); + + // No gav attribute, should return same instance + assertEquals("com.example", result.getGroupId()); + assertEquals("my-mixin", result.getArtifactId()); + assertEquals("2.0", result.getVersion()); + } + + @Test + void testExpandDependencyIdAlsoExpandsExclusions() { + Exclusion exc = Exclusion.newBuilder().id("com.example:unwanted").build(); + Dependency dep = Dependency.newBuilder() + .id("org.slf4j:slf4j-api:2.0.17") + .exclusions(java.util.List.of(exc)) + .build(); + + Dependency result = normalizer.expandDependencyId(dep); + + assertEquals("org.slf4j", result.getGroupId()); + assertEquals("slf4j-api", result.getArtifactId()); + assertEquals("2.0.17", result.getVersion()); + assertEquals(1, result.getExclusions().size()); + assertEquals("com.example", result.getExclusions().get(0).getGroupId()); + assertEquals("unwanted", result.getExclusions().get(0).getArtifactId()); + } +} diff --git a/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelValidatorTest.java b/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelValidatorTest.java index df38b262f720..466fc0142558 100644 --- a/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelValidatorTest.java +++ b/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelValidatorTest.java @@ -37,6 +37,7 @@ import org.junit.jupiter.api.Test; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.mock; @@ -1009,4 +1010,45 @@ void profileActivationConditionWithBasedirExpression() throws Exception { "raw-model/profile-activation-condition-with-basedir.xml", ModelValidator.VALIDATION_LEVEL_STRICT); assertViolations(result, 0, 0, 0); } + + // ===== id attribute tests ===== + + @Test + void testValidDependencyIdAttribute() throws Exception { + SimpleProblemCollector result = validateFile("dependency-id-valid.xml"); + assertViolations(result, 0, 0, 0); + } + + @Test + void testDependencyIdBadFormat() throws Exception { + SimpleProblemCollector result = validateFile("dependency-id-bad-format.xml"); + assertFalse( + result.getErrors().isEmpty(), + "Expected errors. Fatals: " + result.getFatals() + " Warnings: " + result.getWarnings()); + assertTrue( + result.getErrors().stream().anyMatch(e -> e.contains("groupId:artifactId:version")), + "Expected error about format. Errors: " + result.getErrors()); + } + + @Test + void testDependencyIdConflictWithChildElements() throws Exception { + SimpleProblemCollector result = validateFile("dependency-id-conflict.xml"); + assertTrue( + result.getErrors().stream().anyMatch(e -> e.contains("must not be specified when the 'id' attribute")), + "Expected conflict error. Errors: " + result.getErrors()); + } + + @Test + void testValidExclusionIdAttribute() throws Exception { + SimpleProblemCollector result = validateFile("exclusion-id-valid.xml"); + assertViolations(result, 0, 0, 0); + } + + @Test + void testExclusionIdBadFormat() throws Exception { + SimpleProblemCollector result = validateFile("exclusion-id-bad-format.xml"); + assertTrue( + result.getErrors().stream().anyMatch(e -> e.contains("groupId:artifactId")), + "Expected error about format. Errors: " + result.getErrors()); + } } diff --git a/impl/maven-impl/src/test/resources/poms/validation/dependency-id-bad-format.xml b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-bad-format.xml new file mode 100644 index 000000000000..e1fc8f8a4f35 --- /dev/null +++ b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-bad-format.xml @@ -0,0 +1,10 @@ + + 4.2.0 + aid + gid + 0.1 + + + + + diff --git a/impl/maven-impl/src/test/resources/poms/validation/dependency-id-conflict.xml b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-conflict.xml new file mode 100644 index 000000000000..3b37652a29e1 --- /dev/null +++ b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-conflict.xml @@ -0,0 +1,12 @@ + + 4.2.0 + aid + gid + 0.1 + + + + org.slf4j + + + diff --git a/impl/maven-impl/src/test/resources/poms/validation/dependency-id-valid.xml b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-valid.xml new file mode 100644 index 000000000000..9618e8a7ff41 --- /dev/null +++ b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-valid.xml @@ -0,0 +1,13 @@ + + 4.2.0 + aid + gid + 0.1 + + + + + test + + + diff --git a/impl/maven-impl/src/test/resources/poms/validation/exclusion-id-bad-format.xml b/impl/maven-impl/src/test/resources/poms/validation/exclusion-id-bad-format.xml new file mode 100644 index 000000000000..035b826aa7f4 --- /dev/null +++ b/impl/maven-impl/src/test/resources/poms/validation/exclusion-id-bad-format.xml @@ -0,0 +1,14 @@ + + 4.2.0 + aid + gid + 0.1 + + + + + + + + + diff --git a/impl/maven-impl/src/test/resources/poms/validation/exclusion-id-valid.xml b/impl/maven-impl/src/test/resources/poms/validation/exclusion-id-valid.xml new file mode 100644 index 000000000000..163ccd800900 --- /dev/null +++ b/impl/maven-impl/src/test/resources/poms/validation/exclusion-id-valid.xml @@ -0,0 +1,14 @@ + + 4.2.0 + aid + gid + 0.1 + + + + + + + + + diff --git a/impl/maven-support/src/test/java/org/apache/maven/model/v4/MavenStaxReaderTest.java b/impl/maven-support/src/test/java/org/apache/maven/model/v4/MavenStaxReaderTest.java index 8f5b4c355fb3..4be211767e7d 100644 --- a/impl/maven-support/src/test/java/org/apache/maven/model/v4/MavenStaxReaderTest.java +++ b/impl/maven-support/src/test/java/org/apache/maven/model/v4/MavenStaxReaderTest.java @@ -281,6 +281,74 @@ void testLocationReportingForListElements() throws Exception { assertEquals(5, module3Location.getColumnNumber(), "Module 3 should start at column 5"); } + @Test + void testDependencyIdAttribute() throws Exception { + String xml = "\n" + + " 4.2.0\n" + + " \n" + + " \n" + + " \n" + + ""; + + Model model = fromXml(xml); + assertEquals(1, model.getDependencies().size()); + Dependency dep = model.getDependencies().get(0); + assertEquals("org.slf4j:slf4j-api:2.0.17", dep.getId()); + } + + @Test + void testDependencyIdAttributeWithScope() throws Exception { + String xml = "\n" + + " 4.2.0\n" + + " \n" + + " \n" + + " test\n" + + " \n" + + " \n" + + ""; + + Model model = fromXml(xml); + assertEquals(1, model.getDependencies().size()); + Dependency dep = model.getDependencies().get(0); + assertEquals("org.junit.jupiter:junit-jupiter-api:5.14.1", dep.getId()); + assertEquals("test", dep.getScope()); + } + + @Test + void testExclusionIdAttribute() throws Exception { + String xml = "\n" + + " 4.2.0\n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + ""; + + Model model = fromXml(xml); + assertEquals(1, model.getDependencies().size()); + Dependency dep = model.getDependencies().get(0); + assertEquals("org.postgresql:postgresql:42.7.3", dep.getId()); + assertEquals(1, dep.getExclusions().size()); + assertEquals("*:*", dep.getExclusions().get(0).getId()); + } + + @Test + void testMixinIdAttribute() throws Exception { + String xml = "\n" + + " 4.2.0\n" + + " \n" + + " \n" + + " \n" + + ""; + + Model model = fromXml(xml); + assertEquals(1, model.getMixins().size()); + assertEquals("com.example:java-mixin:1.0.0", model.getMixins().get(0).getGav()); + } + private Model fromXml(String xml) throws XMLStreamException { MavenStaxReader reader = new MavenStaxReader(); return reader.read(new StringReader(xml), true, null); From feeedab4b213a26a8196a6d7cd95f3e789b40129 Mon Sep 17 00:00:00 2001 From: Rob Bygrave Date: Wed, 8 Apr 2026 18:59:25 +1200 Subject: [PATCH 02/11] Add more validation tests for dependency and exclusion id conflicts Add more tests and make them more specific to the validation that is being tested by each test. --- .../impl/model/DefaultModelValidatorTest.java | 66 ++++++++++++++++++- .../dependency-id-conflict-artifactId.xml | 12 ++++ ...xml => dependency-id-conflict-groupId.xml} | 0 .../dependency-id-conflict-multiple.xml | 13 ++++ .../dependency-id-conflict-version.xml | 12 ++++ .../exclusion-id-conflict-artifactId.xml | 16 +++++ .../exclusion-id-conflict-groupId.xml | 16 +++++ .../exclusion-id-conflict-multiple.xml | 17 +++++ 8 files changed, 150 insertions(+), 2 deletions(-) create mode 100644 impl/maven-impl/src/test/resources/poms/validation/dependency-id-conflict-artifactId.xml rename impl/maven-impl/src/test/resources/poms/validation/{dependency-id-conflict.xml => dependency-id-conflict-groupId.xml} (100%) create mode 100644 impl/maven-impl/src/test/resources/poms/validation/dependency-id-conflict-multiple.xml create mode 100644 impl/maven-impl/src/test/resources/poms/validation/dependency-id-conflict-version.xml create mode 100644 impl/maven-impl/src/test/resources/poms/validation/exclusion-id-conflict-artifactId.xml create mode 100644 impl/maven-impl/src/test/resources/poms/validation/exclusion-id-conflict-groupId.xml create mode 100644 impl/maven-impl/src/test/resources/poms/validation/exclusion-id-conflict-multiple.xml diff --git a/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelValidatorTest.java b/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelValidatorTest.java index 466fc0142558..49bcf0237c83 100644 --- a/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelValidatorTest.java +++ b/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelValidatorTest.java @@ -1031,13 +1031,44 @@ void testDependencyIdBadFormat() throws Exception { } @Test - void testDependencyIdConflictWithChildElements() throws Exception { - SimpleProblemCollector result = validateFile("dependency-id-conflict.xml"); + void testDependencyIdConflictWithMultipleChildElements() throws Exception { + SimpleProblemCollector result = validateFile("dependency-id-conflict-multiple.xml"); + assertEquals(2, result.getErrors().size(), "Expected 2 errors in " + result.getErrors()); assertTrue( result.getErrors().stream().anyMatch(e -> e.contains("must not be specified when the 'id' attribute")), "Expected conflict error. Errors: " + result.getErrors()); } + @Test + void testDependencyIdConflictWithGroupIdElements() throws Exception { + SimpleProblemCollector result = validateFile("dependency-id-conflict-groupId.xml"); + assertTrue( + result.getErrors().stream() + .anyMatch(e -> e.contains( + "'dependencies.dependency.groupId' must not be specified when the 'id' attribute")), + "Expected conflict error. Errors: " + result.getErrors()); + } + + @Test + void testDependencyIdConflictWithArtifactIdElement() throws Exception { + SimpleProblemCollector result = validateFile("dependency-id-conflict-artifactId.xml"); + assertTrue( + result.getErrors().stream() + .anyMatch(e -> e.contains( + "'dependencies.dependency.artifactId' must not be specified when the 'id' attribute")), + "Expected conflict error. Errors: " + result.getErrors()); + } + + @Test + void testDependencyIdConflictWithVersionElement() throws Exception { + SimpleProblemCollector result = validateFile("dependency-id-conflict-version.xml"); + assertTrue( + result.getErrors().stream() + .anyMatch(e -> e.contains( + "'dependencies.dependency.version' must not be specified when the 'id' attribute")), + "Expected conflict error. Errors: " + result.getErrors()); + } + @Test void testValidExclusionIdAttribute() throws Exception { SimpleProblemCollector result = validateFile("exclusion-id-valid.xml"); @@ -1051,4 +1082,35 @@ void testExclusionIdBadFormat() throws Exception { result.getErrors().stream().anyMatch(e -> e.contains("groupId:artifactId")), "Expected error about format. Errors: " + result.getErrors()); } + + @Test + void testExclusionIdConflictWithGroupId() throws Exception { + SimpleProblemCollector result = validateFile("exclusion-id-conflict-groupId.xml"); + assertTrue( + result.getErrors().stream() + .anyMatch( + e -> e.contains( + "'dependencies.dependency.exclusions.exclusion.groupId' must not be specified when the 'id' attribute")), + "Expected error about format. Errors: " + result.getErrors()); + } + + @Test + void testExclusionIdConflictWithArtifactId() throws Exception { + SimpleProblemCollector result = validateFile("exclusion-id-conflict-artifactId.xml"); + assertTrue( + result.getErrors().stream() + .anyMatch( + e -> e.contains( + "'dependencies.dependency.exclusions.exclusion.artifactId' must not be specified when the 'id' attribute")), + "Expected error about format. Errors: " + result.getErrors()); + } + + @Test + void testExclusionIdConflictWithMultiple() throws Exception { + SimpleProblemCollector result = validateFile("exclusion-id-conflict-multiple.xml"); + assertEquals(2, result.getErrors().size(), "Expected 2 errors in " + result.getErrors()); + assertTrue( + result.getErrors().stream().anyMatch(e -> e.contains("must not be specified when the 'id' attribute")), + "Expected error about format. Errors: " + result.getErrors()); + } } diff --git a/impl/maven-impl/src/test/resources/poms/validation/dependency-id-conflict-artifactId.xml b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-conflict-artifactId.xml new file mode 100644 index 000000000000..8820417e71f4 --- /dev/null +++ b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-conflict-artifactId.xml @@ -0,0 +1,12 @@ + + 4.2.0 + aid + gid + 0.1 + + + + slf4j-api + + + diff --git a/impl/maven-impl/src/test/resources/poms/validation/dependency-id-conflict.xml b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-conflict-groupId.xml similarity index 100% rename from impl/maven-impl/src/test/resources/poms/validation/dependency-id-conflict.xml rename to impl/maven-impl/src/test/resources/poms/validation/dependency-id-conflict-groupId.xml diff --git a/impl/maven-impl/src/test/resources/poms/validation/dependency-id-conflict-multiple.xml b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-conflict-multiple.xml new file mode 100644 index 000000000000..9729ef96404e --- /dev/null +++ b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-conflict-multiple.xml @@ -0,0 +1,13 @@ + + 4.2.0 + aid + gid + 0.1 + + + + org.slf4j + slf4j-api + + + diff --git a/impl/maven-impl/src/test/resources/poms/validation/dependency-id-conflict-version.xml b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-conflict-version.xml new file mode 100644 index 000000000000..a303683e10b9 --- /dev/null +++ b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-conflict-version.xml @@ -0,0 +1,12 @@ + + 4.2.0 + aid + gid + 0.1 + + + + 2.0.17 + + + diff --git a/impl/maven-impl/src/test/resources/poms/validation/exclusion-id-conflict-artifactId.xml b/impl/maven-impl/src/test/resources/poms/validation/exclusion-id-conflict-artifactId.xml new file mode 100644 index 000000000000..5bfcedaba71d --- /dev/null +++ b/impl/maven-impl/src/test/resources/poms/validation/exclusion-id-conflict-artifactId.xml @@ -0,0 +1,16 @@ + + 4.2.0 + aid + gid + 0.1 + + + + + + * + + + + + diff --git a/impl/maven-impl/src/test/resources/poms/validation/exclusion-id-conflict-groupId.xml b/impl/maven-impl/src/test/resources/poms/validation/exclusion-id-conflict-groupId.xml new file mode 100644 index 000000000000..1d88746adffe --- /dev/null +++ b/impl/maven-impl/src/test/resources/poms/validation/exclusion-id-conflict-groupId.xml @@ -0,0 +1,16 @@ + + 4.2.0 + aid + gid + 0.1 + + + + + + * + + + + + diff --git a/impl/maven-impl/src/test/resources/poms/validation/exclusion-id-conflict-multiple.xml b/impl/maven-impl/src/test/resources/poms/validation/exclusion-id-conflict-multiple.xml new file mode 100644 index 000000000000..6c55c1652606 --- /dev/null +++ b/impl/maven-impl/src/test/resources/poms/validation/exclusion-id-conflict-multiple.xml @@ -0,0 +1,17 @@ + + 4.2.0 + aid + gid + 0.1 + + + + + + * + * + + + + + From 24067aa27e62303ad13f10c3b740f00b818c65aa Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Mon, 22 Jun 2026 20:53:01 +0200 Subject: [PATCH 03/11] Fix review comments: expand id attribute in dependency management and profiles - Expand id attributes on dependencies in dependencyManagement, not just top-level dependencies - Expand id attributes on dependencies within profile sections (both regular dependencies and profile dependencyManagement) - Extract expandAndDeduplicateDependencies() and expandProfileDependencyIds() helper methods - Rename isBlank to isNullOrEmpty for clarity - Add tests for dependency management and profile id expansion Co-Authored-By: Claude Opus 4.6 --- .../impl/model/DefaultModelNormalizer.java | 84 ++++++++++++++----- .../model/DefaultModelNormalizerTest.java | 69 ++++++++++++++- 2 files changed, 133 insertions(+), 20 deletions(-) diff --git a/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelNormalizer.java b/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelNormalizer.java index b903df0b384f..2bd72cd8446c 100644 --- a/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelNormalizer.java +++ b/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelNormalizer.java @@ -29,10 +29,12 @@ import org.apache.maven.api.di.Singleton; import org.apache.maven.api.model.Build; import org.apache.maven.api.model.Dependency; +import org.apache.maven.api.model.DependencyManagement; import org.apache.maven.api.model.Exclusion; import org.apache.maven.api.model.Mixin; import org.apache.maven.api.model.Model; import org.apache.maven.api.model.Plugin; +import org.apache.maven.api.model.Profile; import org.apache.maven.api.services.ModelBuilderRequest; import org.apache.maven.api.services.ModelProblemCollector; import org.apache.maven.api.services.model.ModelNormalizer; @@ -82,19 +84,23 @@ public Model mergeDuplicates(Model model, ModelBuilderRequest request, ModelProb * aftereffects and bogus error messages. */ // Expand id attributes on dependencies (and their exclusions), then deduplicate - List dependencies = model.getDependencies(); - List expanded = injectList(dependencies, this::expandDependencyId); - if (expanded != null) { - dependencies = expanded; - } + builder.dependencies(expandAndDeduplicateDependencies(model.getDependencies())); - Map normalizedDeps = new LinkedHashMap<>(dependencies.size() * 2); - for (Dependency dependency : dependencies) { - normalizedDeps.put(dependency.getManagementKey(), dependency); + // Expand id attributes on dependency management dependencies + DependencyManagement mgmt = model.getDependencyManagement(); + if (mgmt != null) { + List expandedMgmt = expandAndDeduplicateDependencies(mgmt.getDependencies()); + if (expandedMgmt != null) { + builder.dependencyManagement(DependencyManagement.newBuilder(mgmt) + .dependencies(expandedMgmt) + .build()); + } } - if (expanded != null || dependencies.size() != normalizedDeps.size()) { - builder.dependencies(normalizedDeps.values()); + // Expand id attributes in profile dependencies and dependency management + List profiles = injectList(model.getProfiles(), this::expandProfileDependencyIds); + if (profiles != null) { + builder.profiles(profiles); } return builder.build(); @@ -155,6 +161,46 @@ private List injectList(List list, UnaryOperator modifer) { return newList; } + private List expandAndDeduplicateDependencies(List dependencies) { + List expanded = injectList(dependencies, this::expandDependencyId); + if (expanded != null) { + dependencies = expanded; + } + Map normalized = new LinkedHashMap<>(dependencies.size() * 2); + for (Dependency dependency : dependencies) { + normalized.put(dependency.getManagementKey(), dependency); + } + if (expanded != null || dependencies.size() != normalized.size()) { + return new ArrayList<>(normalized.values()); + } + return null; + } + + private Profile expandProfileDependencyIds(Profile profile) { + Profile.Builder pb = null; + + List deps = expandAndDeduplicateDependencies(profile.getDependencies()); + if (deps != null) { + pb = Profile.newBuilder(profile); + pb.dependencies(deps); + } + + DependencyManagement mgmt = profile.getDependencyManagement(); + if (mgmt != null) { + List mgmtDeps = expandAndDeduplicateDependencies(mgmt.getDependencies()); + if (mgmtDeps != null) { + if (pb == null) { + pb = Profile.newBuilder(profile); + } + pb.dependencyManagement(DependencyManagement.newBuilder(mgmt) + .dependencies(mgmtDeps) + .build()); + } + } + + return pb != null ? pb.build() : profile; + } + /** * Expands the {@code id} attribute on a dependency into its component fields. * The id format is {@code groupId:artifactId:version}. @@ -172,13 +218,13 @@ Dependency expandDependencyId(Dependency d) { return d; } Dependency.Builder builder = Dependency.newBuilder(d); - if (isBlank(d.getGroupId())) { + if (isNullOrEmpty(d.getGroupId())) { builder.groupId(parts[0]); } - if (isBlank(d.getArtifactId())) { + if (isNullOrEmpty(d.getArtifactId())) { builder.artifactId(parts[1]); } - if (isBlank(d.getVersion())) { + if (isNullOrEmpty(d.getVersion())) { builder.version(parts[2]); } List expanded = injectList(d.getExclusions(), this::expandExclusionId); @@ -203,10 +249,10 @@ Exclusion expandExclusionId(Exclusion e) { return e; } Exclusion.Builder builder = Exclusion.newBuilder(e); - if (isBlank(e.getGroupId())) { + if (isNullOrEmpty(e.getGroupId())) { builder.groupId(parts[0]); } - if (isBlank(e.getArtifactId())) { + if (isNullOrEmpty(e.getArtifactId())) { builder.artifactId(parts[1]); } return builder.build(); @@ -227,19 +273,19 @@ Mixin expandMixinGav(Mixin m) { return m; } Mixin.Builder builder = Mixin.newBuilder(m); - if (isBlank(m.getGroupId())) { + if (isNullOrEmpty(m.getGroupId())) { builder.groupId(parts[0]); } - if (isBlank(m.getArtifactId())) { + if (isNullOrEmpty(m.getArtifactId())) { builder.artifactId(parts[1]); } - if (isBlank(m.getVersion())) { + if (isNullOrEmpty(m.getVersion())) { builder.version(parts[2]); } return builder.build(); } - private static boolean isBlank(String s) { + private static boolean isNullOrEmpty(String s) { return s == null || s.isEmpty(); } } diff --git a/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelNormalizerTest.java b/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelNormalizerTest.java index 247017ddba09..e64c11bfdaf4 100644 --- a/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelNormalizerTest.java +++ b/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelNormalizerTest.java @@ -18,9 +18,14 @@ */ package org.apache.maven.impl.model; +import java.util.List; + import org.apache.maven.api.model.Dependency; +import org.apache.maven.api.model.DependencyManagement; import org.apache.maven.api.model.Exclusion; import org.apache.maven.api.model.Mixin; +import org.apache.maven.api.model.Model; +import org.apache.maven.api.model.Profile; import org.junit.jupiter.api.Test; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -150,7 +155,7 @@ void testExpandDependencyIdAlsoExpandsExclusions() { Exclusion exc = Exclusion.newBuilder().id("com.example:unwanted").build(); Dependency dep = Dependency.newBuilder() .id("org.slf4j:slf4j-api:2.0.17") - .exclusions(java.util.List.of(exc)) + .exclusions(List.of(exc)) .build(); Dependency result = normalizer.expandDependencyId(dep); @@ -162,4 +167,66 @@ void testExpandDependencyIdAlsoExpandsExclusions() { assertEquals("com.example", result.getExclusions().get(0).getGroupId()); assertEquals("unwanted", result.getExclusions().get(0).getArtifactId()); } + + @Test + void testMergeDuplicatesExpandsDependencyManagement() { + Dependency dep = + Dependency.newBuilder().id("org.slf4j:slf4j-api:2.0.17").build(); + Model model = Model.newBuilder() + .dependencyManagement(DependencyManagement.newBuilder() + .dependencies(List.of(dep)) + .build()) + .build(); + + Model result = normalizer.mergeDuplicates(model, null, null); + + assertEquals(1, result.getDependencyManagement().getDependencies().size()); + Dependency expanded = result.getDependencyManagement().getDependencies().get(0); + assertEquals("org.slf4j", expanded.getGroupId()); + assertEquals("slf4j-api", expanded.getArtifactId()); + assertEquals("2.0.17", expanded.getVersion()); + } + + @Test + void testMergeDuplicatesExpandsProfileDependencies() { + Dependency dep = + Dependency.newBuilder().id("org.slf4j:slf4j-api:2.0.17").build(); + Profile profile = Profile.newBuilder() + .id("test-profile") + .dependencies(List.of(dep)) + .build(); + Model model = Model.newBuilder().profiles(List.of(profile)).build(); + + Model result = normalizer.mergeDuplicates(model, null, null); + + assertEquals(1, result.getProfiles().size()); + assertEquals(1, result.getProfiles().get(0).getDependencies().size()); + Dependency expanded = result.getProfiles().get(0).getDependencies().get(0); + assertEquals("org.slf4j", expanded.getGroupId()); + assertEquals("slf4j-api", expanded.getArtifactId()); + assertEquals("2.0.17", expanded.getVersion()); + } + + @Test + void testMergeDuplicatesExpandsProfileDependencyManagement() { + Dependency dep = + Dependency.newBuilder().id("org.slf4j:slf4j-api:2.0.17").build(); + Profile profile = Profile.newBuilder() + .id("test-profile") + .dependencyManagement(DependencyManagement.newBuilder() + .dependencies(List.of(dep)) + .build()) + .build(); + Model model = Model.newBuilder().profiles(List.of(profile)).build(); + + Model result = normalizer.mergeDuplicates(model, null, null); + + assertEquals(1, result.getProfiles().size()); + DependencyManagement mgmt = result.getProfiles().get(0).getDependencyManagement(); + assertEquals(1, mgmt.getDependencies().size()); + Dependency expanded = mgmt.getDependencies().get(0); + assertEquals("org.slf4j", expanded.getGroupId()); + assertEquals("slf4j-api", expanded.getArtifactId()); + assertEquals("2.0.17", expanded.getVersion()); + } } From 5a8fcdb9df105b262b6dec971001c9e6781cd306 Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Tue, 23 Jun 2026 19:45:51 +0200 Subject: [PATCH 04/11] [GH-11904] Add dependency id attribute support for Maven 4.2.0 Add compact `id` XML attribute to Dependency, Exclusion, and Mixin elements in the Maven model (4.2.0+). Supported formats: - Dependency: g:a:v, g:a:type:v, g:a:type:classifier:v - Exclusion: g:a - Mixin (as `gav`/`id`): g:a:v The normalizer expands `id` into individual fields and clears it so consumer POMs never contain the attribute. Uses forceCopy=true in builders to ensure `id(null)` actually clears the field. Includes comprehensive unit tests (normalizer, validator), consumer POM transformation tests, and integration test verifying no id attributes leak into deployed POMs. Co-Authored-By: Claude Opus 4.6 --- api/maven-api-model/src/main/mdo/maven.mdo | 44 +++ .../ConsumerPomArtifactTransformerTest.java | 39 +++ .../projects/transform/jar-with-id/after.pom | 39 +++ .../projects/transform/jar-with-id/before.pom | 20 ++ .../impl/model/DefaultModelNormalizer.java | 180 +++++++++++- .../impl/model/DefaultModelObjectPool.java | 4 +- .../impl/model/DefaultModelValidator.java | 120 ++++++++ .../model/DefaultModelNormalizerTest.java | 268 ++++++++++++++++++ .../impl/model/DefaultModelValidatorTest.java | 51 ++++ .../validation/dependency-id-3parts-valid.xml | 11 + .../validation/dependency-id-4parts-valid.xml | 11 + .../validation/dependency-id-5parts-valid.xml | 11 + .../validation/dependency-id-bad-format.xml | 11 + .../dependency-id-conflict-groupId.xml | 13 + .../dependency-id-conflict-type.xml | 13 + .../validation/exclusion-id-bad-format.xml | 15 + .../poms/validation/exclusion-id-valid.xml | 15 + ...venITgh11904DependencyIdAttributeTest.java | 91 ++++++ .../.mvn/.gitkeep | 0 .../child/pom.xml | 20 ++ .../child/src/main/java/Test.java | 1 + .../lib-a/pom.xml | 15 + .../gh-11904-dependency-id-attribute/pom.xml | 21 ++ 23 files changed, 1006 insertions(+), 7 deletions(-) create mode 100644 impl/maven-core/src/test/resources/projects/transform/jar-with-id/after.pom create mode 100644 impl/maven-core/src/test/resources/projects/transform/jar-with-id/before.pom create mode 100644 impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelNormalizerTest.java create mode 100644 impl/maven-impl/src/test/resources/poms/validation/dependency-id-3parts-valid.xml create mode 100644 impl/maven-impl/src/test/resources/poms/validation/dependency-id-4parts-valid.xml create mode 100644 impl/maven-impl/src/test/resources/poms/validation/dependency-id-5parts-valid.xml create mode 100644 impl/maven-impl/src/test/resources/poms/validation/dependency-id-bad-format.xml create mode 100644 impl/maven-impl/src/test/resources/poms/validation/dependency-id-conflict-groupId.xml create mode 100644 impl/maven-impl/src/test/resources/poms/validation/dependency-id-conflict-type.xml create mode 100644 impl/maven-impl/src/test/resources/poms/validation/exclusion-id-bad-format.xml create mode 100644 impl/maven-impl/src/test/resources/poms/validation/exclusion-id-valid.xml create mode 100644 its/core-it-suite/src/test/java/org/apache/maven/it/MavenITgh11904DependencyIdAttributeTest.java create mode 100644 its/core-it-suite/src/test/resources/gh-11904-dependency-id-attribute/.mvn/.gitkeep create mode 100644 its/core-it-suite/src/test/resources/gh-11904-dependency-id-attribute/child/pom.xml create mode 100644 its/core-it-suite/src/test/resources/gh-11904-dependency-id-attribute/child/src/main/java/Test.java create mode 100644 its/core-it-suite/src/test/resources/gh-11904-dependency-id-attribute/lib-a/pom.xml create mode 100644 its/core-it-suite/src/test/resources/gh-11904-dependency-id-attribute/pom.xml diff --git a/api/maven-api-model/src/main/mdo/maven.mdo b/api/maven-api-model/src/main/mdo/maven.mdo index eb5618d9af02..00681ebe4ffa 100644 --- a/api/maven-api-model/src/main/mdo/maven.mdo +++ b/api/maven-api-model/src/main/mdo/maven.mdo @@ -1205,6 +1205,22 @@ ]]> + + id + 4.2.0+ + + + + String + groupId 3.0.0+ @@ -1441,6 +1457,20 @@ ]]> + + id + 4.2.0+ + + + + String + groupId 4.0.0+ @@ -1864,6 +1894,20 @@ 4.1.0+ Parent + + gav + 4.2.0+ + + + + String + classifier 4.1.0+ diff --git a/impl/maven-core/src/test/java/org/apache/maven/internal/transformation/impl/ConsumerPomArtifactTransformerTest.java b/impl/maven-core/src/test/java/org/apache/maven/internal/transformation/impl/ConsumerPomArtifactTransformerTest.java index 81870abd0f3b..b5610e314830 100644 --- a/impl/maven-core/src/test/java/org/apache/maven/internal/transformation/impl/ConsumerPomArtifactTransformerTest.java +++ b/impl/maven-core/src/test/java/org/apache/maven/internal/transformation/impl/ConsumerPomArtifactTransformerTest.java @@ -29,6 +29,7 @@ import org.apache.maven.api.Constants; import org.apache.maven.api.services.Sources; +import org.apache.maven.impl.model.DefaultModelNormalizer; import org.apache.maven.model.Model; import org.apache.maven.model.v4.MavenStaxReader; import org.apache.maven.project.MavenProject; @@ -114,6 +115,44 @@ void transformJarConsumerPom() throws Exception { assertFalse(diff.hasDifferences(), "XML files should be identical: " + diff.toString()); } + @Test + void transformJarConsumerPomWithDependencyId() throws Exception { + RepositorySystemSession systemSessionMock = Mockito.mock(RepositorySystemSession.class); + SessionData sessionDataMock = Mockito.mock(SessionData.class); + when(systemSessionMock.getData()).thenReturn(sessionDataMock); + + Path beforePomFile = Paths.get("src/test/resources/projects/transform/jar-with-id/before.pom") + .toAbsolutePath(); + Path afterPomFile = Paths.get("src/test/resources/projects/transform/jar-with-id/after.pom") + .toAbsolutePath(); + Path tempFile = Files.createTempFile("", ".pom"); + Files.delete(tempFile); + DefaultModelNormalizer normalizer = new DefaultModelNormalizer(); + try (InputStream expected = Files.newInputStream(beforePomFile)) { + org.apache.maven.api.model.Model rawModel = new MavenStaxReader().read(expected); + // Normalize: expand id attributes and clear them + rawModel = normalizer.mergeDuplicates(rawModel, null, null); + Model model = new Model(rawModel); + MavenProject project = new MavenProject(model); + project.setOriginalModel(model); + ConsumerPomArtifactTransformer t = new ConsumerPomArtifactTransformer((s, p, f) -> { + try (InputStream is = f.openStream()) { + org.apache.maven.api.model.Model m = new MavenStaxReader().read(is); + m = normalizer.mergeDuplicates(m, null, null); + return DefaultConsumerPomBuilder.transformNonPom(m, project); + } + }); + + t.transform(project, systemSessionMock, Sources.buildSource(beforePomFile), tempFile); + } + Diff diff = DiffBuilder.compare(afterPomFile.toFile()) + .withTest(tempFile.toFile()) + .ignoreComments() + .ignoreWhitespace() + .build(); + assertFalse(diff.hasDifferences(), "XML files should be identical: " + diff.toString()); + } + @Test void injectTransformedArtifactsWithoutPomShouldNotInjectAnyArtifacts() throws IOException { MavenProject emptyProject = new MavenProject(); diff --git a/impl/maven-core/src/test/resources/projects/transform/jar-with-id/after.pom b/impl/maven-core/src/test/resources/projects/transform/jar-with-id/after.pom new file mode 100644 index 000000000000..6c3ef9035a3e --- /dev/null +++ b/impl/maven-core/src/test/resources/projects/transform/jar-with-id/after.pom @@ -0,0 +1,39 @@ + + + 4.0.0 + io.github.test + jar-with-id + 1.0.0 + jar-with-id + + + org.slf4j + slf4j-api + 2.0.17 + + + org.example + lib-b + 1.0 + pom + import + + + org.example + lib-c + 1.0 + sources + + + org.example + lib-d + 1.0 + + + org.unwanted + bad-lib + + + + + diff --git a/impl/maven-core/src/test/resources/projects/transform/jar-with-id/before.pom b/impl/maven-core/src/test/resources/projects/transform/jar-with-id/before.pom new file mode 100644 index 000000000000..26ba0dfaedd5 --- /dev/null +++ b/impl/maven-core/src/test/resources/projects/transform/jar-with-id/before.pom @@ -0,0 +1,20 @@ + + + 4.2.0 + io.github.test + jar-with-id + 1.0.0 + jar-with-id + + + + import + + + + + + + + + diff --git a/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelNormalizer.java b/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelNormalizer.java index f36918f770aa..a36992f33fa4 100644 --- a/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelNormalizer.java +++ b/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelNormalizer.java @@ -29,8 +29,12 @@ import org.apache.maven.api.di.Singleton; import org.apache.maven.api.model.Build; import org.apache.maven.api.model.Dependency; +import org.apache.maven.api.model.DependencyManagement; +import org.apache.maven.api.model.Exclusion; +import org.apache.maven.api.model.Mixin; import org.apache.maven.api.model.Model; import org.apache.maven.api.model.Plugin; +import org.apache.maven.api.model.Profile; import org.apache.maven.api.services.ModelBuilderRequest; import org.apache.maven.api.services.ModelProblemCollector; import org.apache.maven.api.services.model.ModelNormalizer; @@ -49,6 +53,8 @@ public class DefaultModelNormalizer implements ModelNormalizer { public Model mergeDuplicates(Model model, ModelBuilderRequest request, ModelProblemCollector problems) { Model.Builder builder = Model.newBuilder(model); + builder.mixins(injectList(model.getMixins(), this::expandMixinGav)); + Build build = model.getBuild(); if (build != null) { List plugins = build.getPlugins(); @@ -76,15 +82,21 @@ 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. */ - List dependencies = model.getDependencies(); - Map normalized = new LinkedHashMap<>(dependencies.size() * 2); + builder.dependencies(expandAndDeduplicateDependencies(model.getDependencies())); - for (Dependency dependency : dependencies) { - normalized.put(dependency.getManagementKey(), dependency); + DependencyManagement mgmt = model.getDependencyManagement(); + if (mgmt != null) { + List expandedMgmt = expandAndDeduplicateDependencies(mgmt.getDependencies()); + if (expandedMgmt != null) { + builder.dependencyManagement(DependencyManagement.newBuilder(mgmt) + .dependencies(expandedMgmt) + .build()); + } } - if (dependencies.size() != normalized.size()) { - builder.dependencies(normalized.values()); + List profiles = injectList(model.getProfiles(), this::expandProfileDependencyIds); + if (profiles != null) { + builder.profiles(profiles); } return builder.build(); @@ -144,4 +156,160 @@ private List injectList(List list, UnaryOperator modifer) { } return newList; } + + private List expandAndDeduplicateDependencies(List dependencies) { + List expanded = injectList(dependencies, this::expandDependencyId); + if (expanded != null) { + dependencies = expanded; + } + Map normalized = new LinkedHashMap<>(dependencies.size() * 2); + for (Dependency dependency : dependencies) { + normalized.put(dependency.getManagementKey(), dependency); + } + if (expanded != null || dependencies.size() != normalized.size()) { + return new ArrayList<>(normalized.values()); + } + return null; + } + + private Profile expandProfileDependencyIds(Profile profile) { + Profile.Builder pb = null; + + List deps = expandAndDeduplicateDependencies(profile.getDependencies()); + if (deps != null) { + pb = Profile.newBuilder(profile); + pb.dependencies(deps); + } + + DependencyManagement mgmt = profile.getDependencyManagement(); + if (mgmt != null) { + List mgmtDeps = expandAndDeduplicateDependencies(mgmt.getDependencies()); + if (mgmtDeps != null) { + if (pb == null) { + pb = Profile.newBuilder(profile); + } + pb.dependencyManagement(DependencyManagement.newBuilder(mgmt) + .dependencies(mgmtDeps) + .build()); + } + } + + return pb != null ? pb.build() : profile; + } + + /** + * Expands the {@code id} attribute on a dependency into its component fields. + * Supported formats: + *
    + *
  • {@code groupId:artifactId:version} (3 parts)
    • + *
  • {@code groupId:artifactId:type:version} (4 parts)
    • + *
  • {@code groupId:artifactId:type:classifier:version} (5 parts)
    • + *
+ */ + Dependency expandDependencyId(Dependency d) { + String id = d.getId(); + if (id == null || id.isEmpty()) { + List expanded = injectList(d.getExclusions(), this::expandExclusionId); + return expanded != null ? d.withExclusions(expanded) : d; + } + String[] parts = id.split(":"); + if (parts.length < 3 || parts.length > 5) { + return d; + } + Dependency.Builder builder = Dependency.newBuilder(d, true); + builder.id(null); + if (isNullOrEmpty(d.getGroupId())) { + builder.groupId(parts[0]); + } + if (isNullOrEmpty(d.getArtifactId())) { + builder.artifactId(parts[1]); + } + switch (parts.length) { + case 3: + // g:a:v + if (isNullOrEmpty(d.getVersion())) { + builder.version(parts[2]); + } + break; + case 4: + // g:a:type:v + if (isNullOrEmptyOrDefault(d.getType())) { + builder.type(parts[2]); + } + if (isNullOrEmpty(d.getVersion())) { + builder.version(parts[3]); + } + break; + case 5: + // g:a:type:classifier:v + if (isNullOrEmptyOrDefault(d.getType())) { + builder.type(parts[2]); + } + if (isNullOrEmpty(d.getClassifier())) { + builder.classifier(parts[3]); + } + if (isNullOrEmpty(d.getVersion())) { + builder.version(parts[4]); + } + break; + default: + break; + } + List expanded = injectList(d.getExclusions(), this::expandExclusionId); + if (expanded != null) { + builder.exclusions(expanded); + } + return builder.build(); + } + + Exclusion expandExclusionId(Exclusion e) { + String id = e.getId(); + if (id == null || id.isEmpty()) { + return e; + } + String[] parts = id.split(":"); + if (parts.length != 2) { + return e; + } + Exclusion.Builder builder = Exclusion.newBuilder(e, true); + builder.id(null); + if (isNullOrEmpty(e.getGroupId())) { + builder.groupId(parts[0]); + } + if (isNullOrEmpty(e.getArtifactId())) { + builder.artifactId(parts[1]); + } + return builder.build(); + } + + Mixin expandMixinGav(Mixin m) { + String gav = m.getGav(); + if (gav == null || gav.isEmpty()) { + return m; + } + String[] parts = gav.split(":"); + if (parts.length != 3) { + return m; + } + Mixin.Builder builder = Mixin.newBuilder(m, true); + builder.gav(null); + if (isNullOrEmpty(m.getGroupId())) { + builder.groupId(parts[0]); + } + if (isNullOrEmpty(m.getArtifactId())) { + builder.artifactId(parts[1]); + } + if (isNullOrEmpty(m.getVersion())) { + builder.version(parts[2]); + } + return builder.build(); + } + + private static boolean isNullOrEmpty(String s) { + return s == null || s.isEmpty(); + } + + private static boolean isNullOrEmptyOrDefault(String type) { + return type == null || type.isEmpty() || "jar".equals(type); + } } diff --git a/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelObjectPool.java b/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelObjectPool.java index bec34fe0fa76..03791aaee4c1 100644 --- a/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelObjectPool.java +++ b/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelObjectPool.java @@ -237,7 +237,8 @@ private static boolean objectsEqual(Object obj1, Object obj2) { */ private static boolean dependenciesEqual( org.apache.maven.api.model.Dependency dep1, org.apache.maven.api.model.Dependency dep2) { - return Objects.equals(dep1.getGroupId(), dep2.getGroupId()) + return Objects.equals(dep1.getId(), dep2.getId()) + && Objects.equals(dep1.getGroupId(), dep2.getGroupId()) && Objects.equals(dep1.getArtifactId(), dep2.getArtifactId()) && Objects.equals(dep1.getVersion(), dep2.getVersion()) && Objects.equals(dep1.getType(), dep2.getType()) @@ -286,6 +287,7 @@ private static int computeHashCode(Object obj) { */ private static int dependencyHashCode(org.apache.maven.api.model.Dependency dep) { return Objects.hash( + dep.getId(), dep.getGroupId(), dep.getArtifactId(), dep.getVersion(), diff --git a/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java b/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java index 8cc8cb3459b3..72e65e1864ff 100644 --- a/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java +++ b/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java @@ -1143,6 +1143,11 @@ private void validate20RawDependencies( for (Dependency dependency : dependencies) { String key = dependency.getManagementKey(); + validateDependencyIdAttribute(problems, dependency, prefix + prefix2); + for (Exclusion exclusion : dependency.getExclusions()) { + validateExclusionIdAttribute(problems, exclusion, prefix + prefix2); + } + if ("import".equals(dependency.getScope())) { if (!"pom".equals(dependency.getType())) { addViolation( @@ -1258,6 +1263,116 @@ private void validate20RawDependencies( } } + private void validateDependencyIdAttribute(ModelProblemCollector problems, Dependency dependency, String prefix) { + String id = dependency.getId(); + if (id == null || id.isEmpty()) { + return; + } + int colonCount = id.length() - id.replace(":", "").length(); + if (colonCount < 2 || colonCount > 4) { + addViolation( + problems, + Severity.ERROR, + Version.V42, + prefix + "id", + null, + "has invalid format '" + id + "', must be 'groupId:artifactId:version', " + + "'groupId:artifactId:type:version', or " + + "'groupId:artifactId:type:classifier:version'.", + dependency); + return; + } + if (!isNullOrEmpty(dependency.getGroupId())) { + addViolation( + problems, + Severity.ERROR, + Version.V42, + prefix + "groupId", + null, + "must not be specified when 'id' attribute is used.", + dependency); + } + if (!isNullOrEmpty(dependency.getArtifactId())) { + addViolation( + problems, + Severity.ERROR, + Version.V42, + prefix + "artifactId", + null, + "must not be specified when 'id' attribute is used.", + dependency); + } + if (!isNullOrEmpty(dependency.getVersion())) { + addViolation( + problems, + Severity.ERROR, + Version.V42, + prefix + "version", + null, + "must not be specified when 'id' attribute is used.", + dependency); + } + if (colonCount >= 3 && !isNullOrEmpty(dependency.getType()) && !"jar".equals(dependency.getType())) { + addViolation( + problems, + Severity.ERROR, + Version.V42, + prefix + "type", + null, + "must not be specified when 'id' attribute includes type.", + dependency); + } + if (colonCount >= 4 && !isNullOrEmpty(dependency.getClassifier())) { + addViolation( + problems, + Severity.ERROR, + Version.V42, + prefix + "classifier", + null, + "must not be specified when 'id' attribute includes classifier.", + dependency); + } + } + + private void validateExclusionIdAttribute(ModelProblemCollector problems, Exclusion exclusion, String prefix) { + String id = exclusion.getId(); + if (id == null || id.isEmpty()) { + return; + } + int colonCount = id.length() - id.replace(":", "").length(); + if (colonCount != 1) { + addViolation( + problems, + Severity.ERROR, + Version.V42, + prefix + "exclusions.exclusion.id", + null, + "has invalid format '" + id + "', must be 'groupId:artifactId'.", + exclusion); + return; + } + if (!isNullOrEmpty(exclusion.getGroupId())) { + addViolation( + problems, + Severity.ERROR, + Version.V42, + prefix + "exclusions.exclusion.groupId", + null, + "must not be specified when 'id' attribute is used.", + exclusion); + } + if (!isNullOrEmpty(exclusion.getArtifactId())) { + addViolation( + problems, + Severity.ERROR, + Version.V42, + prefix + "exclusions.exclusion.artifactId", + null, + "must not be specified when 'id' attribute is used.", + exclusion); + } + } + private void validate20RawDependenciesSelfReferencing( ModelProblemCollector problems, Model model, List dependencies, String prefix) { // We only check for groupId/artifactId/version/classifier cause if there is another @@ -1513,6 +1628,7 @@ private void validateEffectiveDependency( if (validationLevel >= ModelValidator.VALIDATION_LEVEL_MAVEN_2_0) { for (Exclusion exclusion : dependency.getExclusions()) { + validateExclusionIdAttribute(problems, exclusion, prefix); if (validationLevel < ModelValidator.VALIDATION_LEVEL_MAVEN_3_0) { validateCoordinatesId( prefix, @@ -2509,4 +2625,8 @@ static SourceHint resourceDirectory(Resource resource) { return resource::getDirectory; } } + + private static boolean isNullOrEmpty(String s) { + return s == null || s.isEmpty(); + } } diff --git a/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelNormalizerTest.java b/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelNormalizerTest.java new file mode 100644 index 000000000000..e4a84dbbb334 --- /dev/null +++ b/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelNormalizerTest.java @@ -0,0 +1,268 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.impl.model; + +import java.util.List; + +import org.apache.maven.api.model.Dependency; +import org.apache.maven.api.model.DependencyManagement; +import org.apache.maven.api.model.Exclusion; +import org.apache.maven.api.model.Mixin; +import org.apache.maven.api.model.Model; +import org.apache.maven.api.model.Profile; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; + +class DefaultModelNormalizerTest { + + private final DefaultModelNormalizer normalizer = new DefaultModelNormalizer(); + + @Test + void testExpandDependencyId3parts() { + Dependency dep = + Dependency.newBuilder(false).id("org.slf4j:slf4j-api:2.0.17").build(); + + Dependency result = normalizer.expandDependencyId(dep); + + assertEquals("org.slf4j", result.getGroupId()); + assertEquals("slf4j-api", result.getArtifactId()); + assertEquals("2.0.17", result.getVersion()); + assertNull(result.getId()); + } + + @Test + void testExpandDependencyId4parts() { + Dependency dep = + Dependency.newBuilder(false).id("org.example:lib-b:pom:1.0").build(); + + Dependency result = normalizer.expandDependencyId(dep); + + assertEquals("org.example", result.getGroupId()); + assertEquals("lib-b", result.getArtifactId()); + assertEquals("pom", result.getType()); + assertEquals("1.0", result.getVersion()); + assertNull(result.getId()); + } + + @Test + void testExpandDependencyId5parts() { + Dependency dep = Dependency.newBuilder(false) + .id("org.example:lib-c:jar:sources:1.0") + .build(); + + Dependency result = normalizer.expandDependencyId(dep); + + assertEquals("org.example", result.getGroupId()); + assertEquals("lib-c", result.getArtifactId()); + assertEquals("jar", result.getType()); + assertEquals("sources", result.getClassifier()); + assertEquals("1.0", result.getVersion()); + assertNull(result.getId()); + } + + @Test + void testExpandDependencyIdDoesNotOverrideExistingFields() { + Dependency dep = Dependency.newBuilder(false) + .id("org.slf4j:slf4j-api:2.0.17") + .groupId("org.override") + .build(); + + Dependency result = normalizer.expandDependencyId(dep); + + assertEquals("org.override", result.getGroupId()); + assertEquals("slf4j-api", result.getArtifactId()); + assertEquals("2.0.17", result.getVersion()); + assertNull(result.getId()); + } + + @Test + void testExpandDependencyIdInvalidFormat() { + Dependency dep = Dependency.newBuilder(false).id("invalid-no-colons").build(); + + Dependency result = normalizer.expandDependencyId(dep); + + assertNull(result.getGroupId()); + assertNull(result.getArtifactId()); + } + + @Test + void testExpandDependencyIdNull() { + Dependency dep = Dependency.newBuilder() + .groupId("org.example") + .artifactId("my-lib") + .version("1.0") + .build(); + + Dependency result = normalizer.expandDependencyId(dep); + + assertEquals("org.example", result.getGroupId()); + assertEquals("my-lib", result.getArtifactId()); + assertEquals("1.0", result.getVersion()); + } + + @Test + void testExpandDependencyIdAlsoExpandsExclusions() { + Exclusion exc = Exclusion.newBuilder(false).id("com.example:unwanted").build(); + Dependency dep = Dependency.newBuilder(false) + .id("org.slf4j:slf4j-api:2.0.17") + .exclusions(List.of(exc)) + .build(); + + Dependency result = normalizer.expandDependencyId(dep); + + assertEquals("org.slf4j", result.getGroupId()); + assertEquals("slf4j-api", result.getArtifactId()); + assertEquals("2.0.17", result.getVersion()); + assertNull(result.getId()); + assertEquals(1, result.getExclusions().size()); + assertEquals("com.example", result.getExclusions().get(0).getGroupId()); + assertEquals("unwanted", result.getExclusions().get(0).getArtifactId()); + assertNull(result.getExclusions().get(0).getId()); + } + + @Test + void testExpandExclusionId() { + Exclusion exc = + Exclusion.newBuilder(false).id("com.example:unwanted-lib").build(); + + Exclusion result = normalizer.expandExclusionId(exc); + + assertEquals("com.example", result.getGroupId()); + assertEquals("unwanted-lib", result.getArtifactId()); + assertNull(result.getId()); + } + + @Test + void testExpandExclusionIdWildcard() { + Exclusion exc = Exclusion.newBuilder(false).id("*:*").build(); + + Exclusion result = normalizer.expandExclusionId(exc); + + assertEquals("*", result.getGroupId()); + assertEquals("*", result.getArtifactId()); + assertNull(result.getId()); + } + + @Test + void testExpandExclusionIdNull() { + Exclusion exc = + Exclusion.newBuilder().groupId("org.example").artifactId("lib").build(); + + Exclusion result = normalizer.expandExclusionId(exc); + + assertEquals("org.example", result.getGroupId()); + assertEquals("lib", result.getArtifactId()); + } + + @Test + void testExpandMixinGav() { + Mixin mixin = Mixin.newBuilder(false) + .gav("com.example.mixins:java-mixin:1.0.0") + .build(); + + Mixin result = normalizer.expandMixinGav(mixin); + + assertEquals("com.example.mixins", result.getGroupId()); + assertEquals("java-mixin", result.getArtifactId()); + assertEquals("1.0.0", result.getVersion()); + assertNull(result.getGav()); + } + + @Test + void testExpandMixinGavNull() { + Mixin mixin = Mixin.newBuilder() + .groupId("com.example") + .artifactId("my-mixin") + .version("2.0") + .build(); + + Mixin result = normalizer.expandMixinGav(mixin); + + assertEquals("com.example", result.getGroupId()); + assertEquals("my-mixin", result.getArtifactId()); + assertEquals("2.0", result.getVersion()); + } + + @Test + void testMergeDuplicatesExpandsDependencyManagement() { + Dependency dep = + Dependency.newBuilder(false).id("org.slf4j:slf4j-api:2.0.17").build(); + Model model = Model.newBuilder() + .dependencyManagement(DependencyManagement.newBuilder() + .dependencies(List.of(dep)) + .build()) + .build(); + + Model result = normalizer.mergeDuplicates(model, null, null); + + assertEquals(1, result.getDependencyManagement().getDependencies().size()); + Dependency expanded = result.getDependencyManagement().getDependencies().get(0); + assertEquals("org.slf4j", expanded.getGroupId()); + assertEquals("slf4j-api", expanded.getArtifactId()); + assertEquals("2.0.17", expanded.getVersion()); + assertNull(expanded.getId()); + } + + @Test + void testMergeDuplicatesExpandsProfileDependencies() { + Dependency dep = + Dependency.newBuilder(false).id("org.slf4j:slf4j-api:2.0.17").build(); + Profile profile = Profile.newBuilder() + .id("test-profile") + .dependencies(List.of(dep)) + .build(); + Model model = Model.newBuilder().profiles(List.of(profile)).build(); + + Model result = normalizer.mergeDuplicates(model, null, null); + + assertEquals(1, result.getProfiles().size()); + assertEquals(1, result.getProfiles().get(0).getDependencies().size()); + Dependency expanded = result.getProfiles().get(0).getDependencies().get(0); + assertEquals("org.slf4j", expanded.getGroupId()); + assertEquals("slf4j-api", expanded.getArtifactId()); + assertEquals("2.0.17", expanded.getVersion()); + assertNull(expanded.getId()); + } + + @Test + void testMergeDuplicatesExpandsProfileDependencyManagement() { + Dependency dep = + Dependency.newBuilder(false).id("org.slf4j:slf4j-api:2.0.17").build(); + Profile profile = Profile.newBuilder() + .id("test-profile") + .dependencyManagement(DependencyManagement.newBuilder() + .dependencies(List.of(dep)) + .build()) + .build(); + Model model = Model.newBuilder().profiles(List.of(profile)).build(); + + Model result = normalizer.mergeDuplicates(model, null, null); + + assertEquals(1, result.getProfiles().size()); + DependencyManagement mgmt = result.getProfiles().get(0).getDependencyManagement(); + assertEquals(1, mgmt.getDependencies().size()); + Dependency expanded = mgmt.getDependencies().get(0); + assertEquals("org.slf4j", expanded.getGroupId()); + assertEquals("slf4j-api", expanded.getArtifactId()); + assertEquals("2.0.17", expanded.getVersion()); + assertNull(expanded.getId()); + } +} diff --git a/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelValidatorTest.java b/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelValidatorTest.java index 591191e8920f..3514ee326fb0 100644 --- a/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelValidatorTest.java +++ b/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelValidatorTest.java @@ -1022,4 +1022,55 @@ void profileActivationConditionWithBasedirExpression() throws Exception { "raw-model/profile-activation-condition-with-basedir.xml", ModelValidator.VALIDATION_LEVEL_STRICT); assertViolations(result, 0, 0, 0); } + + @Test + void testDependencyId3partsValid() throws Exception { + SimpleProblemCollector result = validateFile("dependency-id-3parts-valid.xml"); + assertViolations(result, 0, 0, 0); + } + + @Test + void testDependencyId4partsValid() throws Exception { + SimpleProblemCollector result = validateFile("dependency-id-4parts-valid.xml"); + assertViolations(result, 0, 0, 0); + } + + @Test + void testDependencyId5partsValid() throws Exception { + SimpleProblemCollector result = validateFile("dependency-id-5parts-valid.xml"); + assertViolations(result, 0, 0, 0); + } + + @Test + void testDependencyIdBadFormat() throws Exception { + SimpleProblemCollector result = validateFile("dependency-id-bad-format.xml"); + assertViolations(result, 0, 1, 0); + assertContains(result.getErrors().get(0), "invalid format"); + } + + @Test + void testDependencyIdConflictGroupId() throws Exception { + SimpleProblemCollector result = validateFile("dependency-id-conflict-groupId.xml"); + assertViolations(result, 0, 1, 0); + assertContains(result.getErrors().get(0), "must not be specified when 'id' attribute is used"); + } + + @Test + void testDependencyIdConflictType() throws Exception { + SimpleProblemCollector result = validateFile("dependency-id-conflict-type.xml"); + assertViolations(result, 0, 1, 0); + assertContains(result.getErrors().get(0), "must not be specified when 'id' attribute includes type"); + } + + @Test + void testExclusionIdValid() throws Exception { + SimpleProblemCollector result = validateFile("exclusion-id-valid.xml"); + assertViolations(result, 0, 0, 0); + } + + @Test + void testExclusionIdBadFormat() throws Exception { + SimpleProblemCollector result = validateFile("exclusion-id-bad-format.xml"); + assertViolations(result, 0, 1, 0); + } } diff --git a/impl/maven-impl/src/test/resources/poms/validation/dependency-id-3parts-valid.xml b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-3parts-valid.xml new file mode 100644 index 000000000000..448265d0dbe9 --- /dev/null +++ b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-3parts-valid.xml @@ -0,0 +1,11 @@ + + 4.2.0 + test + test + 1.0 + + + + diff --git a/impl/maven-impl/src/test/resources/poms/validation/dependency-id-4parts-valid.xml b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-4parts-valid.xml new file mode 100644 index 000000000000..05baf6afb331 --- /dev/null +++ b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-4parts-valid.xml @@ -0,0 +1,11 @@ + + 4.2.0 + test + test + 1.0 + + + + diff --git a/impl/maven-impl/src/test/resources/poms/validation/dependency-id-5parts-valid.xml b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-5parts-valid.xml new file mode 100644 index 000000000000..133c0480d96a --- /dev/null +++ b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-5parts-valid.xml @@ -0,0 +1,11 @@ + + 4.2.0 + test + test + 1.0 + + + + diff --git a/impl/maven-impl/src/test/resources/poms/validation/dependency-id-bad-format.xml b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-bad-format.xml new file mode 100644 index 000000000000..e53959b8b05d --- /dev/null +++ b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-bad-format.xml @@ -0,0 +1,11 @@ + + 4.2.0 + test + test + 1.0 + + + + diff --git a/impl/maven-impl/src/test/resources/poms/validation/dependency-id-conflict-groupId.xml b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-conflict-groupId.xml new file mode 100644 index 000000000000..171ded535502 --- /dev/null +++ b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-conflict-groupId.xml @@ -0,0 +1,13 @@ + + 4.2.0 + test + test + 1.0 + + + org.other + + + diff --git a/impl/maven-impl/src/test/resources/poms/validation/dependency-id-conflict-type.xml b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-conflict-type.xml new file mode 100644 index 000000000000..f1fb01b3dfd8 --- /dev/null +++ b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-conflict-type.xml @@ -0,0 +1,13 @@ + + 4.2.0 + test + test + 1.0 + + + pom + + + diff --git a/impl/maven-impl/src/test/resources/poms/validation/exclusion-id-bad-format.xml b/impl/maven-impl/src/test/resources/poms/validation/exclusion-id-bad-format.xml new file mode 100644 index 000000000000..045977602564 --- /dev/null +++ b/impl/maven-impl/src/test/resources/poms/validation/exclusion-id-bad-format.xml @@ -0,0 +1,15 @@ + + 4.2.0 + test + test + 1.0 + + + + + + + + diff --git a/impl/maven-impl/src/test/resources/poms/validation/exclusion-id-valid.xml b/impl/maven-impl/src/test/resources/poms/validation/exclusion-id-valid.xml new file mode 100644 index 000000000000..3cd9825f2209 --- /dev/null +++ b/impl/maven-impl/src/test/resources/poms/validation/exclusion-id-valid.xml @@ -0,0 +1,15 @@ + + 4.2.0 + test + test + 1.0 + + + + + + + + diff --git a/its/core-it-suite/src/test/java/org/apache/maven/it/MavenITgh11904DependencyIdAttributeTest.java b/its/core-it-suite/src/test/java/org/apache/maven/it/MavenITgh11904DependencyIdAttributeTest.java new file mode 100644 index 000000000000..41a5ba5a2ff1 --- /dev/null +++ b/its/core-it-suite/src/test/java/org/apache/maven/it/MavenITgh11904DependencyIdAttributeTest.java @@ -0,0 +1,91 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.it; + +import java.io.Reader; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; +import java.util.stream.Stream; + +import org.apache.maven.api.model.Model; +import org.apache.maven.model.v4.MavenStaxReader; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Integration test for GH-11904. + * Verifies that dependency {@code id} attributes are expanded into individual + * GAV elements in the consumer POM and that no {@code id} attribute remains. + */ +class MavenITgh11904DependencyIdAttributeTest extends AbstractMavenIntegrationTestCase { + + MavenITgh11904DependencyIdAttributeTest() { + super("[4.0.0-rc-3-SNAPSHOT,)"); + } + + @Test + void testConsumerPomDoesNotContainIdAttributes() throws Exception { + Path basedir = extractResources("/gh-11904-dependency-id-attribute"); + + Verifier verifier = newVerifier(basedir); + verifier.addCliArguments("install"); + verifier.execute(); + verifier.verifyErrorFreeLog(); + + // Verify consumer POM for the child module + Path consumerPomPath = verifier.getArtifactPath( + "org.apache.maven.its.gh-11904", "child", "1.0.0-SNAPSHOT", "pom"); + assertTrue(Files.exists(consumerPomPath), "consumer POM should exist at " + consumerPomPath); + + // Verify raw XML does not contain any id= attribute on dependency or exclusion elements + List consumerPomLines; + try (Stream lines = Files.lines(consumerPomPath)) { + consumerPomLines = lines.toList(); + } + assertTrue( + consumerPomLines.stream() + .noneMatch(line -> line.contains(" elements"); + assertTrue( + consumerPomLines.stream() + .noneMatch(line -> line.contains(" elements"); + + // Parse and verify the consumer POM model + Model consumerModel; + try (Reader r = Files.newBufferedReader(consumerPomPath)) { + consumerModel = new MavenStaxReader().read(r); + } + + // Verify dependency was expanded correctly + assertNotNull(consumerModel.getDependencies(), "Consumer POM should have dependencies"); + assertEquals(1, consumerModel.getDependencies().size(), "Consumer POM should have one dependency"); + + var dep = consumerModel.getDependencies().get(0); + assertNull(dep.getId(), "Dependency id attribute should be null in consumer POM"); + assertEquals("org.apache.maven.its.gh-11904", dep.getGroupId()); + assertEquals("lib-a", dep.getArtifactId()); + assertEquals("1.0.0-SNAPSHOT", dep.getVersion()); + } +} diff --git a/its/core-it-suite/src/test/resources/gh-11904-dependency-id-attribute/.mvn/.gitkeep b/its/core-it-suite/src/test/resources/gh-11904-dependency-id-attribute/.mvn/.gitkeep new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/its/core-it-suite/src/test/resources/gh-11904-dependency-id-attribute/child/pom.xml b/its/core-it-suite/src/test/resources/gh-11904-dependency-id-attribute/child/pom.xml new file mode 100644 index 000000000000..834fd8b271a8 --- /dev/null +++ b/its/core-it-suite/src/test/resources/gh-11904-dependency-id-attribute/child/pom.xml @@ -0,0 +1,20 @@ + + + 4.2.0 + + + org.apache.maven.its.gh-11904 + parent + 1.0.0-SNAPSHOT + + + child + + + + + + + diff --git a/its/core-it-suite/src/test/resources/gh-11904-dependency-id-attribute/child/src/main/java/Test.java b/its/core-it-suite/src/test/resources/gh-11904-dependency-id-attribute/child/src/main/java/Test.java new file mode 100644 index 000000000000..ef22a69f1931 --- /dev/null +++ b/its/core-it-suite/src/test/resources/gh-11904-dependency-id-attribute/child/src/main/java/Test.java @@ -0,0 +1 @@ +class Test {} diff --git a/its/core-it-suite/src/test/resources/gh-11904-dependency-id-attribute/lib-a/pom.xml b/its/core-it-suite/src/test/resources/gh-11904-dependency-id-attribute/lib-a/pom.xml new file mode 100644 index 000000000000..26ab3ed3cd06 --- /dev/null +++ b/its/core-it-suite/src/test/resources/gh-11904-dependency-id-attribute/lib-a/pom.xml @@ -0,0 +1,15 @@ + + + 4.2.0 + + + org.apache.maven.its.gh-11904 + parent + 1.0.0-SNAPSHOT + + + lib-a + + diff --git a/its/core-it-suite/src/test/resources/gh-11904-dependency-id-attribute/pom.xml b/its/core-it-suite/src/test/resources/gh-11904-dependency-id-attribute/pom.xml new file mode 100644 index 000000000000..185a83958493 --- /dev/null +++ b/its/core-it-suite/src/test/resources/gh-11904-dependency-id-attribute/pom.xml @@ -0,0 +1,21 @@ + + + 4.2.0 + + org.apache.maven.its.gh-11904 + parent + 1.0.0-SNAPSHOT + pom + + + lib-a + child + + + + 17 + + + From e83a24fb1ee990b5b250c72b665eefe2506a31bf Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Tue, 23 Jun 2026 19:46:05 +0200 Subject: [PATCH 05/11] [GH-11904] Add mvnup strategy to collapse dependencies into id attributes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add DependencyIdStrategy that migrates verbose dependency/exclusion child elements into compact `id` attributes when upgrading to model version 4.2.0 via mvnup. The strategy runs at @Priority(25), after InferenceStrategy (30), so only dependencies that still have full explicit coordinates after inference are collapsed. Triggered by --model-version 4.2.0 or --all. Supported transformations: - g:a:v / g:a:type:v / g:a:type:classifier:v - g:a - Processes all sections: dependencies, dependencyManagement, profiles, plugin dependencies Inferred ids (partial coordinates like `:a` or `g:a` without version) are intentionally not supported — the `id` attribute is a shorthand for explicit coordinates, not a replacement for Maven's inference mechanism. Dependencies with missing coordinates after inference are left in their expanded form. Co-Authored-By: Claude Opus 4.6 --- .../mvnup/goals/DependencyIdStrategy.java | 294 ++++++ .../mvnup/goals/DependencyIdStrategyTest.java | 873 ++++++++++++++++++ 2 files changed, 1167 insertions(+) create mode 100644 impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/DependencyIdStrategy.java create mode 100644 impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/DependencyIdStrategyTest.java diff --git a/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/DependencyIdStrategy.java b/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/DependencyIdStrategy.java new file mode 100644 index 000000000000..3b77c1e6bf71 --- /dev/null +++ b/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/DependencyIdStrategy.java @@ -0,0 +1,294 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.cling.invoker.mvnup.goals; + +import java.nio.file.Path; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; + +import eu.maveniverse.domtrip.Document; +import eu.maveniverse.domtrip.Element; +import org.apache.maven.api.cli.mvnup.UpgradeOptions; +import org.apache.maven.api.di.Named; +import org.apache.maven.api.di.Priority; +import org.apache.maven.api.di.Singleton; +import org.apache.maven.cling.invoker.mvnup.UpgradeContext; + +import static eu.maveniverse.domtrip.maven.MavenPomElements.Elements.ARTIFACT_ID; +import static eu.maveniverse.domtrip.maven.MavenPomElements.Elements.BUILD; +import static eu.maveniverse.domtrip.maven.MavenPomElements.Elements.CLASSIFIER; +import static eu.maveniverse.domtrip.maven.MavenPomElements.Elements.DEPENDENCIES; +import static eu.maveniverse.domtrip.maven.MavenPomElements.Elements.DEPENDENCY; +import static eu.maveniverse.domtrip.maven.MavenPomElements.Elements.DEPENDENCY_MANAGEMENT; +import static eu.maveniverse.domtrip.maven.MavenPomElements.Elements.EXCLUSION; +import static eu.maveniverse.domtrip.maven.MavenPomElements.Elements.EXCLUSIONS; +import static eu.maveniverse.domtrip.maven.MavenPomElements.Elements.GROUP_ID; +import static eu.maveniverse.domtrip.maven.MavenPomElements.Elements.PLUGIN; +import static eu.maveniverse.domtrip.maven.MavenPomElements.Elements.PLUGINS; +import static eu.maveniverse.domtrip.maven.MavenPomElements.Elements.PROFILE; +import static eu.maveniverse.domtrip.maven.MavenPomElements.Elements.PROFILES; +import static eu.maveniverse.domtrip.maven.MavenPomElements.Elements.TYPE; +import static eu.maveniverse.domtrip.maven.MavenPomElements.Elements.VERSION; +import static eu.maveniverse.domtrip.maven.MavenPomElements.ModelVersions.MODEL_VERSION_4_2_0; + +/** + * Strategy for collapsing dependency coordinate child elements into compact {@code id} attributes. + * Only applies to POMs at model version 4.2.0 or higher. + * + *

Transforms verbose dependency declarations: + * 

{@code
+ * 
+ *   org.example
+ *   lib
+ *   1.0
+ * 
+ * }
+ * into compact form: + * 
{@code
+ * 
+ * }
+ * + *

Supports three formats: + *

    + *
  • {@code g:a:v} (3-part, default type "jar")
    • + *
  • {@code g:a:type:v} (4-part, non-default type)
    • + *
  • {@code g:a:type:classifier:v} (5-part, with classifier)
    • + *
+ * + *

Also collapses exclusions from {@code }/{@code } into {@code id="g:a"}. + */ +@Named +@Singleton +@Priority(25) +public class DependencyIdStrategy extends AbstractUpgradeStrategy { + + private static final String DEFAULT_TYPE = "jar"; + + @Override + public boolean isApplicable(UpgradeContext context) { + UpgradeOptions options = getOptions(context); + + if (options.all().orElse(false)) { + return true; + } + + String modelVersion = options.modelVersion().orElse(null); + return MODEL_VERSION_4_2_0.equals(modelVersion); + } + + @Override + public String getDescription() { + return "Collapsing dependency coordinates into id attributes"; + } + + @Override + protected UpgradeResult doApply(UpgradeContext context, Map pomMap) { + Set processedPoms = new HashSet<>(); + Set modifiedPoms = new HashSet<>(); + Set errorPoms = new HashSet<>(); + + for (Map.Entry entry : pomMap.entrySet()) { + Path pomPath = entry.getKey(); + Document pomDocument = entry.getValue(); + processedPoms.add(pomPath); + + String currentVersion = ModelVersionUtils.detectModelVersion(pomDocument); + context.info(pomPath + " (current: " + currentVersion + ")"); + context.indent(); + + try { + if (!MODEL_VERSION_4_2_0.equals(currentVersion) && !ModelVersionUtils.isNewerThan410(currentVersion)) { + context.success("Skipping (model version " + currentVersion + " < 4.2.0)"); + continue; + } + if (!MODEL_VERSION_4_2_0.equals(currentVersion)) { + context.success("Skipping (model version " + currentVersion + " is not 4.2.0)"); + continue; + } + + boolean hasChanges = collapseDependencies(context, pomDocument); + + if (hasChanges) { + modifiedPoms.add(pomPath); + context.success("Dependency coordinates collapsed into id attributes"); + } else { + context.success("No dependencies to collapse"); + } + } catch (Exception e) { + context.failure("Failed to collapse dependency coordinates: " + e.getMessage()); + errorPoms.add(pomPath); + } finally { + context.unindent(); + } + } + + return new UpgradeResult(processedPoms, modifiedPoms, errorPoms); + } + + private boolean collapseDependencies(UpgradeContext context, Document pomDocument) { + Element root = pomDocument.root(); + boolean hasChanges = false; + + // Process + hasChanges |= processDependenciesSection( + context, root.childElement(DEPENDENCIES).orElse(null)); + + // Process + hasChanges |= root.childElement(DEPENDENCY_MANAGEMENT) + .flatMap(dm -> dm.childElement(DEPENDENCIES)) + .map(deps -> processDependenciesSection(context, deps)) + .orElse(false); + + // Process dependencies and dependencyManagement + hasChanges |= root.childElement(PROFILES).stream() + .flatMap(profiles -> profiles.childElements(PROFILE)) + .map(profile -> { + boolean profileChanges = false; + profileChanges |= profile.childElement(DEPENDENCIES) + .map(deps -> processDependenciesSection(context, deps)) + .orElse(false); + profileChanges |= profile.childElement(DEPENDENCY_MANAGEMENT) + .flatMap(dm -> dm.childElement(DEPENDENCIES)) + .map(deps -> processDependenciesSection(context, deps)) + .orElse(false); + return profileChanges; + }) + .reduce(false, Boolean::logicalOr); + + // Process + hasChanges |= root.childElement(BUILD).flatMap(build -> build.childElement(PLUGINS)).stream() + .flatMap(plugins -> plugins.childElements(PLUGIN)) + .map(plugin -> plugin.childElement(DEPENDENCIES) + .map(deps -> processDependenciesSection(context, deps)) + .orElse(false)) + .reduce(false, Boolean::logicalOr); + + return hasChanges; + } + + private boolean processDependenciesSection(UpgradeContext context, Element dependenciesElement) { + if (dependenciesElement == null) { + return false; + } + + List dependencyElements = + dependenciesElement.childElements(DEPENDENCY).toList(); + boolean hasChanges = false; + + for (Element dependency : dependencyElements) { + hasChanges |= collapseDependency(context, dependency); + hasChanges |= collapseExclusions(context, dependency); + } + + return hasChanges; + } + + private boolean collapseDependency(UpgradeContext context, Element dependency) { + if (dependency.attribute("id") != null) { + return false; + } + + String groupId = dependency.childText(GROUP_ID); + String artifactId = dependency.childText(ARTIFACT_ID); + String version = dependency.childText(VERSION); + + if (groupId == null || artifactId == null || version == null) { + return false; + } + + String type = dependency.childText(TYPE); + String classifier = dependency.childText(CLASSIFIER); + + String id = buildIdValue(groupId, artifactId, version, type, classifier); + dependency.attribute("id", id); + + removeChildElement(dependency, GROUP_ID); + removeChildElement(dependency, ARTIFACT_ID); + removeChildElement(dependency, VERSION); + + if (classifier != null) { + removeChildElement(dependency, CLASSIFIER); + removeChildElement(dependency, TYPE); + } else if (type != null && !DEFAULT_TYPE.equals(type)) { + removeChildElement(dependency, TYPE); + } else if (type != null) { + // default type "jar" — remove it as it's implied + removeChildElement(dependency, TYPE); + } + + context.detail("Collapsed: " + id); + return true; + } + + private boolean collapseExclusions(UpgradeContext context, Element dependency) { + Element exclusionsElement = dependency.childElement(EXCLUSIONS).orElse(null); + if (exclusionsElement == null) { + return false; + } + + List exclusionElements = + exclusionsElement.childElements(EXCLUSION).toList(); + boolean hasChanges = false; + + for (Element exclusion : exclusionElements) { + hasChanges |= collapseExclusion(context, exclusion); + } + + return hasChanges; + } + + private boolean collapseExclusion(UpgradeContext context, Element exclusion) { + if (exclusion.attribute("id") != null) { + return false; + } + + String groupId = exclusion.childText(GROUP_ID); + String artifactId = exclusion.childText(ARTIFACT_ID); + + if (groupId == null || artifactId == null) { + return false; + } + + String id = groupId + ":" + artifactId; + exclusion.attribute("id", id); + + removeChildElement(exclusion, GROUP_ID); + removeChildElement(exclusion, ARTIFACT_ID); + + context.detail("Collapsed exclusion: " + id); + return true; + } + + static String buildIdValue(String groupId, String artifactId, String version, String type, String classifier) { + if (classifier != null && !classifier.isEmpty()) { + String effectiveType = (type != null && !type.isEmpty()) ? type : DEFAULT_TYPE; + return groupId + ":" + artifactId + ":" + effectiveType + ":" + classifier + ":" + version; + } else if (type != null && !type.isEmpty() && !DEFAULT_TYPE.equals(type)) { + return groupId + ":" + artifactId + ":" + type + ":" + version; + } else { + return groupId + ":" + artifactId + ":" + version; + } + } + + private static void removeChildElement(Element parent, String childName) { + parent.childElement(childName).ifPresent(DomUtils::removeElement); + } +} diff --git a/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/DependencyIdStrategyTest.java b/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/DependencyIdStrategyTest.java new file mode 100644 index 000000000000..5c8b9abc9c18 --- /dev/null +++ b/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/DependencyIdStrategyTest.java @@ -0,0 +1,873 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.cling.invoker.mvnup.goals; + +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.HashMap; +import java.util.Map; + +import eu.maveniverse.domtrip.Document; +import eu.maveniverse.domtrip.Element; +import org.apache.maven.cling.invoker.mvnup.UpgradeContext; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +@DisplayName("DependencyIdStrategy") +class DependencyIdStrategyTest { + + private DependencyIdStrategy strategy; + + @BeforeEach + void setUp() { + strategy = new DependencyIdStrategy(); + } + + @Nested + @DisplayName("Applicability") + class ApplicabilityTests { + + @Test + @DisplayName("should not be applicable by default") + void shouldNotBeApplicableByDefault() { + UpgradeContext context = TestUtils.createMockContext(TestUtils.createDefaultOptions()); + assertFalse(strategy.isApplicable(context)); + } + + @Test + @DisplayName("should be applicable with --model-version 4.2.0") + void shouldBeApplicableWithModelVersion420() { + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.2.0")); + assertTrue(strategy.isApplicable(context)); + } + + @Test + @DisplayName("should not be applicable with --model-version 4.1.0") + void shouldNotBeApplicableWithModelVersion410() { + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); + assertFalse(strategy.isApplicable(context)); + } + + @Test + @DisplayName("should be applicable with --all") + void shouldBeApplicableWithAll() { + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithAll(true)); + assertTrue(strategy.isApplicable(context)); + } + + @Test + @DisplayName("should not be applicable with --all=false") + void shouldNotBeApplicableWithAllFalse() { + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithAll(false)); + assertFalse(strategy.isApplicable(context)); + } + } + + @Nested + @DisplayName("Dependency Collapsing") + class DependencyCollapsingTests { + + @Test + @DisplayName("should collapse g:a:v dependency") + void shouldCollapseGav() { + String pomXml = """ + + + 4.2.0 + com.example + test + 1.0 + + + org.example + lib + 1.0 + + + + """; + + Document doc = Document.of(pomXml); + Map pomMap = Map.of(Paths.get("pom.xml"), doc); + + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.2.0")); + strategy.doApply(context, new HashMap<>(pomMap)); + + Element dependency = doc.root() + .childElement("dependencies") + .orElseThrow() + .childElement("dependency") + .orElseThrow(); + + assertEquals("org.example:lib:1.0", dependency.attribute("id")); + assertNull(DomUtils.findChildElement(dependency, "groupId")); + assertNull(DomUtils.findChildElement(dependency, "artifactId")); + assertNull(DomUtils.findChildElement(dependency, "version")); + } + + @Test + @DisplayName("should collapse g:a:type:v dependency with non-default type") + void shouldCollapseGavWithType() { + String pomXml = """ + + + 4.2.0 + com.example + test + 1.0 + + + org.example + lib + pom + 1.0 + + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.2.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + Element dependency = doc.root() + .childElement("dependencies") + .orElseThrow() + .childElement("dependency") + .orElseThrow(); + + assertEquals("org.example:lib:pom:1.0", dependency.attribute("id")); + assertNull(DomUtils.findChildElement(dependency, "type")); + } + + @Test + @DisplayName("should collapse g:a:type:classifier:v dependency") + void shouldCollapseGavWithClassifier() { + String pomXml = """ + + + 4.2.0 + com.example + test + 1.0 + + + org.example + lib + jar + sources + 1.0 + + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.2.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + Element dependency = doc.root() + .childElement("dependencies") + .orElseThrow() + .childElement("dependency") + .orElseThrow(); + + assertEquals("org.example:lib:jar:sources:1.0", dependency.attribute("id")); + assertNull(DomUtils.findChildElement(dependency, "type")); + assertNull(DomUtils.findChildElement(dependency, "classifier")); + } + + @Test + @DisplayName("should use default type jar in 5-part format when type absent but classifier present") + void shouldUseDefaultTypeWhenClassifierPresent() { + String pomXml = """ + + + 4.2.0 + com.example + test + 1.0 + + + org.example + lib + sources + 1.0 + + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.2.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + Element dependency = doc.root() + .childElement("dependencies") + .orElseThrow() + .childElement("dependency") + .orElseThrow(); + + assertEquals("org.example:lib:jar:sources:1.0", dependency.attribute("id")); + } + + @Test + @DisplayName("should not include default type jar in 3-part format") + void shouldNotIncludeDefaultTypeInId() { + String pomXml = """ + + + 4.2.0 + com.example + test + 1.0 + + + org.example + lib + jar + 1.0 + + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.2.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + Element dependency = doc.root() + .childElement("dependencies") + .orElseThrow() + .childElement("dependency") + .orElseThrow(); + + assertEquals("org.example:lib:1.0", dependency.attribute("id")); + assertNull(DomUtils.findChildElement(dependency, "type")); + } + + @Test + @DisplayName("should preserve scope when collapsing") + void shouldPreserveScopeWhenCollapsing() { + String pomXml = """ + + + 4.2.0 + com.example + test + 1.0 + + + org.example + lib + 1.0 + test + + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.2.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + Element dependency = doc.root() + .childElement("dependencies") + .orElseThrow() + .childElement("dependency") + .orElseThrow(); + + assertEquals("org.example:lib:1.0", dependency.attribute("id")); + assertNotNull(DomUtils.findChildElement(dependency, "scope")); + assertEquals("test", dependency.childText("scope")); + } + + @Test + @DisplayName("should preserve optional when collapsing") + void shouldPreserveOptionalWhenCollapsing() { + String pomXml = """ + + + 4.2.0 + com.example + test + 1.0 + + + org.example + lib + 1.0 + true + + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.2.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + Element dependency = doc.root() + .childElement("dependencies") + .orElseThrow() + .childElement("dependency") + .orElseThrow(); + + assertEquals("org.example:lib:1.0", dependency.attribute("id")); + assertNotNull(DomUtils.findChildElement(dependency, "optional")); + } + + @Test + @DisplayName("should skip dependency without version") + void shouldSkipDependencyWithoutVersion() { + String pomXml = """ + + + 4.2.0 + com.example + test + 1.0 + + + org.example + lib + + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.2.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + Element dependency = doc.root() + .childElement("dependencies") + .orElseThrow() + .childElement("dependency") + .orElseThrow(); + + assertNull(dependency.attribute("id")); + assertNotNull(DomUtils.findChildElement(dependency, "groupId")); + assertNotNull(DomUtils.findChildElement(dependency, "artifactId")); + } + + @Test + @DisplayName("should skip dependency that already has id attribute") + void shouldSkipDependencyWithExistingId() { + String pomXml = """ + + + 4.2.0 + com.example + test + 1.0 + + + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.2.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + assertEquals( + "org.example:lib:1.0", + doc.root() + .childElement("dependencies") + .orElseThrow() + .childElement("dependency") + .orElseThrow() + .attribute("id")); + } + + @Test + @DisplayName("should collapse multiple dependencies") + void shouldCollapseMultipleDependencies() { + String pomXml = """ + + + 4.2.0 + com.example + test + 1.0 + + + org.example + lib-a + 1.0 + + + org.example + lib-b + 2.0 + test + + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.2.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + var deps = doc.root() + .childElement("dependencies") + .orElseThrow() + .childElements("dependency") + .toList(); + + assertEquals(2, deps.size()); + assertEquals("org.example:lib-a:1.0", deps.get(0).attribute("id")); + assertEquals("org.example:lib-b:2.0", deps.get(1).attribute("id")); + } + } + + @Nested + @DisplayName("Exclusion Collapsing") + class ExclusionCollapsingTests { + + @Test + @DisplayName("should collapse exclusion into id attribute") + void shouldCollapseExclusion() { + String pomXml = """ + + + 4.2.0 + com.example + test + 1.0 + + + org.example + lib + 1.0 + + + org.unwanted + bad + + + + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.2.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + Element dependency = doc.root() + .childElement("dependencies") + .orElseThrow() + .childElement("dependency") + .orElseThrow(); + + assertEquals("org.example:lib:1.0", dependency.attribute("id")); + + Element exclusion = dependency + .childElement("exclusions") + .orElseThrow() + .childElement("exclusion") + .orElseThrow(); + + assertEquals("org.unwanted:bad", exclusion.attribute("id")); + assertNull(DomUtils.findChildElement(exclusion, "groupId")); + assertNull(DomUtils.findChildElement(exclusion, "artifactId")); + } + + @Test + @DisplayName("should collapse multiple exclusions") + void shouldCollapseMultipleExclusions() { + String pomXml = """ + + + 4.2.0 + com.example + test + 1.0 + + + org.example + lib + 1.0 + + + org.a + bad-a + + + org.b + bad-b + + + + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.2.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + var exclusions = doc.root() + .childElement("dependencies") + .orElseThrow() + .childElement("dependency") + .orElseThrow() + .childElement("exclusions") + .orElseThrow() + .childElements("exclusion") + .toList(); + + assertEquals(2, exclusions.size()); + assertEquals("org.a:bad-a", exclusions.get(0).attribute("id")); + assertEquals("org.b:bad-b", exclusions.get(1).attribute("id")); + } + } + + @Nested + @DisplayName("Section Processing") + class SectionProcessingTests { + + @Test + @DisplayName("should process dependencyManagement dependencies") + void shouldProcessDependencyManagement() { + String pomXml = """ + + + 4.2.0 + com.example + test + 1.0 + + + + org.example + lib + 1.0 + + + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.2.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + Element dependency = doc.root() + .childElement("dependencyManagement") + .orElseThrow() + .childElement("dependencies") + .orElseThrow() + .childElement("dependency") + .orElseThrow(); + + assertEquals("org.example:lib:1.0", dependency.attribute("id")); + } + + @Test + @DisplayName("should process profile dependencies") + void shouldProcessProfileDependencies() { + String pomXml = """ + + + 4.2.0 + com.example + test + 1.0 + + + test-profile + + + org.example + lib + 1.0 + + + + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.2.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + Element dependency = doc.root() + .childElement("profiles") + .orElseThrow() + .childElement("profile") + .orElseThrow() + .childElement("dependencies") + .orElseThrow() + .childElement("dependency") + .orElseThrow(); + + assertEquals("org.example:lib:1.0", dependency.attribute("id")); + } + + @Test + @DisplayName("should process profile dependencyManagement") + void shouldProcessProfileDependencyManagement() { + String pomXml = """ + + + 4.2.0 + com.example + test + 1.0 + + + test-profile + + + + org.example + lib + 1.0 + + + + + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.2.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + Element dependency = doc.root() + .childElement("profiles") + .orElseThrow() + .childElement("profile") + .orElseThrow() + .childElement("dependencyManagement") + .orElseThrow() + .childElement("dependencies") + .orElseThrow() + .childElement("dependency") + .orElseThrow(); + + assertEquals("org.example:lib:1.0", dependency.attribute("id")); + } + + @Test + @DisplayName("should process plugin dependencies") + void shouldProcessPluginDependencies() { + String pomXml = """ + + + 4.2.0 + com.example + test + 1.0 + + + + org.apache.maven.plugins + maven-compiler-plugin + 3.11.0 + + + org.example + lib + 1.0 + + + + + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.2.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + Element dependency = doc.root() + .childElement("build") + .orElseThrow() + .childElement("plugins") + .orElseThrow() + .childElement("plugin") + .orElseThrow() + .childElement("dependencies") + .orElseThrow() + .childElement("dependency") + .orElseThrow(); + + assertEquals("org.example:lib:1.0", dependency.attribute("id")); + } + } + + @Nested + @DisplayName("Model Version Filtering") + class ModelVersionFilteringTests { + + @Test + @DisplayName("should skip non-4.2.0 POMs") + void shouldSkipNon420Poms() { + String pomXml = """ + + + 4.1.0 + com.example + test + 1.0 + + + org.example + lib + 1.0 + + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.2.0")); + UpgradeResult result = strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + Element dependency = doc.root() + .childElement("dependencies") + .orElseThrow() + .childElement("dependency") + .orElseThrow(); + + assertNull(dependency.attribute("id")); + assertNotNull(DomUtils.findChildElement(dependency, "groupId")); + assertEquals(0, result.modifiedCount()); + } + + @Test + @DisplayName("should process only 4.2.0 POMs in mixed map") + void shouldProcessOnly420PomsInMixedMap() { + String pom410Xml = """ + + + 4.1.0 + com.example + old-module + 1.0 + + + org.example + lib + 1.0 + + + + """; + + String pom420Xml = """ + + + 4.2.0 + com.example + new-module + 1.0 + + + org.example + lib + 1.0 + + + + """; + + Document doc410 = Document.of(pom410Xml); + Document doc420 = Document.of(pom420Xml); + Map pomMap = new HashMap<>(); + pomMap.put(Paths.get("old", "pom.xml"), doc410); + pomMap.put(Paths.get("new", "pom.xml"), doc420); + + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.2.0")); + UpgradeResult result = strategy.doApply(context, pomMap); + + // 4.1.0 POM should be untouched + assertNull(doc410.root() + .childElement("dependencies") + .orElseThrow() + .childElement("dependency") + .orElseThrow() + .attribute("id")); + + // 4.2.0 POM should be collapsed + assertEquals( + "org.example:lib:1.0", + doc420.root() + .childElement("dependencies") + .orElseThrow() + .childElement("dependency") + .orElseThrow() + .attribute("id")); + + assertEquals(1, result.modifiedCount()); + } + } + + @Nested + @DisplayName("buildIdValue") + class BuildIdValueTests { + + @Test + void threePartFormat() { + assertEquals("g:a:1.0", DependencyIdStrategy.buildIdValue("g", "a", "1.0", null, null)); + } + + @Test + void threePartFormatWithDefaultType() { + assertEquals("g:a:1.0", DependencyIdStrategy.buildIdValue("g", "a", "1.0", "jar", null)); + } + + @Test + void fourPartFormat() { + assertEquals("g:a:pom:1.0", DependencyIdStrategy.buildIdValue("g", "a", "1.0", "pom", null)); + } + + @Test + void fivePartFormat() { + assertEquals("g:a:jar:sources:1.0", DependencyIdStrategy.buildIdValue("g", "a", "1.0", "jar", "sources")); + } + + @Test + void fivePartFormatWithNonDefaultType() { + assertEquals( + "g:a:test-jar:tests:1.0", DependencyIdStrategy.buildIdValue("g", "a", "1.0", "test-jar", "tests")); + } + + @Test + void fivePartFormatClassifierNoType() { + assertEquals("g:a:jar:sources:1.0", DependencyIdStrategy.buildIdValue("g", "a", "1.0", null, "sources")); + } + } +} From 3d4ddbec46fa36cc1b3f45c6ad31d155f2a191ec Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Tue, 23 Jun 2026 22:13:40 +0200 Subject: [PATCH 06/11] [GH-11904] Support managed-version formats in dependency id attribute Add support for 2-part `g:a` format and trailing-colon formats (`g:a:`, `g:a:type:`, `g:a:type:classifier:`) for dependencies whose version is provided by dependencyManagement. The trailing colon convention signals that version is managed, not missing. Leave aside inferred ids (`:a:` with inferred groupId) for now. Co-Authored-By: Claude Opus 4.6 --- api/maven-api-model/src/main/mdo/maven.mdo | 8 +- .../mvnup/goals/DependencyIdStrategy.java | 34 ++++-- .../mvnup/goals/DependencyIdStrategyTest.java | 104 +++++++++++++++++- .../impl/model/DefaultModelNormalizer.java | 31 ++++-- .../impl/model/DefaultModelValidator.java | 9 +- .../model/DefaultModelNormalizerTest.java | 54 +++++++++ .../impl/model/DefaultModelValidatorTest.java | 24 ++++ .../validation/dependency-id-2parts-valid.xml | 11 ++ ...endency-id-trailing-colon-3parts-valid.xml | 11 ++ ...endency-id-trailing-colon-4parts-valid.xml | 11 ++ ...endency-id-trailing-colon-5parts-valid.xml | 11 ++ 11 files changed, 275 insertions(+), 33 deletions(-) create mode 100644 impl/maven-impl/src/test/resources/poms/validation/dependency-id-2parts-valid.xml create mode 100644 impl/maven-impl/src/test/resources/poms/validation/dependency-id-trailing-colon-3parts-valid.xml create mode 100644 impl/maven-impl/src/test/resources/poms/validation/dependency-id-trailing-colon-4parts-valid.xml create mode 100644 impl/maven-impl/src/test/resources/poms/validation/dependency-id-trailing-colon-5parts-valid.xml diff --git a/api/maven-api-model/src/main/mdo/maven.mdo b/api/maven-api-model/src/main/mdo/maven.mdo index 00681ebe4ffa..8501c3571106 100644 --- a/api/maven-api-model/src/main/mdo/maven.mdo +++ b/api/maven-api-model/src/main/mdo/maven.mdo @@ -1211,9 +1211,13 @@ * } * - *

Supports three formats: + *

Supported formats (trailing {@code :} means version is managed): *

    - *
  • {@code g:a:v} (3-part, default type "jar")
    • - *
  • {@code g:a:type:v} (4-part, non-default type)
    • - *
  • {@code g:a:type:classifier:v} (5-part, with classifier)
    • + *
  • {@code g:a} — version managed
    • + *
  • {@code g:a:v} — default type "jar"
    • + *
  • {@code g:a:type:} — non-default type, version managed
    • + *
  • {@code g:a:type:v} — non-default type
    • + *
  • {@code g:a:type:classifier:} — with classifier, version managed
    • + *
  • {@code g:a:type:classifier:v} — with classifier
    • *
* *

Also collapses exclusions from {@code }/{@code } into {@code id="g:a"}. @@ -210,7 +213,7 @@ private boolean collapseDependency(UpgradeContext context, Element dependency) { String artifactId = dependency.childText(ARTIFACT_ID); String version = dependency.childText(VERSION); - if (groupId == null || artifactId == null || version == null) { + if (groupId == null || artifactId == null) { return false; } @@ -222,7 +225,9 @@ private boolean collapseDependency(UpgradeContext context, Element dependency) { removeChildElement(dependency, GROUP_ID); removeChildElement(dependency, ARTIFACT_ID); - removeChildElement(dependency, VERSION); + if (version != null) { + removeChildElement(dependency, VERSION); + } if (classifier != null) { removeChildElement(dependency, CLASSIFIER); @@ -230,7 +235,6 @@ private boolean collapseDependency(UpgradeContext context, Element dependency) { } else if (type != null && !DEFAULT_TYPE.equals(type)) { removeChildElement(dependency, TYPE); } else if (type != null) { - // default type "jar" — remove it as it's implied removeChildElement(dependency, TYPE); } @@ -280,11 +284,21 @@ private boolean collapseExclusion(UpgradeContext context, Element exclusion) { static String buildIdValue(String groupId, String artifactId, String version, String type, String classifier) { if (classifier != null && !classifier.isEmpty()) { String effectiveType = (type != null && !type.isEmpty()) ? type : DEFAULT_TYPE; - return groupId + ":" + artifactId + ":" + effectiveType + ":" + classifier + ":" + version; + if (version != null) { + return groupId + ":" + artifactId + ":" + effectiveType + ":" + classifier + ":" + version; + } else { + return groupId + ":" + artifactId + ":" + effectiveType + ":" + classifier + ":"; + } } else if (type != null && !type.isEmpty() && !DEFAULT_TYPE.equals(type)) { - return groupId + ":" + artifactId + ":" + type + ":" + version; - } else { + if (version != null) { + return groupId + ":" + artifactId + ":" + type + ":" + version; + } else { + return groupId + ":" + artifactId + ":" + type + ":"; + } + } else if (version != null) { return groupId + ":" + artifactId + ":" + version; + } else { + return groupId + ":" + artifactId; } } diff --git a/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/DependencyIdStrategyTest.java b/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/DependencyIdStrategyTest.java index 5c8b9abc9c18..3be91fb16434 100644 --- a/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/DependencyIdStrategyTest.java +++ b/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/DependencyIdStrategyTest.java @@ -342,8 +342,8 @@ void shouldPreserveOptionalWhenCollapsing() { } @Test - @DisplayName("should skip dependency without version") - void shouldSkipDependencyWithoutVersion() { + @DisplayName("should collapse dependency without version to g:a") + void shouldCollapseDependencyWithoutVersion() { String pomXml = """ @@ -370,9 +370,83 @@ void shouldSkipDependencyWithoutVersion() { .childElement("dependency") .orElseThrow(); - assertNull(dependency.attribute("id")); - assertNotNull(DomUtils.findChildElement(dependency, "groupId")); - assertNotNull(DomUtils.findChildElement(dependency, "artifactId")); + assertEquals("org.example:lib", dependency.attribute("id")); + assertNull(DomUtils.findChildElement(dependency, "groupId")); + assertNull(DomUtils.findChildElement(dependency, "artifactId")); + } + + @Test + @DisplayName("should collapse managed-version dependency with non-default type to g:a:type:") + void shouldCollapseManagedWithType() { + String pomXml = """ + + + 4.2.0 + com.example + test + 1.0 + + + org.example + lib + pom + + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.2.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + Element dependency = doc.root() + .childElement("dependencies") + .orElseThrow() + .childElement("dependency") + .orElseThrow(); + + assertEquals("org.example:lib:pom:", dependency.attribute("id")); + assertNull(DomUtils.findChildElement(dependency, "groupId")); + assertNull(DomUtils.findChildElement(dependency, "artifactId")); + assertNull(DomUtils.findChildElement(dependency, "type")); + } + + @Test + @DisplayName("should collapse managed-version dependency with classifier to g:a:type:classifier:") + void shouldCollapseManagedWithClassifier() { + String pomXml = """ + + + 4.2.0 + com.example + test + 1.0 + + + org.example + lib + jar + sources + + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.2.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + Element dependency = doc.root() + .childElement("dependencies") + .orElseThrow() + .childElement("dependency") + .orElseThrow(); + + assertEquals("org.example:lib:jar:sources:", dependency.attribute("id")); + assertNull(DomUtils.findChildElement(dependency, "groupId")); + assertNull(DomUtils.findChildElement(dependency, "artifactId")); + assertNull(DomUtils.findChildElement(dependency, "type")); + assertNull(DomUtils.findChildElement(dependency, "classifier")); } @Test @@ -839,6 +913,11 @@ void shouldProcessOnly420PomsInMixedMap() { @DisplayName("buildIdValue") class BuildIdValueTests { + @Test + void twoPartFormat() { + assertEquals("g:a", DependencyIdStrategy.buildIdValue("g", "a", null, null, null)); + } + @Test void threePartFormat() { assertEquals("g:a:1.0", DependencyIdStrategy.buildIdValue("g", "a", "1.0", null, null)); @@ -869,5 +948,20 @@ void fivePartFormatWithNonDefaultType() { void fivePartFormatClassifierNoType() { assertEquals("g:a:jar:sources:1.0", DependencyIdStrategy.buildIdValue("g", "a", "1.0", null, "sources")); } + + @Test + void fourPartTrailingColon() { + assertEquals("g:a:pom:", DependencyIdStrategy.buildIdValue("g", "a", null, "pom", null)); + } + + @Test + void fivePartTrailingColon() { + assertEquals("g:a:jar:sources:", DependencyIdStrategy.buildIdValue("g", "a", null, "jar", "sources")); + } + + @Test + void fivePartTrailingColonClassifierNoType() { + assertEquals("g:a:jar:sources:", DependencyIdStrategy.buildIdValue("g", "a", null, null, "sources")); + } } } diff --git a/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelNormalizer.java b/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelNormalizer.java index a36992f33fa4..e8b13988f037 100644 --- a/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelNormalizer.java +++ b/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelNormalizer.java @@ -199,11 +199,15 @@ private Profile expandProfileDependencyIds(Profile profile) { /** * Expands the {@code id} attribute on a dependency into its component fields. - * Supported formats: + * Supported formats (trailing {@code :} means version is managed): *

    - *
  • {@code groupId:artifactId:version} (3 parts)
    • - *
  • {@code groupId:artifactId:type:version} (4 parts)
    • - *
  • {@code groupId:artifactId:type:classifier:version} (5 parts)
    • + *
  • {@code groupId:artifactId} (version managed)
    • + *
  • {@code groupId:artifactId:} (version managed, explicit trailing colon)
    • + *
  • {@code groupId:artifactId:version}
    • + *
  • {@code groupId:artifactId:type:} (version managed, with type)
    • + *
  • {@code groupId:artifactId:type:version}
    • + *
  • {@code groupId:artifactId:type:classifier:} (version managed, with type and classifier)
    • + *
  • {@code groupId:artifactId:type:classifier:version}
    • *
*/ Dependency expandDependencyId(Dependency d) { @@ -212,8 +216,8 @@ Dependency expandDependencyId(Dependency d) { List expanded = injectList(d.getExclusions(), this::expandExclusionId); return expanded != null ? d.withExclusions(expanded) : d; } - String[] parts = id.split(":"); - if (parts.length < 3 || parts.length > 5) { + String[] parts = id.split(":", -1); + if (parts.length < 2 || parts.length > 5) { return d; } Dependency.Builder builder = Dependency.newBuilder(d, true); @@ -225,30 +229,33 @@ Dependency expandDependencyId(Dependency d) { builder.artifactId(parts[1]); } switch (parts.length) { + case 2: + // g:a (version managed) + break; case 3: - // g:a:v - if (isNullOrEmpty(d.getVersion())) { + // g:a:v or g:a: (managed) + if (!parts[2].isEmpty() && isNullOrEmpty(d.getVersion())) { builder.version(parts[2]); } break; case 4: - // g:a:type:v + // g:a:type:v or g:a:type: (managed) if (isNullOrEmptyOrDefault(d.getType())) { builder.type(parts[2]); } - if (isNullOrEmpty(d.getVersion())) { + if (!parts[3].isEmpty() && isNullOrEmpty(d.getVersion())) { builder.version(parts[3]); } break; case 5: - // g:a:type:classifier:v + // g:a:type:classifier:v or g:a:type:classifier: (managed) if (isNullOrEmptyOrDefault(d.getType())) { builder.type(parts[2]); } if (isNullOrEmpty(d.getClassifier())) { builder.classifier(parts[3]); } - if (isNullOrEmpty(d.getVersion())) { + if (!parts[4].isEmpty() && isNullOrEmpty(d.getVersion())) { builder.version(parts[4]); } break; diff --git a/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java b/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java index 72e65e1864ff..e4d5b26f993c 100644 --- a/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java +++ b/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java @@ -1269,16 +1269,17 @@ private void validateDependencyIdAttribute(ModelProblemCollector problems, Depen return; } int colonCount = id.length() - id.replace(":", "").length(); - if (colonCount < 2 || colonCount > 4) { + if (colonCount < 1 || colonCount > 4) { addViolation( problems, Severity.ERROR, Version.V42, prefix + "id", null, - "has invalid format '" + id + "', must be 'groupId:artifactId:version', " - + "'groupId:artifactId:type:version', or " - + "'groupId:artifactId:type:classifier:version'.", + "has invalid format '" + id + "', must be 'groupId:artifactId[:]', " + + "'groupId:artifactId:version', " + + "'groupId:artifactId:type:[version]', or " + + "'groupId:artifactId:type:classifier:[version]'.", dependency); return; } diff --git a/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelNormalizerTest.java b/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelNormalizerTest.java index e4a84dbbb334..8eca6c66adc7 100644 --- a/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelNormalizerTest.java +++ b/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelNormalizerTest.java @@ -35,6 +35,30 @@ class DefaultModelNormalizerTest { private final DefaultModelNormalizer normalizer = new DefaultModelNormalizer(); + @Test + void testExpandDependencyId2parts() { + Dependency dep = Dependency.newBuilder(false).id("org.example:lib").build(); + + Dependency result = normalizer.expandDependencyId(dep); + + assertEquals("org.example", result.getGroupId()); + assertEquals("lib", result.getArtifactId()); + assertNull(result.getVersion()); + assertNull(result.getId()); + } + + @Test + void testExpandDependencyId2partsTrailingColon() { + Dependency dep = Dependency.newBuilder(false).id("org.example:lib:").build(); + + Dependency result = normalizer.expandDependencyId(dep); + + assertEquals("org.example", result.getGroupId()); + assertEquals("lib", result.getArtifactId()); + assertNull(result.getVersion()); + assertNull(result.getId()); + } + @Test void testExpandDependencyId3parts() { Dependency dep = @@ -62,6 +86,20 @@ void testExpandDependencyId4parts() { assertNull(result.getId()); } + @Test + void testExpandDependencyId4partsTrailingColon() { + Dependency dep = + Dependency.newBuilder(false).id("org.example:lib-b:pom:").build(); + + Dependency result = normalizer.expandDependencyId(dep); + + assertEquals("org.example", result.getGroupId()); + assertEquals("lib-b", result.getArtifactId()); + assertEquals("pom", result.getType()); + assertNull(result.getVersion()); + assertNull(result.getId()); + } + @Test void testExpandDependencyId5parts() { Dependency dep = Dependency.newBuilder(false) @@ -78,6 +116,22 @@ void testExpandDependencyId5parts() { assertNull(result.getId()); } + @Test + void testExpandDependencyId5partsTrailingColon() { + Dependency dep = Dependency.newBuilder(false) + .id("org.example:lib-c:jar:sources:") + .build(); + + Dependency result = normalizer.expandDependencyId(dep); + + assertEquals("org.example", result.getGroupId()); + assertEquals("lib-c", result.getArtifactId()); + assertEquals("jar", result.getType()); + assertEquals("sources", result.getClassifier()); + assertNull(result.getVersion()); + assertNull(result.getId()); + } + @Test void testExpandDependencyIdDoesNotOverrideExistingFields() { Dependency dep = Dependency.newBuilder(false) diff --git a/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelValidatorTest.java b/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelValidatorTest.java index 3514ee326fb0..f9e649cafb21 100644 --- a/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelValidatorTest.java +++ b/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelValidatorTest.java @@ -1023,6 +1023,12 @@ void profileActivationConditionWithBasedirExpression() throws Exception { assertViolations(result, 0, 0, 0); } + @Test + void testDependencyId2partsValid() throws Exception { + SimpleProblemCollector result = validateFile("dependency-id-2parts-valid.xml"); + assertViolations(result, 0, 0, 0); + } + @Test void testDependencyId3partsValid() throws Exception { SimpleProblemCollector result = validateFile("dependency-id-3parts-valid.xml"); @@ -1041,6 +1047,24 @@ void testDependencyId5partsValid() throws Exception { assertViolations(result, 0, 0, 0); } + @Test + void testDependencyIdTrailingColon3partsValid() throws Exception { + SimpleProblemCollector result = validateFile("dependency-id-trailing-colon-3parts-valid.xml"); + assertViolations(result, 0, 0, 0); + } + + @Test + void testDependencyIdTrailingColon4partsValid() throws Exception { + SimpleProblemCollector result = validateFile("dependency-id-trailing-colon-4parts-valid.xml"); + assertViolations(result, 0, 0, 0); + } + + @Test + void testDependencyIdTrailingColon5partsValid() throws Exception { + SimpleProblemCollector result = validateFile("dependency-id-trailing-colon-5parts-valid.xml"); + assertViolations(result, 0, 0, 0); + } + @Test void testDependencyIdBadFormat() throws Exception { SimpleProblemCollector result = validateFile("dependency-id-bad-format.xml"); diff --git a/impl/maven-impl/src/test/resources/poms/validation/dependency-id-2parts-valid.xml b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-2parts-valid.xml new file mode 100644 index 000000000000..3702648630b7 --- /dev/null +++ b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-2parts-valid.xml @@ -0,0 +1,11 @@ + + 4.2.0 + test + test + 1.0 + + + + diff --git a/impl/maven-impl/src/test/resources/poms/validation/dependency-id-trailing-colon-3parts-valid.xml b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-trailing-colon-3parts-valid.xml new file mode 100644 index 000000000000..a76ebe1741ac --- /dev/null +++ b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-trailing-colon-3parts-valid.xml @@ -0,0 +1,11 @@ + + 4.2.0 + test + test + 1.0 + + + + diff --git a/impl/maven-impl/src/test/resources/poms/validation/dependency-id-trailing-colon-4parts-valid.xml b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-trailing-colon-4parts-valid.xml new file mode 100644 index 000000000000..731c4d5c431d --- /dev/null +++ b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-trailing-colon-4parts-valid.xml @@ -0,0 +1,11 @@ + + 4.2.0 + test + test + 1.0 + + + + diff --git a/impl/maven-impl/src/test/resources/poms/validation/dependency-id-trailing-colon-5parts-valid.xml b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-trailing-colon-5parts-valid.xml new file mode 100644 index 000000000000..e169e92fa980 --- /dev/null +++ b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-trailing-colon-5parts-valid.xml @@ -0,0 +1,11 @@ + + 4.2.0 + test + test + 1.0 + + + + From c08e34f1bebc0b6be6113e6531b3125fb83a6a31 Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Wed, 24 Jun 2026 07:48:15 +0200 Subject: [PATCH 07/11] Add scope and optional support to dependency id attribute Extend the compact id attribute format from groupId:artifactId:version to groupId:artifactId:version[@scope][?], where @scope sets the dependency scope and trailing ? marks the dependency as optional. Examples: - org.junit:junit-jupiter-api:5.0@test - commons-io:commons-io:2.11.0? - org.apache.maven:maven-core:3.9.0@provided? Co-Authored-By: Claude Opus 4.6 --- api/maven-api-model/src/main/mdo/maven.mdo | 8 +- .../impl/model/DefaultModelNormalizer.java | 25 ++++- .../impl/model/DefaultModelValidator.java | 70 ++++++++++++-- .../model/DefaultModelNormalizerTest.java | 93 +++++++++++++++++++ .../impl/model/DefaultModelValidatorTest.java | 36 +++++++ .../dependency-id-optional-conflict.xml | 12 +++ .../dependency-id-scope-conflict.xml | 12 +++ .../dependency-id-with-optional-valid.xml | 10 ++ ...ependency-id-with-scope-optional-valid.xml | 10 ++ .../dependency-id-with-scope-valid.xml | 10 ++ 10 files changed, 275 insertions(+), 11 deletions(-) create mode 100644 impl/maven-impl/src/test/resources/poms/validation/dependency-id-optional-conflict.xml create mode 100644 impl/maven-impl/src/test/resources/poms/validation/dependency-id-scope-conflict.xml create mode 100644 impl/maven-impl/src/test/resources/poms/validation/dependency-id-with-optional-valid.xml create mode 100644 impl/maven-impl/src/test/resources/poms/validation/dependency-id-with-scope-optional-valid.xml create mode 100644 impl/maven-impl/src/test/resources/poms/validation/dependency-id-with-scope-valid.xml diff --git a/api/maven-api-model/src/main/mdo/maven.mdo b/api/maven-api-model/src/main/mdo/maven.mdo index 7d1d2cd2f555..31633e7c23bb 100644 --- a/api/maven-api-model/src/main/mdo/maven.mdo +++ b/api/maven-api-model/src/main/mdo/maven.mdo @@ -1210,10 +1210,14 @@ 4.2.0+ diff --git a/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelNormalizer.java b/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelNormalizer.java index 2bd72cd8446c..d796743d818c 100644 --- a/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelNormalizer.java +++ b/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelNormalizer.java @@ -203,7 +203,7 @@ private Profile expandProfileDependencyIds(Profile profile) { /** * Expands the {@code id} attribute on a dependency into its component fields. - * The id format is {@code groupId:artifactId:version}. + * The id format is {@code groupId:artifactId:version[@scope][?]}. */ Dependency expandDependencyId(Dependency d) { String id = d.getId(); @@ -212,7 +212,22 @@ Dependency expandDependencyId(Dependency d) { List expanded = injectList(d.getExclusions(), this::expandExclusionId); return expanded != null ? d.withExclusions(expanded) : d; } - String[] parts = id.split(":"); + + String remaining = id; + boolean optional = false; + if (remaining.endsWith("?")) { + optional = true; + remaining = remaining.substring(0, remaining.length() - 1); + } + + String scope = null; + int atIndex = remaining.lastIndexOf('@'); + if (atIndex >= 0) { + scope = remaining.substring(atIndex + 1); + remaining = remaining.substring(0, atIndex); + } + + String[] parts = remaining.split(":"); if (parts.length != 3) { // Invalid format — will be caught by the validator return d; @@ -227,6 +242,12 @@ Dependency expandDependencyId(Dependency d) { if (isNullOrEmpty(d.getVersion())) { builder.version(parts[2]); } + if (scope != null && isNullOrEmpty(d.getScope())) { + builder.scope(scope); + } + if (optional && isNullOrEmpty(d.getOptional())) { + builder.optional("true"); + } List expanded = injectList(d.getExclusions(), this::expandExclusionId); if (expanded != null) { builder.exclusions(expanded); diff --git a/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java b/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java index 0f8bf031b794..53ee8e5e949f 100644 --- a/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java +++ b/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java @@ -1190,11 +1190,11 @@ private void validate20RawDependencies( Map index = new HashMap<>(); for (Dependency dependency : dependencies) { - // When 'id' attribute is set, use it for the key since groupId/artifactId - // have not yet been expanded (normalization runs after validation) + // When 'id' attribute is set, extract the GAV portion (stripping @scope and ?) + // for deduplication, since normalization has not yet run String key; if (dependency.getId() != null && !dependency.getId().isEmpty()) { - key = dependency.getId(); + key = stripScopeAndOptional(dependency.getId()); } else { key = dependency.getManagementKey(); } @@ -1324,15 +1324,31 @@ private void validate20RawDependencies( /** * Validates the {@code id} attribute on a dependency element. - * The id must have the format {@code groupId:artifactId:version} and must not - * be specified alongside individual groupId, artifactId, or version child elements. + * The id must have the format {@code groupId:artifactId:version[@scope][?]} and must not + * be specified alongside individual groupId, artifactId, version, scope, or optional child elements + * when those values are encoded in the id. */ private void validateDependencyIdAttribute(ModelProblemCollector problems, Dependency dependency, String prefix) { String id = dependency.getId(); if (id == null || id.isEmpty()) { return; } - String[] parts = id.split(":"); + + String remaining = id; + boolean hasOptionalMarker = false; + if (remaining.endsWith("?")) { + hasOptionalMarker = true; + remaining = remaining.substring(0, remaining.length() - 1); + } + + boolean hasScopeMarker = false; + int atIndex = remaining.lastIndexOf('@'); + if (atIndex >= 0) { + hasScopeMarker = true; + remaining = remaining.substring(0, atIndex); + } + + String[] parts = remaining.split(":"); if (parts.length != 3) { addViolation( problems, @@ -1340,7 +1356,7 @@ private void validateDependencyIdAttribute(ModelProblemCollector problems, Depen Version.V42, prefix + "id", null, - "must have the format 'groupId:artifactId:version' but is '" + id + "'.", + "must have the format 'groupId:artifactId:version[@scope][?]' but is '" + id + "'.", dependency); } if (dependency.getGroupId() != null && !dependency.getGroupId().isEmpty()) { @@ -1373,6 +1389,46 @@ private void validateDependencyIdAttribute(ModelProblemCollector problems, Depen "must not be specified when the 'id' attribute is used.", dependency); } + if (hasScopeMarker + && dependency.getScope() != null + && !dependency.getScope().isEmpty()) { + addViolation( + problems, + Severity.ERROR, + Version.V42, + prefix + "scope", + null, + "must not be specified when the 'id' attribute contains '@scope'.", + dependency); + } + if (hasOptionalMarker + && dependency.getOptional() != null + && !dependency.getOptional().isEmpty()) { + addViolation( + problems, + Severity.ERROR, + Version.V42, + prefix + "optional", + null, + "must not be specified when the 'id' attribute contains '?'.", + dependency); + } + } + + /** + * Strips the optional {@code @scope} and {@code ?} suffixes from a dependency id string, + * returning just the {@code groupId:artifactId:version} portion. + */ + private static String stripScopeAndOptional(String id) { + String remaining = id; + if (remaining.endsWith("?")) { + remaining = remaining.substring(0, remaining.length() - 1); + } + int atIndex = remaining.lastIndexOf('@'); + if (atIndex >= 0) { + remaining = remaining.substring(0, atIndex); + } + return remaining; } /** diff --git a/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelNormalizerTest.java b/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelNormalizerTest.java index e64c11bfdaf4..3cca76c76887 100644 --- a/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelNormalizerTest.java +++ b/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelNormalizerTest.java @@ -168,6 +168,99 @@ void testExpandDependencyIdAlsoExpandsExclusions() { assertEquals("unwanted", result.getExclusions().get(0).getArtifactId()); } + @Test + void testExpandDependencyIdWithScope() { + Dependency dep = Dependency.newBuilder() + .id("org.junit.jupiter:junit-jupiter-api:5.0@test") + .build(); + + Dependency result = normalizer.expandDependencyId(dep); + + assertEquals("org.junit.jupiter", result.getGroupId()); + assertEquals("junit-jupiter-api", result.getArtifactId()); + assertEquals("5.0", result.getVersion()); + assertEquals("test", result.getScope()); + } + + @Test + void testExpandDependencyIdWithOptional() { + Dependency dep = + Dependency.newBuilder().id("commons-io:commons-io:2.11.0?").build(); + + Dependency result = normalizer.expandDependencyId(dep); + + assertEquals("commons-io", result.getGroupId()); + assertEquals("commons-io", result.getArtifactId()); + assertEquals("2.11.0", result.getVersion()); + assertEquals("true", result.getOptional()); + } + + @Test + void testExpandDependencyIdWithScopeAndOptional() { + Dependency dep = Dependency.newBuilder() + .id("org.apache.maven:maven-core:3.9.0@provided?") + .build(); + + Dependency result = normalizer.expandDependencyId(dep); + + assertEquals("org.apache.maven", result.getGroupId()); + assertEquals("maven-core", result.getArtifactId()); + assertEquals("3.9.0", result.getVersion()); + assertEquals("provided", result.getScope()); + assertEquals("true", result.getOptional()); + } + + @Test + void testExpandDependencyIdWithImportScope() { + Dependency dep = + Dependency.newBuilder().id("org.junit:junit-bom:5.12.0@import").build(); + + Dependency result = normalizer.expandDependencyId(dep); + + assertEquals("org.junit", result.getGroupId()); + assertEquals("junit-bom", result.getArtifactId()); + assertEquals("5.12.0", result.getVersion()); + assertEquals("import", result.getScope()); + } + + @Test + void testExpandDependencyIdScopeDoesNotOverrideExisting() { + Dependency dep = Dependency.newBuilder() + .id("org.junit.jupiter:junit-jupiter-api:5.0@test") + .scope("compile") + .build(); + + Dependency result = normalizer.expandDependencyId(dep); + + assertEquals("compile", result.getScope()); + } + + @Test + void testExpandDependencyIdOptionalDoesNotOverrideExisting() { + Dependency dep = Dependency.newBuilder() + .id("commons-io:commons-io:2.11.0?") + .optional("false") + .build(); + + Dependency result = normalizer.expandDependencyId(dep); + + assertEquals("false", result.getOptional()); + } + + @Test + void testExpandDependencyIdPlainGavNoScopeOrOptional() { + Dependency dep = + Dependency.newBuilder().id("org.slf4j:slf4j-api:2.0.17").build(); + + Dependency result = normalizer.expandDependencyId(dep); + + assertEquals("org.slf4j", result.getGroupId()); + assertEquals("slf4j-api", result.getArtifactId()); + assertEquals("2.0.17", result.getVersion()); + assertNull(result.getScope()); + assertNull(result.getOptional()); + } + @Test void testMergeDuplicatesExpandsDependencyManagement() { Dependency dep = diff --git a/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelValidatorTest.java b/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelValidatorTest.java index 671a23c6f829..2f3bfc5f2bfc 100644 --- a/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelValidatorTest.java +++ b/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelValidatorTest.java @@ -1126,4 +1126,40 @@ void testExclusionIdConflictWithMultiple() throws Exception { result.getErrors().stream().anyMatch(e -> e.contains("must not be specified when the 'id' attribute")), "Expected error about format. Errors: " + result.getErrors()); } + + @Test + void testValidDependencyIdWithScope() throws Exception { + SimpleProblemCollector result = validateFile("dependency-id-with-scope-valid.xml"); + assertViolations(result, 0, 0, 0); + } + + @Test + void testValidDependencyIdWithOptional() throws Exception { + SimpleProblemCollector result = validateFile("dependency-id-with-optional-valid.xml"); + assertViolations(result, 0, 0, 0); + } + + @Test + void testValidDependencyIdWithScopeAndOptional() throws Exception { + SimpleProblemCollector result = validateFile("dependency-id-with-scope-optional-valid.xml"); + assertViolations(result, 0, 0, 0); + } + + @Test + void testDependencyIdScopeConflict() throws Exception { + SimpleProblemCollector result = validateFile("dependency-id-scope-conflict.xml"); + assertTrue( + result.getErrors().stream() + .anyMatch(e -> e.contains("must not be specified when the 'id' attribute contains '@scope'")), + "Expected scope conflict error. Errors: " + result.getErrors()); + } + + @Test + void testDependencyIdOptionalConflict() throws Exception { + SimpleProblemCollector result = validateFile("dependency-id-optional-conflict.xml"); + assertTrue( + result.getErrors().stream() + .anyMatch(e -> e.contains("must not be specified when the 'id' attribute contains '?'")), + "Expected optional conflict error. Errors: " + result.getErrors()); + } } diff --git a/impl/maven-impl/src/test/resources/poms/validation/dependency-id-optional-conflict.xml b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-optional-conflict.xml new file mode 100644 index 000000000000..b1bd0158b6a3 --- /dev/null +++ b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-optional-conflict.xml @@ -0,0 +1,12 @@ + + 4.2.0 + aid + gid + 0.1 + + + + true + + + diff --git a/impl/maven-impl/src/test/resources/poms/validation/dependency-id-scope-conflict.xml b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-scope-conflict.xml new file mode 100644 index 000000000000..3fbea6c1213a --- /dev/null +++ b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-scope-conflict.xml @@ -0,0 +1,12 @@ + + 4.2.0 + aid + gid + 0.1 + + + + test + + + diff --git a/impl/maven-impl/src/test/resources/poms/validation/dependency-id-with-optional-valid.xml b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-with-optional-valid.xml new file mode 100644 index 000000000000..85cae9f857c4 --- /dev/null +++ b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-with-optional-valid.xml @@ -0,0 +1,10 @@ + + 4.2.0 + aid + gid + 0.1 + + + + + diff --git a/impl/maven-impl/src/test/resources/poms/validation/dependency-id-with-scope-optional-valid.xml b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-with-scope-optional-valid.xml new file mode 100644 index 000000000000..bd48b58a4635 --- /dev/null +++ b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-with-scope-optional-valid.xml @@ -0,0 +1,10 @@ + + 4.2.0 + aid + gid + 0.1 + + + + + diff --git a/impl/maven-impl/src/test/resources/poms/validation/dependency-id-with-scope-valid.xml b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-with-scope-valid.xml new file mode 100644 index 000000000000..0a2479544407 --- /dev/null +++ b/impl/maven-impl/src/test/resources/poms/validation/dependency-id-with-scope-valid.xml @@ -0,0 +1,10 @@ + + 4.2.0 + aid + gid + 0.1 + + + + + From fdf544a9b56ffc4f76e6998e006d09098f1a599f Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Wed, 24 Jun 2026 08:18:42 +0200 Subject: [PATCH 08/11] Support empty segments in dependency id for inference Empty segments in the compact id attribute are now skipped rather than setting fields to empty strings, allowing groupId/version/type to be left unset for later inference from dependencyManagement. Co-Authored-By: Claude Opus 4.6 --- api/maven-api-model/src/main/mdo/maven.mdo | 11 +- .../impl/model/DefaultModelNormalizer.java | 10 +- .../model/DefaultModelNormalizerTest.java | 117 ++++++++++++++++++ 3 files changed, 131 insertions(+), 7 deletions(-) diff --git a/api/maven-api-model/src/main/mdo/maven.mdo b/api/maven-api-model/src/main/mdo/maven.mdo index f8409f022d9b..44ba0c98fece 100644 --- a/api/maven-api-model/src/main/mdo/maven.mdo +++ b/api/maven-api-model/src/main/mdo/maven.mdo @@ -1210,18 +1210,25 @@ 4.2.0+ Date: Wed, 24 Jun 2026 08:22:10 +0200 Subject: [PATCH 09/11] Fix id attribute documentation to use correct coordinate order The formal syntax incorrectly showed version before type. Maven consistently uses groupId:artifactId:type:classifier:version order (as in Artifact.key() and Dependency.getManagementKey()). Co-Authored-By: Claude Opus 4.6 --- api/maven-api-model/src/main/mdo/maven.mdo | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/api/maven-api-model/src/main/mdo/maven.mdo b/api/maven-api-model/src/main/mdo/maven.mdo index 44ba0c98fece..35532feb26fa 100644 --- a/api/maven-api-model/src/main/mdo/maven.mdo +++ b/api/maven-api-model/src/main/mdo/maven.mdo @@ -1210,14 +1210,14 @@ 4.2.0+ Date: Wed, 24 Jun 2026 08:23:45 +0200 Subject: [PATCH 10/11] Add full formal syntax to id attribute documentation Co-Authored-By: Claude Opus 4.6 --- api/maven-api-model/src/main/mdo/maven.mdo | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/api/maven-api-model/src/main/mdo/maven.mdo b/api/maven-api-model/src/main/mdo/maven.mdo index 35532feb26fa..cd5d679e586e 100644 --- a/api/maven-api-model/src/main/mdo/maven.mdo +++ b/api/maven-api-model/src/main/mdo/maven.mdo @@ -1210,10 +1210,9 @@ 4.2.0+ Date: Wed, 24 Jun 2026 08:50:36 +0200 Subject: [PATCH 11/11] Clean up review findings in DependencyIdStrategy and DefaultModelValidator - Remove redundant model version guard in DependencyIdStrategy (the second check alone suffices) - Simplify redundant type removal branches in collapseDependency() - Remove redundant validateExclusionIdAttribute call from effective model validation (id is already cleared by normalizer before effective validation) Co-Authored-By: Claude Opus 4.6 --- .../cling/invoker/mvnup/goals/DependencyIdStrategy.java | 6 ------ .../org/apache/maven/impl/model/DefaultModelValidator.java | 1 - 2 files changed, 7 deletions(-) diff --git a/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/DependencyIdStrategy.java b/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/DependencyIdStrategy.java index 5efccfaec3f1..0182712c2ec9 100644 --- a/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/DependencyIdStrategy.java +++ b/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/DependencyIdStrategy.java @@ -118,10 +118,6 @@ protected UpgradeResult doApply(UpgradeContext context, Map pomM context.indent(); try { - if (!MODEL_VERSION_4_2_0.equals(currentVersion) && !ModelVersionUtils.isNewerThan410(currentVersion)) { - context.success("Skipping (model version " + currentVersion + " < 4.2.0)"); - continue; - } if (!MODEL_VERSION_4_2_0.equals(currentVersion)) { context.success("Skipping (model version " + currentVersion + " is not 4.2.0)"); continue; @@ -232,8 +228,6 @@ private boolean collapseDependency(UpgradeContext context, Element dependency) { if (classifier != null) { removeChildElement(dependency, CLASSIFIER); removeChildElement(dependency, TYPE); - } else if (type != null && !DEFAULT_TYPE.equals(type)) { - removeChildElement(dependency, TYPE); } else if (type != null) { removeChildElement(dependency, TYPE); } diff --git a/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java b/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java index 4c91a6d9d7d3..9d479de55fef 100644 --- a/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java +++ b/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java @@ -1736,7 +1736,6 @@ private void validateEffectiveDependency( if (validationLevel >= ModelValidator.VALIDATION_LEVEL_MAVEN_2_0) { for (Exclusion exclusion : dependency.getExclusions()) { - validateExclusionIdAttribute(problems, exclusion, prefix); if (validationLevel < ModelValidator.VALIDATION_LEVEL_MAVEN_3_0) { validateCoordinatesId( prefix,