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

Warn on unknown trait diffs #1468

Merged
merged 1 commit into from
Oct 25, 2022
Merged

Warn on unknown trait diffs #1468

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 10 additions & 3 deletions smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ModifiedTrait.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@
import software.amazon.smithy.model.traits.RequiredTrait;
import software.amazon.smithy.model.traits.Trait;
import software.amazon.smithy.model.traits.TraitDefinition;
import software.amazon.smithy.model.traits.synthetic.OriginalShapeIdTrait;
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.ListUtils;
Expand Down Expand Up @@ -95,7 +97,8 @@ public final class ModifiedTrait extends AbstractDiffEvaluator {
new DiffStrategy(DiffType.REMOVE, Severity.WARNING));

/** Traits in this list have special backward compatibility rules and can't be validated here. */
private static final Set<ShapeId> IGNORED_TRAITS = SetUtils.of(BoxTrait.ID, RequiredTrait.ID);
private static final Set<ShapeId> IGNORED_TRAITS = SetUtils.of(BoxTrait.ID, RequiredTrait.ID,
SyntheticEnumTrait.ID, OriginalShapeIdTrait.ID);

@Override
public List<ValidationEvent> evaluate(Differences differences) {
Expand All @@ -108,8 +111,12 @@ public List<ValidationEvent> evaluate(Differences differences) {
Trait oldTrait = oldTraitNewTraitPair.left;
Trait newTrait = oldTraitNewTraitPair.right;
// Do not emit for the box trait because it is added and removed for backward compatibility.
if (!IGNORED_TRAITS.contains(traitId) && strategies.containsKey(traitId)) {
for (DiffStrategy strategy : strategies.get(traitId)) {
if (!IGNORED_TRAITS.contains(traitId)) {
// If we don't know about the trait, warn on any change to it.
List<DiffStrategy> diffStrategies = strategies.computeIfAbsent(traitId,
t -> ListUtils.of(new DiffStrategy(DiffType.CONST, Severity.WARNING)));

for (DiffStrategy strategy : diffStrategies) {
List<ValidationEvent> diffEvents = strategy.diffType.validate(
differences.getNewModel(),
"",
Expand Down
27 changes: 27 additions & 0 deletions smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ModifiedTraitTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,22 @@ public void testWithTag(String oldValue, String newValue, String tag, String sea
assertThat(events.get(0).getMessage(), containsString(searchString));
}

@ParameterizedTest
@MethodSource("data")
public void testWithoutDefinition(String oldValue, String newValue, String tag, String searchString) {
TestCaseData data = new TestCaseData(oldValue, newValue);

Model modelA = Model.assembler().addShape(data.oldShape).assemble().unwrap();
Model modelB = Model.assembler().addShape(data.newShape).assemble().unwrap();
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);

assertThat(TestHelper.findEvents(events, "ModifiedTrait").size(), equalTo(1));
assertThat(events.get(0).getMessage(), containsString(searchString));
for (ValidationEvent event : events) {
assertThat(event.getSeverity(), equalTo(Severity.WARNING));
}
}

public static Collection<String[]> data() {
return Arrays.asList(new String[][] {
{null, "hi", "diff.error.add", "Added"},
Expand All @@ -108,6 +124,17 @@ private static Shape createDefinition(String tag) {
.build();
}

@Test
public void noEventsForUnmodifiedTraitWithoutDefinition() {
TestCaseData data = new TestCaseData("hi", "hi");

Model modelA = Model.assembler().addShape(data.oldShape).assemble().unwrap();
Model modelB = Model.assembler().addShape(data.newShape).assemble().unwrap();
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);

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

@Test
public void modifiedShapeNoTag() {
Model modelA = Model.assembler()
Expand Down