From db7bdc492b7bdf2569858fa9518220c7c766e1e1 Mon Sep 17 00:00:00 2001 From: Michael Dowling Date: Thu, 29 Sep 2022 16:44:27 -0700 Subject: [PATCH] Always attempt model interop transforms We previously did not attempt to perform model interop transforms if a model had errors. For example, if a model referred to an unknown trait, the model wouldn't use the interop transformation. Some use cases might choose to ignore unknown traits and other errors by _not_ unwrapping the assembled model and instead directly accessing the result from ValidatedResult. In these cases, the model would have not gone through the interop transform, resulting in an unexpect model (for example, if performing a diff against a 1.0 and 2.0 model, the upgrade won't run and causes the models to appear to be wildly different). --- .../smithy/model/loader/ModelAssembler.java | 30 +++++++++++-------- .../model/loader/ModelInteropTransformer.java | 9 +++--- .../model/loader/ModelAssemblerTest.java | 28 +++++++++++++++++ .../loader/invalid-1.0-model-upgraded.smithy | 20 +++++++++++++ 4 files changed, 70 insertions(+), 17 deletions(-) create mode 100644 smithy-model/src/test/resources/software/amazon/smithy/model/loader/invalid-1.0-model-upgraded.smithy diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelAssembler.java b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelAssembler.java index 60d749ea631..eed4e08f590 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelAssembler.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelAssembler.java @@ -35,6 +35,7 @@ import java.util.Set; import java.util.function.Consumer; import java.util.function.Supplier; +import java.util.logging.Level; import java.util.logging.Logger; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -547,29 +548,34 @@ public ValidatedResult assemble() { } Model processedModel = processor.buildModel(); + Model transformed; + + // Do the 1.0 -> 2.0 transform before full-model validation. + try { + transformed = new ModelInteropTransformer(processedModel, events, processor::getShapeVersion) + .transform(); + } catch (SourceException e) { + // The transformation shouldn't throw, but if it does, return here with the original model. + LOGGER.log(Level.SEVERE, "Error in ModelInteropTransformer: ", e); + events.add(ValidationEvent.fromSourceException(e)); + return new ValidatedResult<>(processedModel, events); + } // If ERROR validation events occur while loading, then performing more // granular semantic validation will only obscure the root cause of errors. if (LoaderUtils.containsErrorEvents(events)) { - return returnOnlyErrors(processedModel, events); + return returnOnlyErrors(transformed, events); } - // Do the 1.0 -> 2.0 transform before full-model validation. - ValidatedResult transformed = new ModelInteropTransformer(processedModel, events, - processor::getShapeVersion).transform(); - - if (disableValidation - || !transformed.getResult().isPresent() - || LoaderUtils.containsErrorEvents(transformed.getValidationEvents())) { - // Don't continue to validate the model if the upgrade raised ERROR events. - return transformed; + if (disableValidation) { + return new ValidatedResult<>(transformed, events); } try { - return validate(transformed.getResult().get(), transformed.getValidationEvents()); + return validate(transformed, events); } catch (SourceException e) { events.add(ValidationEvent.fromSourceException(e)); - return new ValidatedResult<>(transformed.getResult().get(), events); + return new ValidatedResult<>(transformed, events); } } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelInteropTransformer.java b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelInteropTransformer.java index 253b7278d72..907904bbd77 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelInteropTransformer.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelInteropTransformer.java @@ -41,7 +41,6 @@ import software.amazon.smithy.model.traits.StreamingTrait; import software.amazon.smithy.model.traits.Trait; import software.amazon.smithy.model.transform.ModelTransformer; -import software.amazon.smithy.model.validation.ValidatedResult; import software.amazon.smithy.model.validation.ValidationEvent; /** @@ -74,13 +73,13 @@ final class ModelInteropTransformer { private final Function fileToVersion; private final List shapeUpgrades = new ArrayList<>(); - ModelInteropTransformer(Model model, List events, Function fileToVersion) { + ModelInteropTransformer(Model model, List mutableEvents, Function fileToVersion) { this.model = model; - this.events = events; + this.events = mutableEvents; this.fileToVersion = fileToVersion; } - ValidatedResult transform() { + Model transform() { // TODO: can these transforms be more efficiently moved into the loader? for (StructureShape struct : model.getStructureShapes()) { if (!Prelude.isPreludeShape(struct)) { @@ -106,7 +105,7 @@ ValidatedResult transform() { } } - return new ValidatedResult<>(ModelTransformer.create().replaceShapes(model, shapeUpgrades), events); + return ModelTransformer.create().replaceShapes(model, shapeUpgrades); } private void upgradeV1Member(MemberShape member, Shape target) { diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelAssemblerTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelAssemblerTest.java index 0db515a4c0c..17ec073312a 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelAssemblerTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelAssemblerTest.java @@ -1108,4 +1108,32 @@ public void appliesBoxTraitsToMixinsToo() { assertThat(model1, equalTo(model2)); } + + @Test + public void versionTransformsAreAlwaysApplied() { + ValidatedResult result = Model.assembler() + .addImport(getClass().getResource("invalid-1.0-model-upgraded.smithy")) + .assemble(); + + // Ensure that the invalid trait caused the model to have errors. + assertThat(result.getValidationEvents(Severity.ERROR), not(empty())); + + // Ensure that the model was created. + assertThat(result.getResult().isPresent(), is(true)); + + Model model = result.getResult().get(); + + // Ensure that the model upgrade transformations happened. + Shape myInteger = model.expectShape(ShapeId.from("smithy.example#MyInteger")); + Shape fooBaz = model.expectShape(ShapeId.from("smithy.example#Foo$baz")); + Shape fooBam = model.expectShape(ShapeId.from("smithy.example#Foo$bam")); + + // These shapes have a default zero value so have a default trait for 2.0. + assertThat(myInteger.getAllTraits(), hasKey(DefaultTrait.ID)); + assertThat(fooBaz.getAllTraits(), hasKey(DefaultTrait.ID)); + + // These members are considered boxed in 1.0. + assertThat(fooBam.getAllTraits(), hasKey(BoxTrait.ID)); + assertThat(fooBam.expectTrait(DefaultTrait.class).toNode(), equalTo(Node.nullNode())); + } } diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/loader/invalid-1.0-model-upgraded.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/invalid-1.0-model-upgraded.smithy new file mode 100644 index 00000000000..bbd4588cc7a --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/invalid-1.0-model-upgraded.smithy @@ -0,0 +1,20 @@ +$version: "1.0" + +namespace smithy.example + +integer MyInteger + +structure Foo { + bar: Integer, + baz: MyInteger, + + @box + bam: PrimitiveInteger, + + // An invalid trait doesn't break the loading process or prevent upgrades. + @thisTraitDoesNotExist + boo: String, + + // An invalid target doesn't break the loading process or prevent upgrades. + bux: InvalidTarget +}