From 9d68b5c15306dc38903466f7d3fd88f076301457 Mon Sep 17 00:00:00 2001 From: Richard Chen Date: Fri, 2 Jun 2023 15:22:44 -0400 Subject: [PATCH 1/2] Extend event ids for Added/RemovedOperationError events --- .../diff/evaluators/AddedOperationError.java | 27 ++++++++++++------- .../evaluators/RemovedOperationError.java | 19 +++++++++---- .../evaluators/AddedOperationErrorTest.java | 2 ++ .../evaluators/RemovedOperationErrorTest.java | 2 ++ 4 files changed, 36 insertions(+), 14 deletions(-) diff --git a/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/AddedOperationError.java b/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/AddedOperationError.java index bbf612801ef..415fb6b43e1 100644 --- a/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/AddedOperationError.java +++ b/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/AddedOperationError.java @@ -23,6 +23,7 @@ import software.amazon.smithy.diff.Differences; import software.amazon.smithy.model.shapes.OperationShape; import software.amazon.smithy.model.shapes.ShapeId; +import software.amazon.smithy.model.validation.Severity; import software.amazon.smithy.model.validation.ValidationEvent; /** @@ -42,15 +43,23 @@ private List createErrorViolations(ChangedShape } List events = new ArrayList<>(); - for (ShapeId id : change.getNewShape().getErrors()) { - if (!change.getOldShape().getErrors().contains(id)) { - events.add(warning(change.getNewShape(), String.format( - "The `%s` error was added to the `%s` operation. This " - + "is backward-compatible if the error is only " - + "encountered as a result of a change in behavior of " - + "the client (for example, the client sends a new " - + "parameter to an operation).", - id, change.getShapeId()))); + for (ShapeId error : change.getNewShape().getErrors()) { + if (!change.getOldShape().getErrors().contains(error)) { + events.add( + ValidationEvent.builder() + .id(getEventId() + "." + error) + .severity(Severity.WARNING) + .message(String.format( + "The `%s` error was added to the `%s` operation. This " + + "is backward-compatible if the error is only " + + "encountered as a result of a change in behavior of " + + "the client (for example, the client sends a new " + + "parameter to an operation).", + error, change.getShapeId())) + .shape(change.getNewShape()) + .sourceLocation(change.getNewShape().getSourceLocation()) + .build() + ); } } diff --git a/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/RemovedOperationError.java b/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/RemovedOperationError.java index b66c05c2d03..0167c6047de 100644 --- a/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/RemovedOperationError.java +++ b/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/RemovedOperationError.java @@ -23,6 +23,7 @@ import software.amazon.smithy.diff.Differences; import software.amazon.smithy.model.shapes.OperationShape; import software.amazon.smithy.model.shapes.ShapeId; +import software.amazon.smithy.model.validation.Severity; import software.amazon.smithy.model.validation.ValidationEvent; /** @@ -42,11 +43,19 @@ private List createErrorViolations(ChangedShape } List events = new ArrayList<>(); - for (ShapeId id : change.getOldShape().getErrors()) { - if (!change.getNewShape().getErrors().contains(id)) { - events.add(warning(change.getNewShape(), String.format( - "The `%s` error was removed from the `%s` operation.", - id, change.getShapeId()))); + for (ShapeId error : change.getOldShape().getErrors()) { + if (!change.getNewShape().getErrors().contains(error)) { + events.add( + ValidationEvent.builder() + .id(getEventId() + "." + error) + .severity(Severity.WARNING) + .message(String.format( + "The `%s` error was removed from the `%s` operation.", + error, change.getShapeId())) + .shape(change.getNewShape()) + .sourceLocation(change.getNewShape().getSourceLocation()) + .build() + ); } } diff --git a/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/AddedOperationErrorTest.java b/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/AddedOperationErrorTest.java index bbebcb6ed90..7088deb01ef 100644 --- a/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/AddedOperationErrorTest.java +++ b/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/AddedOperationErrorTest.java @@ -46,5 +46,7 @@ public void detectsAddedErrors() { List events = ModelDiff.compare(modelA, modelB); assertThat(TestHelper.findEvents(events, "AddedOperationError").size(), equalTo(2)); + assertThat(TestHelper.findEvents(events, "AddedOperationError.foo.baz#E1").size(), equalTo(1)); + assertThat(TestHelper.findEvents(events, "AddedOperationError.foo.baz#E2").size(), equalTo(1)); } } diff --git a/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/RemovedOperationErrorTest.java b/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/RemovedOperationErrorTest.java index 5d7f8cc9611..adf03acc1c2 100644 --- a/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/RemovedOperationErrorTest.java +++ b/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/RemovedOperationErrorTest.java @@ -52,6 +52,8 @@ public void detectsRemovedErrors() { // Emits an event for each removal. assertThat(TestHelper.findEvents(events, "RemovedOperationError").size(), equalTo(2)); + assertThat(TestHelper.findEvents(events, "RemovedOperationError.foo.baz#E1").size(), equalTo(1)); + assertThat(TestHelper.findEvents(events, "RemovedOperationError.foo.baz#E2").size(), equalTo(1)); assertThat(TestHelper.findEvents(events, "RemovedOperationError").get(0).toString(), startsWith("[WARNING] foo.baz#Operation: The `foo.baz#E1` error was removed " + "from the `foo.baz#Operation` operation. | RemovedOperationError")); From 1e1e912f0a497e4c0459386776c337bbb528c1fa Mon Sep 17 00:00:00 2001 From: Richard Chen Date: Thu, 8 Jun 2023 13:44:19 -0400 Subject: [PATCH 2/2] Remove namespace from event id --- .../amazon/smithy/diff/evaluators/AddedOperationError.java | 3 +-- .../amazon/smithy/diff/evaluators/RemovedOperationError.java | 3 +-- .../smithy/diff/evaluators/AddedOperationErrorTest.java | 4 ++-- .../smithy/diff/evaluators/RemovedOperationErrorTest.java | 4 ++-- 4 files changed, 6 insertions(+), 8 deletions(-) diff --git a/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/AddedOperationError.java b/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/AddedOperationError.java index 415fb6b43e1..1f431156c86 100644 --- a/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/AddedOperationError.java +++ b/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/AddedOperationError.java @@ -47,7 +47,7 @@ private List createErrorViolations(ChangedShape if (!change.getOldShape().getErrors().contains(error)) { events.add( ValidationEvent.builder() - .id(getEventId() + "." + error) + .id(getEventId() + "." + error.getName()) .severity(Severity.WARNING) .message(String.format( "The `%s` error was added to the `%s` operation. This " @@ -57,7 +57,6 @@ private List createErrorViolations(ChangedShape + "parameter to an operation).", error, change.getShapeId())) .shape(change.getNewShape()) - .sourceLocation(change.getNewShape().getSourceLocation()) .build() ); } diff --git a/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/RemovedOperationError.java b/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/RemovedOperationError.java index 0167c6047de..ca3a7fa0ba2 100644 --- a/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/RemovedOperationError.java +++ b/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/RemovedOperationError.java @@ -47,13 +47,12 @@ private List createErrorViolations(ChangedShape if (!change.getNewShape().getErrors().contains(error)) { events.add( ValidationEvent.builder() - .id(getEventId() + "." + error) + .id(getEventId() + "." + error.getName()) .severity(Severity.WARNING) .message(String.format( "The `%s` error was removed from the `%s` operation.", error, change.getShapeId())) .shape(change.getNewShape()) - .sourceLocation(change.getNewShape().getSourceLocation()) .build() ); } diff --git a/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/AddedOperationErrorTest.java b/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/AddedOperationErrorTest.java index 7088deb01ef..38fa859a307 100644 --- a/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/AddedOperationErrorTest.java +++ b/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/AddedOperationErrorTest.java @@ -46,7 +46,7 @@ public void detectsAddedErrors() { List events = ModelDiff.compare(modelA, modelB); assertThat(TestHelper.findEvents(events, "AddedOperationError").size(), equalTo(2)); - assertThat(TestHelper.findEvents(events, "AddedOperationError.foo.baz#E1").size(), equalTo(1)); - assertThat(TestHelper.findEvents(events, "AddedOperationError.foo.baz#E2").size(), equalTo(1)); + assertThat(TestHelper.findEvents(events, "AddedOperationError.E1").size(), equalTo(1)); + assertThat(TestHelper.findEvents(events, "AddedOperationError.E2").size(), equalTo(1)); } } diff --git a/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/RemovedOperationErrorTest.java b/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/RemovedOperationErrorTest.java index adf03acc1c2..3c6ff908afb 100644 --- a/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/RemovedOperationErrorTest.java +++ b/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/RemovedOperationErrorTest.java @@ -52,8 +52,8 @@ public void detectsRemovedErrors() { // Emits an event for each removal. assertThat(TestHelper.findEvents(events, "RemovedOperationError").size(), equalTo(2)); - assertThat(TestHelper.findEvents(events, "RemovedOperationError.foo.baz#E1").size(), equalTo(1)); - assertThat(TestHelper.findEvents(events, "RemovedOperationError.foo.baz#E2").size(), equalTo(1)); + assertThat(TestHelper.findEvents(events, "RemovedOperationError.E1").size(), equalTo(1)); + assertThat(TestHelper.findEvents(events, "RemovedOperationError.E2").size(), equalTo(1)); assertThat(TestHelper.findEvents(events, "RemovedOperationError").get(0).toString(), startsWith("[WARNING] foo.baz#Operation: The `foo.baz#E1` error was removed " + "from the `foo.baz#Operation` operation. | RemovedOperationError"));