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

GatewayProxyFactoryBean should not proxy default methods in the interface #3118

Closed
artembilan opened this issue Dec 6, 2019 · 3 comments
Closed

Comments

@artembilan
Copy link
Member

@Autowired
	private Function<Object, Message<?>> functionGateay;

	@Test
	void testHeadersFromFunctionGateway() {
		Object payload = this.functionGateay
				.andThen(message -> {
					assertThat(message.getHeaders()).containsKeys("gatewayMethod", "gatewayArgs");
					return message.getPayload();
				})
				.apply("testPayload");

		assertThat(payload).isEqualTo("testPayload");
	}

So, we definitely would like to use default methods in the interface as well, but it turns out they are proxied as well leading to unexpected behavior when we don't call a real default method, but send a message downstream instead.

On the other hand it is not so easy to exclude those methods from proxing and their normal logic:

			NameMatchMethodPointcutAdvisor gatewayAdvisor = new NameMatchMethodPointcutAdvisor(this);

			Method[] methods =
					ReflectionUtils.getUniqueDeclaredMethods(this.serviceInterface,
							method -> Modifier.isAbstract(method.getModifiers()));
			for (Method method : methods) {
				MethodInvocationGateway gateway = createGatewayForMethod(method);
				this.gatewayMap.put(method, gateway);
				gatewayAdvisor.addMethodName(method.getName());
			}

			ProxyFactory gatewayProxyFactory =
					new ProxyFactory(this.serviceInterface, AdvisedSupport.EMPTY_TARGET_SOURCE);
			gatewayProxyFactory.addAdvisor(gatewayAdvisor);
			this.serviceProxy = gatewayProxyFactory.getProxy(this.beanClassLoader);

When a default method in called we have a target for reflection as null leading to NPE.

Spring Data has something like this DefaultMethodInvokingMethodInterceptor - need to investigate what is the beast.

@artembilan
Copy link
Member Author

The DefaultMethodInvokingMethodInterceptor doesn't work for Function.class proxing use-cases:

Caused by: java.lang.IllegalArgumentException: illegal lookupClass: interface java.util.function.Function
	at java.lang.invoke.MethodHandleStatics.newIllegalArgumentException(MethodHandleStatics.java:139)
	at java.lang.invoke.MethodHandles$Lookup.checkUnprivilegedlookupClass(MethodHandles.java:686)
	at java.lang.invoke.MethodHandles$Lookup.<init>(MethodHandles.java:604)

The code in Java 8 is like this:

   // For caller-sensitive MethodHandles.lookup()
            // disallow lookup more restricted packages
            if (allowedModes == ALL_MODES && lookupClass.getClassLoader() == null) {
                if (name.startsWith("java.") ||
                        (name.startsWith("sun.")
                                && !name.startsWith("sun.invoke.")
                                && !name.equals("sun.reflect.ReflectionFactory"))) {
                    throw newIllegalArgumentException("illegal lookupClass: " + lookupClass);
                }
            }

@artembilan
Copy link
Member Author

Well, we definitely have to fix at least proxiing extra non-abstract methods. Otherwise Integration Graph becomes a bit awkward:

{
    "nodeId" : 28,
    "stats" : null,
    "componentType" : "gateway",
    "integrationPatternType" : "gateway",
    "integrationPatternCategory" : "messaging_endpoint",
    "properties" : { },
    "output" : "controlBusFlow.channel#0",
    "errors" : null,
    "name" : "controlBusFlow.gateway.andThen(java.util.function.Function)"
  }, {
    "nodeId" : 29,
    "stats" : null,
    "componentType" : "gateway",
    "integrationPatternType" : "gateway",
    "integrationPatternCategory" : "messaging_endpoint",
    "properties" : { },
    "output" : "controlBusFlow.channel#0",
    "errors" : null,
    "name" : "controlBusFlow.gateway.apply(java.lang.Object)"
  }, {
    "nodeId" : 30,
    "stats" : null,
    "componentType" : "gateway",
    "integrationPatternType" : "gateway",
    "integrationPatternCategory" : "messaging_endpoint",
    "properties" : { },
    "output" : "controlBusFlow.channel#0",
    "errors" : null,
    "name" : "controlBusFlow.gateway.compose(java.util.function.Function)"
  }, {
    "nodeId" : 31,
    "stats" : null,
    "componentType" : "gateway",
    "integrationPatternType" : "gateway",
    "integrationPatternCategory" : "messaging_endpoint",
    "properties" : { },
    "output" : "controlBusFlow.channel#0",
    "errors" : null,
    "name" : "controlBusFlow.gateway.lambda$compose$0(java.util.function.Function,java.lang.Object)"
  }, {
    "nodeId" : 32,
    "stats" : null,
    "componentType" : "gateway",
    "integrationPatternType" : "gateway",
    "integrationPatternCategory" : "messaging_endpoint",
    "properties" : { },
    "output" : "controlBusFlow.channel#0",
    "errors" : null,
    "name" : "controlBusFlow.gateway.lambda$andThen$1(java.util.function.Function,java.lang.Object)"
  }, {
    "nodeId" : 33,
    "stats" : null,
    "componentType" : "gateway",
    "integrationPatternType" : "gateway",
    "integrationPatternCategory" : "messaging_endpoint",
    "properties" : { },
    "output" : "controlBusFlow.channel#0",
    "errors" : null,
    "name" : "controlBusFlow.gateway.identity()"
  }, {
    "nodeId" : 34,
    "stats" : null,
    "componentType" : "gateway",
    "integrationPatternType" : "gateway",
    "integrationPatternCategory" : "messaging_endpoint",
    "properties" : { },
    "output" : "controlBusFlow.channel#0",
    "errors" : null,
    "name" : "controlBusFlow.gateway.lambda$identity$2(java.lang.Object)"
  }

artembilan added a commit to artembilan/spring-integration that referenced this issue Dec 11, 2019
artembilan added a commit to artembilan/spring-integration that referenced this issue Dec 11, 2019
Fixes spring-projects#3118

The `GatewayProxyFactoryBean` proxies all the methods in the provided interface,
including `default` and `static`.
This is not what is expected from end-users.

* Proxy only `abstract` methods from the provided interface
* Introduce a `DefaultMethodInvokingMethodInterceptor` to call `default`
method on the interface using a `MethodHandle` approach for those methods
calling
artembilan added a commit to artembilan/spring-integration that referenced this issue Dec 11, 2019
Fixes spring-projects#3118

The `GatewayProxyFactoryBean` proxies all the methods in the provided interface,
including `default` and `static`.
This is not what is expected from end-users.

* Proxy only `abstract` methods from the provided interface
* Introduce a `DefaultMethodInvokingMethodInterceptor` to call `default`
method on the interface using a `MethodHandle` approach for those methods
calling
@artembilan artembilan modified the milestones: 5.2.3, 5.3.x, 5.3.0.M1 Dec 12, 2019
artembilan added a commit to artembilan/spring-integration that referenced this issue Dec 13, 2019
Fixes spring-projects#3118

The `GatewayProxyFactoryBean` proxies all the methods in the provided interface,
including `default` and `static`.
This is not what is expected from end-users.

* Proxy only `abstract` methods from the provided interface
* Introduce a `DefaultMethodInvokingMethodInterceptor` to call `default`
method on the interface using a `MethodHandle` approach for those methods
calling
garyrussell pushed a commit that referenced this issue Dec 17, 2019
* GH-3118: MessagingGW: Don't proxy default methods

Fixes #3118

The `GatewayProxyFactoryBean` proxies all the methods in the provided interface,
including `default` and `static`.
This is not what is expected from end-users.

* Proxy only `abstract` methods from the provided interface
* Introduce a `DefaultMethodInvokingMethodInterceptor` to call `default`
method on the interface using a `MethodHandle` approach for those methods
calling

* * Introduce a `proxyDefaultMethods` option to let to
restore a previous behavior
* Handle a new property from XML, annotations & DSL configurations
* Ensure that the property works in various tests
* Document the feature

* * Fix typo in the `build.gradle`
@artembilan
Copy link
Member Author

Hm. GitHub somehow didn't close the issue automatically with the commit.
So, fixed via: 4134fc7

artembilan added a commit that referenced this issue Dec 26, 2019
* GH-3118: MessagingGW: Don't proxy default methods

Fixes #3118

The `GatewayProxyFactoryBean` proxies all the methods in the provided interface,
including `default` and `static`.
This is not what is expected from end-users.

* Proxy only `abstract` methods from the provided interface
* Introduce a `DefaultMethodInvokingMethodInterceptor` to call `default`
method on the interface using a `MethodHandle` approach for those methods
calling

* * Introduce a `proxyDefaultMethods` option to let to
restore a previous behavior
* Handle a new property from XML, annotations & DSL configurations
* Ensure that the property works in various tests
* Document the feature

* * Fix typo in the `build.gradle`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant