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 1 commit
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
12 changes: 6 additions & 6 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 @@ -84,7 +84,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 + definition.getValue())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enum values support arbitrary content, so this seems problematic to include them in the ID. Ideally these events would use the name of an enum definition if available, and if not... I dunno. Maybe the index of the entry? Or a name derived from the value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with always using the index if you think that's acceptable

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah sounds good. The caveat I can think of is that what is, say index 2, today won't be index 2 in the future if index 2 is removed. Not sure if that matters for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't be an issue since we only care about these events in the scope of a single Trebuchet feature and not after that

.build()
);
oldEndPosition--;
Expand All @@ -100,7 +100,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 + definition.getValue())
.build()
);
}
Expand All @@ -119,7 +119,7 @@ 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 + definition.getValue())
.build()
);
} else {
Expand Down
41 changes: 21 additions & 20 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 @@ -128,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.Removed").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed.baz").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, Severity.ERROR).size(), equalTo(1));
}

Expand All @@ -153,8 +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.Removed.baz").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.foo").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").stream()
.allMatch(e -> e.getSeverity() == Severity.ERROR), equalTo(true));
}
Expand Down Expand Up @@ -182,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.Removed").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed").stream()
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed.baz").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed.baz").stream()
.allMatch(e -> e.getSeverity() == Severity.ERROR), equalTo(true));
}

Expand All @@ -205,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.NameChanged").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.foo").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, Severity.ERROR).size(), equalTo(1));
}

Expand All @@ -227,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.NameChanged").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged").get(0).getSeverity(),
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.foo").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.foo").get(0).getSeverity(),
equalTo(Severity.ERROR));
}

Expand All @@ -251,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.NameChanged").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged").get(0).getSeverity(),
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.foo").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.foo").get(0).getSeverity(),
equalTo(Severity.ERROR));
}

Expand All @@ -275,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.OrderChanged").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.OrderChanged").get(0).getSeverity(), equalTo(Severity.ERROR));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.OrderChanged.baz").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.OrderChanged.baz").get(0).getSeverity(), equalTo(Severity.ERROR));
}

@Test
Expand All @@ -299,7 +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.OrderChanged.baz").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").stream()
.allMatch(e -> e.getSeverity() == Severity.ERROR), equalTo(true));
}
Expand All @@ -324,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.OrderChanged").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.OrderChanged").stream()
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.OrderChanged.baz").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.OrderChanged.baz").stream()
.allMatch(e -> e.getSeverity() == Severity.ERROR), equalTo(true));
}

Expand Down Expand Up @@ -353,7 +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));
assertThat(TestHelper.findEvents(changeEvents, "ChangedEnumTrait.Removed.old2").size(), equalTo(1));

ValidationEvent removedEvent = changeEvents.get(0);
assertThat(removedEvent.getSeverity(), equalTo(Severity.ERROR));
Expand Down Expand Up @@ -391,8 +391,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.old1").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed.old2").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.old3").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 +430,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.old2").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed.old2").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