New diff evaluator for added required members (#1781)

smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/AddedRequiredMember.java

@@ -0,0 +1,61 @@
+/*
+ * Copyright 2023 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.
+ * A copy of the License is located at
+ *
+ *  http://aws.amazon.com/apache2.0
+ *
+ * or in the "license" file accompanying this file. This file is distributed
+ * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
+ * express or implied. See the License for the specific language governing
+ * permissions and limitations under the License.
+ */
+
+package software.amazon.smithy.diff.evaluators;
+
+import java.util.List;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+import software.amazon.smithy.diff.Differences;
+import software.amazon.smithy.model.shapes.MemberShape;
+import software.amazon.smithy.model.shapes.StructureShape;
+import software.amazon.smithy.model.traits.DefaultTrait;
+import software.amazon.smithy.model.traits.RequiredTrait;
+import software.amazon.smithy.model.validation.Severity;
+import software.amazon.smithy.model.validation.ValidationEvent;
+
+/**
+ * Validates that no members are newly created with the required trait
+ * (but no default trait) in existing structures.
+ */
+public class AddedRequiredMember extends AbstractDiffEvaluator {
+    @Override
+    public List<ValidationEvent> evaluate(Differences differences) {
+        List<ValidationEvent> events = newRequiredMembers(differences)
+                .map(this::emit)
+                .collect(Collectors.toList());
+
+        return events;
+    }
+
+    private Stream<MemberShape> newRequiredMembers(Differences differences) {
+        return differences.changedShapes(StructureShape.class)
+                .flatMap(change -> change.getNewShape().members().stream()
+                        .filter(newMember -> newMember.hasTrait(RequiredTrait.ID)
+                                && !newMember.hasTrait(DefaultTrait.ID)
+                                // Members that did not exist before
+                                && change.getOldShape().getAllMembers().get(newMember.getMemberName()) == null));
+    }
+
+    private ValidationEvent emit(MemberShape memberShape) {
+        return ValidationEvent.builder()
+                .id(getEventId())
+                .shapeId(memberShape.getId())
+                .message("Adding a new member with the `required` trait "
+                        + "but not the `default` trait is backwards-incompatible.")
+                .severity(Severity.ERROR)
+                .build();
+    }
+}

smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedNullability.java

@@ -163,7 +163,6 @@ private ValidationEvent emit(
                 .shapeId(shape)
                 .message(actualMessage)
                 .severity(severity)
-                .message(message)
                 .build();
     }
 }

smithy-diff/src/main/resources/META-INF/services/software.amazon.smithy.diff.DiffEvaluator

@@ -1,6 +1,7 @@
 software.amazon.smithy.diff.evaluators.AddedEntityBinding
 software.amazon.smithy.diff.evaluators.AddedMetadata
 software.amazon.smithy.diff.evaluators.AddedOperationError
+software.amazon.smithy.diff.evaluators.AddedRequiredMember
 software.amazon.smithy.diff.evaluators.AddedServiceError
 software.amazon.smithy.diff.evaluators.AddedShape
 software.amazon.smithy.diff.evaluators.AddedTraitDefinition

smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/AddedRequiredMemberTest.java

@@ -0,0 +1,86 @@
+package software.amazon.smithy.diff.evaluators;
+
+import org.junit.jupiter.api.Test;
+import software.amazon.smithy.diff.ModelDiff;
+import software.amazon.smithy.model.Model;
+import software.amazon.smithy.model.SourceLocation;
+import software.amazon.smithy.model.node.StringNode;
+import software.amazon.smithy.model.shapes.StringShape;
+import software.amazon.smithy.model.shapes.StructureShape;
+import software.amazon.smithy.model.traits.DefaultTrait;
+import software.amazon.smithy.model.traits.RequiredTrait;
+import software.amazon.smithy.model.validation.Severity;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+
+public class AddedRequiredMemberTest {
+    @Test
+    public void addingRequiredTraitWithoutDefaultIsAnError() {
+        StringShape s = StringShape.builder().id("smithy.example#Str").build();
+        StructureShape a = StructureShape.builder().id("smithy.example#A")
+                .build();
+        StructureShape b = StructureShape.builder().id("smithy.example#A")
+                .addMember("foo", s.getId(), b2 -> b2.addTrait(new RequiredTrait()))
+                .build();
+        Model model1 = Model.builder().addShapes(s, a).build();
+        Model model2 = Model.builder().addShapes(s, b).build();
+        ModelDiff.Result result = ModelDiff.builder().oldModel(model1).newModel(model2).compare();
+
+        assertThat(TestHelper.findEvents(result.getDiffEvents(), Severity.ERROR).size(), equalTo(1));
+        assertThat(TestHelper.findEvents(result.getDiffEvents(), "AddedRequiredMember").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(result.getDiffEvents(), "AddedRequiredMember").get(0).getShapeId().get().toString(),
+                equalTo("smithy.example#A$foo"));
+        assertThat(TestHelper.findEvents(result.getDiffEvents(), "AddedRequiredMember").get(0).getMessage(),
+                equalTo("Adding a new member with the `required` trait " +
+                        "but not the `default` trait is backwards-incompatible."));
+    }
+
+    @Test
+    public void addingRequiredTraitWithDefaultIsOk() {
+        StringShape s = StringShape.builder().id("smithy.example#Str").build();
+        StructureShape a = StructureShape.builder().id("smithy.example#A")
+                .build();
+        StructureShape b = StructureShape.builder().id("smithy.example#A")
+                .addMember("foo", s.getId(), b2 -> {
+                    b2.addTrait(new RequiredTrait());
+                    b2.addTrait(new DefaultTrait(new StringNode("default", SourceLocation.NONE)));
+                })
+                .build();
+        Model model1 = Model.builder().addShapes(s, a).build();
+        Model model2 = Model.builder().addShapes(s, b).build();
+        ModelDiff.Result result = ModelDiff.builder().oldModel(model1).newModel(model2).compare();
+
+        assertThat(TestHelper.findEvents(result.getDiffEvents(), "AddedRequiredMember").size(), equalTo(0));
+    }
+
+    @Test
+    public void addingRequiredTraitToExistingMember() {
+        StringShape s = StringShape.builder().id("smithy.example#Str").build();
+        StructureShape a = StructureShape.builder().id("smithy.example#A")
+                .addMember("foo", s.getId())
+                .build();
+        StructureShape b = StructureShape.builder().id("smithy.example#A")
+                .addMember("foo", s.getId(),
+                        b2 -> b2.addTrait(new RequiredTrait()))
+                .build();
+        Model model1 = Model.builder().addShapes(s, a).build();
+        Model model2 = Model.builder().addShapes(s, b).build();
+        ModelDiff.Result result = ModelDiff.builder().oldModel(model1).newModel(model2).compare();
+
+        assertThat(TestHelper.findEvents(result.getDiffEvents(), "AddedRequiredMember").size(), equalTo(0));
+    }
+
+    @Test
+    public void addingNewStructureWithRequiredMemberIsOk() {
+        StringShape s = StringShape.builder().id("smithy.example#Str").build();
+        StructureShape b = StructureShape.builder().id("smithy.example#A")
+                .addMember("foo", s.getId(), b2 -> b2.addTrait(new RequiredTrait()))
+                .build();
+        Model model1 = Model.builder().addShapes(s).build();
+        Model model2 = Model.builder().addShapes(s, b).build();
+        ModelDiff.Result result = ModelDiff.builder().oldModel(model1).newModel(model2).compare();
+
+        assertThat(TestHelper.findEvents(result.getDiffEvents(), "AddedRequiredMember").size(), equalTo(0));
+    }
+}