Skip to content

Commit c6ac196

Browse files
mtdowlingmtdowling
committed
Add support for hierarchical event IDs
Many of the validators in Smithy and Smithy Diff today emit events for various reasons. A single validator might event different events with different severities, and there is currently no way to suppress just one specific type of event for a given validator. For example, the ChangedNullability SmithyDiff validator emits events for various reasons, some of which different tools may choose to suppress. However, without inspecting the message of the event, there's no way to know what you're suppressing. To give existing validators the ability to emit more granular validation events without breaking current validators, this change introduces event ID hierarchies and hierarchical suppression IDs using dots (.). For example, ChangedNullability.RemovedRequiredTrait is now emitted, and tools can check for "ChangedNullability" to match all events, or more granularly only match "ChangedNullability.RemovedRequiredTrait". This can also be used in suppressions so that existing Smithy validators can be updated to emit more granular events.
1 parent 67af34b commit c6ac196

File tree

12 files changed

+418
-48
lines changed

12 files changed

+418
-48
lines changed

‎docs/source-2.0/spec/model-validation.rst

+52
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,11 @@ objects that are used to constrain a model. Each object in the
6060

6161
If ``id`` is not specified, it will default to the ``name`` property of
6262
the validator definition.
63+
64+
IDs that contain dots (.) are hierarchical. For example, the ID
65+
"Foo.Bar" contains the ID "Foo". Event ID hierarchies can be leveraged
66+
to group validation events and allow more granular
67+
:ref:`suppressions <suppression-definition>`.
6368
* - message
6469
- ``string``
6570
- Provides a custom message to use when emitting validation events. The
@@ -262,6 +267,53 @@ ID of ``OverlyBroadValidator``:
262267
]
263268
264269
270+
Matching suppression event IDs
271+
==============================
272+
273+
Both the :ref:`suppress-trait` and suppressions
274+
:ref:`defined in metadata <suppressions-metadata>` match events hierarchically
275+
based on dot (.) segments. For example, given a validation event ID of
276+
"Foo.Bar", and a suppression ID of "Foo", the suppression ID matches the
277+
event ID because "Foo.Bar" begins with the segment "Foo". However, a suppression
278+
ID of "Foo.Bar" does not match an event ID of "Foo" because "Foo.Bar" is more
279+
specific. Further, a suppression ID of "ABC" does not match an event ID of
280+
"ABC123" because "ABC" is not a segment of the event ID.
281+
282+
.. list-table::
283+
:header-rows: 1
284+
285+
* - Event ID
286+
- Suppression ID
287+
- Is match
288+
* - ``Foo``
289+
- ``Foo``
290+
- Yes
291+
* - ``Foo.Bar``
292+
- ``Foo``
293+
- Yes
294+
* - ``Foo.Bar.Baz``
295+
- ``Foo``
296+
- Yes
297+
* - ``Foo.``
298+
- ``Foo.``
299+
- Yes
300+
* - ``Foo.``
301+
- ``Foo``
302+
- Yes
303+
* - ``Foo``
304+
- ``Foo.``
305+
- No
306+
* - ``Foosball``
307+
- ``Foo``
308+
- No
309+
* - ``Foo``
310+
- ``Foo.Bar``
311+
- No
312+
* - ``Abc.Foo.Bar``
313+
- ``Foo.Bar``
314+
- No
315+
316+
265317
-------------------
266318
Built-in validators
267319
-------------------

‎smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedNullability.java

+51-38
Original file line numberDiff line numberDiff line change
@@ -18,14 +18,14 @@
1818
import java.util.ArrayList;
1919
import java.util.List;
2020
import java.util.Objects;
21-
import java.util.StringJoiner;
2221
import java.util.stream.Stream;
2322
import software.amazon.smithy.diff.ChangedShape;
2423
import software.amazon.smithy.diff.Differences;
2524
import software.amazon.smithy.model.Model;
2625
import software.amazon.smithy.model.knowledge.NullableIndex;
2726
import software.amazon.smithy.model.shapes.MemberShape;
2827
import software.amazon.smithy.model.shapes.Shape;
28+
import software.amazon.smithy.model.shapes.ShapeId;
2929
import software.amazon.smithy.model.shapes.StructureShape;
3030
import software.amazon.smithy.model.traits.ClientOptionalTrait;
3131
import software.amazon.smithy.model.traits.DefaultTrait;
@@ -45,24 +45,23 @@ public class ChangedNullability extends AbstractDiffEvaluator {
4545
public List<ValidationEvent> evaluate(Differences differences) {
4646
NullableIndex oldIndex = NullableIndex.of(differences.getOldModel());
4747
NullableIndex newIndex = NullableIndex.of(differences.getNewModel());
48-
4948
List<ValidationEvent> events = new ArrayList<>();
5049

51-
Stream<ChangedShape<MemberShape>> changed = Stream.concat(
50+
Stream.concat(
5251
// Get members that changed.
5352
differences.changedShapes(MemberShape.class),
5453
// Get members of structures that added/removed the input trait.
5554
changedInputMembers(differences)
56-
);
57-
58-
changed.map(change -> {
55+
).forEach(change -> {
5956
// If NullableIndex says the nullability of a member changed, then that's a breaking change.
6057
MemberShape oldShape = change.getOldShape();
6158
MemberShape newShape = change.getNewShape();
6259
boolean wasNullable = oldIndex.isMemberNullable(oldShape);
6360
boolean isNowNullable = newIndex.isMemberNullable(newShape);
64-
return wasNullable == isNowNullable ? null : createError(differences, change, wasNullable);
65-
}).filter(Objects::nonNull).forEach(events::add);
61+
if (wasNullable != isNowNullable) {
62+
createErrors(differences, change, wasNullable, events);
63+
}
64+
});
6665

6766
return events;
6867
}
@@ -79,78 +78,92 @@ private Stream<ChangedShape<MemberShape>> changedInputMembers(Differences differ
7978
.filter(Objects::nonNull));
8079
}
8180

82-
private ValidationEvent createError(
81+
private void createErrors(
8382
Differences differences,
8483
ChangedShape<MemberShape> change,
85-
boolean wasNullable
84+
boolean wasNullable,
85+
List<ValidationEvent> events
8686
) {
8787
MemberShape oldMember = change.getOldShape();
8888
MemberShape newMember = change.getNewShape();
8989
String message = String.format("Member `%s` changed from %s to %s: ",
9090
oldMember.getMemberName(),
9191
wasNullable ? "nullable" : "non-nullable",
9292
wasNullable ? "non-nullable" : "nullable");
93-
StringJoiner joiner = new StringJoiner("; ", message, "");
9493
boolean oldHasInput = hasInputTrait(differences.getOldModel(), oldMember);
9594
boolean newHasInput = hasInputTrait(differences.getNewModel(), newMember);
95+
ShapeId shape = change.getShapeId();
9696
Shape newTarget = differences.getNewModel().expectShape(newMember.getTarget());
97-
Severity severity = null;
97+
int currentEventSize = events.size();
9898

9999
if (oldHasInput && !newHasInput) {
100100
// If there was an input trait before, but not now, then the nullability must have
101101
// changed from nullable to non-nullable.
102-
joiner.add("The @input trait was removed from " + newMember.getContainer());
103-
severity = Severity.ERROR;
102+
events.add(emit(Severity.ERROR, "RemovedInputTrait", shape, message,
103+
"The @input trait was removed from " + newMember.getContainer()));
104104
} else if (!oldHasInput && newHasInput) {
105105
// If there was no input trait before, but there is now, then the nullability must have
106106
// changed from non-nullable to nullable.
107-
joiner.add("The @input trait was added to " + newMember.getContainer());
108-
severity = Severity.DANGER;
107+
events.add(emit(Severity.DANGER, "AddedInputTrait", shape, message,
108+
"The @input trait was added to " + newMember.getContainer()));
109109
} else if (!newHasInput) {
110110
// Can't add nullable to a preexisting required member.
111111
if (change.isTraitAdded(ClientOptionalTrait.ID) && change.isTraitInBoth(RequiredTrait.ID)) {
112-
joiner.add("The @nullable trait was added to a @required member.");
113-
severity = Severity.ERROR;
112+
events.add(emit(Severity.ERROR, "AddedNullableTrait", shape, message,
113+
"The @nullable trait was added to a @required member."));
114114
}
115115
// Can't add required to a member unless the member is marked as nullable.
116116
if (change.isTraitAdded(RequiredTrait.ID) && !newMember.hasTrait(ClientOptionalTrait.ID)) {
117-
joiner.add("The @required trait was added to a member that is not marked as @nullable.");
118-
severity = Severity.ERROR;
117+
events.add(emit(Severity.ERROR, "AddedRequiredTrait", shape, message,
118+
"The @required trait was added to a member that is not marked as @nullable."));
119119
}
120120
// Can't add the default trait to a member unless the member was previously required.
121121
if (change.isTraitAdded(DefaultTrait.ID) && !change.isTraitRemoved(RequiredTrait.ID)) {
122-
joiner.add("The @default trait was added to a member that was not previously @required.");
123-
severity = Severity.ERROR;
122+
events.add(emit(Severity.ERROR, "AddedDefaultTrait", shape, message,
123+
"The @default trait was added to a member that was not previously @required."));
124124
}
125125
// Can only remove the required trait if the member was nullable or replaced by the default trait.
126126
if (change.isTraitRemoved(RequiredTrait.ID)
127127
&& !newMember.hasTrait(DefaultTrait.ID)
128128
&& !oldMember.hasTrait(ClientOptionalTrait.ID)) {
129129
if (newTarget.isStructureShape() || newTarget.isUnionShape()) {
130-
severity = severity == null ? Severity.WARNING : severity;
131-
joiner.add("The @required trait was removed from a member that targets a " + newTarget.getType()
132-
+ ". This is backward compatible in generators that always treat structures and "
133-
+ "unions as optional (e.g., AWS generators)");
130+
events.add(emit(Severity.WARNING, "RemovedRequiredTrait.StructureOrUnion", shape, message,
131+
"The @required trait was removed from a member that targets a "
132+
+ newTarget.getType() + ". This is backward compatible in generators that "
133+
+ "always treat structures and unions as optional (e.g., AWS generators)"));
134134
} else {
135-
joiner.add("The @required trait was removed and not replaced with the @default trait and "
136-
+ "@addedDefault trait.");
137-
severity = Severity.ERROR;
135+
events.add(emit(Severity.ERROR, "RemovedRequiredTrait", shape, message,
136+
"The @required trait was removed and not replaced with the @default trait and "
137+
+ "@addedDefault trait."));
138138
}
139139
}
140140
}
141141

142-
// If no explicit severity was detected, then assume an error.
143-
severity = severity == null ? Severity.ERROR : severity;
144-
145-
return ValidationEvent.builder()
146-
.id(getEventId())
147-
.shapeId(change.getNewShape())
148-
.severity(severity)
149-
.message(joiner.toString())
150-
.build();
142+
// If not specific event was emitted, emit a generic event.
143+
if (events.size() == currentEventSize) {
144+
events.add(emit(Severity.ERROR, null, shape, null, message));
145+
}
151146
}
152147

153148
private boolean hasInputTrait(Model model, MemberShape member) {
154149
return model.getShape(member.getContainer()).filter(shape -> shape.hasTrait(InputTrait.ID)).isPresent();
155150
}
151+
152+
private ValidationEvent emit(
153+
Severity severity,
154+
String eventIdSuffix,
155+
ShapeId shape,
156+
String prefixMessage,
157+
String message
158+
) {
159+
String actualId = eventIdSuffix == null ? getEventId() : (getEventId() + '.' + eventIdSuffix);
160+
String actualMessage = prefixMessage == null ? message : (prefixMessage + "; " + message);
161+
return ValidationEvent.builder()
162+
.id(actualId)
163+
.shapeId(shape)
164+
.message(actualMessage)
165+
.severity(severity)
166+
.message(message)
167+
.build();
168+
}
156169
}

‎smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedNullabilityTest.java

+7-7
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ public void detectsInvalidAdditionOfDefaultTrait() {
9090
assertThat(result.isDiffBreaking(), is(true));
9191
assertThat(result.getDiffEvents().stream()
9292
.filter(event -> event.getSeverity() == Severity.ERROR)
93-
.filter(event -> event.getId().equals("ChangedNullability"))
93+
.filter(event -> event.getId().equals("ChangedNullability.AddedDefaultTrait"))
9494
.filter(event -> event.getShapeId().get().equals(a.getAllMembers().get("foo").getId()))
9595
.filter(event -> event.getMessage().contains("The @default trait was added to a member that "
9696
+ "was not previously @required"))
@@ -115,7 +115,7 @@ public void removingTheRequiredTraitOnInputStructureIsOk() {
115115
ModelDiff.Result result = ModelDiff.builder().oldModel(model1).newModel(model2).compare();
116116

117117
assertThat(result.getDiffEvents().stream()
118-
.filter(event -> event.getId().equals("ChangedNullability"))
118+
.filter(event -> event.getId().equals("ChangedNullability.RemovedRequiredTrait.StructureOrUnion"))
119119
.count(), equalTo(0L));
120120
}
121121

@@ -137,7 +137,7 @@ public void detectsInvalidRemovalOfRequired() {
137137
assertThat(result.isDiffBreaking(), is(true));
138138
assertThat(result.getDiffEvents().stream()
139139
.filter(event -> event.getSeverity() == Severity.ERROR)
140-
.filter(event -> event.getId().equals("ChangedNullability"))
140+
.filter(event -> event.getId().equals("ChangedNullability.RemovedRequiredTrait"))
141141
.filter(event -> event.getShapeId().get().equals(a.getAllMembers().get("foo").getId()))
142142
.filter(event -> event.getMessage().contains("The @required trait was removed and not "
143143
+ "replaced with the @default trait"))
@@ -157,7 +157,7 @@ public void detectAdditionOfRequiredTrait() {
157157

158158
assertThat(events.stream()
159159
.filter(event -> event.getSeverity() == Severity.ERROR)
160-
.filter(event -> event.getId().equals("ChangedNullability"))
160+
.filter(event -> event.getId().equals("ChangedNullability.AddedRequiredTrait"))
161161
.filter(event -> event.getMessage().contains("The @required trait was added to a member "
162162
+ "that is not marked as @nullable"))
163163
.count(), equalTo(1L));
@@ -180,7 +180,7 @@ public void detectAdditionOfNullableTrait() {
180180

181181
assertThat(events.stream()
182182
.filter(event -> event.getSeverity() == Severity.ERROR)
183-
.filter(event -> event.getId().equals("ChangedNullability"))
183+
.filter(event -> event.getId().equals("ChangedNullability.AddedNullableTrait"))
184184
.filter(event -> event.getMessage().contains("The @nullable trait was added to a "
185185
+ "@required member"))
186186
.count(), equalTo(1L));
@@ -202,7 +202,7 @@ public void detectsAdditionOfInputTrait() {
202202

203203
assertThat(events.stream()
204204
.filter(event -> event.getSeverity() == Severity.DANGER)
205-
.filter(event -> event.getId().equals("ChangedNullability"))
205+
.filter(event -> event.getId().equals("ChangedNullability.AddedInputTrait"))
206206
.filter(event -> event.getMessage().contains("The @input trait was added to"))
207207
.count(), equalTo(1L));
208208
}
@@ -226,7 +226,7 @@ public void detectsRemovalOfInputTrait() {
226226

227227
assertThat(events.stream()
228228
.filter(event -> event.getSeverity() == Severity.ERROR)
229-
.filter(event -> event.getId().equals("ChangedNullability"))
229+
.filter(event -> event.getId().equals("ChangedNullability.RemovedInputTrait"))
230230
.filter(event -> event.getMessage().contains("The @input trait was removed from"))
231231
.count(), equalTo(1L));
232232
}

‎smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/TestHelper.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@
2424
public final class TestHelper {
2525
public static List<ValidationEvent> findEvents(List<ValidationEvent> events, String eventId) {
2626
return events.stream()
27-
.filter(event -> event.getId().equals(eventId))
27+
.filter(event -> event.containsId(eventId))
2828
.collect(Collectors.toList());
2929
}
3030

‎smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidationEvent.java

+25
Original file line numberDiff line numberDiff line change
@@ -241,6 +241,31 @@ public String getEventId() {
241241
return getId();
242242
}
243243

244+
/**
245+
* Tests if the event ID hierarchically contains the given ID.
246+
*
247+
* <p>Event IDs that contain dots (.) are hierarchical. An event ID of
248+
* {@code "Foo.Bar"} contains the ID {@code "Foo"} and {@code "Foo.Bar"}.
249+
* However, an event ID of {@code "Foo"} does not contain the ID
250+
* {@code "Foo.Bar"} as {@code "Foo.Bar"} is more specific than {@code "Foo"}.
251+
* If an event ID exactly matches the given {@code id}, then it also contains
252+
* the ID (for example, {@code "Foo.Bar."} contains {@code "Foo.Bar."}.
253+
*
254+
* @param id ID to test.
255+
* @return Returns true if the event's event ID contains the given {@code id}.
256+
*/
257+
public boolean containsId(String id) {
258+
int eventLength = eventId.length();
259+
int suppressionLength = id.length();
260+
if (suppressionLength == eventLength) {
261+
return id.equals(eventId);
262+
} else if (suppressionLength > eventLength) {
263+
return false;
264+
} else {
265+
return eventId.startsWith(id) && eventId.charAt(id.length()) == '.';
266+
}
267+
}
268+
244269
/**
245270
* Returns the identifier of the validation event.
246271
*

‎smithy-model/src/main/java/software/amazon/smithy/model/validation/suppressions/MetadataSuppression.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ static MetadataSuppression fromNode(Node node) {
5555

5656
@Override
5757
public boolean test(ValidationEvent event) {
58-
return event.getId().equals(id) && matchesNamespace(event);
58+
return event.containsId(id) && matchesNamespace(event);
5959
}
6060

6161
@Override

‎smithy-model/src/main/java/software/amazon/smithy/model/validation/suppressions/TraitSuppression.java

+11-1
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,16 @@ final class TraitSuppression implements Suppression {
3434

3535
@Override
3636
public boolean test(ValidationEvent event) {
37-
return event.getShapeId().filter(shape::equals).isPresent() && trait.getValues().contains(event.getId());
37+
if (!event.getShapeId().filter(shape::equals).isPresent()) {
38+
return false;
39+
}
40+
41+
for (String value : trait.getValues()) {
42+
if (event.containsId(value)) {
43+
return true;
44+
}
45+
}
46+
47+
return false;
3848
}
3949
}

0 commit comments

Comments
 (0)