Skip to content

Commit

Permalink
Set source locations on smithy-diff events
Browse files Browse the repository at this point in the history
  • Loading branch information
srchase committed Oct 5, 2023
1 parent 428c8a1 commit ab41c74
Show file tree
Hide file tree
Showing 21 changed files with 271 additions and 50 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
import java.util.stream.Collectors;
import software.amazon.smithy.diff.ChangedShape;
import software.amazon.smithy.diff.Differences;
import software.amazon.smithy.model.Model;
import software.amazon.smithy.model.SourceLocation;
import software.amazon.smithy.model.shapes.OperationShape;
import software.amazon.smithy.model.shapes.ShapeId;
import software.amazon.smithy.model.validation.Severity;
Expand All @@ -33,17 +35,18 @@ public final class AddedOperationError extends AbstractDiffEvaluator {
@Override
public List<ValidationEvent> evaluate(Differences differences) {
return differences.changedShapes(OperationShape.class)
.flatMap(change -> createErrorViolations(change).stream())
.flatMap(change -> createErrorViolations(change, differences.getNewModel()).stream())
.collect(Collectors.toList());
}

private List<ValidationEvent> createErrorViolations(ChangedShape<OperationShape> change) {
private List<ValidationEvent> createErrorViolations(ChangedShape<OperationShape> change, Model newModel) {
if (change.getOldShape().getErrors().equals(change.getNewShape().getErrors())) {
return Collections.emptyList();
}

List<ValidationEvent> events = new ArrayList<>();
for (ShapeId error : change.getNewShape().getErrors()) {
SourceLocation errorSource = newModel.expectShape(error).getSourceLocation();
if (!change.getOldShape().getErrors().contains(error)) {
events.add(
ValidationEvent.builder()
Expand All @@ -57,6 +60,7 @@ private List<ValidationEvent> createErrorViolations(ChangedShape<OperationShape>
+ "parameter to an operation).",
error, change.getShapeId()))
.shape(change.getNewShape())
.sourceLocation(errorSource)
.build()
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ private ValidationEvent emit(MemberShape memberShape) {
return ValidationEvent.builder()
.id(getEventId())
.shapeId(memberShape.getId())
.sourceLocation(memberShape.getSourceLocation())
.message("Adding a new member with the `required` trait "
+ "but not the `default` trait is backwards-incompatible.")
.severity(Severity.ERROR)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ public List<ValidationEvent> evaluate(Differences differences) {
.id(getEventId())
.severity(Severity.NOTE)
.shape(shape)
.sourceLocation(shape.getSourceLocation())
.message(String.format("Trait definition `%s` was added", shape.getId()))
.build())
.collect(Collectors.toList());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import java.util.stream.Collectors;
import software.amazon.smithy.diff.ChangedShape;
import software.amazon.smithy.diff.Differences;
import software.amazon.smithy.model.SourceLocation;
import software.amazon.smithy.model.shapes.Shape;
import software.amazon.smithy.model.traits.EnumDefinition;
import software.amazon.smithy.model.traits.EnumTrait;
Expand Down Expand Up @@ -85,6 +86,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())
.sourceLocation(oldTrait.getSourceLocation())
.id(getEventId() + REMOVED + enumIndex)
.build()
);
Expand All @@ -101,6 +103,7 @@ private List<ValidationEvent> validateEnum(ChangedShape<Shape> change, Pair<Enum
newValue.getName().orElse(null),
definition.getValue()))
.shape(change.getNewShape())
.sourceLocation(change.getNewShape().getSourceLocation())
.id(getEventId() + NAME_CHANGED + enumIndex)
.build()
);
Expand All @@ -120,12 +123,17 @@ 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())
.sourceLocation(oldTrait.getSourceLocation())
.id(getEventId() + ORDER_CHANGED + newPosition)
.build()
);
oldEndPosition++;
} else {
events.add(note(change.getNewShape(), String.format(
SourceLocation appendedSource = newTrait.getSourceLocation();
if (newTrait.isSynthetic()) {
appendedSource = change.getNewShape().getSourceLocation();
}
events.add(note(change.getNewShape(), appendedSource, String.format(
"Enum value `%s` was appended", definition.getValue())));
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ private ValidationEvent createChangeEvent(Differences differences, ChangedShape<
.severity(severity)
.id(getEventId())
.shape(change.getNewShape())
.sourceLocation(change.getNewShape().getSourceLocation())
.message(message)
.build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import software.amazon.smithy.diff.ChangedShape;
import software.amazon.smithy.diff.Differences;
import software.amazon.smithy.model.Model;
import software.amazon.smithy.model.SourceLocation;
import software.amazon.smithy.model.knowledge.NullableIndex;
import software.amazon.smithy.model.shapes.MemberShape;
import software.amazon.smithy.model.shapes.Shape;
Expand Down Expand Up @@ -96,33 +97,36 @@ private void createErrors(
boolean oldHasInput = hasInputTrait(differences.getOldModel(), oldMember);
boolean newHasInput = hasInputTrait(differences.getNewModel(), newMember);
ShapeId shape = change.getShapeId();
SourceLocation oldMemberSourceLocation = oldMember.getSourceLocation();
Shape newTarget = differences.getNewModel().expectShape(newMember.getTarget());
List<ValidationEvent> eventsToAdd = new ArrayList<>();
SourceLocation newMemberContainerSource = differences.getNewModel().expectShape(newMember.getContainer())
.getSourceLocation();

if (oldHasInput && !newHasInput) {
// If there was an input trait before, but not now, then the nullability must have
// changed from nullable to non-nullable.
eventsToAdd.add(emit(Severity.ERROR, "RemovedInputTrait", shape, message,
eventsToAdd.add(emit(Severity.ERROR, "RemovedInputTrait", shape, newMemberContainerSource, message,
"The @input trait was removed from " + newMember.getContainer()));
} else if (!oldHasInput && newHasInput) {
// If there was no input trait before, but there is now, then the nullability must have
// changed from non-nullable to nullable.
eventsToAdd.add(emit(Severity.DANGER, "AddedInputTrait", shape, message,
eventsToAdd.add(emit(Severity.DANGER, "AddedInputTrait", shape, newMemberContainerSource, message,
"The @input trait was added to " + newMember.getContainer()));
} else if (!newHasInput) {
// Can't add clientOptional to a preexisting required member.
if (change.isTraitAdded(ClientOptionalTrait.ID) && change.isTraitInBoth(RequiredTrait.ID)) {
eventsToAdd.add(emit(Severity.ERROR, "AddedClientOptionalTrait", shape, message,
"The @clientOptional trait was added to a @required member."));
eventsToAdd.add(emit(Severity.ERROR, "AddedClientOptionalTrait", shape, oldMemberSourceLocation,
message, "The @clientOptional trait was added to a @required member."));
}
// Can't add required to a member unless the member is marked as @clientOptional or part of @input.
if (change.isTraitAdded(RequiredTrait.ID) && !newMember.hasTrait(ClientOptionalTrait.ID)) {
eventsToAdd.add(emit(Severity.ERROR, "AddedRequiredTrait", shape, message,
eventsToAdd.add(emit(Severity.ERROR, "AddedRequiredTrait", shape, oldMemberSourceLocation, message,
"The @required trait was added to a member."));
}
// Can't add the default trait to a member unless the member was previously required.
if (change.isTraitAdded(DefaultTrait.ID) && !change.isTraitRemoved(RequiredTrait.ID)) {
eventsToAdd.add(emit(Severity.ERROR, "AddedDefaultTrait", shape, message,
eventsToAdd.add(emit(Severity.ERROR, "AddedDefaultTrait", shape, oldMemberSourceLocation, message,
"The @default trait was added to a member that was not previously @required."));
}
// Can only remove the required trait if the member was nullable or replaced by the default trait.
Expand All @@ -131,20 +135,21 @@ private void createErrors(
&& !oldMember.hasTrait(ClientOptionalTrait.ID)) {
if (newTarget.isStructureShape() || newTarget.isUnionShape()) {
eventsToAdd.add(emit(Severity.WARNING, "RemovedRequiredTrait.StructureOrUnion", shape,
message, "The @required trait was removed from a member that targets a "
+ newTarget.getType() + ". This is backward compatible in generators that "
+ "always treat structures and unions as optional (e.g., AWS generators)"));
oldMemberSourceLocation, message, "The @required trait was removed from a member "
+ "that targets a " + newTarget.getType() + ". This is backward compatible in "
+ "generators that always treat structures and unions as optional "
+ "(e.g., AWS generators)"));
} else {
eventsToAdd.add(emit(Severity.ERROR, "RemovedRequiredTrait", shape, message,
"The @required trait was removed and not replaced with the @default trait and "
+ "@addedDefault trait."));
eventsToAdd.add(emit(Severity.ERROR, "RemovedRequiredTrait", shape, oldMemberSourceLocation,
message, "The @required trait was removed and not replaced with the @default "
+ "trait and @addedDefault trait."));
}
}
}

// If not specific event was emitted, emit a generic event.
if (eventsToAdd.isEmpty()) {
eventsToAdd.add(emit(Severity.ERROR, null, shape, null, message));
eventsToAdd.add(emit(Severity.ERROR, null, shape, oldMemberSourceLocation, null, message));
}

events.addAll(eventsToAdd);
Expand All @@ -158,6 +163,7 @@ private ValidationEvent emit(
Severity severity,
String eventIdSuffix,
ShapeId shape,
SourceLocation sourceLocation,
String prefixMessage,
String message
) {
Expand All @@ -166,6 +172,7 @@ private ValidationEvent emit(
return ValidationEvent.builder()
.id(actualId)
.shapeId(shape)
.sourceLocation(sourceLocation)
.message(actualMessage)
.severity(severity)
.build();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,7 @@ List<ValidationEvent> validate(
.id(getValidationEventId(this, trait))
.severity(severity)
.shape(shape)
.sourceLocation(left.getSourceLocation())
.message(message)
.build());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ private ValidationEvent createRemovedEvent(String typeOfRemoval, EntityShape par
.id(typeOfRemoval + typeOfParentShape + childShape.getName())
.severity(Severity.ERROR)
.shape(parentEntity)
.sourceLocation(parentEntity.getSourceLocation())
.message(message)
.build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ public List<ValidationEvent> evaluate(Differences differences) {
.id(getEventId())
.severity(Severity.ERROR)
.shape(shape)
.sourceLocation(shape.expectTrait(TraitDefinition.class).getSourceLocation())
.message(String.format("Trait definition `%s` was removed", shape.getId()))
.build())
.collect(Collectors.toList());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.junit.jupiter.api.Test;
import software.amazon.smithy.diff.ModelDiff;
import software.amazon.smithy.model.Model;
import software.amazon.smithy.model.SourceLocation;
import software.amazon.smithy.model.shapes.OperationShape;
import software.amazon.smithy.model.shapes.Shape;
import software.amazon.smithy.model.shapes.StructureShape;
Expand All @@ -31,13 +32,17 @@
public class AddedOperationErrorTest {
@Test
public void detectsAddedErrors() {
SourceLocation s1 = new SourceLocation("main.smithy", 1, 2);
SourceLocation s2 = new SourceLocation("main.smithy", 3, 4);
Shape e1 = StructureShape.builder()
.id("foo.baz#E1")
.addTrait(new ErrorTrait("client"))
.source(s1)
.build();
Shape e2 = StructureShape.builder()
.id("foo.baz#E2")
.addTrait(new ErrorTrait("client"))
.source(s2)
.build();
OperationShape operation1 = OperationShape.builder().id("foo.baz#Operation").build();
Shape operation2 = operation1.toBuilder().addError(e1.getId()).addError(e2.getId()).build();
Expand All @@ -47,6 +52,8 @@ public void detectsAddedErrors() {

assertThat(TestHelper.findEvents(events, "AddedOperationError").size(), equalTo(2));
assertThat(TestHelper.findEvents(events, "AddedOperationError.E1").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "AddedOperationError.E1").stream().findFirst().get().getSourceLocation(), equalTo(s1));
assertThat(TestHelper.findEvents(events, "AddedOperationError.E2").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "AddedOperationError.E2").stream().findFirst().get().getSourceLocation(), equalTo(s2));
}
}
Original file line number Diff line number Diff line change
@@ -1,15 +1,19 @@
package software.amazon.smithy.diff.evaluators;

import java.util.List;
import org.junit.jupiter.api.Test;
import software.amazon.smithy.diff.ModelDiff;
import software.amazon.smithy.model.Model;
import software.amazon.smithy.model.SourceLocation;
import software.amazon.smithy.model.node.StringNode;
import software.amazon.smithy.model.shapes.MemberShape;
import software.amazon.smithy.model.shapes.ShapeId;
import software.amazon.smithy.model.shapes.StringShape;
import software.amazon.smithy.model.shapes.StructureShape;
import software.amazon.smithy.model.traits.DefaultTrait;
import software.amazon.smithy.model.traits.RequiredTrait;
import software.amazon.smithy.model.validation.Severity;
import software.amazon.smithy.model.validation.ValidationEvent;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;
Expand All @@ -20,8 +24,15 @@ public void addingRequiredTraitWithoutDefaultIsAnError() {
StringShape s = StringShape.builder().id("smithy.example#Str").build();
StructureShape a = StructureShape.builder().id("smithy.example#A")
.build();
SourceLocation source = new SourceLocation("main.smithy", 1, 2);
MemberShape member = MemberShape.builder()
.id(a.getId().withMember("foo"))
.target(s.getId())
.addTrait(new RequiredTrait())
.source(source)
.build();
StructureShape b = StructureShape.builder().id("smithy.example#A")
.addMember("foo", s.getId(), b2 -> b2.addTrait(new RequiredTrait()))
.addMember(member)
.build();
Model model1 = Model.builder().addShapes(s, a).build();
Model model2 = Model.builder().addShapes(s, b).build();
Expand All @@ -34,6 +45,8 @@ public void addingRequiredTraitWithoutDefaultIsAnError() {
assertThat(TestHelper.findEvents(result.getDiffEvents(), "AddedRequiredMember").get(0).getMessage(),
equalTo("Adding a new member with the `required` trait " +
"but not the `default` trait is backwards-incompatible."));
assertThat(TestHelper.findEvents(result.getDiffEvents(), "AddedRequiredMember").get(0).getSourceLocation(),
equalTo(source));
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.junit.jupiter.api.Test;
import software.amazon.smithy.diff.ModelDiff;
import software.amazon.smithy.model.Model;
import software.amazon.smithy.model.SourceLocation;
import software.amazon.smithy.model.shapes.Shape;
import software.amazon.smithy.model.shapes.StringShape;
import software.amazon.smithy.model.traits.TraitDefinition;
Expand All @@ -30,15 +31,19 @@
public class AddedTraitDefinitionTest {
@Test
public void detectsAddedTraitDefinition() {
SourceLocation source = new SourceLocation("main.smithy", 1, 2);
Shape definition = StringShape.builder()
.id("foo.baz#bam")
.addTrait(TraitDefinition.builder().build())
.source(source)
.build();

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

assertThat(TestHelper.findEvents(events, "AddedTraitDefinition").size(), equalTo(1));
assertThat(TestHelper.findEvents(events, "AddedTraitDefinition").stream().findFirst().get()
.getSourceLocation(), equalTo(source));
}
}
Loading

0 comments on commit ab41c74

Please sign in to comment.