From 1f0da40ff2ee24f863f4895189c4c3ec89d275e2 Mon Sep 17 00:00:00 2001 From: Michael Dowling Date: Sun, 11 Sep 2022 09:05:48 -0700 Subject: [PATCH] Fix 1-2 command, add tests, reduce diff noise --- .../cli/commands/Upgrade1to2Command.java | 57 ++++++++++----- .../cli/commands/Upgrade1to2CommandTest.java | 6 +- .../cli/commands/upgrade/cases/box.v1.smithy | 39 +++++++++++ .../cli/commands/upgrade/cases/box.v2.smithy | 35 ++++++++++ .../upgrade/cases/primitive.v2.smithy | 1 + .../diff/evaluators/ChangedDefault.java | 2 +- .../diff/evaluators/ChangedDefaultTest.java | 69 +++++++++++++++++++ .../smithy/model/loader/LoaderShapeMap.java | 2 +- .../smithy/model/loader/ModelAssembler.java | 4 +- ...ader.java => ModelInteropTransformer.java} | 38 +++++----- .../validation/node/BlobLengthPlugin.java | 11 ++- .../node/CollectionLengthPlugin.java | 5 +- .../validation/node/MapLengthPlugin.java | 5 +- .../validation/node/NodeValidatorPlugin.java | 4 ++ .../node/NonNumericFloatValuesPlugin.java | 3 +- .../validation/node/PatternTraitPlugin.java | 3 +- .../validation/node/StringEnumPlugin.java | 3 +- .../validation/node/StringLengthPlugin.java | 5 +- .../node/TimestampFormatPlugin.java | 20 +++--- .../node/TimestampValidationStrategy.java | 8 +-- .../validators/DefaultTraitValidator.java | 4 +- .../model/loader/ModelAssemblerTest.java | 34 +++++++-- ....java => ModelInteropTransformerTest.java} | 4 +- .../defaults/default-with-null.errors | 2 +- .../loader/needs-downgrade-document-node.json | 6 ++ .../loader/needs-upgrade-document-node.json | 6 ++ 26 files changed, 282 insertions(+), 94 deletions(-) create mode 100644 smithy-cli/src/test/resources/software/amazon/smithy/cli/commands/upgrade/cases/box.v1.smithy create mode 100644 smithy-cli/src/test/resources/software/amazon/smithy/cli/commands/upgrade/cases/box.v2.smithy rename smithy-model/src/main/java/software/amazon/smithy/model/loader/{ModelUpgrader.java => ModelInteropTransformer.java} (89%) rename smithy-model/src/test/java/software/amazon/smithy/model/loader/{ModelUpgraderTest.java => ModelInteropTransformerTest.java} (99%) diff --git a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/Upgrade1to2Command.java b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/Upgrade1to2Command.java index 66df4acbc..845dde79d 100644 --- a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/Upgrade1to2Command.java +++ b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/Upgrade1to2Command.java @@ -48,7 +48,6 @@ import software.amazon.smithy.model.SourceLocation; import software.amazon.smithy.model.loader.ModelAssembler; import software.amazon.smithy.model.loader.Prelude; import software.amazon.smithy.model.shapes.MemberShape; -import software.amazon.smithy.model.shapes.NumberShape; import software.amazon.smithy.model.shapes.Shape; import software.amazon.smithy.model.shapes.ShapeType; import software.amazon.smithy.model.shapes.ShapeVisitor; @@ -272,6 +271,8 @@ public final class Upgrade1to2Command extends SimpleCommand { protected Void getDefault(Shape shape) { if (shape.hasTrait(BoxTrait.class)) { writer.eraseTrait(shape, shape.expectTrait(BoxTrait.class)); + } else if (hasSyntheticDefault(shape)) { + addDefault(shape, shape.getType()); } // Handle members in reverse definition order. shape.members().stream() @@ -282,23 +283,7 @@ public final class Upgrade1to2Command extends SimpleCommand { private void handleMemberShape(MemberShape shape) { if (hasSyntheticDefault(shape)) { - SourceLocation memberLocation = shape.getSourceLocation(); - String padding = ""; - if (memberLocation.getColumn() > 1) { - padding = StringUtils.repeat(' ', memberLocation.getColumn() - 1); - } - Shape target = completeModel.expectShape(shape.getTarget()); - String defaultValue = ""; - if (target.isBooleanShape()) { - defaultValue = "false"; - } else if (target instanceof NumberShape) { - defaultValue = "0"; - } else if (target.isBlobShape() || target.isStringShape()) { - defaultValue = "\"\""; - } else { - throw new UnsupportedOperationException("Unexpected default: " + target); - } - writer.insertLine(shape.getSourceLocation().getLine(), padding + "@default(" + defaultValue + ")"); + addDefault(shape, completeModel.expectShape(shape.getTarget()).getType()); } if (shape.hasTrait(BoxTrait.class)) { @@ -306,7 +291,7 @@ public final class Upgrade1to2Command extends SimpleCommand { } } - private boolean hasSyntheticDefault(MemberShape shape) { + private boolean hasSyntheticDefault(Shape shape) { Optional defaultLocation = shape.getTrait(DefaultTrait.class) .map(Trait::getSourceLocation); // When Smithy injects the default trait, it sets the source @@ -316,6 +301,40 @@ public final class Upgrade1to2Command extends SimpleCommand { return defaultLocation.filter(location -> shape.getSourceLocation().equals(location)).isPresent(); } + private void addDefault(Shape shape, ShapeType targetType) { + SourceLocation memberLocation = shape.getSourceLocation(); + String padding = ""; + if (memberLocation.getColumn() > 1) { + padding = StringUtils.repeat(' ', memberLocation.getColumn() - 1); + } + String defaultValue = ""; + // Boxed members get a null default. + if (shape.hasTrait(BoxTrait.class)) { + defaultValue = "null"; + } else { + switch (targetType) { + case BOOLEAN: + defaultValue = "false"; + break; + case BYTE: + case SHORT: + case INTEGER: + case LONG: + case FLOAT: + case DOUBLE: + defaultValue = "0"; + break; + case BLOB: + case STRING: + defaultValue = "\"\""; + break; + default: + throw new UnsupportedOperationException("Unexpected default: " + targetType); + } + } + writer.insertLine(shape.getSourceLocation().getLine(), padding + "@default(" + defaultValue + ")"); + } + @Override public Void memberShape(MemberShape shape) { // members are handled from their containers so that they can diff --git a/smithy-cli/src/test/java/software/amazon/smithy/cli/commands/Upgrade1to2CommandTest.java b/smithy-cli/src/test/java/software/amazon/smithy/cli/commands/Upgrade1to2CommandTest.java index 787f26791..236b265a1 100644 --- a/smithy-cli/src/test/java/software/amazon/smithy/cli/commands/Upgrade1to2CommandTest.java +++ b/smithy-cli/src/test/java/software/amazon/smithy/cli/commands/Upgrade1to2CommandTest.java @@ -31,6 +31,7 @@ import java.nio.file.attribute.FileTime; import java.util.Set; import java.util.stream.Collectors; import java.util.stream.Stream; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; @@ -51,7 +52,10 @@ public class Upgrade1to2CommandTest { Model model = Model.assembler().addImport(initialPath).assemble().unwrap(); String actual = new Upgrade1to2Command("smithy").upgradeFile(model, initialPath); String expected = IoUtils.readUtf8File(expectedPath); - assertThat(actual, equalTo(expected)); + + if (!actual.equals(expected)) { + Assertions.fail("Expected models to be equal:\n\nActual:\n\n" + actual + "\n\nExpected:\n\n" + expected); + } } public static Stream source() throws Exception { diff --git a/smithy-cli/src/test/resources/software/amazon/smithy/cli/commands/upgrade/cases/box.v1.smithy b/smithy-cli/src/test/resources/software/amazon/smithy/cli/commands/upgrade/cases/box.v1.smithy new file mode 100644 index 000000000..6442e8317 --- /dev/null +++ b/smithy-cli/src/test/resources/software/amazon/smithy/cli/commands/upgrade/cases/box.v1.smithy @@ -0,0 +1,39 @@ +$version: "1.0" + +namespace com.example + +@box +integer BoxedInteger + +integer NonBoxedInteger + +structure StructureWithOptionalString { + boxedTarget: BoxedInteger, + + @box + boxedMember: NonBoxedInteger, +} + +union BoxyUnion { + boxedTarget: BoxedInteger, + + @box + boxedMember: NonBoxedInteger, +} + +list BadSparseList { + @box + member: NonBoxedInteger, +} + +set BadSparseSet { + @box + member: NonBoxedInteger, +} + +map BadSparseMap { + key: String, + + @box + value: NonBoxedInteger, +} diff --git a/smithy-cli/src/test/resources/software/amazon/smithy/cli/commands/upgrade/cases/box.v2.smithy b/smithy-cli/src/test/resources/software/amazon/smithy/cli/commands/upgrade/cases/box.v2.smithy new file mode 100644 index 000000000..8633ec27d --- /dev/null +++ b/smithy-cli/src/test/resources/software/amazon/smithy/cli/commands/upgrade/cases/box.v2.smithy @@ -0,0 +1,35 @@ +$version: "2.0" + +namespace com.example + +integer BoxedInteger + +@default(0) +integer NonBoxedInteger + +structure StructureWithOptionalString { + boxedTarget: BoxedInteger, + + @default(null) + boxedMember: NonBoxedInteger, +} + +union BoxyUnion { + boxedTarget: BoxedInteger, + + boxedMember: NonBoxedInteger, +} + +list BadSparseList { + member: NonBoxedInteger, +} + +set BadSparseSet { + member: NonBoxedInteger, +} + +map BadSparseMap { + key: String, + + value: NonBoxedInteger, +} diff --git a/smithy-cli/src/test/resources/software/amazon/smithy/cli/commands/upgrade/cases/primitive.v2.smithy b/smithy-cli/src/test/resources/software/amazon/smithy/cli/commands/upgrade/cases/primitive.v2.smithy index 60db4c4d8..68047e0aa 100644 --- a/smithy-cli/src/test/resources/software/amazon/smithy/cli/commands/upgrade/cases/primitive.v2.smithy +++ b/smithy-cli/src/test/resources/software/amazon/smithy/cli/commands/upgrade/cases/primitive.v2.smithy @@ -25,5 +25,6 @@ structure PrimitiveBearer { @default(0) handlesRequired: PrimitiveLong, + @default(null) handlesBox: PrimitiveByte, } diff --git a/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedDefault.java b/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedDefault.java index 5073e8d50..055496839 100644 --- a/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedDefault.java +++ b/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedDefault.java @@ -77,7 +77,7 @@ public final class ChangedDefault extends AbstractDiffEvaluator { } } } else if (!oldTrait.toNode().equals(newTrait.toNode())) { - if (change.getNewShape().asMemberShape().isPresent()) { + if (change.getNewShape().isMemberShape()) { evaluateChangedTrait(model, change.getNewShape().asMemberShape().get(), oldTrait, newTrait, events); } else { events.add(error(change.getNewShape(), newTrait, diff --git a/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedDefaultTest.java b/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedDefaultTest.java index 89a9bee68..d0a6e873b 100644 --- a/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedDefaultTest.java +++ b/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedDefaultTest.java @@ -1,6 +1,7 @@ package software.amazon.smithy.diff.evaluators; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.equalTo; import java.util.List; @@ -114,6 +115,30 @@ public class ChangedDefaultTest { assertThat(TestHelper.findEvents(events, "ChangedNullability").size(), equalTo(1)); } + @Test + public void updateModelWithAddedDefault() { + String originalModel = + "$version: \"2\"\n" + + "namespace smithy.example\n" + + "structure Foo {\n" + + " @required\n" + + " bar: Integer\n" + + "}\n"; + String updatedModel = + "$version: \"2\"\n" + + "namespace smithy.example\n" + + "structure Foo {\n" + + " @addedDefault\n" + + " bar: Integer = 1\n" + + "}\n"; + Model modelA = Model.assembler().addUnparsedModel("test.smithy", originalModel).assemble().unwrap(); + Model modelB = Model.assembler().addUnparsedModel("test.smithy", updatedModel).assemble().unwrap(); + List events = ModelDiff.compare(modelA, modelB); + + assertThat(TestHelper.findEvents(events, "ChangedDefault"), empty()); + assertThat(TestHelper.findEvents(events, "ChangedNullability"), empty()); + } + @Test public void errorWhenDefaultChangesFromZeroToNonZeroValue() { String originalModel = @@ -179,4 +204,48 @@ public class ChangedDefaultTest { assertThat(TestHelper.findEvents(events, "ChangedDefault").size(), equalTo(0)); assertThat(TestHelper.findEvents(events, "ChangedNullability").size(), equalTo(0)); } + + @Test + public void changingFromNullDefaultToOneIsBreaking() { + String originalModel = + "$version: \"2\"\n" + + "namespace smithy.example\n" + + "structure Foo {\n" + + " bar: Integer = null\n" + + "}\n"; + String updatedModel = + "$version: \"2\"\n" + + "namespace smithy.example\n" + + "structure Foo {\n" + + " bar: Integer = 1\n" + + "}\n"; + Model modelA = Model.assembler().addUnparsedModel("test.smithy", originalModel).assemble().unwrap(); + Model modelB = Model.assembler().addUnparsedModel("test.smithy", updatedModel).assemble().unwrap(); + List events = ModelDiff.compare(modelA, modelB); + + assertThat(TestHelper.findEvents(events, "ChangedDefault").size(), equalTo(1)); + assertThat(TestHelper.findEvents(events, "ChangedNullability").size(), equalTo(1)); + } + + @Test + public void changingFromNullDefaultToZeroIsBreaking() { + String originalModel = + "$version: \"2\"\n" + + "namespace smithy.example\n" + + "structure Foo {\n" + + " bar: Integer = null\n" + + "}\n"; + String updatedModel = + "$version: \"2\"\n" + + "namespace smithy.example\n" + + "structure Foo {\n" + + " bar: Integer = 0\n" + + "}\n"; + Model modelA = Model.assembler().addUnparsedModel("test.smithy", originalModel).assemble().unwrap(); + Model modelB = Model.assembler().addUnparsedModel("test.smithy", updatedModel).assemble().unwrap(); + List events = ModelDiff.compare(modelA, modelB); + + assertThat(TestHelper.findEvents(events, "ChangedDefault").size(), equalTo(1)); + assertThat(TestHelper.findEvents(events, "ChangedNullability").size(), equalTo(1)); + } } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/loader/LoaderShapeMap.java b/smithy-model/src/main/java/software/amazon/smithy/model/loader/LoaderShapeMap.java index d0bedfd39..9ef8e8cf3 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/loader/LoaderShapeMap.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/loader/LoaderShapeMap.java @@ -363,7 +363,7 @@ final class LoaderShapeMap { ) { try { AbstractShapeBuilder builder = defineShape.builder(); - ModelUpgrader.patchShapeBeforeBuilding(defineShape, builder, events); + ModelInteropTransformer.patchShapeBeforeBuilding(defineShape, builder, events); for (MemberShape.Builder memberBuilder : defineShape.memberBuilders().values()) { for (ShapeModifier modifier : defineShape.modifiers()) { 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 cf9a2db2d..60d749ea6 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 @@ -555,8 +555,8 @@ public final class ModelAssembler { } // Do the 1.0 -> 2.0 transform before full-model validation. - ValidatedResult transformed = new ModelUpgrader(processedModel, events, processor::getShapeVersion) - .transform(); + ValidatedResult transformed = new ModelInteropTransformer(processedModel, events, + processor::getShapeVersion).transform(); if (disableValidation || !transformed.getResult().isPresent() 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/ModelInteropTransformer.java similarity index 89% rename from smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelUpgrader.java rename to smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelInteropTransformer.java index 9e67199e8..253b7278d 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/ModelInteropTransformer.java @@ -19,7 +19,6 @@ import java.util.ArrayList; import java.util.Collections; import java.util.EnumSet; import java.util.List; -import java.util.Set; import java.util.function.Function; import software.amazon.smithy.model.FromSourceLocation; import software.amazon.smithy.model.Model; @@ -44,15 +43,21 @@ 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; -import software.amazon.smithy.utils.SetUtils; /** - * Upgrades Smithy models from IDL v1 to IDL v2, specifically taking into - * account the removal of the box trait, the change in default value - * semantics of numbers and booleans, and the @default trait. + * Ensures interoperability between IDL 1 and IDL 2 models by adding default traits and synthetic box traits where + * needed to normalize across versions. + * + *

For 1 to 2 interop, this class adds default traits. If a root level v1 shape is not boxed and supports the box + * trait, a default trait is added set to the zero value of the type. If a v1 member is marked with the box trait, + * then a default trait is added to the member set to null. If a member is targets a streaming blob and is not + * required, the default trait is added set to an empty string. + * + *

For 2 to 1 interop, if a v2 member has a default set to null a synthetic box trait is added. If a root level + * does not have the default trait and the type supports the box trait, a synthetic box trait is added. */ @SuppressWarnings("deprecation") -final class ModelUpgrader { +final class ModelInteropTransformer { /** Shape types in Smithy 1.0 that had a default value. */ private static final EnumSet HAD_DEFAULT_VALUE_IN_1_0 = EnumSet.of( @@ -64,20 +69,12 @@ final class ModelUpgrader { ShapeType.DOUBLE, ShapeType.BOOLEAN); - private static final Set SUPPORTS_RANGE_TRAIT_AND_ZERO_VALUE = SetUtils.of( - ShapeType.BYTE, - ShapeType.SHORT, - ShapeType.INTEGER, - ShapeType.LONG, - ShapeType.FLOAT, - ShapeType.DOUBLE); - private final Model model; private final List events; private final Function fileToVersion; private final List shapeUpgrades = new ArrayList<>(); - ModelUpgrader(Model model, List events, Function fileToVersion) { + ModelInteropTransformer(Model model, List events, Function fileToVersion) { this.model = model; this.events = events; this.fileToVersion = fileToVersion; @@ -123,24 +120,23 @@ final class ModelUpgrader { } else if (member.hasTrait(BoxTrait.class)) { // Add a default trait to the member set to null to indicate it was boxed in v1. MemberShape.Builder builder = member.toBuilder(); - builder.addTrait(new DefaultTrait(Node.nullNode())); + builder.addTrait(new DefaultTrait(new NullNode(member.getSourceLocation()))); shapeUpgrades.add(builder.build()); } } private boolean shouldV1MemberHaveDefaultTrait(MemberShape member, Shape target) { // Only when the targeted shape had a default value by default in v1 or if - // the member has the http payload trait and targets a streaming blob, which - // implies a default in 2.0 - return (HAD_DEFAULT_VALUE_IN_1_0.contains(target.getType()) || isDefaultPayload(member, target)) + // the member targets a streaming blob, which implies a default in 2.0 + return (HAD_DEFAULT_VALUE_IN_1_0.contains(target.getType()) || streamingBlobNeedsDefault(member, target)) // Don't re-add the @default trait && !member.hasTrait(DefaultTrait.ID) // Don't add a @default trait if the member or target are considered boxed in v1. && memberAndTargetAreNotAlreadyExplicitlyBoxed(member, target); } - private boolean isDefaultPayload(MemberShape member, Shape target) { - // httpPayload requires that the member is required or default. + private boolean streamingBlobNeedsDefault(MemberShape member, Shape target) { + // Streaming blobs require that the member is required or default. // No need to add the default trait if the member is required. if (member.isRequired()) { return false; diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/BlobLengthPlugin.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/BlobLengthPlugin.java index a1f17bf93..9b566083f 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/BlobLengthPlugin.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/BlobLengthPlugin.java @@ -20,7 +20,6 @@ import software.amazon.smithy.model.node.StringNode; import software.amazon.smithy.model.shapes.BlobShape; import software.amazon.smithy.model.shapes.Shape; import software.amazon.smithy.model.traits.LengthTrait; -import software.amazon.smithy.model.validation.Severity; import software.amazon.smithy.utils.SmithyInternalApi; /** @@ -40,17 +39,15 @@ final class BlobLengthPlugin extends MemberAndShapeTraitPlugin { if (size < min) { - emitter.accept(node, Severity.ERROR, - "Value provided for `" + shape.getId() + "` must have at least " - + min + " bytes, but the provided value only has " + size + " bytes"); + emitter.accept(node, "Value provided for `" + shape.getId() + "` must have at least " + + min + " bytes, but the provided value only has " + size + " bytes"); } }); trait.getMax().ifPresent(max -> { if (value.getBytes(StandardCharsets.UTF_8).length > max) { - emitter.accept(node, Severity.ERROR, - "Value provided for `" + shape.getId() + "` must have no more than " - + max + " bytes, but the provided value has " + size + " bytes"); + emitter.accept(node, "Value provided for `" + shape.getId() + "` must have no more than " + + max + " bytes, but the provided value has " + size + " bytes"); } }); } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/CollectionLengthPlugin.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/CollectionLengthPlugin.java index 11a32c97b..2dd1305db 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/CollectionLengthPlugin.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/CollectionLengthPlugin.java @@ -19,7 +19,6 @@ import software.amazon.smithy.model.node.ArrayNode; import software.amazon.smithy.model.shapes.CollectionShape; import software.amazon.smithy.model.shapes.Shape; import software.amazon.smithy.model.traits.LengthTrait; -import software.amazon.smithy.model.validation.Severity; import software.amazon.smithy.utils.SmithyInternalApi; /** @@ -37,7 +36,7 @@ final class CollectionLengthPlugin extends MemberAndShapeTraitPlugin { if (node.size() < min) { - emitter.accept(node, Severity.ERROR, String.format( + emitter.accept(node, String.format( "Value provided for `%s` must have at least %d elements, but the provided value only " + "has %d elements", shape.getId(), min, node.size())); } @@ -45,7 +44,7 @@ final class CollectionLengthPlugin extends MemberAndShapeTraitPlugin { if (node.size() > max) { - emitter.accept(node, Severity.ERROR, String.format( + emitter.accept(node, String.format( "Value provided for `%s` must have no more than %d elements, but the provided value " + "has %d elements", shape.getId(), max, node.size())); } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/MapLengthPlugin.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/MapLengthPlugin.java index 0caaa90fd..ac06ed765 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/MapLengthPlugin.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/MapLengthPlugin.java @@ -19,7 +19,6 @@ import software.amazon.smithy.model.node.ObjectNode; import software.amazon.smithy.model.shapes.MapShape; import software.amazon.smithy.model.shapes.Shape; import software.amazon.smithy.model.traits.LengthTrait; -import software.amazon.smithy.model.validation.Severity; import software.amazon.smithy.utils.SmithyInternalApi; /** @@ -36,7 +35,7 @@ final class MapLengthPlugin extends MemberAndShapeTraitPlugin { if (node.size() < min) { - emitter.accept(node, Severity.ERROR, String.format( + emitter.accept(node, String.format( "Value provided for `%s` must have at least %d entries, but the provided value only " + "has %d entries", shape.getId(), min, node.size())); } @@ -44,7 +43,7 @@ final class MapLengthPlugin extends MemberAndShapeTraitPlugin { if (node.size() > max) { - emitter.accept(node, Severity.ERROR, String.format( + emitter.accept(node, String.format( "Value provided for `%s` must have no more than %d entries, but the provided value " + "has %d entries", shape.getId(), max, node.size())); } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/NodeValidatorPlugin.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/NodeValidatorPlugin.java index cda3046a9..4de0373e1 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/NodeValidatorPlugin.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/NodeValidatorPlugin.java @@ -126,5 +126,9 @@ public interface NodeValidatorPlugin { @FunctionalInterface interface Emitter { void accept(FromSourceLocation sourceLocation, Severity severity, String message); + + default void accept(FromSourceLocation sourceLocation, String message) { + accept(sourceLocation, Severity.ERROR, message); + } } } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/NonNumericFloatValuesPlugin.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/NonNumericFloatValuesPlugin.java index 745dae57a..a5b7b5359 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/NonNumericFloatValuesPlugin.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/NonNumericFloatValuesPlugin.java @@ -19,7 +19,6 @@ import java.util.Set; import software.amazon.smithy.model.node.Node; import software.amazon.smithy.model.node.Node.NonNumericFloat; import software.amazon.smithy.model.shapes.Shape; -import software.amazon.smithy.model.validation.Severity; /** * Validates the specific set of non-numeric values allowed for floats and doubles. @@ -34,7 +33,7 @@ final class NonNumericFloatValuesPlugin implements NodeValidatorPlugin { } String nodeValue = value.expectStringNode().getValue(); if (!NON_NUMERIC_FLOAT_VALUES.contains(nodeValue)) { - emitter.accept(value, Severity.ERROR, String.format( + emitter.accept(value, String.format( "Value for `%s` must either be numeric or one of the following strings: [\"%s\"], but was \"%s\"", shape.getId(), String.join("\", \"", NON_NUMERIC_FLOAT_VALUES), nodeValue )); diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/PatternTraitPlugin.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/PatternTraitPlugin.java index 6bf8e2358..987b3d002 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/PatternTraitPlugin.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/PatternTraitPlugin.java @@ -19,7 +19,6 @@ import software.amazon.smithy.model.node.StringNode; import software.amazon.smithy.model.shapes.Shape; import software.amazon.smithy.model.shapes.StringShape; import software.amazon.smithy.model.traits.PatternTrait; -import software.amazon.smithy.model.validation.Severity; /** * Validates the pattern trait on string shapes or members that target them. @@ -33,7 +32,7 @@ final class PatternTraitPlugin extends MemberAndShapeTraitPlugin { shape.getTrait(EnumTrait.class).ifPresent(trait -> { List values = trait.getEnumDefinitionValues(); if (!values.contains(node.getValue())) { - emitter.accept(node, Severity.ERROR, String.format( + emitter.accept(node, String.format( "String value provided for `%s` must be one of the following values: %s", shape.getId(), ValidationUtils.tickedList(values))); } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/StringLengthPlugin.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/StringLengthPlugin.java index 7bc944531..ee6c4ee20 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/StringLengthPlugin.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/StringLengthPlugin.java @@ -19,7 +19,6 @@ import software.amazon.smithy.model.node.StringNode; import software.amazon.smithy.model.shapes.Shape; import software.amazon.smithy.model.shapes.StringShape; import software.amazon.smithy.model.traits.LengthTrait; -import software.amazon.smithy.model.validation.Severity; /** * Validates the length trait on string shapes or members that target them. @@ -34,7 +33,7 @@ final class StringLengthPlugin extends MemberAndShapeTraitPlugin { if (node.getValue().length() < min) { - emitter.accept(node, Severity.ERROR, String.format( + emitter.accept(node, String.format( "String value provided for `%s` must be >= %d characters, but the provided value is " + "only %d characters.", shape.getId(), min, node.getValue().length())); } @@ -42,7 +41,7 @@ final class StringLengthPlugin extends MemberAndShapeTraitPlugin { if (node.getValue().length() > max) { - emitter.accept(node, Severity.ERROR, String.format( + emitter.accept(node, String.format( "String value provided for `%s` must be <= %d characters, but the provided value is " + "%d characters.", shape.getId(), max, node.getValue().length())); } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/TimestampFormatPlugin.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/TimestampFormatPlugin.java index bd508141f..6833bc757 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/TimestampFormatPlugin.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/TimestampFormatPlugin.java @@ -80,19 +80,18 @@ final class TimestampFormatPlugin implements NodeValidatorPlugin { if (value.isStringNode()) { validateDatetime(shape, value, emitter); } else { - emitter.accept(value, Severity.ERROR, - "Invalid " + value.getType() + " value provided for timestamp, `" - + shape.getId() + "`. Expected a number that contains epoch seconds with " - + "optional millisecond precision, or a string that contains an RFC 3339 " - + "formatted timestamp (e.g., \"1985-04-12T23:20:50.52Z\")"); + emitter.accept(value, "Invalid " + value.getType() + " value provided for timestamp, `" + + shape.getId() + "`. Expected a number that contains epoch seconds with " + + "optional millisecond precision, or a string that contains an RFC 3339 " + + "formatted timestamp (e.g., \"1985-04-12T23:20:50.52Z\")"); } } } private void validateDatetime(Shape shape, Node value, Emitter emitter) { if (!value.isStringNode()) { - emitter.accept(value, Severity.ERROR, "Expected a string value for a date-time timestamp " - + "(e.g., \"1985-04-12T23:20:50.52Z\")"); + emitter.accept(value, "Expected a string value for a date-time timestamp " + + "(e.g., \"1985-04-12T23:20:50.52Z\")"); return; } @@ -101,10 +100,9 @@ final class TimestampFormatPlugin implements NodeValidatorPlugin { // See: https://bugs.openjdk.java.net/browse/JDK-8166138 // However, Smithy doesn't allow offsets for timestamp shapes. if (!(timestamp.endsWith("Z") && isValidFormat(timestamp, DATE_TIME_Z))) { - emitter.accept(value, Severity.ERROR, - "Invalid string value, `" + timestamp + "`, provided for timestamp, `" - + shape.getId() + "`. Expected an RFC 3339 formatted timestamp (e.g., " - + "\"1985-04-12T23:20:50.52Z\")"); + emitter.accept(value, "Invalid string value, `" + timestamp + "`, provided for timestamp, `" + + shape.getId() + "`. Expected an RFC 3339 formatted timestamp (e.g., " + + "\"1985-04-12T23:20:50.52Z\")"); } } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/TimestampValidationStrategy.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/TimestampValidationStrategy.java index 072c81f4c..835b4a4af 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/TimestampValidationStrategy.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/TimestampValidationStrategy.java @@ -19,7 +19,6 @@ import software.amazon.smithy.model.Model; import software.amazon.smithy.model.node.Node; import software.amazon.smithy.model.shapes.MemberShape; import software.amazon.smithy.model.shapes.Shape; -import software.amazon.smithy.model.validation.Severity; /** * Defines how timestamps are validated. @@ -47,10 +46,9 @@ public enum TimestampValidationStrategy implements NodeValidatorPlugin { @Override public void apply(Shape shape, Node value, Context context, Emitter emitter) { if (isTimestampMember(context.model(), shape) && !value.isNumberNode()) { - emitter.accept(shape, Severity.ERROR, - "Invalid " + value.getType() + " value provided for timestamp, `" - + shape.getId() + "`. Expected a number that contains epoch " - + "seconds with optional millisecond precision"); + emitter.accept(shape, "Invalid " + value.getType() + " value provided for timestamp, `" + + shape.getId() + "`. Expected a number that contains epoch " + + "seconds with optional millisecond precision"); } } }; diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/DefaultTraitValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/DefaultTraitValidator.java index 91d1b7b4b..ff0bd2922 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/DefaultTraitValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/DefaultTraitValidator.java @@ -62,7 +62,7 @@ public final class DefaultTraitValidator extends AbstractValidator { if (memberDefault == null) { events.add(error(member, String.format( "Member targets %s, which requires that the member defines the same default " - + "of `%s`", + + "of `%s` or `null`", shape.toShapeId(), Node.printJson(value)))); } else if (!memberDefault.toNode().isNullNode() && !value.equals(member.expectTrait(DefaultTrait.class).toNode())) { @@ -101,7 +101,7 @@ public final class DefaultTraitValidator extends AbstractValidator { return visitor; } } else if (value.isNullNode()) { - events.add(error(shape, trait, "The @default trait can only be set to null on members")); + events.add(error(shape, trait, "The @default trait can be set to null only on members")); return visitor; } 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 9816b43a4..3c887675d 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 @@ -63,6 +63,7 @@ import software.amazon.smithy.model.shapes.ShapeType; import software.amazon.smithy.model.shapes.StringShape; import software.amazon.smithy.model.shapes.StructureShape; import software.amazon.smithy.model.traits.BoxTrait; +import software.amazon.smithy.model.traits.DefaultTrait; import software.amazon.smithy.model.traits.DeprecatedTrait; import software.amazon.smithy.model.traits.DocumentationTrait; import software.amazon.smithy.model.traits.DynamicTrait; @@ -958,6 +959,8 @@ public class ModelAssemblerTest { .assemble() .unwrap(); + assertionChecksFor_upgradesAndDowngrades(model1); + // And through unparsed. String contents = IoUtils.readUtf8Resource(getClass(), "needs-upgrade-document-node.json"); Model model2 = Model.assembler() @@ -965,6 +968,8 @@ public class ModelAssemblerTest { .assemble() .unwrap(); + assertionChecksFor_upgradesAndDowngrades(model2); + // And through document node. Node node = Node.parse(contents); Model model3 = Model.assembler() @@ -972,6 +977,8 @@ public class ModelAssemblerTest { .assemble() .unwrap(); + assertionChecksFor_upgradesAndDowngrades(model3); + // Pathological case. Model model4 = Model.assembler() .addModel(model1) @@ -982,22 +989,37 @@ public class ModelAssemblerTest { .assemble() .unwrap(); + assertionChecksFor_upgradesAndDowngrades(model4); + assertThat(model1, equalTo(model2)); assertThat(model1, equalTo(model3)); assertThat(model1, equalTo(model4)); } + private void assertionChecksFor_upgradesAndDowngrades(Model model) { + ShapeId boxDouble = ShapeId.from("smithy.example#BoxDouble"); + ShapeId primitiveDouble = ShapeId.from("smithy.example#PrimitiveDouble"); + ShapeId fooMember = ShapeId.from("smithy.example#Struct$foo"); + ShapeId barMember = ShapeId.from("smithy.example#Struct$bar"); + ShapeId boxedMember = ShapeId.from("smithy.example#Struct$boxed"); + assertThat(model.expectShape(boxDouble).hasTrait(DefaultTrait.class), is(false)); + assertThat(model.expectShape(primitiveDouble).hasTrait(DefaultTrait.class), is(true)); + assertThat(model.expectShape(fooMember).hasTrait(DefaultTrait.class), is(true)); + assertThat(model.expectShape(barMember).hasTrait(DefaultTrait.class), is(false)); + assertThat(model.expectShape(boxedMember).hasTrait(DefaultTrait.class), is(true)); + assertThat(model.expectShape(boxedMember).expectTrait(DefaultTrait.class).toNode(), equalTo(Node.nullNode())); + assertThat(model.expectShape(boxedMember).hasTrait(BoxTrait.class), is(true)); + } + @Test public void patches2_0_documentNodesToo() { - ShapeId boxDouble = ShapeId.from("smithy.example#BoxDouble"); - // Loads fine through import. Model model1 = Model.assembler() .addImport(getClass().getResource("needs-downgrade-document-node.json")) .assemble() .unwrap(); - assertThat(model1.expectShape(boxDouble).hasTrait(BoxTrait.class), is(true)); + assertionChecksFor_upgradesAndDowngrades(model1); // And through unparsed. String contents = IoUtils.readUtf8Resource(getClass(), "needs-downgrade-document-node.json"); @@ -1006,7 +1028,7 @@ public class ModelAssemblerTest { .assemble() .unwrap(); - assertThat(model2.expectShape(boxDouble).hasTrait(BoxTrait.class), is(true)); + assertionChecksFor_upgradesAndDowngrades(model2); // And through document node. Node node = Node.parse(contents); @@ -1015,7 +1037,7 @@ public class ModelAssemblerTest { .assemble() .unwrap(); - assertThat(model3.expectShape(boxDouble).hasTrait(BoxTrait.class), is(true)); + assertionChecksFor_upgradesAndDowngrades(model3); // Pathological case. Model model4 = Model.assembler() @@ -1027,7 +1049,7 @@ public class ModelAssemblerTest { .assemble() .unwrap(); - assertThat(model4.expectShape(boxDouble).hasTrait(BoxTrait.class), is(true)); + assertionChecksFor_upgradesAndDowngrades(model4); assertThat(model1, equalTo(model2)); assertThat(model1, equalTo(model3)); diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelUpgraderTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelInteropTransformerTest.java similarity index 99% rename from smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelUpgraderTest.java rename to smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelInteropTransformerTest.java index 0a72db8e1..d8c4bbc3c 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelUpgraderTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelInteropTransformerTest.java @@ -38,7 +38,7 @@ import software.amazon.smithy.utils.IoUtils; import software.amazon.smithy.utils.ListUtils; import software.amazon.smithy.utils.Pair; -public class ModelUpgraderTest { +public class ModelInteropTransformerTest { @Test public void upgradesWhenAllModelsUse1_0() { UpgradeTestCase testCase = UpgradeTestCase.createAndValidate("upgrade/all-1.0"); @@ -144,7 +144,7 @@ public class ModelUpgraderTest { } private static UpgradeTestCase createFromDirectory(String directory) { - try (Stream paths = Files.walk(Paths.get(ModelUpgraderTest.class.getResource(directory).toURI()))) { + try (Stream paths = Files.walk(Paths.get(ModelInteropTransformerTest.class.getResource(directory).toURI()))) { UpgradeTestCase testCase = new UpgradeTestCase(); paths.filter(Files::isRegularFile).forEach(file -> { if (file.endsWith("upgraded.smithy") || file.endsWith("upgraded.json")) { diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/defaults/default-with-null.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/defaults/default-with-null.errors index 5a42bf3b3..9948f80cd 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/defaults/default-with-null.errors +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/defaults/default-with-null.errors @@ -1 +1 @@ -[ERROR] smithy.example#NullableInteger: The @default trait can only be set to null on members | DefaultTrait +[ERROR] smithy.example#NullableInteger: The @default trait can be set to null only on members | DefaultTrait diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/loader/needs-downgrade-document-node.json b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/needs-downgrade-document-node.json index b7b2d5a42..342533059 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/loader/needs-downgrade-document-node.json +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/needs-downgrade-document-node.json @@ -21,6 +21,12 @@ }, "bar": { "target": "smithy.example#BoxDouble" + }, + "boxed": { + "target": "smithy.example#PrimitiveDouble", + "traits": { + "smithy.api#default": null + } } } } diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/loader/needs-upgrade-document-node.json b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/needs-upgrade-document-node.json index 98db42b65..d4b6965bf 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/loader/needs-upgrade-document-node.json +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/needs-upgrade-document-node.json @@ -18,6 +18,12 @@ }, "bar": { "target": "smithy.example#BoxDouble" + }, + "boxed": { + "target": "smithy.example#PrimitiveDouble", + "traits": { + "smithy.api#box": {} + } } } }