Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add synthetic box to root intEnums w/o a default #2053

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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<ValidationEvent> events;
Expand Down Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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));
Expand All @@ -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));

Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@
"smithy": "1.0",
"shapes": {
"smithy.example#IntEnum": {
"type": "integer"
"type": "integer",
"traits": {
"smithy.api#box": {}
}
},
"smithy.example#StringEnum": {
"type": "string",
Expand Down