Skip to content

Commit

Permalink
Emit specific ids for ChangedEnumTrait ERROR events (smithy-lang#1787)
Browse files Browse the repository at this point in the history 
* Emit id 'ChangedEnumTrait.Inserted' for inserted enum diff events

* Changed .Inserted to .OrderChanged, and added subparts for .Removed and .NameChanged
  • Loading branch information
@rchache
rchache authored and Steven Yuan committed Aug 11, 2023
1 parent 7861233 commit 3c595cd
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 25 deletions.
47 changes: 36 additions & 11 deletions 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 @@ -27,6 +27,7 @@
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.Severity;
import software.amazon.smithy.model.validation.ValidationEvent;
import software.amazon.smithy.utils.OptionalUtils;
import software.amazon.smithy.utils.Pair;
Expand All @@ -38,6 +39,10 @@
* 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";

@Override
public List<ValidationEvent> evaluate(Differences differences) {
return differences.changedShapes()
Expand Down Expand Up @@ -74,17 +79,30 @@ private List<ValidationEvent> validateEnum(ChangedShape<Shape> change, Pair<Enum
.findFirst();

if (!maybeNewValue.isPresent()) {
events.add(error(change.getNewShape(), String.format(
"Enum value `%s` was removed", definition.getValue())));
events.add(
ValidationEvent.builder()
.severity(Severity.ERROR)
.message(String.format("Enum value `%s` was removed", definition.getValue()))
.shape(change.getNewShape())
.id(getEventId() + REMOVED)
.build()
);
oldEndPosition--;
} else {
EnumDefinition newValue = maybeNewValue.get();
if (!newValue.getName().equals(definition.getName())) {
events.add(error(change.getNewShape(), String.format(
"Enum `name` changed from `%s` to `%s` for the `%s` value",
definition.getName().orElse(null),
newValue.getName().orElse(null),
definition.getValue())));
events.add(
ValidationEvent.builder()
.severity(Severity.ERROR)
.message(String.format(
"Enum `name` changed from `%s` to `%s` for the `%s` value",
definition.getName().orElse(null),
newValue.getName().orElse(null),
definition.getValue()))
.shape(change.getNewShape())
.id(getEventId() + NAME_CHANGED)
.build()
);
}
}
}
Expand All @@ -93,10 +111,17 @@ private List<ValidationEvent> validateEnum(ChangedShape<Shape> change, Pair<Enum
for (EnumDefinition definition : newTrait.getValues()) {
if (!oldTrait.getEnumDefinitionValues().contains(definition.getValue())) {
if (newPosition <= oldEndPosition) {
events.add(error(change.getNewShape(), String.format(
"Enum value `%s` was inserted before the end of the list of existing values. This can "
+ "cause compatibility issues when ordinal values are used for iteration, "
+ "serialization, etc.", definition.getValue())));
events.add(
ValidationEvent.builder()
.severity(Severity.ERROR)
.message(String.format(
"Enum value `%s` was inserted before the end of the list of existing values. This "
+ "can cause compatibility issues when ordinal values are used for "
+ "iteration, serialization, etc.", definition.getValue()))
.shape(change.getNewShape())
.id(getEventId() + ORDER_CHANGED)
.build()
);
} else {
events.add(note(change.getNewShape(), String.format(
"Enum value `%s` was appended", definition.getValue())));
Expand Down
39 changes: 25 additions & 14 deletions smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedEnumTraitTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,13 @@
package software.amazon.smithy.diff.evaluators;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.allOf;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.stringContainsInOrder;

import java.util.List;

import org.hamcrest.core.Every;
import org.junit.jupiter.api.Test;
import software.amazon.smithy.diff.ModelDiff;
import software.amazon.smithy.model.Model;
Expand Down Expand Up @@ -74,7 +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").get(0).getSeverity(),
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged").get(0).getSeverity(),
equalTo(Severity.ERROR));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").get(1).getSeverity(),
equalTo(Severity.NOTE));
Expand Down Expand Up @@ -124,7 +128,7 @@ public void detectsRemovedEnums() {
Model modelB = Model.assembler().addShape(s2).assemble().unwrap();
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);

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

Expand All @@ -149,6 +153,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").stream()
.allMatch(e -> e.getSeverity() == Severity.ERROR), equalTo(true));
}
Expand Down Expand Up @@ -176,8 +182,8 @@ public void detectsRemovedEnumsEnumTraitWithNameToEnumShape() {
Model modelB = Model.assembler().addShape(s2).assemble().unwrap();
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);

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

Expand All @@ -199,7 +205,7 @@ public void detectsRenamedEnums() {
Model modelB = Model.assembler().addShape(s2).assemble().unwrap();
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);

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

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

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

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

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

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

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

@Test
Expand All @@ -293,6 +299,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").stream()
.allMatch(e -> e.getSeverity() == Severity.ERROR), equalTo(true));
}
Expand All @@ -317,8 +324,8 @@ public void detectsInsertedEnumsEnumTraitWithNameToEnumShape() {
Model modelB = Model.assembler().addShape(s2).assemble().unwrap();
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);

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

Expand Down Expand Up @@ -346,6 +353,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));

ValidationEvent removedEvent = changeEvents.get(0);
assertThat(removedEvent.getSeverity(), equalTo(Severity.ERROR));
Expand Down Expand Up @@ -383,6 +391,8 @@ 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").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 @@ -419,7 +429,8 @@ public void detectsAppendedEnumsAfterRemovedEnumsEnumTraitWithNameToEnumShape()
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);

assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").size(), equalTo(2));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").subList(0, 1).stream()
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed").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

0 comments on commit 3c595cd

Please sign in to comment.