gnodet-bot commented on code in PR #10952:
URL: https://github.com/apache/maven/pull/10952#discussion_r4408677134
##########
impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/ModelVersionUtilsTest.java:
##########
@@ -415,7 +381,7 @@ void shouldHandleMissingModelVersion() throws Exception {
@ValueSource(
strings = {
"http://maven.apache.org/POM/4.0.0";,
- "http://maven.apache.org/POM/4.1.0";,
+ "http://maven.apache.org/POM/4.0.0";,
Review Comment:
⚠️ **Duplicate test entry (still unaddressed):** After replacing
`"http://maven.apache.org/POM/4.1.0"` with
`"http://maven.apache.org/POM/4.0.0"`, the `@ValueSource` now has
`"http://maven.apache.org/POM/4.0.0"` twice (lines 383 and 384). This removes
the test coverage for 4.1.0 as a valid namespace, and runs the same 4.0.0 case
twice — which provides no additional confidence.
Replace this duplicate with a test for the 4.1.0 namespace (which should now
also validate to `4.0.0` under the new model, if that's the intent), or remove
the duplicate entry.
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/UpgradeConstants.java:
##########
@@ -0,0 +1,234 @@
+/*
+ * 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;
+
+/**
+ * Constants used throughout the Maven upgrade tools.
+ * Organized into logical groups for better maintainability.
+ */
+public final class UpgradeConstants {
Review Comment:
⚠️ **Dead code (still unaddressed):** This 234-line class is not imported by
any other file in this PR. `ModelUpgradeStrategy` was updated to import
`MavenPomElements.Namespaces` directly (not `UpgradeConstants.Namespaces`).
`ModelVersionUtils` inlines its schema location strings. All constants defined
here (`ModelVersions`, `XmlElements`, `Indentation`, `Plugins`, `Files`,
`Namespaces`, `SchemaLocations`, `XmlAttributes`) duplicate what's already in
`MavenPomElements` or are unused.
Remove this file.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultModelXmlFactory.java:
##########
@@ -63,7 +63,7 @@ public Model read(@Nonnull XmlReaderRequest request) throws
XmlReaderException {
requireNonNull(request, "request");
Model model = doRead(request);
if (isModelVersionGreaterThan400(model)
- &&
!model.getNamespaceUri().startsWith("http://maven.apache.org/POM/";)) {
+ &&
!model.getNamespaceUri().equals("http://maven.apache.org/POM/4.0.0";)) {
Review Comment:
🔴 **Compatibility regression:** Changing
`startsWith("http://maven.apache.org/POM/";)` to
`equals("http://maven.apache.org/POM/4.0.0";)` means any POM with `modelVersion`
> 4.0.0 that still uses a version-specific namespace
(`xmlns="http://maven.apache.org/POM/4.1.0"`) will now throw
`XmlReaderException: Invalid namespace '...'`.
The test for this case (`testValidNamespaceWithModelVersion410`) was updated
to use the 4.0.0 namespace in the test input itself — which means it no longer
tests the real-world scenario of a 4.1.0 POM with its pre-existing namespace.
Any user who has a POM with `xmlns="http://maven.apache.org/POM/4.1.0"` and
builds against this Maven version will get a hard parse failure.
If the intent is that all POMs must migrate their namespace to 4.0.0 before
this Maven version can read them, document this migration requirement
explicitly in the error message. If the intent is forward-compatibility, use
`startsWith` or accept any valid Maven namespace.
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/UpgradeConstants.java:
##########
@@ -0,0 +1,234 @@
+/*
+ * 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;
+
+/**
+ * Constants used throughout the Maven upgrade tools.
+ * Organized into logical groups for better maintainability.
+ */
+public final class UpgradeConstants {
+
+ private UpgradeConstants() {
+ // Utility class
+ }
+
+ /**
+ * Maven model version constants.
+ */
+ public static final class ModelVersions {
+ /** Maven 4.0.0 model version */
+ public static final String MODEL_VERSION_4_0_0 = "4.0.0";
+
+ /** Maven 4.1.0 model version */
+ public static final String MODEL_VERSION_4_1_0 = "4.1.0";
+
+ private ModelVersions() {
+ // Utility class
+ }
+ }
+
+ /**
+ * Common XML element names used in Maven POMs.
+ */
+ public static final class XmlElements {
+ // Core POM elements
+ public static final String MODEL_VERSION = "modelVersion";
+ public static final String GROUP_ID = "groupId";
+ public static final String ARTIFACT_ID = "artifactId";
+ public static final String VERSION = "version";
+ public static final String PARENT = "parent";
+ public static final String RELATIVE_PATH = "relativePath";
+ public static final String PACKAGING = "packaging";
+ public static final String NAME = "name";
+ public static final String DESCRIPTION = "description";
+ public static final String URL = "url";
+
+ // Build elements
+ public static final String BUILD = "build";
+ public static final String PLUGINS = "plugins";
+ public static final String PLUGIN = "plugin";
+ public static final String PLUGIN_MANAGEMENT = "pluginManagement";
+ public static final String DEFAULT_GOAL = "defaultGoal";
+ public static final String DIRECTORY = "directory";
+ public static final String FINAL_NAME = "finalName";
+ public static final String SOURCE_DIRECTORY = "sourceDirectory";
+ public static final String SCRIPT_SOURCE_DIRECTORY =
"scriptSourceDirectory";
+ public static final String TEST_SOURCE_DIRECTORY =
"testSourceDirectory";
+ public static final String OUTPUT_DIRECTORY = "outputDirectory";
+ public static final String TEST_OUTPUT_DIRECTORY =
"testOutputDirectory";
+ public static final String EXTENSIONS = "extensions";
+ public static final String EXECUTIONS = "executions";
+ public static final String GOALS = "goals";
+ public static final String INHERITED = "inherited";
+ public static final String CONFIGURATION = "configuration";
+
+ // Module elements
+ public static final String MODULES = "modules";
+ public static final String MODULE = "module";
+ public static final String SUBPROJECTS = "subprojects";
+ public static final String SUBPROJECT = "subproject";
+
+ // Dependency elements
+ public static final String DEPENDENCIES = "dependencies";
+ public static final String DEPENDENCY = "dependency";
+ public static final String DEPENDENCY_MANAGEMENT =
"dependencyManagement";
+ public static final String CLASSIFIER = "classifier";
+ public static final String TYPE = "type";
+ public static final String SCOPE = "scope";
+ public static final String SYSTEM_PATH = "systemPath";
+ public static final String OPTIONAL = "optional";
+ public static final String EXCLUSIONS = "exclusions";
+
+ // Profile elements
+ public static final String PROFILES = "profiles";
+ public static final String PROFILE = "profile";
+
+ // Project information elements
+ public static final String PROPERTIES = "properties";
+ public static final String INCEPTION_YEAR = "inceptionYear";
+ public static final String ORGANIZATION = "organization";
+ public static final String LICENSES = "licenses";
+ public static final String DEVELOPERS = "developers";
+ public static final String CONTRIBUTORS = "contributors";
+ public static final String MAILING_LISTS = "mailingLists";
+ public static final String PREREQUISITES = "prerequisites";
+ public static final String SCM = "scm";
+ public static final String ISSUE_MANAGEMENT = "issueManagement";
+ public static final String CI_MANAGEMENT = "ciManagement";
+ public static final String DISTRIBUTION_MANAGEMENT =
"distributionManagement";
+ public static final String REPOSITORIES = "repositories";
+ public static final String PLUGIN_REPOSITORIES = "pluginRepositories";
+ public static final String REPOSITORY = "repository";
+ public static final String PLUGIN_REPOSITORY = "pluginRepository";
+ public static final String REPORTING = "reporting";
+
+ private XmlElements() {
+ // Utility class
+ }
+ }
+
+ /**
+ * Common indentation patterns for XML formatting.
+ */
+ public static final class Indentation {
+ public static final String TWO_SPACES = " ";
+ public static final String FOUR_SPACES = " ";
+ public static final String TAB = "\t";
+ public static final String DEFAULT = TWO_SPACES;
+
+ private Indentation() {
+ // Utility class
+ }
+ }
+
+ /**
+ * Common Maven plugin constants.
+ */
+ public static final class Plugins {
+ /** Default Maven plugin groupId */
+ public static final String DEFAULT_MAVEN_PLUGIN_GROUP_ID =
"org.apache.maven.plugins";
+
+ /** Maven plugin artifact prefix */
+ public static final String MAVEN_PLUGIN_PREFIX = "maven-";
+
+ /** Standard reason for Maven 4 compatibility upgrades */
+ public static final String MAVEN_4_COMPATIBILITY_REASON = "Maven 4
compatibility";
+
+ private Plugins() {
+ // Utility class
+ }
+ }
+
+ /**
+ * Common file and directory names.
+ */
+ public static final class Files {
+ /** Standard Maven POM file name */
+ public static final String POM_XML = "pom.xml";
+
+ /** Maven configuration directory (alternative name) */
+ public static final String MVN_DIRECTORY = ".mvn";
+
+ /** Default parent POM relative path */
+ public static final String DEFAULT_PARENT_RELATIVE_PATH = "../pom.xml";
+
+ private Files() {
+ // Utility class
+ }
+ }
+
+ /**
+ * Maven namespace constants.
+ */
+ public static final class Namespaces {
+ /** Maven namespace URI */
+ public static final String MAVEN_4_0_0_NAMESPACE =
"http://maven.apache.org/POM/4.0.0";;
+
+ /** Maven namespace URI */
+ public static final String MAVEN_4_1_0_NAMESPACE =
"http://maven.apache.org/POM/4.0.0";;
Review Comment:
🔴 **Wrong constant value (new issue):** `MAVEN_4_1_0_NAMESPACE` is defined
as `"http://maven.apache.org/POM/4.0.0"` — identical to
`MAVEN_4_0_0_NAMESPACE`. The constant name says 4.1.0 but the value is 4.0.0.
This is either a copy-paste error or an intentional conflation — either way,
the constant name is actively misleading. Any code using
`MAVEN_4_1_0_NAMESPACE` expecting a 4.1.0 URI would silently get the 4.0.0 URI
instead.
This class should be removed (see the dead code finding above). If it's
kept, correct the constant name or value.
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/ModelUpgradeStrategy.java:
##########
@@ -181,7 +180,7 @@ private void upgradeNamespaceAndSchemaLocation(Editor
editor, UpgradeContext con
}
// Update namespace based on target model version
- String targetNamespace =
getNamespaceForModelVersion(targetModelVersion);
+ String targetNamespace = Namespaces.MAVEN_4_0_0_NAMESPACE;
Review Comment:
📝 **Stale comment (still unaddressed):** The comment says `// Update
namespace based on target model version` but the code directly below hardcodes
`Namespaces.MAVEN_4_0_0_NAMESPACE` regardless of `targetModelVersion`. The
comment is wrong.
##########
its/core-it-suite/src/test/java/org/apache/maven/it/MavenITmng7228LeakyModelTest.java:
##########
@@ -33,6 +34,7 @@ protected MavenITmng7228LeakyModelTest() {
super();
}
Review Comment:
⚠️ **Avoids root cause (still unaddressed):** `testLeakyModel` is disabled
citing `maven-shade-plugin` as the cause, but this test verifies that Maven
itself does not produce a consumer POM with a namespace that breaks downstream
tools. The shade plugin failure is a symptom — Maven's consumer POM writer is
producing POMs with inconsistent namespaces. Disabling this IT hides a real
Maven regression.
Options:
1. Fix the consumer POM writer to always emit the 4.0.0 namespace (the
intent of this PR)
2. If the shade plugin genuinely can't handle the new namespace yet, track
it as a known limitation with a link to the upstream bug and a TODO to
re-enable when shade is fixed
3. Restructure the IT to avoid using shade plugin, exercising the namespace
leakage through a different path
Leaving this disabled without a tracking issue or re-enable condition is not
acceptable.
##########
impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/UpgradeWorkflowIntegrationTest.java:
##########
@@ -88,7 +88,7 @@ void shouldUpgradeModelVersionWith41Option() throws Exception
{
// Verify POM was upgraded
String upgradedPom = Files.readString(pomFile);
assertTrue(
- upgradedPom.contains("http://maven.apache.org/POM/4.1.0";),
+ upgradedPom.contains("http://maven.apache.org/POM/4.0.0";),
Review Comment:
📝 **Stale assertion message (new issue):** The assertion message still says
`"POM should be upgraded to 4.1.0 namespace"` but the assertion now checks for
`contains("http://maven.apache.org/POM/4.0.0";)`. When this assertion fails, the
message will mislead the developer.
--
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]
