diff --git a/docs/source-2.0/spec/http-bindings.rst b/docs/source-2.0/spec/http-bindings.rst index e2d0dafc106..ee106458bdc 100644 --- a/docs/source-2.0/spec/http-bindings.rst +++ b/docs/source-2.0/spec/http-bindings.rst @@ -53,7 +53,7 @@ The ``http`` trait is a structure that supports the following members: * - code - ``integer`` - The HTTP status code of a successful response. Defaults to ``200`` if - not provided. The provided value SHOULD be between 100 and 599, and + not provided. The provided value SHOULD be between 200 and 299, and it MUST be between 100 and 999. Status codes that do not allow a body like 204 and 205 SHOULD bind all output members to locations other than the body of the response. @@ -487,7 +487,8 @@ Trait selector shapes that also have the :ref:`error-trait`. Value type ``integer`` value representing the HTTP response status code - (for example, ``404``). + (for example, ``404``). The provided value SHOULD be between 400 and 499 + for "client" errors or between 500 and 599 for "server" errors. The following example defines an error with an HTTP status code of ``404``. @@ -559,6 +560,12 @@ Conflicts with Carefully consider the maximum allowed length of each member that is bound to an HTTP header. +.. note:: + + ``httpHeader`` is only considered when applied to top-level members of an + operation's input, output, or error structures. This trait has no meaning + in any other context and is simply ignored. + .. _restricted-headers: @@ -695,15 +702,12 @@ Applying the ``httpLabel`` trait to members - However, if the label is greedy, then "/" MUST NOT be percent-encoded because greedy labels are meant to span multiple path segments. -``httpLabel`` is only used on input ------------------------------------ +``httpLabel`` is only used on top-level input +---------------------------------------------- -``httpLabel`` is ignored when resolving the HTTP bindings of an operation's -output or an error. This means that if a structure that contains members -marked with the ``httpLabel`` trait is used as the top-level output structure -of an operation, then those members are sent as part of the -:ref:`protocol-specific document ` sent in -the body of the response. +``httpLabel`` is only considered when applied to top-level members of an +operation's input structure. This trait has no meaning in any other context and +is simply ignored. .. smithy-trait:: smithy.api#httpPayload @@ -789,6 +793,12 @@ Serialization rules as a :ref:`protocol-specific ` value that is sent as the body of the message. +.. note:: + + ``httpPayload`` is only considered when applied to top-level members of an + operation's input, output, or error structures. This trait has no meaning + in any other context and is simply ignored. + .. smithy-trait:: smithy.api#httpPrefixHeaders .. _httpPrefixHeaders-trait: @@ -870,6 +880,12 @@ start with the same prefix provided in ``httpPrefixHeaders`` trait. If ``httpPrefixHeaders`` is set to an empty string, then no other members can be bound to ``headers``. +.. note:: + + ``httpPrefixHeaders`` is only considered when applied to top-level members + of an operation's input, output, or error structures. This trait has no + meaning in any other context and is simply ignored. + .. smithy-trait:: smithy.api#httpQuery .. _httpQuery-trait: @@ -957,15 +973,12 @@ Serialization rules HTTP requests and automatically percent-decoded when deserializing HTTP requests. -``httpQuery`` is only used on input ------------------------------------ +``httpQuery`` is only used on top-level input +---------------------------------------------- -``httpQuery`` is ignored when resolving the HTTP bindings of an operation's -output or an error. This means that if a structure that contains members -marked with the ``httpQuery`` trait is used as the top-level output structure -of an operation, then those members are sent as part of the -:ref:`protocol-specific document ` sent in -the body of the response. +``httpQuery`` is only considered when applied to top-level members of an +operation's input structure. This trait has no meaning in any other context and +is simply ignored. .. note:: @@ -1127,15 +1140,12 @@ A server should deserialize the following input structure: } } -``httpQueryParams`` is only used on input ------------------------------------------ +``httpQueryParams`` is only used on top-level input +---------------------------------------------------- -``httpQueryParams`` is ignored when resolving the HTTP bindings of an operation's -output or an error. This means that if a structure that contains members -marked with the ``httpQueryParams`` trait is used as the top-level output structure -of an operation, then those members are sent as part of the -:ref:`protocol-specific document ` sent in -the body of the response. +``httpQueryParams`` is only considered when applied to top-level members of an +operation's input structure. This trait has no meaning in any other context and +is simply ignored. .. smithy-trait:: smithy.api#httpResponseCode .. _httpResponseCode-trait: @@ -1170,15 +1180,12 @@ different response codes for an operation, like a 200 or 201 for a PUT operation. If this member isn't provided, server implementations MUST default to the `code` set by the :ref:`http-trait`. -``httpResponseCode`` is only used on output -------------------------------------------- +``httpResponseCode`` is only used on top-level output +----------------------------------------------------- -``httpResponseCode`` is ignored when resolving the HTTP bindings of any -structure except an operation's output structure. This means that if a -structure that contains members marked with the ``httpResponseCode`` trait -is not used as an output structure of an operation, then those members are -sent as part of the :ref:`protocol-specific document ` -sent in the body of the request. +``httpResponseCode`` is only considered when applied to top-level members of an +operation's output structure. This trait has no meaning in any other context +and is simply ignored. .. smithy-trait:: smithy.api#cors diff --git a/smithy-aws-protocol-tests/model/restJson1/http-query.smithy b/smithy-aws-protocol-tests/model/restJson1/http-query.smithy index b1996d785c7..dea2c2a7f4e 100644 --- a/smithy-aws-protocol-tests/model/restJson1/http-query.smithy +++ b/smithy-aws-protocol-tests/model/restJson1/http-query.smithy @@ -414,6 +414,7 @@ apply IgnoreQueryParamsInResponse @httpResponseTests([ structure IgnoreQueryParamsInResponseOutput { @httpQuery("baz") + @suppress(["HttpBindingTraitIgnored"]) baz: String } diff --git a/smithy-aws-protocol-tests/model/restJson1/services/apigateway.smithy b/smithy-aws-protocol-tests/model/restJson1/services/apigateway.smithy index e4e729b14be..c8bb75ddf6e 100644 --- a/smithy-aws-protocol-tests/model/restJson1/services/apigateway.smithy +++ b/smithy-aws-protocol-tests/model/restJson1/services/apigateway.smithy @@ -102,7 +102,6 @@ structure RestApis { @jsonName("item") items: ListOfRestApi, - @httpQuery("position") position: String, } diff --git a/smithy-aws-protocol-tests/model/restXml/http-query.smithy b/smithy-aws-protocol-tests/model/restXml/http-query.smithy index 547705432f2..98f57de8ed6 100644 --- a/smithy-aws-protocol-tests/model/restXml/http-query.smithy +++ b/smithy-aws-protocol-tests/model/restXml/http-query.smithy @@ -362,6 +362,7 @@ apply IgnoreQueryParamsInResponse @httpResponseTests([ structure IgnoreQueryParamsInResponseOutput { @httpQuery("baz") + @suppress(["HttpBindingTraitIgnored"]) baz: String } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/HttpBindingTraitIgnoredValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/HttpBindingTraitIgnoredValidator.java new file mode 100644 index 00000000000..78d2243bac3 --- /dev/null +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/HttpBindingTraitIgnoredValidator.java @@ -0,0 +1,211 @@ +/* + * 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.HashMap; +import java.util.HashSet; +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.ShapeId; +import software.amazon.smithy.model.shapes.StructureShape; +import software.amazon.smithy.model.traits.HttpHeaderTrait; +import software.amazon.smithy.model.traits.HttpLabelTrait; +import software.amazon.smithy.model.traits.HttpPayloadTrait; +import software.amazon.smithy.model.traits.HttpPrefixHeadersTrait; +import software.amazon.smithy.model.traits.HttpQueryParamsTrait; +import software.amazon.smithy.model.traits.HttpQueryTrait; +import software.amazon.smithy.model.traits.HttpResponseCodeTrait; +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 HTTP binding trait that will be ignored + * in some contexts to which it is bound. + * + *
    + *
  • When httpLabel, httpQueryParams, or httpQuery is applied to a member of a shape that + * * is not used as operation inputs.
    • + *
  • When httpResponseCode is applied to a member of a shape that is not used as an + * * operation output.
    • + *
  • When any other HTTP member binding trait is applied to a member of a shape that is + * * not used as a top-level operation input, output, or error.
    • + *
+ */ +public class HttpBindingTraitIgnoredValidator extends AbstractValidator { + private static final List IGNORED_OUTSIDE_INPUT = ListUtils.of( + HttpLabelTrait.ID, + HttpQueryParamsTrait.ID, + HttpQueryTrait.ID); + private static final List IGNORED_OUTSIDE_OUTPUT = ListUtils.of( + HttpResponseCodeTrait.ID); + private static final List HTTP_MEMBER_BINDING_TRAITS = ListUtils.of( + HttpHeaderTrait.ID, + HttpLabelTrait.ID, + HttpPayloadTrait.ID, + HttpPrefixHeadersTrait.ID, + HttpQueryParamsTrait.ID, + HttpQueryTrait.ID, + HttpResponseCodeTrait.ID); + + @Override + public List validate(Model model) { + List events = new ArrayList<>(); + NeighborProvider reverse = NeighborProviderIndex.of(model).getReverseProvider(); + for (MemberShape memberShape : model.getMemberShapes()) { + // Gather all traits that are HTTP member binding. + // Keep the trait instance around so that it can be used it later for source location. + Map traits = new HashMap<>(); + for (Map.Entry traitEntry : memberShape.getAllTraits().entrySet()) { + if (HTTP_MEMBER_BINDING_TRAITS.contains(traitEntry.getKey())) { + traits.put(traitEntry.getKey(), traitEntry.getValue()); + } + } + + // The traits set is now the HTTP binding traits that are ignored outside + // the top level of an operation's components. + if (!traits.isEmpty()) { + StructureShape containerShape = model.expectShape(memberShape.getContainer(), StructureShape.class); + + // All relationships and trait possibilities are checked at once to de-duplicate + // several parts of the iteration logic. + events.addAll(checkRelationships(containerShape, memberShape, traits, reverse)); + } + } + return events; + } + + private List checkRelationships( + StructureShape containerShape, + MemberShape memberShape, + Map traits, + NeighborProvider reverse + ) { + // Prepare which traits need relationship tracking for. + Set ignoredOutsideInputTraits = new HashSet<>(traits.keySet()); + ignoredOutsideInputTraits.retainAll(IGNORED_OUTSIDE_INPUT); + Set ignoredOutsideOutputTraits = new HashSet<>(traits.keySet()); + ignoredOutsideOutputTraits.retainAll(IGNORED_OUTSIDE_OUTPUT); + + // 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; + } + + // Track if we've got a non-input relationship and a trait that's ignored outside input. + // Continue so we don't emit a duplicate for non-top-level. + if (relationship.getRelationshipType() != RelationshipType.INPUT + && !ignoredOutsideInputTraits.isEmpty() + ) { + ignoredRelationships.merge(relationship.getRelationshipType(), + ListUtils.of(relationship.getShape().getId()), this::mergeShapeIdLists); + continue; + } + + // Track if we've got a non-output relationship and a trait that's ignored outside output. + // Continue so we don't emit a duplicate for non-top-level. + if (relationship.getRelationshipType() != RelationshipType.OUTPUT + && !ignoredOutsideOutputTraits.isEmpty() + ) { + ignoredRelationships.merge(relationship.getRelationshipType(), + ListUtils.of(relationship.getShape().getId()), this::mergeShapeIdLists); + continue; + } + + // Track if there are non-top-level relationship and any HTTP member binding trait. + if (relationship.getRelationshipType() != RelationshipType.INPUT + && relationship.getRelationshipType() != RelationshipType.OUTPUT + && relationship.getRelationshipType() != RelationshipType.ERROR + ) { + ignoredRelationships.merge(relationship.getRelationshipType(), + ListUtils.of(relationship.getShape().getId()), this::mergeShapeIdLists); + } + } + + // If we detected invalid relationships, build the right event message based + // on the ignored traits. All the traits are conflicting on members, so the + // immediate grabbing of next traits is all that's necessary. + if (!ignoredRelationships.isEmpty()) { + if (!ignoredOutsideInputTraits.isEmpty()) { + ShapeId traitId = ignoredOutsideInputTraits.iterator().next(); + events.add(emit("Input", memberShape, traits, traitId, + checkedRelationshipCount, ignoredRelationships)); + + } else if (!ignoredOutsideOutputTraits.isEmpty()) { + ShapeId traitId = ignoredOutsideOutputTraits.iterator().next(); + events.add(emit("Output", memberShape, traits, traitId, + checkedRelationshipCount, ignoredRelationships)); + } else { + // The traits list is always non-empty here, so just grab the first. + ShapeId traitId = traits.keySet().iterator().next(); + events.add(emit("TopLevel", memberShape, traits, traitId, + 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( + String type, + MemberShape memberShape, + Map traits, + ShapeId traitId, + int checkedRelationshipCount, + Map> ignoredRelationships + ) { + String message = "The `%s` trait applied to this member is "; + if (checkedRelationshipCount == ignoredRelationships.size()) { + message += "ignored in all contexts."; + } else { + message += format("ignored in some contexts: %s", formatIgnoredRelationships(ignoredRelationships)); + } + return warning(memberShape, traits.get(traitId), format(message, Trait.getIdiomaticTraitName(traitId)), type); + } + + 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 f278bac44bc..1a8691e0879 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 @@ -9,6 +9,7 @@ software.amazon.smithy.model.validation.validators.ExamplesTraitValidator software.amazon.smithy.model.validation.validators.ExclusiveStructureMemberTraitValidator software.amazon.smithy.model.validation.validators.HostLabelTraitValidator software.amazon.smithy.model.validation.validators.HttpApiKeyAuthTraitValidator +software.amazon.smithy.model.validation.validators.HttpBindingTraitIgnoredValidator software.amazon.smithy.model.validation.validators.HttpBindingsMissingValidator software.amazon.smithy.model.validation.validators.HttpHeaderTraitValidator software.amazon.smithy.model.validation.validators.HttpLabelTraitValidator diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/http-binding-trait-ignored.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/http-binding-trait-ignored.errors new file mode 100644 index 00000000000..104a2887ee6 --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/http-binding-trait-ignored.errors @@ -0,0 +1,17 @@ +[ERROR] smithy.example#IgnoredBindingOperationInput$code: Trait `httpResponseCode` cannot be applied to `smithy.example#IgnoredBindingOperationInput$code`. This trait may only be applied to shapes that match the following selector: structure :not([trait|input]) > member :test(> integer) | TraitTarget +[ERROR] smithy.example#NestedBindings: A member of this structure is marked with the `httpPayload` trait, but the following structure members are not explicitly bound to the HTTP message: `code`, `valid` | HttpPayload +[ERROR] smithy.example#NestedBindings: A member of this structure is marked with the `httpPayload` trait, but the following structure members are not explicitly bound to the HTTP message: `string`, `string2`, `stringMap`, `valid` | HttpPayload +[ERROR] smithy.example#NestedBindings$string: This `string` structure member is marked with the `httpLabel` trait, but no corresponding `http` URI label could be found when used as the input of the `smithy.example#NestedBindingOperation` operation. | HttpLabelTrait +[NOTE] smithy.example#IgnoredBindingOperationOutput: Structure member `stringMap` is marked with the `httpQueryParams` trait, and `httpQuery` traits are applied to the following members: `string2`. The service will not be able to disambiguate between query string parameters intended for the `stringMap` member and those explicitly bound to the `httpQuery` members. | HttpQueryParamsTrait +[NOTE] smithy.example#NestedBindings: Structure member `stringMap` is marked with the `httpQueryParams` trait, and `httpQuery` traits are applied to the following members: `string2`. The service will not be able to disambiguate between query string parameters intended for the `stringMap` member and those explicitly bound to the `httpQuery` members. | HttpQueryParamsTrait +[WARNING] smithy.example#IgnoredBindingOperationInput$code: The `httpResponseCode` trait applied to this member is ignored in all contexts. | HttpBindingTraitIgnored.Output +[WARNING] smithy.example#IgnoredBindingOperationOutput$string: The `httpLabel` trait applied to this member is ignored in all contexts. | HttpBindingTraitIgnored.Input +[WARNING] smithy.example#IgnoredBindingOperationOutput$string2: The `httpQuery` trait applied to this member is ignored in all contexts. | HttpBindingTraitIgnored.Input +[WARNING] smithy.example#IgnoredBindingOperationOutput$stringMap: The `httpQueryParams` trait applied to this member is ignored in all contexts. | HttpBindingTraitIgnored.Input +[WARNING] smithy.example#NestedBindings$string: The `httpLabel` trait applied to this member is ignored in some contexts: {member target: [smithy.example#IgnoredBindingOperationInput$nestedBindings, smithy.example#IgnoredBindingOperationOutput$nestedBindings], output: [smithy.example#NestedBindingOperation]} | HttpBindingTraitIgnored.Input +[WARNING] smithy.example#NestedBindings$string2: The `httpQuery` trait applied to this member is ignored in some contexts: {member target: [smithy.example#IgnoredBindingOperationInput$nestedBindings, smithy.example#IgnoredBindingOperationOutput$nestedBindings], output: [smithy.example#NestedBindingOperation]} | HttpBindingTraitIgnored.Input +[WARNING] smithy.example#NestedBindings$stringMap: The `httpQueryParams` trait applied to this member is ignored in some contexts: {member target: [smithy.example#IgnoredBindingOperationInput$nestedBindings, smithy.example#IgnoredBindingOperationOutput$nestedBindings], output: [smithy.example#NestedBindingOperation]} | HttpBindingTraitIgnored.Input +[WARNING] smithy.example#NestedBindings$string3: The `httpHeader` trait applied to this member is ignored in some contexts: {member target: [smithy.example#IgnoredBindingOperationInput$nestedBindings, smithy.example#IgnoredBindingOperationOutput$nestedBindings]} | HttpBindingTraitIgnored.TopLevel +[WARNING] smithy.example#NestedBindings$stringMap2: The `httpPrefixHeaders` trait applied to this member is ignored in some contexts: {member target: [smithy.example#IgnoredBindingOperationInput$nestedBindings, smithy.example#IgnoredBindingOperationOutput$nestedBindings]} | HttpBindingTraitIgnored.TopLevel +[WARNING] smithy.example#NestedBindings$code: The `httpResponseCode` trait applied to this member is ignored in some contexts: {member target: [smithy.example#IgnoredBindingOperationInput$nestedBindings, smithy.example#IgnoredBindingOperationOutput$nestedBindings], input: [smithy.example#NestedBindingOperation]} | HttpBindingTraitIgnored.Output +[WARNING] smithy.example#NestedBindings$payload: The `httpPayload` trait applied to this member is ignored in some contexts: {member target: [smithy.example#IgnoredBindingOperationInput$nestedBindings, smithy.example#IgnoredBindingOperationOutput$nestedBindings]} | HttpBindingTraitIgnored.TopLevel diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/http-binding-trait-ignored.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/http-binding-trait-ignored.smithy new file mode 100644 index 00000000000..1200bad3753 --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/http-binding-trait-ignored.smithy @@ -0,0 +1,79 @@ +$version: "2" + +namespace smithy.example + +service InvalidExample { + version: "2020-12-29", + operations: [ + IgnoredBindingOperation + NestedBindingOperation + ], +} + +@http(method: "POST", uri: "/ignored-binding") +operation IgnoredBindingOperation { + input: IgnoredBindingOperationInput + output: IgnoredBindingOperationOutput +} + +@input +structure IgnoredBindingOperationInput { + @httpResponseCode + code: Integer + + nestedBindings: NestedBindings + valid: String +} + +@output +structure IgnoredBindingOperationOutput { + @httpLabel + @required + string: String + + @httpQuery("query") + string2: String + + @httpQueryParams + stringMap: StringMap + + nestedBindings: NestedBindings + valid: String +} + +@http(method: "POST", uri: "/nested-bindings") +operation NestedBindingOperation { + input: NestedBindings + output: NestedBindings +} + +structure NestedBindings { + @httpLabel + @required + string: String + + @httpQuery("query") + string2: String + + @httpQueryParams + stringMap: StringMap + + @httpHeader("header") + string3: String + + @httpPrefixHeaders("x-headers-") + stringMap2: StringMap + + @httpResponseCode + code: Integer + + @httpPayload + payload: Blob + + valid: String +} + +map StringMap { + key: String + value: String +}