Bump feign to openfeign 13

.palantir/revapi.yml

@@ -85,3 +85,52 @@ acceptedBreaks:
       new: "method T com.palantir.conjure.java.clients.ConjureClients.WithClientOptions<T>::withConjureErrorParameterFormat(com.palantir.conjure.java.api.errors.ConjureErrorParameterFormat)"
       justification: "Adding method to allow clients to request error parameter serialization\
         \ format"
+  "8.30.0":
+    com.palantir.conjure.java.runtime:conjure-java-jaxrs-client:
+    - code: "java.method.removed"
+      old: "method feign.MethodMetadata feign.Contract.BaseContract::parseAndValidatateMetadata(java.lang.reflect.Method)\
+        \ @ com.palantir.conjure.java.client.jaxrs.CompatibleJaxRsContract"
+      justification: "Breaks are downstream of feign bump. We are bumping the major\
+        \ version."
+    - code: "java.method.removed"
+      old: "method java.util.List<feign.MethodMetadata> com.palantir.conjure.java.client.jaxrs.feignimpl.AbstractDelegatingContract::parseAndValidatateMetadata(java.lang.Class<?>)\
+        \ @ com.palantir.conjure.java.client.jaxrs.feignimpl.EndpointNameHeaderEnrichmentContract"
+      justification: "Breaks are downstream of feign bump. We are bumping the major\
+        \ version."
+    - code: "java.method.removed"
+      old: "method java.util.List<feign.MethodMetadata> com.palantir.conjure.java.client.jaxrs.feignimpl.AbstractDelegatingContract::parseAndValidatateMetadata(java.lang.Class<?>)\
+        \ @ com.palantir.conjure.java.client.jaxrs.feignimpl.GuavaOptionalAwareContract"
+      justification: "Breaks are downstream of feign bump. We are bumping the major\
+        \ version."
+    - code: "java.method.removed"
+      old: "method java.util.List<feign.MethodMetadata> com.palantir.conjure.java.client.jaxrs.feignimpl.AbstractDelegatingContract::parseAndValidatateMetadata(java.lang.Class<?>)\
+        \ @ com.palantir.conjure.java.client.jaxrs.feignimpl.Java8OptionalAwareContract"
+      justification: "Breaks are downstream of feign bump. We are bumping the major\
+        \ version."
+    - code: "java.method.removed"
+      old: "method java.util.List<feign.MethodMetadata> com.palantir.conjure.java.client.jaxrs.feignimpl.AbstractDelegatingContract::parseAndValidatateMetadata(java.lang.Class<?>)\
+        \ @ com.palantir.conjure.java.client.jaxrs.feignimpl.MethodHeaderEnrichmentContract"
+      justification: "Breaks are downstream of feign bump. We are bumping the major\
+        \ version."
+    - code: "java.method.removed"
+      old: "method java.util.List<feign.MethodMetadata> com.palantir.conjure.java.client.jaxrs.feignimpl.AbstractDelegatingContract::parseAndValidatateMetadata(java.lang.Class<?>)\
+        \ @ com.palantir.conjure.java.client.jaxrs.feignimpl.PathTemplateHeaderEnrichmentContract"
+      justification: "Breaks are downstream of feign bump. We are bumping the major\
+        \ version."
+    - code: "java.method.removed"
+      old: "method java.util.List<feign.MethodMetadata> com.palantir.conjure.java.client.jaxrs.feignimpl.AbstractDelegatingContract::parseAndValidatateMetadata(java.lang.Class<?>)\
+        \ @ com.palantir.conjure.java.client.jaxrs.feignimpl.SlashEncodingContract"
+      justification: "Breaks are downstream of feign bump. We are bumping the major\
+        \ version."
+    - code: "java.method.removed"
+      old: "method java.util.List<feign.MethodMetadata> feign.Contract.BaseContract::parseAndValidatateMetadata(java.lang.Class<?>)\
+        \ @ com.palantir.conjure.java.client.jaxrs.CompatibleJaxRsContract"
+      justification: "Breaks are downstream of feign bump. We are bumping the major\
+        \ version."
+    - code: "java.method.visibilityReduced"
+      old: "method java.util.Collection<java.lang.String> feign.Contract.BaseContract::addTemplatedParam(java.util.Collection<java.lang.String>,\
+        \ java.lang.String) @ com.palantir.conjure.java.client.jaxrs.CompatibleJaxRsContract"
+      new: "method java.util.Collection<java.lang.String> com.palantir.conjure.java.client.jaxrs.CompatibleJaxRsContract::addTemplatedParam(java.util.Collection<java.lang.String>,\
+        \ java.lang.String)"
+      justification: "Breaks are downstream of feign bump. We are bumping the major\
+        \ version."

conjure-java-jaxrs-client/build.gradle

@@ -8,7 +8,7 @@ dependencies {
     api project(":conjure-java-legacy-clients")
     api "com.google.code.findbugs:jsr305"
     // TODO(dsanduleac): Should be implementation, but can't because we expose feign.TextDelegateEncoder
-    api "com.netflix.feign:feign-core", {
+    api "io.github.openfeign:feign-core", {
         // prefer jakarta.ws.rs:jakarta.ws.rs-api
         exclude group: 'javax.ws.rs', module: 'javax.ws.rs-api'
     }
@@ -35,11 +35,11 @@ dependencies {
     implementation project(":conjure-java-jackson-serialization")
     implementation "com.google.guava:guava"
     implementation "com.github.ben-manes.caffeine:caffeine"
-    implementation "com.netflix.feign:feign-jackson"
+    implementation "io.github.openfeign:feign-jackson"
     testImplementation project(":conjure-java-jersey-jakarta-server")
     testImplementation project(':keystores')
     testImplementation project(':undertow-jakarta-testing')
-    testImplementation "com.netflix.feign:feign-jackson"
+    testImplementation "io.github.openfeign:feign-jackson"
     testImplementation "com.squareup.okhttp3:mockwebserver"
     testImplementation "org.junit.jupiter:junit-jupiter"
     testImplementation 'org.junit.jupiter:junit-jupiter-api'

conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/AbstractFeignJaxRsClientBuilder.java

@@ -127,7 +127,10 @@ static <T> T create(
      * which makes every client appear to use the same URL.
      */
     private static final class FeignDialogueTarget<T> implements Target<T> {
-        private static final String BASE_URL = "dialogue://feign";
+        // Openfeign 13 seems to have a weird thing where it assumes any URL that doesn't start with "http" isn't
+        // absolute and thus causes apply to fail below when it calls RequestTemplate#target. Hopefully nobody
+        // is relying on this URL?
+        private static final String BASE_URL = "httpdialogue://feign";
 
         private final Class<T> serviceClass;
         private final Target<T> delegate;

conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/CompatibleJaxRsContract.java

@@ -23,8 +23,10 @@
 import com.palantir.logsafe.exceptions.SafeIllegalStateException;
 import feign.Contract;
 import feign.MethodMetadata;
+import feign.Request.HttpMethod;
 import java.lang.annotation.Annotation;
 import java.lang.reflect.Method;
+import java.util.ArrayList;
 import java.util.Collection;
 import javax.annotation.Nullable;
 
@@ -50,7 +52,7 @@ protected void processAnnotationOnClass(MethodMetadata data, Class<?> clz) {
                 // Strip off any trailing slashes, since the template has already had slashes appropriately added
                 pathValue = pathValue.substring(0, pathValue.length() - 1);
             }
-            data.template().insert(0, pathValue);
+            data.template().uri(pathValue);
         }
         Annotation consumes = Annotations.CONSUMES.getAnnotation(clz);
         if (consumes != null) {
@@ -74,7 +76,9 @@ protected void processAnnotationOnMethod(MethodMetadata data, Annotation methodA
                     SafeArg.of("method", method.getName()),
                     SafeArg.of("existingMethod", data.template().method()),
                     SafeArg.of("newMethod", httpValue));
-            data.template().method(Preconditions.checkNotNull(httpValue, "Unexpected null HttpMethod value"));
+            data.template()
+                    .method(Preconditions.checkNotNull(
+                            HttpMethod.valueOf(httpValue), "Unexpected null HttpMethod value"));
         } else if (Annotations.PATH.matches(annotationType)) {
             String pathValue = Strings.emptyToNull(getAnnotationValue(methodAnnotation));
             Preconditions.checkState(
@@ -85,7 +89,7 @@ protected void processAnnotationOnMethod(MethodMetadata data, Annotation methodA
             // jax-rs allows whitespace around the param name, as well as an optional regex. The contract should
             // strip these out appropriately.
             pathValue = pathValue.replaceAll("\\{\\s*(.+?)\\s*(:.+?)?\\}", "\\{$1\\}");
-            data.template().append(pathValue);
+            data.template().uri(pathValue, true);
         } else if (Annotations.PRODUCES.matches(annotationType)) {
             handleProducesAnnotation(data, methodAnnotation, "method " + method.getName());
         } else if (Annotations.CONSUMES.matches(annotationType)) {
@@ -180,4 +184,13 @@ private static Object getAnnotationValueInternal(Annotation annotation) {
                     "Failed to read annotation value", e, SafeArg.of("annotationType", annotation.annotationType()));
         }
     }
+
+    private Collection<String> addTemplatedParam(Collection<String> possiblyNull, String name) {
+        Collection<String> toUse = possiblyNull;
+        if (toUse == null) {
+            toUse = new ArrayList<>();
+        }
+        toUse.add(String.format("{%s}", name));
+        return toUse;
+    }
 }

conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/DialogueFeignClient.java

@@ -53,6 +53,7 @@
 import java.io.SequenceInputStream;
 import java.io.StringReader;
 import java.net.URLDecoder;
+import java.nio.charset.Charset;
 import java.nio.charset.StandardCharsets;
 import java.util.Collection;
 import java.util.Collections;
@@ -110,7 +111,8 @@ public feign.Response execute(Request request, Request.Options _options) throws
                 endpointChannels.computeIfAbsent(FeignEndpointKey.of(request), this::toEndpointChannel);
 
         try {
-            return runtime.clients().callBlocking(endpointChannel, builder.build(), FeignResponseDeserializer.INSTANCE);
+            return runtime.clients()
+                    .callBlocking(endpointChannel, builder.build(), new FeignResponseDeserializer(request));
         } catch (UncheckedExecutionException e) {
             // Rethrow IOException to match standard feign behavior
             Throwable cause = e.getCause();
@@ -236,6 +238,11 @@ public InputStream asInputStream() {
 
         @Override
         public Reader asReader() {
+            return asReader(StandardCharsets.UTF_8);
+        }
+
+        @Override
+        public Reader asReader(Charset charset) {
             InputStream inputStream = asInputStream();
             Integer maybeLength = length();
             if (maybeLength != null) {
@@ -251,7 +258,7 @@ public Reader asReader() {
                         if (read <= length) {
                             // fully read input
                             inputStream.close();
-                            return new StringReader(new String(bytes, 0, read, StandardCharsets.UTF_8));
+                            return new StringReader(new String(bytes, 0, read, charset));
                         }
                         // input was larger than provided content length, fallback to stream path
                         inputStream = new SequenceInputStream(new ByteArrayInputStream(bytes), inputStream);
@@ -261,7 +268,7 @@ public Reader asReader() {
                     }
                 }
             }
-            return new InputStreamReader(inputStream, StandardCharsets.UTF_8);
+            return new InputStreamReader(inputStream, charset);
         }
 
         @Override
@@ -275,16 +282,21 @@ public String toString() {
         }
     }
 
-    enum FeignResponseDeserializer implements Deserializer<feign.Response> {
-        INSTANCE;
+    private static final class FeignResponseDeserializer implements Deserializer<feign.Response> {
+        private final Request request;
+
+        FeignResponseDeserializer(Request request) {
+            this.request = request;
+        }
 
         @Override
         public feign.Response deserialize(Response response) {
-            return feign.Response.create(
-                    response.code(),
-                    null,
-                    Multimaps.asMap((Multimap<String, String>) response.headers()),
-                    new DialogueResponseBody(response));
+            return feign.Response.builder()
+                    .status(response.code())
+                    .headers(Multimaps.asMap((Multimap<String, String>) response.headers()))
+                    .body(new DialogueResponseBody(response))
+                    .request(request)
+                    .build();
         }
 
         @Override
@@ -464,7 +476,7 @@ interface FeignEndpointKey {
 
         private static FeignEndpointKey of(Request request) {
             return ImmutableFeignEndpointKey.of(
-                    HttpMethod.valueOf(request.method().toUpperCase(Locale.ENGLISH)),
+                    HttpMethod.valueOf(request.httpMethod().name().toUpperCase(Locale.ENGLISH)),
                     getFirstHeader(request, MethodHeaderEnrichmentContract.METHOD_HEADER)
                             .orElse(""),
                     getFirstHeader(request, EndpointNameHeaderEnrichmentContract.ENDPOINT_NAME_HEADER)

conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/feignimpl/AbstractDelegatingContract.java

@@ -26,7 +26,7 @@
 
 /**
  * Base class that provides the structure for a delegating {@link Contract}. Delegates the initial
- * {@link #parseAndValidatateMetadata(Class)} call to the wrapped Contract and then calls {@link #processMetadata(Class,
+ * {@link #parseAndValidateMetadata(Class)} call to the wrapped Contract and then calls {@link #processMetadata(Class,
  * Method, MethodMetadata)} on all of the methods that have metadata from the initial call.
  */
 abstract class AbstractDelegatingContract implements Contract {
@@ -38,8 +38,8 @@ abstract class AbstractDelegatingContract implements Contract {
     }
 
     @Override
-    public final List<MethodMetadata> parseAndValidatateMetadata(Class<?> targetType) {
-        List<MethodMetadata> mdList = delegate.parseAndValidatateMetadata(targetType);
+    public final List<MethodMetadata> parseAndValidateMetadata(Class<?> targetType) {
+        List<MethodMetadata> mdList = delegate.parseAndValidateMetadata(targetType);
 
         Map<String, MethodMetadata> methodMetadataByConfigKey = new LinkedHashMap<String, MethodMetadata>();
         for (MethodMetadata md : mdList) {

conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/feignimpl/CborDelegateDecoder.java

@@ -49,8 +49,7 @@ public CborDelegateDecoder(ObjectMapper cborMapper, Decoder delegate) {
 
     @Override
     public Object decode(Response response, Type type) throws IOException, FeignException {
-        Collection<String> contentTypes =
-                HeaderAccessUtils.caseInsensitiveGet(response.headers(), HttpHeaders.CONTENT_TYPE);
+        Collection<String> contentTypes = response.headers().get(HttpHeaders.CONTENT_TYPE);
         if (contentTypes == null) {
             contentTypes = ImmutableSet.of();
         }

conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/feignimpl/CborDelegateEncoder.java

@@ -52,8 +52,7 @@ public CborDelegateEncoder(ObjectMapper cborMapper, Encoder delegate) {
 
     @Override
     public void encode(Object object, Type bodyType, RequestTemplate template) throws EncodeException {
-        Collection<String> contentTypes =
-                HeaderAccessUtils.caseInsensitiveGet(template.headers(), HttpHeaders.CONTENT_TYPE);
+        Collection<String> contentTypes = template.headers().get(HttpHeaders.CONTENT_TYPE);
         if (contentTypes == null) {
             contentTypes = ImmutableSet.of();
         }

conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/feignimpl/HeaderAccessUtils.java

@@ -28,6 +28,7 @@
  * map which is case-insensitive with respect to the key. com.netflix.feign:feign-core:8.18.0 will have it for the
  * {@link feign.Response} headers due to https://github.com/Netflix/feign/pull/418.
  */
+// XXX: Delete
 public final class HeaderAccessUtils {
     private HeaderAccessUtils() {}
 

conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/feignimpl/TextDelegateDecoder.java

@@ -42,8 +42,7 @@ public TextDelegateDecoder(Decoder delegate) {
 
     @Override
     public Object decode(Response response, Type type) throws IOException, FeignException {
-        Collection<String> contentTypes =
-                HeaderAccessUtils.caseInsensitiveGet(response.headers(), HttpHeaders.CONTENT_TYPE);
+        Collection<String> contentTypes = response.headers().get(HttpHeaders.CONTENT_TYPE);
         if (contentTypes == null) {
             contentTypes = ImmutableSet.of();
         }

conjure-java-jaxrs-client/src/main/java/com/palantir/conjure/java/client/jaxrs/feignimpl/TextDelegateEncoder.java

@@ -40,8 +40,7 @@ public TextDelegateEncoder(Encoder delegate) {
 
     @Override
     public void encode(Object object, Type bodyType, RequestTemplate template) throws EncodeException {
-        Collection<String> contentTypes =
-                HeaderAccessUtils.caseInsensitiveGet(template.headers(), HttpHeaders.CONTENT_TYPE);
+        Collection<String> contentTypes = template.headers().get(HttpHeaders.CONTENT_TYPE);
         if (contentTypes == null) {
             contentTypes = ImmutableSet.of();
         }

conjure-java-jaxrs-client/src/test/java/com/palantir/conjure/java/client/jaxrs/JaxRsClientDialogueEndpointTest.java

@@ -144,7 +144,6 @@ public void testUnsupportedHttpMethod_trace() {
         Channel channel = mock(Channel.class);
         assertThatThrownBy(() -> JaxRsClient.create(TraceService.class, channel, runtime))
                 .isInstanceOf(IllegalArgumentException.class)
-                .hasMessageContaining("Unsupported HTTP method")
                 .hasMessageContaining("TRACE");
     }
 
@@ -153,7 +152,6 @@ public void testUnsupportedHttpMethod_arbitrary() {
         Channel channel = mock(Channel.class);
         assertThatThrownBy(() -> JaxRsClient.create(ArbitraryMethodService.class, channel, runtime))
                 .isInstanceOf(IllegalArgumentException.class)
-                .hasMessageContaining("Unsupported HTTP method")
                 .hasMessageContaining("ARBITRARY");
     }
 

conjure-java-jaxrs-client/src/test/java/com/palantir/conjure/java/client/jaxrs/JaxrsClientValidationTest.java

@@ -31,7 +31,7 @@ public void testNonJaxrs() {
         Channel channel = mock(Channel.class);
         assertThatThrownBy(() -> JaxRsClient.create(NotJaxRs.class, channel, runtime))
                 .isInstanceOf(IllegalStateException.class)
-                .hasMessageContaining("Method getValue not annotated with HTTP method type");
+                .hasMessageContaining("not annotated with HTTP method type");
     }
 
     @SuppressWarnings("unused")

conjure-java-jaxrs-client/src/test/java/com/palantir/conjure/java/client/jaxrs/feignimpl/EmptyContainerDecoderTest.java

@@ -31,12 +31,12 @@
 import com.google.common.collect.ImmutableSet;
 import com.google.common.net.HttpHeaders;
 import com.palantir.conjure.java.serialization.ObjectMappers;
+import feign.Request;
 import feign.Response;
 import feign.codec.Decoder;
 import jakarta.ws.rs.core.MediaType;
 import java.io.IOException;
 import java.nio.charset.StandardCharsets;
-import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
@@ -51,19 +51,27 @@
 public class EmptyContainerDecoderTest {
 
     private static final JsonMapper mapper = ObjectMappers.newClientJsonMapper();
-    private static final Response HTTP_204 = Response.create(204, "No Content", Collections.emptyMap(), new byte[] {});
+    private static final Request REQUEST =
+            Request.create(Request.HttpMethod.GET, "url", Map.of(), new byte[] {}, StandardCharsets.UTF_8);
+    private static final Response HTTP_204 = Response.builder()
+            .status(204)
+            .reason("No content")
+            .request(REQUEST)
+            .body(new byte[] {})
+            .build();
     private final Decoder delegate = mock(Decoder.class);
     private final EmptyContainerDecoder emptyContainerDecoder = new EmptyContainerDecoder(mapper, delegate);
 
     @Test
     public void http_200_uses_delegate_decoder() throws IOException {
         when(delegate.decode(any(), eq(String.class))).thenReturn("text response");
-        Response http200 = Response.create(
-                200,
-                "OK",
-                ImmutableMap.of(HttpHeaders.CONTENT_TYPE, ImmutableSet.of(MediaType.TEXT_PLAIN)),
-                "text response",
-                StandardCharsets.UTF_8);
+        Response http200 = Response.builder()
+                .status(200)
+                .reason("OK")
+                .headers(ImmutableMap.of(HttpHeaders.CONTENT_TYPE, ImmutableSet.of(MediaType.TEXT_PLAIN)))
+                .request(REQUEST)
+                .body("text response", StandardCharsets.UTF_8)
+                .build();
 
         emptyContainerDecoder.decode(http200, String.class);
         verify(delegate, times(1)).decode(any(), any());