From b10d594dba4aa22707859f02bfe66f1b021cfd30 Mon Sep 17 00:00:00 2001 From: Tamas Cservenak Date: Thu, 3 Apr 2025 13:32:40 +0200 Subject: [PATCH 1/2] [MRESOLVER-673] Improve model validator error messages Source hint should tell what it is: GAV? GATC? Anything else? Especially the GAV and GATC are prone to be mixed by humans when trying to decipher error message. --- .../impl/model/DefaultModelValidator.java | 201 ++++++++++++++---- 1 file changed, 154 insertions(+), 47 deletions(-) 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 index 712de25dace1..52659d0a0bd0 100644 --- 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 @@ -39,6 +39,7 @@ import java.util.stream.Collectors; import java.util.stream.StreamSupport; +import org.apache.maven.api.annotations.Nullable; import org.apache.maven.api.di.Inject; import org.apache.maven.api.di.Named; import org.apache.maven.api.di.Singleton; @@ -75,6 +76,8 @@ import org.apache.maven.model.v4.MavenModelVersion; import org.apache.maven.model.v4.MavenTransformer; +import static java.util.Objects.requireNonNull; + /** */ @Named @@ -804,7 +807,7 @@ private void validateXmlNode( Severity.ERROR, Version.V40, fieldPathPrefix + "." + xmlNode.name(), - xmlNode.inputLocation() != null ? xmlNode.inputLocation().toString() : null, + SourceHint.xmlNodeInputLocation(xmlNode), "Unsupported value '" + childrenCombinationModeAttribute + "' for " + XmlService.CHILDREN_COMBINATION_MODE_ATTRIBUTE + " attribute. " + "Valid values are: " + XmlService.CHILDREN_COMBINATION_APPEND + ", and " + XmlService.CHILDREN_COMBINATION_MERGE @@ -821,7 +824,7 @@ private void validateXmlNode( Severity.ERROR, Version.V40, fieldPathPrefix + "." + xmlNode.name(), - xmlNode.inputLocation() != null ? xmlNode.inputLocation().toString() : null, + SourceHint.xmlNodeInputLocation(xmlNode), "Unsupported value '" + selfCombinationModeAttribute + "' for " + XmlService.SELF_COMBINATION_MODE_ATTRIBUTE + " attribute. " + "Valid values are: " + XmlService.SELF_COMBINATION_OVERRIDE + ", " + XmlService.SELF_COMBINATION_MERGE + ", and " @@ -929,7 +932,12 @@ public void validateEffectiveModel(Model m, int validationLevel, ModelProblemCol "build.plugins.plugin.groupId", problems, Severity.ERROR, Version.V20, p.getGroupId(), p); validate20PluginVersion( - "build.plugins.plugin.version", problems, p.getVersion(), p.getKey(), p, validationLevel); + "build.plugins.plugin.version", + problems, + p.getVersion(), + SourceHint.pluginKey(p), + p, + validationLevel); validateBoolean( "build.plugins.plugin.inherited", @@ -938,7 +946,7 @@ public void validateEffectiveModel(Model m, int validationLevel, ModelProblemCol errOn30, Version.V20, p.getInherited(), - p.getKey(), + SourceHint.pluginKey(p), p); validateBoolean( @@ -948,7 +956,7 @@ public void validateEffectiveModel(Model m, int validationLevel, ModelProblemCol errOn30, Version.V20, p.getExtensions(), - p.getKey(), + SourceHint.pluginKey(p), p); validate20EffectivePluginDependencies(problems, p, validationLevel); @@ -1036,7 +1044,7 @@ private void validate20RawDependencies( Severity.WARNING, Version.V20, prefix + prefix2 + "type", - key, + SourceHint.dependencyManagementKey(dependency), "must be 'pom' to import the managed dependencies.", dependency); } else if (!is41OrBeyond @@ -1047,7 +1055,7 @@ private void validate20RawDependencies( errOn30, Version.V20, prefix + prefix2 + "classifier", - key, + SourceHint.dependencyManagementKey(dependency), "must be empty, imported POM cannot have a classifier.", dependency); } @@ -1059,7 +1067,7 @@ private void validate20RawDependencies( Severity.WARNING, Version.V31, prefix + prefix2 + "scope", - key, + SourceHint.dependencyManagementKey(dependency), "declares usage of deprecated 'system' scope ", dependency); } @@ -1072,7 +1080,7 @@ private void validate20RawDependencies( Severity.WARNING, Version.V20, prefix + prefix2 + "systemPath", - key, + SourceHint.dependencyManagementKey(dependency), "should use a variable instead of a hard-coded path " + sysPath, dependency); } else if (sysPath.contains("${basedir}") || sysPath.contains("${project.basedir}")) { @@ -1081,7 +1089,7 @@ private void validate20RawDependencies( Severity.WARNING, Version.V20, prefix + prefix2 + "systemPath", - key, + SourceHint.dependencyManagementKey(dependency), "should not point at files within the project directory, " + sysPath + " will be unresolvable by dependent projects", dependency); @@ -1095,7 +1103,7 @@ private void validate20RawDependencies( Severity.WARNING, Version.BASE, prefix + prefix2 + "version", - key, + SourceHint.dependencyManagementKey(dependency), "is either LATEST or RELEASE (both of them are being deprecated)", dependency); } @@ -1145,7 +1153,7 @@ private void validate20RawDependenciesSelfReferencing( Severity.FATAL, Version.V31, prefix + "[" + key + "]", - key, + SourceHint.gav(key), "is referencing itself.", dependency); } @@ -1167,11 +1175,25 @@ private void validateEffectiveDependencies( if (validationLevel >= ModelValidator.VALIDATION_LEVEL_MAVEN_2_0) { validateBoolean( - prefix, "optional", problems, errOn30, Version.V20, d.getOptional(), d.getManagementKey(), d); + prefix, + "optional", + problems, + errOn30, + Version.V20, + d.getOptional(), + SourceHint.dependencyManagementKey(d), + d); if (!management) { validateVersion( - prefix, "version", problems, errOn30, Version.V20, d.getVersion(), d.getManagementKey(), d); + prefix, + "version", + problems, + errOn30, + Version.V20, + d.getVersion(), + SourceHint.dependencyManagementKey(d), + d); /* * TODO Extensions like Flex Mojos use custom scopes like "merged", "internal", "external", etc. In @@ -1184,7 +1206,7 @@ private void validateEffectiveDependencies( Severity.WARNING, Version.V20, d.getScope(), - d.getManagementKey(), + SourceHint.dependencyManagementKey(d), d, "provided", "compile", @@ -1201,7 +1223,7 @@ private void validateEffectiveDependencies( Severity.WARNING, Version.V20, d.getScope(), - d.getManagementKey(), + SourceHint.dependencyManagementKey(d), d, "provided", "compile", @@ -1224,7 +1246,13 @@ private void validateEffectiveModelAgainstDependency( // groupId, artifactId, version and classifier coordinates. This is in consequence // a self reference or in other words a circular reference which can not being resolved. addViolation( - problems, Severity.FATAL, Version.V31, prefix + "[" + key + "]", key, "is referencing itself.", d); + problems, + Severity.FATAL, + Version.V31, + prefix + "[" + key + "]", + SourceHint.gav(key), + "is referencing itself.", + d); } } @@ -1241,7 +1269,14 @@ private void validate20EffectivePluginDependencies( validateEffectiveDependency(problems, d, false, prefix, validationLevel); validateVersion( - prefix, "version", problems, errOn30, Version.BASE, d.getVersion(), d.getManagementKey(), d); + prefix, + "version", + problems, + errOn30, + Version.BASE, + d.getVersion(), + SourceHint.dependencyManagementKey(d), + d); validateEnum( prefix, @@ -1250,7 +1285,7 @@ private void validate20EffectivePluginDependencies( errOn30, Version.BASE, d.getScope(), - d.getManagementKey(), + SourceHint.dependencyManagementKey(d), d, "compile", "runtime", @@ -1268,15 +1303,29 @@ private void validateEffectiveDependency( Severity.ERROR, Version.BASE, d.getArtifactId(), - d.getManagementKey(), + SourceHint.dependencyManagementKey(d), d); validateCoordinatesId( - prefix, "groupId", problems, Severity.ERROR, Version.BASE, d.getGroupId(), d.getManagementKey(), d); + prefix, + "groupId", + problems, + Severity.ERROR, + Version.BASE, + d.getGroupId(), + SourceHint.dependencyManagementKey(d), + d); if (!management) { validateStringNotEmpty( - prefix, "type", problems, Severity.ERROR, Version.BASE, d.getType(), d.getManagementKey(), d); + prefix, + "type", + problems, + Severity.ERROR, + Version.BASE, + d.getType(), + SourceHint.dependencyManagementKey(d), + d); validateDependencyVersion(problems, d, prefix); } @@ -1290,7 +1339,7 @@ private void validateEffectiveDependency( Severity.ERROR, Version.BASE, prefix + "systemPath", - d.getManagementKey(), + SourceHint.dependencyManagementKey(d), "is missing.", d); } else { @@ -1301,7 +1350,7 @@ private void validateEffectiveDependency( Severity.ERROR, Version.BASE, prefix + "systemPath", - d.getManagementKey(), + SourceHint.dependencyManagementKey(d), "must specify an absolute path but is " + systemPath, d); } else if (!sysFile.isFile()) { @@ -1311,7 +1360,7 @@ private void validateEffectiveDependency( Severity.WARNING, Version.BASE, prefix + "systemPath", - d.getManagementKey(), + SourceHint.dependencyManagementKey(d), msg, d); } @@ -1322,7 +1371,7 @@ private void validateEffectiveDependency( Severity.ERROR, Version.BASE, prefix + "systemPath", - d.getManagementKey(), + SourceHint.dependencyManagementKey(d), "must be omitted. This field may only be specified for a dependency with system scope.", d); } @@ -1337,7 +1386,7 @@ private void validateEffectiveDependency( Severity.WARNING, Version.V20, exclusion.getGroupId(), - d.getManagementKey(), + SourceHint.dependencyManagementKey(d), exclusion); validateCoordinatesId( @@ -1347,7 +1396,7 @@ private void validateEffectiveDependency( Severity.WARNING, Version.V20, exclusion.getArtifactId(), - d.getManagementKey(), + SourceHint.dependencyManagementKey(d), exclusion); } else { validateCoordinatesIdWithWildcards( @@ -1357,7 +1406,7 @@ private void validateEffectiveDependency( Severity.WARNING, Version.V30, exclusion.getGroupId(), - d.getManagementKey(), + SourceHint.dependencyManagementKey(d), exclusion); validateCoordinatesIdWithWildcards( @@ -1367,7 +1416,7 @@ private void validateEffectiveDependency( Severity.WARNING, Version.V30, exclusion.getArtifactId(), - d.getManagementKey(), + SourceHint.dependencyManagementKey(d), exclusion); } } @@ -1379,7 +1428,14 @@ private void validateEffectiveDependency( */ protected void validateDependencyVersion(ModelProblemCollector problems, Dependency d, String prefix) { validateStringNotEmpty( - prefix, "version", problems, Severity.ERROR, Version.BASE, d.getVersion(), d.getManagementKey(), d); + prefix, + "version", + problems, + Severity.ERROR, + Version.BASE, + d.getVersion(), + SourceHint.dependencyManagementKey(d), + d); } private void validateRawRepositories( @@ -1482,7 +1538,7 @@ private void validate20EffectiveRepository( Severity.WARNING, Version.V20, prefix + "layout", - repository.getId(), + SourceHint.repoId(repository), "uses the unsupported value 'legacy', artifact resolution might fail.", repository); } @@ -1511,7 +1567,7 @@ private void validate20RawResources( errOn30, Version.V20, resource.getFiltering(), - resource.getDirectory(), + SourceHint.resourceDirectory(resource), resource); } } @@ -1533,7 +1589,7 @@ private boolean validateCoordinatesId( Severity severity, Version version, String id, - String sourceHint, + @Nullable SourceHint sourceHint, InputLocationTracker tracker) { if (id != null && validCoordinatesIds.contains(id)) { return true; @@ -1579,7 +1635,7 @@ private boolean validateProfileId( Severity severity, Version version, String id, - String sourceHint, + @Nullable SourceHint sourceHint, InputLocationTracker tracker) { if (validProfileIds.contains(id)) { return true; @@ -1621,7 +1677,7 @@ private boolean validateCoordinatesIdWithWildcards( Severity severity, Version version, String id, - String sourceHint, + @Nullable SourceHint sourceHint, InputLocationTracker tracker) { if (!validateStringNotEmpty(prefix, fieldName, problems, severity, version, id, sourceHint, tracker)) { return false; @@ -1739,7 +1795,7 @@ private boolean validateStringNotEmpty( Severity severity, Version version, String string, - String sourceHint, + @Nullable SourceHint sourceHint, InputLocationTracker tracker) { if (!validateNotNull(prefix, prefix2, fieldName, problems, severity, version, string, sourceHint, tracker)) { return false; @@ -1770,7 +1826,7 @@ private boolean validateStringNotEmpty( Severity severity, Version version, String string, - String sourceHint, + @Nullable SourceHint sourceHint, InputLocationTracker tracker) { if (!validateNotNull(prefix, fieldName, problems, severity, version, string, sourceHint, tracker)) { return false; @@ -1800,7 +1856,7 @@ private boolean validateNotNull( Severity severity, Version version, Object object, - String sourceHint, + @Nullable SourceHint sourceHint, InputLocationTracker tracker) { if (object != null) { return true; @@ -1827,7 +1883,7 @@ private boolean validateNotNull( Severity severity, Version version, Object object, - String sourceHint, + @Nullable SourceHint sourceHint, InputLocationTracker tracker) { if (object != null) { return true; @@ -1846,7 +1902,7 @@ private boolean validateBoolean( Severity severity, Version version, String string, - String sourceHint, + @Nullable SourceHint sourceHint, InputLocationTracker tracker) { if (string == null || string.isEmpty()) { return true; @@ -1876,7 +1932,7 @@ private boolean validateEnum( Severity severity, Version version, String string, - String sourceHint, + @Nullable SourceHint sourceHint, InputLocationTracker tracker, String... validValues) { if (string == null || string.isEmpty()) { @@ -1994,7 +2050,7 @@ private boolean validateBannedCharacters( Severity severity, Version version, String string, - String sourceHint, + @Nullable SourceHint sourceHint, InputLocationTracker tracker, String banned) { if (string != null) { @@ -2024,7 +2080,7 @@ private boolean validateVersion( Severity severity, Version version, String string, - String sourceHint, + @Nullable SourceHint sourceHint, InputLocationTracker tracker) { if (string == null || string.isEmpty()) { return true; @@ -2052,7 +2108,7 @@ private boolean validate20ProperSnapshotVersion( Severity severity, Version version, String string, - String sourceHint, + @Nullable SourceHint sourceHint, InputLocationTracker tracker) { if (string == null || string.isEmpty()) { return true; @@ -2077,7 +2133,7 @@ private boolean validate20PluginVersion( String fieldName, ModelProblemCollector problems, String string, - String sourceHint, + @Nullable SourceHint sourceHint, InputLocationTracker tracker, int validationLevel) { if (string == null) { @@ -2118,7 +2174,7 @@ private static void addViolation( Severity severity, Version version, String fieldName, - String sourceHint, + @Nullable SourceHint sourceHint, String message, InputLocationTracker tracker) { StringBuilder buffer = new StringBuilder(256); @@ -2133,6 +2189,57 @@ private static void addViolation( problems.add(severity, version, buffer.toString(), getLocation(fieldName, tracker)); } + private static class SourceHint { + @Nullable + public static SourceHint xmlNodeInputLocation(XmlNode xmlNode) { + if (xmlNode.inputLocation() != null) { + return new SourceHint(xmlNode.inputLocation().toString(), null); + } else { + return null; + } + } + + public static SourceHint gav(String gav) { + return new SourceHint(gav, null); // "GAV" + } + + public static SourceHint dependencyManagementKey(Dependency dependency) { + return new SourceHint(dependency.getManagementKey(), null); // DMK + } + + public static SourceHint pluginKey(Plugin plugin) { + return new SourceHint(plugin.getKey(), null); // PK + } + + public static SourceHint repoId(Repository repository) { + return new SourceHint(repository.getId(), null); // ID + } + + public static SourceHint resourceDirectory(Resource resource) { + if (resource.getDirectory() == null) { + return null; + } + return new SourceHint(resource.getDirectory(), null); // DIR + } + + private final String hint; + private final String format; + + private SourceHint(String hint, String format) { + this.hint = requireNonNull(hint, "hint"); + this.format = format; + } + + @Override + public String toString() { + String result = hint; + if (format != null) { + result = result + " (" + format + ")"; + } + return result; + } + } + private static InputLocation getLocation(String fieldName, InputLocationTracker tracker) { InputLocation location = null; From 015d9082f62111dd770b029f80d37686f1947885 Mon Sep 17 00:00:00 2001 From: Tamas Cservenak Date: Thu, 3 Apr 2025 13:42:13 +0200 Subject: [PATCH 2/2] Reformat --- .../impl/model/DefaultModelValidator.java | 95 ++++++++++--------- 1 file changed, 48 insertions(+), 47 deletions(-) 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 index 52659d0a0bd0..4ebcf38ddd43 100644 --- 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 @@ -2189,6 +2189,53 @@ private static void addViolation( problems.add(severity, version, buffer.toString(), getLocation(fieldName, tracker)); } + private static InputLocation getLocation(String fieldName, InputLocationTracker tracker) { + InputLocation location = null; + + if (tracker != null) { + if (fieldName != null) { + Object key = fieldName; + + int idx = fieldName.lastIndexOf('.'); + if (idx >= 0) { + fieldName = fieldName.substring(idx + 1); + key = fieldName; + } + + if (fieldName.endsWith("]")) { + key = fieldName.substring(fieldName.lastIndexOf('[') + 1, fieldName.length() - 1); + try { + key = Integer.valueOf(key.toString()); + } catch (NumberFormatException e) { + // use key as is + } + } + + location = tracker.getLocation(key); + } + + if (location == null) { + location = tracker.getLocation(EMPTY); + } + } + + return location; + } + + private static boolean equals(String s1, String s2) { + String c1 = s1 == null ? "" : s1.trim(); + String c2 = s2 == null ? "" : s2.trim(); + return c1.equals(c2); + } + + private static Severity getSeverity(int validationLevel, int errorThreshold) { + if (validationLevel < errorThreshold) { + return Severity.WARNING; + } else { + return Severity.ERROR; + } + } + private static class SourceHint { @Nullable public static SourceHint xmlNodeInputLocation(XmlNode xmlNode) { @@ -2215,6 +2262,7 @@ public static SourceHint repoId(Repository repository) { return new SourceHint(repository.getId(), null); // ID } + @Nullable public static SourceHint resourceDirectory(Resource resource) { if (resource.getDirectory() == null) { return null; @@ -2239,51 +2287,4 @@ public String toString() { return result; } } - - private static InputLocation getLocation(String fieldName, InputLocationTracker tracker) { - InputLocation location = null; - - if (tracker != null) { - if (fieldName != null) { - Object key = fieldName; - - int idx = fieldName.lastIndexOf('.'); - if (idx >= 0) { - fieldName = fieldName.substring(idx + 1); - key = fieldName; - } - - if (fieldName.endsWith("]")) { - key = fieldName.substring(fieldName.lastIndexOf('[') + 1, fieldName.length() - 1); - try { - key = Integer.valueOf(key.toString()); - } catch (NumberFormatException e) { - // use key as is - } - } - - location = tracker.getLocation(key); - } - - if (location == null) { - location = tracker.getLocation(EMPTY); - } - } - - return location; - } - - private static boolean equals(String s1, String s2) { - String c1 = s1 == null ? "" : s1.trim(); - String c2 = s2 == null ? "" : s2.trim(); - return c1.equals(c2); - } - - private static Severity getSeverity(int validationLevel, int errorThreshold) { - if (validationLevel < errorThreshold) { - return Severity.WARNING; - } else { - return Severity.ERROR; - } - } }