From 7dac6eed3de762f4506470106bfae659dc8c911b Mon Sep 17 00:00:00 2001 From: Pritham Marupaka Date: Thu, 9 Jan 2025 11:29:20 -0500 Subject: [PATCH] review changes --- .../java/dialogue/serde/ConjureBodySerDe.java | 34 ++++---- .../dialogue/serde/EndpointErrorDecoder.java | 86 +++++++++++-------- .../java/dialogue/serde/ErrorDecoder.java | 15 +++- .../EndpointErrorsConjureBodySerDeTest.java | 16 ++-- .../java/dialogue/serde/ErrorDecoderTest.java | 82 +++++++++--------- .../palantir/dialogue/DeserializerArgs.java | 48 ++++------- 6 files changed, 151 insertions(+), 130 deletions(-) diff --git a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ConjureBodySerDe.java b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ConjureBodySerDe.java index 5b87aa02d..845a0b159 100644 --- a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ConjureBodySerDe.java +++ b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ConjureBodySerDe.java @@ -82,19 +82,18 @@ final class ConjureBodySerDe implements BodySerDe { emptyContainerDeserializer, BinaryEncoding.MARKER, DeserializerArgs.builder() - .withBaseType(BinaryEncoding.MARKER) - .withExpectedResult(BinaryEncoding.MARKER) + .baseType(BinaryEncoding.MARKER) + .success(BinaryEncoding.MARKER) .build()); this.optionalBinaryInputStreamDeserializer = new EncodingDeserializerForEndpointRegistry<>( ImmutableList.of(BinaryEncoding.INSTANCE), emptyContainerDeserializer, BinaryEncoding.OPTIONAL_MARKER, DeserializerArgs.>builder() - .withBaseType(BinaryEncoding.OPTIONAL_MARKER) - .withExpectedResult(BinaryEncoding.OPTIONAL_MARKER) + .baseType(BinaryEncoding.OPTIONAL_MARKER) + .success(BinaryEncoding.OPTIONAL_MARKER) .build()); - this.emptyBodyDeserializer = - new EmptyBodyDeserializer(new EndpointErrorDecoder<>(Collections.emptyMap(), Optional.empty())); + this.emptyBodyDeserializer = new EmptyBodyDeserializer(new EndpointErrorDecoder<>(Collections.emptyMap())); // Class unloading: Not supported, Jackson keeps strong references to the types // it sees: https://github.com/FasterXML/jackson-databind/issues/489 this.serializers = Caffeine.from(cacheSpec) @@ -108,8 +107,8 @@ private EncodingDeserializerForEndpointRegistry buildCacheEntry(TypeMarke emptyContainerDeserializer, typeMarker, DeserializerArgs.builder() - .withBaseType(typeMarker) - .withExpectedResult(typeMarker) + .baseType(typeMarker) + .success(typeMarker) .build()); } @@ -254,26 +253,25 @@ private static final class EncodingDeserializerForEndpointRegistry implements private final TypeMarker token; EncodingDeserializerForEndpointRegistry( - List encodings, + List encodingsSortedByWeight, EmptyContainerDeserializer empty, TypeMarker token, DeserializerArgs deserializersForEndpoint) { - this.encodings = encodings.stream() - .map(encoding -> new EncodingDeserializerContainer<>( - encoding, deserializersForEndpoint.expectedResultType())) + this.encodings = encodingsSortedByWeight.stream() + .map(encoding -> + new EncodingDeserializerContainer<>(encoding, deserializersForEndpoint.successType())) .collect(ImmutableList.toImmutableList()); this.endpointErrorDecoder = new EndpointErrorDecoder<>( deserializersForEndpoint.errorNameToTypeMarker(), - // Errors are expected to be JSON objects. See - // https://palantir.github.io/conjure/#/docs/spec/wire?id=_55-conjure-errors. - encodings.stream() + encodingsSortedByWeight.stream() .filter(encoding -> encoding.supportsContentType("application/json")) - .findAny()); + .findFirst()); this.token = token; this.emptyInstance = Suppliers.memoize(() -> empty.tryGetEmptyInstance(token)); // Encodings are applied to the accept header in the order of preference based on the provided list. - this.acceptValue = - Optional.of(encodings.stream().map(Encoding::getContentType).collect(Collectors.joining(", "))); + this.acceptValue = Optional.of(encodingsSortedByWeight.stream() + .map(Encoding::getContentType) + .collect(Collectors.joining(", "))); } @Override diff --git a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/EndpointErrorDecoder.java b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/EndpointErrorDecoder.java index c93776e6d..e3afa4f67 100644 --- a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/EndpointErrorDecoder.java +++ b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/EndpointErrorDecoder.java @@ -16,9 +16,10 @@ package com.palantir.conjure.java.dialogue.serde; -import com.fasterxml.jackson.databind.JsonNode; -import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.annotation.JsonProperty; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Maps; import com.google.common.io.CharStreams; import com.google.common.net.HttpHeaders; import com.google.common.primitives.Longs; @@ -30,7 +31,6 @@ import com.palantir.conjure.java.api.errors.SerializableError; import com.palantir.conjure.java.api.errors.UnknownRemoteException; import com.palantir.conjure.java.dialogue.serde.Encoding.Deserializer; -import com.palantir.conjure.java.serialization.ObjectMappers; import com.palantir.dialogue.Response; import com.palantir.dialogue.TypeMarker; import com.palantir.logsafe.Arg; @@ -49,11 +49,10 @@ import java.net.URL; import java.nio.charset.StandardCharsets; import java.time.Duration; -import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Optional; -import java.util.stream.Collectors; +import javax.annotation.Nullable; /** * Extracts the error from a {@link Response}. @@ -65,17 +64,23 @@ */ final class EndpointErrorDecoder { private static final SafeLogger log = SafeLoggerFactory.get(EndpointErrorDecoder.class); - private static final ObjectMapper MAPPER = ObjectMappers.newClientObjectMapper(); + // Errors are currently expected to be JSON objects. See + // https://palantir.github.io/conjure/#/docs/spec/wire?id=_55-conjure-errors. As there is greater adoption of + // endpoint associated errors and larger parameters, we may be interested in supporting SMILE/CBOR for more + // performant handling of larger paramater payloads. + private static final Encoding JSON_ENCODING = Encodings.json(); + private static final Deserializer NAMED_ERROR_DESERIALIZER = + JSON_ENCODING.deserializer(new TypeMarker<>() {}); private final Map> errorNameToJsonDeserializerMap; + EndpointErrorDecoder(Map> errorNameToTypeMap) { + this(errorNameToTypeMap, Optional.empty()); + } + EndpointErrorDecoder( Map> errorNameToTypeMap, Optional maybeJsonEncoding) { - this.errorNameToJsonDeserializerMap = maybeJsonEncoding - .>>map( - jsonEncoding -> errorNameToTypeMap.entrySet().stream() - .collect(Collectors.toMap( - Map.Entry::getKey, entry -> jsonEncoding.deserializer(entry.getValue())))) - .orElseGet(Collections::emptyMap); + this.errorNameToJsonDeserializerMap = ImmutableMap.copyOf( + Maps.transformValues(errorNameToTypeMap, maybeJsonEncoding.orElse(JSON_ENCODING)::deserializer)); } public boolean isError(Response response) { @@ -129,15 +134,28 @@ Optional checkCode(Response response) { return Optional.empty(); } + private @Nullable String extractErrorName(byte[] body) { + try { + NamedError namedError = NAMED_ERROR_DESERIALIZER.deserialize(new ByteArrayInputStream(body)); + if (namedError == null) { + return null; + } + return namedError.errorName(); + } catch (IOException | RuntimeException e) { + return null; + } + } + private T decodeInternal(Response response) { Optional maybeQosException = checkCode(response); if (maybeQosException.isPresent()) { throw maybeQosException.get(); } int code = response.code(); - String body; + + byte[] body; try { - body = toString(response.body()); + body = toByteArray(response.body()); } catch (NullPointerException | IOException e) { UnknownRemoteException exception = new UnknownRemoteException(code, ""); exception.initCause(e); @@ -145,41 +163,39 @@ private T decodeInternal(Response response) { } Optional contentType = response.getFirstHeader(HttpHeaders.CONTENT_TYPE); - String jsonContentType = "application/json"; - if (contentType.isPresent() && Encodings.matchesContentType(jsonContentType, contentType.get())) { + if (contentType.isPresent() + && Encodings.matchesContentType(JSON_ENCODING.getContentType(), contentType.get())) { try { - JsonNode node = MAPPER.readTree(body); - JsonNode errorNameNode = node.get("errorName"); - if (errorNameNode == null) { - throwRemoteException(body, code); + String errorName = extractErrorName(body); + if (errorName == null) { + throw createRemoteException(body, code); } - Optional> maybeDeserializer = - Optional.ofNullable(errorNameToJsonDeserializerMap.get(errorNameNode.asText())); - if (maybeDeserializer.isEmpty()) { - throwRemoteException(body, code); + Deserializer deserializer = errorNameToJsonDeserializerMap.get(errorName); + if (deserializer == null) { + throw createRemoteException(body, code); } - return maybeDeserializer - .get() - .deserialize(new ByteArrayInputStream(body.getBytes(StandardCharsets.UTF_8))); + return deserializer.deserialize(new ByteArrayInputStream(body)); } catch (RemoteException remoteException) { // rethrow the created remote exception throw remoteException; } catch (Exception e) { - throw new UnknownRemoteException(code, body); + throw new UnknownRemoteException(code, new String(body, StandardCharsets.UTF_8)); } } - throw new UnknownRemoteException(code, body); + throw new UnknownRemoteException(code, new String(body, StandardCharsets.UTF_8)); } - private static void throwRemoteException(String body, int code) throws IOException { - SerializableError serializableError = MAPPER.readValue(body, SerializableError.class); - throw new RemoteException(serializableError, code); + private static RemoteException createRemoteException(byte[] body, int code) throws IOException { + SerializableError serializableError = JSON_ENCODING + .deserializer(new TypeMarker() {}) + .deserialize(new ByteArrayInputStream(body)); + return new RemoteException(serializableError, code); } - static String toString(InputStream body) throws IOException { + private static byte[] toByteArray(InputStream body) throws IOException { try (Reader reader = new InputStreamReader(body, StandardCharsets.UTF_8)) { - return CharStreams.toString(reader); + return CharStreams.toString(reader).getBytes(StandardCharsets.UTF_8); } } @@ -247,4 +263,6 @@ public Optional getFirstHeader(Response response, String headerName) { return response.getFirstHeader(headerName); } } + + record NamedError(@JsonProperty("errorName") String errorName) {} } diff --git a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoder.java b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoder.java index 127f9d0ad..91aa488bd 100644 --- a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoder.java +++ b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoder.java @@ -17,6 +17,7 @@ package com.palantir.conjure.java.dialogue.serde; import com.fasterxml.jackson.databind.ObjectMapper; +import com.google.common.io.CharStreams; import com.google.common.net.HttpHeaders; import com.palantir.conjure.java.api.errors.RemoteException; import com.palantir.conjure.java.api.errors.SerializableError; @@ -26,6 +27,10 @@ import com.palantir.logsafe.logger.SafeLogger; import com.palantir.logsafe.logger.SafeLoggerFactory; import java.io.IOException; +import java.io.InputStream; +import java.io.InputStreamReader; +import java.io.Reader; +import java.nio.charset.StandardCharsets; import java.util.Collections; import java.util.Optional; @@ -41,7 +46,7 @@ public enum ErrorDecoder { private static final SafeLogger log = SafeLoggerFactory.get(ErrorDecoder.class); private static final ObjectMapper MAPPER = ObjectMappers.newClientObjectMapper(); private static final EndpointErrorDecoder ENDPOINT_ERROR_DECODER = - new EndpointErrorDecoder<>(Collections.emptyMap(), Optional.empty()); + new EndpointErrorDecoder<>(Collections.emptyMap()); public boolean isError(Response response) { return ENDPOINT_ERROR_DECODER.isError(response); @@ -68,7 +73,7 @@ private RuntimeException decodeInternal(Response response) { int code = response.code(); String body; try { - body = EndpointErrorDecoder.toString(response.body()); + body = toString(response.body()); } catch (NullPointerException | IOException e) { UnknownRemoteException exception = new UnknownRemoteException(code, ""); exception.initCause(e); @@ -87,4 +92,10 @@ private RuntimeException decodeInternal(Response response) { return new UnknownRemoteException(code, body); } + + private static String toString(InputStream body) throws IOException { + try (Reader reader = new InputStreamReader(body, StandardCharsets.UTF_8)) { + return CharStreams.toString(reader); + } + } } diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/EndpointErrorsConjureBodySerDeTest.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/EndpointErrorsConjureBodySerDeTest.java index a08c84806..cde06f5fa 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/EndpointErrorsConjureBodySerDeTest.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/EndpointErrorsConjureBodySerDeTest.java @@ -116,9 +116,9 @@ public void testDeserializeCustomError() throws IOException { .code(500); BodySerDe serializers = conjureBodySerDe("application/json", "text/plain"); DeserializerArgs deserializerArgs = DeserializerArgs.builder() - .withBaseType(new TypeMarker<>() {}) - .withExpectedResult(new TypeMarker() {}) - .withErrorType("Default:FailedPrecondition", new TypeMarker() {}) + .baseType(new TypeMarker<>() {}) + .success(new TypeMarker() {}) + .error("Default:FailedPrecondition", new TypeMarker() {}) .build(); // When @@ -155,8 +155,8 @@ public void testDeserializingUndefinedErrorFallsbackToSerializableError() throws .code(500); BodySerDe serializers = conjureBodySerDe("application/json", "text/plain"); DeserializerArgs deserializerArgs = DeserializerArgs.builder() - .withBaseType(new TypeMarker<>() {}) - .withExpectedResult(new TypeMarker() {}) + .baseType(new TypeMarker<>() {}) + .success(new TypeMarker() {}) // Note: no error types are registered. .build(); @@ -190,9 +190,9 @@ public void testDeserializeExpectedValue() { .code(200); BodySerDe serializers = conjureBodySerDe("application/json", "text/plain"); DeserializerArgs deserializerArgs = DeserializerArgs.builder() - .withBaseType(new TypeMarker<>() {}) - .withExpectedResult(new TypeMarker() {}) - .withErrorType("Default:FailedPrecondition", new TypeMarker() {}) + .baseType(new TypeMarker<>() {}) + .success(new TypeMarker() {}) + .error("Default:FailedPrecondition", new TypeMarker() {}) .build(); // When EndpointReturnBaseType value = diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoderTest.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoderTest.java index b809ac3a6..b58fdfc33 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoderTest.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoderTest.java @@ -40,12 +40,11 @@ import com.palantir.logsafe.SafeArg; import java.time.Duration; import java.util.Collections; -import java.util.Optional; import java.util.function.Consumer; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.ValueSource; +import org.junit.jupiter.params.provider.EnumSource; import org.mockito.junit.jupiter.MockitoExtension; @ExtendWith(MockitoExtension.class) @@ -68,13 +67,18 @@ private static String createServiceException(ServiceException exception) { } } + public enum DecoderType { + LEGACY, + ENDPOINT + } + private static final ErrorDecoder decoder = ErrorDecoder.INSTANCE; private static final EndpointErrorDecoder endpointErrorDecoder = - new EndpointErrorDecoder<>(Collections.emptyMap(), Optional.empty()); + new EndpointErrorDecoder<>(Collections.emptyMap()); @ParameterizedTest - @ValueSource(booleans = {true, false}) - public void extractsRemoteExceptionForAllErrorCodes(boolean isLegacyErrorDecoder) { + @EnumSource(DecoderType.class) + public void extractsRemoteExceptionForAllErrorCodes(DecoderType decoderType) { for (int code : ImmutableList.of(300, 400, 404, 500)) { Response response = TestResponse.withBody(SERIALIZED_EXCEPTION).code(code).contentType("application/json"); @@ -100,7 +104,7 @@ public void extractsRemoteExceptionForAllErrorCodes(boolean isLegacyErrorDecoder + ")"); }; - if (isLegacyErrorDecoder) { + if (decoderType == DecoderType.LEGACY) { assertThat(decoder.isError(response)).isTrue(); RuntimeException result = decoder.decode(response); assertThat(result).isInstanceOfSatisfying(RemoteException.class, validationFunction); @@ -114,8 +118,8 @@ public void extractsRemoteExceptionForAllErrorCodes(boolean isLegacyErrorDecoder } @ParameterizedTest - @ValueSource(booleans = {true, false}) - public void testQos503(boolean isLegacyErrorDecoder) { + @EnumSource(DecoderType.class) + public void testQos503(DecoderType decoderType) { Response response = TestResponse.withBody(SERIALIZED_EXCEPTION).code(503); Consumer validationFunction = exception -> { @@ -124,7 +128,7 @@ public void testQos503(boolean isLegacyErrorDecoder) { }); }; - if (isLegacyErrorDecoder) { + if (decoderType == DecoderType.LEGACY) { assertThat(decoder.isError(response)).isTrue(); RuntimeException result = decoder.decode(response); assertThat(result).isInstanceOfSatisfying(RuntimeException.class, validationFunction); @@ -137,8 +141,8 @@ public void testQos503(boolean isLegacyErrorDecoder) { } @ParameterizedTest - @ValueSource(booleans = {true, false}) - public void testQos503WithMetadata(boolean isLegacyErrorDecoder) { + @EnumSource(DecoderType.class) + public void testQos503WithMetadata(DecoderType decoderType) { Response response = TestResponse.withBody(SERIALIZED_EXCEPTION) .code(503) .withHeader("Qos-Retry-Hint", "do-not-retry") @@ -155,7 +159,7 @@ public void testQos503WithMetadata(boolean isLegacyErrorDecoder) { }); }; - if (isLegacyErrorDecoder) { + if (decoderType == DecoderType.LEGACY) { assertThat(decoder.isError(response)).isTrue(); RuntimeException result = decoder.decode(response); assertThat(result).isInstanceOfSatisfying(RuntimeException.class, validationFunction); @@ -168,8 +172,8 @@ public void testQos503WithMetadata(boolean isLegacyErrorDecoder) { } @ParameterizedTest - @ValueSource(booleans = {true, false}) - public void testQos429(boolean isLegacyErrorDecoder) { + @EnumSource(DecoderType.class) + public void testQos429(DecoderType decoderType) { Response response = TestResponse.withBody(SERIALIZED_EXCEPTION).code(429); Consumer validationFunction = exception -> { @@ -179,7 +183,7 @@ public void testQos429(boolean isLegacyErrorDecoder) { }); }; - if (isLegacyErrorDecoder) { + if (decoderType == DecoderType.LEGACY) { assertThat(decoder.isError(response)).isTrue(); RuntimeException result = decoder.decode(response); assertThat(result).isInstanceOfSatisfying(RuntimeException.class, validationFunction); @@ -192,8 +196,8 @@ public void testQos429(boolean isLegacyErrorDecoder) { } @ParameterizedTest - @ValueSource(booleans = {true, false}) - public void testQos429_retryAfter(boolean isLegacyErrorDecoder) { + @EnumSource(DecoderType.class) + public void testQos429_retryAfter(DecoderType decoderType) { Response response = TestResponse.withBody(SERIALIZED_EXCEPTION).code(429).withHeader(HttpHeaders.RETRY_AFTER, "3"); @@ -204,7 +208,7 @@ public void testQos429_retryAfter(boolean isLegacyErrorDecoder) { }); }; - if (isLegacyErrorDecoder) { + if (decoderType == DecoderType.LEGACY) { assertThat(decoder.isError(response)).isTrue(); RuntimeException result = decoder.decode(response); assertThat(result).isInstanceOfSatisfying(RuntimeException.class, validationFunction); @@ -217,8 +221,8 @@ public void testQos429_retryAfter(boolean isLegacyErrorDecoder) { } @ParameterizedTest - @ValueSource(booleans = {true, false}) - public void testQos429_retryAfter_invalid(boolean isLegacyErrorDecoder) { + @EnumSource(DecoderType.class) + public void testQos429_retryAfter_invalid(DecoderType decoderType) { Response response = TestResponse.withBody(SERIALIZED_EXCEPTION).code(429).withHeader(HttpHeaders.RETRY_AFTER, "bad"); @@ -229,7 +233,7 @@ public void testQos429_retryAfter_invalid(boolean isLegacyErrorDecoder) { }); }; - if (isLegacyErrorDecoder) { + if (decoderType == DecoderType.LEGACY) { assertThat(decoder.isError(response)).isTrue(); RuntimeException result = decoder.decode(response); assertThat(result).isInstanceOfSatisfying(RuntimeException.class, validationFunction); @@ -242,8 +246,8 @@ public void testQos429_retryAfter_invalid(boolean isLegacyErrorDecoder) { } @ParameterizedTest - @ValueSource(booleans = {true, false}) - public void testQos308_noLocation(boolean isLegacyErrorDecoder) { + @EnumSource(DecoderType.class) + public void testQos308_noLocation(DecoderType decoderType) { Response response = TestResponse.withBody(SERIALIZED_EXCEPTION).code(308); Consumer validationFunction = exception -> { @@ -252,7 +256,7 @@ public void testQos308_noLocation(boolean isLegacyErrorDecoder) { }); }; - if (isLegacyErrorDecoder) { + if (decoderType == DecoderType.LEGACY) { assertThat(decoder.isError(response)).isTrue(); RuntimeException result = decoder.decode(response); assertThat(result).isInstanceOfSatisfying(RuntimeException.class, validationFunction); @@ -265,8 +269,8 @@ public void testQos308_noLocation(boolean isLegacyErrorDecoder) { } @ParameterizedTest - @ValueSource(booleans = {true, false}) - public void testQos308_invalidLocation(boolean isLegacyErrorDecoder) { + @EnumSource(DecoderType.class) + public void testQos308_invalidLocation(DecoderType decoderType) { Response response = TestResponse.withBody(SERIALIZED_EXCEPTION).code(308).withHeader(HttpHeaders.LOCATION, "invalid"); @@ -276,7 +280,7 @@ public void testQos308_invalidLocation(boolean isLegacyErrorDecoder) { }); }; - if (isLegacyErrorDecoder) { + if (decoderType == DecoderType.LEGACY) { assertThat(decoder.isError(response)).isTrue(); RuntimeException result = decoder.decode(response); assertThat(result).isInstanceOfSatisfying(RuntimeException.class, validationFunction); @@ -289,8 +293,8 @@ public void testQos308_invalidLocation(boolean isLegacyErrorDecoder) { } @ParameterizedTest - @ValueSource(booleans = {true, false}) - public void testQos308(boolean isLegacyErrorDecoder) { + @EnumSource(ErrorDecoderTest.DecoderType.class) + public void testQos308(DecoderType decoderType) { String expectedLocation = "https://localhost"; Response response = TestResponse.withBody(SERIALIZED_EXCEPTION) .code(308) @@ -306,7 +310,7 @@ public void testQos308(boolean isLegacyErrorDecoder) { }); }; - if (isLegacyErrorDecoder) { + if (decoderType == DecoderType.LEGACY) { assertThat(decoder.isError(response)).isTrue(); RuntimeException result = decoder.decode(response); assertThat(result).isInstanceOfSatisfying(RuntimeException.class, validationFunction); @@ -339,8 +343,8 @@ public void cannotDecodeNonJsonMediaTypes() { } @ParameterizedTest - @ValueSource(booleans = {true, false}) - public void doesNotHandleUnparseableBody(boolean isLegacyErrorDecoder) { + @EnumSource(DecoderType.class) + public void doesNotHandleUnparseableBody(DecoderType decoderType) { Response response = TestResponse.withBody("not json").code(500).contentType("application/json/"); Consumer validationFunction = exception -> { @@ -348,7 +352,7 @@ public void doesNotHandleUnparseableBody(boolean isLegacyErrorDecoder) { assertThat(exception.getBody()).isEqualTo("not json"); }; - if (isLegacyErrorDecoder) { + if (decoderType == DecoderType.LEGACY) { RuntimeException result = decoder.decode(response); assertThat(result).isInstanceOfSatisfying(UnknownRemoteException.class, validationFunction); } else { @@ -372,8 +376,8 @@ public void doesNotHandleNullBody() { } @ParameterizedTest - @ValueSource(booleans = {true, false}) - public void handlesUnexpectedJson(boolean isLegacyErrorDecoder) { + @EnumSource(DecoderType.class) + public void handlesUnexpectedJson(DecoderType decoderType) { Response response = TestResponse.withBody("{\"error\":\"some-unknown-json\"}") .code(502) .contentType("application/json"); @@ -383,7 +387,7 @@ public void handlesUnexpectedJson(boolean isLegacyErrorDecoder) { assertThat(expected.getBody()).isEqualTo("{\"error\":\"some-unknown-json\"}"); assertThat(expected.getMessage()).isEqualTo("Response status: 502"); }; - if (isLegacyErrorDecoder) { + if (decoderType == DecoderType.LEGACY) { assertThat(decoder.decode(response)) .isInstanceOfSatisfying(UnknownRemoteException.class, validationFunction); } else { @@ -394,8 +398,8 @@ public void handlesUnexpectedJson(boolean isLegacyErrorDecoder) { } @ParameterizedTest - @ValueSource(booleans = {true, false}) - public void handlesJsonWithEncoding(boolean isLegacyErrorDecoder) { + @EnumSource(DecoderType.class) + public void handlesJsonWithEncoding(DecoderType decoderType) { int code = 500; Response response = TestResponse.withBody(SERIALIZED_EXCEPTION).code(code).contentType("application/json; charset=utf-8"); @@ -408,7 +412,7 @@ public void handlesJsonWithEncoding(boolean isLegacyErrorDecoder) { assertThat(exception.getError().errorName()).isEqualTo(ErrorType.FAILED_PRECONDITION.name()); }; - if (isLegacyErrorDecoder) { + if (decoderType == DecoderType.LEGACY) { assertThat(decoder.decode(response)).isInstanceOfSatisfying(RemoteException.class, validationFunction); } else { assertThatExceptionOfType(RemoteException.class) diff --git a/dialogue-target/src/main/java/com/palantir/dialogue/DeserializerArgs.java b/dialogue-target/src/main/java/com/palantir/dialogue/DeserializerArgs.java index c2690ef79..66891ce1f 100644 --- a/dialogue-target/src/main/java/com/palantir/dialogue/DeserializerArgs.java +++ b/dialogue-target/src/main/java/com/palantir/dialogue/DeserializerArgs.java @@ -21,18 +21,19 @@ import java.util.HashMap; import java.util.Map; import javax.annotation.Nonnull; +import javax.annotation.Nullable; public final class DeserializerArgs { private final TypeMarker baseType; - private final TypeMarker expectedResultType; + private final TypeMarker successType; private final ImmutableMap> errorNameToTypeMarker; private DeserializerArgs( TypeMarker baseType, - TypeMarker expectedResultType, + TypeMarker successType, ImmutableMap> map) { this.baseType = baseType; - this.expectedResultType = expectedResultType; + this.successType = successType; this.errorNameToTypeMarker = map; } @@ -42,64 +43,53 @@ public static Builder builder() { public static final class Builder { private boolean buildInvoked = false; - private TypeMarker baseType; - private boolean baseTypeSet = false; - private TypeMarker expectedResultType; - private boolean expectedResultSet = false; + private @Nullable TypeMarker baseType; + private @Nullable TypeMarker successType; private final Map> errorNameToTypeMarker; - @SuppressWarnings("NullAway") - // We ensure that the baseType and expectedResultType are set before building. private Builder() { this.errorNameToTypeMarker = new HashMap<>(); } - public Builder withBaseType(@Nonnull TypeMarker base) { + public Builder baseType(@Nonnull TypeMarker baseT) { checkNotBuilt(); - this.baseType = Preconditions.checkNotNull(base, "base type must be non-null"); - this.baseTypeSet = true; + this.baseType = Preconditions.checkNotNull(baseT, "base type must be non-null"); return this; } - public Builder withExpectedResult(TypeMarker expectedResultT) { + public Builder success(@Nonnull TypeMarker successT) { checkNotBuilt(); - this.expectedResultType = - Preconditions.checkNotNull(expectedResultT, "expected result type must be non-null"); - this.expectedResultSet = true; + this.successType = Preconditions.checkNotNull(successT, "success type must be non-null"); return this; } - public Builder withErrorType(@Nonnull String errorName, @Nonnull TypeMarker errorType) { + public Builder error(@Nonnull String errorName, @Nonnull TypeMarker errorT) { checkNotBuilt(); this.errorNameToTypeMarker.put( Preconditions.checkNotNull(errorName, "error name must be non-null"), - Preconditions.checkNotNull(errorType, "error type must be non-null")); + Preconditions.checkNotNull(errorT, "error type must be non-null")); return this; } public DeserializerArgs build() { checkNotBuilt(); - checkRequiredArgsSet(); + Preconditions.checkNotNull(baseType, "base type must be set"); + Preconditions.checkNotNull(successType, "success type must be set"); buildInvoked = true; - return new DeserializerArgs<>(baseType, expectedResultType, ImmutableMap.copyOf(errorNameToTypeMarker)); + return new DeserializerArgs<>(baseType, successType, ImmutableMap.copyOf(errorNameToTypeMarker)); } private void checkNotBuilt() { - Preconditions.checkState(!buildInvoked, "Build has already been called"); - } - - private void checkRequiredArgsSet() { - Preconditions.checkState(baseTypeSet, "base type must be set"); - Preconditions.checkState(expectedResultSet, "expected result type must be set"); + Preconditions.checkState(!buildInvoked, "build has already been called"); } } - public TypeMarker baseType() { + public TypeMarker baseType() { return baseType; } - public TypeMarker expectedResultType() { - return expectedResultType; + public TypeMarker successType() { + return successType; } public Map> errorNameToTypeMarker() {