Skip to content

Commit

Permalink
RemovedShape events from ERROR -> NOTE if the shape is safe to remove…
Browse files Browse the repository at this point in the history 
… from a model.
  • Loading branch information
@rchache
rchache committed Nov 9, 2023
1 parent 0ba148d commit 88238e5
Show file tree
Hide file tree
Showing 3 changed files with 79 additions and 5 deletions.
23 changes: 21 additions & 2 deletions smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/RemovedShape.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,19 +19,23 @@
import java.util.stream.Collectors;
import software.amazon.smithy.diff.Differences;
import software.amazon.smithy.model.shapes.Shape;
import software.amazon.smithy.model.traits.EnumTrait;
import software.amazon.smithy.model.traits.PrivateTrait;
import software.amazon.smithy.model.validation.ValidationEvent;

/**
* Creates an ERROR event when a non-private shape is removed.
* Creates an ERROR event when a non-private non-scalar shape is removed.
* Creates a NOTE event when a non-private scalar shape is removed.
*/
public final class RemovedShape extends AbstractDiffEvaluator {
@Override
public List<ValidationEvent> evaluate(Differences differences) {
return differences.removedShapes()
.filter(shape -> !shape.hasTrait(PrivateTrait.class))
.filter(shape -> !isMemberOfRemovedShape(shape, differences))
.map(shape -> error(shape, String.format("Removed %s `%s`", shape.getType(), shape.getId())))
.map(shape -> isScalarType(shape)
? note(shape, String.format("Removed %s `%s`", shape.getType(), shape.getId()))
: error(shape, String.format("Removed %s `%s`", shape.getType(), shape.getId())))
.collect(Collectors.toList());
}

Expand All @@ -40,4 +44,19 @@ private boolean isMemberOfRemovedShape(Shape shape, Differences differences) {
.filter(member -> !differences.getNewModel().getShapeIds().contains(member.getContainer()))
.isPresent();
}

private boolean isScalarType(Shape shape) {
return shape.isIntegerShape()
|| shape.isBigDecimalShape()
|| shape.isBigIntegerShape()
|| shape.isBlobShape()
|| shape.isBooleanShape()
|| shape.isByteShape()
|| shape.isDoubleShape()
|| shape.isFloatShape()
|| shape.isShortShape()
|| shape.isTimestampShape()
|| shape.isLongShape()
|| (shape.isStringShape() && !shape.hasTrait(EnumTrait.class));
}
}
3 changes: 2 additions & 1 deletion smithy-diff/src/test/java/software/amazon/smithy/diff/ModelDiffTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import software.amazon.smithy.model.Model;
import software.amazon.smithy.model.node.Node;
import software.amazon.smithy.model.shapes.StringShape;
import software.amazon.smithy.model.shapes.StructureShape;
import software.amazon.smithy.model.traits.SensitiveTrait;
import software.amazon.smithy.model.validation.Severity;
import software.amazon.smithy.model.validation.ValidatedResult;
Expand Down Expand Up @@ -77,7 +78,7 @@ public void testsEquality() {
@Test
public void findsBreakingChanges() {
Model oldModel = Model.builder()
.addShape(StringShape.builder().id("smithy.example#Str").build())
.addShape(StructureShape.builder().id("smithy.example#Str").build())
.build();
Model newModel = Model.builder().build();
ModelDiff.Result result = ModelDiff.builder().oldModel(oldModel).newModel(newModel).compare();
Expand Down
58 changes: 56 additions & 2 deletions smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/RemovedShapeTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,19 +23,33 @@
import org.junit.jupiter.api.Test;
import software.amazon.smithy.diff.ModelDiff;
import software.amazon.smithy.model.Model;
import software.amazon.smithy.model.shapes.BigDecimalShape;
import software.amazon.smithy.model.shapes.BigIntegerShape;
import software.amazon.smithy.model.shapes.BlobShape;
import software.amazon.smithy.model.shapes.BooleanShape;
import software.amazon.smithy.model.shapes.ByteShape;
import software.amazon.smithy.model.shapes.DoubleShape;
import software.amazon.smithy.model.shapes.FloatShape;
import software.amazon.smithy.model.shapes.IntegerShape;
import software.amazon.smithy.model.shapes.ListShape;
import software.amazon.smithy.model.shapes.LongShape;
import software.amazon.smithy.model.shapes.MemberShape;
import software.amazon.smithy.model.shapes.Shape;
import software.amazon.smithy.model.shapes.ShortShape;
import software.amazon.smithy.model.shapes.StringShape;
import software.amazon.smithy.model.shapes.StructureShape;
import software.amazon.smithy.model.shapes.TimestampShape;
import software.amazon.smithy.model.traits.EnumDefinition;
import software.amazon.smithy.model.traits.EnumTrait;
import software.amazon.smithy.model.traits.PrivateTrait;
import software.amazon.smithy.model.validation.Severity;
import software.amazon.smithy.model.validation.ValidationEvent;

public class RemovedShapeTest {
@Test
public void detectsShapeRemoval() {
Shape shapeA1 = StringShape.builder().id("foo.baz#Baz").build();
Shape shapeB1 = StringShape.builder().id("foo.baz#Bam").build();
Shape shapeA1 = StructureShape.builder().id("foo.baz#Baz").build();
Shape shapeB1 = StructureShape.builder().id("foo.baz#Bam").build();
Model modelA = Model.assembler().addShapes(shapeA1, shapeB1).assemble().unwrap();
Model modelB = Model.assembler().addShapes(shapeB1).assemble().unwrap();
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
Expand All @@ -45,6 +59,46 @@ public void detectsShapeRemoval() {
assertThat(TestHelper.findEvents(events, Severity.ERROR).size(), equalTo(1));
}

@Test
public void emitsNotesForScalarShapes() {
Shape[] scalarShapes = new Shape[] {
IntegerShape.builder().id("foo.baz#BazOne").build(),
BigDecimalShape.builder().id("foo.baz#BazTwo").build(),
BigIntegerShape.builder().id("foo.baz#BazThree").build(),
BlobShape.builder().id("foo.baz#BazFour").build(),
BooleanShape.builder().id("foo.baz#BazFive").build(),
ByteShape.builder().id("foo.baz#BazSix").build(),
DoubleShape.builder().id("foo.baz#BazSeven").build(),
FloatShape.builder().id("foo.baz#BazEight").build(),
ShortShape.builder().id("foo.baz#BazNine").build(),
TimestampShape.builder().id("foo.baz#BazTen").build(),
LongShape.builder().id("foo.baz#BazEleven").build(),
StringShape.builder().id("foo.baz#BazTwelve").build()
};
Model modelA = Model.assembler().addShapes(scalarShapes).assemble().unwrap();
Model modelB = Model.assembler().assemble().unwrap();
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);

assertThat(TestHelper.findEvents(events, "RemovedShape").size(), equalTo(12));
assertThat("Scalar removals should be NOTE severity",
events.stream().allMatch(event -> Severity.NOTE.equals(event.getSeverity())));
}

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

assertThat(TestHelper.findEvents(events, "RemovedShape").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, shapeA1.getId()).size(), equalTo(1));
assertThat(TestHelper.findEvents(events, Severity.ERROR).size(), equalTo(1));
}

@Test
public void doesNotEmitForPrivateShapes() {
Shape shape = StringShape.builder().id("foo.baz#Baz").addTrait(new PrivateTrait()).build();
Expand Down

0 comments on commit 88238e5

Please sign in to comment.