Add validator to warn on ignored idempotency token trait #2358
Conversation
...ava/software/amazon/smithy/model/validation/validators/IdempotencyTokenIgnoredValidator.java
Outdated
Show resolved
Hide resolved
| } | ||
| List<ValidationEvent> events = new ArrayList<>(); | ||
| NeighborProvider reverse = NeighborProviderIndex.of(model).getReverseProvider(); | ||
| for (MemberShape memberShape : model.getMemberShapes()) { |
There was a problem hiding this comment.
You can use getMemberShapesWithTrait(IdempotencyToken.class)
|
|
||
| // Store relationships so we can emit one event per ignored binding. | ||
| Map<RelationshipType, List<ShapeId>> ignoredRelationships = new TreeMap<>(); | ||
| List<ValidationEvent> events = new ArrayList<>(); |
There was a problem hiding this comment.
minor but we could just pass in the mutable list of events from the validate method rather than needing to create intermediate lists
| continue; | ||
| } | ||
| if (relationship.getRelationshipType() != RelationshipType.INPUT) { | ||
| ignoredRelationships.merge(relationship.getRelationshipType(), |
There was a problem hiding this comment.
Can this be simplified to just use ignoredRelationships.computeIfAbsent? Might not need mergeShapeIdsList either at that point.
| Map<RelationshipType, List<ShapeId>> ignoredRelationships | ||
| ) { | ||
| String mixedIn = memberShape.getMixins().isEmpty() ? "" : " mixed in"; | ||
| String message = "The `%s` trait applied to this%s member is "; |
There was a problem hiding this comment.
Can we say something more clear like "The idempotencyToken trait only has an effect when applied to a top-level operation input member, but it was applied and ignored in the following contexts: " ... etc
There was a problem hiding this comment.
I will change as described.
| ) { | ||
| String mixedIn = memberShape.getMixins().isEmpty() ? "" : " mixed in"; | ||
| String message = "The `%s` trait applied to this%s member is "; | ||
| if (checkedRelationshipCount == ignoredRelationships.size()) { |
There was a problem hiding this comment.
Why omit the context here? It looks like we emit the event on the member that has the trait, but don't say how the containing shape was referenced in a way that caused the trait to be ignored.
…tion/validators/IdempotencyTokenIgnoredValidator.java Co-authored-by: Michael Dowling <michael@mtdowling.com>
| // Store relationships so we can emit one event per ignored binding. | ||
| Map<RelationshipType, List<ShapeId>> ignoredRelationships = new TreeMap<>(); | ||
| List<Relationship> relationships = reverse.getNeighbors(containerShape); | ||
| int checkedRelationshipCount = relationships.size(); |
There was a problem hiding this comment.
looks like this doesn't do anything now
There was a problem hiding this comment.
🤦 yeah, let me remove that.
Background
Added a validator to warn on ignored use of @idempotencyToken trait. As per spec,
This validator warns when we find a member using the trait in a context where it will be ignored. We cannot ERROR here as the structures might be reused in valid and invalid context, thus we only warning about the context where it will be ignored.
Testing
Links
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.