let BanDuplicateClasses ignore when the bytecode matches exactly #50

bradcupit · 2018-05-03T16:19:59Z

I realize this is a commit bomb, but it adds a new feature to BanDuplicateClasses and lots of tests.

Explanation:
If we have two classes with the same fully qualified name on the classpath BanDuplicateClasses fails, but with this enhancement it only fails if the bytecode of those class files differs. In other words, if they're an exact binary match we skip the classes and move on. The idea being: there's technically no problem if they're the same exact file. This also means there's potentially less fixes to make when porting a large pom.xml to start using BanDuplicateClasses for the first time.

But most importantly: with this new option enabled any time BanDuplicateClasses fails it means there is a definite classpath problem, 100% of the time.

The new option is off by default, but can be enabled in pom files like so:

<plugin>
  <artifactId>maven-enforcer-plugin</artifactId>
  <configuration>
    <rules>
      <BanDuplicateClasses>
        <!-- opt in to this new feature -->
        <ignoreWhenIdentical>true</ignoreWhenIdentical>
      </BanDuplicateClasses>
    </rules>
  </configuration>
</plugin>

This addresses #54

bradcupit · 2018-05-03T16:22:48Z

src/main/java/org/apache/maven/plugins/enforcer/BanDuplicateClasses.java

-            }
-            else
-            {
-                StringBuilder buf = new StringBuilder( message == null ? "Duplicate class found:" : message );


the below block of code didn't change, just the indentation level

Previously there was this statement:

if ( classNames.containsKey( name ) )

Which had all the code nested inside it.
Now we have this, which is the opposite, but returns early:

if ( isFirstTimeSeeingThisClass ) ... return;

So that simplifies things by bailing early and removing a level of indentation.

khmarbaise · 2018-05-12T19:23:12Z

src/main/java/org/apache/maven/plugins/enforcer/BanDuplicateClasses.java

-        Map<String, Set<Artifact>> duplicates = new HashMap<String, Set<Artifact>>();
+
+        Map<String, ClassesWithSameName> classesSeen = new HashMap<String, ClassesWithSameName>();
+        Set<String> duplicateClassNames = new HashSet<String>();


Can you please fix these formatting issues...cause otherwise it's harder to follow what has really changed..

@khmarbaise Yes I'd be happy to fix the formatting issues. I tried so hard to match the existing style.

The only problem is: I don't know what the formatting issues are!

Would you be willing to point out an example or two, so I can manually fix the PR?

khmarbaise

It would be great having a issue for that which is mentioned in the commit message and also would it be great to enhanced the documentation accordingly...

bradcupit · 2018-05-14T20:54:05Z

src/main/java/org/apache/maven/plugins/enforcer/BanDuplicateClasses.java

-            Artifact dup = classNames.put( name, artifact );
-            if ( !( findAllDuplicates && duplicates.containsKey( name ) ) )
-            {
-                for ( IgnorableDependency c : ignores )


It turns out we don't need this block since the earlier block, where we loop over the ignores (lines 240 - 250), would already have returned from this method whenever c.matchesArtifact( artifact ) && c.matches(name) is true -- the same condition we test below.

If we have two classes with the exact same name on the classpath BanDuplicateClasses fails, but with this enhancement it only fails if the bytecode of those class files differs. In other words, if they're an exact binary match we skip the classes and move on. The idea being: there's technically no problem if they're the same exact file. This also means there's potentially less fixes to make when porting a large pom.xml to start using BanDuplicateClasses for the first time. With this new option enabled any time BanDuplicateClasses fails it means there is a definite classpath problem, 100% of the time. The new option is off by default, but can be enabled in pom files. Addresses mojohaus#54

bradcupit · 2018-05-17T18:37:23Z

pom.xml

+    <dependency>
+      <groupId>commons-codec</groupId>
+      <artifactId>commons-codec</artifactId>
+      <version>1.11</version>


This gives us the DigestUtils.md5Hex( inputStream ) method used to compare if two .class files are the same.

bradcupit · 2018-05-17T18:41:53Z

src/it/banduplicate-classes-fail-when-not-identical/pom.xml

@@ -0,0 +1,64 @@
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">


This integration test ensures we still fail the build when the same class is on the classpath multiple times. In particular, this test uses the new mode (ignoreWhenIdentical=true) but since the duplicate classes differ, the build should still fail.

This ensures the new mode doesn't break existing functionality.

bradcupit · 2018-05-17T18:44:02Z

src/it/banduplicate-classes-ignore-when-identical/pom.xml

@@ -0,0 +1,63 @@
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">


This integration test uses the new mode (ignoreWhenIdentical=true) to verify the new code doesn't fail the build when the same exact class appears on the classpath multiple times. Meaning, duplicates classes are ignored when they're identical.

bradcupit · 2018-05-17T18:45:31Z

src/main/java/org/apache/maven/plugins/enforcer/BanDuplicateClasses.java

@@ -149,7 +157,7 @@ else if ( file.isDirectory() )
                        "Unable to process dependency " + o.toString() + " due to " + e.getLocalizedMessage(), e );
                }
            }
-            else if ( file.isFile() && "jar".equals( o.getType() ) )
+            else if ( isJarFile( o ) )


I extracted this line to a static utility method so I could use it elsewhere

bradcupit · 2018-05-18T01:17:02Z

@khmarbaise: as requested I:

created an issue, BanDuplicateClasses ignore when the bytecode matches exactly #54, and changed the commit comment to reference it
updated the documentation.
and fixed all checkstyle warnings and errors

Additionally I added several more comments to this PR to make it easier to review.

Feel free to ask for any other changes needed.

bradcupit commented May 3, 2018

View reviewed changes

khmarbaise reviewed May 12, 2018

View reviewed changes

bradcupit commented May 14, 2018

View reviewed changes

bradcupit force-pushed the ban_duplicate_classes_ignore_bytecode_matches branch from 93f1bd3 to 109735b Compare May 17, 2018 14:47

bradcupit commented May 17, 2018

View reviewed changes

bradcupit added 2 commits May 17, 2018 20:42

fix checkstyle warnings and errors

36b4915

update documentation with new BanDuplicateClasses option

021409d

khmarbaise merged commit 764ba34 into mojohaus:master Jun 15, 2018

batmat added this to the 1.0-beta-9 milestone Jun 25, 2018

bradcupit mentioned this pull request Jul 12, 2018

Publish site for 1.0-beta-9 #59

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

let BanDuplicateClasses ignore when the bytecode matches exactly #50

let BanDuplicateClasses ignore when the bytecode matches exactly #50

bradcupit commented May 3, 2018 •

edited

Loading

bradcupit May 3, 2018 •

edited

Loading

khmarbaise May 12, 2018

bradcupit May 14, 2018

khmarbaise left a comment

bradcupit May 14, 2018 •

edited

Loading

bradcupit May 17, 2018

bradcupit May 17, 2018

bradcupit May 17, 2018 •

edited

Loading

bradcupit May 17, 2018

bradcupit commented May 18, 2018 •

edited

Loading

		@@ -0,0 +1,64 @@
		<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">

		@@ -0,0 +1,63 @@
		<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">

let BanDuplicateClasses ignore when the bytecode matches exactly #50

let BanDuplicateClasses ignore when the bytecode matches exactly #50

Conversation

bradcupit commented May 3, 2018 • edited Loading

bradcupit May 3, 2018 • edited Loading

Choose a reason for hiding this comment

khmarbaise May 12, 2018

Choose a reason for hiding this comment

bradcupit May 14, 2018

Choose a reason for hiding this comment

khmarbaise left a comment

Choose a reason for hiding this comment

bradcupit May 14, 2018 • edited Loading

Choose a reason for hiding this comment

bradcupit May 17, 2018

Choose a reason for hiding this comment

bradcupit May 17, 2018

Choose a reason for hiding this comment

bradcupit May 17, 2018 • edited Loading

Choose a reason for hiding this comment

bradcupit May 17, 2018

Choose a reason for hiding this comment

bradcupit commented May 18, 2018 • edited Loading

bradcupit commented May 3, 2018 •

edited

Loading

bradcupit May 3, 2018 •

edited

Loading

bradcupit May 14, 2018 •

edited

Loading

bradcupit May 17, 2018 •

edited

Loading

bradcupit commented May 18, 2018 •

edited

Loading