Skip to content

Commit cc5d123

Browse files
gnodetgnodet
authored
Improve dependency scope validation error messages for import scope (#10991)
- Enhance error message when 'import' scope is used incorrectly in regular dependencies - Provide clear guidance that 'import' scope is only valid in <dependencyManagement> sections - Replace generic error message with context-aware validation - Update both Maven 3 (compat) and Maven 4 (impl) implementations for consistency - Update tests to verify the improved error messages - Fix grammar in comments (don't -> not) - Apply spotless formatting Before: 'dependencies.dependency.scope' must be one of [provided, compile, runtime, test, system] but is 'import'. After: 'dependencies.dependency.scope' has scope 'import'. The 'import' scope is only valid in <dependencyManagement> sections. This addresses the confusion reported in faktorips/faktorips.base#70 where users receive misleading error messages that suggest 'import' scope is never valid, when it's actually valid in dependency management sections with type=pom.
1 parent 735fd50 commit cc5d123

File tree

4 files changed

+120
-21
lines changed

4 files changed

+120
-21
lines changed

‎compat/maven-model-builder/src/main/java/org/apache/maven/model/validation/DefaultModelValidator.java

Lines changed: 58 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -785,10 +785,10 @@ private void validateEffectiveDependencies(
785785
prefix, "version", problems, errOn30, Version.V20, d.getVersion(), d.getManagementKey(), d);
786786

787787
/*
788-
* TODO Extensions like Flex Mojos use custom scopes like "merged", "internal", "external", etc. In
789-
* order to don't break backward-compat with those, only warn but don't error out.
788+
* Extensions like Flex Mojos use custom scopes like "merged", "internal", "external", etc. In
789+
* order to not break backward-compat with those, only warn but don't error out.
790790
*/
791-
validateEnum(
791+
validateDependencyScope(
792792
prefix,
793793
"scope",
794794
problems,
@@ -797,15 +797,11 @@ private void validateEffectiveDependencies(
797797
d.getScope(),
798798
d.getManagementKey(),
799799
d,
800-
"provided",
801-
"compile",
802-
"runtime",
803-
"test",
804-
"system");
800+
false);
805801

806802
validateEffectiveModelAgainstDependency(prefix, problems, m, d, request);
807803
} else {
808-
validateEnum(
804+
validateDependencyScope(
809805
prefix,
810806
"scope",
811807
problems,
@@ -814,12 +810,7 @@ private void validateEffectiveDependencies(
814810
d.getScope(),
815811
d.getManagementKey(),
816812
d,
817-
"provided",
818-
"compile",
819-
"runtime",
820-
"test",
821-
"system",
822-
"import");
813+
true);
823814
}
824815
}
825816
}
@@ -1462,6 +1453,58 @@ private boolean validateEnum(
14621453
return false;
14631454
}
14641455

1456+
@SuppressWarnings("checkstyle:parameternumber")
1457+
private boolean validateDependencyScope(
1458+
String prefix,
1459+
String fieldName,
1460+
ModelProblemCollector problems,
1461+
Severity severity,
1462+
Version version,
1463+
String scope,
1464+
String sourceHint,
1465+
InputLocationTracker tracker,
1466+
boolean isDependencyManagement) {
1467+
if (scope == null || scope.length() <= 0) {
1468+
return true;
1469+
}
1470+
1471+
String[] validScopes;
1472+
if (isDependencyManagement) {
1473+
validScopes = new String[] {"provided", "compile", "runtime", "test", "system", "import"};
1474+
} else {
1475+
validScopes = new String[] {"provided", "compile", "runtime", "test", "system"};
1476+
}
1477+
1478+
List<String> values = Arrays.asList(validScopes);
1479+
1480+
if (values.contains(scope)) {
1481+
return true;
1482+
}
1483+
1484+
// Provide a more helpful error message for the 'import' scope
1485+
if ("import".equals(scope) && !isDependencyManagement) {
1486+
addViolation(
1487+
problems,
1488+
severity,
1489+
version,
1490+
prefix + fieldName,
1491+
sourceHint,
1492+
"has scope 'import'. The 'import' scope is only valid in <dependencyManagement> sections.",
1493+
tracker);
1494+
} else {
1495+
addViolation(
1496+
problems,
1497+
severity,
1498+
version,
1499+
prefix + fieldName,
1500+
sourceHint,
1501+
"must be one of " + values + " but is '" + scope + "'.",
1502+
tracker);
1503+
}
1504+
1505+
return false;
1506+
}
1507+
14651508
@SuppressWarnings("checkstyle:parameternumber")
14661509
private boolean validateModelVersion(
14671510
ModelProblemCollector problems, String string, InputLocationTracker tracker, String... validVersions) {

‎compat/maven-model-builder/src/test/java/org/apache/maven/model/validation/DefaultModelValidatorTest.java

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -355,6 +355,10 @@ void testBadDependencyScope() throws Exception {
355355
assertViolations(result, 0, 0, 2);
356356

357357
assertTrue(result.getWarnings().get(0).contains("test:f"));
358+
// Check that the import scope error message is more helpful
359+
assertTrue(result.getWarnings()
360+
.get(0)
361+
.contains("has scope 'import'. The 'import' scope is only valid in <dependencyManagement> sections"));
358362

359363
assertTrue(result.getWarnings().get(1).contains("test:g"));
360364
}

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

Lines changed: 54 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1257,12 +1257,12 @@ private void validateEffectiveDependencies(
12571257
dependency);
12581258

12591259
/*
1260-
* TODO Extensions like Flex Mojos use custom scopes like "merged", "internal", "external", etc. In
1261-
* order to don't break backward-compat with those, only warn but don't error out.
1260+
* Extensions like Flex Mojos use custom scopes like "merged", "internal", "external", etc. In
1261+
* order to not break backward-compat with those, only warn but don't error out.
12621262
*/
12631263
ScopeManager scopeManager =
12641264
InternalSession.from(session).getSession().getScopeManager();
1265-
validateEnum(
1265+
validateDependencyScope(
12661266
prefix,
12671267
"scope",
12681268
problems,
@@ -1274,7 +1274,8 @@ private void validateEffectiveDependencies(
12741274
scopeManager.getDependencyScopeUniverse().stream()
12751275
.map(DependencyScope::getId)
12761276
.distinct()
1277-
.toArray(String[]::new));
1277+
.toArray(String[]::new),
1278+
false);
12781279

12791280
validateEffectiveModelAgainstDependency(prefix, problems, model, dependency);
12801281
} else {
@@ -1284,7 +1285,7 @@ private void validateEffectiveDependencies(
12841285
.map(DependencyScope::getId)
12851286
.collect(Collectors.toCollection(HashSet::new));
12861287
scopes.add("import");
1287-
validateEnum(
1288+
validateDependencyScope(
12881289
prefix,
12891290
"scope",
12901291
problems,
@@ -1293,7 +1294,8 @@ private void validateEffectiveDependencies(
12931294
dependency.getScope(),
12941295
SourceHint.dependencyManagementKey(dependency),
12951296
dependency,
1296-
scopes.toArray(new String[0]));
1297+
scopes.toArray(new String[0]),
1298+
true);
12971299
}
12981300
}
12991301
}
@@ -2030,6 +2032,52 @@ private boolean validateEnum(
20302032
return false;
20312033
}
20322034

2035+
@SuppressWarnings("checkstyle:parameternumber")
2036+
private boolean validateDependencyScope(
2037+
String prefix,
2038+
String fieldName,
2039+
ModelProblemCollector problems,
2040+
Severity severity,
2041+
Version version,
2042+
String scope,
2043+
@Nullable SourceHint sourceHint,
2044+
InputLocationTracker tracker,
2045+
String[] validScopes,
2046+
boolean isDependencyManagement) {
2047+
if (scope == null || scope.isEmpty()) {
2048+
return true;
2049+
}
2050+
2051+
List<String> values = Arrays.asList(validScopes);
2052+
2053+
if (values.contains(scope)) {
2054+
return true;
2055+
}
2056+
2057+
// Provide a more helpful error message for the 'import' scope
2058+
if ("import".equals(scope) && !isDependencyManagement) {
2059+
addViolation(
2060+
problems,
2061+
severity,
2062+
version,
2063+
prefix + fieldName,
2064+
sourceHint,
2065+
"has scope 'import'. The 'import' scope is only valid in <dependencyManagement> sections.",
2066+
tracker);
2067+
} else {
2068+
addViolation(
2069+
problems,
2070+
severity,
2071+
version,
2072+
prefix + fieldName,
2073+
sourceHint,
2074+
"must be one of " + values + " but is '" + scope + "'.",
2075+
tracker);
2076+
}
2077+
2078+
return false;
2079+
}
2080+
20332081
@SuppressWarnings("checkstyle:parameternumber")
20342082
private boolean validateModelVersion(
20352083
Session session,

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -410,6 +410,10 @@ void testBadDependencyScope() throws Exception {
410410
assertViolations(result, 0, 0, 2);
411411

412412
assertTrue(result.getWarnings().get(0).contains("groupId='test', artifactId='f'"));
413+
// Check that the import scope error message is more helpful
414+
assertTrue(result.getWarnings()
415+
.get(0)
416+
.contains("has scope 'import'. The 'import' scope is only valid in <dependencyManagement> sections"));
413417

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

0 commit comments

Comments
 (0)