Add InvocationHandlerFactory.MethodHandlerCustomizer

[Sam-Kruglov]

This should make it easier for intergrations. Currently have to provide a capability to enrich the InvocationHandlerFactory itself to rebuild the "dispatch" map.

see #1513

[Sam-Kruglov]

this customizer basically acts just like one of enrich methods of Capability but I think it's too low level to be in there. Or is it?

[Sam-Kruglov]

this for loop is repeated twice, I could add a static method MethodHandlerCustomizer#customizeAll(List< MethodHandlerCustomizer>, Target, Method, MethodHandler) to make it a one-liner.

[velo]

This is duplicating the efforts of capabilities, but in a different format.

Let's refactor this as part of capability API

[Sam-Kruglov]

This is duplicating the efforts of capabilities, but in a different format.

Let's refactor this as part of capability API

Are you referring to this?
So, I should convert MethodHandlerCustomizer into a method Capability#enrich?

[Sam-Kruglov]

If it was an enrich method, it would be different. Every other method contains only one argument, but this one would contain more, as it also needs target and method. But I suppose we can still convert it into an enrich method that enriches the InvocationHandlerFactory like in the original issue

[kdavisk6]

I'm going to echo @velo here. The InvocationHandlerFactory provides Feign extension implementers with a way to replace the entire method invocation. To manage certain parts of the invocation, we have our regular dependencies, such as Encoder, Decoder, and the like. Use of a Capability provides type-safe enhancement around specific components within the invocation life-cycle, which appear to be more appropriate for your use case.

Caching, for instance, could be implemented as a Capability, or a new Client implementation, it just depends on where in the invocation life-cycle you want to manage it. Do you want Caching to kick in using method arguments? Then "enrich" the InvocationHandlerFactory that returns a "CacheAware" InvocationHandler.

Do you want it based on HTTP request? "Enrich" the Client, by returning a wrapped "CacheAware" Client.

The general rule here is, InvocationHandlerFactory is for replacing the entire invocation. It's definitely a sledgehammer, whereas in this case, a scalpel may be better.

[Sam-Kruglov]

@kdavisk6 if you look at #1513, I did it using capability but I thought we could add a more specific customizer just for the method handlers. Also, the capability solution does not cover DefaultMethodHandler as it's being used outside the factory (see lines 72 and 77 here):

feign/core/src/main/java/feign/ReflectiveFeign.java

Lines 72 to 78 in 63069fd

	InvocationHandler handler = factory.create(target, methodToHandler);
	T proxy = (T) Proxy.newProxyInstance(target.type().getClassLoader(),
	new Class<?>[] {target.type()}, handler);
	for (DefaultMethodHandler defaultMethodHandler : defaultMethodHandlers) {
	defaultMethodHandler.bindTo(proxy);
	}

[kdavisk6]

Default Method handler proxies default implementations and cannot be enriched. This is by design.

[Sam-Kruglov]

I see, thanks.

So, would something like this be ok? It's basically a convenience method doing exactly the same as what I'm doing in #1513

//new class
MethodInvocation {
  Target<?> target;
  Method method;
  Object[] arguments;
  MethodHandler handler;
  Object invoke() { return handler.invoke(arguments); }
}

Capability {
...

  default MethodInvocation enrich(MethodInvocation invocation)  {
    return invocation;
  }
}

Feign {
...
  Feign build() {
     ...
    invocationHandlerFactory = (target, dispatch) -> invocationHandlerFactory.create(target, 
        dispatch.entrySet().stream()
                .map(e -> Map.entry(e.getKey(), (MethodHandler) args -> Capability.enrich(new MethodInvocation(target, e.getKey(), args, e.getValue()), capabilities).invoke())
                .collect(toMap(Map.Entry::getKey, Map.Entry::getValue)));
    invocationHandlerFactory = Capability.enrich(invocationHandlerFactory, capabilities);
  }
}

//usage
new Capability() {
  MethodInvocation enrich(MethodInvocation invocation)  {
    return new MethodInvocation(...pass stuff from "invocation" or enriched...) {  ...override invoke()...  };
  }
}

[Sam-Kruglov]

@velo @kdavisk6 so, do you guys think I should change it to the way I suggested in my last message or just reject the PR and let people do it through invocation handler capability like here?

[velo]

Hi Sam,

You probably trying to achieve the same as we have for metrics, don't you?
https://github.com/OpenFeign/feign/blob/master/dropwizard-metrics5/src/main/java/feign/metrics5/MeteredInvocationHandleFactory.java#L63

Sounds like there is no need to change feign at all

[Sam-Kruglov]

Thanks, yes, you're right. I updated my PR on the spring side to do like you suggested: spring-cloud/spring-cloud-openfeign#608 (comment)