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