Feign: OptionalAwareDecoder in feign core?

[uschi2000]

Hej again,
How are the chances of getting a "feign-guava-optional" module (or similar) into feign that knows about 404-decoding Guava Optional? Such a module would have a Guava dependency and only make sense where decode404 is enabled.
R.

/**
 * Decorates a Feign {@link Decoder} such that it returns {@link Optional#absent} when observing an HTTP 404 error code
 * for a method with {@link Type} {@link Optional}. Propagates the exception returned by the given {@link ErrorDecoder}
 * in case the response is 404 for a non-Optional method.
 */
public final class OptionalAwareDecoder implements Decoder {

    private final Decoder delegate;
    private final ErrorDecoder errorDecoder;

    public OptionalAwareDecoder(Decoder delegate, ErrorDecoder errorDecoder) {
        this.delegate = delegate;
        this.errorDecoder = errorDecoder;
    }

    @Override
    public Object decode(Response response, Type type) throws IOException, FeignException {
        if (response.status() == 404) {
            if (Types.getRawType(type).equals(Optional.class)) {
                return Optional.absent();
            } else {
                throw Throwables.propagate(errorDecoder.decode(null, response));
            }
        } else {
            return delegate.decode(response, type);
        }
    }
}

[adriancole]

Thanks for the idea. First start would be finding those who'd also use
this. Let's see if this issue gets +1s. In the mean time, thanks for
pasting the solution.

[greyfairer]

+1 for me, allthough I need support for Java 8 Optional, too.

[ilaborie]

+1
but it's require a JDK 8

[spencergibb]

@adriancole this would be good as a separate project when we move feign to an org.

[adriancole]

this is probably doable with some trickery on the 8.x branch since Optional is a type, not a language feature cc @oillio

[ambition-consulting]

+1
I had enough NullPointerExceptions for this life.

[edio]

+1
We have similar thing in our project too.

[adriancole]

Boy I wish it actually made NPEs disappear! nothing more vexing than a
null Option pointer. :D

anyway...

OptionalAwareDecoder can be written as a decorator, which can wrap
something like jackson, and handle the 404 case. It can be done for
java 8, guava you name it in very few lines of code.

If optional means escalating into the decoders themselves, that would
be more work.

We do have an org, now, so depending on scope, it could go there. I'd
suggest if it is about java8's optional, making a feign-java8 repo.
Any takers?

https://github.com/OpenFeign

[mballoni]

I think this is a great idea.
I'm trying to implement an optional decoder like this one as a decorator for the jacksonDecoder.
However the problem is that it can't infer the Optional inner type for the json desserialization an then it try to desserialize the body as an Optional.
Despite that, it requires only a few lines.

[adriancole]

@mballoni something like this should work with whichever Option/Optional one wants

if (isOptional(type)) {
  if (response.status() == 404) {
    return Optional.absent();
  } else {
    Type enclosedType = Util.resolveLastTypeParameter(type, Optional.class);
    return Optional.of(delegate.decode(response, enclosedType));
  }
}
return delegate.decode(response, type);

---snip--
static boolean isOptional(Type type) {
  if (type instanceof ParameterizedType) { // must be parameterized, or otherwise we can't decode the inner type!
      ParameterizedType parameterizedType = (ParameterizedType) type;
      return parameterizedType.getRawType().equals(Optional.class);
  } else {
     return false;
  }
}

[adriancole]

actually the "if (type instanceof Class)" part could be skipped, as that's not decodable (implies Optional raw type, so you wouldn't know the inner type)

[adriancole]

updated my example

[arielsegura]

any news on this? is it merged/implemented?

[adriancole]

so far no-one has offered to make a library out of my code snippet pasted https://github.com/OpenFeign/feign/issues/294#issuecomment-248525836

that means you'd have to copy/paste it into your own delegating decoder for now.

[adriancole]

here's a complete example.

  @Test
  public void optional() throws IOException, InterruptedException {
    MockWebServer server = new MockWebServer();
    server.enqueue(new MockResponse().setResponseCode(404));
    server.enqueue(new MockResponse().setBody("foo"));

    OptionalInterface api = Feign.builder()
        .decode404()
        .decoder(new OptionalDecoder(new Decoder.Default()))
        .target(OptionalInterface.class, server.url("/").toString());

    assertThat(api.get().isPresent()).isFalse();
    assertThat(api.get().get()).isEqualTo("foo");
  }

  interface OptionalInterface {
    @RequestLine("GET /")
    Optional<String> get();
  }

  static final class OptionalDecoder implements Decoder {
    final Decoder delegate;

    OptionalDecoder(Decoder delegate) {
      this.delegate = delegate;
    }

    @Override public Object decode(Response response, Type type) throws IOException {
      if (!isOptional(type)) return delegate.decode(response, type);

      if (response.status() == 404) return Optional.empty();
      Type enclosedType = Util.resolveLastTypeParameter(type, Optional.class);
      return Optional.of(delegate.decode(response, enclosedType));
    }

    static boolean isOptional(Type type) {
      if (!(type instanceof ParameterizedType)) return false;
      ParameterizedType parameterizedType = (ParameterizedType) type;
      return parameterizedType.getRawType().equals(Optional.class);
    }
  }

[arielsegura]

Yes, I was able to do that but it would be great to include that in feign. I'm going on vacations today but will send a PR when I'm back :)

[arielsegura]

@adriancole how can we work out this? Feign doesn't support java 8 by default. Do you think it makes sense to build a separated artifact that contains OptionalDecoder?

[adriancole]

that or make an internal platform thing like okhttp has
https://github.com/square/okhttp/blob/master/okhttp/src/main/java/okhttp3/internal/platform/Platform.java

[ugrave]

Hi,
is there any update on this issue?

[adriancole]

help wanted basically

[arielsegura]

@adriancole I sent a PR with the changes. Sorry for the delay!

[arielsegura]

@adriancole this issue can be closed now