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