From ebab1f60dd3d017e85d04322c4d35d15c2444e85 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20Sugawara=20=28=E2=88=A9=EF=BD=80-=C2=B4=29?= =?UTF-8?q?=E2=8A=83=E2=94=81=E7=82=8E=E7=82=8E=E7=82=8E=E7=82=8E=E7=82=8E?= Date: Thu, 25 Jul 2024 17:12:42 -0700 Subject: [PATCH 1/4] Add validator to warn on ignored idempotency token trait --- .../IdempotencyTokenIgnoredValidator.java | 131 ++++++++++++++++++ ...e.amazon.smithy.model.validation.Validator | 1 + .../idempotency-token-trait-ignored.errors | 4 + .../idempotency-token-trait-ignored.smithy | 62 +++++++++ 4 files changed, 198 insertions(+) create mode 100644 smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/IdempotencyTokenIgnoredValidator.java create mode 100644 smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/idempotency-token-trait-ignored.errors create mode 100644 smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/idempotency-token-trait-ignored.smithy diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/IdempotencyTokenIgnoredValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/IdempotencyTokenIgnoredValidator.java new file mode 100644 index 00000000000..07dcfbeb888 --- /dev/null +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/IdempotencyTokenIgnoredValidator.java @@ -0,0 +1,131 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +package software.amazon.smithy.model.validation.validators; + +import static java.lang.String.format; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Locale; +import java.util.Map; +import java.util.Set; +import java.util.TreeMap; +import java.util.TreeSet; +import software.amazon.smithy.model.Model; +import software.amazon.smithy.model.knowledge.NeighborProviderIndex; +import software.amazon.smithy.model.neighbor.NeighborProvider; +import software.amazon.smithy.model.neighbor.Relationship; +import software.amazon.smithy.model.neighbor.RelationshipType; +import software.amazon.smithy.model.shapes.MemberShape; +import software.amazon.smithy.model.shapes.Shape; +import software.amazon.smithy.model.shapes.ShapeId; +import software.amazon.smithy.model.shapes.StructureShape; +import software.amazon.smithy.model.traits.IdempotencyTokenTrait; +import software.amazon.smithy.model.traits.MixinTrait; +import software.amazon.smithy.model.traits.Trait; +import software.amazon.smithy.model.validation.AbstractValidator; +import software.amazon.smithy.model.validation.ValidationEvent; +import software.amazon.smithy.utils.ListUtils; + +/** + * Emits warnings when a structure member has an idempotency token trait that will be ignored. + */ +public class IdempotencyTokenIgnoredValidator extends AbstractValidator { + + @Override + public List validate(Model model) { + if (!model.getAppliedTraits().contains(IdempotencyTokenTrait.ID)) { + return Collections.emptyList(); + } + List events = new ArrayList<>(); + NeighborProvider reverse = NeighborProviderIndex.of(model).getReverseProvider(); + for (MemberShape memberShape : model.getMemberShapes()) { + Shape container = model.expectShape(memberShape.getContainer()); + // Skip non-structures (invalid) and mixins (handled at mixed site). + if (!container.isStructureShape() || container.hasTrait(MixinTrait.class)) { + continue; + } + + if (memberShape.hasTrait(IdempotencyTokenTrait.class)) { + Trait trait = memberShape.expectTrait(IdempotencyTokenTrait.class); + events.addAll(checkRelationships(container.asStructureShape().get(), memberShape, trait, reverse)); + } + } + return events; + } + + private List checkRelationships( + StructureShape containerShape, + MemberShape memberShape, + Trait trait, + NeighborProvider reverse + ) { + + // Store relationships so we can emit one event per ignored binding. + Map> ignoredRelationships = new TreeMap<>(); + List events = new ArrayList<>(); + List relationships = reverse.getNeighbors(containerShape); + int checkedRelationshipCount = relationships.size(); + for (Relationship relationship : relationships) { + // Skip members of the container. + if (relationship.getRelationshipType() == RelationshipType.MEMBER_CONTAINER) { + checkedRelationshipCount--; + continue; + } + if (relationship.getRelationshipType() != RelationshipType.INPUT) { + ignoredRelationships.merge(relationship.getRelationshipType(), + ListUtils.of(relationship.getShape().getId()), this::mergeShapeIdLists); + } + } + + // If we detected invalid relationships, build the right event message. + if (!ignoredRelationships.isEmpty()) { + events.add(emit(memberShape, trait, checkedRelationshipCount, ignoredRelationships)); + } + + return events; + } + + private List mergeShapeIdLists(List shapeIds1, List shapeIds2) { + List shapeIds = new ArrayList<>(); + shapeIds.addAll(shapeIds1); + shapeIds.addAll(shapeIds2); + return shapeIds; + } + + private ValidationEvent emit( + MemberShape memberShape, + Trait trait, + int checkedRelationshipCount, + Map> ignoredRelationships + ) { + String mixedIn = memberShape.getMixins().isEmpty() ? "" : " mixed in"; + String message = "The `%s` trait applied to this%s member is "; + if (checkedRelationshipCount == ignoredRelationships.size()) { + message += "ignored in all contexts."; + } else { + message += "ignored in some contexts: " + formatIgnoredRelationships(ignoredRelationships); + } + return warning(memberShape, trait, format(message, Trait.getIdiomaticTraitName(trait), mixedIn)); + } + + private String formatIgnoredRelationships(Map> ignoredRelationships) { + List relationshipTypeBindings = new ArrayList<>(); + for (Map.Entry> ignoredRelationshipType : ignoredRelationships.entrySet()) { + StringBuilder stringBuilder = new StringBuilder(ignoredRelationshipType.getKey().toString() + .toLowerCase(Locale.US) + .replace("_", " ")); + Set bindings = new TreeSet<>(); + for (ShapeId binding : ignoredRelationshipType.getValue()) { + bindings.add(binding.toString()); + } + stringBuilder.append(": [").append(String.join(", ", bindings)).append("]"); + relationshipTypeBindings.add(stringBuilder.toString()); + } + return "{" + String.join(", ", relationshipTypeBindings) + "}"; + } +} diff --git a/smithy-model/src/main/resources/META-INF/services/software.amazon.smithy.model.validation.Validator b/smithy-model/src/main/resources/META-INF/services/software.amazon.smithy.model.validation.Validator index b70dbef6a0a..2c4af42885c 100644 --- a/smithy-model/src/main/resources/META-INF/services/software.amazon.smithy.model.validation.Validator +++ b/smithy-model/src/main/resources/META-INF/services/software.amazon.smithy.model.validation.Validator @@ -22,6 +22,7 @@ software.amazon.smithy.model.validation.validators.HttpResponseCodeSemanticsVali software.amazon.smithy.model.validation.validators.HttpUriGreedyLabelValidator software.amazon.smithy.model.validation.validators.HttpUriConflictValidator software.amazon.smithy.model.validation.validators.HttpUriFormatValidator +software.amazon.smithy.model.validation.validators.IdempotencyTokenIgnoredValidator software.amazon.smithy.model.validation.validators.JsonNameValidator software.amazon.smithy.model.validation.validators.LengthTraitValidator software.amazon.smithy.model.validation.validators.MediaTypeValidator diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/idempotency-token-trait-ignored.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/idempotency-token-trait-ignored.errors new file mode 100644 index 00000000000..73cfac5b829 --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/idempotency-token-trait-ignored.errors @@ -0,0 +1,4 @@ +[WARNING] io.smithy.example#StructureOne$stringMember: The `idempotencyToken` trait applied to this member is ignored in some contexts: {member target: [io.smithy.example#StructureTwo$structureOne]} | IdempotencyTokenIgnored +[WARNING] io.smithy.example#StructureThree$stringMember: The `idempotencyToken` trait applied to this mixed in member is ignored in some contexts: {output: [io.smithy.example#OperationFour]} | IdempotencyTokenIgnored + + diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/idempotency-token-trait-ignored.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/idempotency-token-trait-ignored.smithy new file mode 100644 index 00000000000..dff5b778f8d --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/idempotency-token-trait-ignored.smithy @@ -0,0 +1,62 @@ +$version: "2" + +namespace io.smithy.example + +service SmithyExample { + operations: [ + OperationOne + OperationTwo + OperationThree + OperationFour + ] +} + +operation OperationOne { + input := { + @idempotencyToken + stringMember: String + integerMember: Integer + } + output: Unit +} + +operation OperationTwo { + input: StructureOne + output: Unit +} + +operation OperationThree { + input: StructureTwo + output: Unit +} + +operation OperationFour { + input: StructureThree + output: StructureThree +} + + +structure StructureOne { + @idempotencyToken + stringMember: String + integerMember: Integer +} + + +structure StructureTwo { + stringMember: String + structureOne: StructureOne +} + + +@mixin +structure StructureMixin { + @idempotencyToken + stringMember: String +} + + +structure StructureThree with [StructureMixin] { + integerMember: Integer +} + From 4fc8c2cd6bcbd8aa30e66a3143cf5eb984e1c9f4 Mon Sep 17 00:00:00 2001 From: Manuel Sugawara Date: Mon, 29 Jul 2024 11:01:48 -0700 Subject: [PATCH 2/4] Update smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/IdempotencyTokenIgnoredValidator.java Co-authored-by: Michael Dowling --- .../validation/validators/IdempotencyTokenIgnoredValidator.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/IdempotencyTokenIgnoredValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/IdempotencyTokenIgnoredValidator.java index 07dcfbeb888..e4e6da43a59 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/IdempotencyTokenIgnoredValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/IdempotencyTokenIgnoredValidator.java @@ -34,7 +34,7 @@ /** * Emits warnings when a structure member has an idempotency token trait that will be ignored. */ -public class IdempotencyTokenIgnoredValidator extends AbstractValidator { +public final class IdempotencyTokenIgnoredValidator extends AbstractValidator { @Override public List validate(Model model) { From 02323e33339468ba7b77f9c3025980b72c97ef4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20Sugawara=20=28=E2=88=A9=EF=BD=80-=C2=B4=29?= =?UTF-8?q?=E2=8A=83=E2=94=81=E7=82=8E=E7=82=8E=E7=82=8E=E7=82=8E=E7=82=8E?= Date: Mon, 29 Jul 2024 12:10:16 -0700 Subject: [PATCH 3/4] Address PR comments --- .../IdempotencyTokenIgnoredValidator.java | 65 +++++++------------ .../idempotency-token-trait-ignored.errors | 5 +- 2 files changed, 25 insertions(+), 45 deletions(-) diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/IdempotencyTokenIgnoredValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/IdempotencyTokenIgnoredValidator.java index e4e6da43a59..e2abfbd80fe 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/IdempotencyTokenIgnoredValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/IdempotencyTokenIgnoredValidator.java @@ -5,8 +5,6 @@ package software.amazon.smithy.model.validation.validators; -import static java.lang.String.format; - import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -15,6 +13,7 @@ import java.util.Set; import java.util.TreeMap; import java.util.TreeSet; +import java.util.stream.Collectors; import software.amazon.smithy.model.Model; import software.amazon.smithy.model.knowledge.NeighborProviderIndex; import software.amazon.smithy.model.neighbor.NeighborProvider; @@ -29,7 +28,6 @@ import software.amazon.smithy.model.traits.Trait; import software.amazon.smithy.model.validation.AbstractValidator; import software.amazon.smithy.model.validation.ValidationEvent; -import software.amazon.smithy.utils.ListUtils; /** * Emits warnings when a structure member has an idempotency token trait that will be ignored. @@ -43,31 +41,28 @@ public List validate(Model model) { } List events = new ArrayList<>(); NeighborProvider reverse = NeighborProviderIndex.of(model).getReverseProvider(); - for (MemberShape memberShape : model.getMemberShapes()) { + for (MemberShape memberShape : model.getMemberShapesWithTrait(IdempotencyTokenTrait.class)) { Shape container = model.expectShape(memberShape.getContainer()); // Skip non-structures (invalid) and mixins (handled at mixed site). if (!container.isStructureShape() || container.hasTrait(MixinTrait.class)) { continue; } - - if (memberShape.hasTrait(IdempotencyTokenTrait.class)) { - Trait trait = memberShape.expectTrait(IdempotencyTokenTrait.class); - events.addAll(checkRelationships(container.asStructureShape().get(), memberShape, trait, reverse)); - } + Trait trait = memberShape.expectTrait(IdempotencyTokenTrait.class); + checkRelationships(container.asStructureShape().get(), memberShape, trait, reverse, events); } return events; } - private List checkRelationships( + private void checkRelationships( StructureShape containerShape, MemberShape memberShape, Trait trait, - NeighborProvider reverse + NeighborProvider reverse, + List events ) { // Store relationships so we can emit one event per ignored binding. Map> ignoredRelationships = new TreeMap<>(); - List events = new ArrayList<>(); List relationships = reverse.getNeighbors(containerShape); int checkedRelationshipCount = relationships.size(); for (Relationship relationship : relationships) { @@ -77,54 +72,40 @@ private List checkRelationships( continue; } if (relationship.getRelationshipType() != RelationshipType.INPUT) { - ignoredRelationships.merge(relationship.getRelationshipType(), - ListUtils.of(relationship.getShape().getId()), this::mergeShapeIdLists); + ignoredRelationships.computeIfAbsent(relationship.getRelationshipType(), x -> new ArrayList<>()) + .add(relationship.getShape().getId()); } } // If we detected invalid relationships, build the right event message. if (!ignoredRelationships.isEmpty()) { - events.add(emit(memberShape, trait, checkedRelationshipCount, ignoredRelationships)); + events.add(emit(memberShape, trait, ignoredRelationships)); } - - return events; - } - - private List mergeShapeIdLists(List shapeIds1, List shapeIds2) { - List shapeIds = new ArrayList<>(); - shapeIds.addAll(shapeIds1); - shapeIds.addAll(shapeIds2); - return shapeIds; } private ValidationEvent emit( MemberShape memberShape, Trait trait, - int checkedRelationshipCount, Map> ignoredRelationships ) { - String mixedIn = memberShape.getMixins().isEmpty() ? "" : " mixed in"; - String message = "The `%s` trait applied to this%s member is "; - if (checkedRelationshipCount == ignoredRelationships.size()) { - message += "ignored in all contexts."; - } else { - message += "ignored in some contexts: " + formatIgnoredRelationships(ignoredRelationships); - } - return warning(memberShape, trait, format(message, Trait.getIdiomaticTraitName(trait), mixedIn)); + String message = + "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: " + + formatIgnoredRelationships(ignoredRelationships); + return warning(memberShape, trait, message); } private String formatIgnoredRelationships(Map> ignoredRelationships) { List relationshipTypeBindings = new ArrayList<>(); for (Map.Entry> ignoredRelationshipType : ignoredRelationships.entrySet()) { - StringBuilder stringBuilder = new StringBuilder(ignoredRelationshipType.getKey().toString() - .toLowerCase(Locale.US) - .replace("_", " ")); - Set bindings = new TreeSet<>(); - for (ShapeId binding : ignoredRelationshipType.getValue()) { - bindings.add(binding.toString()); - } - stringBuilder.append(": [").append(String.join(", ", bindings)).append("]"); - relationshipTypeBindings.add(stringBuilder.toString()); + StringBuilder buf = new StringBuilder(ignoredRelationshipType.getKey().toString() + .toLowerCase(Locale.US) + .replace("_", " ")); + Set bindings = ignoredRelationshipType.getValue().stream() + .map(ShapeId::toString) + .collect(Collectors.toCollection(TreeSet::new)); + buf.append(": [").append(String.join(", ", bindings)).append("]"); + relationshipTypeBindings.add(buf.toString()); } return "{" + String.join(", ", relationshipTypeBindings) + "}"; } diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/idempotency-token-trait-ignored.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/idempotency-token-trait-ignored.errors index 73cfac5b829..d8401773afe 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/idempotency-token-trait-ignored.errors +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/idempotency-token-trait-ignored.errors @@ -1,4 +1,3 @@ -[WARNING] io.smithy.example#StructureOne$stringMember: The `idempotencyToken` trait applied to this member is ignored in some contexts: {member target: [io.smithy.example#StructureTwo$structureOne]} | IdempotencyTokenIgnored -[WARNING] io.smithy.example#StructureThree$stringMember: The `idempotencyToken` trait applied to this mixed in member is ignored in some contexts: {output: [io.smithy.example#OperationFour]} | IdempotencyTokenIgnored - +[WARNING] io.smithy.example#StructureOne$stringMember: 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: {member target: [io.smithy.example#StructureTwo$structureOne]} | IdempotencyTokenIgnored +[WARNING] io.smithy.example#StructureThree$stringMember: 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: {output: [io.smithy.example#OperationFour]} | IdempotencyTokenIgnored From 2fa0b988fa096464df64d592dee0779167aa970a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20Sugawara=20=28=E2=88=A9=EF=BD=80-=C2=B4=29?= =?UTF-8?q?=E2=8A=83=E2=94=81=E7=82=8E=E7=82=8E=E7=82=8E=E7=82=8E=E7=82=8E?= Date: Mon, 29 Jul 2024 13:04:55 -0700 Subject: [PATCH 4/4] Remove unsued code --- .../validation/validators/IdempotencyTokenIgnoredValidator.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/IdempotencyTokenIgnoredValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/IdempotencyTokenIgnoredValidator.java index e2abfbd80fe..5adf13fe0d5 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/IdempotencyTokenIgnoredValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/IdempotencyTokenIgnoredValidator.java @@ -64,11 +64,9 @@ private void checkRelationships( // Store relationships so we can emit one event per ignored binding. Map> ignoredRelationships = new TreeMap<>(); List relationships = reverse.getNeighbors(containerShape); - int checkedRelationshipCount = relationships.size(); for (Relationship relationship : relationships) { // Skip members of the container. if (relationship.getRelationshipType() == RelationshipType.MEMBER_CONTAINER) { - checkedRelationshipCount--; continue; } if (relationship.getRelationshipType() != RelationshipType.INPUT) {