Optional headers with an empty value are converted to the empty string

[DoronShapiro]

What happened?

I have an endpoint method with an optional argument annotated by @HeaderParam:

@GET
@Path("/doStuff")
String doStuff(@HeaderParam("value") Optional<String> value);

I passed it the value Optional.empty() via a JaxRsClient:

MyService service = JaxRsClient.create(...
service.doStuff(Optional.empty())

However, the implementation of doStuff receives the value Optional.of(""), rather than Optional.empty().

There is a failing test case to show this at https://github.com/palantir/http-remoting/compare/develop...DoronShapiro:optional-header-tests.

This behavior happens because the implementation of Java8OptionalAwareContract serializes empty headers as an empty string, but when they are deserialized by the Java8OptionalParamConverterProvider, it is assumed that the value will be null if empty (and that the empty string represents a non-empty value).

What did you want to happen?

The server should receive a value of Optional.empty() for the header argument.

One solution is for the client to not include the header at all if its value is Optional.empty().

[stale[bot]]

This issue has been automatically marked as stale because it has not been touched in the last 60 days. Please comment if you'd like to keep it open, otherwise it'll be closed in 7 days time.

[iamdanfox]

I think this is still a valid bug - keep open!

[stale[bot]]

This issue has been automatically marked as stale because it has not been touched in the last 60 days. Please comment if you'd like to keep it open, otherwise it'll be closed in 7 days time.

[uschi2000]

keep open

[stale[bot]]

This issue has been automatically marked as stale because it has not been touched in the last 60 days. Please comment if you'd like to keep it open, otherwise it'll be closed in 7 days time.

[pkoenig10]

This is pretty easy to fix, but requires upgrading Feign to get https://github.com/OpenFeign/feign/pull/724.

[pkoenig10]

Looking closer that PR doesn't actually fix the issue. The header value in the RequestTemplate is not null but is the template string: https://github.com/OpenFeign/feign/blob/ed2cef04658a28b5ca8c50645032df0d5c7fd7b7/core/src/main/java/feign/RequestTemplate.java#L150-L154

The tests in that PR pass because of a bug in RecordedRequestAssert#hasNoHeaderNamed: https://github.com/OpenFeign/feign/pull/835

The real fix is in https://github.com/OpenFeign/feign/pull/778, which completely overhauled the RequestTemplate.

[pkoenig10]

After doing some investigation, the latest version of Feign fixes this behavior for Optional.empty() values and does not include them in the request. However it also omits Optional.of("") values as well.

[carterkozak]

I think excluding empty string optional header values is more palatable than updating services to handle empty strings for null. Upgrading our feign version would allow us to stream binary responses without fully buffering them on heap as well.