From 204a18ad2f09e65321304ec184638a0ba092a8b2 Mon Sep 17 00:00:00 2001 From: twalters Date: Sat, 7 Mar 2026 17:22:39 +0000 Subject: [PATCH 1/2] allow for coincidental versions --- .../java/com/solubris/enforcer/Artifact.java | 7 ++++++ .../solubris/enforcer/UnusedPropertyRule.java | 3 ++- .../enforcer/VersionPropertyRule.java | 23 +++++++------------ .../enforcer/VersionPropertyRuleTest.java | 20 +++++++++++++--- 4 files changed, 34 insertions(+), 19 deletions(-) diff --git a/src/main/java/com/solubris/enforcer/Artifact.java b/src/main/java/com/solubris/enforcer/Artifact.java index cebafd3..c8b0843 100644 --- a/src/main/java/com/solubris/enforcer/Artifact.java +++ b/src/main/java/com/solubris/enforcer/Artifact.java @@ -76,4 +76,11 @@ public boolean hasExplicitVersion() { public boolean hasImplicitVersion() { return !Objects.equals(getVersion(), getEffectiveVersion()); } + + /** + * Uniqueness is used to avoid violations on coincidental artifact versions. + */ + public String uniqueness() { + return String.join(":", getGroupId(), getEffectiveVersion()); + } } diff --git a/src/main/java/com/solubris/enforcer/UnusedPropertyRule.java b/src/main/java/com/solubris/enforcer/UnusedPropertyRule.java index 2adf8ec..2e39500 100644 --- a/src/main/java/com/solubris/enforcer/UnusedPropertyRule.java +++ b/src/main/java/com/solubris/enforcer/UnusedPropertyRule.java @@ -15,6 +15,7 @@ import java.util.Set; import java.util.stream.Stream; +import static com.solubris.enforcer.ModelScanner.modelFrom; import static com.solubris.enforcer.ModelScanner.scanModel; import static com.solubris.enforcer.Violations.throwViolations; import static java.util.Collections.emptyList; @@ -39,7 +40,7 @@ public class UnusedPropertyRule extends AbstractEnforcerRule { @SuppressWarnings("unused") @Inject public UnusedPropertyRule(MavenSession session) { - this(ModelScanner.modelFrom(session), session.getCurrentProject().getModel()); + this(modelFrom(session), session.getCurrentProject().getModel()); } protected UnusedPropertyRule(Model originalModel, Model effectiveModel) { diff --git a/src/main/java/com/solubris/enforcer/VersionPropertyRule.java b/src/main/java/com/solubris/enforcer/VersionPropertyRule.java index 7607f89..1885295 100644 --- a/src/main/java/com/solubris/enforcer/VersionPropertyRule.java +++ b/src/main/java/com/solubris/enforcer/VersionPropertyRule.java @@ -4,7 +4,6 @@ import org.apache.maven.enforcer.rule.api.EnforcerRuleException; import org.apache.maven.execution.MavenSession; import org.apache.maven.model.Model; -import org.apache.maven.project.MavenProject; import javax.inject.Inject; import javax.inject.Named; @@ -15,6 +14,7 @@ import java.util.regex.Pattern; import java.util.stream.Stream; +import static com.solubris.enforcer.ModelScanner.modelFrom; import static com.solubris.enforcer.ModelScanner.scanModel; import static com.solubris.enforcer.PropertyUtil.fromPlaceHolder; import static com.solubris.enforcer.Violations.throwViolations; @@ -57,14 +57,6 @@ protected VersionPropertyRule(Model originalModel, Model effectiveModel) { this.effectiveModel = effectiveModel; } - private static Model modelFrom(MavenSession session) { - // Use original model to avoid implicit/default plugins and dependencies - // that Maven adds automatically (e.g., default compiler plugin) - MavenProject project = session.getCurrentProject(); - Model originalModel = project.getOriginalModel(); - return originalModel != null ? originalModel : project.getModel(); - } - @Override public void execute() throws EnforcerRuleException { throwViolations(scan(), "Found {0} version violation(s):"); @@ -78,14 +70,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) @@ -119,7 +111,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. @@ -139,17 +132,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 From abc6875b7f6c2da6f3900326f714c4c02052943d Mon Sep 17 00:00:00 2001 From: twalters Date: Sat, 7 Mar 2026 18:35:07 +0000 Subject: [PATCH 2/2] comment about uniqueness approach --- .../java/com/solubris/enforcer/Artifact.java | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/main/java/com/solubris/enforcer/Artifact.java b/src/main/java/com/solubris/enforcer/Artifact.java index c8b0843..97c51b2 100644 --- a/src/main/java/com/solubris/enforcer/Artifact.java +++ b/src/main/java/com/solubris/enforcer/Artifact.java @@ -79,6 +79,23 @@ public boolean hasImplicitVersion() { /** * 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());