Improve dependency scope validation error messages for import scope

gnodet · gnodet · commit c8519abe9de2 · 2025-07-23T08:24:55.000Z
- Add context-aware error messages when 'import' scope is used incorrectly - Provide clear guidance that 'import' scope is only valid in <dependencyManagement> sections - Enhance both Maven 3 (compat) and Maven 4 (impl) implementations for consistency - Update tests to verify the improved error messages - Addresses confusion reported in faktorips/faktorips.base#70 Before: 'dependencies.dependency.scope' must be one of [provided, compile, runtime, test, system] but is 'import'. After: 'dependencies.dependency.scope' must be one of [provided, compile, runtime, test, system] but is 'import'. The 'import' scope is only valid in <dependencyManagement> sections and requires <type>pom</type>.
diff --git a/IMPROVEMENT_SUMMARY.md b/IMPROVEMENT_SUMMARY.md
@@ -0,0 +1,79 @@
+# Maven Dependency Scope Validation Improvement
+
+## Issue
+GitHub issue: https://github.com/faktorips/faktorips.base/issues/70
+
+The issue reported a confusing Maven warning message:
+```
+Warning: 'dependencies.dependency.scope' for org.faktorips:eclipse-platform-dependencies:pom must be one of [provided, compile, runtime, test, system] but is 'import'.
+```
+
+This message is misleading because it suggests that `import` scope is never valid, when in fact it's valid in `<dependencyManagement>` sections with `<type>pom</type>`.
+
+## Root Cause
+The Maven model validator was using a generic error message that didn't provide context about when the `import` scope is valid. The validation logic correctly distinguished between regular dependencies and dependency management, but the error message didn't reflect this distinction.
+
+## Solution
+Improved the error message to provide more helpful guidance when the `import` scope is used incorrectly:
+
+### Before
+```
+'dependencies.dependency.scope' must be one of [provided, compile, runtime, test, system] but is 'import'.
+```
+
+### After
+```
+'dependencies.dependency.scope' must be one of [provided, compile, runtime, test, system] but is 'import'. The 'import' scope is only valid in <dependencyManagement> sections and requires <type>pom</type>.
+```
+
+## Changes Made
+
+### 1. compat/maven-model-builder/src/main/java/org/apache/maven/model/validation/DefaultModelValidator.java
+- Added new `validateDependencyScope()` method that provides context-aware error messages
+- Replaced calls to `validateEnum()` with `validateDependencyScope()` for dependency scope validation
+- Enhanced error message specifically for `import` scope misuse
+
+### 2. impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java
+- Added similar improvements to the Maven 4 implementation
+- Maintained consistency between Maven 3 and Maven 4 validation behavior
+
+### 3. Test Updates
+- Updated existing tests to verify the improved error message
+- Added assertions to check for the helpful guidance text
+
+## Benefits
+1. **Clearer guidance**: Users now understand when `import` scope is valid
+2. **Faster resolution**: Developers can quickly identify the correct solution
+3. **Better developer experience**: Reduces confusion and debugging time
+4. **Consistent behavior**: Both Maven 3 and Maven 4 implementations provide the same helpful messages
+
+## Example Usage
+The improved message helps users understand that they should move their `import` scope dependency from:
+
+```xml
+<dependencies>
+  <dependency>
+    <groupId>org.faktorips</groupId>
+    <artifactId>eclipse-platform-dependencies</artifactId>
+    <version>26.1.0-SNAPSHOT</version>
+    <type>pom</type>
+    <scope>import</scope>  <!-- ❌ Invalid here -->
+  </dependency>
+</dependencies>
+```
+
+To:
+
+```xml
+<dependencyManagement>
+  <dependencies>
+    <dependency>
+      <groupId>org.faktorips</groupId>
+      <artifactId>eclipse-platform-dependencies</artifactId>
+      <version>26.1.0-SNAPSHOT</version>
+      <type>pom</type>
+      <scope>import</scope>  <!-- ✅ Valid here -->
+    </dependency>
+  </dependencies>
+</dependencyManagement>
+```
diff --git a/compat/maven-model-builder/src/main/java/org/apache/maven/model/validation/DefaultModelValidator.java b/compat/maven-model-builder/src/main/java/org/apache/maven/model/validation/DefaultModelValidator.java
@@ -788,7 +788,7 @@ private void validateEffectiveDependencies(
                      * TODO Extensions like Flex Mojos use custom scopes like "merged", "internal", "external", etc. In
                      * order to don't break backward-compat with those, only warn but don't error out.
                      */
-                    validateEnum(
+                    validateDependencyScope(
                             prefix,
                             "scope",
                             problems,
@@ -797,15 +797,11 @@ private void validateEffectiveDependencies(
                             d.getScope(),
                             d.getManagementKey(),
                             d,
-                            "provided",
-                            "compile",
-                            "runtime",
-                            "test",
-                            "system");
+                            false);
 
                     validateEffectiveModelAgainstDependency(prefix, problems, m, d, request);
                 } else {
-                    validateEnum(
+                    validateDependencyScope(
                             prefix,
                             "scope",
                             problems,
@@ -814,12 +810,7 @@ private void validateEffectiveDependencies(
                             d.getScope(),
                             d.getManagementKey(),
                             d,
-                            "provided",
-                            "compile",
-                            "runtime",
-                            "test",
-                            "system",
-                            "import");
+                            true);
                 }
             }
         }
@@ -1462,6 +1453,59 @@ private boolean validateEnum(
         return false;
     }
 
+    @SuppressWarnings("checkstyle:parameternumber")
+    private boolean validateDependencyScope(
+            String prefix,
+            String fieldName,
+            ModelProblemCollector problems,
+            Severity severity,
+            Version version,
+            String scope,
+            String sourceHint,
+            InputLocationTracker tracker,
+            boolean isDependencyManagement) {
+        if (scope == null || scope.length() <= 0) {
+            return true;
+        }
+
+        String[] validScopes;
+        if (isDependencyManagement) {
+            validScopes = new String[] {"provided", "compile", "runtime", "test", "system", "import"};
+        } else {
+            validScopes = new String[] {"provided", "compile", "runtime", "test", "system"};
+        }
+
+        List<String> values = Arrays.asList(validScopes);
+
+        if (values.contains(scope)) {
+            return true;
+        }
+
+        // Provide a more helpful error message for the 'import' scope
+        if ("import".equals(scope) && !isDependencyManagement) {
+            addViolation(
+                    problems,
+                    severity,
+                    version,
+                    prefix + fieldName,
+                    sourceHint,
+                    "must be one of " + values + " but is '" + scope + "'. "
+                            + "The 'import' scope is only valid in <dependencyManagement> sections and requires <type>pom</type>.",
+                    tracker);
+        } else {
+            addViolation(
+                    problems,
+                    severity,
+                    version,
+                    prefix + fieldName,
+                    sourceHint,
+                    "must be one of " + values + " but is '" + scope + "'.",
+                    tracker);
+        }
+
+        return false;
+    }
+
     @SuppressWarnings("checkstyle:parameternumber")
     private boolean validateModelVersion(
             ModelProblemCollector problems, String string, InputLocationTracker tracker, String... validVersions) {
diff --git a/compat/maven-model-builder/src/test/java/org/apache/maven/model/validation/DefaultModelValidatorTest.java b/compat/maven-model-builder/src/test/java/org/apache/maven/model/validation/DefaultModelValidatorTest.java
@@ -355,6 +355,8 @@ void testBadDependencyScope() throws Exception {
         assertViolations(result, 0, 0, 2);
 
         assertTrue(result.getWarnings().get(0).contains("test:f"));
+        // Check that the import scope error message is more helpful
+        assertTrue(result.getWarnings().get(0).contains("The 'import' scope is only valid in <dependencyManagement> sections"));
 
         assertTrue(result.getWarnings().get(1).contains("test:g"));
     }
diff --git a/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java b/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java
@@ -1225,7 +1225,7 @@ private void validateEffectiveDependencies(
                      */
                     ScopeManager scopeManager =
                             InternalSession.from(session).getSession().getScopeManager();
-                    validateEnum(
+                    validateDependencyScope(
                             prefix,
                             "scope",
                             problems,
@@ -1237,7 +1237,8 @@ private void validateEffectiveDependencies(
                             scopeManager.getDependencyScopeUniverse().stream()
                                     .map(DependencyScope::getId)
                                     .distinct()
-                                    .toArray(String[]::new));
+                                    .toArray(String[]::new),
+                            false);
 
                     validateEffectiveModelAgainstDependency(prefix, problems, model, dependency);
                 } else {
@@ -1247,7 +1248,7 @@ private void validateEffectiveDependencies(
                             .map(DependencyScope::getId)
                             .collect(Collectors.toCollection(HashSet::new));
                     scopes.add("import");
-                    validateEnum(
+                    validateDependencyScope(
                             prefix,
                             "scope",
                             problems,
@@ -1256,7 +1257,8 @@ private void validateEffectiveDependencies(
                             dependency.getScope(),
                             SourceHint.dependencyManagementKey(dependency),
                             dependency,
-                            scopes.toArray(new String[0]));
+                            scopes.toArray(new String[0]),
+                            true);
                 }
             }
         }
@@ -1993,6 +1995,53 @@ private boolean validateEnum(
         return false;
     }
 
+    @SuppressWarnings("checkstyle:parameternumber")
+    private boolean validateDependencyScope(
+            String prefix,
+            String fieldName,
+            ModelProblemCollector problems,
+            Severity severity,
+            Version version,
+            String scope,
+            @Nullable SourceHint sourceHint,
+            InputLocationTracker tracker,
+            String[] validScopes,
+            boolean isDependencyManagement) {
+        if (scope == null || scope.isEmpty()) {
+            return true;
+        }
+
+        List<String> values = Arrays.asList(validScopes);
+
+        if (values.contains(scope)) {
+            return true;
+        }
+
+        // Provide a more helpful error message for the 'import' scope
+        if ("import".equals(scope) && !isDependencyManagement) {
+            addViolation(
+                    problems,
+                    severity,
+                    version,
+                    prefix + fieldName,
+                    sourceHint,
+                    "must be one of " + values + " but is '" + scope + "'. "
+                            + "The 'import' scope is only valid in <dependencyManagement> sections and requires <type>pom</type>.",
+                    tracker);
+        } else {
+            addViolation(
+                    problems,
+                    severity,
+                    version,
+                    prefix + fieldName,
+                    sourceHint,
+                    "must be one of " + values + " but is '" + scope + "'.",
+                    tracker);
+        }
+
+        return false;
+    }
+
     @SuppressWarnings("checkstyle:parameternumber")
     private boolean validateModelVersion(
             Session session,
diff --git a/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelValidatorTest.java b/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelValidatorTest.java
@@ -410,6 +410,8 @@ void testBadDependencyScope() throws Exception {
         assertViolations(result, 0, 0, 2);
 
         assertTrue(result.getWarnings().get(0).contains("groupId='test', artifactId='f'"));
+        // Check that the import scope error message is more helpful
+        assertTrue(result.getWarnings().get(0).contains("The 'import' scope is only valid in <dependencyManagement> sections"));
 
         assertTrue(result.getWarnings().get(1).contains("groupId='test', artifactId='g'"));
     }

Original file line number	Diff line number	Diff line change
`@@ -355,6 +355,8 @@ void testBadDependencyScope() throws Exception {`
`355`	`355`	`assertViolations(result, 0, 0, 2);`
`356`	`356`
`357`	`357`	`assertTrue(result.getWarnings().get(0).contains("test:f"));`
	`358`	`+ // Check that the import scope error message is more helpful`
	`359`	`+ assertTrue(result.getWarnings().get(0).contains("The 'import' scope is only valid in <dependencyManagement> sections"));`
`358`	`360`
`359`	`361`	`assertTrue(result.getWarnings().get(1).contains("test:g"));`
`360`	`362`	`}`
Original file line number	Diff line number	Diff line change
`@@ -410,6 +410,8 @@ void testBadDependencyScope() throws Exception {`
`410`	`410`	`assertViolations(result, 0, 0, 2);`
`411`	`411`
`412`	`412`	`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().get(0).contains("The 'import' scope is only valid in <dependencyManagement> sections"));`
`413`	`415`
`414`	`416`	`assertTrue(result.getWarnings().get(1).contains("groupId='test', artifactId='g'"));`
`415`	`417`	`}`