Skip to content

Commit

Permalink
Update smithy-diff for strings with the enum trait to enum shapes
Browse files Browse the repository at this point in the history 
Remove the following noise from smithy-diff:

- Errors for changing shape type from strings with the `enum` trait to enum shapes since the `enum` trait is deprecated.
- Notes for members added to the converted enum shapes.

Updated `ChangedEnumTrait` to not just compare changed `enum` traits, but also changes of an `enum` trait to its converted enum shape.
  • Loading branch information
Steven Yuan committed Sep 21, 2022
1 parent e04cca6 commit bc7bd4e
Show file tree
Hide file tree
Showing 6 changed files with 516 additions and 1 deletion.
28 changes: 28 additions & 0 deletions smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/AddedShape.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,15 @@
package software.amazon.smithy.diff.evaluators;

import java.util.List;
import java.util.Optional;
import java.util.stream.Collectors;
import software.amazon.smithy.diff.Differences;
import software.amazon.smithy.model.shapes.EnumShape;
import software.amazon.smithy.model.shapes.Shape;
import software.amazon.smithy.model.shapes.ShapeId;
import software.amazon.smithy.model.shapes.ShapeType;
import software.amazon.smithy.model.shapes.StringShape;
import software.amazon.smithy.model.traits.EnumTrait;
import software.amazon.smithy.model.validation.ValidationEvent;

/**
Expand All @@ -29,6 +35,7 @@ public final class AddedShape extends AbstractDiffEvaluator {
public List<ValidationEvent> evaluate(Differences differences) {
return differences.addedShapes()
.filter(shape -> !isMemberOfAddedShape(shape, differences))
.filter(shape -> !isMemberOfConvertedEnumShape(shape, differences))
.map(shape -> note(shape, String.format("Added %s `%s`", shape.getType(), shape.getId())))
.collect(Collectors.toList());
}
Expand All @@ -38,4 +45,25 @@ private boolean isMemberOfAddedShape(Shape shape, Differences differences) {
.filter(member -> !differences.getOldModel().getShapeIds().contains(member.getContainer()))
.isPresent();
}

private boolean isMemberOfConvertedEnumShape(Shape shape, Differences differences) {
if (!shape.asMemberShape().isPresent()) {
return false;
}

ShapeId conversionShapeId = shape.asMemberShape().get().getContainer();

Optional<StringShape> oldStringShapeWithEnumTrait = differences.getOldModel()
.getShape(conversionShapeId)
.flatMap(Shape::asStringShape)
.filter(s -> s.getType() == ShapeType.STRING)
.filter(s -> s.hasTrait(EnumTrait.ID));

Optional<EnumShape> newEnumShape = differences.getNewModel()
.getShape(conversionShapeId)
.flatMap(Shape::asEnumShape);

// Changes in enum values are handled in the ChangedEnumTrait evaluator
return newEnumShape.isPresent() && oldStringShapeWithEnumTrait.isPresent();
}
}
20 changes: 19 additions & 1 deletion smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedEnumTrait.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@

package software.amazon.smithy.diff.evaluators;

import static software.amazon.smithy.diff.evaluators.ChangedShapeType.expectedStringToEnumChange;

import java.util.ArrayList;
import java.util.List;
import java.util.Optional;
Expand All @@ -24,6 +26,7 @@
import software.amazon.smithy.model.shapes.Shape;
import software.amazon.smithy.model.traits.EnumDefinition;
import software.amazon.smithy.model.traits.EnumTrait;
import software.amazon.smithy.model.traits.synthetic.SyntheticEnumTrait;
import software.amazon.smithy.model.validation.ValidationEvent;
import software.amazon.smithy.utils.OptionalUtils;
import software.amazon.smithy.utils.Pair;
Expand All @@ -38,12 +41,27 @@ public final class ChangedEnumTrait extends AbstractDiffEvaluator {
@Override
public List<ValidationEvent> evaluate(Differences differences) {
return differences.changedShapes()
.flatMap(change -> OptionalUtils.stream(change.getChangedTrait(EnumTrait.class))
.flatMap(change -> OptionalUtils.stream(getChangedEnumTraitPair(change))
.map(p -> Pair.of(change, p)))
.flatMap(pair -> validateEnum(pair.getLeft(), pair.getRight()).stream())
.collect(Collectors.toList());
}

private Optional<Pair<EnumTrait, EnumTrait>> getChangedEnumTraitPair(ChangedShape<Shape> change) {
// Change between two enum traits
Optional<Pair<EnumTrait, EnumTrait>> changedEnumTrait = change.getChangedTrait(EnumTrait.class);
if (changedEnumTrait.isPresent()) {
return changedEnumTrait;
}
// Change between enum trait in old model and enum shape synthetic enum trait in new model
if (expectedStringToEnumChange(change)) {
return Optional.of(Pair.of(
change.getOldShape().expectTrait(EnumTrait.class),
change.getNewShape().expectTrait(SyntheticEnumTrait.class)));
}
return Optional.empty();
}

private List<ValidationEvent> validateEnum(ChangedShape<Shape> change, Pair<EnumTrait, EnumTrait> trait) {
EnumTrait oldTrait = trait.getLeft();
EnumTrait newTrait = trait.getRight();
Expand Down
8 changes: 8 additions & 0 deletions smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedShapeType.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import software.amazon.smithy.diff.Differences;
import software.amazon.smithy.model.shapes.Shape;
import software.amazon.smithy.model.shapes.ShapeType;
import software.amazon.smithy.model.traits.EnumTrait;
import software.amazon.smithy.model.traits.UniqueItemsTrait;
import software.amazon.smithy.model.validation.ValidationEvent;

Expand All @@ -33,12 +34,19 @@ public List<ValidationEvent> evaluate(Differences differences) {
return differences.changedShapes()
.filter(diff -> diff.getOldShape().getType() != diff.getNewShape().getType())
.filter(diff -> !expectedSetToListChange(diff))
.filter(diff -> !expectedStringToEnumChange(diff))
.map(diff -> error(diff.getNewShape(), String.format(
"Shape `%s` type was changed from `%s` to `%s`.",
diff.getShapeId(), diff.getOldShape().getType(), diff.getNewShape().getType())))
.collect(Collectors.toList());
}

static boolean expectedStringToEnumChange(ChangedShape<Shape> diff) {
// Smithy diff doesn't raise an issue if a string with an enum trait is changed
// to an enum shape. The enum trait is deprecated and this is a recommended change.
return diff.getOldShape().hasTrait(EnumTrait.class) && diff.getNewShape().getType() == ShapeType.ENUM;
}

private boolean expectedSetToListChange(ChangedShape<Shape> diff) {
ShapeType oldType = diff.getOldShape().getType();
ShapeType newType = diff.getNewShape().getType();
Expand Down
99 changes: 99 additions & 0 deletions smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/AddedShapeTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,13 @@
import org.junit.jupiter.api.Test;
import software.amazon.smithy.diff.ModelDiff;
import software.amazon.smithy.model.Model;
import software.amazon.smithy.model.shapes.EnumShape;
import software.amazon.smithy.model.shapes.ListShape;
import software.amazon.smithy.model.shapes.MemberShape;
import software.amazon.smithy.model.shapes.Shape;
import software.amazon.smithy.model.shapes.StringShape;
import software.amazon.smithy.model.traits.EnumDefinition;
import software.amazon.smithy.model.traits.EnumTrait;
import software.amazon.smithy.model.validation.ValidationEvent;

public class AddedShapeTest {
Expand All @@ -52,4 +55,100 @@ public void doesNotEmitForMembersOfAddedContainerShapes() {
assertThat(TestHelper.findEvents(events, "AddedShape").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, list.getId()).size(), equalTo(1));
}

@Test
public void doesNotEmitForMembersOfConvertedEnumShape() {
Shape stringWithEnumTrait = StringShape.builder()
.id("foo.baz#Baz")
.addTrait(EnumTrait.builder()
.addEnum(EnumDefinition.builder()
.name("FOO")
.value("FOO")
.build())
.build())
.build();
Shape enumShape = EnumShape.builder()
.id("foo.baz#Baz")
.addMember("FOO", "FOO")
.build();
Model modelA = Model.assembler().addShapes(stringWithEnumTrait).assemble().unwrap();
Model modelB = Model.assembler().addShapes(enumShape).assemble().unwrap();
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);

assertThat(TestHelper.findEvents(events, "AddedShape").size(), equalTo(0));
}

@Test
public void doesNotEmitForEnumShapeToEnumTrait() {
Shape enumShape = EnumShape.builder()
.id("foo.baz#Baz")
.addMember("FOO", "FOO")
.build();
Shape stringWithEnumTrait = StringShape.builder()
.id("foo.baz#Baz")
.addTrait(EnumTrait.builder()
.addEnum(EnumDefinition.builder()
.name("FOO")
.value("FOO")
.build())
.build())
.build();
Model modelA = Model.assembler().addShapes(enumShape).assemble().unwrap();
Model modelB = Model.assembler().addShapes(stringWithEnumTrait).assemble().unwrap();
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);

assertThat(TestHelper.findEvents(events, "AddedShape").size(), equalTo(0));
}

@Test
public void doesNotEmitForEnumTraitToEnumTraitAddedEnum() {
Shape stringWithEnumTraitA = StringShape.builder()
.id("foo.baz#Baz")
.addTrait(EnumTrait.builder()
.addEnum(EnumDefinition.builder()
.name("FOO")
.value("FOO")
.build())
.build())
.build();
Shape stringWithEnumTraitB = StringShape.builder()
.id("foo.baz#Baz")
.addTrait(EnumTrait.builder()
.addEnum(EnumDefinition.builder()
.name("FOO")
.value("FOO")
.build())
.addEnum(EnumDefinition.builder()
.name("BAR")
.value("BAR")
.build())
.build())
.build();
Model modelA = Model.assembler().addShapes(stringWithEnumTraitA).assemble().unwrap();
Model modelB = Model.assembler().addShapes(stringWithEnumTraitB).assemble().unwrap();
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);

assertThat(TestHelper.findEvents(events, "AddedShape").size(), equalTo(0));
}

@Test
public void doesEmitForEnumShapeToEnumShapeAddedMember() {
Shape enumShapeA = EnumShape.builder()
.id("foo.baz#Baz")
.addMember("FOO", "FOO")
.build();
Shape enumShapeB = EnumShape.builder()
.id("foo.baz#Baz")
.addMember("FOO", "FOO")
.addMember("BAR", "BAR")
.build();
Model modelA = Model.assembler().addShapes(enumShapeA).assemble().unwrap();
Model modelB = Model.assembler().addShapes(enumShapeB).assemble().unwrap();
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);

assertThat(TestHelper.findEvents(events, "AddedShape").size(), equalTo(1));
assertThat(enumShapeB.getMember("BAR").isPresent(), equalTo(true));
assertThat(TestHelper.findEvents(events, enumShapeB.getMember("BAR").get().toShapeId()).size(),
equalTo(1));
}
}
Loading

0 comments on commit bc7bd4e

Please sign in to comment.