From 25f91b40f5098741939a128284c1ab2c214d1629 Mon Sep 17 00:00:00 2001 From: Patrick Koenig Date: Mon, 30 Sep 2024 18:09:26 -0400 Subject: [PATCH] Cache Feign endpoints (#2973) --- conjure-java-jaxrs-client/build.gradle | 3 + .../AbstractFeignJaxRsClientBuilder.java | 37 +++--- .../client/jaxrs/DialogueFeignClient.java | 110 ++++++++++++------ .../MethodHeaderEnrichmentContract.java | 40 +++++++ .../JaxRsClientDialogueEndpointTest.java | 60 ++++++++-- 5 files changed, 189 insertions(+), 61 deletions(-) create mode 100644 conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/feignimpl/MethodHeaderEnrichmentContract.java diff --git a/conjure-java-jaxrs-client/build.gradle b/conjure-java-jaxrs-client/build.gradle index e1d05c523..257c6a170 100644 --- a/conjure-java-jaxrs-client/build.gradle +++ b/conjure-java-jaxrs-client/build.gradle @@ -29,6 +29,9 @@ dependencies { implementation 'com.palantir.tritium:tritium-registry' implementation 'io.dropwizard.metrics:metrics-core' + annotationProcessor "org.immutables:value" + compileOnly 'org.immutables:value::annotations' + implementation project(":conjure-java-jackson-serialization") implementation "com.google.guava:guava" implementation "com.github.ben-manes.caffeine:caffeine" diff --git a/conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/AbstractFeignJaxRsClientBuilder.java b/conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/AbstractFeignJaxRsClientBuilder.java index 182b57bf7..604a2c97a 100644 --- a/conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/AbstractFeignJaxRsClientBuilder.java +++ b/conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/AbstractFeignJaxRsClientBuilder.java @@ -31,8 +31,8 @@ import com.palantir.conjure.java.client.jaxrs.feignimpl.InputStreamDelegateEncoder; import com.palantir.conjure.java.client.jaxrs.feignimpl.Java8OptionalAwareContract; import com.palantir.conjure.java.client.jaxrs.feignimpl.Java8OptionalAwareDecoder; +import com.palantir.conjure.java.client.jaxrs.feignimpl.MethodHeaderEnrichmentContract; import com.palantir.conjure.java.client.jaxrs.feignimpl.NeverReturnNullDecoder; -import com.palantir.conjure.java.client.jaxrs.feignimpl.PathTemplateHeaderEnrichmentContract; import com.palantir.conjure.java.client.jaxrs.feignimpl.SlashEncodingContract; import com.palantir.conjure.java.client.jaxrs.feignimpl.TextDelegateDecoder; import com.palantir.conjure.java.client.jaxrs.feignimpl.TextDelegateEncoder; @@ -180,28 +180,35 @@ public int hashCode() { } private static Contract createContract() { - return new EndpointNameHeaderEnrichmentContract( - new PathTemplateHeaderEnrichmentContract(new SlashEncodingContract(new Java8OptionalAwareContract( - new GuavaOptionalAwareContract(new CompatibleJaxRsContract()))))); + Contract contract = new CompatibleJaxRsContract(); + contract = new GuavaOptionalAwareContract(contract); + contract = new Java8OptionalAwareContract(contract); + contract = new SlashEncodingContract(contract); + contract = new MethodHeaderEnrichmentContract(contract); + contract = new EndpointNameHeaderEnrichmentContract(contract); + return contract; } private static Decoder createDecoder( @Safe String clientNameForLogging, ObjectMapper jsonMapper, ObjectMapper cborMapper) { - return new NeverReturnNullDecoder( - new Java8OptionalAwareDecoder(new GuavaOptionalAwareDecoder(new EmptyContainerDecoder( - jsonMapper, - new InputStreamDelegateDecoder( - clientNameForLogging, - new TextDelegateDecoder( - new CborDelegateDecoder(cborMapper, new JacksonDecoder(jsonMapper)))))))); + Decoder decoder = new JacksonDecoder(jsonMapper); + decoder = new CborDelegateDecoder(cborMapper, decoder); + decoder = new TextDelegateDecoder(decoder); + decoder = new InputStreamDelegateDecoder(clientNameForLogging, decoder); + decoder = new EmptyContainerDecoder(jsonMapper, decoder); + decoder = new GuavaOptionalAwareDecoder(decoder); + decoder = new Java8OptionalAwareDecoder(decoder); + decoder = new NeverReturnNullDecoder(decoder); + return decoder; } private static Encoder createEncoder( @Safe String clientNameForLogging, ObjectMapper jsonMapper, ObjectMapper cborMapper) { - return new InputStreamDelegateEncoder( - clientNameForLogging, - new TextDelegateEncoder( - new CborDelegateEncoder(cborMapper, new ConjureFeignJacksonEncoder(jsonMapper)))); + Encoder encoder = new ConjureFeignJacksonEncoder(jsonMapper); + encoder = new CborDelegateEncoder(cborMapper, encoder); + encoder = new TextDelegateEncoder(encoder); + encoder = new InputStreamDelegateEncoder(clientNameForLogging, encoder); + return encoder; } private static void verifyClientUsageAnnotations(Class serviceClass) { diff --git a/conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/DialogueFeignClient.java b/conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/DialogueFeignClient.java index c8a94c002..aebb26f2d 100644 --- a/conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/DialogueFeignClient.java +++ b/conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/DialogueFeignClient.java @@ -27,6 +27,7 @@ import com.google.common.util.concurrent.UncheckedExecutionException; import com.palantir.conjure.java.api.errors.UnknownRemoteException; import com.palantir.conjure.java.client.jaxrs.feignimpl.EndpointNameHeaderEnrichmentContract; +import com.palantir.conjure.java.client.jaxrs.feignimpl.MethodHeaderEnrichmentContract; import com.palantir.dialogue.Channel; import com.palantir.dialogue.ConjureRuntime; import com.palantir.dialogue.Deserializer; @@ -57,6 +58,9 @@ import java.util.List; import java.util.Locale; import java.util.Optional; +import java.util.OptionalLong; +import java.util.concurrent.ConcurrentHashMap; +import org.immutables.value.Value; /** * {@link DialogueFeignClient} is an adapter from {@link feign.Client} to {@link Channel Dialogue Channel} @@ -64,10 +68,10 @@ */ final class DialogueFeignClient implements feign.Client { - private static final String PATH_TEMPLATE = "hr-path-template"; - private static final Splitter pathSplitter = Splitter.on('/'); - private static final Splitter querySplitter = Splitter.on('&').omitEmptyStrings(); - private static final Splitter queryValueSplitter = Splitter.on('='); + private static final String REQUEST_URL_PATH_PARAM = "request-url"; + private static final Splitter PATH_SPLITTER = Splitter.on('/'); + private static final Splitter QUERY_SPLITTER = Splitter.on('&').omitEmptyStrings(); + private static final Splitter QUERY_VALUE_SPLITTER = Splitter.on('='); private final ConjureRuntime runtime; private final Channel channel; @@ -75,6 +79,8 @@ final class DialogueFeignClient implements feign.Client { private final String serviceName; private final String version; + private final ConcurrentHashMap endpointChannels = new ConcurrentHashMap<>(); + DialogueFeignClient(Class jaxrsInterface, Channel channel, ConjureRuntime runtime, String baseUrl) { this.channel = Preconditions.checkNotNull(channel, "Channel is required"); this.baseUrl = Preconditions.checkNotNull(baseUrl, "Base URL is required"); @@ -88,20 +94,22 @@ final class DialogueFeignClient implements feign.Client { @Override public feign.Response execute(Request request, Request.Options _options) throws IOException { com.palantir.dialogue.Request.Builder builder = com.palantir.dialogue.Request.builder(); - Optional body = requestBody(request); - if (body.isPresent()) { - builder.body(body); - builder.putHeaderParams(HttpHeaders.CONTENT_LENGTH, Integer.toString(request.body().length)); - } + + builder.putPathParams(REQUEST_URL_PATH_PARAM, request.url()); + + builder.body(requestBody(request)); + request.headers().forEach((headerName, values) -> { if (includeRequestHeader(headerName)) { builder.putAllHeaderParams(headerName, values); } }); + EndpointChannel endpointChannel = + endpointChannels.computeIfAbsent(FeignEndpointKey.of(request), this::toEndpointChannel); + try { - return runtime.clients() - .callBlocking(toEndpointChannel(request), builder.build(), FeignResponseDeserializer.INSTANCE); + return runtime.clients().callBlocking(endpointChannel, builder.build(), FeignResponseDeserializer.INSTANCE); } catch (UncheckedExecutionException e) { // Rethrow IOException to match standard feign behavior Throwable cause = e.getCause(); @@ -112,8 +120,8 @@ public feign.Response execute(Request request, Request.Options _options) throws } } - private EndpointChannel toEndpointChannel(Request feignRequest) { - Endpoint endpoint = new FeignRequestEndpoint(feignRequest); + private EndpointChannel toEndpointChannel(FeignEndpointKey key) { + Endpoint endpoint = new FeignEndpoint(key.httpMethod(), key.endpointName()); return dialogueRequest -> channel.execute(endpoint, dialogueRequest); } @@ -125,8 +133,7 @@ private static boolean includeRequestHeader(String headerName) { if (HttpHeaders.CONTENT_LENGTH.equalsIgnoreCase(headerName)) { return false; } - // Tracing path template is informational only - if (PATH_TEMPLATE.equalsIgnoreCase(headerName)) { + if (MethodHeaderEnrichmentContract.METHOD_HEADER.equalsIgnoreCase(headerName)) { return false; } if (EndpointNameHeaderEnrichmentContract.ENDPOINT_NAME_HEADER.equalsIgnoreCase(headerName)) { @@ -149,10 +156,8 @@ private static Optional requestBody(Request request) { return Optional.empty(); } Optional maybeContentType = getFirstHeader(request, HttpHeaders.CONTENT_TYPE); - if (!maybeContentType.isPresent()) { - if (requestBodyContent.length == 0) { - return Optional.empty(); - } + if (maybeContentType.isEmpty() && requestBodyContent.length == 0) { + return Optional.empty(); } return Optional.of(new ByteArrayRequestBody( requestBodyContent, @@ -173,10 +178,12 @@ private static final class ByteArrayRequestBody implements RequestBody { private final byte[] buffer; private final String contentType; + private final OptionalLong contentLength; ByteArrayRequestBody(byte[] buffer, String contentType) { this.buffer = buffer; this.contentType = contentType; + this.contentLength = OptionalLong.of(buffer.length); } @Override @@ -194,6 +201,11 @@ public boolean repeatable() { return true; } + @Override + public OptionalLong contentLength() { + return contentLength; + } + @Override public void close() { // nothing to do @@ -341,26 +353,32 @@ public Exception decode(String _methodKey, feign.Response response) { } } - private final class FeignRequestEndpoint implements Endpoint { - private final Request request; - private final String endpoint; - private final HttpMethod method; + private final class FeignEndpoint implements Endpoint { - FeignRequestEndpoint(Request request) { - this.request = request; - this.method = HttpMethod.valueOf(request.method().toUpperCase(Locale.ENGLISH)); - endpoint = getFirstHeader(request, EndpointNameHeaderEnrichmentContract.ENDPOINT_NAME_HEADER) - .orElse("feign"); + private final HttpMethod httpMethod; + private final String endpointName; + + FeignEndpoint(HttpMethod httpMethod, String endpointName) { + this.httpMethod = httpMethod; + this.endpointName = endpointName; } @Override - public void renderPath(ListMultimap _params, UrlBuilder url) { - String target = request.url(); + public void renderPath(ListMultimap params, UrlBuilder url) { + List requestUrls = params.get(REQUEST_URL_PATH_PARAM); + Preconditions.checkState( + params.size() == 1 && requestUrls.size() == 1, + "Unexpected path parameters", + SafeArg.of("params", params.size()), + SafeArg.of("requestUrls", requestUrls.size())); + + String target = requestUrls.get(0); Preconditions.checkState( target.startsWith(baseUrl), "Request URL must start with base url", UnsafeArg.of("requestUrl", target), UnsafeArg.of("baseUrl", baseUrl)); + int trailingOffset = 0; // If the trailing section starts with a slash, ignore it to prevent duplicate leading slashes. if (target.length() > baseUrl.length() && target.charAt(baseUrl.length()) == '/') { @@ -370,14 +388,14 @@ public void renderPath(ListMultimap _params, UrlBuilder url) { int queryParamsStart = trailing.indexOf('?'); String queryPortion = queryParamsStart == -1 ? trailing : trailing.substring(0, queryParamsStart); if (!queryPortion.isEmpty()) { - for (String pathSegment : pathSplitter.split(queryPortion)) { + for (String pathSegment : PATH_SPLITTER.split(queryPortion)) { url.pathSegment(urlDecode(pathSegment)); } } if (queryParamsStart != -1) { String querySegments = trailing.substring(queryParamsStart + 1); - for (String querySegment : querySplitter.split(querySegments)) { - List keyValuePair = queryValueSplitter.splitToList(querySegment); + for (String querySegment : QUERY_SPLITTER.split(querySegments)) { + List keyValuePair = QUERY_VALUE_SPLITTER.splitToList(querySegment); if (keyValuePair.size() != 2) { throw new SafeIllegalStateException( "Expected two parameters", @@ -391,7 +409,7 @@ public void renderPath(ListMultimap _params, UrlBuilder url) { @Override public HttpMethod httpMethod() { - return method; + return httpMethod; } @Override @@ -401,7 +419,7 @@ public String serviceName() { @Override public String endpointName() { - return endpoint; + return endpointName; } @Override @@ -409,4 +427,26 @@ public String version() { return version; } } + + @Value.Immutable(builder = false, prehash = true) + interface FeignEndpointKey { + + @Value.Parameter + HttpMethod httpMethod(); + + @Value.Parameter + String method(); + + @Value.Parameter + String endpointName(); + + private static FeignEndpointKey of(Request request) { + return ImmutableFeignEndpointKey.of( + HttpMethod.valueOf(request.method().toUpperCase(Locale.ENGLISH)), + getFirstHeader(request, MethodHeaderEnrichmentContract.METHOD_HEADER) + .orElse(""), + getFirstHeader(request, EndpointNameHeaderEnrichmentContract.ENDPOINT_NAME_HEADER) + .orElse("feign")); + } + } } diff --git a/conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/feignimpl/MethodHeaderEnrichmentContract.java b/conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/feignimpl/MethodHeaderEnrichmentContract.java new file mode 100644 index 000000000..2068934b5 --- /dev/null +++ b/conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/feignimpl/MethodHeaderEnrichmentContract.java @@ -0,0 +1,40 @@ +/* + * (c) Copyright 2017 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.client.jaxrs.feignimpl; + +import feign.Contract; +import feign.MethodMetadata; +import java.lang.reflect.Method; + +/** + * Contract to capture the method name and pass it to a feign client. + * + * This should be considered internal API and should not be depended upon. + */ +public final class MethodHeaderEnrichmentContract extends AbstractDelegatingContract { + + public static final String METHOD_HEADER = "dialogue-method"; + + public MethodHeaderEnrichmentContract(Contract delegate) { + super(delegate); + } + + @Override + protected void processMetadata(Class _targetType, Method method, MethodMetadata metadata) { + metadata.template().header(METHOD_HEADER, method.toString()); + } +} diff --git a/conjure-java-jaxrs-client/src/test/java/com/palantir/conjure/java/client/jaxrs/JaxRsClientDialogueEndpointTest.java b/conjure-java-jaxrs-client/src/test/java/com/palantir/conjure/java/client/jaxrs/JaxRsClientDialogueEndpointTest.java index bb69dbaed..1395d4463 100644 --- a/conjure-java-jaxrs-client/src/test/java/com/palantir/conjure/java/client/jaxrs/JaxRsClientDialogueEndpointTest.java +++ b/conjure-java-jaxrs-client/src/test/java/com/palantir/conjure/java/client/jaxrs/JaxRsClientDialogueEndpointTest.java @@ -20,12 +20,12 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableListMultimap; -import com.google.common.collect.ImmutableMap; import com.google.common.util.concurrent.Futures; import com.palantir.conjure.java.dialogue.serde.DefaultConjureRuntime; import com.palantir.dialogue.Channel; @@ -87,7 +87,7 @@ public void testQueryParameterCollection() { ArgumentCaptor requestCaptor = ArgumentCaptor.forClass(Request.class); verify(channel).execute(endpointCaptor.capture(), requestCaptor.capture()); UrlBuilder urlBuilder = mock(UrlBuilder.class); - endpointCaptor.getValue().renderPath(ImmutableMap.of(), urlBuilder); + endpointCaptor.getValue().renderPath(requestCaptor.getValue().pathParameters(), urlBuilder); verify(urlBuilder).queryParam("query", "a"); verify(urlBuilder).queryParam("query", "/"); verify(urlBuilder).queryParam("query", ""); @@ -111,10 +111,10 @@ public void testPostWithBody() { Request request = requestCaptor.getValue(); assertThat(request.body()).isPresent(); assertThat(request.body().get().contentType()).isEqualTo("text/plain"); + assertThat(request.body().get().contentLength()).hasValue(13); assertThat(request.headerParams().asMap()) .containsExactly( - new AbstractMap.SimpleImmutableEntry<>("Accept", ImmutableList.of("application/json")), - new AbstractMap.SimpleImmutableEntry<>("Content-Length", ImmutableList.of("13"))); + new AbstractMap.SimpleImmutableEntry<>("Accept", ImmutableList.of("application/json"))); } @Test @@ -134,9 +134,9 @@ public void testPostWithBody_defaultContentType() { Request request = requestCaptor.getValue(); assertThat(request.body()).isPresent(); assertThat(request.body().get().contentType()).isEqualTo("application/json"); + assertThat(request.body().get().contentLength()).hasValue(15); - assertThat(request.headerParams().asMap()) - .containsExactly(new AbstractMap.SimpleImmutableEntry<>("Content-Length", ImmutableList.of("15"))); + assertThat(request.headerParams().asMap()).isEmpty(); } @Test @@ -167,7 +167,7 @@ public void testTrailingWildcardParameter_slashes() { ArgumentCaptor requestCaptor = ArgumentCaptor.forClass(Request.class); verify(channel).execute(endpointCaptor.capture(), requestCaptor.capture()); UrlBuilder urlBuilder = mock(UrlBuilder.class); - endpointCaptor.getValue().renderPath(ImmutableMap.of(), urlBuilder); + endpointCaptor.getValue().renderPath(requestCaptor.getValue().pathParameters(), urlBuilder); verify(urlBuilder).pathSegment("foo"); // context path verify(urlBuilder).pathSegment("static0"); verify(urlBuilder).pathSegment("dynamic0"); @@ -186,7 +186,7 @@ public void testTrailingWildcardParameter_emptyString() { ArgumentCaptor requestCaptor = ArgumentCaptor.forClass(Request.class); verify(channel).execute(endpointCaptor.capture(), requestCaptor.capture()); UrlBuilder urlBuilder = mock(UrlBuilder.class); - endpointCaptor.getValue().renderPath(ImmutableMap.of(), urlBuilder); + endpointCaptor.getValue().renderPath(requestCaptor.getValue().pathParameters(), urlBuilder); verify(urlBuilder).pathSegment("foo"); // context path verify(urlBuilder).pathSegment("static0"); verify(urlBuilder).pathSegment("dynamic0"); @@ -205,7 +205,7 @@ public void testEmptyStringPathParameter() { ArgumentCaptor requestCaptor = ArgumentCaptor.forClass(Request.class); verify(channel).execute(endpointCaptor.capture(), requestCaptor.capture()); UrlBuilder urlBuilder = mock(UrlBuilder.class); - endpointCaptor.getValue().renderPath(ImmutableMap.of(), urlBuilder); + endpointCaptor.getValue().renderPath(requestCaptor.getValue().pathParameters(), urlBuilder); verify(urlBuilder).pathSegment("foo"); // context path verify(urlBuilder).pathSegment("begin"); verify(urlBuilder).pathSegment(""); @@ -222,12 +222,42 @@ public void testSlashPathParameter() { ArgumentCaptor requestCaptor = ArgumentCaptor.forClass(Request.class); verify(channel).execute(endpointCaptor.capture(), requestCaptor.capture()); UrlBuilder urlBuilder = mock(UrlBuilder.class); - endpointCaptor.getValue().renderPath(ImmutableMap.of(), urlBuilder); + endpointCaptor.getValue().renderPath(requestCaptor.getValue().pathParameters(), urlBuilder); verify(urlBuilder).pathSegment("foo"); // context path verify(urlBuilder).pathSegment("/"); // encoded into %2F by DefaultUrlBuilder } - static Channel stubNoContentResponseChannel() { + @Test + public void testCache() { + Channel channel = stubNoContentResponseChannel(); + StubService service = JaxRsClient.create(StubService.class, channel, runtime); + service.ping(); + service.ping(); + + ArgumentCaptor endpointCaptor = ArgumentCaptor.forClass(Endpoint.class); + verify(channel, times(2)).execute(endpointCaptor.capture(), any()); + + assertThat(endpointCaptor.getAllValues()).hasSize(2); + assertThat(endpointCaptor.getAllValues().get(0)) + .isSameAs(endpointCaptor.getAllValues().get(1)); + } + + @Test + public void testOverload() { + Channel channel = stubNoContentResponseChannel(); + StubService service = JaxRsClient.create(StubService.class, channel, runtime); + service.overload(); + service.overload("path"); + + ArgumentCaptor endpointCaptor = ArgumentCaptor.forClass(Endpoint.class); + verify(channel, times(2)).execute(endpointCaptor.capture(), any()); + + assertThat(endpointCaptor.getAllValues()).hasSize(2); + assertThat(endpointCaptor.getAllValues().get(0)) + .isNotSameAs(endpointCaptor.getAllValues().get(1)); + } + + private static Channel stubNoContentResponseChannel() { Channel channel = mock(Channel.class); Response response = mock(Response.class); when(response.body()).thenReturn(new ByteArrayInputStream(new byte[0])); @@ -262,6 +292,14 @@ public interface StubService { @GET @Path("begin/{path}/end") void innerPath(@PathParam("path") String path); + + @GET + @Path("overload") + void overload(); + + @GET + @Path("overload/{path}") + void overload(@PathParam("path") String path); } @Path("bar")