THRIFT-3784: thrift-maven-plugin generates invalid include directories for IDL in dependency JARs

Client: thrift-maven (contrib)

This closes #984

4 files changed, 145 insertions, 15 deletions

diff --git a/contrib/thrift-maven-plugin/pom.xml b/contrib/thrift-maven-plugin/pom.xml
index 5bc100414..0af595764 100644
--- a/contrib/thrift-maven-plugin/pom.xml
+++ b/contrib/thrift-maven-plugin/pom.xml
@@ -77,6 +77,12 @@
       <artifactId>plexus-utils</artifactId>
       <version>3.0.14</version>
     </dependency>
+    <dependency>
+      <groupId>org.mockito</groupId>
+      <artifactId>mockito-all</artifactId>
+      <version>1.10.19</version>
+      <scope>test</scope>
+   </dependency>
   </dependencies>
   <properties>
     <thrift.root>${basedir}/../..</thrift.root>
diff --git a/contrib/thrift-maven-plugin/src/main/java/org/apache/thrift/maven/AbstractThriftMojo.java b/contrib/thrift-maven-plugin/src/main/java/org/apache/thrift/maven/AbstractThriftMojo.java
index 0913c77cd..869be95e4 100644
--- a/contrib/thrift-maven-plugin/src/main/java/org/apache/thrift/maven/AbstractThriftMojo.java
+++ b/contrib/thrift-maven-plugin/src/main/java/org/apache/thrift/maven/AbstractThriftMojo.java
@@ -117,7 +117,7 @@ abstract class AbstractThriftMojo extends AbstractMojo {
      * @parameter default-value="${localRepository}"
      * @required
      */
-    private ArtifactRepository localRepository;
+    protected ArtifactRepository localRepository;
 
     /**
      * Set this to {@code false} to disable hashing of dependent jar paths.
@@ -129,7 +129,7 @@ abstract class AbstractThriftMojo extends AbstractMojo {
      * @parameter default-value="true"
      * @required
      */
-    private boolean hashDependentPaths;
+    protected boolean hashDependentPaths;
 
     /**
      * @parameter
@@ -229,7 +229,7 @@ abstract class AbstractThriftMojo extends AbstractMojo {
         checkNotNull(generator, "generator");
         final File thriftSourceRoot = getThriftSourceRoot();
         checkNotNull(thriftSourceRoot);
-        checkArgument(!thriftSourceRoot.isFile(), "thriftSourceRoot is a file, not a diretory");
+        checkArgument(!thriftSourceRoot.isFile(), "thriftSourceRoot is a file, not a directory");
         checkNotNull(temporaryThriftFileDirectory, "temporaryThriftFileDirectory");
         checkState(!temporaryThriftFileDirectory.isFile(), "temporaryThriftFileDirectory is a file, not a directory");
         final File outputDirectory = getOutputDirectory();
@@ -268,7 +268,9 @@ abstract class AbstractThriftMojo extends AbstractMojo {
         if (temporaryThriftFileDirectory.exists()) {
             cleanDirectory(temporaryThriftFileDirectory);
         }
+
         Set<File> thriftDirectories = newHashSet();
+
         for (File classpathElementFile : classpathElementFiles) {
             // for some reason under IAM, we receive poms as dependent files
             // I am excluding .xml rather than including .jar as there may be other extensions in use (sar, har, zip)
@@ -283,18 +285,27 @@ abstract class AbstractThriftMojo extends AbstractMojo {
                     throw new IllegalArgumentException(format(
                             "%s was not a readable artifact", classpathElementFile));
                 }
+
+                /**
+                 * Copy each .thrift file found in the JAR into a temporary directory, preserving the
+                 * directory path it had relative to its containing JAR. Add the resulting root directory
+                 * (unique for each JAR processed) to the set of thrift include directories to use when
+                 * compiling.
+                 */
                 for (JarEntry jarEntry : list(classpathJar.entries())) {
                     final String jarEntryName = jarEntry.getName();
                     if (jarEntry.getName().endsWith(THRIFT_FILE_SUFFIX)) {
+                        final String truncatedJarPath = truncatePath(classpathJar.getName());
+                        final File thriftRootDirectory = new File(temporaryThriftFileDirectory, truncatedJarPath);
                         final File uncompressedCopy =
-                                new File(new File(temporaryThriftFileDirectory,
-                                        truncatePath(classpathJar.getName())), jarEntryName);
+                                new File(thriftRootDirectory, jarEntryName);
                         uncompressedCopy.getParentFile().mkdirs();
                         copyStreamToFile(new RawInputStreamFacade(classpathJar
                                 .getInputStream(jarEntry)), uncompressedCopy);
-                        thriftDirectories.add(uncompressedCopy.getParentFile());
+                        thriftDirectories.add(thriftRootDirectory);
                     }
                 }
+
             } else if (classpathElementFile.isDirectory()) {
                 File[] thriftFiles = classpathElementFile.listFiles(new FilenameFilter() {
                     public boolean accept(File dir, String name) {
@@ -307,6 +318,7 @@ abstract class AbstractThriftMojo extends AbstractMojo {
                 }
             }
         }
+
         return ImmutableSet.copyOf(thriftDirectories);
     }
 
@@ -319,15 +331,6 @@ abstract class AbstractThriftMojo extends AbstractMojo {
         return ImmutableSet.copyOf(thriftFilesInDirectory);
     }
 
-    ImmutableSet<File> findThriftFilesInDirectories(Iterable<File> directories) throws IOException {
-        checkNotNull(directories);
-        Set<File> thriftFiles = newHashSet();
-        for (File directory : directories) {
-            thriftFiles.addAll(findThriftFilesInDirectory(directory));
-        }
-        return ImmutableSet.copyOf(thriftFiles);
-    }
-
     /**
      * Truncates the path of jar files so that they are relative to the local repository.
      *
diff --git a/contrib/thrift-maven-plugin/src/main/java/org/apache/thrift/maven/ThriftTestCompileMojo.java b/contrib/thrift-maven-plugin/src/main/java/org/apache/thrift/maven/ThriftTestCompileMojo.java
index fb89d966f..1b1d99e0d 100644
--- a/contrib/thrift-maven-plugin/src/main/java/org/apache/thrift/maven/ThriftTestCompileMojo.java
+++ b/contrib/thrift-maven-plugin/src/main/java/org/apache/thrift/maven/ThriftTestCompileMojo.java
@@ -23,6 +23,7 @@ import java.io.File;
 import java.util.List;
 import org.apache.maven.artifact.Artifact;
 import com.google.common.collect.ImmutableList;
+import org.apache.maven.artifact.repository.ArtifactRepository;
 
 /**
  * @phase generate-test-sources
@@ -71,4 +72,22 @@ public final class ThriftTestCompileMojo extends AbstractThriftMojo {
     protected File getThriftSourceRoot() {
         return thriftTestSourceRoot;
     }
+
+    /**
+     * Set the local maven ArtifactRepository. Exposed only to allow testing outside of Maven itself.
+     *
+     * @param localRepository local ArtifactRepository
+     */
+    public void setLocalMavenRepository(final ArtifactRepository localRepository) {
+        this.localRepository = localRepository;
+    }
+
+    /**
+     * Set the option to hash dependent JAR paths. Exposed only to allow testing outside of Maven itself.
+     *
+     * @param hashDependentPaths whether or not to hash paths to dependent JARs
+     */
+    public void setHashDependentPaths(final boolean hashDependentPaths) {
+        this.hashDependentPaths = hashDependentPaths;
+    }
 }
diff --git a/contrib/thrift-maven-plugin/src/test/java/org/apache/thrift/maven/TestAbstractThriftMojo.java b/contrib/thrift-maven-plugin/src/test/java/org/apache/thrift/maven/TestAbstractThriftMojo.java
new file mode 100644
index 000000000..c1176712d
--- /dev/null
+++ b/contrib/thrift-maven-plugin/src/test/java/org/apache/thrift/maven/TestAbstractThriftMojo.java
@@ -0,0 +1,102 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.apache.thrift.maven;
+
+import com.google.common.collect.Lists;
+import com.google.common.collect.Sets;
+import org.apache.maven.artifact.repository.ArtifactRepository;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+import java.io.File;
+import java.util.Set;
+
+import static junit.framework.TestCase.assertEquals;
+
+
+public class TestAbstractThriftMojo {
+
+    private ThriftTestCompileMojo mojo;
+    private File testRootDir;
+    private ArtifactRepository mavenRepository;
+
+
+    @Before
+    public void setUp() throws Exception {
+        final File tmpDir = new File(System.getProperty("java.io.tmpdir"));
+        testRootDir = new File(tmpDir, "thrift-test");
+
+        // the truncatePath method assumes a maven repository, but it only cares about the base dir
+        mavenRepository = Mockito.mock(ArtifactRepository.class);
+        Mockito.when(mavenRepository.getBasedir()).thenReturn("/test/maven/repo/basedir");
+
+        mojo = new ThriftTestCompileMojo();
+        mojo.setLocalMavenRepository(mavenRepository);
+    }
+
+    @Test
+    public void testMakeThriftPathFromJars() throws Throwable {
+        final File temporaryThriftFileDirectory = testRootDir;
+
+        // The SharedIdl.jar file contains the same idl/shared.thrift and idl/tutorial.thrift hierarchy
+        // used by other tests. It's used here to represent a dependency of the project maven is building,
+        // one that is contributing .thrift IDL files as well as any other artifacts.
+        final Iterable<File> classpathElementFiles = Lists.newArrayList(
+                new File("src/test/resources/dependency-jar-test/SharedIdl.jar")
+        );
+
+        final Set<File> thriftDirectories = mojo.makeThriftPathFromJars(temporaryThriftFileDirectory, classpathElementFiles);
+
+        // The results should be a path to a directory named after the JAR itself (assuming no path hashing,
+        // but see below for a separate test of that) representing the root of a hierarchy containing thrift
+        // files, suitable for providing to the thrift compiler as an include directory. In this case, that
+        // means it points to the directory containing the "idl" hierarchy rather than to the idl directory
+        // itself.
+        final Set<File> expected = Sets.newHashSet(
+                new File(testRootDir, "src/test/resources/dependency-jar-test/SharedIdl.jar")
+        );
+
+        assertEquals("makeThriftPathFromJars should return thrift IDL base path from within JAR", expected, thriftDirectories);
+    }
+
+    @Test
+    public void testTruncatePath() throws Throwable {
+        // JAR path is unrelated to maven repo, and should be unchanged
+        assertEquals("/path/to/somejar.jar", mojo.truncatePath("/path/to/somejar.jar"));
+
+        // JAR path is within maven repo, and should be made relative to the repo
+        assertEquals("path/to/somejar.jar", mojo.truncatePath("/test/maven/repo/basedir/path/to/somejar.jar"));
+
+        // JAR path contains forward slashes that should be normalized
+        assertEquals("/path/to/somejar.jar", mojo.truncatePath("\\path\\to\\somejar.jar"));
+    }
+
+    @Test
+    public void testTruncatePathWithDependentPathHashing() throws Throwable {
+        mojo.setHashDependentPaths(true);
+
+        // hashDependentPaths set to true, the JAR path is immediately hashed (MD5) and converted to a hex string
+
+        assertEquals("1c85950987b23493462cf3c261d9510a", mojo.truncatePath("/path/to/somejar.jar"));
+        assertEquals("39fc2b4c34cb6cb0da38bed5d8b5fc67", mojo.truncatePath("/test/maven/repo/basedir/path/to/somejar.jar"));
+        assertEquals("25b6924f5b0e19486d0ff88448e999d5", mojo.truncatePath("\\path\\to\\somejar.jar"));
+    }
+
+}