Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade Feign #1337

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions .palantir/revapi.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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: []
7 changes: 7 additions & 0 deletions changelog/@unreleased/pr-1337.v2.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
type: improvement
improvement:
description: Feign has been upgraded to 10.6.0. This involved a maven group change
from `com.netflix.feign` -> `io.github.openfeign`. This fixes the serialization
of absent optional header parameters.
links:
- https://github.com/palantir/conjure-java-runtime/pull/1337
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,11 @@ client:
- '{"10": true, "10e0": false}'
- '{"10": true, "10.0": false}'

singleHeaderService: {}
singleHeaderService:
headerString:
- '""'
headerAliasString:
- '""'

singlePathParamService:
pathParamString:
Expand Down
14 changes: 7 additions & 7 deletions conjure-java-jaxrs-client/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pkoenig10 I think the TODO above might be out of date... afaik the reason this has to be api is because people may want to catch a RetryableException


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"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand All @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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}.
*/
Expand All @@ -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(),
Expand All @@ -63,6 +62,4 @@ protected void processMetadata(Class<?> targetType, Method method, MethodMetadat
}
}
}

private static final Function<Annotation, Class<?>> EXTRACT_CLASS = input -> input.annotationType();
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
Expand All @@ -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;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,7 @@ protected void processMetadata(Class<?> _targetType, Method _method, MethodMetad
metadata.template().method() + " "
+ metadata.template()
.url()
// escape from feign string interpolation
// See RequestTemplate.expand
.replace("{", "{{"));
.replace("{", "<")
.replace("}", ">"));
}
}
Loading