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

GH-3118: MessagingGW: Don't proxy default methods #3119

Merged
merged 3 commits into from Dec 17, 2019

Conversation

artembilan
Copy link
Member

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

@garyrussell
Copy link
Contributor

While this generally makes sense; it's a severely breaking change and, at least, it needs to be an option at the class or method level.

Consider

public interfact Foo {

    String bar(String baz);

    default String qux(String fiz) {
        throw new UnsupportedOperationException();
    }

}

A user might reasonably expect to be able to override qux with an SI flow.

Furthermore, someone might already be using it that way.

@artembilan
Copy link
Member Author

it's a severely breaking change

Oh! I didn't think it that way. "How an unusual behavior can be OK?!" 😄

So, proxyDeafultMethods on the <gateway> and @MessagingGateway be it!
Or if you would like to proxy only some specific one, you can use a @Gateway explicitly on that method.

Is this OK?

@garyrussell
Copy link
Contributor

Yes; that would be sufficient (and false by default is ok too).

Do we really need all that MethodHandle complexity; we can't use normal reflection?

@artembilan
Copy link
Member Author

we can't use normal reflection?

No, because there is no target for method call in case of proxy against just an interface.
I saw somewhere only a solution with creating a class as an implementation on the fly and then use it as a target for that default method call.

The MethodHandle complexity here is with the same reason Spring Data has: proxy an interface without any class implementation.
More over: pay attention to the docs change - we still are not good with JDK interfaces anyway.
Here is some article on the matter: https://rmannibucau.wordpress.com/2014/03/27/java-8-default-interface-methods-and-jdk-dynamic-proxies/
And SO on the matter: https://stackoverflow.com/questions/26206614/java8-dynamic-proxy-and-default-methods

@artembilan
Copy link
Member Author

it needs to be an option at the class or method level.

After more thinking I treat this issue as a 5.3 theme: no one complained so far about default methods, plus it really may feel as an expected behavior to have those methods proxied as well.
So, changing behavior (even if it is kinda bug fix) should go to the next version.

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
restore a previous behavior
* Handle a new property from XML, annotations & DSL configurations
* Ensure that the property works in various tests
* Document the feature
@artembilan artembilan changed the base branch from master to 5.3-WIP December 13, 2019 20:56
@artembilan
Copy link
Member Author

OK. Introduced a proxyDefaultMethods options and incorporated the change against 5.3-WIP branch.

Thanks

@garyrussell garyrussell merged commit 4134fc7 into spring-projects:5.3-WIP Dec 17, 2019
artembilan added a commit that referenced this pull request 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants