Update docs. Review changes

palantir · Oct 31, 2024 · 7c06435 · 7c06435
1 parent d9e37b3
commit 7c06435
Show file tree

Hide file tree

Showing 8 changed files with 98 additions and 30 deletions.
diff --git a/...e-core/src/main/java/com/palantir/conjure/defs/validator/EndpointDefinitionValidator.java b/...e-core/src/main/java/com/palantir/conjure/defs/validator/EndpointDefinitionValidator.java
@@ -25,6 +25,7 @@
 import com.palantir.conjure.spec.ArgumentDefinition;
 import com.palantir.conjure.spec.ArgumentName;
 import com.palantir.conjure.spec.EndpointDefinition;
+import com.palantir.conjure.spec.EndpointError;
 import com.palantir.conjure.spec.ExternalReference;
 import com.palantir.conjure.spec.HttpMethod;
 import com.palantir.conjure.spec.ListType;
@@ -64,7 +65,8 @@ public enum EndpointDefinitionValidator implements ConjureContextualValidator<En
     NO_OPTIONAL_BINARY_BODY_PARAM_VALIDATOR(new NoOptionalBinaryBodyParamValidator()),
     PARAMETER_NAME(new ParameterNameValidator()),
     PARAM_ID(new ParamIdValidator()),
-    NO_UNSUPPORTED_HTTP_METHOD(new NoUnsupportedHttpMethodValidator());
+    NO_UNSUPPORTED_HTTP_METHOD(new NoUnsupportedHttpMethodValidator()),
+    NO_DUPLICATE_ENDPOINT_ERRORS(new NoDuplicateEndpointErrorsValidation());
 
     private static final Logger log = LoggerFactory.getLogger(EndpointDefinitionValidator.class);
 
@@ -471,6 +473,22 @@ public void validate(EndpointDefinition definition) {
         }
     }
 
+    @com.google.errorprone.annotations.Immutable
+    private static final class NoDuplicateEndpointErrorsValidation implements ConjureValidator<EndpointDefinition> {
+        @Override
+        public void validate(EndpointDefinition definition) {
+            Set<String> endpointErrorNames = new HashSet<>();
+            for (EndpointError endpointErrorDef : definition.getErrors()) {
+                String errorName = endpointErrorDef.getError().getName();
+                Preconditions.checkArgument(
+                        endpointErrorNames.add(errorName),
+                        "Error '%s' is declared multiple times in endpoint '%s'",
+                        errorName,
+                        describe(definition));
+            }
+        }
+    }
+
     private static String describe(EndpointDefinition endpoint) {
         return String.format(
                 "%s{http: %s %s}", endpoint.getEndpointName(), endpoint.getHttpMethod(), endpoint.getHttpPath());

diff --git a/conjure-core/src/main/java/com/palantir/conjure/parser/services/EndpointError.java b/conjure-core/src/main/java/com/palantir/conjure/parser/services/EndpointError.java
@@ -16,19 +16,14 @@
 
 package com.palantir.conjure.parser.services;
 
-import com.fasterxml.jackson.core.JsonParser;
-import com.fasterxml.jackson.databind.DeserializationContext;
-import com.fasterxml.jackson.databind.JsonDeserializer;
-import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.annotation.JsonCreator;
 import com.palantir.conjure.defs.ConjureImmutablesStyle;
-import com.palantir.conjure.parser.services.EndpointError.EndpointErrorDeserializer;
+import com.palantir.conjure.parser.services.ImmutableEndpointError.Json;
 import com.palantir.conjure.parser.types.ConjureType;
 import com.palantir.parsec.ParseException;
-import java.io.IOException;
 import java.util.Optional;
 import org.immutables.value.Value;
 
-@JsonDeserialize(using = EndpointErrorDeserializer.class)
 @Value.Immutable
 @ConjureImmutablesStyle
 public interface EndpointError {
@@ -40,21 +35,17 @@ static EndpointError of(ConjureType errorName) {
         return ImmutableEndpointError.builder().error(errorName).build();
     }
 
-    // TODO(pm): see if there's something we can do with @JsonCreator.
-    final class EndpointErrorDeserializer extends JsonDeserializer<EndpointError> {
-        @Override
-        public EndpointError deserialize(JsonParser parser, DeserializationContext _context) throws IOException {
-
-            String candidate = parser.getValueAsString();
-            if (candidate != null) {
-                try {
-                    return EndpointError.of(ConjureType.fromString(candidate));
-                } catch (ParseException e) {
-                    throw new RuntimeException(e);
-                }
-            }
-
-            return ImmutableEndpointError.fromJson(parser.readValueAs(ImmutableEndpointError.Json.class));
+    @JsonCreator(mode = JsonCreator.Mode.DELEGATING)
+    static EndpointError fromString(String error) {
+        try {
+            return EndpointError.of(ConjureType.fromString(error));
+        } catch (ParseException e) {
+            throw new RuntimeException(e);
         }
     }
+
+    @JsonCreator(mode = JsonCreator.Mode.DELEGATING)
+    static EndpointError fromJson(Json json) {
+        return ImmutableEndpointError.fromJson(json);
+    }
 }
diff --git a/conjure-core/src/test/java/com/palantir/conjure/defs/ConjureDefTest.java b/conjure-core/src/test/java/com/palantir/conjure/defs/ConjureDefTest.java
@@ -23,6 +23,7 @@
 import com.palantir.conjure.exceptions.ConjureRuntimeException;
 import com.palantir.conjure.parser.ConjureParser;
 import com.palantir.conjure.spec.ConjureDefinition;
+import com.palantir.conjure.spec.Documentation;
 import com.palantir.conjure.spec.EndpointError;
 import com.palantir.logsafe.exceptions.SafeIllegalArgumentException;
 import java.io.File;
@@ -110,12 +111,17 @@ public void testEndpointErrorsCanBeImported() {
                     assertThat(serviceDefinition.getEndpoints())
                             .singleElement()
                             .satisfies(endpointDefinition -> assertThat(endpointDefinition.getErrors())
-                                    .extracting(EndpointError::getError)
+                                    // .extracting(EndpointError::getError)
                                     .containsExactlyInAnyOrder(
-                                            // Invalid argument should be from the `test.api` package that was imported.
-                                            com.palantir.conjure.spec.TypeName.of("InvalidArgument", "test.api"),
-                                            com.palantir.conjure.spec.TypeName.of(
-                                                    "Error2", "test.api.with.imported.errors")));
+                                            EndpointError.builder()
+                                                    .error(com.palantir.conjure.spec.TypeName.of(
+                                                            "Error2", "test.api.with.imported.errors"))
+                                                    .build(),
+                                            // The InvalidArgument is imported from the `test.api` package.
+                                            EndpointError.of(
+                                                    com.palantir.conjure.spec.TypeName.of(
+                                                            "InvalidArgument", "test.api"),
+                                                    Documentation.of("Docs for the imported error"))));
                 });
     }
 }
diff --git a/conjure-core/src/test/java/com/palantir/conjure/defs/validator/EndpointDefinitionTest.java b/conjure-core/src/test/java/com/palantir/conjure/defs/validator/EndpointDefinitionTest.java
@@ -26,6 +26,7 @@
 import com.palantir.conjure.spec.BodyParameterType;
 import com.palantir.conjure.spec.Documentation;
 import com.palantir.conjure.spec.EndpointDefinition;
+import com.palantir.conjure.spec.EndpointError;
 import com.palantir.conjure.spec.EndpointName;
 import com.palantir.conjure.spec.HeaderParameterType;
 import com.palantir.conjure.spec.HttpMethod;
@@ -43,6 +44,7 @@
 import com.palantir.conjure.spec.TypeDefinition;
 import com.palantir.conjure.spec.TypeName;
 import com.palantir.conjure.visitor.DealiasingTypeVisitor;
+import java.util.List;
 import java.util.stream.Collectors;
 import org.junit.jupiter.api.Test;
 
@@ -351,4 +353,27 @@ public void testNoUnsupportedHttpMethod() {
                         + HttpMethod.values().stream().map(HttpMethod::toString).collect(Collectors.joining("|"))
                         + "), but received 'UNKNOWN' in endpoint 'test{http: UNKNOWN /}'.");
     }
+
+    @Test
+    public void testDuplicateEndpointErrorsAreInvalid() {
+        TypeName errorType = TypeName.of("Error1", "test.api");
+        EndpointDefinition definition = EndpointDefinition.builder()
+                .endpointName(ENDPOINT_NAME)
+                .httpMethod(HttpMethod.GET)
+                .httpPath(HttpPath.of("/get"))
+                .errors(List.of(
+                        EndpointError.builder()
+                                .error(errorType)
+                                .docs(Documentation.of("docs"))
+                                .build(),
+                        EndpointError.builder()
+                                .error(errorType)
+                                .docs(Documentation.of("different docs but same error"))
+                                .build()))
+                .build();
+
+        assertThatThrownBy(() -> EndpointDefinitionValidator.validateAll(definition, emptyDealiasingVisitor))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessage("Error 'Error1' is declared multiple times in endpoint 'test{http: GET /get}'");
+    }
 }
diff --git a/conjure-core/src/test/resources/example-imported-endpoint-error.yml b/conjure-core/src/test/resources/example-imported-endpoint-error.yml
@@ -20,5 +20,6 @@ services:
         http: GET /string
         returns: string
         errors:
-          - imports.InvalidArgument
+          - error: imports.InvalidArgument
+            docs: Docs for the imported error
           - Error2
diff --git a/conjure.schema.json b/conjure.schema.json
@@ -526,7 +526,10 @@
         "docs": {
           "$ref": "#/definitions/DocString"
         }
-      }
+      },
+      "required": [
+        "error"
+      ]
     },
     "DocString": {
       "type": "string"

diff --git a/docs/spec/conjure_definitions.md b/docs/spec/conjure_definitions.md
@@ -30,6 +30,7 @@ The Conjure compiler requires each file to conform to the [ConjureSourceFile][]
     - [ServiceDefinition][]
       - [AuthDefinition][]
       - [EndpointDefinition][]
+      - [EndpointError][]
       - [ArgumentDefinition][]
       - [ArgumentDefinition.ParamType][]
     - [DocString][]
@@ -278,6 +279,15 @@ safe&#8209;args | Map[`string` &rarr; [FieldDefinition][]&nbsp;or&nbsp;[ConjureT
 unsafe&#8209;args | Map[`string` &rarr; [FieldDefinition][]&nbsp;or&nbsp;[ConjureType][]] | **REQUIRED**. A map from argument names to type names. These arguments are considered unsafe in accordance with the SLS specification. If the value of the field is a `string` it MUST be a type name that exists within the Conjure definition.
 docs | [DocString][] | Documentation for the type. [CommonMark syntax](http://spec.commonmark.org/) MAY be used for rich text representation.
 
+**Example:**
+
+```yaml
+CategoryNotFound:
+  namespace: com.example
+  code: NOT_FOUND
+  safe-args:
+    allCategories: list<CategoryId>
+```
 
 ## ErrorCode
 [ErrorCode]: #errorcode
@@ -347,6 +357,7 @@ Field | Type | Description
 http | `string` | **REQUIRED** The operation and path for the endpoint. It MUST follow the shorthand `<method> <path>`, where `<method>` is one of GET, DELETE, POST, or PUT, and `<path>` is a [PathString][].
 auth | [AuthDefinition][] | The authentication mechanism for the endpoint. Overrides `default-auth` in [ServiceDefinition][].
 returns | [ConjureType][] | The name of the return type of the endpoint. The value MUST be a type name that exists within the Conjure definition. If not specified, then the endpoint does not return a value.
+errors | [EndpointError][] | The errors that this endpoint may return. The errors listed here should be closely tied to problems that uniquely arise from the generating a response to the endpoint, which clients are expected to handle.
 args | Map[`string` &rarr; [ArgumentDefinition][]&nbsp;or&nbsp;[ConjureType][]] | A map between argument names and argument definitions. If the value of the field is a `string` it MUST be a type name that exists within the Conjure definition. Furthermore, if a `string` the argument will default to `auto` [ArgumentDefinition.ParamType][].
 docs | [DocString][] | Documentation for the endpoint. [CommonMark syntax](http://spec.commonmark.org/) MAY be used for rich text representation.
 deprecated | [DocString][] | Documentation for the deprecation of the endpoint. [CommonMark syntax](http://spec.commonmark.org/) MAY be used for rich text representation.
@@ -362,7 +373,11 @@ createRecipe:
       param-type: body
       type: Recipe
   returns: RecipeId
+  errors:
+    - error: CategoryNotFound
+      docs: A category with the provided category ID was not found.
 ```
+See the example in the [ErrorDefinition][] section for the definition of `CategoryNotFound`
 
 ## ArgumentDefinition
 [ArgumentDefinition]: #argumentdefinition
@@ -417,6 +432,14 @@ A field describing the type of an endpoint parameter. It is a `string` which MUS
 - `header`: defined as a header parameter.
 - `query`: defined as a querystring parameter.
 
+## EndpointError
+[EndpointError]: #endpointerror
+A reference to an [ErrorDefinition][] associated with a service endpoint.
+
+Field | Type | Description
+---|:---:|---
+error |  [TypeName][] | **REQUIRED**. A reference to a Conjure-defined [ErrorDefinition][].
+docs |  [DocString][]  | Documentation for the argument. [CommonMark syntax](http://spec.commonmark.org/) MAY be used for rich text representation.
 
 ## DocString
 [DocString]: #docstring

diff --git a/docs/spec/intermediate_representation.md b/docs/spec/intermediate_representation.md
@@ -364,6 +364,7 @@ parameter name. Each argument definition may have a "docs" key containing string
 Each argument definition may also have a "markers" key, consisting of a list of [types](#representation-of-conjure-types)
 that serve as additional metadata for the argument.
 - "returns": a [representation](#representation-of-conjure-types) of the return type of the endpoint
+- "errors": a reference to a Conjure-defined [error type](#errors).
 - "docs": string documentation for the endpoint
 - "deprecated": a string explanation indicating the endpoint is deprecated and why