Always attempt model interop transforms

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<Model> 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<Model> 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);
         }
     }
 

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<Shape, Version> fileToVersion;
     private final List<Shape> shapeUpgrades = new ArrayList<>();
 
-    ModelInteropTransformer(Model model, List<ValidationEvent> events, Function<Shape, Version> fileToVersion) {
+    ModelInteropTransformer(Model model, List<ValidationEvent> mutableEvents, Function<Shape, Version> fileToVersion) {
         this.model = model;
-        this.events = events;
+        this.events = mutableEvents;
         this.fileToVersion = fileToVersion;
     }
 
-    ValidatedResult<Model> 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<Model> transform() {
             }
         }
 
-        return new ValidatedResult<>(ModelTransformer.create().replaceShapes(model, shapeUpgrades), events);
+        return ModelTransformer.create().replaceShapes(model, shapeUpgrades);
     }
 
     private void upgradeV1Member(MemberShape member, Shape target) {

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<Model> 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()));
+    }
 }

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
+}