Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added enum values to ids for ChangedEnumTrait events #1807

Merged
merged 2 commits into from
Jun 8, 2023
Merged

Added enum values to ids for ChangedEnumTrait events #1807

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
82facba
Added enum values to ids for ChangedEnumTrait events
rchache Jun 3, 2023
95be9b4
Uses enum index instead of enum value, fixed bug when multiple enums …
rchache Jun 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedEnumTrait.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,9 @@
* list of existing values.
*/
public final class ChangedEnumTrait extends AbstractDiffEvaluator {
private static final String ORDER_CHANGED = ".OrderChanged";
private static final String NAME_CHANGED = ".NameChanged";
private static final String REMOVED = ".Removed";
private static final String ORDER_CHANGED = ".OrderChanged.";
private static final String NAME_CHANGED = ".NameChanged.";
private static final String REMOVED = ".Removed.";

@Override
public List<ValidationEvent> evaluate(Differences differences) {
Expand Down Expand Up @@ -73,7 +73,8 @@ private List<ValidationEvent> validateEnum(ChangedShape<Shape> change, Pair<Enum
List<ValidationEvent> events = new ArrayList<>();
int oldEndPosition = oldTrait.getValues().size() - 1;

for (EnumDefinition definition : oldTrait.getValues()) {
for (int enumIndex = 0; enumIndex < oldTrait.getValues().size(); enumIndex++) {
EnumDefinition definition = oldTrait.getValues().get(enumIndex);
Optional<EnumDefinition> maybeNewValue = newTrait.getValues().stream()
.filter(d -> d.getValue().equals(definition.getValue()))
.findFirst();
Expand All @@ -84,7 +85,7 @@ private List<ValidationEvent> validateEnum(ChangedShape<Shape> change, Pair<Enum
.severity(Severity.ERROR)
.message(String.format("Enum value `%s` was removed", definition.getValue()))
.shape(change.getNewShape())
.id(getEventId() + REMOVED)
.id(getEventId() + REMOVED + enumIndex)
.build()
);
oldEndPosition--;
Expand All @@ -100,7 +101,7 @@ private List<ValidationEvent> validateEnum(ChangedShape<Shape> change, Pair<Enum
newValue.getName().orElse(null),
definition.getValue()))
.shape(change.getNewShape())
.id(getEventId() + NAME_CHANGED)
.id(getEventId() + NAME_CHANGED + enumIndex)
.build()
);
}
Expand All @@ -119,9 +120,10 @@ private List<ValidationEvent> validateEnum(ChangedShape<Shape> change, Pair<Enum
+ "can cause compatibility issues when ordinal values are used for "
+ "iteration, serialization, etc.", definition.getValue()))
.shape(change.getNewShape())
.id(getEventId() + ORDER_CHANGED)
.id(getEventId() + ORDER_CHANGED + newPosition)
.build()
);
oldEndPosition++;
} else {
events.add(note(change.getNewShape(), String.format(
"Enum value `%s` was appended", definition.getValue())));
Expand Down
86 changes: 60 additions & 26 deletions smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedEnumTraitTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,8 @@ public void detectsAppendedEnumsEnumTraitNoNameToEnumShape() {
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);

assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").size(), equalTo(2));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged").get(0).getSeverity(),
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.0").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.0").get(0).getSeverity(),
equalTo(Severity.ERROR));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").get(1).getSeverity(),
equalTo(Severity.NOTE));
Expand Down Expand Up @@ -116,6 +116,7 @@ public void detectsRemovedEnums() {
.addTrait(EnumTrait.builder()
.addEnum(EnumDefinition.builder().value("foo").build())
.addEnum(EnumDefinition.builder().value("baz").build())
.addEnum(EnumDefinition.builder().value("bat").build())
.build())
.build();
StringShape s2 = StringShape.builder()
Expand All @@ -128,8 +129,9 @@ public void detectsRemovedEnums() {
Model modelB = Model.assembler().addShape(s2).assemble().unwrap();
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);

assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, Severity.ERROR).size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed.1").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed.2").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, Severity.ERROR).size(), equalTo(2));
}

@Test
Expand All @@ -153,8 +155,8 @@ public void detectsRemovedEnumsEnumTraitNoNameToEnumShape() {
Model modelB = Model.assembler().addShape(s2).assemble().unwrap();
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);

assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed.1").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.0").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").stream()
.allMatch(e -> e.getSeverity() == Severity.ERROR), equalTo(true));
}
Expand Down Expand Up @@ -182,8 +184,8 @@ public void detectsRemovedEnumsEnumTraitWithNameToEnumShape() {
Model modelB = Model.assembler().addShape(s2).assemble().unwrap();
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);

assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed").stream()
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed.1").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed.1").stream()
.allMatch(e -> e.getSeverity() == Severity.ERROR), equalTo(true));
}

Expand All @@ -192,21 +194,24 @@ public void detectsRenamedEnums() {
StringShape s1 = StringShape.builder()
.id("foo.baz#Baz")
.addTrait(EnumTrait.builder()
.addEnum(EnumDefinition.builder().value("foo").name("OLD").build())
.addEnum(EnumDefinition.builder().value("foo").name("OLD1").build())
.addEnum(EnumDefinition.builder().value("baz").name("OLD2").build())
.build())
.build();
StringShape s2 = StringShape.builder()
.id("foo.baz#Baz")
.addTrait(EnumTrait.builder()
.addEnum(EnumDefinition.builder().value("foo").name("NEW").build())
.addEnum(EnumDefinition.builder().value("foo").name("NEW1").build())
.addEnum(EnumDefinition.builder().value("baz").name("NEW2").build())
.build())
.build();
Model modelA = Model.assembler().addShape(s1).assemble().unwrap();
Model modelB = Model.assembler().addShape(s2).assemble().unwrap();
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);

assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, Severity.ERROR).size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.0").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.1").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, Severity.ERROR).size(), equalTo(2));
}

@Test
Expand All @@ -227,8 +232,8 @@ public void detectsRenamedEnumsEnumTraitNoNameToEnumShape() {
Model modelB = Model.assembler().addShape(s2).assemble().unwrap();
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);

assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged").get(0).getSeverity(),
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.0").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.0").get(0).getSeverity(),
equalTo(Severity.ERROR));
}

Expand All @@ -251,8 +256,8 @@ public void detectsRenamedEnumsEnumTraitWithNameToEnumShape() {
Model modelB = Model.assembler().addShape(s2).assemble().unwrap();
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);

assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged").get(0).getSeverity(),
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.0").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.0").get(0).getSeverity(),
equalTo(Severity.ERROR));
}

Expand All @@ -268,15 +273,43 @@ public void detectsInsertedEnums() {
.id("foo.baz#Baz")
.addTrait(EnumTrait.builder()
.addEnum(EnumDefinition.builder().value("baz").build())
.addEnum(EnumDefinition.builder().value("bat").build())
.addEnum(EnumDefinition.builder().value("foo").build())
.build())
.build();
Model modelA = Model.assembler().addShape(s1).assemble().unwrap();
Model modelB = Model.assembler().addShape(s2).assemble().unwrap();
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);

assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.OrderChanged").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.OrderChanged").get(0).getSeverity(), equalTo(Severity.ERROR));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.OrderChanged.0").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.OrderChanged.1").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, Severity.ERROR).size(), equalTo(2));
}

@Test
public void detectsInsertedEnumsBeforeAppendedEnums() {
StringShape s1 = StringShape.builder()
.id("foo.baz#Baz")
.addTrait(EnumTrait.builder()
.addEnum(EnumDefinition.builder().value("foo").build())
.build())
.build();
StringShape s2 = StringShape.builder()
.id("foo.baz#Baz")
.addTrait(EnumTrait.builder()
.addEnum(EnumDefinition.builder().value("baz").build())
.addEnum(EnumDefinition.builder().value("bat").build())
.addEnum(EnumDefinition.builder().value("foo").build())
.addEnum(EnumDefinition.builder().value("bar").build())
.build())
.build();
Model modelA = Model.assembler().addShape(s1).assemble().unwrap();
Model modelB = Model.assembler().addShape(s2).assemble().unwrap();
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);

assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.OrderChanged.0").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.OrderChanged.1").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, Severity.ERROR).size(), equalTo(2));
}

@Test
Expand All @@ -299,7 +332,7 @@ public void detectsInsertedEnumsEnumTraitNoNameToEnumShape() {
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);

assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").size(), equalTo(2));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.OrderChanged").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.OrderChanged.0").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").stream()
.allMatch(e -> e.getSeverity() == Severity.ERROR), equalTo(true));
}
Expand All @@ -324,8 +357,8 @@ public void detectsInsertedEnumsEnumTraitWithNameToEnumShape() {
Model modelB = Model.assembler().addShape(s2).assemble().unwrap();
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);

assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.OrderChanged").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.OrderChanged").stream()
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.OrderChanged.0").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.OrderChanged.0").stream()
.allMatch(e -> e.getSeverity() == Severity.ERROR), equalTo(true));
}

Expand Down Expand Up @@ -353,7 +386,7 @@ public void detectsAppendedEnumsAfterRemovedEnums() {

List<ValidationEvent> changeEvents = TestHelper.findEvents(allEvents, "ChangedEnumTrait");
assertThat(changeEvents.size(), equalTo(2));
assertThat(TestHelper.findEvents(changeEvents, "ChangedEnumTrait.Removed").size(), equalTo(1));
assertThat(TestHelper.findEvents(changeEvents, "ChangedEnumTrait.Removed.1").size(), equalTo(1));

ValidationEvent removedEvent = changeEvents.get(0);
assertThat(removedEvent.getSeverity(), equalTo(Severity.ERROR));
Expand Down Expand Up @@ -391,8 +424,9 @@ public void detectsAppendedEnumsAfterRemovedEnumsEnumTraitNoNameToEnumShape() {
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);

assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").size(), equalTo(4));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged").size(), equalTo(2));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.0").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed.1").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.2").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").subList(0, 3).stream()
.allMatch(e -> e.getSeverity() == Severity.ERROR), equalTo(true));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").subList(3, 4).stream()
Expand Down Expand Up @@ -429,8 +463,8 @@ public void detectsAppendedEnumsAfterRemovedEnumsEnumTraitWithNameToEnumShape()
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);

assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").size(), equalTo(2));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed").stream()
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed.1").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed.1").stream()
.allMatch(e -> e.getSeverity() == Severity.ERROR), equalTo(true));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").subList(1, 2).stream()
.allMatch(e -> e.getSeverity() == Severity.NOTE), equalTo(true));
Expand Down