Skip to content

Commit

Permalink
review changes
Browse files Browse the repository at this point in the history
  • Loading branch information
Pritham Marupaka committed Jan 9, 2025
1 parent f00bd5f commit 7dac6ee
Show file tree
Hide file tree
Showing 6 changed files with 151 additions and 130 deletions.
34 changes: 16 additions & 18 deletions dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ConjureBodySerDe.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,19 +82,18 @@ final class ConjureBodySerDe implements BodySerDe {
emptyContainerDeserializer,
BinaryEncoding.MARKER,
DeserializerArgs.<InputStream>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.<Optional<InputStream>>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)
Expand All @@ -108,8 +107,8 @@ private <T> EncodingDeserializerForEndpointRegistry<?> buildCacheEntry(TypeMarke
emptyContainerDeserializer,
typeMarker,
DeserializerArgs.<T>builder()
.withBaseType(typeMarker)
.withExpectedResult(typeMarker)
.baseType(typeMarker)
.success(typeMarker)
.build());
}

Expand Down Expand Up @@ -254,26 +253,25 @@ private static final class EncodingDeserializerForEndpointRegistry<T> implements
private final TypeMarker<T> token;

EncodingDeserializerForEndpointRegistry(
List<Encoding> encodings,
List<Encoding> encodingsSortedByWeight,
EmptyContainerDeserializer empty,
TypeMarker<T> token,
DeserializerArgs<T> 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
Expand Down
86 changes: 52 additions & 34 deletions ...ue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/EndpointErrorDecoder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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}.
Expand All @@ -65,17 +64,23 @@
*/
final class EndpointErrorDecoder<T> {
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<NamedError> NAMED_ERROR_DESERIALIZER =
JSON_ENCODING.deserializer(new TypeMarker<>() {});
private final Map<String, Encoding.Deserializer<? extends T>> errorNameToJsonDeserializerMap;

EndpointErrorDecoder(Map<String, TypeMarker<? extends T>> errorNameToTypeMap) {
this(errorNameToTypeMap, Optional.empty());
}

EndpointErrorDecoder(
Map<String, TypeMarker<? extends T>> errorNameToTypeMap, Optional<Encoding> maybeJsonEncoding) {
this.errorNameToJsonDeserializerMap = maybeJsonEncoding
.<Map<String, Encoding.Deserializer<? extends T>>>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) {
Expand Down Expand Up @@ -129,57 +134,68 @@ Optional<RuntimeException> 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<RuntimeException> 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, "<unparseable>");
exception.initCause(e);
throw exception;
}

Optional<String> 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<Deserializer<? extends T>> maybeDeserializer =
Optional.ofNullable(errorNameToJsonDeserializerMap.get(errorNameNode.asText()));
if (maybeDeserializer.isEmpty()) {
throwRemoteException(body, code);
Deserializer<? extends T> 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<SerializableError>() {})
.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);
}
}

Expand Down Expand Up @@ -247,4 +263,6 @@ public Optional<String> getFirstHeader(Response response, String headerName) {
return response.getFirstHeader(headerName);
}
}

record NamedError(@JsonProperty("errorName") String errorName) {}
}
15 changes: 13 additions & 2 deletions dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand All @@ -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);
Expand All @@ -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, "<unparseable>");
exception.initCause(e);
Expand All @@ -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);
}
}
}
16 changes: 8 additions & 8 deletions ...est/java/com/palantir/conjure/java/dialogue/serde/EndpointErrorsConjureBodySerDeTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,9 +116,9 @@ public void testDeserializeCustomError() throws IOException {
.code(500);
BodySerDe serializers = conjureBodySerDe("application/json", "text/plain");
DeserializerArgs<EndpointReturnBaseType> deserializerArgs = DeserializerArgs.<EndpointReturnBaseType>builder()
.withBaseType(new TypeMarker<>() {})
.withExpectedResult(new TypeMarker<ExpectedReturnValue>() {})
.withErrorType("Default:FailedPrecondition", new TypeMarker<ErrorReturnValue>() {})
.baseType(new TypeMarker<>() {})
.success(new TypeMarker<ExpectedReturnValue>() {})
.error("Default:FailedPrecondition", new TypeMarker<ErrorReturnValue>() {})
.build();

// When
Expand Down Expand Up @@ -155,8 +155,8 @@ public void testDeserializingUndefinedErrorFallsbackToSerializableError() throws
.code(500);
BodySerDe serializers = conjureBodySerDe("application/json", "text/plain");
DeserializerArgs<EndpointReturnBaseType> deserializerArgs = DeserializerArgs.<EndpointReturnBaseType>builder()
.withBaseType(new TypeMarker<>() {})
.withExpectedResult(new TypeMarker<ExpectedReturnValue>() {})
.baseType(new TypeMarker<>() {})
.success(new TypeMarker<ExpectedReturnValue>() {})
// Note: no error types are registered.
.build();

Expand Down Expand Up @@ -190,9 +190,9 @@ public void testDeserializeExpectedValue() {
.code(200);
BodySerDe serializers = conjureBodySerDe("application/json", "text/plain");
DeserializerArgs<EndpointReturnBaseType> deserializerArgs = DeserializerArgs.<EndpointReturnBaseType>builder()
.withBaseType(new TypeMarker<>() {})
.withExpectedResult(new TypeMarker<ExpectedReturnValue>() {})
.withErrorType("Default:FailedPrecondition", new TypeMarker<ErrorReturnValue>() {})
.baseType(new TypeMarker<>() {})
.success(new TypeMarker<ExpectedReturnValue>() {})
.error("Default:FailedPrecondition", new TypeMarker<ErrorReturnValue>() {})
.build();
// When
EndpointReturnBaseType value =
Expand Down
Loading

0 comments on commit 7dac6ee

Please sign in to comment.