Skip to content

Commit 3ba9489

Browse files
cstamascstamas
authored
[MNG-8676] Improve model builder error messages (#2257)
Only the one that is easy to be mixed up with GAV: DepMgtKey is GAT and not GAV. The rest (XML, PK/GA, REPOID, DIR, GAV) ones are not quite mixable, and string "A:B:C" is usually always assumed is GAV. The problem with these two are that they look pretty much same (both a "A:B:C", colon segmented string of 3 or 4), but in one case C is dependency type, in other is version. This PR now lessens the "A:B:C" string overload a bit, and also makes crystal clear that T is not V. The message is precise, but a bit longer. In turn, is not misleading at all now. --- https://issues.apache.org/jira/browse/MNG-8676
1 parent 9f123c1 commit 3ba9489

File tree

4 files changed

+79
-47
lines changed

4 files changed

+79
-47
lines changed

‎impl/maven-core/src/test/java/org/apache/maven/project/ProjectBuilderTest.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -100,8 +100,9 @@ void testVersionlessManagedDependency() throws Exception {
100100
.build(pomFile, configuration));
101101
assertThat(
102102
e.getResults(),
103-
contains(projectBuildingResultWithProblemMessage(
104-
"'dependencies.dependency.version' for org.apache.maven.its:a:jar is missing")));
103+
contains(
104+
projectBuildingResultWithProblemMessage(
105+
"'dependencies.dependency.version' for groupId='org.apache.maven.its', artifactId='a', type='jar' is missing")));
105106
assertThat(e.getResults(), contains(projectBuildingResultWithLocation(5, 9)));
106107
}
107108

‎impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java‎

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2241,11 +2241,29 @@ public static SourceHint xmlNodeInputLocation(XmlNode xmlNode) {
22412241
}
22422242

22432243
public static SourceHint gav(String gav) {
2244-
return new SourceHint(gav, null); // "GAV"
2244+
return new SourceHint(gav, null); // GAV
22452245
}
22462246

22472247
public static SourceHint dependencyManagementKey(Dependency dependency) {
2248-
return new SourceHint(dependency.getManagementKey(), null); // DMK
2248+
String hint;
2249+
if (dependency.getClassifier() == null
2250+
|| dependency.getClassifier().trim().isEmpty()) {
2251+
hint = String.format(
2252+
"groupId=%s, artifactId=%s, type=%s",
2253+
nvl(dependency.getGroupId()), nvl(dependency.getArtifactId()), nvl(dependency.getType()));
2254+
} else {
2255+
hint = String.format(
2256+
"groupId=%s, artifactId=%s, classifier=%s, type=%s",
2257+
nvl(dependency.getGroupId()),
2258+
nvl(dependency.getArtifactId()),
2259+
nvl(dependency.getClassifier()),
2260+
nvl(dependency.getType()));
2261+
}
2262+
return new SourceHint(hint, null); // DMK
2263+
}
2264+
2265+
private static String nvl(String value) {
2266+
return value == null ? "" : "'" + value + "'";
22492267
}
22502268

22512269
public static SourceHint pluginKey(Plugin plugin) {

‎impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelValidatorTest.java‎

Lines changed: 54 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -196,9 +196,11 @@ void testMissingDependencyArtifactId() throws Exception {
196196

197197
assertViolations(result, 0, 1, 0);
198198

199-
assertTrue(result.getErrors()
200-
.get(0)
201-
.contains("'dependencies.dependency.artifactId' for groupId:null:jar is missing"));
199+
assertTrue(
200+
result.getErrors()
201+
.get(0)
202+
.contains(
203+
"'dependencies.dependency.artifactId' for groupId='groupId', artifactId=, type='jar' is missing"));
202204
}
203205

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

208210
assertViolations(result, 0, 1, 0);
209211

210-
assertTrue(result.getErrors()
211-
.get(0)
212-
.contains("'dependencies.dependency.groupId' for null:artifactId:jar is missing"));
212+
assertTrue(
213+
result.getErrors()
214+
.get(0)
215+
.contains(
216+
"'dependencies.dependency.groupId' for groupId=, artifactId='artifactId', type='jar' is missing"));
213217
}
214218

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

219223
assertViolations(result, 0, 1, 0);
220224

221-
assertTrue(result.getErrors()
222-
.get(0)
223-
.contains("'dependencies.dependency.version' for groupId:artifactId:jar is missing"));
225+
assertTrue(
226+
result.getErrors()
227+
.get(0)
228+
.contains(
229+
"'dependencies.dependency.version' for groupId='groupId', artifactId='artifactId', type='jar' is missing"));
224230
}
225231

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

230236
assertViolations(result, 0, 1, 0);
231237

232-
assertTrue(result.getErrors()
233-
.get(0)
234-
.contains("'dependencyManagement.dependencies.dependency.artifactId' for groupId:null:jar is missing"));
238+
assertTrue(
239+
result.getErrors()
240+
.get(0)
241+
.contains(
242+
"'dependencyManagement.dependencies.dependency.artifactId' for groupId='groupId', artifactId=, type='jar' is missing"));
235243
}
236244

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

241249
assertViolations(result, 0, 1, 0);
242250

243-
assertTrue(result.getErrors()
244-
.get(0)
245-
.contains("'dependencyManagement.dependencies.dependency.groupId' for null:artifactId:jar is missing"));
251+
assertTrue(
252+
result.getErrors()
253+
.get(0)
254+
.contains(
255+
"'dependencyManagement.dependencies.dependency.groupId' for groupId=, artifactId='artifactId', type='jar' is missing"));
246256
}
247257

248258
@Test
@@ -327,11 +337,11 @@ void testBadPluginDependencyScope() throws Exception {
327337

328338
assertViolations(result, 0, 3, 0);
329339

330-
assertTrue(result.getErrors().get(0).contains("test:d"));
340+
assertTrue(result.getErrors().get(0).contains("groupId='test', artifactId='d'"));
331341

332-
assertTrue(result.getErrors().get(1).contains("test:e"));
342+
assertTrue(result.getErrors().get(1).contains("groupId='test', artifactId='e'"));
333343

334-
assertTrue(result.getErrors().get(2).contains("test:f"));
344+
assertTrue(result.getErrors().get(2).contains("groupId='test', artifactId='f'"));
335345
}
336346

337347
@Test
@@ -340,9 +350,9 @@ void testBadDependencyScope() throws Exception {
340350

341351
assertViolations(result, 0, 0, 2);
342352

343-
assertTrue(result.getWarnings().get(0).contains("test:f"));
353+
assertTrue(result.getWarnings().get(0).contains("groupId='test', artifactId='f'"));
344354

345-
assertTrue(result.getWarnings().get(1).contains("test:g"));
355+
assertTrue(result.getWarnings().get(1).contains("groupId='test', artifactId='g'"));
346356
}
347357

348358
@Test
@@ -351,7 +361,7 @@ void testBadDependencyManagementScope() throws Exception {
351361

352362
assertViolations(result, 0, 0, 1);
353363

354-
assertContains(result.getWarnings().get(0), "test:g");
364+
assertContains(result.getWarnings().get(0), "groupId='test', artifactId='g'");
355365
}
356366

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

363373
assertContains(
364-
result.getErrors().get(0), "'dependencies.dependency.version' for test:b:jar must be a valid version");
374+
result.getErrors().get(0),
375+
"'dependencies.dependency.version' for groupId='test', artifactId='b', type='jar' must be a valid version");
365376
assertContains(
366377
result.getErrors().get(1),
367-
"'dependencies.dependency.version' for test:c:jar must not contain any of these characters");
378+
"'dependencies.dependency.version' for groupId='test', artifactId='c', type='jar' must not contain any of these characters");
368379
}
369380

370381
@Test
@@ -441,13 +452,13 @@ void testHardCodedSystemPath() throws Exception {
441452

442453
assertContains(
443454
result.getWarnings().get(0),
444-
"'dependencies.dependency.scope' for test:a:jar declares usage of deprecated 'system' scope");
455+
"'dependencies.dependency.scope' for groupId='test', artifactId='a', type='jar' declares usage of deprecated 'system' scope");
445456
assertContains(
446457
result.getWarnings().get(1),
447-
"'dependencies.dependency.systemPath' for test:a:jar should use a variable instead of a hard-coded path");
458+
"'dependencies.dependency.systemPath' for groupId='test', artifactId='a', type='jar' should use a variable instead of a hard-coded path");
448459
assertContains(
449460
result.getWarnings().get(2),
450-
"'dependencies.dependency.scope' for test:b:jar declares usage of deprecated 'system' scope");
461+
"'dependencies.dependency.scope' for groupId='test', artifactId='b', type='jar' declares usage of deprecated 'system' scope");
451462
}
452463

453464
@Test
@@ -501,7 +512,7 @@ void testMissingPluginDependencyGroupId() throws Exception {
501512

502513
assertViolations(result, 0, 1, 0);
503514

504-
assertTrue(result.getErrors().get(0).contains(":a:"));
515+
assertTrue(result.getErrors().get(0).contains("groupId=, artifactId='a',"));
505516
}
506517

507518
@Test
@@ -510,7 +521,7 @@ void testMissingPluginDependencyArtifactId() throws Exception {
510521

511522
assertViolations(result, 0, 1, 0);
512523

513-
assertTrue(result.getErrors().get(0).contains("test:"));
524+
assertTrue(result.getErrors().get(0).contains("groupId='test', artifactId=,"));
514525
}
515526

516527
@Test
@@ -519,7 +530,7 @@ void testMissingPluginDependencyVersion() throws Exception {
519530

520531
assertViolations(result, 0, 1, 0);
521532

522-
assertTrue(result.getErrors().get(0).contains("test:a"));
533+
assertTrue(result.getErrors().get(0).contains("groupId='test', artifactId='a',"));
523534
}
524535

525536
@Test
@@ -528,7 +539,7 @@ void testBadPluginDependencyVersion() throws Exception {
528539

529540
assertViolations(result, 0, 1, 0);
530541

531-
assertTrue(result.getErrors().get(0).contains("test:b"));
542+
assertTrue(result.getErrors().get(0).contains("groupId='test', artifactId='b'"));
532543
}
533544

534545
@Test
@@ -576,10 +587,11 @@ void testBadDependencyExclusionId() throws Exception {
576587
assertViolations(result, 0, 0, 2);
577588

578589
assertContains(
579-
result.getWarnings().get(0), "'dependencies.dependency.exclusions.exclusion.groupId' for gid:aid:jar");
590+
result.getWarnings().get(0),
591+
"'dependencies.dependency.exclusions.exclusion.groupId' for groupId='gid', artifactId='aid', type='jar'");
580592
assertContains(
581593
result.getWarnings().get(1),
582-
"'dependencies.dependency.exclusions.exclusion.artifactId' for gid:aid:jar");
594+
"'dependencies.dependency.exclusions.exclusion.artifactId' for groupId='gid', artifactId='aid', type='jar'");
583595

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

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

597609
assertContains(
598610
result.getWarnings().get(0),
599-
"'dependencies.dependency.exclusions.exclusion.groupId' for gid:aid:jar is missing");
611+
"'dependencies.dependency.exclusions.exclusion.groupId' for groupId='gid', artifactId='aid', type='jar' is missing");
600612
assertContains(
601613
result.getWarnings().get(1),
602-
"'dependencies.dependency.exclusions.exclusion.artifactId' for gid:aid:jar is missing");
614+
"'dependencies.dependency.exclusions.exclusion.artifactId' for groupId='gid', artifactId='aid', type='jar' is missing");
603615
}
604616

605617
@Test
@@ -610,7 +622,7 @@ void testBadImportScopeType() throws Exception {
610622

611623
assertContains(
612624
result.getWarnings().get(0),
613-
"'dependencyManagement.dependencies.dependency.type' for test:a:jar must be 'pom'");
625+
"'dependencyManagement.dependencies.dependency.type' for groupId='test', artifactId='a', type='jar' must be 'pom'");
614626
}
615627

616628
@Test
@@ -621,7 +633,7 @@ void testBadImportScopeClassifier() throws Exception {
621633

622634
assertContains(
623635
result.getErrors().get(0),
624-
"'dependencyManagement.dependencies.dependency.classifier' for test:a:pom:cls must be empty");
636+
"'dependencyManagement.dependencies.dependency.classifier' for groupId='test', artifactId='a', classifier='cls', type='pom' must be empty");
625637
}
626638

627639
@Test
@@ -632,16 +644,16 @@ void testSystemPathRefersToProjectBasedir() throws Exception {
632644

633645
assertContains(
634646
result.getWarnings().get(0),
635-
"'dependencies.dependency.scope' for test:a:jar declares usage of deprecated 'system' scope");
647+
"'dependencies.dependency.scope' for groupId='test', artifactId='a', type='jar' declares usage of deprecated 'system' scope");
636648
assertContains(
637649
result.getWarnings().get(1),
638-
"'dependencies.dependency.systemPath' for test:a:jar should not point at files within the project directory");
650+
"'dependencies.dependency.systemPath' for groupId='test', artifactId='a', type='jar' should not point at files within the project directory");
639651
assertContains(
640652
result.getWarnings().get(2),
641-
"'dependencies.dependency.scope' for test:b:jar declares usage of deprecated 'system' scope");
653+
"'dependencies.dependency.scope' for groupId='test', artifactId='b', type='jar' declares usage of deprecated 'system' scope");
642654
assertContains(
643655
result.getWarnings().get(3),
644-
"'dependencies.dependency.systemPath' for test:b:jar should not point at files within the project directory");
656+
"'dependencies.dependency.systemPath' for groupId='test', artifactId='b', type='jar' should not point at files within the project directory");
645657
}
646658

647659
@Test
@@ -707,10 +719,10 @@ void testDeprecatedDependencyMetaversionsLatestAndRelease() throws Exception {
707719

708720
assertContains(
709721
result.getWarnings().get(0),
710-
"'dependencies.dependency.version' for test:a:jar is either LATEST or RELEASE (both of them are being deprecated)");
722+
"'dependencies.dependency.version' for groupId='test', artifactId='a', type='jar' is either LATEST or RELEASE (both of them are being deprecated)");
711723
assertContains(
712724
result.getWarnings().get(1),
713-
"'dependencies.dependency.version' for test:b:jar is either LATEST or RELEASE (both of them are being deprecated)");
725+
"'dependencies.dependency.version' for groupId='test', artifactId='b', type='jar' is either LATEST or RELEASE (both of them are being deprecated)");
714726
}
715727

716728
@Test

‎its/core-it-suite/src/test/java/org/apache/maven/it/MavenITmng3220ImportScopeTest.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,8 @@ public void testitMNG3220b() throws Exception {
8181
boolean found = false;
8282
for (String line : lines) {
8383
if (line.contains("\'dependencies.dependency.version\' is missing for junit:junit")
84-
|| line.contains("\'dependencies.dependency.version\' for junit:junit:jar is missing")) {
84+
|| line.contains(
85+
"\'dependencies.dependency.version\' for groupId='junit', artifactId='junit', type='jar' is missing")) {
8586
found = true;
8687
break;
8788
}

0 commit comments

Comments
 (0)