From 2bd81ac2d47803a59807856f064c8e1ee05bc6a2 Mon Sep 17 00:00:00 2001 From: Miles Ziemer <45497130+milesziemer@users.noreply.github.com> Date: Sun, 26 Nov 2023 15:44:44 -0500 Subject: [PATCH] Add synthetic box to root intEnums w/o a default (#2053) Primitive root shapes without the default trait have a synthetic box trait appplied when the shape is built in the loader (see https://github.com/smithy-lang/smithy/blob/d457aabb80feb4088caa3ac27d337b84e3ebc43d/smithy-model/src/main/java/software/amazon/smithy/model/loader/LoaderShapeMap.java#L366) if the shape would have a default of 0 in Smithy 1.0. Previously, this didn't include intEnum, so if you convert to 1.0, the intEnum which previously had no default in 2.0 will have a default of 0. This commit updates ModelInteropTransformer to consider intEnum as a shape that has a default value in 1.0, so the loader applies the synthetic box trait to root intEnums. Tests were updated to make sure box is added to root intEnums, and to fix some incorrect assertions. --- .../model/loader/ModelInteropTransformer.java | 8 ++-- .../loader/ModelInteropTransformerTest.java | 48 ++++++++++++++++++- .../intEnumSetToZeroValueToNonNullable.smithy | 6 +-- .../ast-serialization/cases/v2/enums.1.0.json | 5 +- 4 files changed, 57 insertions(+), 10 deletions(-) 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 907904bbd77..15ec4f75054 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 @@ -66,7 +66,8 @@ final class ModelInteropTransformer { ShapeType.LONG, ShapeType.FLOAT, ShapeType.DOUBLE, - ShapeType.BOOLEAN); + ShapeType.BOOLEAN, + ShapeType.INT_ENUM); // intEnum is actually an integer in v1, but the ShapeType is different. private final Model model; private final List events; @@ -167,10 +168,9 @@ private boolean v2ShapeNeedsBoxTrait(MemberShape member, Shape target) { && memberAndTargetAreNotAlreadyExplicitlyBoxed(member, target); } - // Only apply box to members where the trait can be applied. Note that intEnum is treated - // like a normal integer in v1 implementations, so it is allowed to be synthetically boxed. + // Only apply box to members where the trait can be applied. private boolean canBoxTargetThisKindOfShape(Shape target) { - return HAD_DEFAULT_VALUE_IN_1_0.contains(target.getType()) || target.isIntEnumShape(); + return HAD_DEFAULT_VALUE_IN_1_0.contains(target.getType()); } private boolean memberAndTargetAreNotAlreadyExplicitlyBoxed(MemberShape member, Shape target) { diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelInteropTransformerTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelInteropTransformerTest.java index d8c4bbc3c02..c55f8a902cf 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelInteropTransformerTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelInteropTransformerTest.java @@ -360,11 +360,28 @@ public void boxTraitOnlyAddedToRootWhenNotSetToZeroValueDefault() { + "@default(0)\n" + "integer PrimitiveInteger\n" + "\n" + + "intEnum BoxedIntEnum {\n" + + " ONE = 1\n" + + "}\n" + + "\n" + + "@default(1)\n" + + "intEnum BoxedIntEnumWithDefault {\n" + + " ONE = 1\n" + + "}\n" + + "\n" + + "@default(0)\n" + + "intEnum PrimitiveIntEnum {\n" + + " ZERO = 0\n" + + "}\n" + + "\n" + "structure Foo {\n" + " DefaultString: DefaultString = \"\"\n" + " BoxedInteger: BoxedInteger\n" + " PrimitiveInteger: PrimitiveInteger = 0\n" + " BoxedIntegerWithDefault: BoxedIntegerWithDefault = 1\n" + + " BoxedIntEnum: BoxedIntEnum\n" + + " BoxedIntEnumWithDefault: BoxedIntEnumWithDefault = 1\n" + + " PrimitiveIntEnum: PrimitiveIntEnum = 0\n" +"}\n") .assemble() .unwrap(); @@ -385,11 +402,20 @@ private void boxTraitRootAssertionsV2(Model model, int roundTrip) { ShapeId fooBoxedInteger = ShapeId.from("smithy.example#Foo$BoxedInteger"); ShapeId boxedIntegerWithDefault = ShapeId.from("smithy.example#BoxedIntegerWithDefault"); - ShapeId fooBoxedIntegerWithDefault = ShapeId.from("smithy.example#BoxedIntegerWithDefault"); + ShapeId fooBoxedIntegerWithDefault = ShapeId.from("smithy.example#Foo$BoxedIntegerWithDefault"); ShapeId primitiveInteger = ShapeId.from("smithy.example#PrimitiveInteger"); ShapeId fooPrimitiveInteger = ShapeId.from("smithy.example#Foo$PrimitiveInteger"); + ShapeId boxedIntEnum = ShapeId.from("smithy.example#BoxedIntEnum"); + ShapeId fooBoxedIntEnum = ShapeId.from("smithy.example#Foo$BoxedIntEnum"); + + ShapeId boxedIntEnumWithDefault = ShapeId.from("smithy.example#BoxedIntEnumWithDefault"); + ShapeId fooBoxedIntEnumWithDefault = ShapeId.from("smithy.example#Foo$BoxedIntEnumWithDefault"); + + ShapeId primitiveIntEnum = ShapeId.from("smithy.example#PrimitiveIntEnum"); + ShapeId fooPrimitiveIntEnum = ShapeId.from("smithy.example#Foo$PrimitiveIntEnum"); + // Do not box strings for v1 compatibility. assertThat(model.expectShape(defaultString).hasTrait(BoxTrait.class), is(false)); assertThat(model.expectShape(fooDefaultString).hasTrait(BoxTrait.class), is(false)); @@ -404,7 +430,7 @@ private void boxTraitRootAssertionsV2(Model model, int roundTrip) { // Add box to BoxedIntegerWithDefault because it has a default that isn't the v1 zero value. assertThat(model.expectShape(boxedIntegerWithDefault).hasTrait(BoxTrait.class), is(true)); - assertThat(model.expectShape(fooBoxedIntegerWithDefault).hasTrait(BoxTrait.class), is(true)); // no need to box the member too + assertThat(model.expectShape(fooBoxedIntegerWithDefault).hasTrait(BoxTrait.class), is(false)); // no need to box the member too assertThat(model.expectShape(boxedIntegerWithDefault).hasTrait(DefaultTrait.class), is(true)); assertThat(model.expectShape(fooBoxedIntegerWithDefault).hasTrait(DefaultTrait.class), is(true)); @@ -413,6 +439,24 @@ private void boxTraitRootAssertionsV2(Model model, int roundTrip) { assertThat(model.expectShape(fooPrimitiveInteger).hasTrait(BoxTrait.class), is(false)); assertThat(model.expectShape(primitiveInteger).hasTrait(DefaultTrait.class), is(true)); assertThat(model.expectShape(fooPrimitiveInteger).hasTrait(DefaultTrait.class), is(true)); + + // Add box to BoxedIntEnum because it has no default trait. + assertThat(model.expectShape(boxedIntEnum).hasTrait(BoxTrait.class), is(true)); + assertThat(model.expectShape(fooBoxedIntEnum).hasTrait(BoxTrait.class), is(false)); // no need to box the member too + assertThat(model.expectShape(boxedIntEnum).hasTrait(DefaultTrait.class), is(false)); + assertThat(model.expectShape(fooBoxedIntEnum).hasTrait(DefaultTrait.class), is(false)); + + // Add box to BoxedIntEnumWithDefault because it has a default that isn't the v1 zero value. + assertThat(model.expectShape(boxedIntEnumWithDefault).hasTrait(BoxTrait.class), is(true)); + assertThat(model.expectShape(fooBoxedIntEnumWithDefault).hasTrait(BoxTrait.class), is(false)); // no need to box the member too + assertThat(model.expectShape(boxedIntEnumWithDefault).hasTrait(DefaultTrait.class), is(true)); + assertThat(model.expectShape(fooBoxedIntEnumWithDefault).hasTrait(DefaultTrait.class), is(true)); + + // No box trait on PrimitiveIntEnum because it has a zero value default. + assertThat(model.expectShape(primitiveIntEnum).hasTrait(BoxTrait.class), is(false)); + assertThat(model.expectShape(fooPrimitiveIntEnum).hasTrait(BoxTrait.class), is(false)); + assertThat(model.expectShape(primitiveIntEnum).hasTrait(DefaultTrait.class), is(true)); + assertThat(model.expectShape(fooPrimitiveIntEnum).hasTrait(DefaultTrait.class), is(true)); } @Test diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/loader/upgrade-box/2-to-1/intEnumSetToZeroValueToNonNullable.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/upgrade-box/2-to-1/intEnumSetToZeroValueToNonNullable.smithy index 4a0d7660fb4..8b5d72b763d 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/loader/upgrade-box/2-to-1/intEnumSetToZeroValueToNonNullable.smithy +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/upgrade-box/2-to-1/intEnumSetToZeroValueToNonNullable.smithy @@ -1,11 +1,11 @@ -// {"v1-box": false, "v1-client-zero-value": false, "v2": false} +// {"v1-box": true, "v1-client-zero-value": false, "v2": false} +// V1 style box checks will think this member is nullable because it +// targets a shape with the box trait. $version: "2.0" namespace smithy.example structure Foo { - // V1 models treat intEnum as normal integers, so they just see a default zero value, hence this is - // non-nullable in v1 and v2. intEnumSetToZeroValueToNonNullable: MyIntEnum = 0 } diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/shapes/ast-serialization/cases/v2/enums.1.0.json b/smithy-model/src/test/resources/software/amazon/smithy/model/shapes/ast-serialization/cases/v2/enums.1.0.json index c02810a98ae..f294cb92704 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/shapes/ast-serialization/cases/v2/enums.1.0.json +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/shapes/ast-serialization/cases/v2/enums.1.0.json @@ -2,7 +2,10 @@ "smithy": "1.0", "shapes": { "smithy.example#IntEnum": { - "type": "integer" + "type": "integer", + "traits": { + "smithy.api#box": {} + } }, "smithy.example#StringEnum": { "type": "string",