Fix for issue 33 (#40)

Use more care when deciding whether a given file is a class file or a
resource. Align closer to the java specification (and what the class
loaders consider a valid class name).

Add an integration test for issue 33 that tests the example from #33.

src/it/test-issue-33/invoker.properties

@@ -0,0 +1,16 @@
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+invoker.goals=clean verify
+invoker.buildResult = success

src/it/test-issue-33/pom.xml

@@ -0,0 +1,51 @@
+<!--
+~   Licensed under the Apache License, Version 2.0 (the "License");
+~   you may not use this file except in compliance with the License.
+~   You may obtain a copy of the License at
+~
+~   http://www.apache.org/licenses/LICENSE-2.0
+~
+~   Unless required by applicable law or agreed to in writing, software
+~   distributed under the License is distributed on an "AS IS" BASIS,
+~   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+~   See the License for the specific language governing permissions and
+~   limitations under the License.
+-->
+<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/maven-v4_0_0.xsd">
+
+    <modelVersion>4.0.0</modelVersion>
+
+    <parent>
+        <groupId>@project.groupId@[email protected]@</groupId>
+        <artifactId>basepom</artifactId>
+        <version>1.0.under-test</version>
+    </parent>
+
+    <artifactId>test-issue-33</artifactId>
+
+    <dependencies>
+        <dependency>
+            <groupId>org.assertj</groupId>
+            <artifactId>assertj-core</artifactId>
+            <version>3.14.0</version>
+        </dependency>
+        <dependency>
+            <groupId>net.bytebuddy</groupId>
+            <artifactId>byte-buddy</artifactId>
+            <version>1.10.2</version>
+        </dependency>
+    </dependencies>
+
+    <build>
+        <plugins>
+            <plugin>
+                <groupId>@project.groupId@</groupId>
+                <artifactId>@project.artifactId@</artifactId>
+                <configuration>
+                    <failBuildInCaseOfConflict>true</failBuildInCaseOfConflict>
+                </configuration>
+            </plugin>
+        </plugins>
+    </build>
+</project>

src/it/test-issue-33/verify.groovy

@@ -0,0 +1,21 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+import static org.basepom.mojo.duplicatefinder.groovy.ITools.*
+
+def (result, xml) = loadXmlAndResult(basedir, "test")
+
+// the jolokia agent actually conflicts quite a bit, so there are two expected conflicts.
+overallState(NO_CONFLICT, 0, NOT_FAILED, result)
+
+return true

src/main/java/org/basepom/mojo/duplicatefinder/classpath/ClasspathDescriptor.java

@@ -25,6 +25,7 @@
 import java.util.Collection;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
@@ -33,6 +34,9 @@
 import java.util.zip.ZipEntry;
 import java.util.zip.ZipInputStream;
 
+import javax.lang.model.SourceVersion;
+
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.MoreObjects;
 import com.google.common.base.Predicate;
 import com.google.common.base.Predicates;
@@ -347,16 +351,12 @@ private void addArchive(final ClasspathCacheElement.Builder cacheBuilder, final
             while ((entry = zipInput.getNextEntry()) != null) {
                 if (!entry.isDirectory()) {
                     final String name = entry.getName();
-                    if ("class".equals(Files.getFileExtension(name))) {
-                        final List<String> nameElements = Splitter.on("/").splitToList(name); // ZIP/Jars always use "/" as separators
-                        if (nameElements.isEmpty()) {
-                            LOG.warn("ZIP entry '%s' split into empty list!", entry);
-                        }
-                        else {
-                            final PackageNameHolder packageName = new PackageNameHolder(nameElements.subList(0, nameElements.size() - 1));
-                            final String className = packageName.getQualifiedName(Files.getNameWithoutExtension(name));
-                            cacheBuilder.addClass(className);
-                        }
+                    Optional<List<String>> validatedElements = validateClassName(name);
+                    if (validatedElements.isPresent()) {
+                        List<String> nameElements = validatedElements.get();
+                        final PackageNameHolder packageName = new PackageNameHolder(nameElements.subList(0, nameElements.size() - 1));
+                        final String className = packageName.getQualifiedName(Files.getNameWithoutExtension(name));
+                        cacheBuilder.addClass(className);
                     }
                     else {
                         final String resourcePath = name.replace('\\', File.separatorChar);
@@ -369,4 +369,38 @@ private void addArchive(final ClasspathCacheElement.Builder cacheBuilder, final
             closer.close();
         }
     }
+
+    @VisibleForTesting
+    static Optional<List<String>> validateClassName(String fullClassPath) {
+        if (fullClassPath == null) {
+            return Optional.empty();
+        }
+
+        // ZIP/Jars always use "/" as separators
+        final List<String> nameElements = ImmutableList.copyOf(Splitter.on("/").splitToList(fullClassPath));
+        if (nameElements.isEmpty()) {
+            LOG.warn("ZIP entry '%s' split into empty list!", fullClassPath);
+            return Optional.empty();
+        }
+        String classFileName = nameElements.get(nameElements.size() - 1);
+
+        if (!"class".equals(Files.getFileExtension(classFileName))) {
+            LOG.debug("Ignoring %s, %s is not a class file", fullClassPath, classFileName);
+            return Optional.empty();
+        }
+        for (int i = 0; i < nameElements.size() - 1; i++) {
+            if (!SourceVersion.isIdentifier(nameElements.get(i))) {
+                LOG.debug("Ignoring %s, %s is not a valid package element", fullClassPath, nameElements.get(i));
+                return Optional.empty();
+            }
+        }
+
+        String className = Files.getNameWithoutExtension(classFileName);
+        if (!(SourceVersion.isIdentifier(className) || "module-info".equals(className))) {
+            LOG.debug("Ignoring %s, %s is not a valid class identifier", fullClassPath, className);
+            return Optional.empty();
+        }
+
+        return Optional.of(nameElements);
+    }
 }

src/test/java/org/basepom/mojo/duplicatefinder/classpath/TestClasspathDescriptor.java

@@ -0,0 +1,66 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.basepom.mojo.duplicatefinder.classpath;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import java.util.List;
+import java.util.Optional;
+
+import org.junit.Test;
+
+public class TestClasspathDescriptor
+{
+    @Test
+    public void testValidPackageNames()
+    {
+        String[] validNames = {
+                "_.class",
+                "hello/world.class",
+                "test.class",
+                "__auto_generated/from/some/tool.class",
+                "some/inner$thing.class",
+                "some/package/module-info.class", // module info in sub package
+                "module-info.class" // module info in root package
+        };
+
+        for (String test : validNames) {
+            Optional<List<String>> result = ClasspathDescriptor.validateClassName(test);
+            assertTrue("Failure for '" + test + "'", result.isPresent());
+        }
+    }
+
+    @Test
+    public void testInvalidPackageNames()
+    {
+        String[] invalidNames = {
+                null, // null value
+                "", // empty string
+                "/", // only slash
+                "hello.world", // not a class file
+                "foo/hello.world", // not a class file in subfolder
+                "META-INF/test.class", // package name invalid
+                "2.0/foo.class", // package name invalid
+                "foo/bar/baz-blo/tst.class", // package name is invalid
+                "META-INF/versions/9/foo/module-info.class", // module info in version sub package
+        };
+
+
+        for (String test : invalidNames) {
+            Optional<List<String>> result = ClasspathDescriptor.validateClassName(test);
+            assertFalse("Failure for '" + test + "'", result.isPresent());
+        }
+    }
+}