From d74dea3a758d78278df57c4898d8891ebece0996 Mon Sep 17 00:00:00 2001 From: Pritham Marupaka Date: Tue, 3 Dec 2024 11:59:52 -0500 Subject: [PATCH 01/17] wip --- .palantir/revapi.yml | 6 + .../java/dialogue/serde/ConjureBodySerDe.java | 124 +++++++++- .../dialogue/serde/EndpointErrorDecoder.java | 229 ++++++++++++++++++ .../serde/EndpointErrorTestUtils.java | 100 ++++++++ .../EndpointErrorsConjureBodySerDeTest.java | 182 ++++++++++++++ .../java/dialogue/serde/ErrorDecoderTest.java | 3 +- .../java/com/palantir/dialogue/BodySerDe.java | 2 + .../palantir/dialogue/DeserializerArgs.java | 108 +++++++++ 8 files changed, 751 insertions(+), 3 deletions(-) create mode 100644 dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/EndpointErrorDecoder.java create mode 100644 dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/EndpointErrorTestUtils.java create mode 100644 dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/EndpointErrorsConjureBodySerDeTest.java create mode 100644 dialogue-target/src/main/java/com/palantir/dialogue/DeserializerArgs.java diff --git a/.palantir/revapi.yml b/.palantir/revapi.yml index fe9ee372f..ff2dd1cc5 100644 --- a/.palantir/revapi.yml +++ b/.palantir/revapi.yml @@ -311,3 +311,9 @@ acceptedBreaks: new: "method com.palantir.dialogue.clients.DialogueClients.StickyChannelSession\ \ com.palantir.dialogue.clients.DialogueClients.StickyChannelFactory2::session()" justification: "interface for consumption, not extension" + "4.6.0": + com.palantir.dialogue:dialogue-target: + - code: "java.method.addedToInterface" + new: "method com.palantir.dialogue.Deserializer com.palantir.dialogue.BodySerDe::deserializer(com.palantir.dialogue.DeserializerArgs)" + justification: "Adding a new method to create deserializers in support of endpoint\ + \ associated error deserialization" 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 b738b8529..8e67cddd2 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 @@ -26,6 +26,7 @@ import com.palantir.dialogue.BinaryRequestBody; import com.palantir.dialogue.BodySerDe; import com.palantir.dialogue.Deserializer; +import com.palantir.dialogue.DeserializerArgs; import com.palantir.dialogue.RequestBody; import com.palantir.dialogue.Response; import com.palantir.dialogue.Serializer; @@ -48,6 +49,12 @@ import java.util.function.Supplier; import java.util.stream.Collectors; +/** + * items: + * - we don't want to use `String` for the error identifier. Let's create an `ErrorName` class. + * - re-consider using a map for the deserializersForEndpointBaseType field. is there a more direct way to get this info + */ + /** Package private internal API. */ final class ConjureBodySerDe implements BodySerDe { @@ -58,7 +65,8 @@ final class ConjureBodySerDe implements BodySerDe { private final Deserializer> optionalBinaryInputStreamDeserializer; private final Deserializer emptyBodyDeserializer; private final LoadingCache> serializers; - private final LoadingCache> deserializers; + private final LoadingCache> deserializers; + private final EmptyContainerDeserializer emptyContainerDeserializer; /** * Selects the first (based on input order) of the provided encodings that @@ -74,6 +82,7 @@ final class ConjureBodySerDe implements BodySerDe { this.encodingsSortedByWeight = sortByWeight(encodings); Preconditions.checkArgument(encodings.size() > 0, "At least one Encoding is required"); this.defaultEncoding = encodings.get(0).encoding(); + this.emptyContainerDeserializer = emptyContainerDeserializer; this.binaryInputStreamDeserializer = new EncodingDeserializerRegistry<>( ImmutableList.of(BinaryEncoding.INSTANCE), errorDecoder, @@ -122,6 +131,16 @@ public Deserializer deserializer(TypeMarker token) { return (Deserializer) deserializers.get(token.getType()); } + @Override + @SuppressWarnings("unchecked") + public Deserializer deserializer(DeserializerArgs deserializerArgs) { + return new EncodingDeserializerForEndpointRegistry<>( + encodingsSortedByWeight, + emptyContainerDeserializer, + (TypeMarker) deserializerArgs.baseType(), + deserializerArgs); + } + @Override public Deserializer emptyBodyDeserializer() { return emptyBodyDeserializer; @@ -301,6 +320,106 @@ Encoding.Deserializer getResponseDeserializer(String contentType) { return throwingDeserializer(contentType); } + private Encoding.Deserializer throwingDeserializer(String contentType) { + return input -> { + try { + input.close(); + } catch (RuntimeException | IOException e) { + log.warn("Failed to close InputStream", e); + } + throw new SafeRuntimeException( + "Unsupported Content-Type", + SafeArg.of("received", contentType), + SafeArg.of("supportedEncodings", encodings)); + }; + } + } + + private static final class EncodingDeserializerForEndpointRegistry implements Deserializer { + + private static final SafeLogger log = SafeLoggerFactory.get(EncodingDeserializerForEndpointRegistry.class); + private final ImmutableList> encodings; + private final EndpointErrorDecoder endpointErrorDecoder; + private final Optional acceptValue; + private final Supplier> emptyInstance; + private final TypeMarker token; + + EncodingDeserializerForEndpointRegistry( + List encodings, + EmptyContainerDeserializer empty, + TypeMarker token, + DeserializerArgs deserializersForEndpoint) { + this.encodings = encodings.stream() + .map(encoding -> new EncodingDeserializerContainer<>( + encoding, deserializersForEndpoint.expectedResultType())) + .collect(ImmutableList.toImmutableList()); + this.endpointErrorDecoder = + new EndpointErrorDecoder<>(deserializersForEndpoint.errorNameToTypeMarker(), encodings); + 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(", "))); + } + + @Override + public T deserialize(Response response) { + boolean closeResponse = true; + try { + if (endpointErrorDecoder.isError(response)) { + // TODO(pm): This needs to return T for the new deserializer API, but throw an exception for the old + return endpointErrorDecoder.decode(response); + } else if (response.code() == 204) { + Optional maybeEmptyInstance = emptyInstance.get(); + if (maybeEmptyInstance.isPresent()) { + return maybeEmptyInstance.get(); + } + throw new SafeRuntimeException( + "Unable to deserialize non-optional response type from 204", SafeArg.of("type", token)); + } + + Optional contentType = response.getFirstHeader(HttpHeaders.CONTENT_TYPE); + if (!contentType.isPresent()) { + throw new SafeIllegalArgumentException( + "Response is missing Content-Type header", + SafeArg.of("received", response.headers().keySet())); + } + Encoding.Deserializer deserializer = getResponseDeserializer(contentType.get()); + T deserialized = deserializer.deserialize(response.body()); + // deserializer has taken on responsibility for closing the response body + closeResponse = false; + return deserialized; + } catch (IOException e) { + throw new SafeRuntimeException( + "Failed to deserialize response stream", + e, + SafeArg.of("contentType", response.getFirstHeader(HttpHeaders.CONTENT_TYPE)), + SafeArg.of("type", token)); + } finally { + if (closeResponse) { + response.close(); + } + } + } + + @Override + public Optional accepts() { + return acceptValue; + } + + /** Returns the {@link EncodingDeserializerContainer} to use to deserialize the request body. */ + @SuppressWarnings("ForLoopReplaceableByForEach") + // performance sensitive code avoids iterator allocation + Encoding.Deserializer getResponseDeserializer(String contentType) { + for (int i = 0; i < encodings.size(); i++) { + EncodingDeserializerContainer container = encodings.get(i); + if (container.encoding.supportsContentType(contentType)) { + return container.deserializer; + } + } + return throwingDeserializer(contentType); + } + private Encoding.Deserializer throwingDeserializer(String contentType) { return new Encoding.Deserializer() { @Override @@ -320,7 +439,8 @@ public T deserialize(InputStream input) { } /** Effectively just a pair. */ - private static final class EncodingDeserializerContainer { + // TODO(pm): what does saving the deserializer do for us? + static final class EncodingDeserializerContainer { private final Encoding encoding; private final Encoding.Deserializer deserializer; 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 new file mode 100644 index 000000000..f2ecfdfcc --- /dev/null +++ b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/EndpointErrorDecoder.java @@ -0,0 +1,229 @@ +/* + * (c) Copyright 2024 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.conjure.java.dialogue.serde; + +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.google.common.collect.ImmutableList; +import com.google.common.io.CharStreams; +import com.google.common.net.HttpHeaders; +import com.google.common.primitives.Longs; +import com.palantir.conjure.java.api.errors.QosException; +import com.palantir.conjure.java.api.errors.QosReason; +import com.palantir.conjure.java.api.errors.QosReasons; +import com.palantir.conjure.java.api.errors.QosReasons.QosResponseDecodingAdapter; +import com.palantir.conjure.java.api.errors.RemoteException; +import com.palantir.conjure.java.api.errors.SerializableError; +import com.palantir.conjure.java.api.errors.UnknownRemoteException; +import com.palantir.conjure.java.serialization.ObjectMappers; +import com.palantir.dialogue.Response; +import com.palantir.dialogue.TypeMarker; +import com.palantir.logsafe.Arg; +import com.palantir.logsafe.SafeArg; +import com.palantir.logsafe.SafeLoggable; +import com.palantir.logsafe.UnsafeArg; +import com.palantir.logsafe.exceptions.SafeExceptions; +import com.palantir.logsafe.logger.SafeLogger; +import com.palantir.logsafe.logger.SafeLoggerFactory; +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.io.InputStreamReader; +import java.io.Reader; +import java.net.MalformedURLException; +import java.net.URL; +import java.nio.charset.StandardCharsets; +import java.time.Duration; +import java.util.List; +import java.util.Map; +import java.util.Optional; + +// TODO(pm): public because maybe we need to expose this in the dialogue annotations. What does that do? +// T is the base type of the endpoint response. It's a union of the result type and all of the error types. +public final class EndpointErrorDecoder { + private static final SafeLogger log = SafeLoggerFactory.get(EndpointErrorDecoder.class); + private static final ObjectMapper MAPPER = ObjectMappers.newClientObjectMapper(); + private final Map> errorNameToTypeMap; + private final List encodings; + + public EndpointErrorDecoder(Map> errorNameToTypeMap, List encodings) { + this.errorNameToTypeMap = errorNameToTypeMap; + this.encodings = encodings; + } + + public boolean isError(Response response) { + return 300 <= response.code() && response.code() <= 599; + } + + public T decode(Response response) { + if (log.isDebugEnabled()) { + log.debug("Received an error response", diagnosticArgs(response)); + } + try { + return decodeInternal(response); + } catch (Exception e) { + // TODO(pm): do we want to add the diagnostic information to the result type as well? + e.addSuppressed(diagnostic(response)); + throw e; + } + } + + // performance sensitive code avoids iterator allocation + @SuppressWarnings({"checkstyle:CyclomaticComplexity", "ForLoopReplaceableByForEach"}) + private T decodeInternal(Response response) { + int code = response.code(); + switch (code) { + case 308: + Optional location = response.getFirstHeader(HttpHeaders.LOCATION); + if (location.isPresent()) { + String locationHeader = location.get(); + try { + UnknownRemoteException remoteException = new UnknownRemoteException(code, ""); + remoteException.initCause( + QosException.retryOther(qosReason(response), new URL(locationHeader))); + throw remoteException; + } catch (MalformedURLException e) { + log.error( + "Failed to parse location header for QosException.RetryOther", + UnsafeArg.of("locationHeader", locationHeader), + e); + } + } else { + log.error("Retrieved HTTP status code 308 without Location header, cannot perform " + + "redirect. This appears to be a server-side protocol violation."); + } + break; + case 429: + throw response.getFirstHeader(HttpHeaders.RETRY_AFTER) + .map(Longs::tryParse) + .map(Duration::ofSeconds) + .map(duration -> QosException.throttle(qosReason(response), duration)) + .orElseGet(() -> QosException.throttle(qosReason(response))); + case 503: + throw QosException.unavailable(qosReason(response)); + } + + String body; + try { + body = toString(response.body()); + } catch (NullPointerException | IOException e) { + UnknownRemoteException exception = new UnknownRemoteException(code, ""); + exception.initCause(e); + throw exception; + } + + Optional contentType = response.getFirstHeader(HttpHeaders.CONTENT_TYPE); + // Use a factory: given contentType, create the deserailizer. + // We need Encoding.Deserializer here. That depends on the encoding. + if (contentType.isPresent() && Encodings.matchesContentType("application/json", contentType.get())) { + try { + JsonNode node = MAPPER.readTree(body); + if (node.get("errorName") != null) { + // TODO(pm): Update this to use some struct instead of errorName. + TypeMarker container = Optional.ofNullable( + errorNameToTypeMap.get(node.get("errorName").asText())) + .orElseThrow(); + for (int i = 0; i < encodings.size(); i++) { + Encoding encoding = encodings.get(i); + if (encoding.supportsContentType(contentType.get())) { + return encoding.deserializer(container) + .deserialize(new ByteArrayInputStream(body.getBytes(StandardCharsets.UTF_8))); + } + } + } else { + SerializableError serializableError = MAPPER.readValue(body, SerializableError.class); + throw new RemoteException(serializableError, code); + } + } catch (Exception e) { + throw new UnknownRemoteException(code, body); + } + } + + throw 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); + } + } + + private static ResponseDiagnostic diagnostic(Response response) { + return new ResponseDiagnostic(diagnosticArgs(response)); + } + + private static ImmutableList> diagnosticArgs(Response response) { + ImmutableList.Builder> args = ImmutableList.>builder().add(SafeArg.of("status", response.code())); + recordHeader(HttpHeaders.SERVER, response, args); + recordHeader(HttpHeaders.CONTENT_TYPE, response, args); + recordHeader(HttpHeaders.CONTENT_LENGTH, response, args); + recordHeader(HttpHeaders.CONNECTION, response, args); + recordHeader(HttpHeaders.DATE, response, args); + recordHeader("x-envoy-response-flags", response, args); + recordHeader("x-envoy-response-code-details", response, args); + recordHeader("Response-Flags", response, args); + recordHeader("Response-Code-Details", response, args); + return args.build(); + } + + private static void recordHeader(String header, Response response, ImmutableList.Builder> args) { + response.getFirstHeader(header).ifPresent(server -> args.add(SafeArg.of(header, server))); + } + + private static final class ResponseDiagnostic extends RuntimeException implements SafeLoggable { + + private static final String SAFE_MESSAGE = "Response Diagnostic Information"; + + private final ImmutableList> args; + + ResponseDiagnostic(ImmutableList> args) { + super(SafeExceptions.renderMessage(SAFE_MESSAGE, args.toArray(new Arg[0]))); + this.args = args; + } + + @Override + public String getLogMessage() { + return SAFE_MESSAGE; + } + + @Override + public List> getArgs() { + return args; + } + + @Override + @SuppressWarnings("UnsynchronizedOverridesSynchronized") // nop + public Throwable fillInStackTrace() { + // no-op: stack trace generation is expensive, this type exists + // to simply associate diagnostic information with a failure. + return this; + } + } + + private static QosReason qosReason(Response response) { + return QosReasons.parseFromResponse(response, DialogueQosResponseDecodingAdapter.INSTANCE); + } + + private enum DialogueQosResponseDecodingAdapter implements QosResponseDecodingAdapter { + INSTANCE; + + @Override + public Optional getFirstHeader(Response response, String headerName) { + return response.getFirstHeader(headerName); + } + } +} diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/EndpointErrorTestUtils.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/EndpointErrorTestUtils.java new file mode 100644 index 000000000..69bcb6949 --- /dev/null +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/EndpointErrorTestUtils.java @@ -0,0 +1,100 @@ +/* + * (c) Copyright 2024 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.conjure.java.dialogue.serde; + +import com.fasterxml.jackson.annotation.JsonProperty; +import com.palantir.conjure.java.api.errors.CheckedServiceException; +import com.palantir.dialogue.TypeMarker; +import com.palantir.logsafe.Arg; +import java.util.HashMap; +import java.util.Map; +import java.util.Optional; +import java.util.OptionalDouble; +import java.util.OptionalInt; +import java.util.OptionalLong; + +final class EndpointErrorTestUtils { + private EndpointErrorTestUtils() {} + + record ConjureError( + @JsonProperty("errorCode") String errorCode, + @JsonProperty("errorName") String errorName, + @JsonProperty("errorInstanceId") String errorInstanceId, + @JsonProperty("parameters") Map parameters) { + static ConjureError fromCheckedServiceException(CheckedServiceException exception) { + Map parameters = new HashMap<>(); + for (Arg arg : exception.getArgs()) { + if (shouldIncludeArgInParameters(arg)) { + parameters.put(arg.getName(), arg.getValue()); + } + } + return new ConjureError( + exception.getErrorType().code().name(), + exception.getErrorType().name(), + exception.getErrorInstanceId(), + parameters); + } + + private static boolean shouldIncludeArgInParameters(Arg arg) { + Object obj = arg.getValue(); + return obj != null + && (!(obj instanceof Optional) || ((Optional) obj).isPresent()) + && (!(obj instanceof OptionalInt) || ((OptionalInt) obj).isPresent()) + && (!(obj instanceof OptionalLong) || ((OptionalLong) obj).isPresent()) + && (!(obj instanceof OptionalDouble) || ((OptionalDouble) obj).isPresent()); + } + } + + /** Deserializes requests as the type. */ + public static final class TypeReturningStubEncoding implements Encoding { + + private final String contentType; + + TypeReturningStubEncoding(String contentType) { + this.contentType = contentType; + } + + @Override + public Encoding.Serializer serializer(TypeMarker _type) { + return (_value, _output) -> { + // nop + }; + } + + @Override + public Encoding.Deserializer deserializer(TypeMarker type) { + return input -> { + return (T) Encodings.json().deserializer(type).deserialize(input); + }; + } + + @Override + public String getContentType() { + return contentType; + } + + @Override + public boolean supportsContentType(String input) { + return contentType.equals(input); + } + + @Override + public String toString() { + return "TypeReturningStubEncoding{" + contentType + '}'; + } + } +} 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 new file mode 100644 index 000000000..296bb193d --- /dev/null +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/EndpointErrorsConjureBodySerDeTest.java @@ -0,0 +1,182 @@ +/* + * (c) Copyright 2024 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.conjure.java.dialogue.serde; + +import static org.assertj.core.api.AssertionsForClassTypes.assertThat; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.google.common.collect.ImmutableList; +import com.palantir.conjure.java.api.errors.CheckedServiceException; +import com.palantir.conjure.java.api.errors.ErrorType; +import com.palantir.conjure.java.dialogue.serde.EndpointErrorTestUtils.ConjureError; +import com.palantir.conjure.java.dialogue.serde.EndpointErrorTestUtils.TypeReturningStubEncoding; +import com.palantir.conjure.java.serialization.ObjectMappers; +import com.palantir.dialogue.BodySerDe; +import com.palantir.dialogue.DeserializerArgs; +import com.palantir.dialogue.TestResponse; +import com.palantir.dialogue.TypeMarker; +import com.palantir.logsafe.Preconditions; +import com.palantir.logsafe.Safe; +import com.palantir.logsafe.SafeArg; +import com.palantir.logsafe.Unsafe; +import com.palantir.logsafe.UnsafeArg; +import java.io.IOException; +import java.util.Arrays; +import java.util.Optional; +import javax.annotation.Nullable; +import javax.annotation.processing.Generated; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.junit.jupiter.MockitoExtension; + +@ExtendWith(MockitoExtension.class) +public class EndpointErrorsConjureBodySerDeTest { + private static final ObjectMapper MAPPER = ObjectMappers.newServerObjectMapper(); + private ErrorDecoder errorDecoder = ErrorDecoder.INSTANCE; + + @Generated("by conjure-java") + private sealed interface EndpointReturnBaseType permits StringReturn, ErrorForEndpoint {} + + @Generated("by conjure-java") + record StringReturn(String value) implements EndpointReturnBaseType { + @JsonCreator + public static StringReturn create(String value) { + return new StringReturn(Preconditions.checkArgumentNotNull(value, "value cannot be null")); + } + } + + abstract static class EndpointError { + @Safe + String errorCode; + + @Safe + String errorName; + + @Safe + String errorInstanceId; + + T args; + + EndpointError(String errorCode, String errorName, String errorInstanceId, T args) { + this.errorCode = errorCode; + this.errorName = errorName; + this.errorInstanceId = errorInstanceId; + this.args = args; + } + } + + record ErrorForEndpointArgs( + @JsonProperty("arg") @Safe String arg, + @JsonProperty("unsafeArg") @Unsafe String unsafeArg, + @JsonProperty("complexArg") @Safe ComplexArg complexArg, + @JsonProperty("optionalArg") @Safe Optional optionalArg) {} + + static final class ErrorForEndpoint extends EndpointError implements EndpointReturnBaseType { + @JsonCreator + ErrorForEndpoint( + @JsonProperty("errorCode") String errorCode, + @JsonProperty("errorName") String errorName, + @JsonProperty("errorInstanceId") String errorInstanceId, + @JsonProperty("parameters") ErrorForEndpointArgs args) { + super(errorCode, errorName, errorInstanceId, args); + } + } + + @Generated("by conjure-java") + record ComplexArg(int foo, String bar) {} + + @Generated("by conjure-java") + public static final class TestEndpointError extends CheckedServiceException { + private TestEndpointError( + @Safe String arg, + @Unsafe String unsafeArg, + @Safe ComplexArg complexArg, + @Safe Optional optionalArg, + @Nullable Throwable cause) { + super( + ErrorType.FAILED_PRECONDITION, + cause, + SafeArg.of("arg", arg), + UnsafeArg.of("unsafeArg", unsafeArg), + SafeArg.of("complexArg", complexArg), + SafeArg.of("optionalArg", optionalArg)); + } + } + + @Test + public void testDeserializeCustomErrors() throws IOException { + TestEndpointError errorThrownByEndpoint = + new TestEndpointError("value", "unsafeValue", new ComplexArg(1, "bar"), Optional.of(2), null); + + ErrorForEndpoint expectedErrorForEndpoint = new ErrorForEndpoint( + "FAILED_PRECONDITION", + "Default:FailedPrecondition", + errorThrownByEndpoint.getErrorInstanceId(), + new ErrorForEndpointArgs("value", "unsafeValue", new ComplexArg(1, "bar"), Optional.of(2))); + + String responseBody = + MAPPER.writeValueAsString(ConjureError.fromCheckedServiceException(errorThrownByEndpoint)); + TestResponse response = TestResponse.withBody(responseBody) + .contentType("application/json") + .code(500); + BodySerDe serializers = conjureBodySerDe("application/json", "text/plain"); + DeserializerArgs deserializerArgs = DeserializerArgs.builder() + .withBaseType(new TypeMarker<>() {}) + .withExpectedResult(new TypeMarker() {}) + .withErrorType("Default:FailedPrecondition", new TypeMarker() {}) + .build(); + EndpointErrorsConjureBodySerDeTest.EndpointReturnBaseType value = + serializers.deserializer(deserializerArgs).deserialize(response); + + assertThat(value) + .extracting("errorCode", "errorName", "errorInstanceId", "args") + .containsExactly( + expectedErrorForEndpoint.errorCode, + expectedErrorForEndpoint.errorName, + expectedErrorForEndpoint.errorInstanceId, + expectedErrorForEndpoint.args); + } + + @Test + public void testDeserializeExpectedValue() { + String expectedString = "expectedString"; + TestResponse response = TestResponse.withBody(String.format("\"%s\"", expectedString)) + .contentType("application/json") + .code(200); + BodySerDe serializers = conjureBodySerDe("application/json", "text/plain"); + DeserializerArgs deserializerArgs = DeserializerArgs.builder() + .withBaseType(new TypeMarker<>() {}) + .withExpectedResult(new TypeMarker() {}) + .withErrorType("Default:FailedPrecondition", new TypeMarker() {}) + .build(); + EndpointReturnBaseType value = + serializers.deserializer(deserializerArgs).deserialize(response); + assertThat(value).isEqualTo(new StringReturn(expectedString)); + } + + private ConjureBodySerDe conjureBodySerDe(String... contentTypes) { + return new ConjureBodySerDe( + Arrays.stream(contentTypes) + .map(c -> WeightedEncoding.of(new TypeReturningStubEncoding(c))) + .collect(ImmutableList.toImmutableList()), + errorDecoder, + Encodings.emptyContainerDeserializer(), + DefaultConjureRuntime.DEFAULT_SERDE_CACHE_SPEC); + } +} 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 9f26a7aa6..510914481 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 @@ -54,7 +54,8 @@ public final class ErrorDecoderTest { private static String createServiceException(ServiceException exception) { try { - return SERVER_MAPPER.writeValueAsString(SerializableError.forException(exception)); + String ret = SERVER_MAPPER.writeValueAsString(SerializableError.forException(exception)); + return ret; } catch (JsonProcessingException e) { fail("failed to serialize"); return ""; diff --git a/dialogue-target/src/main/java/com/palantir/dialogue/BodySerDe.java b/dialogue-target/src/main/java/com/palantir/dialogue/BodySerDe.java index 8801f0c44..f7c69d264 100644 --- a/dialogue-target/src/main/java/com/palantir/dialogue/BodySerDe.java +++ b/dialogue-target/src/main/java/com/palantir/dialogue/BodySerDe.java @@ -28,6 +28,8 @@ public interface BodySerDe { /** Creates a {@link Deserializer} for the requested type. Deserializer instances should be reused. */ Deserializer deserializer(TypeMarker type); + Deserializer deserializer(DeserializerArgs deserializerArgs); + /** * Returns a {@link Deserializer} that fails if a non-empty reponse body is presented and returns null otherwise. */ diff --git a/dialogue-target/src/main/java/com/palantir/dialogue/DeserializerArgs.java b/dialogue-target/src/main/java/com/palantir/dialogue/DeserializerArgs.java new file mode 100644 index 000000000..c2690ef79 --- /dev/null +++ b/dialogue-target/src/main/java/com/palantir/dialogue/DeserializerArgs.java @@ -0,0 +1,108 @@ +/* + * (c) Copyright 2024 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.dialogue; + +import com.google.common.collect.ImmutableMap; +import com.palantir.logsafe.Preconditions; +import java.util.HashMap; +import java.util.Map; +import javax.annotation.Nonnull; + +public final class DeserializerArgs { + private final TypeMarker baseType; + private final TypeMarker expectedResultType; + private final ImmutableMap> errorNameToTypeMarker; + + private DeserializerArgs( + TypeMarker baseType, + TypeMarker expectedResultType, + ImmutableMap> map) { + this.baseType = baseType; + this.expectedResultType = expectedResultType; + this.errorNameToTypeMarker = map; + } + + public static Builder builder() { + return new 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 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) { + checkNotBuilt(); + this.baseType = Preconditions.checkNotNull(base, "base type must be non-null"); + this.baseTypeSet = true; + return this; + } + + public Builder withExpectedResult(TypeMarker expectedResultT) { + checkNotBuilt(); + this.expectedResultType = + Preconditions.checkNotNull(expectedResultT, "expected result type must be non-null"); + this.expectedResultSet = true; + return this; + } + + public Builder withErrorType(@Nonnull String errorName, @Nonnull TypeMarker errorType) { + checkNotBuilt(); + this.errorNameToTypeMarker.put( + Preconditions.checkNotNull(errorName, "error name must be non-null"), + Preconditions.checkNotNull(errorType, "error type must be non-null")); + return this; + } + + public DeserializerArgs build() { + checkNotBuilt(); + checkRequiredArgsSet(); + buildInvoked = true; + return new DeserializerArgs<>(baseType, expectedResultType, 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"); + } + } + + public TypeMarker baseType() { + return baseType; + } + + public TypeMarker expectedResultType() { + return expectedResultType; + } + + public Map> errorNameToTypeMarker() { + return errorNameToTypeMarker; + } +} From f00bd5f4f65df3a9e4a01025af9a2cfb93ef060b Mon Sep 17 00:00:00 2001 From: Pritham Marupaka Date: Mon, 6 Jan 2025 11:55:42 -0500 Subject: [PATCH 02/17] wip --- .../java/dialogue/serde/ConjureBodySerDe.java | 173 +++------ .../dialogue/serde/DefaultConjureRuntime.java | 1 - .../dialogue/serde/EndpointErrorDecoder.java | 97 +++-- .../java/dialogue/serde/ErrorDecoder.java | 136 +------ .../dialogue/serde/BinaryEncodingTest.java | 2 - .../dialogue/serde/ConjureBodySerDeTest.java | 5 - .../dialogue/serde/DefaultClientsTest.java | 1 - .../serde/EndpointErrorTestUtils.java | 21 ++ .../EndpointErrorsConjureBodySerDeTest.java | 116 +++--- .../java/dialogue/serde/ErrorDecoderTest.java | 352 +++++++++++++----- 10 files changed, 461 insertions(+), 443 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 8e67cddd2..5b87aa02d 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 @@ -43,18 +43,13 @@ import java.io.OutputStream; import java.lang.reflect.Type; import java.util.ArrayList; +import java.util.Collections; import java.util.Comparator; import java.util.List; import java.util.Optional; import java.util.function.Supplier; import java.util.stream.Collectors; -/** - * items: - * - we don't want to use `String` for the error identifier. Let's create an `ErrorName` class. - * - re-consider using a map for the deserializersForEndpointBaseType field. is there a more direct way to get this info - */ - /** Package private internal API. */ final class ConjureBodySerDe implements BodySerDe { @@ -65,7 +60,7 @@ final class ConjureBodySerDe implements BodySerDe { private final Deserializer> optionalBinaryInputStreamDeserializer; private final Deserializer emptyBodyDeserializer; private final LoadingCache> serializers; - private final LoadingCache> deserializers; + private final LoadingCache> deserializers; private final EmptyContainerDeserializer emptyContainerDeserializer; /** @@ -75,7 +70,6 @@ final class ConjureBodySerDe implements BodySerDe { */ ConjureBodySerDe( List rawEncodings, - ErrorDecoder errorDecoder, EmptyContainerDeserializer emptyContainerDeserializer, CaffeineSpec cacheSpec) { List encodings = decorateEncodings(rawEncodings); @@ -83,24 +77,40 @@ final class ConjureBodySerDe implements BodySerDe { Preconditions.checkArgument(encodings.size() > 0, "At least one Encoding is required"); this.defaultEncoding = encodings.get(0).encoding(); this.emptyContainerDeserializer = emptyContainerDeserializer; - this.binaryInputStreamDeserializer = new EncodingDeserializerRegistry<>( + this.binaryInputStreamDeserializer = new EncodingDeserializerForEndpointRegistry<>( ImmutableList.of(BinaryEncoding.INSTANCE), - errorDecoder, emptyContainerDeserializer, - BinaryEncoding.MARKER); - this.optionalBinaryInputStreamDeserializer = new EncodingDeserializerRegistry<>( + BinaryEncoding.MARKER, + DeserializerArgs.builder() + .withBaseType(BinaryEncoding.MARKER) + .withExpectedResult(BinaryEncoding.MARKER) + .build()); + this.optionalBinaryInputStreamDeserializer = new EncodingDeserializerForEndpointRegistry<>( ImmutableList.of(BinaryEncoding.INSTANCE), - errorDecoder, emptyContainerDeserializer, - BinaryEncoding.OPTIONAL_MARKER); - this.emptyBodyDeserializer = new EmptyBodyDeserializer(errorDecoder); + BinaryEncoding.OPTIONAL_MARKER, + DeserializerArgs.>builder() + .withBaseType(BinaryEncoding.OPTIONAL_MARKER) + .withExpectedResult(BinaryEncoding.OPTIONAL_MARKER) + .build()); + this.emptyBodyDeserializer = + new EmptyBodyDeserializer(new EndpointErrorDecoder<>(Collections.emptyMap(), Optional.empty())); // 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) .build(type -> new EncodingSerializerRegistry<>(defaultEncoding, TypeMarker.of(type))); - this.deserializers = Caffeine.from(cacheSpec) - .build(type -> new EncodingDeserializerRegistry<>( - encodingsSortedByWeight, errorDecoder, emptyContainerDeserializer, TypeMarker.of(type))); + this.deserializers = Caffeine.from(cacheSpec).build(type -> buildCacheEntry(TypeMarker.of(type))); + } + + private EncodingDeserializerForEndpointRegistry buildCacheEntry(TypeMarker typeMarker) { + return new EncodingDeserializerForEndpointRegistry<>( + encodingsSortedByWeight, + emptyContainerDeserializer, + typeMarker, + DeserializerArgs.builder() + .withBaseType(typeMarker) + .withExpectedResult(typeMarker) + .build()); } private static List decorateEncodings(List input) { @@ -235,108 +245,7 @@ private static final class EncodingSerializerContainer { } } - private static final class EncodingDeserializerRegistry implements Deserializer { - - private static final SafeLogger log = SafeLoggerFactory.get(EncodingDeserializerRegistry.class); - private final ImmutableList> encodings; - private final ErrorDecoder errorDecoder; - private final Optional acceptValue; - private final Supplier> emptyInstance; - private final TypeMarker token; - - EncodingDeserializerRegistry( - List encodings, - ErrorDecoder errorDecoder, - EmptyContainerDeserializer empty, - TypeMarker token) { - this.encodings = encodings.stream() - .map(encoding -> new EncodingDeserializerContainer<>(encoding, token)) - .collect(ImmutableList.toImmutableList()); - this.errorDecoder = errorDecoder; - 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(", "))); - } - - @Override - public T deserialize(Response response) { - boolean closeResponse = true; - try { - if (errorDecoder.isError(response)) { - throw errorDecoder.decode(response); - } else if (response.code() == 204) { - // TODO(dfox): what if we get a 204 for a non-optional type??? - // TODO(dfox): support http200 & body=null - // TODO(dfox): what if we were expecting an empty list but got {}? - Optional maybeEmptyInstance = emptyInstance.get(); - if (maybeEmptyInstance.isPresent()) { - return maybeEmptyInstance.get(); - } - throw new SafeRuntimeException( - "Unable to deserialize non-optional response type from 204", SafeArg.of("type", token)); - } - - Optional contentType = response.getFirstHeader(HttpHeaders.CONTENT_TYPE); - if (!contentType.isPresent()) { - throw new SafeIllegalArgumentException( - "Response is missing Content-Type header", - SafeArg.of("received", response.headers().keySet())); - } - Encoding.Deserializer deserializer = getResponseDeserializer(contentType.get()); - T deserialized = deserializer.deserialize(response.body()); - // deserializer has taken on responsibility for closing the response body - closeResponse = false; - return deserialized; - } catch (IOException e) { - throw new SafeRuntimeException( - "Failed to deserialize response stream", - e, - SafeArg.of("contentType", response.getFirstHeader(HttpHeaders.CONTENT_TYPE)), - SafeArg.of("type", token)); - } finally { - if (closeResponse) { - response.close(); - } - } - } - - @Override - public Optional accepts() { - return acceptValue; - } - - /** Returns the {@link EncodingDeserializerContainer} to use to deserialize the request body. */ - @SuppressWarnings("ForLoopReplaceableByForEach") - // performance sensitive code avoids iterator allocation - Encoding.Deserializer getResponseDeserializer(String contentType) { - for (int i = 0; i < encodings.size(); i++) { - EncodingDeserializerContainer container = encodings.get(i); - if (container.encoding.supportsContentType(contentType)) { - return container.deserializer; - } - } - return throwingDeserializer(contentType); - } - - private Encoding.Deserializer throwingDeserializer(String contentType) { - return input -> { - try { - input.close(); - } catch (RuntimeException | IOException e) { - log.warn("Failed to close InputStream", e); - } - throw new SafeRuntimeException( - "Unsupported Content-Type", - SafeArg.of("received", contentType), - SafeArg.of("supportedEncodings", encodings)); - }; - } - } - private static final class EncodingDeserializerForEndpointRegistry implements Deserializer { - private static final SafeLogger log = SafeLoggerFactory.get(EncodingDeserializerForEndpointRegistry.class); private final ImmutableList> encodings; private final EndpointErrorDecoder endpointErrorDecoder; @@ -353,8 +262,13 @@ private static final class EncodingDeserializerForEndpointRegistry implements .map(encoding -> new EncodingDeserializerContainer<>( encoding, deserializersForEndpoint.expectedResultType())) .collect(ImmutableList.toImmutableList()); - this.endpointErrorDecoder = - new EndpointErrorDecoder<>(deserializersForEndpoint.errorNameToTypeMarker(), encodings); + 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() + .filter(encoding -> encoding.supportsContentType("application/json")) + .findAny()); 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. @@ -367,9 +281,11 @@ public T deserialize(Response response) { boolean closeResponse = true; try { if (endpointErrorDecoder.isError(response)) { - // TODO(pm): This needs to return T for the new deserializer API, but throw an exception for the old return endpointErrorDecoder.decode(response); } else if (response.code() == 204) { + // TODO(dfox): what if we get a 204 for a non-optional type??? + // TODO(dfox): support http200 & body=null + // TODO(dfox): what if we were expecting an empty list but got {}? Optional maybeEmptyInstance = emptyInstance.get(); if (maybeEmptyInstance.isPresent()) { return maybeEmptyInstance.get(); @@ -439,8 +355,7 @@ public T deserialize(InputStream input) { } /** Effectively just a pair. */ - // TODO(pm): what does saving the deserializer do for us? - static final class EncodingDeserializerContainer { + private static final class EncodingDeserializerContainer { private final Encoding encoding; private final Encoding.Deserializer deserializer; @@ -457,10 +372,10 @@ public String toString() { } private static final class EmptyBodyDeserializer implements Deserializer { - private final ErrorDecoder errorDecoder; + private final EndpointErrorDecoder endpointErrorDecoder; - EmptyBodyDeserializer(ErrorDecoder errorDecoder) { - this.errorDecoder = errorDecoder; + EmptyBodyDeserializer(EndpointErrorDecoder endpointErrorDecoder) { + this.endpointErrorDecoder = endpointErrorDecoder; } @Override @@ -468,8 +383,8 @@ private static final class EmptyBodyDeserializer implements Deserializer { public Void deserialize(Response response) { // We should not fail if a server that previously returned nothing starts returning a response try (Response unused = response) { - if (errorDecoder.isError(response)) { - throw errorDecoder.decode(response); + if (endpointErrorDecoder.isError(response)) { + endpointErrorDecoder.decode(response); } return null; } diff --git a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/DefaultConjureRuntime.java b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/DefaultConjureRuntime.java index 3e4766fda..befd4c350 100644 --- a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/DefaultConjureRuntime.java +++ b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/DefaultConjureRuntime.java @@ -45,7 +45,6 @@ public final class DefaultConjureRuntime implements ConjureRuntime { private DefaultConjureRuntime(Builder builder) { this.bodySerDe = new ConjureBodySerDe( builder.encodings.isEmpty() ? DEFAULT_ENCODINGS : builder.encodings, - ErrorDecoder.INSTANCE, Encodings.emptyContainerDeserializer(), DEFAULT_SERDE_CACHE_SPEC); } 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 f2ecfdfcc..c93776e6d 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 @@ -29,6 +29,7 @@ import com.palantir.conjure.java.api.errors.RemoteException; 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; @@ -48,21 +49,33 @@ 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; -// TODO(pm): public because maybe we need to expose this in the dialogue annotations. What does that do? -// T is the base type of the endpoint response. It's a union of the result type and all of the error types. -public final class EndpointErrorDecoder { +/** + * Extracts the error from a {@link Response}. + *

If the error's name is in the {@link #errorNameToJsonDeserializerMap}, this class attempts to deserialize the + * {@link Response} body as JSON, to the error type. Otherwise, a {@link RemoteException} is thrown. If the + * {@link Response} does not adhere to the expected format, an {@link UnknownRemoteException} is thrown. + * + * @param the base type of the endpoint response. It's a union of the result type and all the error types. + */ +final class EndpointErrorDecoder { private static final SafeLogger log = SafeLoggerFactory.get(EndpointErrorDecoder.class); private static final ObjectMapper MAPPER = ObjectMappers.newClientObjectMapper(); - private final Map> errorNameToTypeMap; - private final List encodings; - - public EndpointErrorDecoder(Map> errorNameToTypeMap, List encodings) { - this.errorNameToTypeMap = errorNameToTypeMap; - this.encodings = encodings; + private final Map> errorNameToJsonDeserializerMap; + + 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); } public boolean isError(Response response) { @@ -76,15 +89,12 @@ public T decode(Response response) { try { return decodeInternal(response); } catch (Exception e) { - // TODO(pm): do we want to add the diagnostic information to the result type as well? e.addSuppressed(diagnostic(response)); throw e; } } - // performance sensitive code avoids iterator allocation - @SuppressWarnings({"checkstyle:CyclomaticComplexity", "ForLoopReplaceableByForEach"}) - private T decodeInternal(Response response) { + Optional checkCode(Response response) { int code = response.code(); switch (code) { case 308: @@ -95,7 +105,7 @@ private T decodeInternal(Response response) { UnknownRemoteException remoteException = new UnknownRemoteException(code, ""); remoteException.initCause( QosException.retryOther(qosReason(response), new URL(locationHeader))); - throw remoteException; + return Optional.of(remoteException); } catch (MalformedURLException e) { log.error( "Failed to parse location header for QosException.RetryOther", @@ -108,15 +118,23 @@ private T decodeInternal(Response response) { } break; case 429: - throw response.getFirstHeader(HttpHeaders.RETRY_AFTER) + return Optional.of(response.getFirstHeader(HttpHeaders.RETRY_AFTER) .map(Longs::tryParse) .map(Duration::ofSeconds) .map(duration -> QosException.throttle(qosReason(response), duration)) - .orElseGet(() -> QosException.throttle(qosReason(response))); + .orElseGet(() -> QosException.throttle(qosReason(response)))); case 503: - throw QosException.unavailable(qosReason(response)); + return Optional.of(QosException.unavailable(qosReason(response))); } + return Optional.empty(); + } + private T decodeInternal(Response response) { + Optional maybeQosException = checkCode(response); + if (maybeQosException.isPresent()) { + throw maybeQosException.get(); + } + int code = response.code(); String body; try { body = toString(response.body()); @@ -127,27 +145,25 @@ private T decodeInternal(Response response) { } Optional contentType = response.getFirstHeader(HttpHeaders.CONTENT_TYPE); - // Use a factory: given contentType, create the deserailizer. - // We need Encoding.Deserializer here. That depends on the encoding. - if (contentType.isPresent() && Encodings.matchesContentType("application/json", contentType.get())) { + String jsonContentType = "application/json"; + if (contentType.isPresent() && Encodings.matchesContentType(jsonContentType, contentType.get())) { try { JsonNode node = MAPPER.readTree(body); - if (node.get("errorName") != null) { - // TODO(pm): Update this to use some struct instead of errorName. - TypeMarker container = Optional.ofNullable( - errorNameToTypeMap.get(node.get("errorName").asText())) - .orElseThrow(); - for (int i = 0; i < encodings.size(); i++) { - Encoding encoding = encodings.get(i); - if (encoding.supportsContentType(contentType.get())) { - return encoding.deserializer(container) - .deserialize(new ByteArrayInputStream(body.getBytes(StandardCharsets.UTF_8))); - } - } - } else { - SerializableError serializableError = MAPPER.readValue(body, SerializableError.class); - throw new RemoteException(serializableError, code); + JsonNode errorNameNode = node.get("errorName"); + if (errorNameNode == null) { + throwRemoteException(body, code); } + Optional> maybeDeserializer = + Optional.ofNullable(errorNameToJsonDeserializerMap.get(errorNameNode.asText())); + if (maybeDeserializer.isEmpty()) { + throwRemoteException(body, code); + } + return maybeDeserializer + .get() + .deserialize(new ByteArrayInputStream(body.getBytes(StandardCharsets.UTF_8))); + } catch (RemoteException remoteException) { + // rethrow the created remote exception + throw remoteException; } catch (Exception e) { throw new UnknownRemoteException(code, body); } @@ -156,17 +172,22 @@ private T decodeInternal(Response response) { throw new UnknownRemoteException(code, body); } - private static String toString(InputStream body) throws IOException { + private static void throwRemoteException(String body, int code) throws IOException { + SerializableError serializableError = MAPPER.readValue(body, SerializableError.class); + throw new RemoteException(serializableError, code); + } + + static String toString(InputStream body) throws IOException { try (Reader reader = new InputStreamReader(body, StandardCharsets.UTF_8)) { return CharStreams.toString(reader); } } - private static ResponseDiagnostic diagnostic(Response response) { + static ResponseDiagnostic diagnostic(Response response) { return new ResponseDiagnostic(diagnosticArgs(response)); } - private static ImmutableList> diagnosticArgs(Response response) { + static ImmutableList> diagnosticArgs(Response response) { ImmutableList.Builder> args = ImmutableList.>builder().add(SafeArg.of("status", response.code())); recordHeader(HttpHeaders.SERVER, response, args); recordHeader(HttpHeaders.CONTENT_TYPE, response, args); 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 50642aea0..127f9d0ad 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,35 +17,16 @@ package com.palantir.conjure.java.dialogue.serde; import com.fasterxml.jackson.databind.ObjectMapper; -import com.google.common.collect.ImmutableList; -import com.google.common.io.CharStreams; import com.google.common.net.HttpHeaders; -import com.google.common.primitives.Longs; -import com.palantir.conjure.java.api.errors.QosException; -import com.palantir.conjure.java.api.errors.QosReason; -import com.palantir.conjure.java.api.errors.QosReasons; -import com.palantir.conjure.java.api.errors.QosReasons.QosResponseDecodingAdapter; import com.palantir.conjure.java.api.errors.RemoteException; import com.palantir.conjure.java.api.errors.SerializableError; import com.palantir.conjure.java.api.errors.UnknownRemoteException; import com.palantir.conjure.java.serialization.ObjectMappers; import com.palantir.dialogue.Response; -import com.palantir.logsafe.Arg; -import com.palantir.logsafe.SafeArg; -import com.palantir.logsafe.SafeLoggable; -import com.palantir.logsafe.UnsafeArg; -import com.palantir.logsafe.exceptions.SafeExceptions; 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.net.MalformedURLException; -import java.net.URL; -import java.nio.charset.StandardCharsets; -import java.time.Duration; -import java.util.List; +import java.util.Collections; import java.util.Optional; /** @@ -59,17 +40,19 @@ 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()); public boolean isError(Response response) { - return 300 <= response.code() && response.code() <= 599; + return ENDPOINT_ERROR_DECODER.isError(response); } public RuntimeException decode(Response response) { if (log.isDebugEnabled()) { - log.debug("Received an error response", diagnosticArgs(response)); + log.debug("Received an error response", EndpointErrorDecoder.diagnosticArgs(response)); } RuntimeException result = decodeInternal(response); - result.addSuppressed(diagnostic(response)); + result.addSuppressed(EndpointErrorDecoder.diagnostic(response)); return result; } @@ -77,41 +60,15 @@ private RuntimeException decodeInternal(Response response) { // TODO(rfink): What about HTTP/101 switching protocols? // TODO(rfink): What about HEAD requests? - int code = response.code(); - switch (code) { - case 308: - Optional location = response.getFirstHeader(HttpHeaders.LOCATION); - if (location.isPresent()) { - String locationHeader = location.get(); - try { - UnknownRemoteException remoteException = new UnknownRemoteException(code, ""); - remoteException.initCause( - QosException.retryOther(qosReason(response), new URL(locationHeader))); - return remoteException; - } catch (MalformedURLException e) { - log.error( - "Failed to parse location header for QosException.RetryOther", - UnsafeArg.of("locationHeader", locationHeader), - e); - } - } else { - log.error("Retrieved HTTP status code 308 without Location header, cannot perform " - + "redirect. This appears to be a server-side protocol violation."); - } - break; - case 429: - return response.getFirstHeader(HttpHeaders.RETRY_AFTER) - .map(Longs::tryParse) - .map(Duration::ofSeconds) - .map(duration -> QosException.throttle(qosReason(response), duration)) - .orElseGet(() -> QosException.throttle(qosReason(response))); - case 503: - return QosException.unavailable(qosReason(response)); + Optional maybeQosException = ENDPOINT_ERROR_DECODER.checkCode(response); + if (maybeQosException.isPresent()) { + return maybeQosException.get(); } + int code = response.code(); String body; try { - body = toString(response.body()); + body = EndpointErrorDecoder.toString(response.body()); } catch (NullPointerException | IOException e) { UnknownRemoteException exception = new UnknownRemoteException(code, ""); exception.initCause(e); @@ -130,75 +87,4 @@ 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); - } - } - - private static ResponseDiagnostic diagnostic(Response response) { - return new ResponseDiagnostic(diagnosticArgs(response)); - } - - private static ImmutableList> diagnosticArgs(Response response) { - ImmutableList.Builder> args = ImmutableList.>builder().add(SafeArg.of("status", response.code())); - recordHeader(HttpHeaders.SERVER, response, args); - recordHeader(HttpHeaders.CONTENT_TYPE, response, args); - recordHeader(HttpHeaders.CONTENT_LENGTH, response, args); - recordHeader(HttpHeaders.CONNECTION, response, args); - recordHeader(HttpHeaders.DATE, response, args); - recordHeader("x-envoy-response-flags", response, args); - recordHeader("x-envoy-response-code-details", response, args); - recordHeader("Response-Flags", response, args); - recordHeader("Response-Code-Details", response, args); - return args.build(); - } - - private static void recordHeader(String header, Response response, ImmutableList.Builder> args) { - response.getFirstHeader(header).ifPresent(server -> args.add(SafeArg.of(header, server))); - } - - private static final class ResponseDiagnostic extends RuntimeException implements SafeLoggable { - - private static final String SAFE_MESSAGE = "Response Diagnostic Information"; - - private final ImmutableList> args; - - ResponseDiagnostic(ImmutableList> args) { - super(SafeExceptions.renderMessage(SAFE_MESSAGE, args.toArray(new Arg[0]))); - this.args = args; - } - - @Override - public String getLogMessage() { - return SAFE_MESSAGE; - } - - @Override - public List> getArgs() { - return args; - } - - @Override - @SuppressWarnings("UnsynchronizedOverridesSynchronized") // nop - public Throwable fillInStackTrace() { - // no-op: stack trace generation is expensive, this type exists - // to simply associate diagnostic information with a failure. - return this; - } - } - - private static QosReason qosReason(Response response) { - return QosReasons.parseFromResponse(response, DialogueQosResponseDecodingAdapter.INSTANCE); - } - - private enum DialogueQosResponseDecodingAdapter implements QosResponseDecodingAdapter { - INSTANCE; - - @Override - public Optional getFirstHeader(Response response, String headerName) { - return response.getFirstHeader(headerName); - } - } } diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/BinaryEncodingTest.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/BinaryEncodingTest.java index 0d13b0b4a..fe8a9bcd2 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/BinaryEncodingTest.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/BinaryEncodingTest.java @@ -34,7 +34,6 @@ public void testBinary() throws IOException { TestResponse response = new TestResponse().code(200).contentType("application/octet-stream"); BodySerDe serializers = new ConjureBodySerDe( ImmutableList.of(WeightedEncoding.of(new ConjureBodySerDeTest.StubEncoding("application/json"))), - ErrorDecoder.INSTANCE, Encodings.emptyContainerDeserializer(), DefaultConjureRuntime.DEFAULT_SERDE_CACHE_SPEC); InputStream deserialized = serializers.inputStreamDeserializer().deserialize(response); @@ -58,7 +57,6 @@ public void testBinary_optional_present() throws IOException { TestResponse response = new TestResponse().code(200).contentType("application/octet-stream"); BodySerDe serializers = new ConjureBodySerDe( ImmutableList.of(WeightedEncoding.of(new ConjureBodySerDeTest.StubEncoding("application/json"))), - ErrorDecoder.INSTANCE, Encodings.emptyContainerDeserializer(), DefaultConjureRuntime.DEFAULT_SERDE_CACHE_SPEC); Optional maybe = diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ConjureBodySerDeTest.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ConjureBodySerDeTest.java index 684c5acdd..03879e807 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ConjureBodySerDeTest.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ConjureBodySerDeTest.java @@ -52,8 +52,6 @@ public class ConjureBodySerDeTest { private static final TypeMarker TYPE = new TypeMarker() {}; private static final TypeMarker> OPTIONAL_TYPE = new TypeMarker>() {}; - private ErrorDecoder errorDecoder = ErrorDecoder.INSTANCE; - @Test public void testRequestContentType() throws IOException { @@ -76,7 +74,6 @@ private ConjureBodySerDe conjureBodySerDe(String... contentTypes) { Arrays.stream(contentTypes) .map(c -> WeightedEncoding.of(new StubEncoding(c))) .collect(ImmutableList.toImmutableList()), - errorDecoder, Encodings.emptyContainerDeserializer(), DefaultConjureRuntime.DEFAULT_SERDE_CACHE_SPEC); } @@ -115,7 +112,6 @@ public void testAcceptBasedOnWeight() throws IOException { BodySerDe serializers = new ConjureBodySerDe( ImmutableList.of(WeightedEncoding.of(plain, .5), WeightedEncoding.of(json, 1)), - ErrorDecoder.INSTANCE, Encodings.emptyContainerDeserializer(), DefaultConjureRuntime.DEFAULT_SERDE_CACHE_SPEC); // first encoding is default @@ -174,7 +170,6 @@ public void if_deserialize_throws_response_is_still_closed() { TestResponse response = new TestResponse().code(200).contentType("application/json"); BodySerDe serializers = new ConjureBodySerDe( ImmutableList.of(WeightedEncoding.of(BrokenEncoding.INSTANCE)), - ErrorDecoder.INSTANCE, Encodings.emptyContainerDeserializer(), DefaultConjureRuntime.DEFAULT_SERDE_CACHE_SPEC); assertThatThrownBy(() -> serializers.deserializer(TYPE).deserialize(response)) diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java index a8ac9ab0d..58f1e4631 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java @@ -80,7 +80,6 @@ public final class DefaultClientsTest { private Response response = new TestResponse(); private BodySerDe bodySerde = new ConjureBodySerDe( DefaultConjureRuntime.DEFAULT_ENCODINGS, - ErrorDecoder.INSTANCE, Encodings.emptyContainerDeserializer(), DefaultConjureRuntime.DEFAULT_SERDE_CACHE_SPEC); private final SettableFuture responseFuture = SettableFuture.create(); diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/EndpointErrorTestUtils.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/EndpointErrorTestUtils.java index 69bcb6949..6f45ffbc4 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/EndpointErrorTestUtils.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/EndpointErrorTestUtils.java @@ -20,6 +20,7 @@ import com.palantir.conjure.java.api.errors.CheckedServiceException; import com.palantir.dialogue.TypeMarker; import com.palantir.logsafe.Arg; +import com.palantir.logsafe.Safe; import java.util.HashMap; import java.util.Map; import java.util.Optional; @@ -30,6 +31,26 @@ final class EndpointErrorTestUtils { private EndpointErrorTestUtils() {} + abstract static class EndpointError { + @Safe + String errorCode; + + @Safe + String errorName; + + @Safe + String errorInstanceId; + + T args; + + EndpointError(String errorCode, String errorName, String errorInstanceId, T args) { + this.errorCode = errorCode; + this.errorName = errorName; + this.errorInstanceId = errorInstanceId; + this.args = args; + } + } + record ConjureError( @JsonProperty("errorCode") String errorCode, @JsonProperty("errorName") String errorName, 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 296bb193d..a08c84806 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 @@ -17,6 +17,7 @@ package com.palantir.conjure.java.dialogue.serde; import static org.assertj.core.api.AssertionsForClassTypes.assertThat; +import static org.assertj.core.api.AssertionsForClassTypes.assertThatExceptionOfType; import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; @@ -24,7 +25,10 @@ import com.google.common.collect.ImmutableList; import com.palantir.conjure.java.api.errors.CheckedServiceException; import com.palantir.conjure.java.api.errors.ErrorType; +import com.palantir.conjure.java.api.errors.RemoteException; +import com.palantir.conjure.java.api.errors.SerializableError; import com.palantir.conjure.java.dialogue.serde.EndpointErrorTestUtils.ConjureError; +import com.palantir.conjure.java.dialogue.serde.EndpointErrorTestUtils.EndpointError; import com.palantir.conjure.java.dialogue.serde.EndpointErrorTestUtils.TypeReturningStubEncoding; import com.palantir.conjure.java.serialization.ObjectMappers; import com.palantir.dialogue.BodySerDe; @@ -48,48 +52,31 @@ @ExtendWith(MockitoExtension.class) public class EndpointErrorsConjureBodySerDeTest { private static final ObjectMapper MAPPER = ObjectMappers.newServerObjectMapper(); - private ErrorDecoder errorDecoder = ErrorDecoder.INSTANCE; @Generated("by conjure-java") - private sealed interface EndpointReturnBaseType permits StringReturn, ErrorForEndpoint {} + private sealed interface EndpointReturnBaseType permits ExpectedReturnValue, ErrorReturnValue {} @Generated("by conjure-java") - record StringReturn(String value) implements EndpointReturnBaseType { + record ExpectedReturnValue(String value) implements EndpointReturnBaseType { @JsonCreator - public static StringReturn create(String value) { - return new StringReturn(Preconditions.checkArgumentNotNull(value, "value cannot be null")); + public static ExpectedReturnValue create(String value) { + return new ExpectedReturnValue(Preconditions.checkArgumentNotNull(value, "value cannot be null")); } } - abstract static class EndpointError { - @Safe - String errorCode; - - @Safe - String errorName; - - @Safe - String errorInstanceId; - - T args; - - EndpointError(String errorCode, String errorName, String errorInstanceId, T args) { - this.errorCode = errorCode; - this.errorName = errorName; - this.errorInstanceId = errorInstanceId; - this.args = args; - } - } + @Generated("by conjure-java") + record ComplexArg(int foo, String bar) {} + @Generated("by conjure-java") record ErrorForEndpointArgs( @JsonProperty("arg") @Safe String arg, @JsonProperty("unsafeArg") @Unsafe String unsafeArg, @JsonProperty("complexArg") @Safe ComplexArg complexArg, @JsonProperty("optionalArg") @Safe Optional optionalArg) {} - static final class ErrorForEndpoint extends EndpointError implements EndpointReturnBaseType { + static final class ErrorReturnValue extends EndpointError implements EndpointReturnBaseType { @JsonCreator - ErrorForEndpoint( + ErrorReturnValue( @JsonProperty("errorCode") String errorCode, @JsonProperty("errorName") String errorName, @JsonProperty("errorInstanceId") String errorInstanceId, @@ -98,9 +85,6 @@ static final class ErrorForEndpoint extends EndpointError } } - @Generated("by conjure-java") - record ComplexArg(int foo, String bar) {} - @Generated("by conjure-java") public static final class TestEndpointError extends CheckedServiceException { private TestEndpointError( @@ -120,30 +104,34 @@ private TestEndpointError( } @Test - public void testDeserializeCustomErrors() throws IOException { + public void testDeserializeCustomError() throws IOException { + // Given TestEndpointError errorThrownByEndpoint = new TestEndpointError("value", "unsafeValue", new ComplexArg(1, "bar"), Optional.of(2), null); - - ErrorForEndpoint expectedErrorForEndpoint = new ErrorForEndpoint( - "FAILED_PRECONDITION", - "Default:FailedPrecondition", - errorThrownByEndpoint.getErrorInstanceId(), - new ErrorForEndpointArgs("value", "unsafeValue", new ComplexArg(1, "bar"), Optional.of(2))); - String responseBody = MAPPER.writeValueAsString(ConjureError.fromCheckedServiceException(errorThrownByEndpoint)); + TestResponse response = TestResponse.withBody(responseBody) .contentType("application/json") .code(500); BodySerDe serializers = conjureBodySerDe("application/json", "text/plain"); DeserializerArgs deserializerArgs = DeserializerArgs.builder() .withBaseType(new TypeMarker<>() {}) - .withExpectedResult(new TypeMarker() {}) - .withErrorType("Default:FailedPrecondition", new TypeMarker() {}) + .withExpectedResult(new TypeMarker() {}) + .withErrorType("Default:FailedPrecondition", new TypeMarker() {}) .build(); + + // When EndpointErrorsConjureBodySerDeTest.EndpointReturnBaseType value = serializers.deserializer(deserializerArgs).deserialize(response); + // Then + ErrorReturnValue expectedErrorForEndpoint = new ErrorReturnValue( + ErrorType.FAILED_PRECONDITION.code().name(), + ErrorType.FAILED_PRECONDITION.name(), + errorThrownByEndpoint.getErrorInstanceId(), + new ErrorForEndpointArgs("value", "unsafeValue", new ComplexArg(1, "bar"), Optional.of(2))); + assertThat(value).isInstanceOf(ErrorReturnValue.class); assertThat(value) .extracting("errorCode", "errorName", "errorInstanceId", "args") .containsExactly( @@ -153,8 +141,49 @@ public void testDeserializeCustomErrors() throws IOException { expectedErrorForEndpoint.args); } + // When an error is deserialized, but the error type is not registered, the error should be deserialized as a + // SerializableError and a RemoteException should be thrown. + @Test + public void testDeserializingUndefinedErrorFallsbackToSerializableError() throws IOException { + TestEndpointError errorThrownByEndpoint = + new TestEndpointError("value", "unsafeValue", new ComplexArg(1, "bar"), Optional.of(2), null); + String responseBody = + MAPPER.writeValueAsString(ConjureError.fromCheckedServiceException(errorThrownByEndpoint)); + + TestResponse response = TestResponse.withBody(responseBody) + .contentType("application/json") + .code(500); + BodySerDe serializers = conjureBodySerDe("application/json", "text/plain"); + DeserializerArgs deserializerArgs = DeserializerArgs.builder() + .withBaseType(new TypeMarker<>() {}) + .withExpectedResult(new TypeMarker() {}) + // Note: no error types are registered. + .build(); + + // Then + assertThatExceptionOfType(RemoteException.class) + .isThrownBy(() -> { + serializers.deserializer(deserializerArgs).deserialize(response); + }) + .satisfies(exception -> { + SerializableError error = exception.getError(); + assertThat(error.errorCode()) + .isEqualTo(ErrorType.FAILED_PRECONDITION.code().name()); + assertThat(error.errorInstanceId()).isEqualTo(errorThrownByEndpoint.getErrorInstanceId()); + assertThat(error.errorName()).isEqualTo(ErrorType.FAILED_PRECONDITION.name()); + assertThat(error.parameters()) + .extracting("arg", "unsafeArg", "complexArg", "optionalArg") + .containsExactly( + "value", + "unsafeValue", + MAPPER.writeValueAsString(new ComplexArg(1, "bar")), + MAPPER.writeValueAsString(Optional.of(2))); + }); + } + @Test public void testDeserializeExpectedValue() { + // Given String expectedString = "expectedString"; TestResponse response = TestResponse.withBody(String.format("\"%s\"", expectedString)) .contentType("application/json") @@ -162,12 +191,14 @@ public void testDeserializeExpectedValue() { BodySerDe serializers = conjureBodySerDe("application/json", "text/plain"); DeserializerArgs deserializerArgs = DeserializerArgs.builder() .withBaseType(new TypeMarker<>() {}) - .withExpectedResult(new TypeMarker() {}) - .withErrorType("Default:FailedPrecondition", new TypeMarker() {}) + .withExpectedResult(new TypeMarker() {}) + .withErrorType("Default:FailedPrecondition", new TypeMarker() {}) .build(); + // When EndpointReturnBaseType value = serializers.deserializer(deserializerArgs).deserialize(response); - assertThat(value).isEqualTo(new StringReturn(expectedString)); + // Then + assertThat(value).isEqualTo(new ExpectedReturnValue(expectedString)); } private ConjureBodySerDe conjureBodySerDe(String... contentTypes) { @@ -175,7 +206,6 @@ private ConjureBodySerDe conjureBodySerDe(String... contentTypes) { Arrays.stream(contentTypes) .map(c -> WeightedEncoding.of(new TypeReturningStubEncoding(c))) .collect(ImmutableList.toImmutableList()), - errorDecoder, Encodings.emptyContainerDeserializer(), DefaultConjureRuntime.DEFAULT_SERDE_CACHE_SPEC); } 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 510914481..b809ac3a6 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 @@ -17,6 +17,7 @@ package com.palantir.conjure.java.dialogue.serde; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.assertj.core.api.Assertions.fail; import com.fasterxml.jackson.core.JsonProcessingException; @@ -38,8 +39,13 @@ import com.palantir.logsafe.Preconditions; 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.mockito.junit.jupiter.MockitoExtension; @ExtendWith(MockitoExtension.class) @@ -63,16 +69,17 @@ private static String createServiceException(ServiceException exception) { } private static final ErrorDecoder decoder = ErrorDecoder.INSTANCE; + private static final EndpointErrorDecoder endpointErrorDecoder = + new EndpointErrorDecoder<>(Collections.emptyMap(), Optional.empty()); - @Test - public void extractsRemoteExceptionForAllErrorCodes() { + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void extractsRemoteExceptionForAllErrorCodes(boolean isLegacyErrorDecoder) { for (int code : ImmutableList.of(300, 400, 404, 500)) { Response response = TestResponse.withBody(SERIALIZED_EXCEPTION).code(code).contentType("application/json"); - assertThat(decoder.isError(response)).isTrue(); - RuntimeException result = decoder.decode(response); - assertThat(result).isInstanceOfSatisfying(RemoteException.class, exception -> { + Consumer validationFunction = exception -> { assertThat(exception.getCause()).isNull(); assertThat(exception.getStatus()).isEqualTo(code); assertThat(exception.getError().errorCode()) @@ -91,117 +98,224 @@ public void extractsRemoteExceptionForAllErrorCodes() { + " (" + ErrorType.FAILED_PRECONDITION.name() + ")"); - }); + }; + + if (isLegacyErrorDecoder) { + assertThat(decoder.isError(response)).isTrue(); + RuntimeException result = decoder.decode(response); + assertThat(result).isInstanceOfSatisfying(RemoteException.class, validationFunction); + } else { + assertThat(endpointErrorDecoder.isError(response)).isTrue(); + assertThatExceptionOfType(RemoteException.class) + .isThrownBy(() -> endpointErrorDecoder.decode(response)) + .satisfies(validationFunction); + } } } - @Test - public void testQos503() { + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void testQos503(boolean isLegacyErrorDecoder) { Response response = TestResponse.withBody(SERIALIZED_EXCEPTION).code(503); - assertThat(decoder.isError(response)).isTrue(); - RuntimeException result = decoder.decode(response); - assertThat(result).isInstanceOfSatisfying(QosException.Unavailable.class, exception -> { - assertThat(exception.getReason()).isEqualTo(QOS_REASON); - }); + Consumer validationFunction = exception -> { + assertThat(exception).isInstanceOfSatisfying(QosException.Unavailable.class, qosException -> { + assertThat(qosException.getReason()).isEqualTo(QOS_REASON); + }); + }; + + if (isLegacyErrorDecoder) { + assertThat(decoder.isError(response)).isTrue(); + RuntimeException result = decoder.decode(response); + assertThat(result).isInstanceOfSatisfying(RuntimeException.class, validationFunction); + } else { + assertThat(endpointErrorDecoder.isError(response)).isTrue(); + assertThatExceptionOfType(RuntimeException.class) + .isThrownBy(() -> endpointErrorDecoder.decode(response)) + .satisfies(validationFunction); + } } - @Test - public void testQos503WithMetadata() { + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void testQos503WithMetadata(boolean isLegacyErrorDecoder) { Response response = TestResponse.withBody(SERIALIZED_EXCEPTION) .code(503) .withHeader("Qos-Retry-Hint", "do-not-retry") .withHeader("Qos-Due-To", "custom"); - assertThat(decoder.isError(response)).isTrue(); - - RuntimeException result = decoder.decode(response); - assertThat(result).isInstanceOfSatisfying(QosException.Unavailable.class, exception -> { - assertThat(exception.getReason()) - .isEqualTo(QosReason.builder() - .from(QOS_REASON) - .dueTo(DueTo.CUSTOM) - .retryHint(RetryHint.DO_NOT_RETRY) - .build()); - }); + + Consumer validationFunction = exception -> { + assertThat(exception).isInstanceOfSatisfying(QosException.Unavailable.class, qosException -> { + assertThat(qosException.getReason()) + .isEqualTo(QosReason.builder() + .from(QOS_REASON) + .dueTo(DueTo.CUSTOM) + .retryHint(RetryHint.DO_NOT_RETRY) + .build()); + }); + }; + + if (isLegacyErrorDecoder) { + assertThat(decoder.isError(response)).isTrue(); + RuntimeException result = decoder.decode(response); + assertThat(result).isInstanceOfSatisfying(RuntimeException.class, validationFunction); + } else { + assertThat(endpointErrorDecoder.isError(response)).isTrue(); + assertThatExceptionOfType(RuntimeException.class) + .isThrownBy(() -> endpointErrorDecoder.decode(response)) + .satisfies(validationFunction); + } } - @Test - public void testQos429() { + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void testQos429(boolean isLegacyErrorDecoder) { Response response = TestResponse.withBody(SERIALIZED_EXCEPTION).code(429); - assertThat(decoder.isError(response)).isTrue(); - RuntimeException result = decoder.decode(response); - assertThat(result).isInstanceOfSatisfying(QosException.Throttle.class, exception -> { - assertThat(exception.getReason()).isEqualTo(QOS_REASON); - assertThat(exception.getRetryAfter()).isEmpty(); - }); + Consumer validationFunction = exception -> { + assertThat(exception).isInstanceOfSatisfying(QosException.Throttle.class, qosException -> { + assertThat(qosException.getReason()).isEqualTo(QOS_REASON); + assertThat(qosException.getRetryAfter()).isEmpty(); + }); + }; + + if (isLegacyErrorDecoder) { + assertThat(decoder.isError(response)).isTrue(); + RuntimeException result = decoder.decode(response); + assertThat(result).isInstanceOfSatisfying(RuntimeException.class, validationFunction); + } else { + assertThat(endpointErrorDecoder.isError(response)).isTrue(); + assertThatExceptionOfType(RuntimeException.class) + .isThrownBy(() -> endpointErrorDecoder.decode(response)) + .satisfies(validationFunction); + } } - @Test - public void testQos429_retryAfter() { + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void testQos429_retryAfter(boolean isLegacyErrorDecoder) { Response response = TestResponse.withBody(SERIALIZED_EXCEPTION).code(429).withHeader(HttpHeaders.RETRY_AFTER, "3"); - assertThat(decoder.isError(response)).isTrue(); - RuntimeException result = decoder.decode(response); - assertThat(result).isInstanceOfSatisfying(QosException.Throttle.class, exception -> { - assertThat(exception.getReason()).isEqualTo(QOS_REASON); - assertThat(exception.getRetryAfter()).hasValue(Duration.ofSeconds(3)); - }); + Consumer validationFunction = exception -> { + assertThat(exception).isInstanceOfSatisfying(QosException.Throttle.class, qosException -> { + assertThat(qosException.getReason()).isEqualTo(QOS_REASON); + assertThat(qosException.getRetryAfter()).hasValue(Duration.ofSeconds(3)); + }); + }; + + if (isLegacyErrorDecoder) { + assertThat(decoder.isError(response)).isTrue(); + RuntimeException result = decoder.decode(response); + assertThat(result).isInstanceOfSatisfying(RuntimeException.class, validationFunction); + } else { + assertThat(endpointErrorDecoder.isError(response)).isTrue(); + assertThatExceptionOfType(RuntimeException.class) + .isThrownBy(() -> endpointErrorDecoder.decode(response)) + .satisfies(validationFunction); + } } - @Test - public void testQos429_retryAfter_invalid() { + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void testQos429_retryAfter_invalid(boolean isLegacyErrorDecoder) { Response response = TestResponse.withBody(SERIALIZED_EXCEPTION).code(429).withHeader(HttpHeaders.RETRY_AFTER, "bad"); - assertThat(decoder.isError(response)).isTrue(); - RuntimeException result = decoder.decode(response); - assertThat(result).isInstanceOfSatisfying(QosException.Throttle.class, exception -> { - assertThat(exception.getReason()).isEqualTo(QOS_REASON); - assertThat(exception.getRetryAfter()).isEmpty(); - }); + Consumer validationFunction = exception -> { + assertThat(exception).isInstanceOfSatisfying(QosException.Throttle.class, qosException -> { + assertThat(qosException.getReason()).isEqualTo(QOS_REASON); + assertThat(qosException.getRetryAfter()).isEmpty(); + }); + }; + + if (isLegacyErrorDecoder) { + assertThat(decoder.isError(response)).isTrue(); + RuntimeException result = decoder.decode(response); + assertThat(result).isInstanceOfSatisfying(RuntimeException.class, validationFunction); + } else { + assertThat(endpointErrorDecoder.isError(response)).isTrue(); + assertThatExceptionOfType(RuntimeException.class) + .isThrownBy(() -> endpointErrorDecoder.decode(response)) + .satisfies(validationFunction); + } } - @Test - public void testQos308_noLocation() { + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void testQos308_noLocation(boolean isLegacyErrorDecoder) { Response response = TestResponse.withBody(SERIALIZED_EXCEPTION).code(308); - assertThat(decoder.isError(response)).isTrue(); - RuntimeException result = decoder.decode(response); - assertThat(result) - .isInstanceOfSatisfying(UnknownRemoteException.class, exception -> assertThat(exception.getStatus()) - .isEqualTo(308)); + Consumer validationFunction = exception -> { + assertThat(exception).isInstanceOfSatisfying(UnknownRemoteException.class, unknownException -> { + assertThat(unknownException.getStatus()).isEqualTo(308); + }); + }; + + if (isLegacyErrorDecoder) { + assertThat(decoder.isError(response)).isTrue(); + RuntimeException result = decoder.decode(response); + assertThat(result).isInstanceOfSatisfying(RuntimeException.class, validationFunction); + } else { + assertThat(endpointErrorDecoder.isError(response)).isTrue(); + assertThatExceptionOfType(RuntimeException.class) + .isThrownBy(() -> endpointErrorDecoder.decode(response)) + .satisfies(validationFunction); + } } - @Test - public void testQos308_invalidLocation() { + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void testQos308_invalidLocation(boolean isLegacyErrorDecoder) { Response response = TestResponse.withBody(SERIALIZED_EXCEPTION).code(308).withHeader(HttpHeaders.LOCATION, "invalid"); - assertThat(decoder.isError(response)).isTrue(); - RuntimeException result = decoder.decode(response); - assertThat(result) - .isInstanceOfSatisfying(UnknownRemoteException.class, exception -> assertThat(exception.getStatus()) - .isEqualTo(308)); + Consumer validationFunction = exception -> { + assertThat(exception).isInstanceOfSatisfying(UnknownRemoteException.class, unknownException -> { + assertThat(unknownException.getStatus()).isEqualTo(308); + }); + }; + + if (isLegacyErrorDecoder) { + assertThat(decoder.isError(response)).isTrue(); + RuntimeException result = decoder.decode(response); + assertThat(result).isInstanceOfSatisfying(RuntimeException.class, validationFunction); + } else { + assertThat(endpointErrorDecoder.isError(response)).isTrue(); + assertThatExceptionOfType(RuntimeException.class) + .isThrownBy(() -> endpointErrorDecoder.decode(response)) + .satisfies(validationFunction); + } } - @Test - public void testQos308() { + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void testQos308(boolean isLegacyErrorDecoder) { String expectedLocation = "https://localhost"; Response response = TestResponse.withBody(SERIALIZED_EXCEPTION) .code(308) .withHeader(HttpHeaders.LOCATION, expectedLocation); - assertThat(decoder.isError(response)).isTrue(); - RuntimeException result = decoder.decode(response); - assertThat(result) - .isInstanceOf(UnknownRemoteException.class) - .getRootCause() - .isInstanceOfSatisfying(QosException.RetryOther.class, exception -> { - assertThat(exception.getReason()).isEqualTo(QOS_REASON); - assertThat(exception.getRedirectTo()).asString().isEqualTo(expectedLocation); - }); + Consumer validationFunction = exception -> { + assertThat(exception) + .isInstanceOf(UnknownRemoteException.class) + .getRootCause() + .isInstanceOfSatisfying(QosException.RetryOther.class, qosException -> { + assertThat(qosException.getReason()).isEqualTo(QOS_REASON); + assertThat(qosException.getRedirectTo()).asString().isEqualTo(expectedLocation); + }); + }; + + if (isLegacyErrorDecoder) { + assertThat(decoder.isError(response)).isTrue(); + RuntimeException result = decoder.decode(response); + assertThat(result).isInstanceOfSatisfying(RuntimeException.class, validationFunction); + } else { + assertThat(endpointErrorDecoder.isError(response)).isTrue(); + assertThatExceptionOfType(RuntimeException.class) + .isThrownBy(() -> endpointErrorDecoder.decode(response)) + .satisfies(validationFunction); + } } @Test @@ -217,16 +331,31 @@ public void cannotDecodeNonJsonMediaTypes() { TestResponse.withBody(SERIALIZED_EXCEPTION).code(500).contentType("text/plain"))) .isInstanceOf(UnknownRemoteException.class) .hasMessage("Response status: 500"); + + assertThatExceptionOfType(UnknownRemoteException.class) + .isThrownBy(() -> endpointErrorDecoder.decode( + TestResponse.withBody(SERIALIZED_EXCEPTION).code(500).contentType("text/plain"))) + .satisfies(exception -> assertThat(exception.getMessage()).isEqualTo("Response status: 500")); } - @Test - public void doesNotHandleUnparseableBody() { - assertThat(decoder.decode(TestResponse.withBody("not json").code(500).contentType("application/json/"))) - .isInstanceOfSatisfying(UnknownRemoteException.class, expected -> { - assertThat(expected.getStatus()).isEqualTo(500); - assertThat(expected.getBody()).isEqualTo("not json"); - assertThat(expected.getMessage()).isEqualTo("Response status: 500"); - }); + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void doesNotHandleUnparseableBody(boolean isLegacyErrorDecoder) { + Response response = TestResponse.withBody("not json").code(500).contentType("application/json/"); + + Consumer validationFunction = exception -> { + assertThat(exception.getStatus()).isEqualTo(500); + assertThat(exception.getBody()).isEqualTo("not json"); + }; + + if (isLegacyErrorDecoder) { + RuntimeException result = decoder.decode(response); + assertThat(result).isInstanceOfSatisfying(UnknownRemoteException.class, validationFunction); + } else { + assertThatExceptionOfType(UnknownRemoteException.class) + .isThrownBy(() -> endpointErrorDecoder.decode(response)) + .satisfies(validationFunction); + } } @Test @@ -235,32 +364,57 @@ public void doesNotHandleNullBody() { assertThat(decoder.decode(TestResponse.withBody(null).code(500).contentType("application/json"))) .isInstanceOf(UnknownRemoteException.class) .hasMessage("Response status: 500"); + + assertThatExceptionOfType(UnknownRemoteException.class) + .isThrownBy(() -> endpointErrorDecoder.decode( + TestResponse.withBody(null).code(500).contentType("application/json"))) + .satisfies(exception -> assertThat(exception.getMessage()).isEqualTo("Response status: 500")); } - @Test - public void handlesUnexpectedJson() { - assertThat(decoder.decode(TestResponse.withBody("{\"error\":\"some-unknown-json\"}") - .code(502) - .contentType("application/json"))) - .isInstanceOfSatisfying(UnknownRemoteException.class, expected -> { - assertThat(expected.getStatus()).isEqualTo(502); - assertThat(expected.getBody()).isEqualTo("{\"error\":\"some-unknown-json\"}"); - assertThat(expected.getMessage()).isEqualTo("Response status: 502"); - }); + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void handlesUnexpectedJson(boolean isLegacyErrorDecoder) { + Response response = TestResponse.withBody("{\"error\":\"some-unknown-json\"}") + .code(502) + .contentType("application/json"); + + Consumer validationFunction = expected -> { + assertThat(expected.getStatus()).isEqualTo(502); + assertThat(expected.getBody()).isEqualTo("{\"error\":\"some-unknown-json\"}"); + assertThat(expected.getMessage()).isEqualTo("Response status: 502"); + }; + if (isLegacyErrorDecoder) { + assertThat(decoder.decode(response)) + .isInstanceOfSatisfying(UnknownRemoteException.class, validationFunction); + } else { + assertThatExceptionOfType(UnknownRemoteException.class) + .isThrownBy(() -> endpointErrorDecoder.decode(response)) + .satisfies(validationFunction); + } } - @Test - public void handlesJsonWithEncoding() { + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void handlesJsonWithEncoding(boolean isLegacyErrorDecoder) { int code = 500; - RuntimeException result = decoder.decode( - TestResponse.withBody(SERIALIZED_EXCEPTION).code(code).contentType("application/json; charset=utf-8")); - assertThat(result).isInstanceOfSatisfying(RemoteException.class, exception -> { + Response response = + TestResponse.withBody(SERIALIZED_EXCEPTION).code(code).contentType("application/json; charset=utf-8"); + + Consumer validationFunction = exception -> { assertThat(exception.getCause()).isNull(); assertThat(exception.getStatus()).isEqualTo(code); assertThat(exception.getError().errorCode()) .isEqualTo(ErrorType.FAILED_PRECONDITION.code().name()); assertThat(exception.getError().errorName()).isEqualTo(ErrorType.FAILED_PRECONDITION.name()); - }); + }; + + if (isLegacyErrorDecoder) { + assertThat(decoder.decode(response)).isInstanceOfSatisfying(RemoteException.class, validationFunction); + } else { + assertThatExceptionOfType(RemoteException.class) + .isThrownBy(() -> endpointErrorDecoder.decode(response)) + .satisfies(validationFunction); + } } private static RemoteException encodeAndDecode(Exception exception) { From 7dac6eed3de762f4506470106bfae659dc8c911b Mon Sep 17 00:00:00 2001 From: Pritham Marupaka Date: Thu, 9 Jan 2025 11:29:20 -0500 Subject: [PATCH 03/17] 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() { From bcbf495e90650afb39c38634e50501a417211890 Mon Sep 17 00:00:00 2001 From: Pritham Marupaka Date: Fri, 10 Jan 2025 11:23:53 -0500 Subject: [PATCH 04/17] add test to ensure supplied JSON deserializer is used when available --- .../serde/EndpointErrorTestUtils.java | 45 +++++++++++++++- .../EndpointErrorsConjureBodySerDeTest.java | 52 +++++++++++++++++-- 2 files changed, 93 insertions(+), 4 deletions(-) diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/EndpointErrorTestUtils.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/EndpointErrorTestUtils.java index 6f45ffbc4..b53de896e 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/EndpointErrorTestUtils.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/EndpointErrorTestUtils.java @@ -21,12 +21,19 @@ import com.palantir.dialogue.TypeMarker; import com.palantir.logsafe.Arg; import com.palantir.logsafe.Safe; +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.Optional; import java.util.OptionalDouble; import java.util.OptionalInt; import java.util.OptionalLong; +import java.util.function.Function; final class EndpointErrorTestUtils { private EndpointErrorTestUtils() {} @@ -84,9 +91,17 @@ private static boolean shouldIncludeArgInParameters(Arg arg) { public static final class TypeReturningStubEncoding implements Encoding { private final String contentType; + private final Function, Encoding.Deserializer> deserializerFactory; + private final Map, Encoding.Deserializer> deserializers = new HashMap<>(); TypeReturningStubEncoding(String contentType) { + this(contentType, typeMarker -> Encodings.json().deserializer(typeMarker)); + } + + TypeReturningStubEncoding( + String contentType, Function, Encoding.Deserializer> deserializerFactory) { this.contentType = contentType; + this.deserializerFactory = deserializerFactory; } @Override @@ -97,9 +112,12 @@ public Encoding.Serializer serializer(TypeMarker _type) { } @Override + @SuppressWarnings("unchecked") public Encoding.Deserializer deserializer(TypeMarker type) { return input -> { - return (T) Encodings.json().deserializer(type).deserialize(input); + Deserializer deserializer = + (Deserializer) deserializers.computeIfAbsent(type, deserializerFactory); + return deserializer.deserialize(input); }; } @@ -117,5 +135,30 @@ public boolean supportsContentType(String input) { public String toString() { return "TypeReturningStubEncoding{" + contentType + '}'; } + + @SuppressWarnings("unchecked") + public Encoding.Deserializer getDeserializer(TypeMarker type) { + return (Deserializer) deserializers.get(type); + } + } + + public static final class ContentRecordingJsonDeserializer implements Encoding.Deserializer { + private final List deserializedContent = new ArrayList<>(); + private final Encoding.Deserializer delegate; + + ContentRecordingJsonDeserializer(TypeMarker type) { + this.delegate = Encodings.json().deserializer(type); + } + + public List getDeserializedContent() { + return deserializedContent; + } + + @Override + public T deserialize(InputStream input) throws IOException { + String inputString = new String(input.readAllBytes(), StandardCharsets.UTF_8); + deserializedContent.add(inputString); + return delegate.deserialize(new ByteArrayInputStream(inputString.getBytes(StandardCharsets.UTF_8))); + } } } 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 cde06f5fa..858e9df20 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 @@ -21,6 +21,7 @@ import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.collect.ImmutableList; import com.palantir.conjure.java.api.errors.CheckedServiceException; @@ -28,6 +29,7 @@ import com.palantir.conjure.java.api.errors.RemoteException; import com.palantir.conjure.java.api.errors.SerializableError; import com.palantir.conjure.java.dialogue.serde.EndpointErrorTestUtils.ConjureError; +import com.palantir.conjure.java.dialogue.serde.EndpointErrorTestUtils.ContentRecordingJsonDeserializer; import com.palantir.conjure.java.dialogue.serde.EndpointErrorTestUtils.EndpointError; import com.palantir.conjure.java.dialogue.serde.EndpointErrorTestUtils.TypeReturningStubEncoding; import com.palantir.conjure.java.serialization.ObjectMappers; @@ -42,11 +44,15 @@ import com.palantir.logsafe.UnsafeArg; import java.io.IOException; import java.util.Arrays; +import java.util.List; import java.util.Optional; import javax.annotation.Nullable; import javax.annotation.processing.Generated; +import org.assertj.core.api.InstanceOfAssertFactories; 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.mockito.junit.jupiter.MockitoExtension; @ExtendWith(MockitoExtension.class) @@ -103,8 +109,10 @@ private TestEndpointError( } } - @Test - public void testDeserializeCustomError() throws IOException { + // The error should be deserialized using Encodings.json(), when a JSON encoding is not provided. + @ParameterizedTest + @ValueSource(strings = {"application/json", "text/plain"}) + public void testDeserializeCustomError(String supportedContentType) throws IOException { // Given TestEndpointError errorThrownByEndpoint = new TestEndpointError("value", "unsafeValue", new ComplexArg(1, "bar"), Optional.of(2), null); @@ -114,7 +122,7 @@ public void testDeserializeCustomError() throws IOException { TestResponse response = TestResponse.withBody(responseBody) .contentType("application/json") .code(500); - BodySerDe serializers = conjureBodySerDe("application/json", "text/plain"); + BodySerDe serializers = conjureBodySerDe(supportedContentType); DeserializerArgs deserializerArgs = DeserializerArgs.builder() .baseType(new TypeMarker<>() {}) .success(new TypeMarker() {}) @@ -201,6 +209,44 @@ public void testDeserializeExpectedValue() { assertThat(value).isEqualTo(new ExpectedReturnValue(expectedString)); } + // Ensure that the supplied JSON encoding is used when available. + @Test + public void testDeserializeWithCustomEncoding() throws JsonProcessingException { + // Given + TestEndpointError errorThrownByEndpoint = + new TestEndpointError("value", "unsafeValue", new ComplexArg(1, "bar"), Optional.of(2), null); + String responseBody = + MAPPER.writeValueAsString(ConjureError.fromCheckedServiceException(errorThrownByEndpoint)); + + TypeReturningStubEncoding stubbingEncoding = + new TypeReturningStubEncoding("application/json", ContentRecordingJsonDeserializer::new); + BodySerDe serializers = new ConjureBodySerDe( + List.of(WeightedEncoding.of(stubbingEncoding)), + Encodings.emptyContainerDeserializer(), + DefaultConjureRuntime.DEFAULT_SERDE_CACHE_SPEC); + TestResponse response = TestResponse.withBody(responseBody) + .contentType("application/json") + .code(500); + + TypeMarker errorTypeMarker = new TypeMarker<>() {}; + DeserializerArgs deserializerArgs = DeserializerArgs.builder() + .baseType(new TypeMarker<>() {}) + .success(new TypeMarker() {}) + .error("Default:FailedPrecondition", errorTypeMarker) + .build(); + + // When + serializers.deserializer(deserializerArgs).deserialize(response); + + // Then + assertThat(stubbingEncoding.getDeserializer(errorTypeMarker)) + .isInstanceOfSatisfying(ContentRecordingJsonDeserializer.class, deserializer -> { + assertThat(deserializer.getDeserializedContent()) + .asInstanceOf(InstanceOfAssertFactories.LIST) + .containsExactly(responseBody); + }); + } + private ConjureBodySerDe conjureBodySerDe(String... contentTypes) { return new ConjureBodySerDe( Arrays.stream(contentTypes) From c67cb58c7b0e8ec77fae2da3c632b34b8300478f Mon Sep 17 00:00:00 2001 From: Pritham Marupaka Date: Fri, 10 Jan 2025 12:45:44 -0500 Subject: [PATCH 05/17] fix silly mistakes --- .../dialogue/serde/EndpointErrorDecoder.java | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) 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 e3afa4f67..2c1547240 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 @@ -20,7 +20,6 @@ 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; import com.palantir.conjure.java.api.errors.QosException; @@ -43,8 +42,6 @@ import java.io.ByteArrayInputStream; import java.io.IOException; import java.io.InputStream; -import java.io.InputStreamReader; -import java.io.Reader; import java.net.MalformedURLException; import java.net.URL; import java.nio.charset.StandardCharsets; @@ -71,6 +68,8 @@ final class EndpointErrorDecoder { private static final Encoding JSON_ENCODING = Encodings.json(); private static final Deserializer NAMED_ERROR_DESERIALIZER = JSON_ENCODING.deserializer(new TypeMarker<>() {}); + private static final Deserializer SERIALIZABLE_ERROR_DESERIALIZER = + JSON_ENCODING.deserializer(new TypeMarker<>() {}); private final Map> errorNameToJsonDeserializerMap; EndpointErrorDecoder(Map> errorNameToTypeMap) { @@ -83,11 +82,11 @@ final class EndpointErrorDecoder { Maps.transformValues(errorNameToTypeMap, maybeJsonEncoding.orElse(JSON_ENCODING)::deserializer)); } - public boolean isError(Response response) { + boolean isError(Response response) { return 300 <= response.code() && response.code() <= 599; } - public T decode(Response response) { + T decode(Response response) { if (log.isDebugEnabled()) { log.debug("Received an error response", diagnosticArgs(response)); } @@ -187,15 +186,14 @@ private T decodeInternal(Response response) { } private static RemoteException createRemoteException(byte[] body, int code) throws IOException { - SerializableError serializableError = JSON_ENCODING - .deserializer(new TypeMarker() {}) - .deserialize(new ByteArrayInputStream(body)); + SerializableError serializableError = + SERIALIZABLE_ERROR_DESERIALIZER.deserialize(new ByteArrayInputStream(body)); return new RemoteException(serializableError, code); } private static byte[] toByteArray(InputStream body) throws IOException { - try (Reader reader = new InputStreamReader(body, StandardCharsets.UTF_8)) { - return CharStreams.toString(reader).getBytes(StandardCharsets.UTF_8); + try (body) { + return body.readAllBytes(); } } From ae9dfffc22ccfe71fcf8ee4849949ce6cdaaba41 Mon Sep 17 00:00:00 2001 From: svc-autorelease Date: Fri, 10 Jan 2025 21:26:04 +0000 Subject: [PATCH 06/17] Release 4.8.0-rc1 [skip ci] From e09c1b96cd975d36d1ecb225366f33b0292d9039 Mon Sep 17 00:00:00 2001 From: Pritham Marupaka Date: Tue, 21 Jan 2025 18:37:19 -0500 Subject: [PATCH 07/17] handle empty body --- .../java/dialogue/serde/ConjureBodySerDe.java | 8 ++-- .../dialogue/serde/ConjureBodySerDeTest.java | 20 ++++++++++ .../serde/EndpointErrorTestUtils.java | 18 +++++++++ .../EndpointErrorsConjureBodySerDeTest.java | 37 ++++++++++++++++++- 4 files changed, 79 insertions(+), 4 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 845a0b159..253d04faf 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 @@ -249,14 +249,16 @@ private static final class EncodingDeserializerForEndpointRegistry implements private final ImmutableList> encodings; private final EndpointErrorDecoder endpointErrorDecoder; private final Optional acceptValue; - private final Supplier> emptyInstance; + private final Supplier> emptyInstance; private final TypeMarker token; + private final TypeMarker successTypeMarker; EncodingDeserializerForEndpointRegistry( List encodingsSortedByWeight, EmptyContainerDeserializer empty, TypeMarker token, DeserializerArgs deserializersForEndpoint) { + this.successTypeMarker = deserializersForEndpoint.successType(); this.encodings = encodingsSortedByWeight.stream() .map(encoding -> new EncodingDeserializerContainer<>(encoding, deserializersForEndpoint.successType())) @@ -267,7 +269,7 @@ private static final class EncodingDeserializerForEndpointRegistry implements .filter(encoding -> encoding.supportsContentType("application/json")) .findFirst()); this.token = token; - this.emptyInstance = Suppliers.memoize(() -> empty.tryGetEmptyInstance(token)); + this.emptyInstance = Suppliers.memoize(() -> empty.tryGetEmptyInstance(successTypeMarker)); // Encodings are applied to the accept header in the order of preference based on the provided list. this.acceptValue = Optional.of(encodingsSortedByWeight.stream() .map(Encoding::getContentType) @@ -284,7 +286,7 @@ public T deserialize(Response response) { // TODO(dfox): what if we get a 204 for a non-optional type??? // TODO(dfox): support http200 & body=null // TODO(dfox): what if we were expecting an empty list but got {}? - Optional maybeEmptyInstance = emptyInstance.get(); + Optional maybeEmptyInstance = emptyInstance.get(); if (maybeEmptyInstance.isPresent()) { return maybeEmptyInstance.get(); } diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ConjureBodySerDeTest.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ConjureBodySerDeTest.java index 03879e807..709e905bc 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ConjureBodySerDeTest.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ConjureBodySerDeTest.java @@ -22,11 +22,13 @@ import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.annotation.JsonDeserialize; import com.google.common.collect.ImmutableList; import com.palantir.conjure.java.api.errors.ErrorType; import com.palantir.conjure.java.api.errors.RemoteException; import com.palantir.conjure.java.api.errors.SerializableError; import com.palantir.conjure.java.api.errors.ServiceException; +import com.palantir.conjure.java.dialogue.serde.EndpointErrorTestUtils.CustomNullDeserializer; import com.palantir.conjure.java.serialization.ObjectMappers; import com.palantir.dialogue.BinaryRequestBody; import com.palantir.dialogue.BodySerDe; @@ -69,6 +71,24 @@ public void testRequestOptionalEmpty() { assertThat(value).isEmpty(); } + @Test + public void testRequestCustomEmpty() { + @JsonDeserialize(using = EmptyRecord.EmptyRecordDeserializer.class) + record EmptyRecord() { + private static final class EmptyRecordDeserializer extends CustomNullDeserializer { + @Override + public EmptyRecord create() { + return new EmptyRecord(); + } + } + } + TestResponse response = new TestResponse().code(204); + BodySerDe serializers = conjureBodySerDe("application/json"); + EmptyRecord value = + serializers.deserializer(new TypeMarker() {}).deserialize(response); + assertThat(value).isNotNull(); + } + private ConjureBodySerDe conjureBodySerDe(String... contentTypes) { return new ConjureBodySerDe( Arrays.stream(contentTypes) diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/EndpointErrorTestUtils.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/EndpointErrorTestUtils.java index b53de896e..dc4b3711f 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/EndpointErrorTestUtils.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/EndpointErrorTestUtils.java @@ -17,10 +17,14 @@ package com.palantir.conjure.java.dialogue.serde; import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.core.JsonParser; +import com.fasterxml.jackson.databind.DeserializationContext; +import com.fasterxml.jackson.databind.JsonDeserializer; import com.palantir.conjure.java.api.errors.CheckedServiceException; import com.palantir.dialogue.TypeMarker; import com.palantir.logsafe.Arg; import com.palantir.logsafe.Safe; +import com.palantir.logsafe.exceptions.SafeIllegalStateException; import java.io.ByteArrayInputStream; import java.io.IOException; import java.io.InputStream; @@ -58,6 +62,20 @@ abstract static class EndpointError { } } + abstract static class CustomNullDeserializer extends JsonDeserializer { + public abstract T create(); + + @Override + public T deserialize(JsonParser _parser, DeserializationContext _ctxt) { + throw new SafeIllegalStateException("Attempted to deserialize non-null value as null"); + } + + @Override + public T getNullValue(DeserializationContext _ctxt) { + return create(); + } + } + record ConjureError( @JsonProperty("errorCode") String errorCode, @JsonProperty("errorName") String errorName, 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 858e9df20..51bbd8de8 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 @@ -23,6 +23,7 @@ import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.annotation.JsonDeserialize; import com.google.common.collect.ImmutableList; import com.palantir.conjure.java.api.errors.CheckedServiceException; import com.palantir.conjure.java.api.errors.ErrorType; @@ -30,6 +31,7 @@ import com.palantir.conjure.java.api.errors.SerializableError; import com.palantir.conjure.java.dialogue.serde.EndpointErrorTestUtils.ConjureError; import com.palantir.conjure.java.dialogue.serde.EndpointErrorTestUtils.ContentRecordingJsonDeserializer; +import com.palantir.conjure.java.dialogue.serde.EndpointErrorTestUtils.CustomNullDeserializer; import com.palantir.conjure.java.dialogue.serde.EndpointErrorTestUtils.EndpointError; import com.palantir.conjure.java.dialogue.serde.EndpointErrorTestUtils.TypeReturningStubEncoding; import com.palantir.conjure.java.serialization.ObjectMappers; @@ -59,6 +61,20 @@ public class EndpointErrorsConjureBodySerDeTest { private static final ObjectMapper MAPPER = ObjectMappers.newServerObjectMapper(); + @Generated("by conjure-java") + private sealed interface EmptyBodyEndpointReturnBaseType permits EmptyReturnValue, ErrorReturnValue {} + + @Generated("by conjure-java") + @JsonDeserialize(using = EmptyReturnValue.EmptyReturnValueDeserializer.class) + record EmptyReturnValue() implements EmptyBodyEndpointReturnBaseType { + private static final class EmptyReturnValueDeserializer extends CustomNullDeserializer { + @Override + public EmptyReturnValue create() { + return new EmptyReturnValue(); + } + } + } + @Generated("by conjure-java") private sealed interface EndpointReturnBaseType permits ExpectedReturnValue, ErrorReturnValue {} @@ -80,7 +96,8 @@ record ErrorForEndpointArgs( @JsonProperty("complexArg") @Safe ComplexArg complexArg, @JsonProperty("optionalArg") @Safe Optional optionalArg) {} - static final class ErrorReturnValue extends EndpointError implements EndpointReturnBaseType { + static final class ErrorReturnValue extends EndpointError + implements EndpointReturnBaseType, EmptyBodyEndpointReturnBaseType { @JsonCreator ErrorReturnValue( @JsonProperty("errorCode") String errorCode, @@ -209,6 +226,24 @@ public void testDeserializeExpectedValue() { assertThat(value).isEqualTo(new ExpectedReturnValue(expectedString)); } + @Test + public void testDeserializeEmptyBody() { + // Given + TestResponse response = new TestResponse().code(204); + BodySerDe serializers = conjureBodySerDe("application/json", "text/plain"); + DeserializerArgs deserializerArgs = + DeserializerArgs.builder() + .baseType(new TypeMarker<>() {}) + .success(new TypeMarker() {}) + .error("Default:FailedPrecondition", new TypeMarker() {}) + .build(); + // When + EmptyBodyEndpointReturnBaseType value = + serializers.deserializer(deserializerArgs).deserialize(response); + // Then + assertThat(value).isEqualTo(new EmptyReturnValue()); + } + // Ensure that the supplied JSON encoding is used when available. @Test public void testDeserializeWithCustomEncoding() throws JsonProcessingException { From 7cc4b7f719220e88fe40072ac4f5e915543fa02c Mon Sep 17 00:00:00 2001 From: Pritham Marupaka Date: Wed, 22 Jan 2025 13:20:55 -0500 Subject: [PATCH 08/17] Refactor JacksonEmptyContainerLoader#constructEmptyInstance To construct an empty instance when the `type` is a record with a 0-parameter static factory method annotated with @JsonCreator. --- .../serde/JacksonEmptyContainerLoader.java | 55 +++++++++++-------- .../dialogue/serde/ConjureBodySerDeTest.java | 21 +++---- .../serde/EndpointErrorTestUtils.java | 18 ------ .../EndpointErrorsConjureBodySerDeTest.java | 11 +--- 4 files changed, 47 insertions(+), 58 deletions(-) diff --git a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/JacksonEmptyContainerLoader.java b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/JacksonEmptyContainerLoader.java index 5f03e3399..0506ac7d2 100644 --- a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/JacksonEmptyContainerLoader.java +++ b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/JacksonEmptyContainerLoader.java @@ -64,23 +64,26 @@ private Optional constructEmptyInstance(Type type, TypeMarker origina } // fallback to manual reflection to handle aliases of optionals (and aliases of aliases of optionals) - Method method = getJsonCreatorStaticMethod(type); - if (method != null) { - Type parameterType = method.getParameters()[0].getParameterizedType(); - // Class parameterType = method.getParameters()[0].getType(); - Optional parameter = - constructEmptyInstance(parameterType, originalType, decrement(maxRecursion, originalType)); - - if (parameter.isPresent()) { - return invokeStaticFactoryMethod(method, parameter.get()); - } else { - if (log.isDebugEnabled()) { - log.debug( - "Found a @JsonCreator, but couldn't construct the parameter", - SafeArg.of("type", type), - SafeArg.of("parameter", parameter)); + MethodAndMaybeParameter methodAndMaybeParameter = getJsonCreatorStaticMethodAndMaybeParam(type); + if (methodAndMaybeParameter != null) { + Method method = methodAndMaybeParameter.method(); + Type parameterType = methodAndMaybeParameter.parameterType(); + if (parameterType != null) { + Optional parameter = + constructEmptyInstance(parameterType, originalType, decrement(maxRecursion, originalType)); + if (parameter.isPresent()) { + return invokeStaticFactoryMethod(method, parameter.get()); + } else { + if (log.isDebugEnabled()) { + log.debug( + "Found a @JsonCreator, but couldn't construct the parameter", + SafeArg.of("type", type), + SafeArg.of("parameter", parameter)); + } + return Optional.empty(); } - return Optional.empty(); + } else { + return invokeStaticFactoryMethod(method, null); } } @@ -138,23 +141,29 @@ private Optional jacksonDeserializeFromNull(Type type) { // doesn't attempt to handle multiple @JsonCreator methods on one class @Nullable - private static Method getJsonCreatorStaticMethod(Type type) { + private static MethodAndMaybeParameter getJsonCreatorStaticMethodAndMaybeParam(Type type) { if (type instanceof Class) { Class clazz = (Class) type; for (Method method : clazz.getMethods()) { + int parameterCount = method.getParameterCount(); if (Modifier.isStatic(method.getModifiers()) - && method.getParameterCount() == 1 - && method.getAnnotation(JsonCreator.class) != null) { - return method; + && method.getAnnotation(JsonCreator.class) != null + && parameterCount < 2) { + if (parameterCount == 0) { + return new MethodAndMaybeParameter(method, null); + } else if (parameterCount == 1) { + Type parameterType = method.getParameters()[0].getParameterizedType(); + return new MethodAndMaybeParameter(method, parameterType); + } } } } return null; } - private static Optional invokeStaticFactoryMethod(Method method, Object parameter) { + private static Optional invokeStaticFactoryMethod(Method method, @Nullable Object parameter) { try { - return Optional.ofNullable(method.invoke(null, parameter)); + return Optional.ofNullable(parameter == null ? method.invoke(null) : method.invoke(null, parameter)); } catch (IllegalAccessException | InvocationTargetException e) { if (log.isDebugEnabled()) { log.debug("Reflection instantiation failed", e); @@ -162,4 +171,6 @@ private static Optional invokeStaticFactoryMethod(Method method, Object return Optional.empty(); } } + + private record MethodAndMaybeParameter(Method method, @Nullable Type parameterType) {} } diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ConjureBodySerDeTest.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ConjureBodySerDeTest.java index 709e905bc..b0985ec47 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ConjureBodySerDeTest.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ConjureBodySerDeTest.java @@ -20,18 +20,18 @@ import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; -import com.fasterxml.jackson.databind.annotation.JsonDeserialize; import com.google.common.collect.ImmutableList; import com.palantir.conjure.java.api.errors.ErrorType; import com.palantir.conjure.java.api.errors.RemoteException; import com.palantir.conjure.java.api.errors.SerializableError; import com.palantir.conjure.java.api.errors.ServiceException; -import com.palantir.conjure.java.dialogue.serde.EndpointErrorTestUtils.CustomNullDeserializer; import com.palantir.conjure.java.serialization.ObjectMappers; import com.palantir.dialogue.BinaryRequestBody; import com.palantir.dialogue.BodySerDe; +import com.palantir.dialogue.DeserializerArgs; import com.palantir.dialogue.RequestBody; import com.palantir.dialogue.TestResponse; import com.palantir.dialogue.TypeMarker; @@ -73,19 +73,20 @@ public void testRequestOptionalEmpty() { @Test public void testRequestCustomEmpty() { - @JsonDeserialize(using = EmptyRecord.EmptyRecordDeserializer.class) record EmptyRecord() { - private static final class EmptyRecordDeserializer extends CustomNullDeserializer { - @Override - public EmptyRecord create() { - return new EmptyRecord(); - } + @JsonCreator + public static EmptyRecord create() { + return new EmptyRecord(); } } TestResponse response = new TestResponse().code(204); BodySerDe serializers = conjureBodySerDe("application/json"); - EmptyRecord value = - serializers.deserializer(new TypeMarker() {}).deserialize(response); + EmptyRecord value = serializers + .deserializer(DeserializerArgs.builder() + .baseType(new TypeMarker<>() {}) + .success(new TypeMarker<>() {}) + .build()) + .deserialize(response); assertThat(value).isNotNull(); } diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/EndpointErrorTestUtils.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/EndpointErrorTestUtils.java index dc4b3711f..b53de896e 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/EndpointErrorTestUtils.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/EndpointErrorTestUtils.java @@ -17,14 +17,10 @@ package com.palantir.conjure.java.dialogue.serde; import com.fasterxml.jackson.annotation.JsonProperty; -import com.fasterxml.jackson.core.JsonParser; -import com.fasterxml.jackson.databind.DeserializationContext; -import com.fasterxml.jackson.databind.JsonDeserializer; import com.palantir.conjure.java.api.errors.CheckedServiceException; import com.palantir.dialogue.TypeMarker; import com.palantir.logsafe.Arg; import com.palantir.logsafe.Safe; -import com.palantir.logsafe.exceptions.SafeIllegalStateException; import java.io.ByteArrayInputStream; import java.io.IOException; import java.io.InputStream; @@ -62,20 +58,6 @@ abstract static class EndpointError { } } - abstract static class CustomNullDeserializer extends JsonDeserializer { - public abstract T create(); - - @Override - public T deserialize(JsonParser _parser, DeserializationContext _ctxt) { - throw new SafeIllegalStateException("Attempted to deserialize non-null value as null"); - } - - @Override - public T getNullValue(DeserializationContext _ctxt) { - return create(); - } - } - record ConjureError( @JsonProperty("errorCode") String errorCode, @JsonProperty("errorName") String errorName, 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 51bbd8de8..dcdfd90b0 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 @@ -23,7 +23,6 @@ import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; -import com.fasterxml.jackson.databind.annotation.JsonDeserialize; import com.google.common.collect.ImmutableList; import com.palantir.conjure.java.api.errors.CheckedServiceException; import com.palantir.conjure.java.api.errors.ErrorType; @@ -31,7 +30,6 @@ import com.palantir.conjure.java.api.errors.SerializableError; import com.palantir.conjure.java.dialogue.serde.EndpointErrorTestUtils.ConjureError; import com.palantir.conjure.java.dialogue.serde.EndpointErrorTestUtils.ContentRecordingJsonDeserializer; -import com.palantir.conjure.java.dialogue.serde.EndpointErrorTestUtils.CustomNullDeserializer; import com.palantir.conjure.java.dialogue.serde.EndpointErrorTestUtils.EndpointError; import com.palantir.conjure.java.dialogue.serde.EndpointErrorTestUtils.TypeReturningStubEncoding; import com.palantir.conjure.java.serialization.ObjectMappers; @@ -65,13 +63,10 @@ public class EndpointErrorsConjureBodySerDeTest { private sealed interface EmptyBodyEndpointReturnBaseType permits EmptyReturnValue, ErrorReturnValue {} @Generated("by conjure-java") - @JsonDeserialize(using = EmptyReturnValue.EmptyReturnValueDeserializer.class) record EmptyReturnValue() implements EmptyBodyEndpointReturnBaseType { - private static final class EmptyReturnValueDeserializer extends CustomNullDeserializer { - @Override - public EmptyReturnValue create() { - return new EmptyReturnValue(); - } + @JsonCreator + public static EmptyReturnValue create() { + return new EmptyReturnValue(); } } From 01a1589e67b1ed28dfb6841357af2f01ce0102b7 Mon Sep 17 00:00:00 2001 From: Pritham Marupaka Date: Thu, 23 Jan 2025 00:15:02 -0500 Subject: [PATCH 09/17] Support inputStreamDeserializer and optionalInputStreamDeserializer --- .../java/dialogue/serde/ConjureBodySerDe.java | 146 ++++++++++++++---- .../EndpointErrorsConjureBodySerDeTest.java | 52 ++++++- .../java/com/palantir/dialogue/BodySerDe.java | 4 + 3 files changed, 165 insertions(+), 37 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 253d04faf..f480f51a6 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 @@ -47,8 +47,10 @@ import java.util.Comparator; import java.util.List; import java.util.Optional; +import java.util.function.Function; import java.util.function.Supplier; import java.util.stream.Collectors; +import javax.annotation.Nullable; /** Package private internal API. */ final class ConjureBodySerDe implements BodySerDe { @@ -60,7 +62,7 @@ final class ConjureBodySerDe implements BodySerDe { private final Deserializer> optionalBinaryInputStreamDeserializer; private final Deserializer emptyBodyDeserializer; private final LoadingCache> serializers; - private final LoadingCache> deserializers; + private final LoadingCache> deserializers; private final EmptyContainerDeserializer emptyContainerDeserializer; /** @@ -77,7 +79,7 @@ final class ConjureBodySerDe implements BodySerDe { Preconditions.checkArgument(encodings.size() > 0, "At least one Encoding is required"); this.defaultEncoding = encodings.get(0).encoding(); this.emptyContainerDeserializer = emptyContainerDeserializer; - this.binaryInputStreamDeserializer = new EncodingDeserializerForEndpointRegistry<>( + this.binaryInputStreamDeserializer = EncodingDeserializerForEndpointRegistry.create( ImmutableList.of(BinaryEncoding.INSTANCE), emptyContainerDeserializer, BinaryEncoding.MARKER, @@ -85,7 +87,7 @@ final class ConjureBodySerDe implements BodySerDe { .baseType(BinaryEncoding.MARKER) .success(BinaryEncoding.MARKER) .build()); - this.optionalBinaryInputStreamDeserializer = new EncodingDeserializerForEndpointRegistry<>( + this.optionalBinaryInputStreamDeserializer = EncodingDeserializerForEndpointRegistry.create( ImmutableList.of(BinaryEncoding.INSTANCE), emptyContainerDeserializer, BinaryEncoding.OPTIONAL_MARKER, @@ -101,8 +103,8 @@ final class ConjureBodySerDe implements BodySerDe { this.deserializers = Caffeine.from(cacheSpec).build(type -> buildCacheEntry(TypeMarker.of(type))); } - private EncodingDeserializerForEndpointRegistry buildCacheEntry(TypeMarker typeMarker) { - return new EncodingDeserializerForEndpointRegistry<>( + private EncodingDeserializerForEndpointRegistry buildCacheEntry(TypeMarker typeMarker) { + return EncodingDeserializerForEndpointRegistry.create( encodingsSortedByWeight, emptyContainerDeserializer, typeMarker, @@ -143,11 +145,8 @@ public Deserializer deserializer(TypeMarker token) { @Override @SuppressWarnings("unchecked") public Deserializer deserializer(DeserializerArgs deserializerArgs) { - return new EncodingDeserializerForEndpointRegistry<>( - encodingsSortedByWeight, - emptyContainerDeserializer, - (TypeMarker) deserializerArgs.baseType(), - deserializerArgs); + return EncodingDeserializerForEndpointRegistry.create( + encodingsSortedByWeight, emptyContainerDeserializer, deserializerArgs.baseType(), deserializerArgs); } @Override @@ -160,11 +159,34 @@ public Deserializer inputStreamDeserializer() { return binaryInputStreamDeserializer; } + @Override + @SuppressWarnings("unchecked") + public Deserializer inputStreamDeserializer(DeserializerArgs deserializerArgs) { + return new EncodingDeserializerForEndpointRegistry<>( + encodingsSortedByWeight, + emptyContainerDeserializer, + deserializerArgs.baseType(), + deserializerArgs, + BinaryEncoding.MARKER, + is -> createSuccessType(deserializerArgs.successType(), is)); + } + @Override public Deserializer> optionalInputStreamDeserializer() { return optionalBinaryInputStreamDeserializer; } + @Override + public Deserializer optionalInputStreamDeserializer(DeserializerArgs deserializerArgs) { + return new EncodingDeserializerForEndpointRegistry<>( + encodingsSortedByWeight, + emptyContainerDeserializer, + deserializerArgs.baseType(), + deserializerArgs, + BinaryEncoding.OPTIONAL_MARKER, + ois -> createSuccessType(deserializerArgs.successType(), ois)); + } + @Override public RequestBody serialize(BinaryRequestBody value) { Preconditions.checkNotNull(value, "A BinaryRequestBody value is required"); @@ -244,24 +266,41 @@ private static final class EncodingSerializerContainer { } } - private static final class EncodingDeserializerForEndpointRegistry implements Deserializer { + private static final class EncodingDeserializerForEndpointRegistry implements Deserializer { private static final SafeLogger log = SafeLoggerFactory.get(EncodingDeserializerForEndpointRegistry.class); - private final ImmutableList> encodings; + private final ImmutableList> encodings; private final EndpointErrorDecoder endpointErrorDecoder; private final Optional acceptValue; private final Supplier> emptyInstance; private final TypeMarker token; private final TypeMarker successTypeMarker; + private final @Nullable Function transform; - EncodingDeserializerForEndpointRegistry( + @SuppressWarnings("unchecked") + static EncodingDeserializerForEndpointRegistry create( List encodingsSortedByWeight, EmptyContainerDeserializer empty, TypeMarker token, DeserializerArgs deserializersForEndpoint) { + return new EncodingDeserializerForEndpointRegistry<>( + encodingsSortedByWeight, + empty, + token, + deserializersForEndpoint, + (TypeMarker) deserializersForEndpoint.successType(), + null); + } + + EncodingDeserializerForEndpointRegistry( + List encodingsSortedByWeight, + EmptyContainerDeserializer empty, + TypeMarker token, + DeserializerArgs deserializersForEndpoint, + TypeMarker intermediateResult, + @Nullable Function transform) { this.successTypeMarker = deserializersForEndpoint.successType(); this.encodings = encodingsSortedByWeight.stream() - .map(encoding -> - new EncodingDeserializerContainer<>(encoding, deserializersForEndpoint.successType())) + .map(encoding -> new EncodingDeserializerContainer<>(encoding, intermediateResult)) .collect(ImmutableList.toImmutableList()); this.endpointErrorDecoder = new EndpointErrorDecoder<>( deserializersForEndpoint.errorNameToTypeMarker(), @@ -270,13 +309,14 @@ private static final class EncodingDeserializerForEndpointRegistry implements .findFirst()); this.token = token; this.emptyInstance = Suppliers.memoize(() -> empty.tryGetEmptyInstance(successTypeMarker)); - // Encodings are applied to the accept header in the order of preference based on the provided list. this.acceptValue = Optional.of(encodingsSortedByWeight.stream() .map(Encoding::getContentType) .collect(Collectors.joining(", "))); + this.transform = transform; } @Override + @SuppressWarnings("unchecked") public T deserialize(Response response) { boolean closeResponse = true; try { @@ -300,11 +340,21 @@ public T deserialize(Response response) { "Response is missing Content-Type header", SafeArg.of("received", response.headers().keySet())); } - Encoding.Deserializer deserializer = getResponseDeserializer(contentType.get()); - T deserialized = deserializer.deserialize(response.body()); + Encoding.Deserializer deserializer = getResponseDeserializer(contentType.get()); + S deserialized = deserializer.deserialize(response.body()); // deserializer has taken on responsibility for closing the response body closeResponse = false; - return deserialized; + if (transform == null) { + return (T) deserialized; + } + T responseValue = transform.apply(deserialized); + if (responseValue == null) { + throw new SafeRuntimeException( + "Failed to create success type", + SafeArg.of("type", token), + SafeArg.of("deserialized", deserialized)); + } + return responseValue; } catch (IOException e) { throw new SafeRuntimeException( "Failed to deserialize response stream", @@ -326,9 +376,9 @@ public Optional accepts() { /** Returns the {@link EncodingDeserializerContainer} to use to deserialize the request body. */ @SuppressWarnings("ForLoopReplaceableByForEach") // performance sensitive code avoids iterator allocation - Encoding.Deserializer getResponseDeserializer(String contentType) { + Encoding.Deserializer getResponseDeserializer(String contentType) { for (int i = 0; i < encodings.size(); i++) { - EncodingDeserializerContainer container = encodings.get(i); + EncodingDeserializerContainer container = encodings.get(i); if (container.encoding.supportsContentType(contentType)) { return container.deserializer; } @@ -336,20 +386,17 @@ Encoding.Deserializer getResponseDeserializer(String contentType) { return throwingDeserializer(contentType); } - private Encoding.Deserializer throwingDeserializer(String contentType) { - return new Encoding.Deserializer() { - @Override - public T deserialize(InputStream input) { - try { - input.close(); - } catch (RuntimeException | IOException e) { - log.warn("Failed to close InputStream", e); - } - throw new SafeRuntimeException( - "Unsupported Content-Type", - SafeArg.of("received", contentType), - SafeArg.of("supportedEncodings", encodings)); + private Encoding.Deserializer throwingDeserializer(String contentType) { + return input -> { + try { + input.close(); + } catch (RuntimeException | IOException e) { + log.warn("Failed to close InputStream", e); } + throw new SafeRuntimeException( + "Unsupported Content-Type", + SafeArg.of("received", contentType), + SafeArg.of("supportedEncodings", encodings)); }; } } @@ -400,4 +447,35 @@ public String toString() { return "EmptyBodyDeserializer{}"; } } + + @SuppressWarnings("unchecked") + @Nullable + private static T createSuccessType(TypeMarker successT, InputStream inputStream) { + if (successT.getType() instanceof Class) { + try { + return (T) ((Class) successT.getType()) + .getConstructor(InputStream.class) + .newInstance(inputStream); + } catch (ReflectiveOperationException e) { + throw new SafeRuntimeException("Failed to create success type", e); + } + } + return null; + } + + @SuppressWarnings("unchecked") + @Nullable + private static T createSuccessType(TypeMarker successT, Optional optionalInputStream) { + if (successT.getType() instanceof Class) { + try { + return (T) ((Class) successT.getType()) + // TODO(pm): check that the param is Optional + .getConstructor(Optional.class) + .newInstance(optionalInputStream); + } catch (ReflectiveOperationException e) { + throw new SafeRuntimeException("Failed to create success type", e); + } + } + return null; + } } 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 dcdfd90b0..447cfc449 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 @@ -24,6 +24,7 @@ import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.collect.ImmutableList; +import com.google.errorprone.annotations.MustBeClosed; import com.palantir.conjure.java.api.errors.CheckedServiceException; import com.palantir.conjure.java.api.errors.ErrorType; import com.palantir.conjure.java.api.errors.RemoteException; @@ -43,6 +44,7 @@ import com.palantir.logsafe.Unsafe; import com.palantir.logsafe.UnsafeArg; import java.io.IOException; +import java.io.InputStream; import java.util.Arrays; import java.util.List; import java.util.Optional; @@ -73,6 +75,9 @@ public static EmptyReturnValue create() { @Generated("by conjure-java") private sealed interface EndpointReturnBaseType permits ExpectedReturnValue, ErrorReturnValue {} + @Generated("by conjure-java") + private sealed interface EndpointBinaryReturnBaseType permits BinaryReturnValue, ErrorReturnValue {} + @Generated("by conjure-java") record ExpectedReturnValue(String value) implements EndpointReturnBaseType { @JsonCreator @@ -81,6 +86,13 @@ public static ExpectedReturnValue create(String value) { } } + @Generated("by conjure-java") + record BinaryReturnValue(@MustBeClosed InputStream value) implements EndpointBinaryReturnBaseType { + public BinaryReturnValue { + Preconditions.checkArgumentNotNull(value, "value cannot be null"); + } + } + @Generated("by conjure-java") record ComplexArg(int foo, String bar) {} @@ -92,7 +104,9 @@ record ErrorForEndpointArgs( @JsonProperty("optionalArg") @Safe Optional optionalArg) {} static final class ErrorReturnValue extends EndpointError - implements EndpointReturnBaseType, EmptyBodyEndpointReturnBaseType { + implements EndpointErrorsConjureBodySerDeTest.EndpointReturnBaseType, + EndpointErrorsConjureBodySerDeTest.EmptyBodyEndpointReturnBaseType, + EndpointErrorsConjureBodySerDeTest.EndpointBinaryReturnBaseType { @JsonCreator ErrorReturnValue( @JsonProperty("errorCode") String errorCode, @@ -137,7 +151,7 @@ public void testDeserializeCustomError(String supportedContentType) throws IOExc BodySerDe serializers = conjureBodySerDe(supportedContentType); DeserializerArgs deserializerArgs = DeserializerArgs.builder() .baseType(new TypeMarker<>() {}) - .success(new TypeMarker() {}) + .success(new TypeMarker() {}) .error("Default:FailedPrecondition", new TypeMarker() {}) .build(); @@ -221,6 +235,38 @@ public void testDeserializeExpectedValue() { assertThat(value).isEqualTo(new ExpectedReturnValue(expectedString)); } + @Test + public void testDeserializeBinaryValue() { + // Given + byte[] is = new byte[] {1, 2, 3}; + TestResponse response = + new TestResponse(is).contentType("application/octet-stream").code(200); + // BodySerDe serializers = conjureBodySerDe("application/octet-stream", "text/plain"); + + BodySerDe serializers = new ConjureBodySerDe( + ImmutableList.of(WeightedEncoding.of(BinaryEncoding.INSTANCE)), + Encodings.emptyContainerDeserializer(), + DefaultConjureRuntime.DEFAULT_SERDE_CACHE_SPEC); + + DeserializerArgs deserializerArgs = + DeserializerArgs.builder() + .baseType(new TypeMarker<>() {}) + .success(new TypeMarker() {}) + .error("Default:FailedPrecondition", new TypeMarker() {}) + .build(); + // When + EndpointBinaryReturnBaseType value = + serializers.inputStreamDeserializer(deserializerArgs).deserialize(response); + // Then + assertThat(value).isInstanceOfSatisfying(BinaryReturnValue.class, binaryReturnValue -> { + try { + assertThat(binaryReturnValue.value.readAllBytes()).isEqualTo(is); + } catch (IOException e) { + throw new RuntimeException(e); + } + }); + } + @Test public void testDeserializeEmptyBody() { // Given @@ -229,7 +275,7 @@ public void testDeserializeEmptyBody() { DeserializerArgs deserializerArgs = DeserializerArgs.builder() .baseType(new TypeMarker<>() {}) - .success(new TypeMarker() {}) + .success(new TypeMarker() {}) .error("Default:FailedPrecondition", new TypeMarker() {}) .build(); // When diff --git a/dialogue-target/src/main/java/com/palantir/dialogue/BodySerDe.java b/dialogue-target/src/main/java/com/palantir/dialogue/BodySerDe.java index f7c69d264..6529e9662 100644 --- a/dialogue-target/src/main/java/com/palantir/dialogue/BodySerDe.java +++ b/dialogue-target/src/main/java/com/palantir/dialogue/BodySerDe.java @@ -43,9 +43,13 @@ public interface BodySerDe { */ Deserializer inputStreamDeserializer(); + Deserializer inputStreamDeserializer(DeserializerArgs deserializerArgs); + /** Same as {@link #inputStreamDeserializer()} with support for 204 responses. */ Deserializer> optionalInputStreamDeserializer(); + Deserializer optionalInputStreamDeserializer(DeserializerArgs deserializerArgs); + /** Serializes a {@link BinaryRequestBody} to 
application/octet-stream
. */ RequestBody serialize(BinaryRequestBody value); } From e0f5d259001bea91788b34ebf1e41a28fe8eb6a3 Mon Sep 17 00:00:00 2001 From: Pritham Marupaka Date: Thu, 23 Jan 2025 13:56:55 -0500 Subject: [PATCH 10/17] wip: review changes --- .palantir/revapi.yml | 10 ++ .../java/dialogue/serde/ConjureBodySerDe.java | 92 +++++++----- .../EndpointErrorsConjureBodySerDeTest.java | 134 +++++++++++++++--- 3 files changed, 185 insertions(+), 51 deletions(-) diff --git a/.palantir/revapi.yml b/.palantir/revapi.yml index ff2dd1cc5..d05c19c62 100644 --- a/.palantir/revapi.yml +++ b/.palantir/revapi.yml @@ -317,3 +317,13 @@ acceptedBreaks: new: "method com.palantir.dialogue.Deserializer com.palantir.dialogue.BodySerDe::deserializer(com.palantir.dialogue.DeserializerArgs)" justification: "Adding a new method to create deserializers in support of endpoint\ \ associated error deserialization" + "4.8.0-rc1": + com.palantir.dialogue:dialogue-target: + - code: "java.method.addedToInterface" + new: "method com.palantir.dialogue.Deserializer com.palantir.dialogue.BodySerDe::inputStreamDeserializer(com.palantir.dialogue.DeserializerArgs)" + justification: "Adding a new method to create deserializers in support of endpoint\ + \ associated error deserialization" + - code: "java.method.addedToInterface" + new: "method com.palantir.dialogue.Deserializer com.palantir.dialogue.BodySerDe::optionalInputStreamDeserializer(com.palantir.dialogue.DeserializerArgs)" + justification: "Adding a new method to create deserializers in support of endpoint\ + \ associated error deserialization" 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 f480f51a6..b8b0b907d 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 @@ -41,6 +41,8 @@ import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; +import java.lang.reflect.Constructor; +import java.lang.reflect.ParameterizedType; import java.lang.reflect.Type; import java.util.ArrayList; import java.util.Collections; @@ -168,7 +170,7 @@ public Deserializer inputStreamDeserializer(DeserializerArgs deseriali deserializerArgs.baseType(), deserializerArgs, BinaryEncoding.MARKER, - is -> createSuccessType(deserializerArgs.successType(), is)); + (Function) createSuccessTypeFunctionForInputStream(deserializerArgs.successType())); } @Override @@ -177,6 +179,7 @@ public Deserializer> optionalInputStreamDeserializer() { } @Override + @SuppressWarnings("unchecked") public Deserializer optionalInputStreamDeserializer(DeserializerArgs deserializerArgs) { return new EncodingDeserializerForEndpointRegistry<>( encodingsSortedByWeight, @@ -184,7 +187,8 @@ public Deserializer optionalInputStreamDeserializer(DeserializerArgs d deserializerArgs.baseType(), deserializerArgs, BinaryEncoding.OPTIONAL_MARKER, - ois -> createSuccessType(deserializerArgs.successType(), ois)); + (Function, T>) + createSuccessTypeFunctionForOptionalInputStream(deserializerArgs.successType())); } @Override @@ -271,9 +275,8 @@ private static final class EncodingDeserializerForEndpointRegistry impleme private final ImmutableList> encodings; private final EndpointErrorDecoder endpointErrorDecoder; private final Optional acceptValue; - private final Supplier> emptyInstance; + private final Supplier> emptyInstance; private final TypeMarker token; - private final TypeMarker successTypeMarker; private final @Nullable Function transform; @SuppressWarnings("unchecked") @@ -298,7 +301,6 @@ static EncodingDeserializerForEndpointRegistry create( DeserializerArgs deserializersForEndpoint, TypeMarker intermediateResult, @Nullable Function transform) { - this.successTypeMarker = deserializersForEndpoint.successType(); this.encodings = encodingsSortedByWeight.stream() .map(encoding -> new EncodingDeserializerContainer<>(encoding, intermediateResult)) .collect(ImmutableList.toImmutableList()); @@ -308,7 +310,7 @@ static EncodingDeserializerForEndpointRegistry create( .filter(encoding -> encoding.supportsContentType("application/json")) .findFirst()); this.token = token; - this.emptyInstance = Suppliers.memoize(() -> empty.tryGetEmptyInstance(successTypeMarker)); + this.emptyInstance = Suppliers.memoize(() -> empty.tryGetEmptyInstance(intermediateResult)); this.acceptValue = Optional.of(encodingsSortedByWeight.stream() .map(Encoding::getContentType) .collect(Collectors.joining(", "))); @@ -326,9 +328,12 @@ public T deserialize(Response response) { // TODO(dfox): what if we get a 204 for a non-optional type??? // TODO(dfox): support http200 & body=null // TODO(dfox): what if we were expecting an empty list but got {}? - Optional maybeEmptyInstance = emptyInstance.get(); + Optional maybeEmptyInstance = emptyInstance.get(); if (maybeEmptyInstance.isPresent()) { - return maybeEmptyInstance.get(); + if (transform == null) { + return (T) maybeEmptyInstance.get(); + } + return transform.apply(maybeEmptyInstance.get()); } throw new SafeRuntimeException( "Unable to deserialize non-optional response type from 204", SafeArg.of("type", token)); @@ -347,14 +352,7 @@ public T deserialize(Response response) { if (transform == null) { return (T) deserialized; } - T responseValue = transform.apply(deserialized); - if (responseValue == null) { - throw new SafeRuntimeException( - "Failed to create success type", - SafeArg.of("type", token), - SafeArg.of("deserialized", deserialized)); - } - return responseValue; + return transform.apply(deserialized); } catch (IOException e) { throw new SafeRuntimeException( "Failed to deserialize response stream", @@ -449,33 +447,57 @@ public String toString() { } @SuppressWarnings("unchecked") - @Nullable - private static T createSuccessType(TypeMarker successT, InputStream inputStream) { - if (successT.getType() instanceof Class) { + private static Function createSuccessTypeFunctionForInputStream(TypeMarker successT) { + return successTypeCreatorFactory(successT, successType -> { try { - return (T) ((Class) successT.getType()) - .getConstructor(InputStream.class) - .newInstance(inputStream); - } catch (ReflectiveOperationException e) { - throw new SafeRuntimeException("Failed to create success type", e); + return ((Class) successType.getType()).getConstructor(InputStream.class); + } catch (ReflectiveOperationException ex) { + throw new SafeRuntimeException("Failed to create success type", ex); } - } - return null; + }); } @SuppressWarnings("unchecked") - @Nullable - private static T createSuccessType(TypeMarker successT, Optional optionalInputStream) { - if (successT.getType() instanceof Class) { + private static Function, T> createSuccessTypeFunctionForOptionalInputStream( + TypeMarker successT) { + return successTypeCreatorFactory(successT, successType -> { try { - return (T) ((Class) successT.getType()) - // TODO(pm): check that the param is Optional - .getConstructor(Optional.class) - .newInstance(optionalInputStream); + Class clazz = (Class) successType.getType(); + for (Constructor ctor : clazz.getConstructors()) { + if (ctor.getParameterCount() != 1) { + continue; + } + Type paramType = ctor.getGenericParameterTypes()[0]; + if (paramType instanceof ParameterizedType parameterizedType) { + if (parameterizedType.getRawType().equals(Optional.class) + && parameterizedType.getActualTypeArguments()[0].equals(InputStream.class)) { + return (Constructor) ctor; + } + } + } + } catch (SecurityException ex) { + throw new SafeRuntimeException("Failed to create success type", ex); + } + throw new SafeRuntimeException( + "Failed to create success type. Could not find constructor with Optional parameter"); + }); + } + + private static Function successTypeCreatorFactory( + TypeMarker successT, Function, Constructor> ctorExtractor) { + if (!(successT.getType() instanceof Class)) { + throw new SafeRuntimeException("Failed to create success type", SafeArg.of("type", successT)); + } + Constructor ctor = ctorExtractor.apply(successT); + if (ctor == null) { + throw new SafeRuntimeException("Failed to create success type", SafeArg.of("type", successT)); + } + return ctorParam -> { + try { + return ctor.newInstance(ctorParam); } catch (ReflectiveOperationException e) { throw new SafeRuntimeException("Failed to create success type", e); } - } - return null; + }; } } 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 447cfc449..8661981dd 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 @@ -48,12 +48,18 @@ import java.util.Arrays; import java.util.List; import java.util.Optional; +import java.util.function.Supplier; +import java.util.stream.Stream; import javax.annotation.Nullable; import javax.annotation.processing.Generated; import org.assertj.core.api.InstanceOfAssertFactories; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.api.extension.ExtensionContext; import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.ArgumentsProvider; +import org.junit.jupiter.params.provider.ArgumentsSource; import org.junit.jupiter.params.provider.ValueSource; import org.mockito.junit.jupiter.MockitoExtension; @@ -78,6 +84,9 @@ private sealed interface EndpointReturnBaseType permits ExpectedReturnValue, Err @Generated("by conjure-java") private sealed interface EndpointBinaryReturnBaseType permits BinaryReturnValue, ErrorReturnValue {} + @Generated("by conjure-java") + private sealed interface EndpointOptionalBinaryReturnBaseType permits OptionalBinaryReturnValue, ErrorReturnValue {} + @Generated("by conjure-java") record ExpectedReturnValue(String value) implements EndpointReturnBaseType { @JsonCreator @@ -93,6 +102,13 @@ record BinaryReturnValue(@MustBeClosed InputStream value) implements EndpointBin } } + @Generated("by conjure-java") + record OptionalBinaryReturnValue(Optional value) implements EndpointOptionalBinaryReturnBaseType { + public OptionalBinaryReturnValue { + Preconditions.checkArgumentNotNull(value, "value cannot be null"); + } + } + @Generated("by conjure-java") record ComplexArg(int foo, String bar) {} @@ -106,7 +122,8 @@ record ErrorForEndpointArgs( static final class ErrorReturnValue extends EndpointError implements EndpointErrorsConjureBodySerDeTest.EndpointReturnBaseType, EndpointErrorsConjureBodySerDeTest.EmptyBodyEndpointReturnBaseType, - EndpointErrorsConjureBodySerDeTest.EndpointBinaryReturnBaseType { + EndpointErrorsConjureBodySerDeTest.EndpointBinaryReturnBaseType, + EndpointErrorsConjureBodySerDeTest.EndpointOptionalBinaryReturnBaseType { @JsonCreator ErrorReturnValue( @JsonProperty("errorCode") String errorCode, @@ -235,12 +252,13 @@ public void testDeserializeExpectedValue() { assertThat(value).isEqualTo(new ExpectedReturnValue(expectedString)); } - @Test - public void testDeserializeBinaryValue() { + @ParameterizedTest + @ArgumentsSource(BinaryBodyArgumentsProvider.class) + public void testDeserializeBinaryValue(byte[] binaryBody) { // Given - byte[] is = new byte[] {1, 2, 3}; - TestResponse response = - new TestResponse(is).contentType("application/octet-stream").code(200); + TestResponse response = new TestResponse(binaryBody) + .contentType("application/octet-stream") + .code(200); // BodySerDe serializers = conjureBodySerDe("application/octet-stream", "text/plain"); BodySerDe serializers = new ConjureBodySerDe( @@ -259,14 +277,84 @@ public void testDeserializeBinaryValue() { serializers.inputStreamDeserializer(deserializerArgs).deserialize(response); // Then assertThat(value).isInstanceOfSatisfying(BinaryReturnValue.class, binaryReturnValue -> { - try { - assertThat(binaryReturnValue.value.readAllBytes()).isEqualTo(is); - } catch (IOException e) { - throw new RuntimeException(e); - } + assertThat(EndpointErrorsConjureBodySerDeTest.readAllBytesUnchecked(binaryReturnValue::value)) + .isEqualTo(binaryBody); + }); + } + + @ParameterizedTest + @ArgumentsSource(BinaryBodyArgumentsProvider.class) + public void testDeserializeOptionalBinaryValuePresent(byte[] binaryBody) { + // Given + TestResponse response = new TestResponse(binaryBody) + .contentType("application/octet-stream") + .code(200); + // BodySerDe serializers = conjureBodySerDe("application/octet-stream", "text/plain"); + + BodySerDe serializers = new ConjureBodySerDe( + ImmutableList.of(WeightedEncoding.of(BinaryEncoding.INSTANCE)), + Encodings.emptyContainerDeserializer(), + DefaultConjureRuntime.DEFAULT_SERDE_CACHE_SPEC); + + DeserializerArgs deserializerArgs = + DeserializerArgs.builder() + .baseType(new TypeMarker<>() {}) + .success(new TypeMarker() {}) + .error("Default:FailedPrecondition", new TypeMarker() {}) + .build(); + // When + EndpointOptionalBinaryReturnBaseType value = + serializers.optionalInputStreamDeserializer(deserializerArgs).deserialize(response); + // Then + assertThat(value).isInstanceOfSatisfying(OptionalBinaryReturnValue.class, optionalBinaryReturnValue -> { + assertThat(optionalBinaryReturnValue.value()).isPresent(); + assertThat(EndpointErrorsConjureBodySerDeTest.readAllBytesUnchecked(optionalBinaryReturnValue.value()::get)) + .isEqualTo(binaryBody); }); } + @Test + public void testDeserializeOptionalBinaryValueError() throws JsonProcessingException { + // Given + TestEndpointError errorThrownByEndpoint = + new TestEndpointError("value", "unsafeValue", new ComplexArg(1, "bar"), Optional.of(2), null); + String responseBody = + MAPPER.writeValueAsString(ConjureError.fromCheckedServiceException(errorThrownByEndpoint)); + + TestResponse response = TestResponse.withBody(responseBody) + .contentType("application/json") + .code(500); + + BodySerDe serializers = new ConjureBodySerDe( + ImmutableList.of(WeightedEncoding.of(BinaryEncoding.INSTANCE)), + Encodings.emptyContainerDeserializer(), + DefaultConjureRuntime.DEFAULT_SERDE_CACHE_SPEC); + + DeserializerArgs deserializerArgs = + DeserializerArgs.builder() + .baseType(new TypeMarker<>() {}) + .success(new TypeMarker() {}) + .error("Default:FailedPrecondition", new TypeMarker() {}) + .build(); + // When + EndpointOptionalBinaryReturnBaseType value = + serializers.optionalInputStreamDeserializer(deserializerArgs).deserialize(response); + // Then + ErrorReturnValue expectedErrorForEndpoint = new ErrorReturnValue( + ErrorType.FAILED_PRECONDITION.code().name(), + ErrorType.FAILED_PRECONDITION.name(), + errorThrownByEndpoint.getErrorInstanceId(), + new ErrorForEndpointArgs("value", "unsafeValue", new ComplexArg(1, "bar"), Optional.of(2))); + assertThat(value).isInstanceOf(ErrorReturnValue.class); + assertThat(value) + .extracting("errorCode", "errorName", "errorInstanceId", "args") + .containsExactly( + expectedErrorForEndpoint.errorCode, + expectedErrorForEndpoint.errorName, + expectedErrorForEndpoint.errorInstanceId, + expectedErrorForEndpoint.args); + } + @Test public void testDeserializeEmptyBody() { // Given @@ -316,11 +404,18 @@ public void testDeserializeWithCustomEncoding() throws JsonProcessingException { // Then assertThat(stubbingEncoding.getDeserializer(errorTypeMarker)) - .isInstanceOfSatisfying(ContentRecordingJsonDeserializer.class, deserializer -> { - assertThat(deserializer.getDeserializedContent()) - .asInstanceOf(InstanceOfAssertFactories.LIST) - .containsExactly(responseBody); - }); + .isInstanceOfSatisfying(ContentRecordingJsonDeserializer.class, deserializer -> assertThat( + deserializer.getDeserializedContent()) + .asInstanceOf(InstanceOfAssertFactories.LIST) + .containsExactly(responseBody)); + } + + private static byte[] readAllBytesUnchecked(Supplier stream) { + try (InputStream is = stream.get()) { + return is.readAllBytes(); + } catch (IOException e) { + throw new RuntimeException(e); + } } private ConjureBodySerDe conjureBodySerDe(String... contentTypes) { @@ -331,4 +426,11 @@ private ConjureBodySerDe conjureBodySerDe(String... contentTypes) { Encodings.emptyContainerDeserializer(), DefaultConjureRuntime.DEFAULT_SERDE_CACHE_SPEC); } + + private static final class BinaryBodyArgumentsProvider implements ArgumentsProvider { + @Override + public Stream provideArguments(ExtensionContext _context) { + return Stream.of(Arguments.of((Object) new byte[] {1, 2, 3}), Arguments.of((Object) new byte[] {})); + } + } } From b175b5430a2b085eb727884a9ace0cb455699c4d Mon Sep 17 00:00:00 2001 From: Pritham Marupaka Date: Mon, 27 Jan 2025 14:02:15 -0500 Subject: [PATCH 11/17] Find empty record canonical constructor in JacksonEmptyContainerLoader --- .../serde/JacksonEmptyContainerLoader.java | 81 +++++++++++-------- .../dialogue/serde/ConjureBodySerDeTest.java | 8 +- .../EndpointErrorsConjureBodySerDeTest.java | 7 +- 3 files changed, 50 insertions(+), 46 deletions(-) diff --git a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/JacksonEmptyContainerLoader.java b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/JacksonEmptyContainerLoader.java index 0506ac7d2..7caf03d7d 100644 --- a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/JacksonEmptyContainerLoader.java +++ b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/JacksonEmptyContainerLoader.java @@ -24,6 +24,7 @@ import com.palantir.logsafe.logger.SafeLogger; import com.palantir.logsafe.logger.SafeLoggerFactory; import java.io.IOException; +import java.lang.reflect.Constructor; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.lang.reflect.Modifier; @@ -64,26 +65,35 @@ private Optional constructEmptyInstance(Type type, TypeMarker origina } // fallback to manual reflection to handle aliases of optionals (and aliases of aliases of optionals) - MethodAndMaybeParameter methodAndMaybeParameter = getJsonCreatorStaticMethodAndMaybeParam(type); - if (methodAndMaybeParameter != null) { - Method method = methodAndMaybeParameter.method(); - Type parameterType = methodAndMaybeParameter.parameterType(); - if (parameterType != null) { - Optional parameter = - constructEmptyInstance(parameterType, originalType, decrement(maxRecursion, originalType)); - if (parameter.isPresent()) { - return invokeStaticFactoryMethod(method, parameter.get()); - } else { - if (log.isDebugEnabled()) { - log.debug( - "Found a @JsonCreator, but couldn't construct the parameter", - SafeArg.of("type", type), - SafeArg.of("parameter", parameter)); - } - return Optional.empty(); - } + Method method = getJsonCreatorStaticMethod(type); + if (method != null) { + Type parameterType = method.getParameters()[0].getParameterizedType(); + // Class parameterType = method.getParameters()[0].getType(); + Optional parameter = + constructEmptyInstance(parameterType, originalType, decrement(maxRecursion, originalType)); + + if (parameter.isPresent()) { + return invokeStaticFactoryMethod(method, parameter.get()); } else { - return invokeStaticFactoryMethod(method, null); + if (log.isDebugEnabled()) { + log.debug( + "Found a @JsonCreator, but couldn't construct the parameter", + SafeArg.of("type", type), + SafeArg.of("parameter", parameter)); + } + return Optional.empty(); + } + } + + Constructor emptyRecordConstructor = getEmptyRecordCanonicalConstructor(type); + if (emptyRecordConstructor != null) { + try { + return Optional.of(emptyRecordConstructor.newInstance()); + } catch (InstantiationException | IllegalAccessException | InvocationTargetException e) { + if (log.isDebugEnabled()) { + log.debug("Empty record construction failed", SafeArg.of("type", type), e); + } + return Optional.empty(); } } @@ -141,29 +151,36 @@ private Optional jacksonDeserializeFromNull(Type type) { // doesn't attempt to handle multiple @JsonCreator methods on one class @Nullable - private static MethodAndMaybeParameter getJsonCreatorStaticMethodAndMaybeParam(Type type) { + private static Method getJsonCreatorStaticMethod(Type type) { if (type instanceof Class) { Class clazz = (Class) type; for (Method method : clazz.getMethods()) { - int parameterCount = method.getParameterCount(); if (Modifier.isStatic(method.getModifiers()) - && method.getAnnotation(JsonCreator.class) != null - && parameterCount < 2) { - if (parameterCount == 0) { - return new MethodAndMaybeParameter(method, null); - } else if (parameterCount == 1) { - Type parameterType = method.getParameters()[0].getParameterizedType(); - return new MethodAndMaybeParameter(method, parameterType); - } + && method.getParameterCount() == 1 + && method.getAnnotation(JsonCreator.class) != null) { + return method; + } + } + } + return null; + } + + @Nullable + private static Constructor getEmptyRecordCanonicalConstructor(Type type) { + if (type instanceof Class) { + Class clazz = (Class) type; + for (Constructor ctor : clazz.getDeclaredConstructors()) { + if (0 == ctor.getParameterCount()) { + return ctor; } } } return null; } - private static Optional invokeStaticFactoryMethod(Method method, @Nullable Object parameter) { + private static Optional invokeStaticFactoryMethod(Method method, Object parameter) { try { - return Optional.ofNullable(parameter == null ? method.invoke(null) : method.invoke(null, parameter)); + return Optional.ofNullable(method.invoke(null, parameter)); } catch (IllegalAccessException | InvocationTargetException e) { if (log.isDebugEnabled()) { log.debug("Reflection instantiation failed", e); @@ -171,6 +188,4 @@ private static Optional invokeStaticFactoryMethod(Method method, @Nullab return Optional.empty(); } } - - private record MethodAndMaybeParameter(Method method, @Nullable Type parameterType) {} } diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ConjureBodySerDeTest.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ConjureBodySerDeTest.java index b0985ec47..e7e2356b7 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ConjureBodySerDeTest.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ConjureBodySerDeTest.java @@ -20,7 +20,6 @@ import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.assertj.core.api.Assertions.assertThatThrownBy; -import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.collect.ImmutableList; @@ -73,12 +72,7 @@ public void testRequestOptionalEmpty() { @Test public void testRequestCustomEmpty() { - record EmptyRecord() { - @JsonCreator - public static EmptyRecord create() { - return new EmptyRecord(); - } - } + record EmptyRecord() {} TestResponse response = new TestResponse().code(204); BodySerDe serializers = conjureBodySerDe("application/json"); EmptyRecord value = serializers 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 8661981dd..eccf32126 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 @@ -71,12 +71,7 @@ public class EndpointErrorsConjureBodySerDeTest { private sealed interface EmptyBodyEndpointReturnBaseType permits EmptyReturnValue, ErrorReturnValue {} @Generated("by conjure-java") - record EmptyReturnValue() implements EmptyBodyEndpointReturnBaseType { - @JsonCreator - public static EmptyReturnValue create() { - return new EmptyReturnValue(); - } - } + record EmptyReturnValue() implements EmptyBodyEndpointReturnBaseType {} @Generated("by conjure-java") private sealed interface EndpointReturnBaseType permits ExpectedReturnValue, ErrorReturnValue {} From e7a51056debc1a8e5452e5812766248a1b695c24 Mon Sep 17 00:00:00 2001 From: Pritham Marupaka Date: Mon, 27 Jan 2025 15:50:09 -0500 Subject: [PATCH 12/17] Update encodings list when constructing binary deserializers --- .../java/dialogue/serde/ConjureBodySerDe.java | 4 +- .../EndpointErrorsConjureBodySerDeTest.java | 56 +++++++++---------- 2 files changed, 30 insertions(+), 30 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 b8b0b907d..b2c45e017 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 @@ -165,7 +165,7 @@ public Deserializer inputStreamDeserializer() { @SuppressWarnings("unchecked") public Deserializer inputStreamDeserializer(DeserializerArgs deserializerArgs) { return new EncodingDeserializerForEndpointRegistry<>( - encodingsSortedByWeight, + ImmutableList.of(BinaryEncoding.INSTANCE), emptyContainerDeserializer, deserializerArgs.baseType(), deserializerArgs, @@ -182,7 +182,7 @@ public Deserializer> optionalInputStreamDeserializer() { @SuppressWarnings("unchecked") public Deserializer optionalInputStreamDeserializer(DeserializerArgs deserializerArgs) { return new EncodingDeserializerForEndpointRegistry<>( - encodingsSortedByWeight, + ImmutableList.of(BinaryEncoding.INSTANCE), emptyContainerDeserializer, deserializerArgs.baseType(), deserializerArgs, 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 eccf32126..8d8c47203 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 @@ -21,6 +21,7 @@ import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.annotation.JsonValue; import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.collect.ImmutableList; @@ -83,22 +84,23 @@ private sealed interface EndpointBinaryReturnBaseType permits BinaryReturnValue, private sealed interface EndpointOptionalBinaryReturnBaseType permits OptionalBinaryReturnValue, ErrorReturnValue {} @Generated("by conjure-java") - record ExpectedReturnValue(String value) implements EndpointReturnBaseType { - @JsonCreator - public static ExpectedReturnValue create(String value) { - return new ExpectedReturnValue(Preconditions.checkArgumentNotNull(value, "value cannot be null")); + record ExpectedReturnValue(@JsonValue String value) implements EndpointReturnBaseType { + public ExpectedReturnValue { + Preconditions.checkArgumentNotNull(value, "value cannot be null"); } } @Generated("by conjure-java") - record BinaryReturnValue(@MustBeClosed InputStream value) implements EndpointBinaryReturnBaseType { + record BinaryReturnValue(@MustBeClosed @JsonValue InputStream value) + implements EndpointErrorsConjureBodySerDeTest.EndpointBinaryReturnBaseType { public BinaryReturnValue { Preconditions.checkArgumentNotNull(value, "value cannot be null"); } } @Generated("by conjure-java") - record OptionalBinaryReturnValue(Optional value) implements EndpointOptionalBinaryReturnBaseType { + record OptionalBinaryReturnValue(@JsonValue Optional value) + implements EndpointOptionalBinaryReturnBaseType { public OptionalBinaryReturnValue { Preconditions.checkArgumentNotNull(value, "value cannot be null"); } @@ -147,6 +149,26 @@ private TestEndpointError( } } + @Test + public void testDeserializeExpectedValue() { + // Given + String expectedString = "expectedString"; + TestResponse response = TestResponse.withBody(String.format("\"%s\"", expectedString)) + .contentType("application/json") + .code(200); + BodySerDe serializers = conjureBodySerDe("application/json", "text/plain"); + DeserializerArgs deserializerArgs = DeserializerArgs.builder() + .baseType(new TypeMarker<>() {}) + .success(new TypeMarker() {}) + .error("Default:FailedPrecondition", new TypeMarker() {}) + .build(); + // When + EndpointReturnBaseType value = + serializers.deserializer(deserializerArgs).deserialize(response); + // Then + assertThat(value).isEqualTo(new ExpectedReturnValue(expectedString)); + } + // The error should be deserialized using Encodings.json(), when a JSON encoding is not provided. @ParameterizedTest @ValueSource(strings = {"application/json", "text/plain"}) @@ -227,26 +249,6 @@ public void testDeserializingUndefinedErrorFallsbackToSerializableError() throws }); } - @Test - public void testDeserializeExpectedValue() { - // Given - String expectedString = "expectedString"; - TestResponse response = TestResponse.withBody(String.format("\"%s\"", expectedString)) - .contentType("application/json") - .code(200); - BodySerDe serializers = conjureBodySerDe("application/json", "text/plain"); - DeserializerArgs deserializerArgs = DeserializerArgs.builder() - .baseType(new TypeMarker<>() {}) - .success(new TypeMarker() {}) - .error("Default:FailedPrecondition", new TypeMarker() {}) - .build(); - // When - EndpointReturnBaseType value = - serializers.deserializer(deserializerArgs).deserialize(response); - // Then - assertThat(value).isEqualTo(new ExpectedReturnValue(expectedString)); - } - @ParameterizedTest @ArgumentsSource(BinaryBodyArgumentsProvider.class) public void testDeserializeBinaryValue(byte[] binaryBody) { @@ -254,7 +256,6 @@ public void testDeserializeBinaryValue(byte[] binaryBody) { TestResponse response = new TestResponse(binaryBody) .contentType("application/octet-stream") .code(200); - // BodySerDe serializers = conjureBodySerDe("application/octet-stream", "text/plain"); BodySerDe serializers = new ConjureBodySerDe( ImmutableList.of(WeightedEncoding.of(BinaryEncoding.INSTANCE)), @@ -284,7 +285,6 @@ public void testDeserializeOptionalBinaryValuePresent(byte[] binaryBody) { TestResponse response = new TestResponse(binaryBody) .contentType("application/octet-stream") .code(200); - // BodySerDe serializers = conjureBodySerDe("application/octet-stream", "text/plain"); BodySerDe serializers = new ConjureBodySerDe( ImmutableList.of(WeightedEncoding.of(BinaryEncoding.INSTANCE)), From a590c3c9d4acdb8313e0b14280d850db4d0d88f2 Mon Sep 17 00:00:00 2001 From: Pritham Marupaka Date: Mon, 27 Jan 2025 16:31:55 -0500 Subject: [PATCH 13/17] Add ConjureErrors --- .../com/palantir/dialogue/ConjureErrors.java | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 dialogue-target/src/main/java/com/palantir/dialogue/ConjureErrors.java diff --git a/dialogue-target/src/main/java/com/palantir/dialogue/ConjureErrors.java b/dialogue-target/src/main/java/com/palantir/dialogue/ConjureErrors.java new file mode 100644 index 000000000..5a474009b --- /dev/null +++ b/dialogue-target/src/main/java/com/palantir/dialogue/ConjureErrors.java @@ -0,0 +1,59 @@ +/* + * (c) Copyright 2025 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.dialogue; + +import com.palantir.logsafe.Safe; + +public final class ConjureErrors { + private ConjureErrors() {} + + public abstract static class BaseEndpointError { + @Safe + private final String errorCode; + + @Safe + private final String errorName; + + @Safe + private final String errorInstanceId; + + private final T params; + + protected BaseEndpointError(String errorCode, String errorName, String errorInstanceId, T params) { + this.errorCode = errorCode; + this.errorName = errorName; + this.errorInstanceId = errorInstanceId; + this.params = params; + } + + public final String getErrorCode() { + return errorCode; + } + + public final String getErrorName() { + return errorName; + } + + public final String getErrorInstanceId() { + return errorInstanceId; + } + + public final T getParams() { + return params; + } + } +} From c7bfd9093f4a61555bb3eba8daf5a30926f1dbe9 Mon Sep 17 00:00:00 2001 From: Pritham Marupaka Date: Mon, 27 Jan 2025 16:51:31 -0500 Subject: [PATCH 14/17] add isRecord check --- .../java/dialogue/serde/JacksonEmptyContainerLoader.java | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/JacksonEmptyContainerLoader.java b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/JacksonEmptyContainerLoader.java index 7caf03d7d..4ce406d5d 100644 --- a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/JacksonEmptyContainerLoader.java +++ b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/JacksonEmptyContainerLoader.java @@ -169,9 +169,11 @@ private static Method getJsonCreatorStaticMethod(Type type) { private static Constructor getEmptyRecordCanonicalConstructor(Type type) { if (type instanceof Class) { Class clazz = (Class) type; - for (Constructor ctor : clazz.getDeclaredConstructors()) { - if (0 == ctor.getParameterCount()) { - return ctor; + if (clazz.isRecord()) { + for (Constructor ctor : clazz.getDeclaredConstructors()) { + if (0 == ctor.getParameterCount()) { + return ctor; + } } } } From 55b661dafd02b381d6076cfc04257a34a5ba02ef Mon Sep 17 00:00:00 2001 From: Pritham Marupaka Date: Mon, 27 Jan 2025 17:05:41 -0500 Subject: [PATCH 15/17] update revapi --- .palantir/revapi.yml | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/.palantir/revapi.yml b/.palantir/revapi.yml index d05c19c62..0a70f8cb5 100644 --- a/.palantir/revapi.yml +++ b/.palantir/revapi.yml @@ -311,19 +311,17 @@ acceptedBreaks: new: "method com.palantir.dialogue.clients.DialogueClients.StickyChannelSession\ \ com.palantir.dialogue.clients.DialogueClients.StickyChannelFactory2::session()" justification: "interface for consumption, not extension" - "4.6.0": + "5.0.0": com.palantir.dialogue:dialogue-target: - code: "java.method.addedToInterface" new: "method com.palantir.dialogue.Deserializer com.palantir.dialogue.BodySerDe::deserializer(com.palantir.dialogue.DeserializerArgs)" - justification: "Adding a new method to create deserializers in support of endpoint\ + justification: "Adding new methods to create deserializers in support of endpoint\ \ associated error deserialization" - "4.8.0-rc1": - com.palantir.dialogue:dialogue-target: - code: "java.method.addedToInterface" new: "method com.palantir.dialogue.Deserializer com.palantir.dialogue.BodySerDe::inputStreamDeserializer(com.palantir.dialogue.DeserializerArgs)" - justification: "Adding a new method to create deserializers in support of endpoint\ + justification: "Adding new methods to create deserializers in support of endpoint\ \ associated error deserialization" - code: "java.method.addedToInterface" new: "method com.palantir.dialogue.Deserializer com.palantir.dialogue.BodySerDe::optionalInputStreamDeserializer(com.palantir.dialogue.DeserializerArgs)" - justification: "Adding a new method to create deserializers in support of endpoint\ + justification: "Adding new methods to create deserializers in support of endpoint\ \ associated error deserialization" From b59d006bc8e3ff25d3149756f3a10a72f60ef931 Mon Sep 17 00:00:00 2001 From: Pritham Marupaka Date: Tue, 28 Jan 2025 11:23:13 -0500 Subject: [PATCH 16/17] nit: update log message --- .../java/dialogue/serde/JacksonEmptyContainerLoader.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/JacksonEmptyContainerLoader.java b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/JacksonEmptyContainerLoader.java index 4ce406d5d..85ee3580c 100644 --- a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/JacksonEmptyContainerLoader.java +++ b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/JacksonEmptyContainerLoader.java @@ -99,7 +99,8 @@ private Optional constructEmptyInstance(Type type, TypeMarker origina if (log.isDebugEnabled()) { log.debug( - "Jackson couldn't instantiate an empty instance and also couldn't find a usable @JsonCreator", + "Jackson couldn't instantiate an empty instance and also couldn't find a usable @JsonCreator" + + " or an empty record constructor", SafeArg.of("type", type)); } return Optional.empty(); From c5fdef41de92ecd232ca0756a9944f27078c79f4 Mon Sep 17 00:00:00 2001 From: Pritham Marupaka Date: Tue, 28 Jan 2025 11:30:58 -0500 Subject: [PATCH 17/17] review comments --- .../serde/JacksonEmptyContainerLoader.java | 5 +- .../com/palantir/dialogue/ConjureErrors.java | 59 ------------------- .../com/palantir/dialogue/EndpointError.java | 55 +++++++++++++++++ 3 files changed, 59 insertions(+), 60 deletions(-) delete mode 100644 dialogue-target/src/main/java/com/palantir/dialogue/ConjureErrors.java create mode 100644 dialogue-target/src/main/java/com/palantir/dialogue/EndpointError.java diff --git a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/JacksonEmptyContainerLoader.java b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/JacksonEmptyContainerLoader.java index 85ee3580c..83bdac0e0 100644 --- a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/JacksonEmptyContainerLoader.java +++ b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/JacksonEmptyContainerLoader.java @@ -166,11 +166,14 @@ private static Method getJsonCreatorStaticMethod(Type type) { return null; } + /** + * If {@code type} is a record with 0 components, return the canonical constructor. + */ @Nullable private static Constructor getEmptyRecordCanonicalConstructor(Type type) { if (type instanceof Class) { Class clazz = (Class) type; - if (clazz.isRecord()) { + if (clazz.isRecord() && clazz.getRecordComponents().length == 0) { for (Constructor ctor : clazz.getDeclaredConstructors()) { if (0 == ctor.getParameterCount()) { return ctor; diff --git a/dialogue-target/src/main/java/com/palantir/dialogue/ConjureErrors.java b/dialogue-target/src/main/java/com/palantir/dialogue/ConjureErrors.java deleted file mode 100644 index 5a474009b..000000000 --- a/dialogue-target/src/main/java/com/palantir/dialogue/ConjureErrors.java +++ /dev/null @@ -1,59 +0,0 @@ -/* - * (c) Copyright 2025 Palantir Technologies Inc. All rights reserved. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.palantir.dialogue; - -import com.palantir.logsafe.Safe; - -public final class ConjureErrors { - private ConjureErrors() {} - - public abstract static class BaseEndpointError { - @Safe - private final String errorCode; - - @Safe - private final String errorName; - - @Safe - private final String errorInstanceId; - - private final T params; - - protected BaseEndpointError(String errorCode, String errorName, String errorInstanceId, T params) { - this.errorCode = errorCode; - this.errorName = errorName; - this.errorInstanceId = errorInstanceId; - this.params = params; - } - - public final String getErrorCode() { - return errorCode; - } - - public final String getErrorName() { - return errorName; - } - - public final String getErrorInstanceId() { - return errorInstanceId; - } - - public final T getParams() { - return params; - } - } -} diff --git a/dialogue-target/src/main/java/com/palantir/dialogue/EndpointError.java b/dialogue-target/src/main/java/com/palantir/dialogue/EndpointError.java new file mode 100644 index 000000000..d2d595a27 --- /dev/null +++ b/dialogue-target/src/main/java/com/palantir/dialogue/EndpointError.java @@ -0,0 +1,55 @@ +/* + * (c) Copyright 2025 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.dialogue; + +import com.palantir.logsafe.Safe; + +public abstract class EndpointError { + @Safe + private final String errorCode; + + @Safe + private final String errorName; + + @Safe + private final String errorInstanceId; + + private final T params; + + protected EndpointError(String errorCode, String errorName, String errorInstanceId, T params) { + this.errorCode = errorCode; + this.errorName = errorName; + this.errorInstanceId = errorInstanceId; + this.params = params; + } + + public final String getErrorCode() { + return errorCode; + } + + public final String getErrorName() { + return errorName; + } + + public final String getErrorInstanceId() { + return errorInstanceId; + } + + public final T getParams() { + return params; + } +}