Skip to content

[MNG-8676] Improve model builder error messages #2257

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Apr 16, 2025
+79 −47
Merged

[MNG-8676] Improve model builder error messages #2257

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1fb8178
[MNG-8676] Improve model builder error messages
cstamas Apr 13, 2025
cf8efc4
Fix test
cstamas Apr 13, 2025
3190ae3
Fix IT
cstamas Apr 13, 2025
d152945
Merge remote-tracking branch 'upstream/master' into MNG-8676
cstamas Apr 14, 2025
9cfc174
Just change how we emit DMK to users
cstamas Apr 14, 2025
87af2d5
Fix IT as well
cstamas Apr 14, 2025
e3c6039
Do not user abbreviations, use model field name (XML tag name)
cstamas Apr 14, 2025
6215fd5
Reformat
cstamas Apr 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions impl/maven-core/src/test/java/org/apache/maven/project/ProjectBuilderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,9 @@ void testVersionlessManagedDependency() throws Exception {
.build(pomFile, configuration));
assertThat(
e.getResults(),
contains(projectBuildingResultWithProblemMessage(
"'dependencies.dependency.version' for org.apache.maven.its:a:jar is missing")));
contains(
projectBuildingResultWithProblemMessage(
"'dependencies.dependency.version' for groupId='org.apache.maven.its', artifactId='a', type='jar' is missing")));
assertThat(e.getResults(), contains(projectBuildingResultWithLocation(5, 9)));
}

Expand Down
22 changes: 20 additions & 2 deletions impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2241,11 +2241,29 @@ public static SourceHint xmlNodeInputLocation(XmlNode xmlNode) {
}

public static SourceHint gav(String gav) {
return new SourceHint(gav, null); // "GAV"
return new SourceHint(gav, null); // GAV
}

public static SourceHint dependencyManagementKey(Dependency dependency) {
return new SourceHint(dependency.getManagementKey(), null); // DMK
String hint;
if (dependency.getClassifier() == null
|| dependency.getClassifier().trim().isEmpty()) {
hint = String.format(
"groupId=%s, artifactId=%s, type=%s",
nvl(dependency.getGroupId()), nvl(dependency.getArtifactId()), nvl(dependency.getType()));
} else {
hint = String.format(
"groupId=%s, artifactId=%s, classifier=%s, type=%s",
nvl(dependency.getGroupId()),
nvl(dependency.getArtifactId()),
nvl(dependency.getClassifier()),
nvl(dependency.getType()));
}
return new SourceHint(hint, null); // DMK
}

private static String nvl(String value) {
return value == null ? "" : "'" + value + "'";
}

public static SourceHint pluginKey(Plugin plugin) {
Expand Down
96 changes: 54 additions & 42 deletions impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelValidatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,9 +196,11 @@ void testMissingDependencyArtifactId() throws Exception {

assertViolations(result, 0, 1, 0);

assertTrue(result.getErrors()
.get(0)
.contains("'dependencies.dependency.artifactId' for groupId:null:jar is missing"));
assertTrue(
result.getErrors()
.get(0)
.contains(
"'dependencies.dependency.artifactId' for groupId='groupId', artifactId=, type='jar' is missing"));
}

@Test
Expand All @@ -207,9 +209,11 @@ void testMissingDependencyGroupId() throws Exception {

assertViolations(result, 0, 1, 0);

assertTrue(result.getErrors()
.get(0)
.contains("'dependencies.dependency.groupId' for null:artifactId:jar is missing"));
assertTrue(
result.getErrors()
.get(0)
.contains(
"'dependencies.dependency.groupId' for groupId=, artifactId='artifactId', type='jar' is missing"));
}

@Test
Expand All @@ -218,9 +222,11 @@ void testMissingDependencyVersion() throws Exception {

assertViolations(result, 0, 1, 0);

assertTrue(result.getErrors()
.get(0)
.contains("'dependencies.dependency.version' for groupId:artifactId:jar is missing"));
assertTrue(
result.getErrors()
.get(0)
.contains(
"'dependencies.dependency.version' for groupId='groupId', artifactId='artifactId', type='jar' is missing"));
}

@Test
Expand All @@ -229,9 +235,11 @@ void testMissingDependencyManagementArtifactId() throws Exception {

assertViolations(result, 0, 1, 0);

assertTrue(result.getErrors()
.get(0)
.contains("'dependencyManagement.dependencies.dependency.artifactId' for groupId:null:jar is missing"));
assertTrue(
result.getErrors()
.get(0)
.contains(
"'dependencyManagement.dependencies.dependency.artifactId' for groupId='groupId', artifactId=, type='jar' is missing"));
}

@Test
Expand All @@ -240,9 +248,11 @@ void testMissingDependencyManagementGroupId() throws Exception {

assertViolations(result, 0, 1, 0);

assertTrue(result.getErrors()
.get(0)
.contains("'dependencyManagement.dependencies.dependency.groupId' for null:artifactId:jar is missing"));
assertTrue(
result.getErrors()
.get(0)
.contains(
"'dependencyManagement.dependencies.dependency.groupId' for groupId=, artifactId='artifactId', type='jar' is missing"));
}

@Test
Expand Down Expand Up @@ -327,11 +337,11 @@ void testBadPluginDependencyScope() throws Exception {

assertViolations(result, 0, 3, 0);

assertTrue(result.getErrors().get(0).contains("test:d"));
assertTrue(result.getErrors().get(0).contains("groupId='test', artifactId='d'"));

assertTrue(result.getErrors().get(1).contains("test:e"));
assertTrue(result.getErrors().get(1).contains("groupId='test', artifactId='e'"));

assertTrue(result.getErrors().get(2).contains("test:f"));
assertTrue(result.getErrors().get(2).contains("groupId='test', artifactId='f'"));
}

@Test
Expand All @@ -340,9 +350,9 @@ void testBadDependencyScope() throws Exception {

assertViolations(result, 0, 0, 2);

assertTrue(result.getWarnings().get(0).contains("test:f"));
assertTrue(result.getWarnings().get(0).contains("groupId='test', artifactId='f'"));

assertTrue(result.getWarnings().get(1).contains("test:g"));
assertTrue(result.getWarnings().get(1).contains("groupId='test', artifactId='g'"));
}

@Test
Expand All @@ -351,7 +361,7 @@ void testBadDependencyManagementScope() throws Exception {

assertViolations(result, 0, 0, 1);

assertContains(result.getWarnings().get(0), "test:g");
assertContains(result.getWarnings().get(0), "groupId='test', artifactId='g'");
}

@Test
Expand All @@ -361,10 +371,11 @@ void testBadDependencyVersion() throws Exception {
assertViolations(result, 0, 2, 0);

assertContains(
result.getErrors().get(0), "'dependencies.dependency.version' for test:b:jar must be a valid version");
result.getErrors().get(0),
"'dependencies.dependency.version' for groupId='test', artifactId='b', type='jar' must be a valid version");
assertContains(
result.getErrors().get(1),
"'dependencies.dependency.version' for test:c:jar must not contain any of these characters");
"'dependencies.dependency.version' for groupId='test', artifactId='c', type='jar' must not contain any of these characters");
}

@Test
Expand Down Expand Up @@ -441,13 +452,13 @@ void testHardCodedSystemPath() throws Exception {

assertContains(
result.getWarnings().get(0),
"'dependencies.dependency.scope' for test:a:jar declares usage of deprecated 'system' scope");
"'dependencies.dependency.scope' for groupId='test', artifactId='a', type='jar' declares usage of deprecated 'system' scope");
assertContains(
result.getWarnings().get(1),
"'dependencies.dependency.systemPath' for test:a:jar should use a variable instead of a hard-coded path");
"'dependencies.dependency.systemPath' for groupId='test', artifactId='a', type='jar' should use a variable instead of a hard-coded path");
assertContains(
result.getWarnings().get(2),
"'dependencies.dependency.scope' for test:b:jar declares usage of deprecated 'system' scope");
"'dependencies.dependency.scope' for groupId='test', artifactId='b', type='jar' declares usage of deprecated 'system' scope");
}

@Test
Expand Down Expand Up @@ -501,7 +512,7 @@ void testMissingPluginDependencyGroupId() throws Exception {

assertViolations(result, 0, 1, 0);

assertTrue(result.getErrors().get(0).contains(":a:"));
assertTrue(result.getErrors().get(0).contains("groupId=, artifactId='a',"));
}

@Test
Expand All @@ -510,7 +521,7 @@ void testMissingPluginDependencyArtifactId() throws Exception {

assertViolations(result, 0, 1, 0);

assertTrue(result.getErrors().get(0).contains("test:"));
assertTrue(result.getErrors().get(0).contains("groupId='test', artifactId=,"));
}

@Test
Expand All @@ -519,7 +530,7 @@ void testMissingPluginDependencyVersion() throws Exception {

assertViolations(result, 0, 1, 0);

assertTrue(result.getErrors().get(0).contains("test:a"));
assertTrue(result.getErrors().get(0).contains("groupId='test', artifactId='a',"));
}

@Test
Expand All @@ -528,7 +539,7 @@ void testBadPluginDependencyVersion() throws Exception {

assertViolations(result, 0, 1, 0);

assertTrue(result.getErrors().get(0).contains("test:b"));
assertTrue(result.getErrors().get(0).contains("groupId='test', artifactId='b'"));
}

@Test
Expand Down Expand Up @@ -576,10 +587,11 @@ void testBadDependencyExclusionId() throws Exception {
assertViolations(result, 0, 0, 2);

assertContains(
result.getWarnings().get(0), "'dependencies.dependency.exclusions.exclusion.groupId' for gid:aid:jar");
result.getWarnings().get(0),
"'dependencies.dependency.exclusions.exclusion.groupId' for groupId='gid', artifactId='aid', type='jar'");
assertContains(
result.getWarnings().get(1),
"'dependencies.dependency.exclusions.exclusion.artifactId' for gid:aid:jar");
"'dependencies.dependency.exclusions.exclusion.artifactId' for groupId='gid', artifactId='aid', type='jar'");

// MNG-3832: Aether (part of M3+) supports wildcard expressions for exclusions

Expand All @@ -596,10 +608,10 @@ void testMissingDependencyExclusionId() throws Exception {

assertContains(
result.getWarnings().get(0),
"'dependencies.dependency.exclusions.exclusion.groupId' for gid:aid:jar is missing");
"'dependencies.dependency.exclusions.exclusion.groupId' for groupId='gid', artifactId='aid', type='jar' is missing");
assertContains(
result.getWarnings().get(1),
"'dependencies.dependency.exclusions.exclusion.artifactId' for gid:aid:jar is missing");
"'dependencies.dependency.exclusions.exclusion.artifactId' for groupId='gid', artifactId='aid', type='jar' is missing");
}

@Test
Expand All @@ -610,7 +622,7 @@ void testBadImportScopeType() throws Exception {

assertContains(
result.getWarnings().get(0),
"'dependencyManagement.dependencies.dependency.type' for test:a:jar must be 'pom'");
"'dependencyManagement.dependencies.dependency.type' for groupId='test', artifactId='a', type='jar' must be 'pom'");
}

@Test
Expand All @@ -621,7 +633,7 @@ void testBadImportScopeClassifier() throws Exception {

assertContains(
result.getErrors().get(0),
"'dependencyManagement.dependencies.dependency.classifier' for test:a:pom:cls must be empty");
"'dependencyManagement.dependencies.dependency.classifier' for groupId='test', artifactId='a', classifier='cls', type='pom' must be empty");
}

@Test
Expand All @@ -632,16 +644,16 @@ void testSystemPathRefersToProjectBasedir() throws Exception {

assertContains(
result.getWarnings().get(0),
"'dependencies.dependency.scope' for test:a:jar declares usage of deprecated 'system' scope");
"'dependencies.dependency.scope' for groupId='test', artifactId='a', type='jar' declares usage of deprecated 'system' scope");
assertContains(
result.getWarnings().get(1),
"'dependencies.dependency.systemPath' for test:a:jar should not point at files within the project directory");
"'dependencies.dependency.systemPath' for groupId='test', artifactId='a', type='jar' should not point at files within the project directory");
assertContains(
result.getWarnings().get(2),
"'dependencies.dependency.scope' for test:b:jar declares usage of deprecated 'system' scope");
"'dependencies.dependency.scope' for groupId='test', artifactId='b', type='jar' declares usage of deprecated 'system' scope");
assertContains(
result.getWarnings().get(3),
"'dependencies.dependency.systemPath' for test:b:jar should not point at files within the project directory");
"'dependencies.dependency.systemPath' for groupId='test', artifactId='b', type='jar' should not point at files within the project directory");
}

@Test
Expand Down Expand Up @@ -707,10 +719,10 @@ void testDeprecatedDependencyMetaversionsLatestAndRelease() throws Exception {

assertContains(
result.getWarnings().get(0),
"'dependencies.dependency.version' for test:a:jar is either LATEST or RELEASE (both of them are being deprecated)");
"'dependencies.dependency.version' for groupId='test', artifactId='a', type='jar' is either LATEST or RELEASE (both of them are being deprecated)");
assertContains(
result.getWarnings().get(1),
"'dependencies.dependency.version' for test:b:jar is either LATEST or RELEASE (both of them are being deprecated)");
"'dependencies.dependency.version' for groupId='test', artifactId='b', type='jar' is either LATEST or RELEASE (both of them are being deprecated)");
}

@Test
Expand Down
3 changes: 2 additions & 1 deletion its/core-it-suite/src/test/java/org/apache/maven/it/MavenITmng3220ImportScopeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,8 @@ public void testitMNG3220b() throws Exception {
boolean found = false;
for (String line : lines) {
if (line.contains("\'dependencies.dependency.version\' is missing for junit:junit")
|| line.contains("\'dependencies.dependency.version\' for junit:junit:jar is missing")) {
|| line.contains(
"\'dependencies.dependency.version\' for groupId='junit', artifactId='junit', type='jar' is missing")) {
found = true;
break;
}
Expand Down