Upgrade Feign

.palantir/revapi.yml

@@ -6,3 +6,36 @@ acceptedBreaks:
       old: null
       new: "method com.codahale.metrics.Timer com.palantir.conjure.java.okhttp.HostMetrics::getQos()"
       justification: "ABI-safe change to rarely used class, low impact"
+  4.43.0:
+    com.palantir.conjure.java.runtime:okhttp-clients: []
+    com.palantir.conjure.java.runtime:refresh-utils: []
+    com.palantir.conjure.java.runtime:jetty-http2-agent: []
+    com.palantir.conjure.java.runtime:keystores: []
+    com.palantir.conjure.java.runtime:pkcs1-reader-bouncy-castle: []
+    com.palantir.conjure.java.runtime:conjure-java-jackson-serialization: []
+    com.palantir.conjure.java.runtime:conjure-scala-jaxrs-client: []
+    com.palantir.conjure.java.runtime:conjure-java-jaxrs-client:
+    - 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"
+      new: null
+      justification: "{Feign method was renamed}"
+    - 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"
+      new: null
+      justification: "{Feign method was renamed}"
+    - 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"
+      new: null
+      justification: "{Feign method was renamed}"
+    - 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"
+      new: null
+      justification: "{Feign method was renamed}"
+    com.palantir.conjure.java.runtime:conjure-java-retrofit2-client: []
+    com.palantir.conjure.java.runtime:conjure-java-jersey-server: []
+    com.palantir.conjure.java.runtime:pkcs1-reader-sun: []
+    com.palantir.conjure.java.runtime:client-config: []

changelog/@unreleased/pr-1337.v2.yml

@@ -0,0 +1,6 @@
+type: improvement
+improvement:
+  description: Feign has been upgraded to 10.6.0. This fixes the serialization of
+    absent optional header parameters.
+  links:
+  - https://github.com/palantir/conjure-java-runtime/pull/1337

conjure-java-client-verifier/src/test/java/com/palantir/verification/SingleParamServicesTest.java

@@ -21,6 +21,7 @@
 import com.palantir.conjure.java.api.errors.RemoteException;
 import com.palantir.conjure.java.serialization.ObjectMappers;
 import com.palantir.conjure.verification.server.EndpointName;
+import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
 import java.lang.reflect.Type;
 import java.util.ArrayList;
@@ -129,8 +130,12 @@ public void runTestCase() throws Exception {
                     }
 
                     log.info("Successfully post param to endpoint {} and index {}", endpointName, index);
-                } catch (RemoteException e) {
-                    log.error("Caught exception with params: {}", e.getError().parameters(), e);
+                } catch (InvocationTargetException e) {
+                    Throwable targetException = e.getTargetException();
+                    if (targetException instanceof RemoteException) {
+                        RemoteException remoteException = (RemoteException) targetException;
+                        log.error("Caught exception with params: {}", remoteException.getError().parameters(), e);
+                    }
                     throw e;
                 }
             }

conjure-java-client-verifier/src/test/resources/ignored-test-cases.yml

@@ -77,7 +77,11 @@ client:
    - '{"10": true, "10e0": false}'
    - '{"10": true, "10.0": false}'
 
-  singleHeaderService: {}
+  singleHeaderService:
+    headerString:
+    - '""'
+    headerAliasString:
+    - '""'
 
   singlePathParamService:
    pathParamString:

conjure-java-jaxrs-client/build.gradle

@@ -8,26 +8,26 @@ dependencies {
     api "com.google.code.findbugs:jsr305"
     api "javax.ws.rs:javax.ws.rs-api"
     // 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"
 
     implementation project(":conjure-java-jackson-serialization")
     implementation project(":keystores")
     implementation "com.google.guava:guava"
     implementation "com.github.ben-manes.caffeine:caffeine"
-    implementation "com.netflix.feign:feign-jackson"
-    implementation("com.netflix.feign:feign-jaxrs") {
+    implementation "com.palantir.tracing:tracing-okhttp3"
+    implementation "io.github.openfeign:feign-jackson"
+    implementation("io.github.openfeign:feign-jaxrs") {
         // the shipped version clashes with the newer javax.ws.rs:javax.ws.rs-api used by (e.g.) dropwizard
         exclude group: "javax.ws.rs", module: "jsr311-api"
     }
-    implementation "com.netflix.feign:feign-okhttp"
-    implementation "com.netflix.feign:feign-slf4j"
-    implementation "com.palantir.tracing:tracing-okhttp3"
+    implementation "io.github.openfeign:feign-okhttp"
+    implementation "io.github.openfeign:feign-slf4j"
     implementation "org.slf4j:slf4j-api"
 
     testImplementation project(":conjure-java-retrofit2-client")
     testImplementation project(":conjure-java-jersey-server")
     testImplementation project(':keystores')
-    testImplementation "com.netflix.feign:feign-jackson"
+    testImplementation "io.github.openfeign:feign-jackson"
     testImplementation "com.squareup.okhttp3:mockwebserver"
     testImplementation("io.dropwizard:dropwizard-testing") { exclude module: 'metrics-core' }
     testImplementation "com.palantir.tracing:tracing-test-utils"

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
+ * Delegates the initial {@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.
  */
@@ -39,8 +39,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/GuavaEmptyOptionalExpander.java

@@ -23,7 +23,10 @@
 /**
  * Expands Optional by using the empty string for {@link com.google.common.base.Optional#absent()} and
  * the {@link Object#toString()} of the value otherwise.
+ *
+ * @deprecated no longer used
  */
+@Deprecated
 public final class GuavaEmptyOptionalExpander implements Expander {
 
     @Override

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

@@ -16,9 +16,6 @@
 
 package com.palantir.conjure.java.client.jaxrs.feignimpl;
 
-import com.google.common.base.Function;
-import com.google.common.collect.FluentIterable;
-import com.google.common.collect.Lists;
 import feign.Contract;
 import feign.MethodMetadata;
 import java.lang.annotation.Annotation;
@@ -28,9 +25,9 @@
 import javax.ws.rs.QueryParam;
 
 /**
- * Decorates a {@link Contract} and uses {@link GuavaNullOptionalExpander} for any {@link QueryParam} parameters,
- * {@link GuavaEmptyOptionalExpander} for any {@link HeaderParam} parameters, and throws a {@link RuntimeException}
- * at first encounter of an {@link com.google.common.base.Optional} typed {@link PathParam}.
+ * Decorates a {@link Contract} and uses {@link GuavaNullOptionalExpander} for any {@link QueryParam} parametersor
+ * {@link HeaderParam} parameters, and throws a {@link RuntimeException} at the first encounter of an
+ * {@link com.google.common.base.Optional} typed {@link PathParam}.
  * <p>
  * {@link PathParam}s require a value, and so we explicitly disallow use with {@link com.google.common.base.Optional}.
  */
@@ -46,14 +43,16 @@ protected void processMetadata(Class<?> targetType, Method method, MethodMetadat
         Annotation[][] annotations = method.getParameterAnnotations();
         for (int i = 0; i < parameterTypes.length; i++) {
             Class<?> cls = parameterTypes[i];
-            if (cls.equals(com.google.common.base.Optional.class)) {
-                FluentIterable<Class<?>> paramAnnotations =
-                        FluentIterable.from(Lists.newArrayList(annotations[i])).transform(EXTRACT_CLASS);
-                if (paramAnnotations.contains(HeaderParam.class)) {
-                    metadata.indexToExpanderClass().put(i, GuavaEmptyOptionalExpander.class);
-                } else if (paramAnnotations.contains(QueryParam.class)) {
+            if (!cls.equals(com.google.common.base.Optional.class)) {
+                continue;
+            }
+
+            for (Annotation annotation : annotations[i]) {
+                Class<? extends Annotation> annotationType = annotation.annotationType();
+                if (annotationType.equals(HeaderParam.class)
+                        || annotationType.equals(QueryParam.class)) {
                     metadata.indexToExpanderClass().put(i, GuavaNullOptionalExpander.class);
-                } else if (paramAnnotations.contains(PathParam.class)) {
+                } else if (annotationType.equals(PathParam.class)) {
                     throw new RuntimeException(String.format(
                             "Cannot use Guava Optionals with PathParams. (Class: %s, Method: %s, Param: arg%d)",
                             targetType.getName(),
@@ -63,6 +62,4 @@ protected void processMetadata(Class<?> targetType, Method method, MethodMetadat
             }
         }
     }
-
-    private static final Function<Annotation, Class<?>> EXTRACT_CLASS = input -> input.annotationType();
 }

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

@@ -24,7 +24,10 @@
 /**
  * Expands OptionalDouble by using the empty string for {@link OptionalDouble#empty()} and the
  * {@link Double#toString()} of the value otherwise.
+ *
+ * @deprecated no longer used
  */
+@Deprecated
 public final class Java8EmptyOptionalDoubleExpander implements Expander {
 
     @Override

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

@@ -24,7 +24,10 @@
 /**
  * Expands Optional by using the empty string for {@link Optional#empty()} and the {@link Object#toString()} of
  * the value otherwise.
+ *
+ * @deprecated no longer used
  */
+@Deprecated
 public final class Java8EmptyOptionalExpander implements Expander {
 
     @Override

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

@@ -24,7 +24,10 @@
 /**
  * Expands OptionalInt by using the empty string for {@link OptionalInt#empty()} and the {@link Integer#toString()} of
  * the value otherwise.
+ *
+ * @deprecated no longer used
  */
+@Deprecated
 public final class Java8EmptyOptionalIntExpander implements Expander {
 
     @Override

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

@@ -24,7 +24,10 @@
 /**
  * Expands OptionalLong by using the empty string for {@link OptionalLong#empty()} and the {@link Long#toString()} of
  * the value otherwise.
+ *
+ * @deprecated no longer used
  */
+@Deprecated
 public final class Java8EmptyOptionalLongExpander implements Expander {
 
     @Override

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

@@ -16,10 +16,7 @@
 
 package com.palantir.conjure.java.client.jaxrs.feignimpl;
 
-import com.google.common.base.Function;
-import com.google.common.collect.FluentIterable;
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.Lists;
 import feign.Contract;
 import feign.MethodMetadata;
 import feign.Param.Expander;
@@ -35,23 +32,19 @@
 import javax.ws.rs.QueryParam;
 
 /**
- * Decorates a {@link Contract} and uses {@link Java8NullOptionalExpander} for any {@link QueryParam} parameters,
- * {@link Java8EmptyOptionalExpander} for any {@link HeaderParam} parameters, and throws a {@link RuntimeException} at
- * first encounter of an {@link Optional} typed {@link PathParam}.
+ * Decorates a {@link Contract} and uses {@link Java8NullOptionalExpander} for any {@link QueryParam} or
+ * {@link HeaderParam} parameters, and throws a {@link RuntimeException} at the first encounter of an {@link Optional}
+ * typed {@link PathParam}.
  * <p>
  * {@link PathParam}s require a value, and so we explicitly disallow use with {@link Optional}.
  */
 public final class Java8OptionalAwareContract extends AbstractDelegatingContract {
 
     private static final List<ExpanderDef> expanders = ImmutableList.of(
-            new ExpanderDef(Optional.class, Java8EmptyOptionalExpander.class, Java8NullOptionalExpander.class),
-            new ExpanderDef(OptionalInt.class, Java8EmptyOptionalIntExpander.class, Java8NullOptionalIntExpander.class),
-            new ExpanderDef(OptionalDouble.class,
-                    Java8EmptyOptionalDoubleExpander.class,
-                    Java8NullOptionalDoubleExpander.class),
-            new ExpanderDef(OptionalLong.class,
-                    Java8EmptyOptionalLongExpander.class,
-                    Java8NullOptionalLongExpander.class));
+            new ExpanderDef(Optional.class, Java8NullOptionalExpander.class),
+            new ExpanderDef(OptionalInt.class, Java8NullOptionalIntExpander.class),
+            new ExpanderDef(OptionalDouble.class, Java8NullOptionalDoubleExpander.class),
+            new ExpanderDef(OptionalLong.class, Java8NullOptionalLongExpander.class));
 
     public Java8OptionalAwareContract(Contract delegate) {
         super(delegate);
@@ -64,59 +57,37 @@ protected void processMetadata(Class<?> targetType, Method method, MethodMetadat
         for (int i = 0; i < parameterTypes.length; i++) {
             Class<?> cls = parameterTypes[i];
             for (ExpanderDef def : expanders) {
-                if (cls.equals(def.match)) {
-                    FluentIterable<Class<?>> paramAnnotations = getAnnotations(annotations, i);
-                    configureOptionalExpanders(targetType,
-                            method,
-                            metadata,
-                            i,
-                            paramAnnotations,
-                            def.emptyExpanderClass,
-                            def.nullExpanderClass);
+                if (!cls.equals(def.match)) {
+                    continue;
                 }
-            }
-        }
-    }
-
-    private FluentIterable<Class<?>> getAnnotations(Annotation[][] annotations, int index) {
-        return FluentIterable.from(Lists.newArrayList(annotations[index])).transform(EXTRACT_CLASS);
-    }
 
-    private void configureOptionalExpanders(
-            Class<?> targetType,
-            Method method,
-            MethodMetadata metadata,
-            int index,
-            FluentIterable<Class<?>> paramAnnotations,
-            Class<? extends Expander> emptyExpanderClass,
-            Class<? extends Expander> nullExpanderClass) {
-        if (paramAnnotations.contains(HeaderParam.class)) {
-            metadata.indexToExpanderClass().put(index, emptyExpanderClass);
-        } else if (paramAnnotations.contains(QueryParam.class)) {
-            metadata.indexToExpanderClass().put(index, nullExpanderClass);
-        } else if (paramAnnotations.contains(PathParam.class)) {
-            throw new RuntimeException(String.format(
-                    "Cannot use Java8 Optionals with PathParams. (Class: %s, Method: %s, Param: arg%d)",
-                    targetType.getName(),
-                    method.getName(),
-                    index));
+                for (Annotation annotation : annotations[i]) {
+                    Class<? extends Annotation> annotationType = annotation.annotationType();
+                    if (annotationType.equals(HeaderParam.class)
+                            || annotationType.equals(QueryParam.class)) {
+                        metadata.indexToExpanderClass().put(i, def.expanderClass);
+                        break;
+                    } else if (annotationType.equals(PathParam.class)) {
+                        throw new RuntimeException(String.format(
+                                "Cannot use Java8 Optionals with PathParams. (Class: %s, Method: %s, Param: arg%d)",
+                                targetType.getName(),
+                                method.getName(),
+                                i));
+                    }
+                }
+            }
         }
     }
 
-    private static final Function<Annotation, Class<?>> EXTRACT_CLASS = input -> input.annotationType();
-
     private static final class ExpanderDef {
         private final Class<?> match;
-        private final Class<? extends Expander> emptyExpanderClass;
-        private final Class<? extends Expander> nullExpanderClass;
+        private final Class<? extends Expander> expanderClass;
 
         ExpanderDef(
                 Class<?> match,
-                Class<? extends Expander> emptyExpanderClass,
-                Class<? extends Expander> nullExpanderClass) {
+                Class<? extends Expander> expanderClass) {
             this.match = match;
-            this.emptyExpanderClass = emptyExpanderClass;
-            this.nullExpanderClass = nullExpanderClass;
+            this.expanderClass = expanderClass;
         }
     }
 }

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

@@ -105,7 +105,7 @@ public void testAbsentQuery() throws Exception {
     public void testEmptyStringQuery() throws Exception {
         proxy.query(com.google.common.base.Optional.of(""), "str2");
         RecordedRequest takeRequest = server.takeRequest();
-        assertThat(takeRequest.getRequestLine()).isEqualTo("GET /foo?opt=&req=str2 HTTP/1.1");
+        assertThat(takeRequest.getRequestLine()).isEqualTo("GET /foo?opt&req=str2 HTTP/1.1");
     }
 
     @Test
@@ -119,15 +119,15 @@ public void testStringQuery() throws Exception {
     public void testAbsentHeader() throws Exception {
         proxy.header(com.google.common.base.Optional.absent(), "str2");
         RecordedRequest takeRequest = server.takeRequest();
-        assertThat(takeRequest.getHeader("opt")).isEqualTo("");
+        assertThat(takeRequest.getHeader("opt")).isEqualTo(null);
         assertThat(takeRequest.getHeader("req")).isEqualTo("str2");
     }
 
     @Test
     public void testEmptyStringHeader() throws Exception {
         proxy.header(com.google.common.base.Optional.of(""), "str2");
         RecordedRequest takeRequest = server.takeRequest();
-        assertThat(takeRequest.getHeader("opt")).isEqualTo("");
+        assertThat(takeRequest.getHeader("opt")).isEqualTo(null);
         assertThat(takeRequest.getHeader("req")).isEqualTo("str2");
     }