Rework entry ordering of repackaged archives

Previously, the Repackager would write entries in the following
order:

- Libraries that require unpacking
- Existing entries
    - Application classes
    - WEB-INF/lib jars in a war
- Libraries that do not require unpacking
- Loader classes

Libraries that require unpacking were written before existing entries
so that, when repackaging a war, an entry in WEB-INF/lib would not
get in first and prevent a library with same location from being
unpacked. However, this had the unwanted side-effect of changing
the classpath order when an entry requires unpacking.

This commit reworks the handling of existing entries and libraries
that require unpacking so that existing entries can be written first
while also marking any that match a library that requires unpacking
as requiring unpacking.

Additionally, loader classes are now written first. They are the
first classes in the jar that will be used so it seems to make sense
for them to appear first. This aligns Maven-based repackaging
with the Gradle plugin's behaviour and with the structure documented
in the reference documentation's "The Executable Jar Format" appendix.

The net result of the changes described above is that entries are
now written in the following order:

- Loader classes
- Existing entries
    - Application classes
    - WEB-INF/lib jars in a war marked for unpacking if needed
- Libraries

Closes gh-11695
Closes gh-11696

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

@@ -18,6 +18,7 @@ package org.springframework.boot.loader.tools;
 
 import java.io.BufferedInputStream;
 import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.FileNotFoundException;
@@ -54,6 +55,8 @@ import org.apache.commons.compress.archivers.zip.UnixStat;
  */
 public class JarWriter implements LoaderClassesWriter, AutoCloseable {
 
+	private static final UnpackHandler NEVER_UNPACK = new NeverUnpackHandler();
+
 	private static final String NESTED_LOADER_JAR = "META-INF/loader/spring-boot-loader.jar";
 
 	private static final int BUFFER_SIZE = 32 * 1024;
@@ -119,11 +122,15 @@ public class JarWriter implements LoaderClassesWriter, AutoCloseable {
 	 * @throws IOException if the entries cannot be written
 	 */
 	public void writeEntries(JarFile jarFile) throws IOException {
-		this.writeEntries(jarFile, new IdentityEntryTransformer());
+		this.writeEntries(jarFile, new IdentityEntryTransformer(), NEVER_UNPACK);
 	}
 
-	void writeEntries(JarFile jarFile, EntryTransformer entryTransformer)
-			throws IOException {
+	void writeEntries(JarFile jarFile, UnpackHandler unpackHandler) throws IOException {
+		this.writeEntries(jarFile, new IdentityEntryTransformer(), unpackHandler);
+	}
+
+	void writeEntries(JarFile jarFile, EntryTransformer entryTransformer,
+			UnpackHandler unpackHandler) throws IOException {
 		Enumeration<JarEntry> entries = jarFile.entries();
 		while (entries.hasMoreElements()) {
 			JarArchiveEntry entry = new JarArchiveEntry(entries.nextElement());
@@ -133,7 +140,7 @@ public class JarWriter implements LoaderClassesWriter, AutoCloseable {
 				EntryWriter entryWriter = new InputStreamEntryWriter(inputStream, true);
 				JarArchiveEntry transformedEntry = entryTransformer.transform(entry);
 				if (transformedEntry != null) {
-					writeEntry(transformedEntry, entryWriter);
+					writeEntry(transformedEntry, entryWriter, unpackHandler);
 				}
 			}
 		}
@@ -172,11 +179,9 @@ public class JarWriter implements LoaderClassesWriter, AutoCloseable {
 		File file = library.getFile();
 		JarArchiveEntry entry = new JarArchiveEntry(destination + library.getName());
 		entry.setTime(getNestedLibraryTime(file));
-		if (library.isUnpackRequired()) {
-			entry.setComment("UNPACK:" + FileUtils.sha1Hash(file));
-		}
 		new CrcAndSize(file).setupStoredEntry(entry);
-		writeEntry(entry, new InputStreamEntryWriter(new FileInputStream(file), true));
+		writeEntry(entry, new InputStreamEntryWriter(new FileInputStream(file), true),
+				new LibraryUnpackHandler(library));
 	}
 
 	private long getNestedLibraryTime(File file) {
@@ -236,15 +241,21 @@ public class JarWriter implements LoaderClassesWriter, AutoCloseable {
 		this.jarOutput.close();
 	}
 
+	private void writeEntry(JarArchiveEntry entry, EntryWriter entryWriter)
+			throws IOException {
+		writeEntry(entry, entryWriter, NEVER_UNPACK);
+	}
+
 	/**
-	 * Perform the actual write of a {@link JarEntry}. All other {@code write} method
+	 * Perform the actual write of a {@link JarEntry}. All other {@code write} methods
 	 * delegate to this one.
 	 * @param entry the entry to write
 	 * @param entryWriter the entry writer or {@code null} if there is no content
+	 * @param unpackHandler handles possible unpacking for the entry
 	 * @throws IOException in case of I/O errors
 	 */
-	private void writeEntry(JarArchiveEntry entry, EntryWriter entryWriter)
-			throws IOException {
+	private void writeEntry(JarArchiveEntry entry, EntryWriter entryWriter,
+			UnpackHandler unpackHandler) throws IOException {
 		String parent = entry.getName();
 		if (parent.endsWith("/")) {
 			parent = parent.substring(0, parent.length() - 1);
@@ -256,11 +267,12 @@ public class JarWriter implements LoaderClassesWriter, AutoCloseable {
 		if (parent.lastIndexOf('/') != -1) {
 			parent = parent.substring(0, parent.lastIndexOf('/') + 1);
 			if (!parent.isEmpty()) {
-				writeEntry(new JarArchiveEntry(parent), null);
+				writeEntry(new JarArchiveEntry(parent), null, unpackHandler);
 			}
 		}
 
 		if (this.writtenEntries.add(entry.getName())) {
+			entryWriter = addUnpackCommentIfNecessary(entry, entryWriter, unpackHandler);
 			this.jarOutput.putArchiveEntry(entry);
 			if (entryWriter != null) {
 				entryWriter.write(this.jarOutput);
@@ -269,6 +281,18 @@ public class JarWriter implements LoaderClassesWriter, AutoCloseable {
 		}
 	}
 
+	private EntryWriter addUnpackCommentIfNecessary(JarArchiveEntry entry,
+			EntryWriter entryWriter, UnpackHandler unpackHandler) throws IOException {
+		if (entryWriter == null || !unpackHandler.requiresUnpack(entry.getName())) {
+			return entryWriter;
+		}
+		ByteArrayOutputStream output = new ByteArrayOutputStream();
+		entryWriter.write(output);
+		entry.setComment("UNPACK:" + unpackHandler.sha1Hash(entry.getName()));
+		return new InputStreamEntryWriter(new ByteArrayInputStream(output.toByteArray()),
+				true);
+	}
+
 	/**
 	 * Interface used to write jar entry date.
 	 */
@@ -421,4 +445,50 @@ public class JarWriter implements LoaderClassesWriter, AutoCloseable {
 
 	}
 
+	/**
+	 * An {@code UnpackHandler} determines whether or not unpacking is required and
+	 * provides a SHA1 hash if required.
+	 */
+	interface UnpackHandler {
+
+		boolean requiresUnpack(String name);
+
+		String sha1Hash(String name) throws IOException;
+
+	}
+
+	private static final class NeverUnpackHandler implements UnpackHandler {
+
+		@Override
+		public boolean requiresUnpack(String name) {
+			return false;
+		}
+
+		@Override
+		public String sha1Hash(String name) {
+			throw new UnsupportedOperationException();
+		}
+
+	}
+
+	private static final class LibraryUnpackHandler implements UnpackHandler {
+
+		private final Library library;
+
+		private LibraryUnpackHandler(Library library) {
+			this.library = library;
+		}
+
+		@Override
+		public boolean requiresUnpack(String name) {
+			return this.library.isUnpackRequired();
+		}
+
+		@Override
+		public String sha1Hash(String name) throws IOException {
+			return FileUtils.sha1Hash(this.library.getFile());
+		}
+
+	}
+
 }

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2012-2017 the original author or authors.
+ * Copyright 2012-2018 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -21,9 +21,10 @@ import java.io.FileInputStream;
 import java.io.IOException;
 import java.io.InputStream;
 import java.util.ArrayList;
-import java.util.HashSet;
+import java.util.LinkedHashMap;
 import java.util.List;
-import java.util.Set;
+import java.util.Map;
+import java.util.Map.Entry;
 import java.util.concurrent.TimeUnit;
 import java.util.jar.JarFile;
 import java.util.jar.Manifest;
@@ -31,6 +32,7 @@ import java.util.jar.Manifest;
 import org.apache.commons.compress.archivers.jar.JarArchiveEntry;
 
 import org.springframework.boot.loader.tools.JarWriter.EntryTransformer;
+import org.springframework.boot.loader.tools.JarWriter.UnpackHandler;
 import org.springframework.core.io.support.SpringFactoriesLoader;
 import org.springframework.util.Assert;
 import org.springframework.util.StringUtils;
@@ -231,53 +233,19 @@ public class Repackager {
 
 	private void repackage(JarFile sourceJar, File destination, Libraries libraries,
 			LaunchScript launchScript) throws IOException {
+		WritableLibraries writeableLibraries = new WritableLibraries(libraries);
 		try (JarWriter writer = new JarWriter(destination, launchScript)) {
-			final List<Library> unpackLibraries = new ArrayList<>();
-			final List<Library> standardLibraries = new ArrayList<>();
-			libraries.doWithLibraries((library) -> {
-				File file = library.getFile();
-				if (isZip(file)) {
-					if (library.isUnpackRequired()) {
-						unpackLibraries.add(library);
-					}
-					else {
-						standardLibraries.add(library);
-					}
-				}
-			});
-			repackage(sourceJar, writer, unpackLibraries, standardLibraries);
-		}
-	}
-
-	private void repackage(JarFile sourceJar, JarWriter writer,
-			final List<Library> unpackLibraries, final List<Library> standardLibraries)
-					throws IOException {
-		writer.writeManifest(buildManifest(sourceJar));
-		Set<String> seen = new HashSet<>();
-		writeNestedLibraries(unpackLibraries, seen, writer);
-		if (this.layout instanceof RepackagingLayout) {
-			writer.writeEntries(sourceJar, new RenamingEntryTransformer(
-					((RepackagingLayout) this.layout).getRepackagedClassesLocation()));
-		}
-		else {
-			writer.writeEntries(sourceJar);
-		}
-		writeNestedLibraries(standardLibraries, seen, writer);
-		writeLoaderClasses(writer);
-	}
-
-	private void writeNestedLibraries(List<Library> libraries, Set<String> alreadySeen,
-			JarWriter writer) throws IOException {
-		for (Library library : libraries) {
-			String destination = Repackager.this.layout
-					.getLibraryDestination(library.getName(), library.getScope());
-			if (destination != null) {
-				if (!alreadySeen.add(destination + library.getName())) {
-					throw new IllegalStateException(
-							"Duplicate library " + library.getName());
-				}
-				writer.writeNestedLibrary(destination, library);
+			writer.writeManifest(buildManifest(sourceJar));
+			writeLoaderClasses(writer);
+			if (this.layout instanceof RepackagingLayout) {
+				writer.writeEntries(sourceJar, new RenamingEntryTransformer(
+						((RepackagingLayout) this.layout).getRepackagedClassesLocation()),
+						writeableLibraries);
 			}
+			else {
+				writer.writeEntries(sourceJar, writeableLibraries);
+			}
+			writeableLibraries.write(writer);
 		}
 	}
 
@@ -443,4 +411,54 @@ public class Repackager {
 
 	}
 
+	/**
+	 * An {@link UnpackHandler} that determines that an entry needs to be unpacked if a
+	 * library that requires unpacking has a matching entry name.
+	 */
+	private final class WritableLibraries implements UnpackHandler {
+
+		private final Map<String, Library> libraryEntryNames = new LinkedHashMap<>();
+
+		private WritableLibraries(Libraries libraries) throws IOException {
+			libraries.doWithLibraries((library) -> {
+				if (isZip(library.getFile())) {
+					String libraryDestination = Repackager.this.layout
+							.getLibraryDestination(library.getName(), library.getScope())
+							+ library.getName();
+					Library existing = this.libraryEntryNames
+							.putIfAbsent(libraryDestination, library);
+					if (existing != null) {
+						throw new IllegalStateException(
+								"Duplicate library " + library.getName());
+					}
+				}
+			});
+		}
+
+		@Override
+		public boolean requiresUnpack(String name) {
+			Library library = this.libraryEntryNames.get(name);
+			return library != null && library.isUnpackRequired();
+		}
+
+		@Override
+		public String sha1Hash(String name) throws IOException {
+			Library library = this.libraryEntryNames.get(name);
+			if (library == null) {
+				throw new IllegalArgumentException(
+						"No library found for entry name '" + name + "'");
+			}
+			return FileUtils.sha1Hash(library.getFile());
+		}
+
+		private void write(JarWriter writer) throws IOException {
+			for (Entry<String, Library> entry : this.libraryEntryNames.entrySet()) {
+				writer.writeNestedLibrary(
+						entry.getKey().substring(0, entry.getKey().lastIndexOf('/') + 1),
+						entry.getValue());
+			}
+		}
+
+	}
+
 }

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2012-2017 the original author or authors.
+ * Copyright 2012-2018 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -21,8 +21,10 @@ import java.io.File;
 import java.io.IOException;
 import java.nio.file.Files;
 import java.nio.file.attribute.PosixFilePermission;
+import java.util.ArrayList;
 import java.util.Calendar;
 import java.util.Enumeration;
+import java.util.List;
 import java.util.jar.Attributes;
 import java.util.jar.JarEntry;
 import java.util.jar.JarFile;
@@ -575,6 +577,58 @@ public class RepackagerTests {
 		}
 	}
 
+	@Test
+	public void loaderIsWrittenFirstThenApplicationClassesThenLibraries()
+			throws IOException {
+		this.testJarFile.addClass("com/example/Application.class",
+				ClassWithMainMethod.class);
+		File source = this.testJarFile.getFile();
+		File dest = this.temporaryFolder.newFile("dest.jar");
+		File libraryOne = createLibrary();
+		File libraryTwo = createLibrary();
+		File libraryThree = createLibrary();
+		Repackager repackager = new Repackager(source);
+		repackager.repackage(dest, (callback) -> {
+			callback.library(new Library(libraryOne, LibraryScope.COMPILE, false));
+			callback.library(new Library(libraryTwo, LibraryScope.COMPILE, true));
+			callback.library(new Library(libraryThree, LibraryScope.COMPILE, false));
+		});
+		assertThat(getEntryNames(dest)).containsSubsequence(
+				"org/springframework/boot/loader/",
+				"BOOT-INF/classes/com/example/Application.class",
+				"BOOT-INF/lib/" + libraryOne.getName(),
+				"BOOT-INF/lib/" + libraryTwo.getName(),
+				"BOOT-INF/lib/" + libraryThree.getName());
+	}
+
+	@Test
+	public void existingEntryThatMatchesUnpackLibraryIsMarkedForUnpack()
+			throws IOException {
+		File library = createLibrary();
+		this.testJarFile.addClass("WEB-INF/classes/com/example/Application.class",
+				ClassWithMainMethod.class);
+		this.testJarFile.addFile("WEB-INF/lib/" + library.getName(), library);
+		File source = this.testJarFile.getFile("war");
+		File dest = this.temporaryFolder.newFile("dest.war");
+		Repackager repackager = new Repackager(source);
+		repackager.setLayout(new Layouts.War());
+		repackager.repackage(dest, (callback) -> callback
+				.library(new Library(library, LibraryScope.COMPILE, true)));
+		assertThat(getEntryNames(dest)).containsSubsequence(
+				"org/springframework/boot/loader/",
+				"WEB-INF/classes/com/example/Application.class",
+				"WEB-INF/lib/" + library.getName());
+		JarEntry unpackLibrary = getEntry(dest, "WEB-INF/lib/" + library.getName());
+		assertThat(unpackLibrary.getComment()).startsWith("UNPACK:");
+	}
+
+	private File createLibrary() throws IOException {
+		TestJarFile library = new TestJarFile(this.temporaryFolder);
+		library.addClass("com/example/library/Library.class",
+				ClassWithoutMainMethod.class);
+		return library.getFile();
+	}
+
 	private boolean hasLauncherClasses(File file) throws IOException {
 		return hasEntry(file, "org/springframework/boot/")
 				&& hasEntry(file, "org/springframework/boot/loader/JarLauncher.class");
@@ -596,6 +650,17 @@ public class RepackagerTests {
 		}
 	}
 
+	private List<String> getEntryNames(File file) throws IOException {
+		List<String> entryNames = new ArrayList<>();
+		try (JarFile jarFile = new JarFile(file)) {
+			Enumeration<JarEntry> entries = jarFile.entries();
+			while (entries.hasMoreElements()) {
+				entryNames.add(entries.nextElement().getName());
+			}
+		}
+		return entryNames;
+	}
+
 	private static class MockLauncherScript implements LaunchScript {
 
 		private final byte[] bytes;