Copilot commented on code in PR #11904:
URL: https://github.com/apache/maven/pull/11904#discussion_r4329179117
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java:
##########
@@ -1190,7 +1239,22 @@ private void validate20RawDependencies(
Map<String, Dependency> index = new HashMap<>();
for (Dependency dependency : dependencies) {
- String key = dependency.getManagementKey();
+ // 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 = stripScopeAndOptional(dependency.getId());
+ } else {
+ key = dependency.getManagementKey();
+ }
Review Comment:
Using `stripScopeAndOptional(id)` as the deduplication key changes semantics
compared to `dependency.getManagementKey()` because it retains the version
portion (and any other id-encoded data) instead of computing the management key
fields (typically excluding version). This can cause duplicates that should be
detected/merged (same management key but different versions) to be missed for
`id`-based dependencies. Fix by deriving a management-key-equivalent from the
`id` (strip `@scope` / `?`, split into parts, then build the same key shape as
`getManagementKey()` would—e.g., groupId/artifactId/type/classifier with
default type handling—instead of using the raw id string).
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/DependencyIdStrategy.java:
##########
@@ -0,0 +1,302 @@
+/*
+ * 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.
+ *
+ * <p>Transforms verbose dependency declarations:
+ * <pre>{@code
+ * <dependency>
+ * <groupId>org.example</groupId>
+ * <artifactId>lib</artifactId>
+ * <version>1.0</version>
+ * </dependency>
+ * }</pre>
+ * into compact form:
+ * <pre>{@code
+ * <dependency id="org.example:lib:1.0"/>
+ * }</pre>
+ *
+ * <p>Supported formats (trailing {@code :} means version is managed):
+ * <ul>
+ * <li>{@code g:a} — version managed</li>
+ * <li>{@code g:a:v} — default type "jar"</li>
+ * <li>{@code g:a:type:} — non-default type, version managed</li>
+ * <li>{@code g:a:type:v} — non-default type</li>
+ * <li>{@code g:a:type:classifier:} — with classifier, version managed</li>
+ * <li>{@code g:a:type:classifier:v} — with classifier</li>
+ * </ul>
+ *
+ * <p>Also collapses exclusions from {@code <groupId>}/{@code <artifactId>}
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) {
Review Comment:
The class-level Javadoc says this strategy applies to “model version 4.2.0
or higher”, but `isApplicable` currently checks equality with `4.2.0` only.
Please either (a) update the Javadoc to say “only 4.2.0” or (b) change
applicability logic to treat versions >= 4.2.0 as eligible (if the CLI has a
version comparison helper, using that would avoid string-based comparisons).
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/DependencyIdStrategy.java:
##########
@@ -0,0 +1,302 @@
+/*
+ * 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.
+ *
Review Comment:
The class-level Javadoc says this strategy applies to “model version 4.2.0
or higher”, but `isApplicable` currently checks equality with `4.2.0` only.
Please either (a) update the Javadoc to say “only 4.2.0” or (b) change
applicability logic to treat versions >= 4.2.0 as eligible (if the CLI has a
version comparison helper, using that would avoid string-based comparisons).
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java:
##########
@@ -1307,6 +1371,165 @@ private void validate20RawDependencies(
}
}
+ private void validateDependencyIdAttribute(ModelProblemCollector problems,
Dependency dependency, String prefix) {
+ String id = dependency.getId();
+ if (id == null || id.isEmpty()) {
+ return;
+ }
+
+ 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);
+ }
+
+ int colonCount = remaining.length() - remaining.replace(":",
"").length();
+ if (colonCount < 1 || colonCount > 4) {
+ addViolation(
+ problems,
+ Severity.ERROR,
+ Version.V42,
+ prefix + "id",
+ null,
+ "has invalid format '" + id + "', must be
'groupId:artifactId[:]', "
+ + "'groupId:artifactId:version', "
+ + "'groupId:artifactId:type:[version]', or "
+ + "'groupId:artifactId:type:classifier:[version]'.
"
+ + "An optional '@scope' suffix and/or trailing '?'
may be appended.",
+ 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);
+ }
Review Comment:
The conflict checks for `<type>` and `<classifier>` rely only on colon
count, which incorrectly flags cases where those *segments are present but
empty* (e.g. `org.example:lib::1.0` or `org.example:lib:jar::1.0`). Per the PR
description, empty segments mean “not encoded” and should not forbid specifying
that field via child elements. Fix by parsing the `id` into parts (e.g.,
`split(":", -1)` after stripping `@scope` / `?`) and only treating
`type`/`classifier` as encoded when the corresponding part exists *and is
non-empty* (and for type, only if the id is 4/5-part and `parts[2]` is
non-empty; for classifier, only if 5-part and `parts[3]` is non-empty).
##########
impl/maven-core/src/main/java/org/apache/maven/internal/transformation/impl/DefaultConsumerPomBuilder.java:
##########
@@ -800,6 +807,201 @@ private static void warnNotDowngraded(MavenProject
project) {
+ "attribute on the <project> element of your POM.");
}
+ /**
+ * Expands {@code id} attributes on {@code <dependency>} and {@code
<exclusion>}
+ * elements into their individual GAV fields, then clears the {@code id}
attribute.
+ * This mirrors the expansion performed by {@code DefaultModelNormalizer}
during
+ * model building and ensures the consumer POM does not contain
compact-form
+ * {@code id} attributes that downstream consumers (Maven 3, Gradle)
cannot parse.
+ *
+ * @param model the model to expand
+ * @return the model with all dependency/exclusion {@code id} attributes
expanded
+ */
+ static Model expandDependencyIds(Model model) {
+ Model.Builder mb = null;
+ List<Dependency> deps =
expandDependencyIdList(model.getDependencies());
+ if (deps != null) {
+ mb = Model.newBuilder(model, true);
+ mb.dependencies(deps);
+ }
Review Comment:
The consumer-POM expansion logic duplicates the expansion rules already
implemented in `DefaultModelNormalizer` (`expandDependencyId` /
`expandExclusionId`) with effectively the same parsing and field-setting
behavior. This duplication increases long-term maintenance risk (any future
change to id syntax, scope/optional handling, defaults, or inference rules must
be updated in two places). Consider extracting the parsing/expansion into a
shared utility (or reusing `DefaultModelNormalizer` as a dependency/service) so
both code paths stay consistent.
##########
impl/maven-core/src/main/java/org/apache/maven/internal/transformation/impl/DefaultConsumerPomBuilder.java:
##########
@@ -800,6 +807,201 @@ private static void warnNotDowngraded(MavenProject
project) {
+ "attribute on the <project> element of your POM.");
}
+ /**
+ * Expands {@code id} attributes on {@code <dependency>} and {@code
<exclusion>}
+ * elements into their individual GAV fields, then clears the {@code id}
attribute.
+ * This mirrors the expansion performed by {@code DefaultModelNormalizer}
during
+ * model building and ensures the consumer POM does not contain
compact-form
+ * {@code id} attributes that downstream consumers (Maven 3, Gradle)
cannot parse.
+ *
+ * @param model the model to expand
+ * @return the model with all dependency/exclusion {@code id} attributes
expanded
+ */
+ static Model expandDependencyIds(Model model) {
+ Model.Builder mb = null;
+ List<Dependency> deps =
expandDependencyIdList(model.getDependencies());
+ if (deps != null) {
+ mb = Model.newBuilder(model, true);
+ mb.dependencies(deps);
+ }
+ DependencyManagement mgmt = model.getDependencyManagement();
+ if (mgmt != null) {
+ List<Dependency> mgmtDeps =
expandDependencyIdList(mgmt.getDependencies());
+ if (mgmtDeps != null) {
+ if (mb == null) {
+ mb = Model.newBuilder(model, true);
+ }
+ mb.dependencyManagement(DependencyManagement.newBuilder(mgmt,
true)
+ .dependencies(mgmtDeps)
+ .build());
+ }
+ }
+ List<Profile> profiles = model.getProfiles();
+ if (!profiles.isEmpty()) {
+ List<Profile> expandedProfiles = null;
+ for (int i = 0; i < profiles.size(); i++) {
+ Profile profile = profiles.get(i);
+ Profile.Builder pb = null;
+ List<Dependency> pdeps =
expandDependencyIdList(profile.getDependencies());
+ if (pdeps != null) {
+ pb = Profile.newBuilder(profile, true);
+ pb.dependencies(pdeps);
+ }
+ DependencyManagement pmgmt = profile.getDependencyManagement();
+ if (pmgmt != null) {
+ List<Dependency> pmgmtDeps =
expandDependencyIdList(pmgmt.getDependencies());
+ if (pmgmtDeps != null) {
+ if (pb == null) {
+ pb = Profile.newBuilder(profile, true);
+ }
+
pb.dependencyManagement(DependencyManagement.newBuilder(pmgmt, true)
+ .dependencies(pmgmtDeps)
+ .build());
+ }
+ }
+ if (pb != null) {
+ if (expandedProfiles == null) {
+ expandedProfiles = new ArrayList<>(profiles);
+ }
+ expandedProfiles.set(i, pb.build());
+ }
+ }
+ if (expandedProfiles != null) {
+ if (mb == null) {
+ mb = Model.newBuilder(model, true);
+ }
+ mb.profiles(expandedProfiles);
+ }
+ }
+ return mb != null ? mb.build() : model;
+ }
+
+ private static List<Dependency> expandDependencyIdList(List<Dependency>
dependencies) {
+ List<Dependency> result = null;
+ for (int i = 0; i < dependencies.size(); i++) {
+ Dependency dep = dependencies.get(i);
+ Dependency expanded = expandSingleDependencyId(dep);
+ if (expanded != dep) {
+ if (result == null) {
+ result = new ArrayList<>(dependencies);
+ }
+ result.set(i, expanded);
+ }
+ }
+ return result;
+ }
+
+ private static Dependency expandSingleDependencyId(Dependency d) {
+ String id = d.getId();
+ if (id == null || id.isEmpty()) {
+ // Still need to check exclusions
+ List<Exclusion> expanded =
expandExclusionIdList(d.getExclusions());
+ return expanded != null ? d.withExclusions(expanded) : d;
+ }
Review Comment:
The consumer-POM expansion logic duplicates the expansion rules already
implemented in `DefaultModelNormalizer` (`expandDependencyId` /
`expandExclusionId`) with effectively the same parsing and field-setting
behavior. This duplication increases long-term maintenance risk (any future
change to id syntax, scope/optional handling, defaults, or inference rules must
be updated in two places). Consider extracting the parsing/expansion into a
shared utility (or reusing `DefaultModelNormalizer` as a dependency/service) so
both code paths stay consistent.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java:
##########
@@ -375,6 +376,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);
+ }
Review Comment:
Mixins currently only validate the number of `:` segments, but accept empty
segments such as `g::1.0` (artifactId empty) or `:a:1.0` (groupId empty), which
then expand into empty/invalid fields. If mixin `id`/`gav` is intended to be
strict `groupId:artifactId:version`, validation should also reject empty parts
(i.e., require all 3 segments to be non-empty).
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/DependencyIdStrategy.java:
##########
@@ -0,0 +1,302 @@
+/*
+ * 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.
+ *
+ * <p>Transforms verbose dependency declarations:
+ * <pre>{@code
+ * <dependency>
+ * <groupId>org.example</groupId>
+ * <artifactId>lib</artifactId>
+ * <version>1.0</version>
+ * </dependency>
+ * }</pre>
+ * into compact form:
+ * <pre>{@code
+ * <dependency id="org.example:lib:1.0"/>
+ * }</pre>
+ *
+ * <p>Supported formats (trailing {@code :} means version is managed):
+ * <ul>
+ * <li>{@code g:a} — version managed</li>
+ * <li>{@code g:a:v} — default type "jar"</li>
+ * <li>{@code g:a:type:} — non-default type, version managed</li>
+ * <li>{@code g:a:type:v} — non-default type</li>
+ * <li>{@code g:a:type:classifier:} — with classifier, version managed</li>
+ * <li>{@code g:a:type:classifier:v} — with classifier</li>
+ * </ul>
+ *
+ * <p>Also collapses exclusions from {@code <groupId>}/{@code <artifactId>}
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);
+ }
Review Comment:
The class-level Javadoc says this strategy applies to “model version 4.2.0
or higher”, but `isApplicable` currently checks equality with `4.2.0` only.
Please either (a) update the Javadoc to say “only 4.2.0” or (b) change
applicability logic to treat versions >= 4.2.0 as eligible (if the CLI has a
version comparison helper, using that would avoid string-based comparisons).
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java:
##########
@@ -1307,6 +1371,165 @@ private void validate20RawDependencies(
}
}
+ private void validateDependencyIdAttribute(ModelProblemCollector problems,
Dependency dependency, String prefix) {
+ String id = dependency.getId();
+ if (id == null || id.isEmpty()) {
+ return;
+ }
+
+ 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);
+ }
+
+ int colonCount = remaining.length() - remaining.replace(":",
"").length();
+ if (colonCount < 1 || colonCount > 4) {
+ addViolation(
+ problems,
+ Severity.ERROR,
+ Version.V42,
+ prefix + "id",
+ null,
+ "has invalid format '" + id + "', must be
'groupId:artifactId[:]', "
+ + "'groupId:artifactId:version', "
+ + "'groupId:artifactId:type:[version]', or "
+ + "'groupId:artifactId:type:classifier:[version]'.
"
+ + "An optional '@scope' suffix and/or trailing '?'
may be appended.",
+ 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);
+ }
+ if (hasScopeMarker && !isNullOrEmpty(dependency.getScope())) {
+ addViolation(
+ problems,
+ Severity.ERROR,
+ Version.V42,
+ prefix + "scope",
+ null,
+ "must not be specified when 'id' attribute contains
'@scope'.",
+ dependency);
+ }
+ if (hasOptionalMarker && !isNullOrEmpty(dependency.getOptional())) {
+ addViolation(
+ problems,
+ Severity.ERROR,
+ Version.V42,
+ prefix + "optional",
+ null,
+ "must not be specified when 'id' attribute contains '?'.",
+ dependency);
+ }
+ }
+
+ 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;
+ }
+
+ 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;
+ }
Review Comment:
The exclusion `id` validator only checks `colonCount == 1`, which allows
invalid values like `:artifactId`, `groupId:`, or `:` (empty segments). Since
exclusions are documented as `groupId:artifactId` (and tests already treat
“only-one-part” as invalid), the validator should also ensure both segments are
non-empty (or explicitly whitelist the wildcard case `*:*` if that’s intended
to be valid everywhere).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
