From 50865347999e806f4b039a421b74e0961dceaf80 Mon Sep 17 00:00:00 2001 From: Michael Dowling Date: Thu, 21 Jul 2022 13:19:15 -0700 Subject: [PATCH] Handle unknown model versions for built-shapes The handling of unknown model versions for built shapes needs to make no assumptions about the version of provided shapes. Instead, it should use the UNKNOWN version which allows for any kind of 1.0 or 2.0 trait/shape, but still attempts to convert 1.0 -> 2.0 compatible models when loading shapes. This was a little haphazard before and we exposed controls to set the assumed parsed version that have now been removed. I don't think there's a reasonable use case where automated tooling will set that right now. --- .../model/loader/FullyResolvedModelFile.java | 7 +- .../smithy/model/loader/ModelAssembler.java | 24 +-- .../smithy/model/loader/ModelUpgrader.java | 9 +- .../amazon/smithy/model/loader/Version.java | 170 +++++++++++++----- .../model/loader/ModelAssemblerTest.java | 16 ++ .../model/transform/ChangeShapeTypeTest.java | 2 - 6 files changed, 150 insertions(+), 78 deletions(-) diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/loader/FullyResolvedModelFile.java b/smithy-model/src/main/java/software/amazon/smithy/model/loader/FullyResolvedModelFile.java index c4a0ef37e1b..5c7a1f19703 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/loader/FullyResolvedModelFile.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/loader/FullyResolvedModelFile.java @@ -52,10 +52,9 @@ final class FullyResolvedModelFile extends AbstractMutableModelFile { * @return Returns the create {@code FullyResolvedModelFile} containing the shapes. */ static FullyResolvedModelFile fromShapes(TraitFactory traitFactory, Collection shapes) { - return fromShapes(traitFactory, shapes, null); + return fromShapes(traitFactory, shapes, Version.UNKNOWN); } - /** * Create a {@code FullyResolvedModelFile} from already built shapes. * @@ -67,9 +66,7 @@ static FullyResolvedModelFile fromShapes(TraitFactory traitFactory, Collection properties = new HashMap<>(); private boolean disablePrelude; private Consumer validationEventListener = DEFAULT_EVENT_LISTENER; - private Version parsedShapesVersion; // Lazy initialization holder class idiom to hold a default trait factory. static final class LazyTraitFactoryHolder { @@ -368,20 +367,6 @@ public ModelAssembler addShapes(Shape... shapes) { return this; } - /** - * Sets the Smithy version to use for parsed shapes added directly to the - * assembler. - * - * If unset, the default version of 1.0 will be assumed. - * - * @param version A Smithy IDL version. - * @return Returns the assembler. - */ - public ModelAssembler setParsedShapesVersion(String version) { - this.parsedShapesVersion = Version.fromString(version); - return this; - } - /** * Explicitly adds a trait to a shape in the assembled model. * @@ -632,8 +617,7 @@ private List createModelFiles() { } // A modelFile is created for the assembler to capture anything that was manually added. - FullyResolvedModelFile assemblerModelFile = FullyResolvedModelFile.fromShapes( - traitFactory, shapes, parsedShapesVersion); + FullyResolvedModelFile assemblerModelFile = FullyResolvedModelFile.fromShapes(traitFactory, shapes); modelFiles.add(assemblerModelFile); metadata.forEach(assemblerModelFile::putMetadata); @@ -648,11 +632,7 @@ private List createModelFiles() { List nonPrelude = model.shapes() .filter(FunctionalUtils.not(Prelude::isPreludeShape)) .collect(Collectors.toList()); - // Since we're pulling from a loaded model, we know that it has been converted to the latest - // supported version. We include that here to ensure we don't hit any validation issues from - // using new features. - FullyResolvedModelFile resolvedFile = FullyResolvedModelFile.fromShapes( - traitFactory, nonPrelude, Version.fromString(Model.MODEL_VERSION)); + FullyResolvedModelFile resolvedFile = FullyResolvedModelFile.fromShapes(traitFactory, nonPrelude); model.getMetadata().forEach(resolvedFile::putMetadata); modelFiles.add(resolvedFile); } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelUpgrader.java b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelUpgrader.java index 12a0ab070f4..a2bff3cf756 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelUpgrader.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelUpgrader.java @@ -91,13 +91,16 @@ ValidatedResult transform() { // We must assume v2 for manually created shapes. Version version = fileToVersion.getOrDefault(member.getSourceLocation().getFilename(), Version.VERSION_2_0); - if (version == Version.VERSION_1_0) { + + if (version == Version.VERSION_2_0) { + validateV2Member(member); + } else { + // Also attempt to upgrade unknown versions since they could be 1.0 and + // trying to upgrade 2.0 shapes has no effect. // For v1 shape checks, we need to know the containing shape type to apply the appropriate transform. model.getShape(member.getContainer()).ifPresent(container -> { upgradeV1Member(container.getType(), member); }); - } else if (version == Version.VERSION_2_0) { - validateV2Member(member); } } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/loader/Version.java b/smithy-model/src/main/java/software/amazon/smithy/model/loader/Version.java index 67a6c323d9e..f09e7b6ef27 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/loader/Version.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/loader/Version.java @@ -1,5 +1,5 @@ /* - * Copyright 2021 Amazon.com, Inc. or its affiliates. All Rights Reserved. + * Copyright 2022 Amazon.com, Inc. or its affiliates. All Rights Reserved. * * Licensed under the Apache License, Version 2.0 (the "License"). * You may not use this file except in compliance with the License. @@ -25,15 +25,121 @@ */ enum Version { - UNKNOWN(""), - VERSION_1_0("1.0"), - VERSION_2_0("2.0"); + /** + * Unknown is used for in-memory models that aren't tied to a specific version. + * For these kinds of models, we just assume every feature is supported. + * + *

When loading IDL models with no $version specified, the default assumed + * version is 1.0, not UNKNOWN (see {@link TraitContainer.VersionAwareTraitContainer}). + */ + UNKNOWN { + @Override + public String toString() { + return ""; + } - private final String version; + @Override + boolean supportsMixins() { + return true; + } - Version(String version) { - this.version = version; - } + @Override + boolean supportsInlineOperationIO() { + return true; + } + + @Override + boolean supportsTargetElision() { + return true; + } + + @Override + boolean isDefaultSupported() { + return true; + } + + @Override + boolean isShapeTypeSupported(ShapeType shapeType) { + return true; + } + }, + + VERSION_1_0 { + @Override + public String toString() { + return "1.0"; + } + + @Override + boolean supportsMixins() { + return false; + } + + @Override + boolean supportsInlineOperationIO() { + return false; + } + + @Override + boolean supportsTargetElision() { + return false; + } + + @Override + boolean isDefaultSupported() { + return false; + } + + @Override + boolean isShapeTypeSupported(ShapeType shapeType) { + return shapeType != ShapeType.ENUM && shapeType != ShapeType.INT_ENUM; + } + + @Override + void validateVersionedTrait(ShapeId target, ShapeId traitId, Node value) { + if (traitId.equals(MixinTrait.ID)) { + throw ModelSyntaxException.builder() + .message(String.format("Mixins can only be used in Smithy 2.0 or later. Attempted to apply " + + "a @mixin trait to `%s` in a model file using version `%s`.", + target, this)) + .shapeId(target) + .sourceLocation(value) + .build(); + } + } + }, + + VERSION_2_0 { + @Override + public String toString() { + return "2.0"; + } + + @Override + boolean supportsMixins() { + return true; + } + + @Override + boolean supportsInlineOperationIO() { + return true; + } + + @Override + boolean supportsTargetElision() { + return true; + } + + @Override + boolean isDefaultSupported() { + return true; + } + + @Override + boolean isShapeTypeSupported(ShapeType shapeType) { + return shapeType != ShapeType.SET; + } + }; /** * Creates a Version from a string, or returns null if the version @@ -55,28 +161,19 @@ static Version fromString(String value) { } } - @Override - public String toString() { - return version; - } - /** * Checks if this version of the IDL supports mixins. * * @return Returns true if this version supports mixins. */ - boolean supportsMixins() { - return this == VERSION_2_0; - } + abstract boolean supportsMixins(); /** * Checks if this version of the IDL supports inlined operation IO shapes. * * @return Returns true if this version supports inlined operation IO shapes. */ - boolean supportsInlineOperationIO() { - return this == VERSION_2_0; - } + abstract boolean supportsInlineOperationIO(); /** * Checks if this version of the IDL supports eliding targets for structures @@ -84,9 +181,13 @@ boolean supportsInlineOperationIO() { * * @return Returns true if the version supports eliding targets. */ - boolean supportsTargetElision() { - return this == VERSION_2_0; - } + abstract boolean supportsTargetElision(); + + /** + * Checks if the default trait is supported. + * @return Returns true if supported (i.e., IDL 2.0 or UNKNOWN). + */ + abstract boolean isDefaultSupported(); /** * Checks if the given shape type is supported in this version. @@ -94,17 +195,7 @@ boolean supportsTargetElision() { * @param shapeType The shape type to check. * @return Returns true if the shape type is supported in this version. */ - boolean isShapeTypeSupported(ShapeType shapeType) { - switch (shapeType) { - case SET: - return this == VERSION_1_0; - case ENUM: - case INT_ENUM: - return this == VERSION_2_0; - default: - return true; - } - } + abstract boolean isShapeTypeSupported(ShapeType shapeType); /** * Perform version-specific trait validation. @@ -115,18 +206,5 @@ boolean isShapeTypeSupported(ShapeType shapeType) { * @throws ModelSyntaxException if the given trait cannot be used in this version. */ void validateVersionedTrait(ShapeId target, ShapeId traitId, Node value) { - if (traitId.equals(MixinTrait.ID) && (this != Version.VERSION_2_0)) { - throw ModelSyntaxException.builder() - .message(String.format("Mixins can only be used in Smithy 2.0 or later. Attempted to apply " - + "a @mixin trait to `%s` in a model file using version `%s`.", - target, version)) - .shapeId(target) - .sourceLocation(value) - .build(); - } - } - - boolean isDefaultSupported() { - return this == VERSION_2_0; } } 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 05ab73ee0c4..97a0dc6e6c5 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 @@ -54,6 +54,8 @@ import software.amazon.smithy.model.node.Node; import software.amazon.smithy.model.node.ObjectNode; import software.amazon.smithy.model.node.StringNode; +import software.amazon.smithy.model.shapes.SetShape; +import software.amazon.smithy.model.shapes.SetShapeTest; import software.amazon.smithy.model.shapes.Shape; import software.amazon.smithy.model.shapes.ShapeId; import software.amazon.smithy.model.shapes.ShapeType; @@ -749,4 +751,18 @@ public void failsWhenMixinsConflictAndAreNotEquivalent() { assertThat(e.getMessage(), containsString("Conflicting shape definition for `smithy.example#A`")); } + + @Test + public void canLoadSetsUsingBuiltModel() { + SetShape set = SetShape.builder() + .id("smithy.example#Set") + .member(ShapeId.from("smithy.api#String")) + .build(); + Model model = Model.assembler() + .addShape(set) + .assemble() + .unwrap(); + + Model.assembler().addModel(model).assemble().unwrap(); + } } diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/transform/ChangeShapeTypeTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/transform/ChangeShapeTypeTest.java index a57bad6ee16..515233564b9 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/transform/ChangeShapeTypeTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/transform/ChangeShapeTypeTest.java @@ -330,7 +330,6 @@ public void canConvertEnumToString() { .build(); Model model = Model.assembler() - .setParsedShapesVersion("2.0") .addShape(startShape) .assemble() .unwrap(); @@ -431,7 +430,6 @@ public void canDowngradeEnums() { IntEnumShape intEnum = intEnumBuilder.addMember("FOO", 1).build(); Model model = Model.assembler() - .setParsedShapesVersion("2.0") .addShapes(stringEnum, intEnum) .assemble().unwrap(); Model result = ModelTransformer.create().downgradeEnums(model);