From ffb278319ee492cc865ce7430edea250a47ef19b Mon Sep 17 00:00:00 2001 From: Michael Dowling Date: Tue, 14 Sep 2021 13:44:22 -0700 Subject: [PATCH] Fail by default when a build plugin can't be found smithy-build previously just logged an info when a build plugin couldn't be found on the classpath. This was a bad user experience for actually using Smithy. The requirement to be able to ignore a missing build plugin is really an edge case for build tools that want to be able to build just models and ignore other plugins, so those tools should opt-in to that behavior instead. Just logging was way too subtle for diagnosing issues with config files and dependencies. --- .../guides/building-models/build-config.rst | 5 +++ .../amazon/smithy/build/SmithyBuildImpl.java | 15 +++++--- .../smithy/build/model/SmithyBuildConfig.java | 34 ++++++++++++++++++- .../amazon/smithy/build/SmithyBuildTest.java | 23 +++++++++++-- .../build/model/SmithyBuildConfigTest.java | 22 ++++++++++++ .../smithy/build/unknown-plugin-ignored.json | 19 +++++++++++ 6 files changed, 111 insertions(+), 7 deletions(-) create mode 100644 smithy-build/src/test/resources/software/amazon/smithy/build/unknown-plugin-ignored.json diff --git a/docs/source/1.0/guides/building-models/build-config.rst b/docs/source/1.0/guides/building-models/build-config.rst index 9ed68cd9400..0d3166e351e 100644 --- a/docs/source/1.0/guides/building-models/build-config.rst +++ b/docs/source/1.0/guides/building-models/build-config.rst @@ -49,6 +49,11 @@ The configuration file accepts the following properties: - Defines the plugins to apply to the model when building every projection. Plugins are a mapping of plugin names to an arbitrary plugin configuration object. + * - ignoreMissingPlugins + - ``bool`` + - If a plugin can't be found, Smithy will by default fail the build. This + setting can be set to ``true`` to allow the build to progress even if + a plugin can't be found on the classpath. The following is an example ``smithy-build.json`` configuration: diff --git a/smithy-build/src/main/java/software/amazon/smithy/build/SmithyBuildImpl.java b/smithy-build/src/main/java/software/amazon/smithy/build/SmithyBuildImpl.java index f00fe143254..c2f566a2a9c 100644 --- a/smithy-build/src/main/java/software/amazon/smithy/build/SmithyBuildImpl.java +++ b/smithy-build/src/main/java/software/amazon/smithy/build/SmithyBuildImpl.java @@ -362,9 +362,14 @@ private void applyPlugin( SmithyBuildPlugin resolved = pluginFactory.apply(pluginName).orElse(null); if (resolved == null) { - LOGGER.info(() -> String.format( - "Unable to find a plugin for `%s` in the `%s` projection", - pluginName, projectionName)); + String message = "Unable to find a plugin named `" + pluginName + "` in the `" + projectionName + "` " + + "projection. Is this the correct spelling? Are you missing a dependency? Is your " + + "classpath configured correctly?"; + if (config.isIgnoreMissingPlugins()) { + LOGGER.severe(message); + } else { + throw new SmithyBuildException(message); + } } else if (resolved.requiresValidModel() && modelResult.isBroken()) { LOGGER.fine(() -> String.format( "Skipping `%s` plugin for `%s` projection because the model is broken", @@ -411,7 +416,9 @@ private List> createTransformers( ProjectionTransformer transformer = transformFactory.apply(name) .orElseThrow(() -> new UnknownTransformException(String.format( - "Unable to find a transform for `%s` in the `%s` projection.", name, projectionName))); + "Unable to find a transform named `%s` in the `%s` projection. Is this the correct " + + "spelling? Are you missing a dependency? Is your classpath configured correctly?", + name, projectionName))); resolved.add(Pair.of(transformConfig.getArgs(), transformer)); } diff --git a/smithy-build/src/main/java/software/amazon/smithy/build/model/SmithyBuildConfig.java b/smithy-build/src/main/java/software/amazon/smithy/build/model/SmithyBuildConfig.java index 856704fbb88..d4e7a0dcd82 100644 --- a/smithy-build/src/main/java/software/amazon/smithy/build/model/SmithyBuildConfig.java +++ b/smithy-build/src/main/java/software/amazon/smithy/build/model/SmithyBuildConfig.java @@ -45,6 +45,7 @@ public final class SmithyBuildConfig implements ToSmithyBuilder projections; private final Map plugins; + private final boolean ignoreMissingPlugins; private SmithyBuildConfig(Builder builder) { SmithyBuilder.requiredState("version", builder.version); @@ -53,6 +54,7 @@ private SmithyBuildConfig(Builder builder) { imports = ListUtils.copyOf(builder.imports); projections = MapUtils.copyOf(builder.projections); plugins = new HashMap<>(builder.plugins); + ignoreMissingPlugins = builder.ignoreMissingPlugins; for (String builtin : BUILTIN_PLUGINS) { plugins.put(builtin, Node.objectNode()); } @@ -113,7 +115,8 @@ public Builder toBuilder() { .outputDirectory(outputDirectory) .imports(imports) .projections(projections) - .plugins(plugins); + .plugins(plugins) + .ignoreMissingPlugins(ignoreMissingPlugins); } /** @@ -163,6 +166,17 @@ public Map getPlugins() { return Collections.unmodifiableMap(plugins); } + /** + * If a plugin can't be found, Smithy will by default fail the build. + * This setting can be set to true to allow the build to progress even + * if there is a missing plugin. + * + * @return Returns true if missing build plugins are allowed. + */ + public boolean isIgnoreMissingPlugins() { + return ignoreMissingPlugins; + } + /** * Builder used to create a {@link SmithyBuildConfig}. */ @@ -172,6 +186,7 @@ public static final class Builder implements SmithyBuilder { private final Map plugins = new LinkedHashMap<>(); private String version; private String outputDirectory; + private boolean ignoreMissingPlugins; Builder() {} @@ -213,6 +228,12 @@ public Builder merge(SmithyBuildConfig config) { imports.addAll(config.getImports()); projections.putAll(config.getProjections()); plugins.putAll(config.getPlugins()); + + // If either one wants to ignore missing plugins, then ignore them. + if (config.isIgnoreMissingPlugins()) { + ignoreMissingPlugins(config.ignoreMissingPlugins); + } + return this; } @@ -262,5 +283,16 @@ public Builder plugins(Map plugins) { this.plugins.putAll(plugins); return this; } + + /** + * Logs instead of failing when a plugin can't be found by name. + * + * @param ignoreMissingPlugins Set to true to ignore missing plugins on the classpath. + * @return Returns the builder. + */ + public Builder ignoreMissingPlugins(boolean ignoreMissingPlugins) { + this.ignoreMissingPlugins = ignoreMissingPlugins; + return this; + } } } 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 83df27f87b6..ee586253a81 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 @@ -187,9 +187,9 @@ public void doesNotCopyErroneousModelsToBuildOutput() throws Exception { } @Test - public void ignoresUnknownPlugins() throws Exception { + public void canIgnoreUnknownPlugins() throws Exception { SmithyBuildConfig config = SmithyBuildConfig.builder() - .load(Paths.get(getClass().getResource("unknown-plugin.json").toURI())) + .load(Paths.get(getClass().getResource("unknown-plugin-ignored.json").toURI())) .outputDirectory(outputDirectory.toString()) .build(); Model model = Model.assembler() @@ -200,6 +200,25 @@ public void ignoresUnknownPlugins() throws Exception { builder.build(); } + @Test + public void failsByDefaultForUnknownPlugins() throws Exception { + SmithyBuildConfig config = SmithyBuildConfig.builder() + .load(Paths.get(getClass().getResource("unknown-plugin.json").toURI())) + .outputDirectory(outputDirectory.toString()) + .build(); + Model model = Model.assembler() + .addImport(Paths.get(getClass().getResource("resource-model.json").toURI())) + .assemble() + .unwrap(); + + SmithyBuildException e = Assertions.assertThrows(SmithyBuildException.class, () -> { + SmithyBuild builder = new SmithyBuild(config).model(model); + builder.build(); + }); + + assertThat(e.getMessage(), containsString("Unable to find a plugin named `unknown1`")); + } + @Test public void cannotSetFiltersOrMappersOnSourceProjection() { Throwable thrown = Assertions.assertThrows(SmithyBuildException.class, () -> { diff --git a/smithy-build/src/test/java/software/amazon/smithy/build/model/SmithyBuildConfigTest.java b/smithy-build/src/test/java/software/amazon/smithy/build/model/SmithyBuildConfigTest.java index e2217ee4543..206e499ebc3 100644 --- a/smithy-build/src/test/java/software/amazon/smithy/build/model/SmithyBuildConfigTest.java +++ b/smithy-build/src/test/java/software/amazon/smithy/build/model/SmithyBuildConfigTest.java @@ -157,4 +157,26 @@ private String getResourcePath(String name) { throw new RuntimeException(e); } } + + @Test + public void convertingToBuilderRetainsIgnoreMissingPlugins() { + SmithyBuildConfig a = SmithyBuildConfig.builder() + .version("1") + .ignoreMissingPlugins(true) + .build(); + + assertThat(a.toBuilder().build().isIgnoreMissingPlugins(), equalTo(true)); + } + + @Test + public void mergingTakesIgnoreMissingPluginsFromEither() { + SmithyBuildConfig a = SmithyBuildConfig.builder() + .version("1") + .ignoreMissingPlugins(true) + .build(); + SmithyBuildConfig b = SmithyBuildConfig.builder().version("1").build(); + + assertThat(a.toBuilder().merge(b).build().isIgnoreMissingPlugins(), equalTo(true)); + assertThat(b.toBuilder().merge(a).build().isIgnoreMissingPlugins(), equalTo(true)); + } } diff --git a/smithy-build/src/test/resources/software/amazon/smithy/build/unknown-plugin-ignored.json b/smithy-build/src/test/resources/software/amazon/smithy/build/unknown-plugin-ignored.json new file mode 100644 index 00000000000..f0255a0a7a2 --- /dev/null +++ b/smithy-build/src/test/resources/software/amazon/smithy/build/unknown-plugin-ignored.json @@ -0,0 +1,19 @@ +{ + "version": "1.0", + "ignoreMissingPlugins": true, + "plugins": { + "unknown1": {} + }, + "projections": { + "a": { + "plugins": { + "unknown2": {} + } + }, + "b": { + "plugins": { + "unknown3": {} + } + } + } +}