Skip to content

Commit

Permalink
Improve ChangedNullability diff evaluator
Browse files Browse the repository at this point in the history 
This validator now does not emit trait change warnings when the required
trait is changed, and instead relies on ChangedNullability (this matches
how the box trait is handled too). When the required trait is removed
from a member that targets a structure or union, a warning is emitted
instead of an error to indicate that this is often allowed in Smithy
generators that treat these members as always nullable regardless of
the required trait.
  • Loading branch information
@mtdowling
mtdowling committed Oct 19, 2022
1 parent 6cc1ba4 commit b130116
Show file tree
Hide file tree
Showing 4 changed files with 111 additions and 4 deletions.
30 changes: 28 additions & 2 deletions smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedNullability.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,13 @@
import software.amazon.smithy.model.Model;
import software.amazon.smithy.model.knowledge.NullableIndex;
import software.amazon.smithy.model.shapes.MemberShape;
import software.amazon.smithy.model.shapes.Shape;
import software.amazon.smithy.model.shapes.StructureShape;
import software.amazon.smithy.model.traits.ClientOptionalTrait;
import software.amazon.smithy.model.traits.DefaultTrait;
import software.amazon.smithy.model.traits.InputTrait;
import software.amazon.smithy.model.traits.RequiredTrait;
import software.amazon.smithy.model.validation.Severity;
import software.amazon.smithy.model.validation.ValidationEvent;

/**
Expand Down Expand Up @@ -91,37 +93,61 @@ private ValidationEvent createError(
StringJoiner joiner = new StringJoiner("; ", message, "");
boolean oldHasInput = hasInputTrait(differences.getOldModel(), oldMember);
boolean newHasInput = hasInputTrait(differences.getNewModel(), newMember);
Shape newTarget = differences.getNewModel().expectShape(newMember.getTarget());
Severity severity = null;

if (oldHasInput && !newHasInput) {
// If there was an input trait before, but not now, then the nullability must have
// changed from nullable to non-nullable.
joiner.add("The @input trait was removed from " + newMember.getContainer());
severity = Severity.ERROR;
} else if (!oldHasInput && newHasInput) {
// If there was no input trait before, but there is now, then the nullability must have
// changed from non-nullable to nullable.
joiner.add("The @input trait was added to " + newMember.getContainer());
severity = Severity.DANGER;
} else if (!newHasInput) {
// Can't add nullable to a preexisting required member.
if (change.isTraitAdded(ClientOptionalTrait.ID) && change.isTraitInBoth(RequiredTrait.ID)) {
joiner.add("The @nullable trait was added to a @required member.");
severity = Severity.ERROR;
}
// Can't add required to a member unless the member is marked as nullable.
if (change.isTraitAdded(RequiredTrait.ID) && !newMember.hasTrait(ClientOptionalTrait.ID)) {
joiner.add("The @required trait was added to a member that is not marked as @nullable.");
severity = Severity.ERROR;
}
// Can't add the default trait to a member unless the member was previously required.
if (change.isTraitAdded(DefaultTrait.ID) && !change.isTraitRemoved(RequiredTrait.ID)) {
joiner.add("The @default trait was added to a member that was not previously @required.");
severity = Severity.ERROR;
}
// Can only remove the required trait if the member was nullable or replaced by the default trait.
if (change.isTraitRemoved(RequiredTrait.ID)
&& !newMember.hasTrait(DefaultTrait.ID)
&& !oldMember.hasTrait(ClientOptionalTrait.ID)) {
joiner.add("The @required trait was removed and not replaced with the @default trait.");
if (newTarget.isStructureShape() || newTarget.isUnionShape()) {
severity = severity == null ? Severity.WARNING : severity;
joiner.add("The @required trait was removed from a member that targets a " + newTarget.getType()
+ ". This is backward compatible in generators that always treat structures and "
+ "unions as optional (e.g., AWS generators)");
} else {
joiner.add("The @required trait was removed and not replaced with the @default trait and "
+ "@addedDefault trait.");
severity = Severity.ERROR;
}
}
}

return error(change.getNewShape(), joiner.toString());
// If no explicit severity was detected, then assume an error.
severity = severity == null ? Severity.ERROR : severity;

return ValidationEvent.builder()
.id(getEventId())
.shapeId(change.getNewShape())
.severity(severity)
.message(joiner.toString())
.build();
}

private boolean hasInputTrait(Model model, MemberShape member) {
Expand Down
8 changes: 7 additions & 1 deletion smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ModifiedTrait.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import software.amazon.smithy.diff.Differences;
import software.amazon.smithy.model.Model;
import software.amazon.smithy.model.node.Node;
Expand All @@ -34,11 +35,13 @@
import software.amazon.smithy.model.shapes.StructureShape;
import software.amazon.smithy.model.shapes.UnionShape;
import software.amazon.smithy.model.traits.BoxTrait;
import software.amazon.smithy.model.traits.RequiredTrait;
import software.amazon.smithy.model.traits.Trait;
import software.amazon.smithy.model.traits.TraitDefinition;
import software.amazon.smithy.model.validation.Severity;
import software.amazon.smithy.model.validation.ValidationEvent;
import software.amazon.smithy.utils.ListUtils;
import software.amazon.smithy.utils.SetUtils;

/**
* Finds breaking changes related to when a trait is added, removed, or
Expand Down Expand Up @@ -91,6 +94,9 @@ public final class ModifiedTrait extends AbstractDiffEvaluator {
new DiffStrategy(DiffType.UPDATE, Severity.NOTE),
new DiffStrategy(DiffType.REMOVE, Severity.WARNING));

/** Traits in this list have special backward compatibility rules and can't be validated here. */
private static final Set<ShapeId> IGNORED_TRAITS = SetUtils.of(BoxTrait.ID, RequiredTrait.ID);

@Override
public List<ValidationEvent> evaluate(Differences differences) {
// Map of trait shape ID to diff strategies to evaluate.
Expand All @@ -102,7 +108,7 @@ public List<ValidationEvent> evaluate(Differences differences) {
Trait oldTrait = oldTraitNewTraitPair.left;
Trait newTrait = oldTraitNewTraitPair.right;
// Do not emit for the box trait because it is added and removed for backward compatibility.
if (!traitId.equals(BoxTrait.ID) && strategies.containsKey(traitId)) {
if (!IGNORED_TRAITS.contains(traitId) && strategies.containsKey(traitId)) {
for (DiffStrategy strategy : strategies.get(traitId)) {
List<ValidationEvent> diffEvents = strategy.diffType.validate(
differences.getNewModel(),
Expand Down
54 changes: 53 additions & 1 deletion smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedNullabilityTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.empty;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasSize;
import static org.hamcrest.Matchers.is;

import java.util.List;
Expand All @@ -12,6 +13,7 @@
import software.amazon.smithy.model.node.Node;
import software.amazon.smithy.model.shapes.MemberShape;
import software.amazon.smithy.model.shapes.ModelSerializer;
import software.amazon.smithy.model.shapes.Shape;
import software.amazon.smithy.model.shapes.ShapeId;
import software.amazon.smithy.model.shapes.StringShape;
import software.amazon.smithy.model.shapes.StructureShape;
Expand All @@ -23,6 +25,7 @@
import software.amazon.smithy.model.transform.ModelTransformer;
import software.amazon.smithy.model.validation.Severity;
import software.amazon.smithy.model.validation.ValidationEvent;
import software.amazon.smithy.utils.ListUtils;

public class ChangedNullabilityTest {
@Test
Expand Down Expand Up @@ -197,7 +200,7 @@ public void detectsAdditionOfInputTrait() {
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);

assertThat(events.stream()
.filter(event -> event.getSeverity() == Severity.ERROR)
.filter(event -> event.getSeverity() == Severity.DANGER)
.filter(event -> event.getId().equals("ChangedNullability"))
.filter(event -> event.getMessage().contains("The @input trait was added to"))
.count(), equalTo(1L));
Expand Down Expand Up @@ -305,4 +308,53 @@ public void roundTrippedV1ModelHasNoEvents() {

assertThat(events, empty());
}

@Test
public void specialHandlingForRequiredStructureMembers() {
String originalModel =
"$version: \"2.0\"\n"
+ "namespace smithy.example\n"
+ "structure Baz {}\n"
+ "structure Foo {\n"
+ " @required\n"
+ " baz: Baz\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(RequiredTrait.ID)
.build()));

List<ValidationEvent> events = TestHelper.findEvents(
ModelDiff.compare(oldModel, newModel), "ChangedNullability");

assertThat(events, hasSize(1));
assertThat(events.get(0).getSeverity(), is(Severity.WARNING));
}

@Test
public void specialHandlingForRequiredUnionMembers() {
String originalModel =
"$version: \"2.0\"\n"
+ "namespace smithy.example\n"
+ "union Baz {\n"
+ " a: String\n"
+ " b: String\n"
+ "}\n"
+ "structure Foo {\n"
+ " @required\n"
+ " baz: Baz\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(RequiredTrait.ID)
.build()));

List<ValidationEvent> events = TestHelper.findEvents(
ModelDiff.compare(oldModel, newModel), "ChangedNullability");

assertThat(events, hasSize(1));
assertThat(events.get(0).getSeverity(), is(Severity.WARNING));
}
}
23 changes: 23 additions & 0 deletions smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ModifiedTraitTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.empty;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasSize;

Expand All @@ -35,10 +36,13 @@
import software.amazon.smithy.model.shapes.ShapeId;
import software.amazon.smithy.model.shapes.StringShape;
import software.amazon.smithy.model.traits.DynamicTrait;
import software.amazon.smithy.model.traits.RequiredTrait;
import software.amazon.smithy.model.traits.TagsTrait;
import software.amazon.smithy.model.traits.TraitDefinition;
import software.amazon.smithy.model.transform.ModelTransformer;
import software.amazon.smithy.model.validation.Severity;
import software.amazon.smithy.model.validation.ValidationEvent;
import software.amazon.smithy.utils.ListUtils;

public class ModifiedTraitTest {
private static final ShapeId ID = ShapeId.from("com.foo#baz");
Expand Down Expand Up @@ -267,4 +271,23 @@ public void findsDifferencesInTraitValuesOfAllSeverities() {
"Changed trait contents of `smithy.example#aTrait` at path `/l` from \"a\" to \"l\""
));
}

@Test
public void letsOtherValidatorsHandleRequiredTrait() {
String originalModel =
"$version: \"2.0\"\n"
+ "namespace smithy.example\n"
+ "structure Baz {}\n"
+ "structure Foo {\n"
+ " @required\n"
+ " baz: Baz\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(RequiredTrait.ID)
.build()));

assertThat(TestHelper.findEvents(ModelDiff.compare(oldModel, newModel), "ModifiedTrait"), empty());
}
}

0 comments on commit b130116

Please sign in to comment.