Repository: thrift Updated Branches: refs/heads/master fcf447679 -> f65db706b
THRIFT-3784: thrift-maven-plugin generates invalid include directories for IDL in dependency JARs Client: thrift-maven (contrib) This closes #984 Project: http://git-wip-us.apache.org/repos/asf/thrift/repo Commit: http://git-wip-us.apache.org/repos/asf/thrift/commit/f65db706 Tree: http://git-wip-us.apache.org/repos/asf/thrift/tree/f65db706 Diff: http://git-wip-us.apache.org/repos/asf/thrift/diff/f65db706 Branch: refs/heads/master Commit: f65db706b39ceb4898d6c78fe8a7a37501e02c13 Parents: fcf4476 Author: Chris Osborn <cosb...@conductor.com> Authored: Thu Apr 7 12:20:02 2016 -0400 Committer: James E. King, III <jk...@apache.org> Committed: Sat Apr 1 11:15:54 2017 -0400 ---------------------------------------------------------------------- contrib/thrift-maven-plugin/pom.xml | 6 ++ .../apache/thrift/maven/AbstractThriftMojo.java | 33 +++--- .../thrift/maven/ThriftTestCompileMojo.java | 19 ++++ .../thrift/maven/TestAbstractThriftMojo.java | 102 +++++++++++++++++++ 4 files changed, 145 insertions(+), 15 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/thrift/blob/f65db706/contrib/thrift-maven-plugin/pom.xml ---------------------------------------------------------------------- diff --git a/contrib/thrift-maven-plugin/pom.xml b/contrib/thrift-maven-plugin/pom.xml index 5bc1004..0af5957 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> http://git-wip-us.apache.org/repos/asf/thrift/blob/f65db706/contrib/thrift-maven-plugin/src/main/java/org/apache/thrift/maven/AbstractThriftMojo.java ---------------------------------------------------------------------- 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 0913c77..869be95 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. * http://git-wip-us.apache.org/repos/asf/thrift/blob/f65db706/contrib/thrift-maven-plugin/src/main/java/org/apache/thrift/maven/ThriftTestCompileMojo.java ---------------------------------------------------------------------- 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 fb89d96..1b1d99e 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; + } } http://git-wip-us.apache.org/repos/asf/thrift/blob/f65db706/contrib/thrift-maven-plugin/src/test/java/org/apache/thrift/maven/TestAbstractThriftMojo.java ---------------------------------------------------------------------- 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 0000000..c117671 --- /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")); + } + +}
