From 3208ebb6d236f11a6e3cd04b2bb97d9f229d839b Mon Sep 17 00:00:00 2001 From: Michael Dowling Date: Tue, 11 Jul 2023 15:59:22 -0500 Subject: [PATCH] Omit unrecognized models from sources Smithy-Build is often invoked from the Smithy CLI, and developers often provide directories that contain model files. If a directory contains files other than recognized Smithy models, those unknown files are treated as "sources" and ultimately wind up in the sources plugin manifest file. Files that are not Smithy model files should not show up in the manifest as this can cause invalid JARs to get created by tooling like the Smithy Gradle plugin. Sources are now eagerly filtered when they are added to SmithyBuild. If someone, somehow, manually configures the sources plugin to use unrecognized Smithy model files, the plugin will skip those files and log a warning rather than create an invalid manifest. --- .../amazon/smithy/build/SmithyBuild.java | 36 ++++++++++++- .../smithy/build/plugins/SourcesPlugin.java | 12 ++++- .../amazon/smithy/build/SmithyBuildTest.java | 52 ++++++++++++++++++- .../build/plugins/SourcesPluginTest.java | 24 +++++++++ .../a.smithy | 5 ++ .../sources-ignores-unrecognized-files/foo.md | 4 ++ .../smithy/model/loader/ModelLoader.java | 2 +- 7 files changed, 130 insertions(+), 5 deletions(-) create mode 100644 smithy-build/src/test/resources/software/amazon/smithy/build/plugins/sources-ignores-unrecognized-files/a.smithy create mode 100644 smithy-build/src/test/resources/software/amazon/smithy/build/plugins/sources-ignores-unrecognized-files/foo.md diff --git a/smithy-build/src/main/java/software/amazon/smithy/build/SmithyBuild.java b/smithy-build/src/main/java/software/amazon/smithy/build/SmithyBuild.java index 0dddac1005f..136b03f779c 100644 --- a/smithy-build/src/main/java/software/amazon/smithy/build/SmithyBuild.java +++ b/smithy-build/src/main/java/software/amazon/smithy/build/SmithyBuild.java @@ -15,7 +15,12 @@ package software.amazon.smithy.build; +import java.io.IOException; +import java.io.UncheckedIOException; +import java.nio.file.FileSystems; +import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.PathMatcher; import java.nio.file.Paths; import java.util.Collections; import java.util.HashSet; @@ -29,6 +34,7 @@ import java.util.function.Function; import java.util.function.Predicate; import java.util.function.Supplier; +import java.util.logging.Logger; import software.amazon.smithy.build.model.SmithyBuildConfig; import software.amazon.smithy.model.Model; import software.amazon.smithy.model.loader.ModelAssembler; @@ -42,6 +48,9 @@ public final class SmithyBuild { /** The version of Smithy build. */ public static final String VERSION = "1.0"; + private static final Logger LOGGER = Logger.getLogger(SmithyBuild.class.getName()); + private static final PathMatcher VALID_MODELS = FileSystems.getDefault().getPathMatcher("glob:*.{json,jar,smithy}"); + SmithyBuildConfig config; Path outputDirectory; Function> transformFactory; @@ -187,8 +196,27 @@ public SmithyBuild config(SmithyBuildConfig config) { // Add a source path using absolute paths to better de-conflict source files. ModelAssembler also // de-conflicts imports with absolute paths, but this ensures the same file doesn't appear twice in // the build plugin output (though it does not use realpath to de-conflict based on symlinks). + // + // Ignores and logs when an unsupported model file is encountered. private void addSource(Path path) { - sources.add(path.toAbsolutePath()); + try { + if (Files.isDirectory(path)) { + // Pre-emptively crawl the given models to filter them up front into a flat, file-only, list. + Files.list(path).filter(Files::isRegularFile).forEach(this::addSourceFile); + } else { + addSourceFile(path); + } + } catch (IOException e) { + throw new UncheckedIOException(e); + } + } + + private void addSourceFile(Path file) { + if (!VALID_MODELS.matches(file.getFileName())) { + LOGGER.warning("Omitting unsupported Smithy model file from model sources: " + file); + } else { + sources.add(file.toAbsolutePath()); + } } /** @@ -380,6 +408,12 @@ public SmithyBuild pluginClassLoader(ClassLoader pluginClassLoader) { * unique across the entire set of files. The sources directories are * essentially flattened into a single directory. * + *

Unsupported model files are ignored and not treated as sources. + * This can happen when adding model files from a directory that contains + * a mix of model files and non-model files. Filtering models here prevents + * unsupported files from appearing in places like JAR manifest files where + * they are not allowed. + * * @param pathToSources Path to source directories to mark. * @return Returns the builder. */ diff --git a/smithy-build/src/main/java/software/amazon/smithy/build/plugins/SourcesPlugin.java b/smithy-build/src/main/java/software/amazon/smithy/build/plugins/SourcesPlugin.java index d6a50629aba..603aa4c2f74 100644 --- a/smithy-build/src/main/java/software/amazon/smithy/build/plugins/SourcesPlugin.java +++ b/smithy-build/src/main/java/software/amazon/smithy/build/plugins/SourcesPlugin.java @@ -145,8 +145,16 @@ private static void copyFile(List names, FileManifest manifest, Path tar + ValidationUtils.tickedList(manifest.getFiles())); } - manifest.writeFile(target, contents); - names.add(target.toString()); + String filename = target.toString(); + + // Even though sources are filtered in SmithyBuild, it's theoretically possible that someone could call this + // plugin manually. In that case, refuse to write unsupported files to the manifest. + if (filename.endsWith(".smithy") || filename.endsWith(".json")) { + manifest.writeFile(target, contents); + names.add(target.toString()); + } else { + LOGGER.warning("Omitting unrecognized file from Smithy model manifest: " + filename); + } } private static void projectSources(PluginContext context) { diff --git a/smithy-build/src/test/java/software/amazon/smithy/build/SmithyBuildTest.java b/smithy-build/src/test/java/software/amazon/smithy/build/SmithyBuildTest.java index f140b2d7005..ad941ffeb86 100644 --- a/smithy-build/src/test/java/software/amazon/smithy/build/SmithyBuildTest.java +++ b/smithy-build/src/test/java/software/amazon/smithy/build/SmithyBuildTest.java @@ -48,12 +48,16 @@ import org.junit.jupiter.api.Assumptions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import software.amazon.smithy.build.model.ProjectionConfig; import software.amazon.smithy.build.model.SmithyBuildConfig; import software.amazon.smithy.build.model.TransformConfig; import software.amazon.smithy.model.Model; import software.amazon.smithy.model.SourceException; import software.amazon.smithy.model.SourceLocation; +import software.amazon.smithy.model.loader.ModelAssembler; import software.amazon.smithy.model.node.Node; import software.amazon.smithy.model.node.ObjectNode; import software.amazon.smithy.model.shapes.ShapeId; @@ -898,5 +902,51 @@ public void detectsConflictingArtifactNames() throws Exception { assertThat(e.getMessage(), containsString("Multiple plugins use the same artifact name 'foo' in " + "the 'source' projection")); } -} + @ParameterizedTest + @MethodSource("unrecognizedModelPaths") + public void ignoreUnrecognizedModels(List models) throws URISyntaxException { + ModelAssembler assembler = Model.assembler(); + for (Path path : models) { + assembler.addImport(path); + } + Model model = assembler.assemble().unwrap(); + + SmithyBuild builder = new SmithyBuild().model(model).fileManifestFactory(MockManifest::new); + models.forEach(builder::registerSources); + + // Apply multiple projections to ensure that sources are filtered even in projections. + builder.config(SmithyBuildConfig.builder() + .load(Paths.get(getClass().getResource("apply-multiple-projections.json").toURI())) + .outputDirectory("/foo") + .build()); + + SmithyBuildResult results = builder.build(); + assertTrue(results.getProjectionResult("source").isPresent()); + assertTrue(results.getProjectionResult("a").isPresent()); + + ProjectionResult sourceResult = results.getProjectionResult("source").get(); + MockManifest sourceManifest = (MockManifest) sourceResult.getPluginManifest("sources").get(); + String sourceSourcesManifestText = sourceManifest.getFileString("manifest").get(); + + assertThat(sourceSourcesManifestText, containsString("a.smithy")); + assertThat(sourceSourcesManifestText, not(containsString("foo.md"))); + + ProjectionResult aResult = results.getProjectionResult("source").get(); + MockManifest aManifest = (MockManifest) aResult.getPluginManifest("sources").get(); + String aSourcesManifestText = aManifest.getFileString("manifest").get(); + + assertThat(aSourcesManifestText, not(containsString("foo.md"))); + } + + public static List unrecognizedModelPaths() throws URISyntaxException { + Path rootPath = Paths.get(SmithyBuildTest.class.getResource("plugins/sources-ignores-unrecognized-files") + .toURI()); + return ListUtils.of( + // Test that crawling the directory works. + Arguments.of(ListUtils.of(rootPath)), + // Test that passing explicit files works too. + Arguments.of(ListUtils.of(rootPath.resolve("a.smithy"), rootPath.resolve("foo.md"))) + ); + } +} diff --git a/smithy-build/src/test/java/software/amazon/smithy/build/plugins/SourcesPluginTest.java b/smithy-build/src/test/java/software/amazon/smithy/build/plugins/SourcesPluginTest.java index 74e943c30e9..28eca8dff7d 100644 --- a/smithy-build/src/test/java/software/amazon/smithy/build/plugins/SourcesPluginTest.java +++ b/smithy-build/src/test/java/software/amazon/smithy/build/plugins/SourcesPluginTest.java @@ -208,4 +208,28 @@ public void copiesModelsDefinedInConfigAsSources() throws URISyntaxException { assertThat(manifest.hasFile("a.smithy"), is(true)); assertThat(manifest.hasFile("d.smithy"), is(false)); } + + // When the sources plugin sees a file it does not support, it is ignored. + @Test + public void omitsUnsupportedFilesFromManifest() throws URISyntaxException { + Model model = Model.assembler() + .addImport(getClass().getResource("sources-ignores-unrecognized-files/a.smithy")) + .addImport(getClass().getResource("sources-ignores-unrecognized-files/foo.md")) + .assemble() + .unwrap(); + MockManifest manifest = new MockManifest(); + PluginContext context = PluginContext.builder() + .fileManifest(manifest) + .model(model) + .originalModel(model) + .sources(ListUtils.of(Paths.get(getClass().getResource("sources-ignores-unrecognized-files").toURI()))) + .build(); + new SourcesPlugin().execute(context); + String manifestString = manifest.getFileString("manifest").get(); + // Normalize for Windows. + manifestString = manifestString.replace("\\", "/"); + + assertThat(manifestString, containsString("a.smithy\n")); + assertThat(manifestString, not(containsString("foo.md"))); + } } diff --git a/smithy-build/src/test/resources/software/amazon/smithy/build/plugins/sources-ignores-unrecognized-files/a.smithy b/smithy-build/src/test/resources/software/amazon/smithy/build/plugins/sources-ignores-unrecognized-files/a.smithy new file mode 100644 index 00000000000..b528584b5d0 --- /dev/null +++ b/smithy-build/src/test/resources/software/amazon/smithy/build/plugins/sources-ignores-unrecognized-files/a.smithy @@ -0,0 +1,5 @@ +$version: "2.0" + +namespace smithy.example + +string MyString diff --git a/smithy-build/src/test/resources/software/amazon/smithy/build/plugins/sources-ignores-unrecognized-files/foo.md b/smithy-build/src/test/resources/software/amazon/smithy/build/plugins/sources-ignores-unrecognized-files/foo.md new file mode 100644 index 00000000000..3b20c7ce0ef --- /dev/null +++ b/smithy-build/src/test/resources/software/amazon/smithy/build/plugins/sources-ignores-unrecognized-files/foo.md @@ -0,0 +1,4 @@ +# Ignore me! + +The sources plugin should ignore unsupported files so that they don't show up +in places like JARs. diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelLoader.java b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelLoader.java index cc189a5af29..c2ee05ffd99 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelLoader.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelLoader.java @@ -82,7 +82,7 @@ static boolean load( return loadParsedNode(Node.parse(inputStream, filename), operationConsumer); } } else { - LOGGER.warning(() -> "Ignoring unrecognized file: " + filename); + LOGGER.warning(() -> "Ignoring unrecognized Smithy model file: " + filename); return false; } } catch (IOException e) {