Plugin to validate all classes on the classpath are uniquely named #267

iamdanfox · 2018-04-30T15:26:13Z

Usage

For now, I'm suggesting people explicitly opt-in to this plugin. It adds a new task called checkClassUniqueness which runs as a dependency of check. This task should fail if any jars in the specified classpath happen to contain identically named classes (ie same package and class name).

apply plugin: 'com.palantir.baseline-class-uniqueness'

Example output

> Task :checkClassUniqueness FAILED
26 Identically named classes with differing impls found in [javax.servlet.jsp:jsp-api:2.1, javax.el:javax.el-api:3.0.0]: [javax.el.ExpressionFactory, javax.el.BeanELResolver, javax.el.CompositeELResolver$CompositeIterator, javax.el.ELContextListener, javax.el.MethodExpression, javax.el.PropertyNotWritableException, javax.el.FunctionMapper, javax.el.MethodInfo, javax.el.BeanELResolver$BeanProperty, javax.el.ELException, javax.el.MapELResolver, javax.el.ArrayELResolver, javax.el.CompositeELResolver, javax.el.ListELResolver, javax.el.Expression, javax.el.ELUtil, javax.el.BeanELResolver$BeanProperties, javax.el.ELResolver, javax.el.MethodNotFoundException, javax.el.ELUtil$1, javax.el.ValueExpression, javax.el.ELContextEvent, javax.el.ResourceBundleELResolver, javax.el.ELContext, javax.el.PropertyNotFoundException, javax.el.VariableMapper]

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':checkClassUniqueness'.
> 'runtime' contains multiple copies of identically named classes - this may cause different runtime behaviour depending on classpath ordering.
  To resolve this, try excluding one of the following jars:
  
  	(26 classes)  javax.servlet.jsp:jsp-api:2.1 javax.el:javax.el-api:3.0.0

Followup to #255.

Future work

More speed
handle project(':foo') dependencies

Note: I intentionally didn't include 'classpath' in the name because java10 replaces the concept of a 'classpath' with a 'modulepath' and I think this plugin should be able to cover both.

carterkozak

lgtm.
May need to aded a configuration block for allowed duplicates, there are some instances that can be difficult to deduplicate. If we can get away with not providing that functionality, that's likely much better.

iamdanfox · 2018-05-01T12:59:26Z

I think this features actually needs more work - I just ran this on a bigger project and got a pretty intimidating failure. I think the summary needs more information. Also unclear how the singleton set [org.apache.xmlbeans:xmlbeans:2.6.0] appeared in this list:

EDIT, updated sorted table:

'testRuntime' contains multiple copies of identically named classes - this may cause different runtime behaviour depending on classpath ordering.
  To resolve this, try excluding one of the following jars, changing a version or shadowing:
  
        (4 classes)   com.google.code.findbugs:annotations:3.0.0        com.github.stephenc.jcip:jcip-annotations:1.0-1   
        (35 classes)  com.google.code.findbugs:annotations:3.0.0        com.google.code.findbugs:jsr305:3.0.0             
        (1 classes)   com.palantir.tritium:tritium-api:0.9.0            com.palantir.tritium:tritium-core:0.9.0           
        (18 classes)  com.sun.jersey:jersey-server:1.9                  org.glassfish.jersey.core:jersey-server:2.25.1    
        (6 classes)   commons-logging:commons-logging:1.2               org.slf4j:jcl-over-slf4j:1.7.25                   
        (26 classes)  javax.servlet.jsp:jsp-api:2.1                     org.glassfish:javax.el:3.0.0                      
        (29 classes)  log4j:log4j:1.2.17                                org.slf4j:log4j-over-slf4j:1.7.25                 
        (8 classes)   org.apache.xmlbeans:xmlbeans:2.6.0                
        (6 classes)   org.glassfish.hk2.external:javax.inject:2.5.0-b32 javax.inject:javax.inject:1                       
        (35 classes)  org.hamcrest:hamcrest-all:1.3                     org.hamcrest:hamcrest-library:1.3                 
        (45 classes)  org.hamcrest:hamcrest-core:1.3                    org.hamcrest:hamcrest-all:1.3

UPDATE 2: I think some of these are actually false positives because the duplicated classes might be identical. For example, the following two jars both contain this guy:

com.google.code.findbugs:annotations:3.0.0
com.google.code.findbugs:jsr305:3.0.0

//
// Source code recreated from a .class file by IntelliJ IDEA
// (powered by Fernflower decompiler)
//

package javax.annotation;

import java.lang.annotation.Documented;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import javax.annotation.meta.TypeQualifier;
import javax.annotation.meta.When;

@Documented
@TypeQualifier(
    applicableTo = CharSequence.class
)
@Retention(RetentionPolicy.RUNTIME)
public @interface Syntax {
    String value();

    When when() default When.ALWAYS;
}

gracew · 2018-05-01T16:38:38Z

is there a reason we can't use https://github.com/nebula-plugins/gradle-lint-plugin/wiki/Duplicate-Classes-Rule instead of implementing this ourselves?

gracew · 2018-05-01T17:03:52Z

README.md

+
+### Class Uniqueness Plugin (com.palantir.baseline-class-uniqueness)
+
+Run `./gradlew checkClassUniqueness` to scan all jars on the `testRuntime` classpath for identically named classes.


the plugin configures the task with the runtime configuration

gracew · 2018-05-01T17:05:01Z

...seline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineClassUniquenessPlugin.java

+import com.palantir.baseline.tasks.CheckClassUniquenessTask;
+import org.gradle.api.Project;
+
+@SuppressWarnings("checkstyle:designforextension") // making this 'final' breaks gradle


you can make the apply method final and get rid of this suppression

gracew · 2018-05-01T17:05:46Z

gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckClassUniquenessTask.java

+import org.gradle.api.tasks.OutputFile;
+import org.gradle.api.tasks.TaskAction;
+
+@SuppressWarnings("checkstyle:designforextension") // making this 'final' breaks gradle


again, should be able to make the individual methods final. also, maybe include some javadoc mentioning the gradle lint rule and differences in our implementation (e.g. hashing of class files)?

gracew · 2018-05-01T17:10:11Z

gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckClassUniquenessTask.java

+                    "'%s' contains multiple copies of identically named classes - "
+                            + "this may cause different runtime behaviour depending on classpath ordering.\n"
+                            + "To resolve this, try excluding one of the following jars, "
+                            + "changing a version or shadowing:\n\n%s",


I don't really want to encourage changing a version or shadowing =/

gracew · 2018-05-01T17:11:18Z

gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckClassUniquenessTask.java

+    }
+
+    /**
+     * This only exists to convince gradle this task is incremental.


don't think this comment is strictly necessary -- I feel this is pretty standard/well understood for people who write gradle plugins

I think it's helpful to reassure readers of this code that the file we write really is inconsequential and not magically wired to something elsewhere in the project.

gracew · 2018-05-01T17:22:43Z

gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/ClassUniquenessAnalyzer.java

+                .getResolvedConfiguration()
+                .getResolvedArtifacts();
+
+        Map<String, Set<ModuleVersionIdentifier>> tempClassToJarMap = new HashMap<>();


nit, the other one is called classToJarsMap (plural jars)

gracew · 2018-05-01T17:24:07Z

gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/ClassUniquenessAnalyzer.java

+                            resolvedArtifact.getModuleVersion().getId());
+                }
+            } catch (Exception e) {
+                log.error("Failed to read JarFile {}", resolvedArtifact, e);


the resolvedArtifact might not be a jar -- we should probs filter those out first, a la https://github.com/nebula-plugins/gradle-lint-plugin/blob/master/src/main/groovy/com/netflix/nebula/lint/rule/dependency/DependencyService.groovy#L92-L94

gracew · 2018-05-01T17:26:10Z

gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/ClassUniquenessAnalyzer.java

+                .filter(entry -> entry.getValue().size() > 1)
+                .forEach(entry -> {
+                    // add to the top level map
+                    entry.getValue().forEach(value -> multiMapPut(classToJarsMap, entry.getKey(), value));


not sure why we need to save off this collection if we're not returning it anywhere -- getProblemJars could be implemented with jarsToClasses.keySet(), i believe

gracew · 2018-05-01T17:28:48Z

...va/src/test/groovy/com/palantir/baseline/BaselineClassUniquenessPluginIntegrationTest.groovy

+        when:
+        buildFile << standardBuildFile
+        buildFile << """
+        dependencies {


i'd prefer to trim this test case down to a more minimal one

Lots of entries here allows me to verify the sorted table works nicely.

EDIT have thrown away the complicated sorting and split these unit tests.

gracew · 2018-05-01T17:30:05Z

...va/src/test/groovy/com/palantir/baseline/BaselineClassUniquenessPluginIntegrationTest.groovy

+        then:
+        result.getOutput().contains("Identically named classes found in 2 jars ([javax.servlet.jsp:jsp-api:2.1, javax.el:javax.el-api:3.0.0]): [javax.")
+        result.getOutput().contains("'runtime' contains multiple copies of identically named classes")
+        result.getOutput().contains("(4 classes)   com.google.code.findbugs:annotations:3.0.1      net.jcip:jcip-annotations:1.0                   com.github.stephenc.jcip:jcip-annotations:1.0-1");


I feel like people would want to know the class names that are duplicated -- thoughts on listing that out? (it would be more verbose =/)

These appear in log lines (at ERROR) level, so they'll appear in the console

gracew · 2018-05-01T17:37:01Z

talked offline, one benefit cited for this approach over the nebula lint rule is that this PR hashes class files, not erroring in cases where identical classes are found on the classpath (this is considered unhygienic but isn't actively harmful).

however, after reading through the PR I'm not sure where that hashing of class file content is happening -- ClassUniquenessAnalyzer only seems to handle jar/class names?

gracew · 2018-05-02T16:53:12Z

gradle-baseline-java/build.gradle

 dependencies {
    compile gradleApi()
    compile 'net.ltgt.gradle:gradle-errorprone-plugin'
+    compile 'com.google.guava:guava'


let's remove the testCompile dep in line 17

gracew · 2018-05-02T16:55:47Z

...va/src/test/groovy/com/palantir/baseline/BaselineClassUniquenessPluginIntegrationTest.groovy

+        buildFile << """
+        dependencies {
+            compile 'com.palantir.tritium:tritium-api:0.9.0'
+            compile 'com.palantir.tritium:tritium-core:0.9.0'


what are the duplicate classes here? i'd expect tritium-core to pull in tritium-api and therefore not have any duplicate classes

Just a package-info.java

gracew · 2018-05-02T16:57:07Z

gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/ClassUniquenessAnalyzer.java

+
+        dependencies.stream().forEach(resolvedArtifact -> {
+            File file = resolvedArtifact.getFile();
+            if (!file.exists()) {


probably wanna filter out non-jars too to avoid spurious errors in line 92

TBH I'd kinda like to just get an MVP released and get a few repos to pick it up and see what we encounter! The -sources.jar and -javadoc.jar thing doesn't seem to cause a problem because we only consider files ending in .class.

gracew · 2018-05-02T17:00:04Z

gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/ClassUniquenessAnalyzer.java

+        });
+
+        // discard all the classes that only come from one jar - these are completely safe!
+        classToJars.entrySet().stream()


don't think we need this collection -- finding which entries of tempClassToHashCodes have value of size > 1 (which we're doing a few lines down) should be sufficient

If we delete classToJars then I don't really see how we can accumulate information about where a class came from and then derive the Map<Set<ModuleVersionIdentifier>, Set<String>> jarsToClasses we need to report problems?

People need to know which are the offending jars!

oh whoops I didn't read this closely enough

gracew · 2018-05-02T21:45:15Z

gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/ClassUniquenessAnalyzer.java

+        });
+
+        // discard all the classes that only come from one jar - these are completely safe!
+        classToJars.entrySet().stream()


oh whoops I didn't read this closely enough

iamdanfox mentioned this pull request Apr 30, 2018

Added BaselineClasspathConflict plugin #255

Closed

Carter Kozak and others added 12 commits April 30, 2018 19:55

Added BaselineClasspathConflict plugin

77d0413

Rename to BaselineClasspathDuplicatesPlugin

8c38f0c

Add meta info properties file

d94f939

silentConflict -> checkClasspathIsDuplicateFree

7941840

Factor out logic to a new CheckUniqueClassNamesTask

19b30c2

apply plugin order shouldn't matter

5ce2ee8

Checkstyle

c9c3b55

Ugh groovy

0919961

Add some magic publishing stuff

9d0745b

Try removing the 'final' modifier

a81b6b3

Convert to Java

b66f48c

Remove 'final' modifier

a1275df

iamdanfox force-pushed the classpath-duplicates-plugin branch from 5a3deec to a1275df Compare April 30, 2018 18:55

iamdanfox added 6 commits April 30, 2018 19:58

Verify up-to-date behaviour

ed4425e

Slightly different syntax

5ea80f6

Don't use org.gradle.internal.impldep HashMultimap

a1b2156

Checkstyle

982e92d

Test verifies duplicates can be detected

bcf86e8

test for up-to-date

cfa7aec

iamdanfox force-pushed the classpath-duplicates-plugin branch from 4bbd47b to cfa7aec Compare April 30, 2018 19:55

iamdanfox added 5 commits April 30, 2018 21:31

Sweet human readable output

830af37

Factor out the crawling

7784164

Get ready for parallel

9e9ab49

parallelStream over dependencies

4591774

Test case for project dependencies

ea7cd0b

iamdanfox changed the title ~~Baseline classpath duplicates plugin~~ Plugin to validate all classes on the classpath are uniquely named Apr 30, 2018

iamdanfox added 3 commits April 30, 2018 23:13

Rename to 'com.palantir.baseline-class-uniqueness'

67264ff

README

27b793c

Don't enable straight away for everyone

8692c46

iamdanfox requested a review from uschi2000 April 30, 2018 22:23

carterkozak approved these changes May 1, 2018

View reviewed changes

iamdanfox added 4 commits May 1, 2018 14:23

Add sweet formatted table with count of offending classes

e676bb2

Sort table nicely

94c6b38

Check runtime instead of testRuntime by default

4f1f8c8

More comprehensive test

f6b4ebd

iamdanfox force-pushed the classpath-duplicates-plugin branch from cb013a1 to f6b4ebd Compare May 1, 2018 13:52

Factor out ClassUniquenessAnalyzer

9f4963d

gracew reviewed May 1, 2018

View reviewed changes

iamdanfox added 9 commits May 2, 2018 15:40

Pull in guava

c17aa36

Tests pass

73bf75f

Checkstyle

3d5c250

Some CR tweaks

778c4fa

Add some javadoc

2bdcc50

Don't bother sorting the table

087003f

Only report classes with differing impls

2ac6526

Ensure we only log classes with different impls

f3e98d4

More minimal tests

452a0b6

iamdanfox force-pushed the classpath-duplicates-plugin branch from 503f40d to 452a0b6 Compare May 2, 2018 15:57

iamdanfox added 2 commits May 2, 2018 17:03

Javadoc mentioning netflix's impl

4526967

Be better at programming

4640a4d

gracew reviewed May 2, 2018

View reviewed changes

Remove duplicate guava dep

caaa580

gracew approved these changes May 2, 2018

View reviewed changes

iamdanfox merged commit de7737c into develop May 2, 2018

robert3005 deleted the classpath-duplicates-plugin branch August 13, 2018 19:58

pkoenig10 mentioned this pull request Nov 17, 2020

Apply baseline-class-unqiueness by default #1553

Merged


		### Class Uniqueness Plugin (com.palantir.baseline-class-uniqueness)

		Run `./gradlew checkClassUniqueness` to scan all jars on the `testRuntime` classpath for identically named classes.

Plugin to validate all classes on the classpath are uniquely named #267

Plugin to validate all classes on the classpath are uniquely named #267

Uh oh!

Conversation

iamdanfox commented Apr 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Usage

Example output

Future work

Uh oh!

carterkozak left a comment

Choose a reason for hiding this comment

Uh oh!

iamdanfox commented May 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gracew commented May 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamdanfox May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamdanfox May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gracew commented May 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamdanfox May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

iamdanfox commented Apr 30, 2018 •

edited

Loading

iamdanfox commented May 1, 2018 •

edited

Loading

iamdanfox May 2, 2018 •

edited

Loading

iamdanfox May 2, 2018 •

edited

Loading

gracew commented May 1, 2018 •

edited

Loading

iamdanfox May 2, 2018 •

edited

Loading