Add group discriminant in case of conflict

Prior to this commit, the repackage goal silently ignored the case of
two libraries having the same name and version but a different group.
As a result, the second library was overwriting the first one in the
repackaged jar.

This commit adds support for custom Library names and updates the
Maven and Gradle plugins so that the name includes the group ID
when there would otherwise be a duplicate.

Fixes gh-1475

spring-boot-samples/spring-boot-sample-simple/application.properties

@@ -0,0 +1 @@
+name: Phil

spring-boot-tools/spring-boot-gradle-plugin/src/main/groovy/org/springframework/boot/gradle/repackage/ProjectLibraries.java

@@ -75,18 +75,18 @@ class ProjectLibraries implements Libraries {
 
 	@Override
 	public void doWithLibraries(LibraryCallback callback) throws IOException {
-		Set<Library> custom = getLibraries(this.customConfigurationName,
+		Set<GradleLibrary> custom = getLibraries(this.customConfigurationName,
 				LibraryScope.CUSTOM);
 		if (custom != null) {
 			libraries(custom, callback);
 		}
 		else {
-			Set<Library> compile = getLibraries("compile", LibraryScope.COMPILE);
+			Set<GradleLibrary> compile = getLibraries("compile", LibraryScope.COMPILE);
 
-			Set<Library> runtime = getLibraries("runtime", LibraryScope.RUNTIME);
+			Set<GradleLibrary> runtime = getLibraries("runtime", LibraryScope.RUNTIME);
 			runtime = minus(runtime, compile);
 
-			Set<Library> provided = getLibraries(this.providedConfigurationName,
+			Set<GradleLibrary> provided = getLibraries(this.providedConfigurationName,
 					LibraryScope.PROVIDED);
 			if (provided != null) {
 				compile = minus(compile, provided);
@@ -99,13 +99,13 @@ class ProjectLibraries implements Libraries {
 		}
 	}
 
-	private Set<Library> getLibraries(String configurationName, LibraryScope scope) {
+	private Set<GradleLibrary> getLibraries(String configurationName, LibraryScope scope) {
 		Configuration configuration = (configurationName == null ? null : this.project
 				.getConfigurations().findByName(configurationName));
 		if (configuration == null) {
 			return null;
 		}
-		Set<Library> libraries = new LinkedHashSet<Library>();
+		Set<GradleLibrary> libraries = new LinkedHashSet<GradleLibrary>();
 		for (ResolvedArtifact artifact : configuration.getResolvedConfiguration()
 				.getResolvedArtifacts()) {
 			libraries.add(new ResolvedArtifactLibrary(artifact, scope));
@@ -115,14 +115,14 @@ class ProjectLibraries implements Libraries {
 		return libraries;
 	}
 
-	private Set<Library> getLibrariesForFileDependencies(Configuration configuration,
+	private Set<GradleLibrary> getLibrariesForFileDependencies(Configuration configuration,
 			LibraryScope scope) {
-		Set<Library> libraries = new LinkedHashSet<Library>();
+		Set<GradleLibrary> libraries = new LinkedHashSet<GradleLibrary>();
 		for (Dependency dependency : configuration.getIncoming().getDependencies()) {
 			if (dependency instanceof FileCollectionDependency) {
 				FileCollectionDependency fileDependency = (FileCollectionDependency) dependency;
 				for (File file : fileDependency.resolve()) {
-					libraries.add(new Library(file, scope));
+					libraries.add(new GradleLibrary(fileDependency.getGroup(), file, scope));
 				}
 			}
 			else if (dependency instanceof ProjectDependency) {
@@ -134,16 +134,16 @@ class ProjectLibraries implements Libraries {
 		return libraries;
 	}
 
-	private Set<Library> minus(Set<Library> source, Set<Library> toRemove) {
+	private Set<GradleLibrary> minus(Set<GradleLibrary> source, Set<GradleLibrary> toRemove) {
 		if (source == null || toRemove == null) {
 			return source;
 		}
 		Set<File> filesToRemove = new HashSet<File>();
-		for (Library library : toRemove) {
+		for (GradleLibrary library : toRemove) {
 			filesToRemove.add(library.getFile());
 		}
-		Set<Library> result = new LinkedHashSet<Library>();
-		for (Library library : source) {
+		Set<GradleLibrary> result = new LinkedHashSet<GradleLibrary>();
+		for (GradleLibrary library : source) {
 			if (!filesToRemove.contains(library.getFile())) {
 				result.add(library);
 			}
@@ -151,24 +151,63 @@ class ProjectLibraries implements Libraries {
 		return result;
 	}
 
-	private void libraries(Set<Library> libraries, LibraryCallback callback)
+	private void libraries(Set<GradleLibrary> libraries, LibraryCallback callback)
 			throws IOException {
 		if (libraries != null) {
-			for (Library library : libraries) {
+			Set<String> duplicates = getDuplicates(libraries);
+			for (GradleLibrary library : libraries) {
+				library.setIncludeGroupName(duplicates.contains(library.getName()));
 				callback.library(library);
 			}
 		}
 	}
 
+	private Set<String> getDuplicates(Set<GradleLibrary> libraries) {
+		Set<String> duplicates = new HashSet<String>();
+		Set<String> seen = new HashSet<String>();
+		for (GradleLibrary library : libraries) {
+			if (library.getFile() != null && !seen.add(library.getFile().getName())) {
+				duplicates.add(library.getFile().getName());
+			}
+		}
+		return duplicates;
+	}
+
+	private class GradleLibrary extends Library {
+
+		private final String group;
+
+		private boolean includeGroupName;
+
+		public GradleLibrary(String group, File file, LibraryScope scope) {
+			super(file, scope);
+			this.group = group;
+		}
+
+		public void setIncludeGroupName(boolean includeGroupName) {
+			this.includeGroupName = includeGroupName;
+		}
+
+		@Override
+		public String getName() {
+			String name = super.getName();
+			if(this.includeGroupName && this.group != null) {
+				name = this.group + "-" + name;
+			}
+			return name;
+		}
+
+	}
+
 	/**
 	 * Adapts a {@link ResolvedArtifact} to a {@link Library}.
 	 */
-	private class ResolvedArtifactLibrary extends Library {
+	private class ResolvedArtifactLibrary extends GradleLibrary {
 
 		private final ResolvedArtifact artifact;
 
 		public ResolvedArtifactLibrary(ResolvedArtifact artifact, LibraryScope scope) {
-			super(artifact.getFile(), scope);
+			super(null, artifact.getFile(), scope);
 			this.artifact = artifact;
 		}
 

spring-boot-tools/spring-boot-loader-tools/src/main/java/org/springframework/boot/loader/tools/JarWriter.java

@@ -128,7 +128,7 @@ public class JarWriter {
 	public void writeNestedLibrary(String destination, Library library)
 			throws IOException {
 		File file = library.getFile();
-		JarEntry entry = new JarEntry(destination + file.getName());
+		JarEntry entry = new JarEntry(destination + library.getName());
 		if (library.isUnpackRequired()) {
 			entry.setComment("UNPACK:" + FileUtils.sha1Hash(file));
 		}

spring-boot-tools/spring-boot-loader-tools/src/main/java/org/springframework/boot/loader/tools/Library.java

@@ -27,6 +27,8 @@ import java.io.File;
  */
 public class Library {
 
+	private final String name;
+
 	private final File file;
 
 	private final LibraryScope scope;
@@ -49,11 +51,31 @@ public class Library {
 	 * @param unpackRequired if the library needs to be unpacked before it can be used
 	 */
 	public Library(File file, LibraryScope scope, boolean unpackRequired) {
+		this(null, file, scope, unpackRequired);
+	}
+
+	/**
+	 * Create a new {@link Library}.
+	 * @param name the name of the library as it should be written or {@code null} to use
+	 * the file name
+	 * @param file the source file
+	 * @param scope the scope of the library
+	 * @param unpackRequired if the library needs to be unpacked before it can be used
+	 */
+	public Library(String name, File file, LibraryScope scope, boolean unpackRequired) {
+		this.name = (name == null ? file.getName() : name);
 		this.file = file;
 		this.scope = scope;
 		this.unpackRequired = unpackRequired;
 	}
 
+	/**
+	 * @return the name of file as it should be written
+	 */
+	public String getName() {
+		return this.name;
+	}
+
 	/**
 	 * @return the library file
 	 */

spring-boot-tools/spring-boot-loader-tools/src/main/java/org/springframework/boot/loader/tools/Repackager.java

@@ -20,6 +20,8 @@ import java.io.File;
 import java.io.FileInputStream;
 import java.io.IOException;
 import java.io.InputStream;
+import java.util.HashSet;
+import java.util.Set;
 import java.util.jar.JarFile;
 import java.util.jar.Manifest;
 
@@ -153,17 +155,22 @@ public class Repackager {
 			throws IOException {
 		final JarWriter writer = new JarWriter(destination);
 		try {
+			final Set<String> seen = new HashSet<String>();
 			writer.writeManifest(buildManifest(sourceJar));
 			writer.writeEntries(sourceJar);
-
 			libraries.doWithLibraries(new LibraryCallback() {
 				@Override
 				public void library(Library library) throws IOException {
 					File file = library.getFile();
 					if (isZip(file)) {
 						String destination = Repackager.this.layout
-								.getLibraryDestination(file.getName(), library.getScope());
+								.getLibraryDestination(library.getName(),
+										library.getScope());
 						if (destination != null) {
+							if (!seen.add(destination + library.getName())) {
+								throw new IllegalStateException("Duplicate library "
+										+ library.getName());
+							}
 							writer.writeNestedLibrary(destination, library);
 						}
 					}

spring-boot-tools/spring-boot-loader-tools/src/test/java/org/springframework/boot/loader/tools/RepackagerTests.java

@@ -299,6 +299,25 @@ public class RepackagerTests {
 		assertThat(entry.getComment().length(), equalTo(47));
 	}
 
+	@Test
+	public void duplicateLibraries() throws Exception {
+		TestJarFile libJar = new TestJarFile(this.temporaryFolder);
+		libJar.addClass("a/b/C.class", ClassWithoutMainMethod.class);
+		final File libJarFile = libJar.getFile();
+		this.testJarFile.addClass("a/b/C.class", ClassWithMainMethod.class);
+		File file = this.testJarFile.getFile();
+		Repackager repackager = new Repackager(file);
+		this.thrown.expect(IllegalStateException.class);
+		this.thrown.expectMessage("Duplicate library");
+		repackager.repackage(new Libraries() {
+			@Override
+			public void doWithLibraries(LibraryCallback callback) throws IOException {
+				callback.library(new Library(libJarFile, LibraryScope.COMPILE, false));
+				callback.library(new Library(libJarFile, LibraryScope.COMPILE, false));
+			}
+		});
+	}
+
 	@Test
 	public void customLayout() throws Exception {
 		TestJarFile libJar = new TestJarFile(this.temporaryFolder);

spring-boot-tools/spring-boot-maven-plugin/src/it/jar-lib-name-conflict/acme-lib/pom.xml

@@ -0,0 +1,20 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<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">
+	<modelVersion>4.0.0</modelVersion>
+	<artifactId>acme-lib</artifactId>
+
+	<parent>
+		<groupId>org.springframework.boot.maven.it</groupId>
+		<artifactId>jar-lib-name-conflict</artifactId>
+		<version>0.0.1.BUILD-SNAPSHOT</version>
+	</parent>
+
+	<dependencies>
+		<dependency>
+			<groupId>org.springframework</groupId>
+			<artifactId>spring-context</artifactId>
+			<version>4.0.5.RELEASE</version>
+		</dependency>
+	</dependencies>
+</project>

spring-boot-tools/spring-boot-maven-plugin/src/it/jar-lib-name-conflict/another-acme-lib/pom.xml

@@ -0,0 +1,14 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<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">
+	<modelVersion>4.0.0</modelVersion>
+	<groupId>org.springframework.boot.maven.it.another</groupId>
+	<artifactId>acme-lib</artifactId>
+
+	<parent>
+		<groupId>org.springframework.boot.maven.it</groupId>
+		<artifactId>jar-lib-name-conflict</artifactId>
+		<version>0.0.1.BUILD-SNAPSHOT</version>
+	</parent>
+
+</project>

spring-boot-tools/spring-boot-maven-plugin/src/it/jar-lib-name-conflict/pom.xml

@@ -0,0 +1,19 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<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">
+	<modelVersion>4.0.0</modelVersion>
+	<groupId>org.springframework.boot.maven.it</groupId>
+	<artifactId>jar-lib-name-conflict</artifactId>
+	<version>0.0.1.BUILD-SNAPSHOT</version>
+	<packaging>pom</packaging>
+	<properties>
+		<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
+	</properties>
+
+	<modules>
+		<module>acme-lib</module>
+		<module>another-acme-lib</module>
+		<module>test-project</module>
+	</modules>
+
+</project>

spring-boot-tools/spring-boot-maven-plugin/src/it/jar-lib-name-conflict/test-project/pom.xml

@@ -0,0 +1,58 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<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">
+	<modelVersion>4.0.0</modelVersion>
+	<groupId>org.springframework.boot.maven.it</groupId>
+	<artifactId>test-project</artifactId>
+	<version>0.0.1.BUILD-SNAPSHOT</version>
+	<properties>
+		<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
+	</properties>
+	<build>
+		<plugins>
+			<plugin>
+				<groupId>@project.groupId@</groupId>
+				<artifactId>@project.artifactId@</artifactId>
+				<version>@project.version@</version>
+				<executions>
+					<execution>
+						<goals>
+							<goal>repackage</goal>
+						</goals>
+					</execution>
+				</executions>
+			</plugin>
+			<plugin>
+				<groupId>org.apache.maven.plugins</groupId>
+				<artifactId>maven-jar-plugin</artifactId>
+				<version>2.4</version>
+				<configuration>
+					<archive>
+						<manifestEntries>
+							<Not-Used>Foo</Not-Used>
+						</manifestEntries>
+					</archive>
+				</configuration>
+			</plugin>
+		</plugins>
+	</build>
+	<dependencies>
+		<!-- Two dependencies with the same artifactId -->
+		<dependency>
+			<groupId>org.springframework.boot.maven.it</groupId>
+			<artifactId>acme-lib</artifactId>
+			<version>0.0.1.BUILD-SNAPSHOT</version>
+		</dependency>
+		<dependency>
+			<groupId>org.springframework.boot.maven.it.another</groupId>
+			<artifactId>acme-lib</artifactId>
+			<version>0.0.1.BUILD-SNAPSHOT</version>
+		</dependency>
+		<dependency>
+			<groupId>javax.servlet</groupId>
+			<artifactId>javax.servlet-api</artifactId>
+			<version>3.0.1</version>
+			<scope>provided</scope>
+		</dependency>
+	</dependencies>
+</project>

spring-boot-tools/spring-boot-maven-plugin/src/it/jar-lib-name-conflict/test-project/src/main/java/org/test/SampleApplication.java

@@ -0,0 +1,8 @@
+package org.test;
+
+public class SampleApplication {
+
+	public static void main(String[] args) {
+	}
+
+}

spring-boot-tools/spring-boot-maven-plugin/src/it/jar-lib-name-conflict/verify.groovy

@@ -0,0 +1,14 @@
+import java.io.*;
+import org.springframework.boot.maven.*;
+
+File f = new File(basedir, "test-project/target/test-project-0.0.1.BUILD-SNAPSHOT.jar")
+new Verify.JarArchiveVerification(f, Verify.SAMPLE_APP) {
+	@Override
+	protected void verifyZipEntries(Verify.ArchiveVerifier verifier) throws Exception {
+		super.verifyZipEntries(verifier)
+		verifier.assertHasEntryNameStartingWith("lib/org.springframework.boot.maven.it-acme-lib-0.0.1.BUILD-SNAPSHOT.jar")
+		verifier.assertHasEntryNameStartingWith("lib/org.springframework.boot.maven.it.another-acme-lib-0.0.1.BUILD-SNAPSHOT.jar")
+	}
+}.verify();
+
+

spring-boot-tools/spring-boot-maven-plugin/src/main/java/org/springframework/boot/maven/ArtifactsLibraries.java

@@ -20,11 +20,13 @@ import java.io.IOException;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.Map;
 import java.util.Set;
 
 import org.apache.maven.artifact.Artifact;
 import org.apache.maven.model.Dependency;
+import org.apache.maven.plugin.logging.Log;
 import org.springframework.boot.loader.tools.Libraries;
 import org.springframework.boot.loader.tools.Library;
 import org.springframework.boot.loader.tools.LibraryCallback;
@@ -51,22 +53,44 @@ public class ArtifactsLibraries implements Libraries {
 
 	private final Collection<Dependency> unpacks;
 
-	public ArtifactsLibraries(Set<Artifact> artifacts, Collection<Dependency> unpacks) {
+	private final Log log;
+
+	public ArtifactsLibraries(Set<Artifact> artifacts, Collection<Dependency> unpacks,
+			Log log) {
 		this.artifacts = artifacts;
 		this.unpacks = unpacks;
+		this.log = log;
 	}
 
 	@Override
 	public void doWithLibraries(LibraryCallback callback) throws IOException {
+		Set<String> duplicates = getDuplicates(this.artifacts);
 		for (Artifact artifact : this.artifacts) {
 			LibraryScope scope = SCOPES.get(artifact.getScope());
 			if (scope != null && artifact.getFile() != null) {
-				callback.library(new Library(artifact.getFile(), scope,
+				String name = artifact.getFile().getName();
+				if (duplicates.contains(name)) {
+					this.log.debug("Duplicate found: " + name);
+					name = artifact.getGroupId() + "-" + name;
+					this.log.debug("Renamed to: " + name);
+				}
+				callback.library(new Library(name, artifact.getFile(), scope,
 						isUnpackRequired(artifact)));
 			}
 		}
 	}
 
+	private Set<String> getDuplicates(Set<Artifact> artifacts) {
+		Set<String> duplicates = new HashSet<String>();
+		Set<String> seen = new HashSet<String>();
+		for (Artifact artifact : artifacts) {
+			if (artifact.getFile() != null && !seen.add(artifact.getFile().getName())) {
+				duplicates.add(artifact.getFile().getName());
+			}
+		}
+		return duplicates;
+	}
+
 	private boolean isUnpackRequired(Artifact artifact) {
 		if (this.unpacks != null) {
 			for (Dependency unpack : this.unpacks) {
@@ -78,4 +102,5 @@ public class ArtifactsLibraries implements Libraries {
 		}
 		return false;
 	}
+
 }

spring-boot-tools/spring-boot-maven-plugin/src/main/java/org/springframework/boot/maven/RepackageMojo.java

@@ -153,7 +153,8 @@ public class RepackageMojo extends AbstractDependencyFilterMojo {
 		Set<Artifact> artifacts = filterDependencies(this.project.getArtifacts(),
 				getFilters());
 
-		Libraries libraries = new ArtifactsLibraries(artifacts, this.requiresUnpack);
+		Libraries libraries = new ArtifactsLibraries(artifacts, this.requiresUnpack,
+				getLog());
 		try {
 			repackager.repackage(target, libraries);
 		}

spring-boot-tools/spring-boot-maven-plugin/src/test/java/org/springframework/boot/maven/ArtifactsLibrariesTests.java

@@ -17,11 +17,14 @@
 package org.springframework.boot.maven;
 
 import java.io.File;
+import java.util.Arrays;
 import java.util.Collections;
+import java.util.LinkedHashSet;
 import java.util.Set;
 
 import org.apache.maven.artifact.Artifact;
 import org.apache.maven.model.Dependency;
+import org.apache.maven.plugin.logging.Log;
 import org.junit.Before;
 import org.junit.Test;
 import org.mockito.ArgumentCaptor;
@@ -35,6 +38,8 @@ import org.springframework.boot.loader.tools.LibraryScope;
 import static org.hamcrest.Matchers.equalTo;
 import static org.junit.Assert.assertThat;
 import static org.mockito.BDDMockito.given;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 
 /**
@@ -63,7 +68,7 @@ public class ArtifactsLibrariesTests {
 	public void setup() {
 		MockitoAnnotations.initMocks(this);
 		this.artifacts = Collections.singleton(this.artifact);
-		this.libs = new ArtifactsLibraries(this.artifacts, null);
+		this.libs = new ArtifactsLibraries(this.artifacts, null, mock(Log.class));
 		given(this.artifact.getFile()).willReturn(this.file);
 	}
 
@@ -88,9 +93,31 @@ public class ArtifactsLibrariesTests {
 		Dependency unpack = new Dependency();
 		unpack.setGroupId("gid");
 		unpack.setArtifactId("aid");
-		this.libs = new ArtifactsLibraries(this.artifacts, Collections.singleton(unpack));
+		this.libs = new ArtifactsLibraries(this.artifacts, Collections.singleton(unpack),
+				mock(Log.class));
 		this.libs.doWithLibraries(this.callback);
 		verify(this.callback).library(this.libraryCaptor.capture());
 		assertThat(this.libraryCaptor.getValue().isUnpackRequired(), equalTo(true));
 	}
+
+	@Test
+	public void renamesDuplicates() throws Exception {
+		Artifact artifact1 = mock(Artifact.class);
+		Artifact artifact2 = mock(Artifact.class);
+		given(artifact1.getType()).willReturn("jar");
+		given(artifact1.getScope()).willReturn("compile");
+		given(artifact1.getGroupId()).willReturn("g1");
+		given(artifact1.getFile()).willReturn(new File("a"));
+		given(artifact2.getType()).willReturn("jar");
+		given(artifact2.getScope()).willReturn("compile");
+		given(artifact2.getGroupId()).willReturn("g2");
+		given(artifact2.getFile()).willReturn(new File("a"));
+		this.artifacts = new LinkedHashSet<Artifact>(Arrays.asList(artifact1, artifact2));
+		this.libs = new ArtifactsLibraries(this.artifacts, null, mock(Log.class));
+		this.libs.doWithLibraries(this.callback);
+		verify(this.callback, times(2)).library(this.libraryCaptor.capture());
+		assertThat(this.libraryCaptor.getAllValues().get(0).getName(), equalTo("g1-a"));
+		assertThat(this.libraryCaptor.getAllValues().get(1).getName(), equalTo("g2-a"));
+	}
+
 }