Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve validation for http binding protocols #1873

Merged
merged 1 commit into from
Jul 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions docs/source-1.0/spec/aws/aws-restjson1-protocol.rst
Original file line number Diff line number Diff line change
Expand Up @@ -154,9 +154,8 @@ that affect serialization:
- Binds a top-level operation input structure member to a label in
the hostPrefix of an endpoint trait.
* - :ref:`http <http-trait>`
- Configures the HTTP bindings of an operation. An operation that
does not define the ``http`` trait is ineligible for use with
this protocol.
- Configures the HTTP bindings of an operation. An operation bound to a
service with this protocol applied MUST have the ``http`` trait applied.
* - :ref:`httpError <httpError-trait>`
- A ``client`` error has a default status code of ``400``, and a
``server`` error has a default status code of ``500``. The
Expand Down
5 changes: 2 additions & 3 deletions docs/source-1.0/spec/aws/aws-restxml-protocol.rst
Original file line number Diff line number Diff line change
Expand Up @@ -150,9 +150,8 @@ that affect serialization:
- Binds a top-level operation input structure member to a label in
the hostPrefix of an endpoint trait.
* - :ref:`http <http-trait>`
- Configures the HTTP bindings of an operation. An operation that
does not define the ``http`` trait is ineligible for use with
this protocol.
- Configures the HTTP bindings of an operation. An operation bound to a
service with this protocol applied MUST have the ``http`` trait applied.
* - :ref:`httpError <httpError-trait>`
- A ``client`` error has a default status code of ``400``, and a
``server`` error has a default status code of ``500``. The
Expand Down
5 changes: 2 additions & 3 deletions docs/source-2.0/aws/protocols/aws-restjson1-protocol.rst
Original file line number Diff line number Diff line change
Expand Up @@ -146,9 +146,8 @@ that affect serialization:
- Binds a top-level operation input structure member to a label in
the hostPrefix of an endpoint trait.
* - :ref:`http <http-trait>`
- Configures the HTTP bindings of an operation. An operation that
does not define the ``http`` trait is ineligible for use with
this protocol.
- Configures the HTTP bindings of an operation. An operation bound to a
service with this protocol applied MUST have the ``http`` trait applied.
* - :ref:`httpError <httpError-trait>`
- A ``client`` error has a default status code of ``400``, and a
``server`` error has a default status code of ``500``. The
Expand Down
5 changes: 2 additions & 3 deletions docs/source-2.0/aws/protocols/aws-restxml-protocol.rst
Original file line number Diff line number Diff line change
Expand Up @@ -141,9 +141,8 @@ that affect serialization:
- Binds a top-level operation input structure member to a label in
the hostPrefix of an endpoint trait.
* - :ref:`http <http-trait>`
- Configures the HTTP bindings of an operation. An operation that
does not define the ``http`` trait is ineligible for use with
this protocol.
- Configures the HTTP bindings of an operation. An operation bound to a
service with this protocol applied MUST have the ``http`` trait applied.
* - :ref:`httpError <httpError-trait>`
- A ``client`` error has a default status code of ``400``, and a
``server`` error has a default status code of ``500``. The
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,14 @@
import software.amazon.smithy.model.knowledge.TopDownIndex;
import software.amazon.smithy.model.shapes.OperationShape;
import software.amazon.smithy.model.shapes.ServiceShape;
import software.amazon.smithy.model.shapes.ShapeId;
import software.amazon.smithy.model.traits.HttpTrait;
import software.amazon.smithy.model.traits.ProtocolDefinitionTrait;
import software.amazon.smithy.model.validation.AbstractValidator;
import software.amazon.smithy.model.validation.Severity;
import software.amazon.smithy.model.validation.ValidationEvent;
import software.amazon.smithy.utils.ListUtils;
import software.amazon.smithy.utils.Pair;

/**
* Validates that if any operation in a service uses the http trait,
Expand All @@ -50,37 +52,46 @@ public List<ValidationEvent> validate(Model model) {

private List<ValidationEvent> validateService(TopDownIndex topDownIndex, Model model, ServiceShape service) {
Set<OperationShape> operations = topDownIndex.getContainedOperations(service);

ShapeId protocolTraitId = protocolWithBindings(service, model);
if (protocolTraitId != null) {
// Emit ERROR events if any of the protocols for the service support HTTP traits.
String reason = "The `" + protocolTraitId + "` protocol requires all";
return validateOperations(service, operations, Severity.ERROR, reason);
}

// Stop early if there are no bindings at all in the model for any operation.
if (operations.stream().noneMatch(this::hasBindings)) {
return ListUtils.of();
}

Severity severity = determineSeverity(service, model);

return operations.stream()
.filter(shape -> !shape.getTrait(HttpTrait.class).isPresent())
.map(shape -> createEvent(severity, service, shape))
.collect(Collectors.toList());
return validateOperations(service, operations, Severity.WARNING, "One or more");
}

// Only emit ERROR events if any of the protocols for the service support HTTP traits
private Severity determineSeverity(ServiceShape service, Model model) {
if (ServiceIndex.of(model).getProtocols(service).values().stream()
private ShapeId protocolWithBindings(ServiceShape service, Model model) {
return ServiceIndex.of(model).getProtocols(service).values().stream()
.map(t -> model.expectShape(t.toShapeId()))
.map(s -> s.expectTrait(ProtocolDefinitionTrait.class))
.anyMatch(pdt -> pdt.getTraits().contains(HttpTrait.ID))) {
return Severity.ERROR;
}
return Severity.WARNING;
.map(s -> Pair.of(s.getId(), s.expectTrait(ProtocolDefinitionTrait.class)))
.filter(pair -> pair.getRight().getTraits().contains(HttpTrait.ID))
.map(Pair::getLeft)
.findFirst().orElse(null);
}

private boolean hasBindings(OperationShape op) {
return op.getTrait(HttpTrait.class).isPresent();
}

private ValidationEvent createEvent(Severity severity, ServiceShape service, OperationShape operation) {
return createEvent(severity, operation, operation.getSourceLocation(), String.format(
"One or more operations in the `%s` service define the `http` trait, but this "
+ "operation is missing the `http` trait.", service.getId()));
private List<ValidationEvent> validateOperations(
ServiceShape service,
Set<OperationShape> operations,
Severity severity,
String reason
) {
return operations.stream()
.filter(operation -> !operation.getTrait(HttpTrait.class).isPresent())
.map(operation -> createEvent(severity, operation, operation.getSourceLocation(),
String.format("%s operations in the `%s` service define the `http` trait, but this "
+ "operation is missing the `http` trait.", reason, service.getId())))
.collect(Collectors.toList());
}
}
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
[ERROR] ns.foo#MissingBindings1: One or more operations in the `ns.foo#MyService` service define the `http` trait, but this operation is missing the `http` trait. | HttpBindingsMissing
[ERROR] ns.foo#MissingBindings2: One or more operations in the `ns.foo#MyService` service define the `http` trait, but this operation is missing the `http` trait. | HttpBindingsMissing
[ERROR] ns.foo#MissingBindings1: The `ns.foo#restProtocol` protocol requires all operations in the `ns.foo#MyService` service define the `http` trait, but this operation is missing the `http` trait. | HttpBindingsMissing
[ERROR] ns.foo#MissingBindings2: The `ns.foo#restProtocol` protocol requires all operations in the `ns.foo#MyService` service define the `http` trait, but this operation is missing the `http` trait. | HttpBindingsMissing