Make clientEndpointDiscoveryTrait error optional

docs/source/1.0/spec/aws/aws-core.rst

@@ -886,8 +886,8 @@ following members:
         operation MUST be bound to the service.
     * - error
       - ``shapeId``
-      - **REQUIRED** An error shape which indicates to a client that an endpoint they are
-        using is no longer valid. This error MUST be bound to any operation marked with
+      - **RECOMMENDED** An error shape which indicates to a client that an endpoint they are
+        using is no longer valid. If present, this error MUST be bound to any operation marked with
         the ``clientDiscoveredEndpoint`` trait that is bound to the service.
 
 The input of the operation targeted by ``operation`` MAY contain none, either,

smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/clientendpointdiscovery/CleanClientDiscoveryTraitTransformer.java

@@ -64,7 +64,7 @@ private Set<Shape> getServicesToUpdate(Model model, Set<ShapeId> removedOperatio
         Set<Shape> result = new HashSet<>();
         for (ServiceShape service : model.getServiceShapesWithTrait(ClientEndpointDiscoveryTrait.class)) {
             ClientEndpointDiscoveryTrait trait = service.expectTrait(ClientEndpointDiscoveryTrait.class);
-            if (removedOperations.contains(trait.getOperation()) || removedErrors.contains(trait.getError())) {
+            if (removedOperations.contains(trait.getOperation())) {
                 ServiceShape.Builder builder = service.toBuilder();
                 builder.removeTrait(ClientEndpointDiscoveryTrait.ID);
                 result.add(builder.build());

smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/clientendpointdiscovery/ClientEndpointDiscoveryIndex.java

@@ -44,16 +44,17 @@ public ClientEndpointDiscoveryIndex(Model model) {
         for (ServiceShape service : model.getServiceShapesWithTrait(ClientEndpointDiscoveryTrait.class)) {
             ClientEndpointDiscoveryTrait trait = service.expectTrait(ClientEndpointDiscoveryTrait.class);
             ShapeId endpointOperationId = trait.getOperation();
-            ShapeId endpointErrorId = trait.getError();
+            Optional<ShapeId> endpointErrorId = trait.getOptionalError();
 
             Optional<OperationShape> endpointOperation = model.getShape(endpointOperationId)
                     .flatMap(Shape::asOperationShape);
-            Optional<StructureShape> endpointError = model.getShape(endpointErrorId)
+            Optional<StructureShape> endpointError = endpointErrorId
+                    .flatMap(model::getShape)
                     .flatMap(Shape::asStructureShape);
 
-            if (endpointOperation.isPresent() && endpointError.isPresent()) {
+            if (endpointOperation.isPresent()) {
                 Map<ShapeId, ClientEndpointDiscoveryInfo> serviceInfo = getOperations(
-                        service, endpointOperation.get(), endpointError.get(), topDownIndex, opIndex);
+                        service, endpointOperation.get(), endpointError.orElse(null), topDownIndex, opIndex);
                 if (!serviceInfo.isEmpty()) {
                     endpointDiscoveryInfo.put(service.getId(), serviceInfo);
                 }

smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/clientendpointdiscovery/ClientEndpointDiscoveryInfo.java

@@ -16,6 +16,7 @@
 package software.amazon.smithy.aws.traits.clientendpointdiscovery;
 
 import java.util.List;
+import java.util.Optional;
 import software.amazon.smithy.model.shapes.MemberShape;
 import software.amazon.smithy.model.shapes.OperationShape;
 import software.amazon.smithy.model.shapes.ServiceShape;
@@ -58,10 +59,18 @@ public OperationShape getDiscoveryOperation() {
         return discoveryOperation;
     }
 
+    /**
+     * Deprecated in favor of {@link ClientEndpointDiscoveryInfo#getOptionalError()}.
+     */
+    @Deprecated
     public StructureShape getError() {
         return error;
     }
 
+    public Optional<StructureShape> getOptionalError() {
+        return Optional.ofNullable(error);
+    }
+
     public List<MemberShape> getDiscoveryIds() {
         return discoveryIds;
     }

smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/clientendpointdiscovery/ClientEndpointDiscoveryTrait.java

@@ -15,6 +15,7 @@
 
 package software.amazon.smithy.aws.traits.clientendpointdiscovery;
 
+import java.util.Optional;
 import software.amazon.smithy.model.node.Node;
 import software.amazon.smithy.model.node.ObjectNode;
 import software.amazon.smithy.model.shapes.ShapeId;
@@ -68,6 +69,14 @@ public ShapeId getOperation() {
      *
      * @return The ShapeId of the invalid endpoint error.
      */
+    public Optional<ShapeId> getOptionalError() {
+        return Optional.ofNullable(error);
+    }
+
+    /**
+     * Deprecated in favor of {@link ClientEndpointDiscoveryTrait#getOptionalError}.
+     */
+    @Deprecated
     public ShapeId getError() {
         return error;
     }
@@ -76,14 +85,14 @@ public ShapeId getError() {
     protected Node createNode() {
         return Node.objectNode()
                 .withMember(OPERATION, Node.from(getOperation().toString()))
-                .withMember(ERROR, Node.from(getError().toString()));
+                .withOptionalMember(ERROR, getOptionalError().map(error -> Node.from(error.toString())));
     }
 
     @Override
     public Builder toBuilder() {
         return new Builder()
                 .operation(getOperation())
-                .error(getError());
+                .error(error);
     }
 
     /** Builder for {@link ClientEndpointDiscoveryTrait}. */
@@ -135,10 +144,9 @@ public Provider() {
         @Override
         public ClientEndpointDiscoveryTrait createTrait(ShapeId target, Node value) {
             ObjectNode objectNode = value.expectObjectNode();
-            return builder()
-                    .operation(objectNode.expectStringMember(OPERATION).expectShapeId())
-                    .error(objectNode.expectStringMember(ERROR).expectShapeId())
-                    .build();
+            Builder builder = builder().operation(objectNode.expectStringMember(OPERATION).expectShapeId());
+            objectNode.getStringMember(ERROR).ifPresent(error -> builder.error(error.expectShapeId()));
+            return builder.build();
         }
     }
 }

smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/clientendpointdiscovery/ClientEndpointDiscoveryValidator.java

@@ -34,6 +34,7 @@
 import software.amazon.smithy.model.shapes.StructureShape;
 import software.amazon.smithy.model.validation.AbstractValidator;
 import software.amazon.smithy.model.validation.ValidationEvent;
+import software.amazon.smithy.utils.ListUtils;
 import software.amazon.smithy.utils.Pair;
 import software.amazon.smithy.utils.SetUtils;
 import software.amazon.smithy.utils.SmithyInternalApi;
@@ -47,25 +48,36 @@ public List<ValidationEvent> validate(Model model) {
         ClientEndpointDiscoveryIndex discoveryIndex = ClientEndpointDiscoveryIndex.of(model);
         OperationIndex opIndex = OperationIndex.of(model);
 
+        List<ValidationEvent> validationEvents = new ArrayList<>();
         Map<ServiceShape, ClientEndpointDiscoveryTrait> endpointDiscoveryServices = new HashMap<>();
         for (ServiceShape service : model.getServiceShapesWithTrait(ClientEndpointDiscoveryTrait.class)) {
-            endpointDiscoveryServices.put(service, service.expectTrait(ClientEndpointDiscoveryTrait.class));
+            ClientEndpointDiscoveryTrait trait = service.expectTrait(ClientEndpointDiscoveryTrait.class);
+            endpointDiscoveryServices.put(service, trait);
+            validationEvents.addAll(validateTrait(service, trait));
         }
 
-        List<ValidationEvent> validationEvents = endpointDiscoveryServices.values().stream()
+        validationEvents.addAll(endpointDiscoveryServices.values().stream()
                 .map(ClientEndpointDiscoveryTrait::getOperation)
                 .map(operation -> model.getShape(operation).flatMap(Shape::asOperationShape))
                 .filter(Optional::isPresent)
                 .map(Optional::get)
                 .flatMap(endpointOperation -> validateEndpointOperation(
                         model, opIndex, endpointOperation).stream())
-                .collect(Collectors.toList());
+                .collect(Collectors.toList()));
 
         validationEvents.addAll(validateServices(discoveryIndex, endpointDiscoveryServices));
         validationEvents.addAll(validateOperations(model, discoveryIndex, endpointDiscoveryServices));
         return validationEvents;
     }
 
+    private List<ValidationEvent> validateTrait(ServiceShape service, ClientEndpointDiscoveryTrait trait) {
+        if (!trait.getOptionalError().isPresent()) {
+            return ListUtils.of(danger(service, trait,
+                    "Services SHOULD define an error which indicates an endpoint is invalid."));
+        }
+        return Collections.emptyList();
+    }
+
     private List<ValidationEvent> validateServices(
             ClientEndpointDiscoveryIndex discoveryIndex,
             Map<ServiceShape, ClientEndpointDiscoveryTrait> endpointDiscoveryServices
@@ -121,14 +133,16 @@ private List<ValidationEvent> validateOperation(OperationShape operation, List<C
         }
 
         return infos.stream()
-                .filter(discoveryInfo -> !operation.getErrors().contains(discoveryInfo.getError().getId()))
+                .filter(discoveryInfo -> discoveryInfo.getOptionalError().isPresent())
+                .filter(discoveryInfo -> !operation.getErrors().contains(
+                        discoveryInfo.getOptionalError().get().getId()))
                 .map(discoveryInfo -> error(operation, String.format(
                         "The operation `%s` is marked with `%s` and is bound to the service "
                                 + "`%s` but does not have the required error `%s`.",
                         operation.getId(),
                         ClientDiscoveredEndpointTrait.ID.toString(),
                         discoveryInfo.getService().getId(),
-                        discoveryInfo.getError().getId()
+                        discoveryInfo.getOptionalError().get().getId()
                 )))
                 .collect(Collectors.toList());
     }

smithy-aws-traits/src/main/resources/META-INF/smithy/aws.api.json

@@ -199,7 +199,6 @@
                             "failWhenMissing": true,
                             "selector": "structure[trait|error]"
                         },
-                        "smithy.api#required": {},
                         "smithy.api#documentation": "Indicates the error that tells clients that the endpoint they are using is no longer valid. This error MUST be bound to any operation bound to the service which is marked with the aws.api#clientDiscoveredEndpoint trait."
                     }
                 }

smithy-aws-traits/src/test/java/software/amazon/smithy/aws/traits/clientendpointdiscovery/CleanClientDiscoveryTraitTransformerTest.java

@@ -69,7 +69,7 @@ public void removesTraitsWhenOperationRemoved() {
     }
 
     @Test
-    public void removesTraitsWhenErrorRemoved() {
+    public void doesntRemoveTraitsWhenErrorRemoved() {
         Model model = Model.assembler()
                 .discoverModels(getClass().getClassLoader())
                 .addImport(getClass().getResource("test-model.json"))
@@ -100,11 +100,10 @@ public void removesTraitsWhenErrorRemoved() {
                 .flatMap(Shape::asMemberShape)
                 .get();
 
-        assertFalse(service.hasTrait(ClientEndpointDiscoveryTrait.class));
-        // discovery is required for this operation, so it keeps the trait
+        assertTrue(service.hasTrait(ClientEndpointDiscoveryTrait.class));
         assertTrue(getOperation.hasTrait(ClientDiscoveredEndpointTrait.class));
-        assertFalse(putOperation.hasTrait(ClientDiscoveredEndpointTrait.class));
-        assertFalse(putId.hasTrait(ClientEndpointDiscoveryIdTrait.class));
+        assertTrue(putOperation.hasTrait(ClientDiscoveredEndpointTrait.class));
+        assertTrue(putId.hasTrait(ClientEndpointDiscoveryIdTrait.class));
     }
 
     @Test

smithy-aws-traits/src/test/java/software/amazon/smithy/aws/traits/clientendpointdiscovery/ClientEndpointDiscoveryIndexTest.java

@@ -45,7 +45,7 @@ public void getsDiscoveryInformation() {
         assertEquals(service, info.getService().getId());
         assertEquals(operation, info.getOperation().getId());
         assertEquals(ShapeId.from("ns.foo#DescribeEndpoints"), info.getDiscoveryOperation().getId());
-        assertEquals(ShapeId.from("ns.foo#InvalidEndpointError"), info.getError().getId());
+        assertEquals(ShapeId.from("ns.foo#InvalidEndpointError"), info.getOptionalError().get().getId());
         assertEquals(1, info.getDiscoveryIds().size());
         assertEquals(ShapeId.from("ns.foo#GetObjectInput$Id"), info.getDiscoveryIds().get(0).getId());
     }

smithy-aws-traits/src/test/java/software/amazon/smithy/aws/traits/clientendpointdiscovery/ClientEndpointDiscoveryTraitTest.java

@@ -37,6 +37,6 @@ public void loadsFromModel() {
         ClientEndpointDiscoveryTrait trait = service.getTrait(ClientEndpointDiscoveryTrait.class).get();
 
         assertEquals(trait.getOperation(), ShapeId.from("ns.foo#DescribeEndpoints"));
-        assertEquals(trait.getError(), ShapeId.from("ns.foo#InvalidEndpointError"));
+        assertEquals(trait.getOptionalError().get(), ShapeId.from("ns.foo#InvalidEndpointError"));
     }
 }

smithy-aws-traits/src/test/resources/software/amazon/smithy/aws/traits/errorfiles/clientendpointdiscovery/endpoint-error.errors

@@ -0,0 +1,2 @@
+[DANGER] ns.foo#FooService: Services SHOULD define an error which indicates an endpoint is invalid. | ClientEndpointDiscovery
+[ERROR] ns.foo#GetObject: The operation `ns.foo#GetObject` is marked with `aws.api#clientDiscoveredEndpoint` and is bound to the service `ns.foo#BarService` but does not have the required error `ns.foo#InvalidEndpointError`. | ClientEndpointDiscovery

smithy-aws-traits/src/test/resources/software/amazon/smithy/aws/traits/errorfiles/clientendpointdiscovery/endpoint-error.smithy

@@ -0,0 +1,90 @@
+$version: "1.0"
+
+namespace ns.foo
+
+use aws.api#clientEndpointDiscovery
+use aws.api#clientDiscoveredEndpoint
+
+// This deliberately doesn't have an endpoint error. This should result in
+// a DANGER.
+@clientEndpointDiscovery(operation: DescribeEndpoints)
+service FooService {
+    version: "2021-06-29",
+    operations: [DescribeEndpoints, GetObject],
+}
+
+// This DOES have an error, but it's not bound to the operations. This should
+// result in an ERROR.
+@clientEndpointDiscovery(
+    operation: DescribeEndpoints,
+    error: InvalidEndpointError,
+)
+service BarService {
+    version: "2021-06-29",
+    operations: [DescribeEndpoints, GetObject],
+}
+
+// This DOES have an error, and it IS bound to the operations. This should
+// not produce any validation events.
+@clientEndpointDiscovery(
+    operation: DescribeEndpoints,
+    error: InvalidEndpointError,
+)
+service BazService {
+    version: "2021-06-29",
+    operations: [DescribeEndpoints, GetObjectWithEndpointError],
+}
+
+operation DescribeEndpoints {
+    input: DescribeEndpointsInput,
+    output: DescribeEndpointsOutput,
+}
+
+structure DescribeEndpointsInput {
+    Operation: String,
+    Identifiers: Identifiers,
+}
+
+map Identifiers {
+    key: String,
+    value: String,
+}
+
+structure DescribeEndpointsOutput {
+    Endpoints: Endpoints,
+}
+
+list Endpoints {
+    member: Endpoint,
+}
+
+structure Endpoint {
+    Address: String,
+    CachePeriodInMinutes: Long,
+}
+
+@clientDiscoveredEndpoint(required: true)
+operation GetObject {
+    input: GetObjectInput,
+    output: GetObjectOutput,
+}
+
+structure GetObjectInput {
+    @required
+    Id: String,
+}
+
+structure GetObjectOutput {
+    Object: Blob,
+}
+
+@clientDiscoveredEndpoint(required: true)
+operation GetObjectWithEndpointError {
+    input: GetObjectInput,
+    output: GetObjectOutput,
+    errors: [InvalidEndpointError],
+}
+
+@error("client")
+@httpError(421)
+structure InvalidEndpointError {}