Fix loading jars with encoded paths

smithy-build/src/main/java/software/amazon/smithy/build/PluginContext.java

@@ -5,6 +5,7 @@
 package software.amazon.smithy.build;
 
 import java.nio.file.Path;
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
@@ -37,6 +38,7 @@ public final class PluginContext implements ToSmithyBuilder<PluginContext> {
     private final ClassLoader pluginClassLoader;
     private final Set<Path> sources;
     private final String artifactName;
+    private final List<String> sourceUriPaths;
     private Model nonTraitsModel;
 
     private PluginContext(Builder builder) {
@@ -50,6 +52,15 @@ private PluginContext(Builder builder) {
         settings = builder.settings;
         pluginClassLoader = builder.pluginClassLoader;
         sources = builder.sources.copy();
+        // Normalize the source paths into URI paths, which are URI encoded and use '/' separators,
+        // which is the format of paths in 'jar:file:/' filenames, so they can be compared in the
+        // 'isSource*' methods.
+        // This is done preemptively so we don't have to do the same work repeatedly, and the number
+        // of configured sources is typically very low.
+        sourceUriPaths = new ArrayList<>(sources.size());
+        for (Path source : sources) {
+            sourceUriPaths.add(source.toUri().getRawPath());
+        }
     }
 
     /**
@@ -223,6 +234,18 @@ private boolean isSource(FromSourceLocation sourceLocation) {
         String location = sourceLocation.getSourceLocation().getFilename();
         int offsetFromStart = findOffsetFromStart(location);
 
+        if (offsetFromStart > 0) {
+            // Offset means the filename is a URI, so compare to the URI paths to account for encoding and windows.
+            for (String sourceUriPath : sourceUriPaths) {
+                if (location.regionMatches(offsetFromStart, sourceUriPath, 0, sourceUriPath.length())) {
+                    return true;
+                }
+            }
+
+            return false;
+        }
+
+        // Filename is a regular path, so we can compare to sources directly.
         for (Path path : sources) {
             String pathString = path.toString();
             int offsetFromStartInSource = findOffsetFromStart(pathString);

smithy-build/src/test/java/software/amazon/smithy/build/plugins/SourcesPluginTest.java

@@ -10,9 +10,16 @@
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.not;
 
+import java.io.File;
+import java.io.IOException;
 import java.net.URISyntaxException;
+import java.nio.file.Files;
+import java.nio.file.Path;
 import java.nio.file.Paths;
+import java.util.Comparator;
+import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import software.amazon.smithy.build.MockManifest;
 import software.amazon.smithy.build.PluginContext;
@@ -26,6 +33,19 @@
 import software.amazon.smithy.utils.ListUtils;
 
 public class SourcesPluginTest {
+
+    private Path tempDirectory;
+
+    @BeforeEach
+    public void before() throws IOException {
+        tempDirectory = Files.createTempDirectory(getClass().getSimpleName());
+    }
+
+    @AfterEach
+    public void after() throws IOException {
+        Files.walk(tempDirectory).sorted(Comparator.reverseOrder()).map(Path::toFile).forEach(File::delete);
+    }
+
     @Test
     public void copiesFilesForSourceProjection() throws URISyntaxException {
         Model model = Model.assembler()
@@ -236,4 +256,69 @@ public void omitsUnsupportedFilesFromManifest() throws URISyntaxException {
         assertThat(manifestString, containsString("a.smithy\n"));
         assertThat(manifestString, not(containsString("foo.md")));
     }
+
+    @Test
+    public void copiesModelFromJarWithEncodedPathWithSourceProjection()
+            throws IOException, URISyntaxException {
+        Path source = Paths.get(getClass().getResource("sources/jar-import.jar").toURI());
+        Path jarPath = tempDirectory.resolve(Paths.get("special%45path", "special.jar"));
+
+        Files.createDirectories(jarPath.getParent());
+        Files.copy(source, jarPath);
+
+        Model model = Model.assembler()
+                .addImport(jarPath)
+                .addImport(getClass().getResource("notsources/d.smithy"))
+                .assemble()
+                .unwrap();
+        MockManifest manifest = new MockManifest();
+        PluginContext context = PluginContext.builder()
+                .fileManifest(manifest)
+                .model(model)
+                .originalModel(model)
+                .sources(ListUtils.of(jarPath))
+                .build();
+        new SourcesPlugin().execute(context);
+        String manifestString = manifest.getFileString("manifest").get();
+
+        assertThat(manifestString, containsString("special/a.smithy\n"));
+        assertThat(manifestString, containsString("special/b/b.smithy\n"));
+        assertThat(manifestString, containsString("special/b/c/c.json\n"));
+        assertThat(manifestString, not(containsString("jar-import/d.json")));
+        assertThat(manifest.getFileString("special/a.smithy").get(), containsString("string A"));
+        assertThat(manifest.getFileString("special/b/b.smithy").get(), containsString("string B"));
+        assertThat(manifest.getFileString("special/b/c/c.json").get(), containsString("\"foo.baz#C\""));
+    }
+
+    @Test
+    public void copiesModelFromJarWithEncodedPathWithNonSourceProjection()
+            throws IOException, URISyntaxException {
+        Path source = Paths.get(getClass().getResource("sources/jar-import.jar").toURI());
+        Path jarPath = tempDirectory.resolve(Paths.get("special%45path", "special.jar"));
+
+        Files.createDirectories(jarPath.getParent());
+        Files.copy(source, jarPath);
+
+        Model model = Model.assembler()
+                .addImport(jarPath)
+                .assemble()
+                .unwrap();
+        MockManifest manifest = new MockManifest();
+        ProjectionConfig projection = ProjectionConfig.builder().build();
+        PluginContext context = PluginContext.builder()
+                .fileManifest(manifest)
+                .projection("foo", projection)
+                .model(model)
+                .originalModel(model)
+                .sources(ListUtils.of(jarPath))
+                .build();
+        new SourcesPlugin().execute(context);
+        String manifestString = manifest.getFileString("manifest").get();
+
+        assertThat(manifestString, containsString("model.json"));
+        assertThat(manifestString, not(containsString("special")));
+        assertThat(manifest.getFileString("model.json").get(), containsString("\"foo.baz#A\""));
+        assertThat(manifest.getFileString("model.json").get(), containsString("\"foo.baz#B\""));
+        assertThat(manifest.getFileString("model.json").get(), containsString("\"foo.baz#C\""));
+    }
 }

smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelDiscovery.java

@@ -10,9 +10,12 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
+import java.net.URI;
+import java.net.URISyntaxException;
 import java.net.URL;
 import java.net.URLConnection;
 import java.nio.charset.StandardCharsets;
+import java.nio.file.Paths;
 import java.util.ArrayList;
 import java.util.Enumeration;
 import java.util.LinkedHashSet;
@@ -185,19 +188,30 @@ public static String getSmithyModelPathFromJarUrl(URL modelUrl) {
      */
     public static URL createSmithyJarManifestUrl(String fileOrUrl) {
         try {
-            return new URL(getFilenameWithScheme(fileOrUrl) + "!/" + MANIFEST_PATH);
-        } catch (IOException e) {
+            // This normalizes the path to the jar into the format expected by URI,
+            // accounting for the different string representation of a path on windows
+            // (otherwise all '\' path separators will be encoded).
+            URI jarUri = Paths.get(removeScheme(fileOrUrl)).toUri();
+            String manifestPath = jarUri.getPath() + "!/" + MANIFEST_PATH;
+
+            // Building a URI from parts (scheme + file path) will make sure the file path
+            // gets properly URI encoded. URL does not do this, instead expecting callers
+            // to encode first, and consumers to decode. Encoding it here means that when
+            // it is decoded by the consumer (e.g. the JDK reading the jar from the file system),
+            // it will point to the correct location.
+            return new URI("jar:file", null, manifestPath, null).toURL();
+        } catch (IOException | URISyntaxException e) {
             throw new ModelImportException(e.getMessage(), e);
         }
     }
 
-    private static String getFilenameWithScheme(String filename) {
-        if (filename.startsWith("jar:")) {
-            return filename;
-        } else if (filename.startsWith("file:")) {
-            return "jar:" + filename;
+    private static String removeScheme(String fileOrUrl) {
+        // Index will also cover jar: part of jar:file:
+        int pathStart = fileOrUrl.indexOf("file:");
+        if (pathStart == -1) {
+            return fileOrUrl;
         } else {
-            return "jar:file:" + filename;
+            return fileOrUrl.substring(pathStart + "file:".length());
         }
     }
 

smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelAssemblerTest.java

@@ -1459,4 +1459,57 @@ public void handlesSameModelWhenBuiltAndImported() throws Exception {
         assertTrue(combinedModel.expectShape(ShapeId.from("smithy.example#MachineData$machineId"), MemberShape.class)
                 .hasTrait(RequiredTrait.ID));
     }
+
+    @Test
+    public void importsJarFromPathWithEncodedChars() throws Exception {
+        Path source = Paths.get(getClass().getResource("assembler-valid-jar").toURI());
+        Path jarPath = outputDirectory.resolve(Paths.get("path%20with%45encoded%25chars", "test.jar"));
+
+        Files.createDirectories(jarPath.getParent());
+        Files.copy(JarUtils.createJarFromDir(source), jarPath);
+
+        Model model = Model.assembler()
+                .addImport(jarPath)
+                .assemble()
+                .unwrap();
+
+        assertTrue(model.getShape(ShapeId.from("smithy.example#ExampleString")).isPresent());
+    }
+
+    @Test
+    public void importsJarFromUrlWithEncodedChars() throws Exception {
+        Path source = Paths.get(getClass().getResource("assembler-valid-jar").toURI());
+        Path jarPath = outputDirectory.resolve(Paths.get("path%20with%45encoded%25chars", "test.jar"));
+
+        Files.createDirectories(jarPath.getParent());
+        Files.copy(JarUtils.createJarFromDir(source), jarPath);
+
+        Model model = Model.assembler()
+                .addImport(jarPath.toUri().toURL())
+                .assemble()
+                .unwrap();
+
+        assertTrue(model.getShape(ShapeId.from("smithy.example#ExampleString")).isPresent());
+    }
+
+    @Test
+    public void importedJarsWithEncodedCharsHaveCorrectFilename() throws Exception {
+        Path source = Paths.get(getClass().getResource("assembler-valid-jar").toURI());
+        Path jarPath = outputDirectory.resolve(Paths.get("path%20with%45encoded%25chars", "test.jar"));
+
+        Files.createDirectories(jarPath.getParent());
+        Files.copy(JarUtils.createJarFromDir(source), jarPath);
+
+        Model model = Model.assembler()
+                .addImport(jarPath)
+                .assemble()
+                .unwrap();
+
+        String filename = model.expectShape(ShapeId.from("smithy.example#ExampleString"))
+                .getSourceLocation()
+                .getFilename();
+        String fileContents = IoUtils.readUtf8Url(new URL(filename));
+
+        assertThat(fileContents, containsString("string ExampleString"));
+    }
 }

smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelDiscoveryTest.java

@@ -147,4 +147,14 @@ public void createSmithyManifestUrlFromJarUrl() throws IOException {
         assertThat(ModelDiscovery.createSmithyJarManifestUrl("jar:file:/foo.jar"),
                 equalTo(new URL("jar:file:/foo.jar!/META-INF/smithy/manifest")));
     }
+
+    @Test
+    public void encodesSmithyManifestUrl() throws IOException {
+        assertThat(ModelDiscovery.createSmithyJarManifestUrl("/foo bar.jar"),
+                equalTo(new URL("jar:file:/foo%20bar.jar!/META-INF/smithy/manifest")));
+        assertThat(ModelDiscovery.createSmithyJarManifestUrl("file:/foo bar.jar"),
+                equalTo(new URL("jar:file:/foo%20bar.jar!/META-INF/smithy/manifest")));
+        assertThat(ModelDiscovery.createSmithyJarManifestUrl("jar:file:/foo bar.jar"),
+                equalTo(new URL("jar:file:/foo%20bar.jar!/META-INF/smithy/manifest")));
+    }
 }

smithy-model/src/test/resources/software/amazon/smithy/model/loader/assembler-valid-jar/META-INF/MANIFEST.MF

@@ -0,0 +1,2 @@
+Manifest-Version: 1.0
+Created-By: 11.0.6 (Amazon.com Inc.)

smithy-model/src/test/resources/software/amazon/smithy/model/loader/assembler-valid-jar/META-INF/smithy/manifest

@@ -0,0 +1 @@
+valid.smithy

smithy-model/src/test/resources/software/amazon/smithy/model/loader/assembler-valid-jar/META-INF/smithy/valid.smithy

@@ -0,0 +1,5 @@
+$version: "2.0"
+
+namespace smithy.example
+
+string ExampleString