From 8beb77af9e9dd1ca8cdb822f3a99da5ecff24321 Mon Sep 17 00:00:00 2001 From: Michael Dowling Date: Fri, 21 Oct 2022 13:25:56 -0700 Subject: [PATCH] Correct precedence in NullableIndex and fix diffs Smithy-Diff was flagging acceptable changes of removing `@default(null)` from member when the trait had no effect because the target shape was already nullable. NullableIndex was also giving the `@default(null)` trait precedence over the `@required` trait which is incorrect. Required members are non-nullable, and the `@default(null)` trait doesn't override this. It simply means that there is no default value for the member (so coupled with the required trait, the member is still always present). --- .../diff/evaluators/ChangedDefault.java | 26 ++++++++++++++++--- .../evaluators/ChangedNullabilityTest.java | 24 +++++++++++++++++ .../smithy/model/knowledge/NullableIndex.java | 14 ++++------ .../model/knowledge/NullableIndexTest.java | 25 ++++++++++++++++++ 4 files changed, 77 insertions(+), 12 deletions(-) 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 0554968393b..7eb89926857 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 @@ -59,10 +59,11 @@ private void validateChange( DefaultTrait oldTrait, DefaultTrait newTrait ) { - // Removing the default trait is always a breaking change. if (newTrait == null) { - events.add(error(change.getNewShape(), - "@default trait was removed. This will break previously generated code.")); + if (!isInconsequentialRemovalOfDefaultTrait(model, oldTrait, change.getNewShape())) { + events.add(error(change.getNewShape(), + "@default trait was removed. This will break previously generated code.")); + } } else if (oldTrait == null) { if (change.getNewShape().getType() != ShapeType.MEMBER) { events.add(error(change.getNewShape(), newTrait, @@ -88,6 +89,25 @@ private void validateChange( } } + private boolean isInconsequentialRemovalOfDefaultTrait(Model model, DefaultTrait trait, Shape removedFrom) { + // Removing a default of null if the target is nullable is not an issue. + return removedFrom.asMemberShape() + .map(member -> { + if (trait.toNode().isNullNode()) { + // If the target has no defined default, then removing a default(null) trait is fine. + Node targetDefault = model.expectShape(member.getTarget()) + .getTrait(DefaultTrait.class) + .map(DefaultTrait::toNode) + .orElse(Node.nullNode()); // If no default, then assume target has a default of null. + return targetDefault.isNullNode(); + } else { + // Removing a non-null trait is always an issue. + return false; + } + }) + .orElse(false); + } + private void evaluateChangedTrait(Model model, MemberShape member, DefaultTrait oldTrait, DefaultTrait newTrait, List events) { Shape target = model.expectShape(member.getTarget()); diff --git a/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedNullabilityTest.java b/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedNullabilityTest.java index bf45c2ae942..8c7ea877b61 100644 --- a/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedNullabilityTest.java +++ b/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedNullabilityTest.java @@ -5,6 +5,7 @@ import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.not; import java.util.List; import org.junit.jupiter.api.Test; @@ -357,4 +358,27 @@ public void specialHandlingForRequiredUnionMembers() { assertThat(events, hasSize(1)); assertThat(events.get(0).getSeverity(), is(Severity.WARNING)); } + + @Test + public void doesNotWarnWhenExtraneousDefaultNullTraitRemoved() { + String originalModel = + "$version: \"2.0\"\n" + + "namespace smithy.example\n" + + "structure Foo {\n" + + " @required\n" + + " baz: Integer = null\n" + + "}\n"; + Model oldModel = Model.assembler().addUnparsedModel("foo.smithy", originalModel).assemble().unwrap(); + Model newModel = ModelTransformer.create().replaceShapes(oldModel, ListUtils.of( + Shape.shapeToBuilder(oldModel.expectShape(ShapeId.from("smithy.example#Foo$baz"))) + .removeTrait(DefaultTrait.ID) + .build())); + + // The only emitted even should be a warning about the removal of the default trait, which can be ignored + // given the effective nullability of the member is unchanged. + List events = ModelDiff.compare(oldModel, newModel); + assertThat(TestHelper.findEvents(events, "ChangedNullability"), empty()); + assertThat(TestHelper.findEvents(events, "ChangedDefault"), empty()); + assertThat(TestHelper.findEvents(events, "ModifiedTrait"), not(empty())); + } } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/knowledge/NullableIndex.java b/smithy-model/src/main/java/software/amazon/smithy/model/knowledge/NullableIndex.java index 7858db760f1..849fb00aefd 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/knowledge/NullableIndex.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/knowledge/NullableIndex.java @@ -135,15 +135,11 @@ boolean isStructureMemberOptional(StructureShape container, MemberShape member, SERVER { @Override boolean isStructureMemberOptional(StructureShape container, MemberShape member, Shape target) { - // Defaults set to null on a member make the member optional. - if (member.hasNullDefault()) { - return true; - } else if (member.hasNonNullDefault()) { - return false; - } else { - // A 2.0 member with the required trait is never nullable. - return !member.hasTrait(RequiredTrait.class); - } + // Evaluated in this order. + // 1. Does the member have the required trait? Stop further checks, it's non-optional. + // 2. Does the member have a default trait set to null? Stop further checks, it's optional. + // 3. Does the member have a default trait not set to null? Stop further checks, it's non-optional. + return !member.hasTrait(RequiredTrait.class) && !member.hasNonNullDefault(); } }; diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/knowledge/NullableIndexTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/knowledge/NullableIndexTest.java index 2afd6bcc1fa..f0fdfb386e8 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/knowledge/NullableIndexTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/knowledge/NullableIndexTest.java @@ -523,4 +523,29 @@ private void addedDefaultMakesMemberNullableInV1NotV2Assertions(Model model) { assertThat(index.isMemberNullable(model.expectShape(ShapeId.from("smithy.example#Foo$baz"), MemberShape.class)), is(false)); } + + // The required trait makes this non-nullable. The default(null) trait just means that there's no default value, + // it doesn't make the member nullable. A value is still required to be present. Therefore, it's non-nullable + // in code generated types. "default(null)" just means the framework does not provide a default value. + @Test + public void requiredMembersAreNonNullableEvenIfDefaultNullTraitIsPresent() { + String modelText = + "$version: \"2.0\"\n" + + "namespace smithy.example\n" + + "structure Foo {\n" + + " @required\n" + + " baz: Integer = null\n" + + "}\n"; + Model model = Model.assembler() + .addUnparsedModel("foo.smithy", modelText) + .assemble() + .unwrap(); + + NullableIndex index = NullableIndex.of(model); + + assertThat( + index.isMemberNullable(model.expectShape(ShapeId.from("smithy.example#Foo$baz"), MemberShape.class)), + is(false) + ); + } }