Skip to content

Commit

Permalink
Add warnings for ignored HTTP member bindings
Browse files Browse the repository at this point in the history
This commit adds a new validator that emits a warning when a member
has an HTTP member binding trait applied and its container is referenced
through a relationship where it would be ignored.

Spec clarification of this behavior has been added.
  • Loading branch information
kstich committed Sep 1, 2023
1 parent 1412661 commit 309862e
Show file tree
Hide file tree
Showing 8 changed files with 351 additions and 35 deletions.
75 changes: 41 additions & 34 deletions docs/source-2.0/spec/http-bindings.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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``.

Expand Down Expand Up @@ -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:

Expand Down Expand Up @@ -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 <http-protocol-document-payloads>` 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
Expand Down Expand Up @@ -789,6 +793,12 @@ Serialization rules
as a :ref:`protocol-specific <protocolDefinition-trait>` 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:
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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 <http-protocol-document-payloads>` 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::

Expand Down Expand Up @@ -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 <http-protocol-document-payloads>` 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:
Expand Down Expand Up @@ -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 <http-protocol-document-payloads>`
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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,7 @@ apply IgnoreQueryParamsInResponse @httpResponseTests([

structure IgnoreQueryParamsInResponseOutput {
@httpQuery("baz")
@suppress(["HttpBindingTraitIgnored"])
baz: String
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,6 @@ structure RestApis {
@jsonName("item")
items: ListOfRestApi,

@httpQuery("position")
position: String,
}

Expand Down
1 change: 1 addition & 0 deletions smithy-aws-protocol-tests/model/restXml/http-query.smithy
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,7 @@ apply IgnoreQueryParamsInResponse @httpResponseTests([

structure IgnoreQueryParamsInResponseOutput {
@httpQuery("baz")
@suppress(["HttpBindingTraitIgnored"])
baz: String
}

Expand Down
Original file line number Diff line number Diff line change
@@ -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.
*
* <ul>
* <li>When httpLabel, httpQueryParams, or httpQuery is applied to a member of a shape that
* * is not used as operation inputs.</li>
* <li>When httpResponseCode is applied to a member of a shape that is not used as an
* * operation output.</li>
* <li>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.</li>
* </ul>
*/
public class HttpBindingTraitIgnoredValidator extends AbstractValidator {
private static final List<ShapeId> IGNORED_OUTSIDE_INPUT = ListUtils.of(
HttpLabelTrait.ID,
HttpQueryParamsTrait.ID,
HttpQueryTrait.ID);
private static final List<ShapeId> IGNORED_OUTSIDE_OUTPUT = ListUtils.of(
HttpResponseCodeTrait.ID);
private static final List<ShapeId> HTTP_MEMBER_BINDING_TRAITS = ListUtils.of(
HttpHeaderTrait.ID,
HttpLabelTrait.ID,
HttpPayloadTrait.ID,
HttpPrefixHeadersTrait.ID,
HttpQueryParamsTrait.ID,
HttpQueryTrait.ID,
HttpResponseCodeTrait.ID);

@Override
public List<ValidationEvent> validate(Model model) {
List<ValidationEvent> 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<ShapeId, Trait> traits = new HashMap<>();
for (Map.Entry<ShapeId, Trait> 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<ValidationEvent> checkRelationships(
StructureShape containerShape,
MemberShape memberShape,
Map<ShapeId, Trait> traits,
NeighborProvider reverse
) {
// Prepare which traits need relationship tracking for.
Set<ShapeId> ignoredOutsideInputTraits = new HashSet<>(traits.keySet());
ignoredOutsideInputTraits.retainAll(IGNORED_OUTSIDE_INPUT);
Set<ShapeId> ignoredOutsideOutputTraits = new HashSet<>(traits.keySet());
ignoredOutsideOutputTraits.retainAll(IGNORED_OUTSIDE_OUTPUT);

// Store relationships so we can emit one event per ignored binding.
Map<RelationshipType, List<ShapeId>> ignoredRelationships = new TreeMap<>();
List<ValidationEvent> events = new ArrayList<>();
List<Relationship> 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<ShapeId> mergeShapeIdLists(List<ShapeId> shapeIds1, List<ShapeId> shapeIds2) {
List<ShapeId> shapeIds = new ArrayList<>();
shapeIds.addAll(shapeIds1);
shapeIds.addAll(shapeIds2);
return shapeIds;
}

private ValidationEvent emit(
String type,
MemberShape memberShape,
Map<ShapeId, Trait> traits,
ShapeId traitId,
int checkedRelationshipCount,
Map<RelationshipType, List<ShapeId>> 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<RelationshipType, List<ShapeId>> ignoredRelationships) {
List<String> relationshipTypeBindings = new ArrayList<>();
for (Map.Entry<RelationshipType, List<ShapeId>> ignoredRelationshipType : ignoredRelationships.entrySet()) {
StringBuilder stringBuilder = new StringBuilder(ignoredRelationshipType.getKey().toString()
.toLowerCase(Locale.US).replace("_", " "));
Set<String> 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) + "}";
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading

0 comments on commit 309862e

Please sign in to comment.