From 81b6261c078ecb130475c36b533297980de9ee34 Mon Sep 17 00:00:00 2001 From: Miles Ziemer Date: Fri, 1 Nov 2024 13:44:04 -0400 Subject: [PATCH 1/2] Add validator for xmlFlattened + xmlName Adds a validator for the xmlFlattened trait that checks if the member's target is a list that has a member with xmlName, and that xmlName doesn't match the name of the xmlFlattened member. xmlFlattened causes the xmlName of the list member to be ignored. This is called out in our docs: https://smithy.io/2.0/spec/protocol-traits.html#flattened-list-serialization, but we were missing a validator for it. It is a warning because you might mean to do this, i.e. if the list is used in multiple places. I also added some suppressions to our protocol tests that are testing the behavior for models that would trip up this validator. --- .../model/awsQuery/main.smithy | 10 ++++ .../model/ec2Query/main.smithy | 10 ++++ .../model/restXml/main.smithy | 10 ++++ .../XmlFlattenedTraitValidator.java | 51 +++++++++++++++++++ ...e.amazon.smithy.model.validation.Validator | 1 + .../validators/xml-flattened-and-name.errors | 1 + .../validators/xml-flattened-and-name.smithy | 43 ++++++++++++++++ 7 files changed, 126 insertions(+) create mode 100644 smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/XmlFlattenedTraitValidator.java create mode 100644 smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/xml-flattened-and-name.errors create mode 100644 smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/xml-flattened-and-name.smithy diff --git a/smithy-aws-protocol-tests/model/awsQuery/main.smithy b/smithy-aws-protocol-tests/model/awsQuery/main.smithy index 4e2c7c513b4..603f457999d 100644 --- a/smithy-aws-protocol-tests/model/awsQuery/main.smithy +++ b/smithy-aws-protocol-tests/model/awsQuery/main.smithy @@ -1,5 +1,15 @@ $version: "2.0" +metadata suppressions = [ + { + id: "XmlFlattenedTrait" + namespace: "aws.protocoltests.query" + reason: """ + Some tests are for asserting the correct behavior in the case that + xmlFlattened is wrong and trips this validator.""" + } +] + namespace aws.protocoltests.query use aws.api#service diff --git a/smithy-aws-protocol-tests/model/ec2Query/main.smithy b/smithy-aws-protocol-tests/model/ec2Query/main.smithy index c0bcbd9950f..0d84362bba3 100644 --- a/smithy-aws-protocol-tests/model/ec2Query/main.smithy +++ b/smithy-aws-protocol-tests/model/ec2Query/main.smithy @@ -26,6 +26,16 @@ $version: "2.0" +metadata suppressions = [ + { + id: "XmlFlattenedTrait" + namespace: "aws.protocoltests.ec2" + reason: """ + Some tests are for asserting the correct behavior in the case that + xmlFlattened is wrong and trips this validator.""" + } +] + namespace aws.protocoltests.ec2 use aws.api#service diff --git a/smithy-aws-protocol-tests/model/restXml/main.smithy b/smithy-aws-protocol-tests/model/restXml/main.smithy index afe0928bd8d..12eaf180793 100644 --- a/smithy-aws-protocol-tests/model/restXml/main.smithy +++ b/smithy-aws-protocol-tests/model/restXml/main.smithy @@ -1,5 +1,15 @@ $version: "2.0" +metadata suppressions = [ + { + id: "XmlFlattenedTrait" + namespace: "aws.protocoltests.restxml" + reason: """ + Some tests are for asserting the correct behavior in the case that + xmlFlattened is wrong and trips this validator.""" + } +] + namespace aws.protocoltests.restxml use aws.api#service diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/XmlFlattenedTraitValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/XmlFlattenedTraitValidator.java new file mode 100644 index 00000000000..b02b1931089 --- /dev/null +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/XmlFlattenedTraitValidator.java @@ -0,0 +1,51 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +package software.amazon.smithy.model.validation.validators; + +import java.util.ArrayList; +import java.util.List; +import java.util.Optional; +import software.amazon.smithy.model.Model; +import software.amazon.smithy.model.shapes.ListShape; +import software.amazon.smithy.model.shapes.MemberShape; +import software.amazon.smithy.model.shapes.Shape; +import software.amazon.smithy.model.traits.XmlFlattenedTrait; +import software.amazon.smithy.model.traits.XmlNameTrait; +import software.amazon.smithy.model.validation.AbstractValidator; +import software.amazon.smithy.model.validation.ValidationEvent; + +/** + * Validates that xmlFlattened members aren't unintentionally ignoring the + * xmlName of their targets. + */ +public final class XmlFlattenedTraitValidator extends AbstractValidator { + @Override + public List validate(Model model) { + List events = new ArrayList<>(); + for (MemberShape member : model.getMemberShapesWithTrait(XmlFlattenedTrait.class)) { + // Don't emit the event if they're being explicit about the xmlName on this member + if (member.hasTrait(XmlNameTrait.class)) { + continue; + } + + model.getShape(member.getTarget()) + .flatMap(Shape::asListShape) + .flatMap(listShape -> validateMemberTargetingList(member, listShape)) + .ifPresent(events::add); + } + return events; + } + + private Optional validateMemberTargetingList(MemberShape member, ListShape targetList) { + return targetList.getMember().getTrait(XmlNameTrait.class) + .filter(xmlName -> !member.getMemberName().equals(xmlName.getValue())) + .map(xmlName -> warning(member, String.format( + "Member is `@xmlFlattened`, so `@xmlName` of target's member (`%s`) has no effect." + + " The flattened list elements will have the name of this member - `%s`. If this" + + " is unintended, you can add `@xmlName(\"%s\")` to this member.", + targetList.getMember().getId(), member.getMemberName(), xmlName.getValue()))); + } +} 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 a43408752c1..4b76f019461 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 @@ -59,3 +59,4 @@ software.amazon.smithy.model.validation.validators.UnitTypeValidator software.amazon.smithy.model.validation.validators.UnstableTraitValidator software.amazon.smithy.model.validation.validators.XmlNamespaceTraitValidator software.amazon.smithy.model.validation.validators.TraitValidatorsValidator +software.amazon.smithy.model.validation.validators.XmlFlattenedTraitValidator diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/xml-flattened-and-name.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/xml-flattened-and-name.errors new file mode 100644 index 00000000000..a8b044edf7c --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/xml-flattened-and-name.errors @@ -0,0 +1 @@ +[WARNING] smithy.example#Struct$flattenedNoXmlName: Member is `@xmlFlattened`, so `@xmlName` of target's member (`smithy.example#ListWithXmlName$member`) has no effect. The flattened list elements will have the name of this member - `flattenedNoXmlName`. If this is unintended, you can add `@xmlName("customMember")` to this member. | XmlFlattenedTrait diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/xml-flattened-and-name.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/xml-flattened-and-name.smithy new file mode 100644 index 00000000000..ae2080f5627 --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/xml-flattened-and-name.smithy @@ -0,0 +1,43 @@ +$version: "2" + +namespace smithy.example + +structure Struct { + // No event because not flattened + notFlattened: ListWithXmlName + + // Event because flattened and non-matching member name + @xmlFlattened + flattenedNoXmlName: ListWithXmlName + + // No event because the member name matches the xml name + @xmlFlattened + customMember: ListWithXmlName + + // No event because you're being explicit about the name to use + @xmlFlattened + @xmlName("customMember") + flattenedMatchingXmlName: ListWithXmlName + + // No event because you're being explicit about the name to use + @xmlFlattened + @xmlName("Bar") + flattenedNonMatchingXmlName: ListWithXmlName + + // Validator doesn't apply to maps + @xmlFlattened + flattenedMap: MapWithXmlName +} + +list ListWithXmlName { + @xmlName("customMember") + member: String +} + +map MapWithXmlName { + @xmlName("customKey") + key: String + + @xmlName("customValue") + value: String +} From fbd772b2401faa7273a3f750a52e419dc393de8c Mon Sep 17 00:00:00 2001 From: Miles Ziemer Date: Mon, 11 Nov 2024 12:17:12 -0500 Subject: [PATCH 2/2] Address feedback --- .../model/awsQuery/main.smithy | 10 --------- .../model/awsQuery/xml-lists.smithy | 2 ++ .../model/ec2Query/main.smithy | 10 --------- .../model/ec2Query/xml-lists.smithy | 2 ++ .../model/restXml/document-lists.smithy | 5 +++++ .../model/restXml/main.smithy | 10 --------- .../XmlFlattenedTraitValidator.java | 21 +++++++++++-------- 7 files changed, 21 insertions(+), 39 deletions(-) diff --git a/smithy-aws-protocol-tests/model/awsQuery/main.smithy b/smithy-aws-protocol-tests/model/awsQuery/main.smithy index 603f457999d..4e2c7c513b4 100644 --- a/smithy-aws-protocol-tests/model/awsQuery/main.smithy +++ b/smithy-aws-protocol-tests/model/awsQuery/main.smithy @@ -1,15 +1,5 @@ $version: "2.0" -metadata suppressions = [ - { - id: "XmlFlattenedTrait" - namespace: "aws.protocoltests.query" - reason: """ - Some tests are for asserting the correct behavior in the case that - xmlFlattened is wrong and trips this validator.""" - } -] - namespace aws.protocoltests.query use aws.api#service diff --git a/smithy-aws-protocol-tests/model/awsQuery/xml-lists.smithy b/smithy-aws-protocol-tests/model/awsQuery/xml-lists.smithy index be3c8981869..5d61ded7e58 100644 --- a/smithy-aws-protocol-tests/model/awsQuery/xml-lists.smithy +++ b/smithy-aws-protocol-tests/model/awsQuery/xml-lists.smithy @@ -185,8 +185,10 @@ structure XmlListsOutput { @xmlName("renamed") renamedListMembers: RenamedListMembers, + @suppress(["XmlFlattenedTrait"]) @xmlFlattened // The xmlname on the targeted list is ignored, and the member name is used. + // The validation that flags this is suppressed so we can test this behavior. flattenedList: RenamedListMembers, @xmlName("customName") diff --git a/smithy-aws-protocol-tests/model/ec2Query/main.smithy b/smithy-aws-protocol-tests/model/ec2Query/main.smithy index 0d84362bba3..c0bcbd9950f 100644 --- a/smithy-aws-protocol-tests/model/ec2Query/main.smithy +++ b/smithy-aws-protocol-tests/model/ec2Query/main.smithy @@ -26,16 +26,6 @@ $version: "2.0" -metadata suppressions = [ - { - id: "XmlFlattenedTrait" - namespace: "aws.protocoltests.ec2" - reason: """ - Some tests are for asserting the correct behavior in the case that - xmlFlattened is wrong and trips this validator.""" - } -] - namespace aws.protocoltests.ec2 use aws.api#service diff --git a/smithy-aws-protocol-tests/model/ec2Query/xml-lists.smithy b/smithy-aws-protocol-tests/model/ec2Query/xml-lists.smithy index cd8f59f04ee..c68123881d6 100644 --- a/smithy-aws-protocol-tests/model/ec2Query/xml-lists.smithy +++ b/smithy-aws-protocol-tests/model/ec2Query/xml-lists.smithy @@ -183,8 +183,10 @@ structure XmlListsOutput { @xmlName("renamed") renamedListMembers: RenamedListMembers, + @suppress(["XmlFlattenedTrait"]) @xmlFlattened // The xmlname on the targeted list is ignored, and the member name is used. + // The validation that flags this is suppressed so we can test this behavior. flattenedList: RenamedListMembers, @xmlName("customName") diff --git a/smithy-aws-protocol-tests/model/restXml/document-lists.smithy b/smithy-aws-protocol-tests/model/restXml/document-lists.smithy index b2dc28b992d..c5ed12f0b96 100644 --- a/smithy-aws-protocol-tests/model/restXml/document-lists.smithy +++ b/smithy-aws-protocol-tests/model/restXml/document-lists.smithy @@ -352,8 +352,10 @@ structure XmlListsInputOutput { @xmlName("renamed") renamedListMembers: RenamedListMembers, + @suppress(["XmlFlattenedTrait"]) @xmlFlattened // The xmlname on the targeted list is ignored, and the member name is used. + // The validation that flags this is suppressed so we can test this behavior. flattenedList: RenamedListMembers, @xmlName("customName") @@ -376,7 +378,10 @@ structure XmlListsInputOutput { @xmlName("myStructureList") structureList: StructureList, + @suppress(["XmlFlattenedTrait"]) @xmlFlattened + // The xmlname on the targeted list is ignored, and the member name is used. + // The validation that flags this is suppressed so we can test this behavior. flattenedStructureList: StructureList } diff --git a/smithy-aws-protocol-tests/model/restXml/main.smithy b/smithy-aws-protocol-tests/model/restXml/main.smithy index 12eaf180793..afe0928bd8d 100644 --- a/smithy-aws-protocol-tests/model/restXml/main.smithy +++ b/smithy-aws-protocol-tests/model/restXml/main.smithy @@ -1,15 +1,5 @@ $version: "2.0" -metadata suppressions = [ - { - id: "XmlFlattenedTrait" - namespace: "aws.protocoltests.restxml" - reason: """ - Some tests are for asserting the correct behavior in the case that - xmlFlattened is wrong and trips this validator.""" - } -] - namespace aws.protocoltests.restxml use aws.api#service diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/XmlFlattenedTraitValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/XmlFlattenedTraitValidator.java index b02b1931089..6a5dc14e85e 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/XmlFlattenedTraitValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/XmlFlattenedTraitValidator.java @@ -7,7 +7,6 @@ import java.util.ArrayList; import java.util.List; -import java.util.Optional; import software.amazon.smithy.model.Model; import software.amazon.smithy.model.shapes.ListShape; import software.amazon.smithy.model.shapes.MemberShape; @@ -31,21 +30,25 @@ public List validate(Model model) { continue; } - model.getShape(member.getTarget()) - .flatMap(Shape::asListShape) - .flatMap(listShape -> validateMemberTargetingList(member, listShape)) - .ifPresent(events::add); + Shape target = model.expectShape(member.getTarget()); + if (target instanceof ListShape) { + ListShape targetList = (ListShape) target; + validateMemberTargetingList(member, targetList, events); + } } return events; } - private Optional validateMemberTargetingList(MemberShape member, ListShape targetList) { - return targetList.getMember().getTrait(XmlNameTrait.class) - .filter(xmlName -> !member.getMemberName().equals(xmlName.getValue())) - .map(xmlName -> warning(member, String.format( + private void validateMemberTargetingList(MemberShape member, ListShape targetList, List events) { + if (targetList.getMember().hasTrait(XmlNameTrait.class)) { + XmlNameTrait xmlName = targetList.getMember().expectTrait(XmlNameTrait.class); + if (!member.getMemberName().equals(xmlName.getValue())) { + events.add(warning(member, String.format( "Member is `@xmlFlattened`, so `@xmlName` of target's member (`%s`) has no effect." + " The flattened list elements will have the name of this member - `%s`. If this" + " is unintended, you can add `@xmlName(\"%s\")` to this member.", targetList.getMember().getId(), member.getMemberName(), xmlName.getValue()))); + } + } } }