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

Allow runtime detection of CGLib proxying VS. JDK proxying for different parts of the framework [SPR-16532] #21075

Open
spring-projects-issues opened this issue Feb 23, 2018 · 4 comments
Labels
in: core status: waiting-for-triage

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Feb 23, 2018

Oliver Drotbohm opened SPR-16532 and commented

The change in Spring Boot 2.0's default proxying behavior (defaulting to CGLib proxies), prevents it to actually consider user configuration explicitly using e.g. @EnableTransactionManagement(proxyTargetClass = false) as they all end up in the registration of the org.springframework.aop.config.internalAutoProxyCreator bean. This would cause the singular use of e.g. @ETM(pTC=true) to disable the Boot default for all cases that – according to Boot's default – use proxying (general AOP, caching etc.).

It would be cool if there was a way for downstream projects to find out about for which part of the framework a particular proxy setting was activated explicitly, so that Boot can apply its default to all others. I guess another option would be to allow the default to be used in the framework to be configured externally, so that Boot could just forward the flag and would not have to consider the exclusions itself.

The effect of the current state of affairs is that you can have a Boot 2.0 application with e.g. transactions explicitly configured to use JDK proxies but Boot's default still trumping this. This is particularly worrying as it's AFAIK the only situation in which user configuration does not trump Boots defaults. Additionally, the Spring Framework log output in erroneous scenarios (e.g. the class to be proxied using final methods) hints at enabling JDK proxies, which is exactly what the user configuration does. This leaves users with hard to diagnose configuration issues.


Affects: 4.3.14, 5.0.4

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Feb 23, 2018

Andy Wilkinson commented

Additionally, the Spring Framework log output in erroneous scenarios (e.g. the class to be proxied using final methods) hints at enabling JDK proxies, which is exactly what the user configuration does.

Can you point me to an example of this hint please? I've been unable to trigger a failure that includes such a hint.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Feb 23, 2018

Oliver Drotbohm commented

CglibAopProxy.doValidateClass(…) issues:

Unable to proxy interface-implementing method […] because it is marked as final: Consider using interface-based JDK proxies instead!

If I now inspect my configuration and find @EnableTransactionManagement(proxyTargetClass = false) being configured explicitly, I think: "That's exactly what I have running." I now then have to find out that in contrast to every other area of Boot defaulting, it doesn't consider my explicit Spring Framework configuration and overrides it with it's own default.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Feb 23, 2018

Andy Wilkinson commented

Thanks. That's a pretty long way from the description on the Boot issue where you said the message "suggests to set proxyTargetClass to false".

I think the Framework's suggestion is a good one, and, given the situation, it's also good that it's not specific about how to achieve that goal. As I've already said, finding @EnableTransactionManagement(proxyTargetClass = false) being configured explicitly isn't enough. You need to find everywhere that @EnableAspectJAutoProxy, @EnableAsync, @EnableCaching, and @EnableTransactionManagement is used and ensure that none of them set proxyTargetClass to true. Having to check for all four is cumbersome, particularly if one of them is in code outside of your main application code (in Boot or in some shared code, for example). Having a switch of some form that takes precedence over the aforementioned annotations would, I think, make things easier.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Feb 23, 2018

Oliver Drotbohm commented

That's a pretty long way from the description on the Boot issue where you said the message "suggests to set proxyTargetClass to false".

It is knowing in which way you control the proxy setup in Spring Framework and applying that knowledge in a Boot app. I think that Boot shouldn't start exposing functionality that undermines that. Using Boot shouldn't invalidate knowledge users have about the framework. It shouldn't change or break the way that configuration means which the core framework provides work. If there's something in the framework that makes it hard for Boot to actually do so, I think this should be improved.

Would it make sense for the framework to detect contradicting, explicit configurations of the proxy setup? Because if there's explicit configuration for both ways, that seems to indicate that there were reasons that these settings were chosen and we'd have to resort to the user to resolve that conflict, don't we? Reading the JavaDoc of @EnableTransactionManagement.proxyTargetClass, it indicates that explicit configuration of this flag is global (although it only speaks about the true case). So one could argue, that, if any of the annotations exposing proxy configuration settings is used explicitly, they all have to be used in the same way and then disable a global default.

@spring-projects-issues spring-projects-issues added status: waiting-for-triage type: enhancement in: core and removed type: enhancement labels Jan 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core status: waiting-for-triage
Projects
None yet
Development

No branches or pull requests

1 participant