From b560b9584870e39d87bdf3e2a6dc005a042ed92c Mon Sep 17 00:00:00 2001 From: Pritham Marupaka Date: Thu, 31 Oct 2024 11:52:06 -0400 Subject: [PATCH] Update docs. Review changes --- .../EndpointDefinitionValidator.java | 20 ++++++++++- .../parser/services/EndpointError.java | 35 +++++++------------ .../validator/EndpointDefinitionTest.java | 25 +++++++++++++ conjure.schema.json | 5 ++- docs/spec/conjure_definitions.md | 25 ++++++++----- docs/spec/intermediate_representation.md | 1 + 6 files changed, 79 insertions(+), 32 deletions(-) diff --git a/conjure-core/src/main/java/com/palantir/conjure/defs/validator/EndpointDefinitionValidator.java b/conjure-core/src/main/java/com/palantir/conjure/defs/validator/EndpointDefinitionValidator.java index 641e80336..eab013aa9 100644 --- a/conjure-core/src/main/java/com/palantir/conjure/defs/validator/EndpointDefinitionValidator.java +++ b/conjure-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 { + @Override + public void validate(EndpointDefinition definition) { + Set 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 index 65aca2f9c..cc6231efc 100644 --- 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 { - @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/validator/EndpointDefinitionTest.java b/conjure-core/src/test/java/com/palantir/conjure/defs/validator/EndpointDefinitionTest.java index 0d016a930..8701fd5b4 100644 --- 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.schema.json b/conjure.schema.json index dbb7c432e..c3b54e2f4 100644 --- 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 index 098a40be3..956485753 100644 --- a/docs/spec/conjure_definitions.md +++ b/docs/spec/conjure_definitions.md @@ -342,15 +342,16 @@ A field describing an authentication mechanism. It is a `string` which MUST be o [EndpointDefinition]: #endpointdefinition An object representing an endpoint. An endpoint describes a method, arguments and return type. -Field | Type | Description ----|:---:|--- -http | `string` | **REQUIRED** The operation and path for the endpoint. It MUST follow the shorthand ` `, where `` is one of GET, DELETE, POST, or PUT, and `` 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. +Field | Type | Description +------|:------------------------------------------------------------------------:|--- +http | `string` | **REQUIRED** The operation and path for the endpoint. It MUST follow the shorthand ` `, where `` is one of GET, DELETE, POST, or PUT, and `` 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` → [ArgumentDefinition][] or [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. -tags | Set[`string`] | Set of tags that serves as additional metadata for the endpoint. +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. +tags | Set[`string`] | Set of tags that serves as additional metadata for the endpoint. **Example:** ```yaml @@ -417,6 +418,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 index 4ae691267..66803bfb0 100644 --- 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