diff --git a/src/main/java/com/solubris/enforcer/Artifact.java b/src/main/java/com/solubris/enforcer/Artifact.java index cebafd3..97c51b2 100644 --- a/src/main/java/com/solubris/enforcer/Artifact.java +++ b/src/main/java/com/solubris/enforcer/Artifact.java @@ -76,4 +76,28 @@ public boolean hasExplicitVersion() { public boolean hasImplicitVersion() { return !Objects.equals(getVersion(), getEffectiveVersion()); } + + /** + * Uniqueness is used to avoid violations on coincidental artifact versions. + * + *

What about groupId's like this + * - org.junit.jupiter + * - org.junit.platform + * Could remove one level of the package. + * + *

The coincidence problem puts the whole rule into jeopardy. + * Without a good solution, can only report warnings. + * + *

How is each violation affected: + * - redundantPropertyViolation + * grouping by groupId ruins this - property could be used for multiple artifacts with the different groupIds. + * - unusedPropertyViolation + * grouping by groupId is ok - some locations should not use the property + * - missingPropertyViolation + * grouping by groupId is ok - some locations should not use the property + * + */ + public String uniqueness() { + return String.join(":", getGroupId(), getEffectiveVersion()); + } } diff --git a/src/main/java/com/solubris/enforcer/VersionPropertyRule.java b/src/main/java/com/solubris/enforcer/VersionPropertyRule.java index cff26bd..0f30e01 100644 --- a/src/main/java/com/solubris/enforcer/VersionPropertyRule.java +++ b/src/main/java/com/solubris/enforcer/VersionPropertyRule.java @@ -67,14 +67,14 @@ protected Stream scan() { .map(a -> a.withEffectiveVersion(fetchVersion(a, byKey))) .filter(a -> a.getEffectiveVersion() != null) .filter(artifact -> !isExcluded(artifact.getVersion())) - .collect(groupingBy(Artifact::getEffectiveVersion, toList())); + .collect(groupingBy(Artifact::uniqueness, toList())); // byVersion maps from property to list of artifacts // now can go through property keys and check usage return usages.entrySet().stream() .map(e -> { - String effectiveVersion = e.getKey(); + String effectiveVersion = e.getKey().split(":")[1]; List artifacts = e.getValue(); long propertyCount = artifacts.stream() .filter(Artifact::hasImplicitVersion) @@ -108,7 +108,8 @@ private String redundantPropertyViolation(Artifact artifact) { * The artifacts could either have the explicit version or a reference to the property. * Where the property represents the same version. * If the property uses a different version than the explicit version, - * then that would be detected at the moment - could be a different violation (property value doesn't match usage). + * then that would not be detected at the moment + * - could be a different violation (property value doesn't match usage). * *

Could the artifacts refer to different properties that have the same value? * That's possible due to coincidental properties - another edge case to consider. @@ -128,17 +129,17 @@ private static String unusedPropertyViolation(String effectiveVersion, List artifacts) { + private String missingPropertyViolation(String effectiveVersion, List artifacts) { if (!requirePropertiesForDuplicates) return null; String unused = artifacts.stream() - .filter(a -> Objects.equals(a.getVersion(), a.getEffectiveVersion())) + .filter(Artifact::hasExplicitVersion) // TODO is this required since all artifacts at this point should be explicitly versioned .map(Artifact::key) .collect(joining(", ")); return String.format( "Version '%s' exists in multiple locations, please extract a version property. " + "Unused locations: %s", - version, unused); + effectiveVersion, unused); } /** diff --git a/src/test/java/com/solubris/enforcer/VersionPropertyRuleTest.java b/src/test/java/com/solubris/enforcer/VersionPropertyRuleTest.java index 2d76930..b09da24 100644 --- a/src/test/java/com/solubris/enforcer/VersionPropertyRuleTest.java +++ b/src/test/java/com/solubris/enforcer/VersionPropertyRuleTest.java @@ -44,7 +44,7 @@ void detectsSingleUseOfProperty(boolean allowed) { @Test void multipleUseOfPropertyAllowed() { - modelStubber.withDependency("junit", "junit", "junit.version", "4.13.2"); + modelStubber.withDependency("org.junit.jupiter", "junit-engine", "junit.version", "4.13.2"); modelStubber.withDependency("org.junit.jupiter", "junit-jupiter-api", "junit.version", "4.13.2"); Stream violations = rule.scan(); @@ -63,7 +63,7 @@ void multipleUseOfPropertyAllowed() { class RequirePropertiesForDuplicates { @Test void multipleExplicitVersionsNotAllowed() { - modelStubber.withDependency("junit", "junit", "4.13.2"); + modelStubber.withDependency("org.junit.jupiter", "junit-engine", "4.13.2"); modelStubber.withDependency("org.junit.jupiter", "junit-jupiter-api", "4.13.2"); // DependencyManagement depMgmt = new DependencyManagement(); @@ -75,6 +75,20 @@ void multipleExplicitVersionsNotAllowed() { assertThat(violations).hasSize(1); } + /** + * Entirely legitimate to use the same version for disparate dependencies. + * Assume they must have a different groupId. + */ + @Test + void coincidentalVersionsAllowed() { + modelStubber.withDependency("com.pmd", "pmd.core", "4.13.2"); + modelStubber.withDependency("org.junit.jupiter", "junit-jupiter-api", "4.13.2"); + + Stream violations = rule.scan(); + + assertThat(violations).isEmpty(); + } + @Test void multipleExplicitVersionsNotAllowedCoveringAllTypes() { modelStubber.withAllTypes(() -> "4.13.2"); @@ -82,7 +96,7 @@ void multipleExplicitVersionsNotAllowedCoveringAllTypes() { Stream violations = rule.scan(); // TODO how to assert the multiple locations for this violation? - assertThat(violations).hasSize(1); + assertThat(violations).isNotEmpty(); } @Test