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

Default CGLib proxy setting default cannot be overridden by using core framework annotations (@EnableTransactionManagement, @EnableAspectJAutoProxy) #12194

Closed
odrotbohm opened this issue Feb 23, 2018 · 8 comments
Assignees
Labels
status: declined A suggestion or change that we don't feel we should currently apply

Comments

@odrotbohm
Copy link
Member

odrotbohm commented Feb 23, 2018

Spring Boot 2.0 changes the default value for the configuration property spring.aop.proxy-target-class to true if nothing is configured. This causes an explicit @EnableAspectJAutoProxy(proxy-target-class = false) not being picked up anymore. Also an explicit @EnableTransactionManagement(proxy-target-class = false) is not causing transactional proxies being created as JDK proxies anymore as the default true gets applied.

I.e. you see the same symptoms as described in #8434.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 23, 2018
@odrotbohm odrotbohm changed the title CglibAutoProxyConfiguration does not back off when @EnableAspectJAutoProxy is used explicitly Default CGLib proxy setting default cannot be overridden by using core framework annotations (@EnableTransactionManagement, @EnableAspectJAutoProxy) Feb 23, 2018
odrotbohm added a commit to st-tu-dresden/salespoint that referenced this issue Feb 23, 2018
Upgraded to Spring Boot 2.0 RC2. Adapted API changes in Spring Data, removed custom Streamable implementation and SalespointRepository. Re-enabled integration test that checks that unencrypted passwords cannot be persisted.

Added EnforceJdkProxiesProcessor to make sure we continue to use JDK proxies so that we can use final methods in component implementations. See [0] for details.

[0] spring-projects/spring-boot#12194
@wilkinsona wilkinsona added type: bug A general bug priority: normal and removed status: waiting-for-triage An issue we've not yet triaged labels Feb 23, 2018
@wilkinsona wilkinsona added this to the 2.0.0 milestone Feb 23, 2018
@wilkinsona wilkinsona self-assigned this Feb 23, 2018
@wilkinsona
Copy link
Member

wilkinsona commented Feb 23, 2018

Also an explicit @EnableTransactionManagement(proxy-target-class = false) is not causing transactional proxies being created as JDK proxies anymore as the default true gets applied.

We have a test that disagrees with this statement. I think the problem is purely in CglibAutoProxyConfiguration. Use of CGLib proxies is an application-context-wide switch so if CgLibAutoProxyConfiguration forces CGLib proxies then whatever EnableTransactionManagementConfiguration does will not make any difference.

I'll update AopAutoConfiguration to back off if the auto-proxy creator bean already exists. That will mean that specifying both @EnableTransactionManagement and @EnableAspectJAutoProxy will switch off CGLib proxies. Alternatively, you can configure both with a single property: spring.aop.proxy-target-class.

@odrotbohm
Copy link
Member Author

odrotbohm commented Feb 23, 2018

We have a test that disagrees with this statement.

Find this demo project showing that @EnableTransactionManagement(proxyTargetClass = false) still creates CGLib proxies for classes annotated with @Transactional: demo-3.zip.

My extended inspection of also @ EnableAspectJAutoProxy was caused by my debug attempts not revealing any transaction specific components being involved but only a generic AnnotationAwareAspectJAutoProxyCreator that made me think the changed defaults might now require also @EnableAspectJAutoProxy(proxyTargetClass = true).

From a user point of view, I'd like to see explicit configuration using core Spring Framework means take precedence over Spring Boot defaults.

@wilkinsona
Copy link
Member

I'll update AopAutoConfiguration to back off if the auto-proxy creator bean already exists.

Unfortunately, it's not as simple as that. The problem is that we're fighting against three different ways in the Framework that the proxy type can be configured:

  1. @EnableAspectJAutoProxy
  2. @EnableCaching
  3. @EnableTransactionManagement

Use of any of the three will define the org.springframework.aop.config.internalAutoProxyCreator bean. If we change AopAutoConfiguration to back off in the presence of that bean, using any of the three annotations above will stop spring.aop.proxy-target-class from having an effect.

Generally speaking, 1 and 3 aren't needed in a Boot app, so they are perhaps not too bad. However, 2 is required if you want to enable caching. We have no way of distinguishing between a user that just wants to enable caching and a user that wants to enable caching and override the type of proxies that are created.

Given the above, I don't think we can do anything that would allow us to override the default using Framework's annotations. Instead, we really need to use a single context-wide switch to control the type of proxies that are created. We already have that switch in the form of the spring.aop.proxy-target-class.

While looking at this issue I found some discrepancies in our javadoc and documentation. I've opened #12196 to take care of those.

@wilkinsona wilkinsona removed this from the 2.0.0 milestone Feb 23, 2018
@wilkinsona wilkinsona added status: declined A suggestion or change that we don't feel we should currently apply and removed priority: normal type: bug A general bug labels Feb 23, 2018
@odrotbohm
Copy link
Member Author

That's sorry to hear as as far as I can see, this is the first occasion that I encounter, that Boot doesn't respect standard Spring user configuration, which breaks the "Boot fills configuration gaps and respects what you've configured explicitly." story.

Regarding the "aren't needed in a Boot app" statement. I don't think I can agree with this. If you want to ship code that by definition can't use CGLib proxies (e.g. because the code is deliberately using interfaces to allow final methods in the interface implementation) and you want to make sure that this code works as expected, shipping a configuration class that enforces JDK proxies is a reasonable option. Relying on someone downstream configuring the property in a special way is cumbersome as it creates much bigger distance between the code that needs the setting and where the setting is configured.

I haven't been too happy with the switch to CGLib proxies by default in the first place as CGLib still imposes quite severe limitations on the code being written. That switch in default that was basically a convenience switch and was considered to not have any implications now basically leads to one of Boot's core principles to be undermined. Even worse, we break the usually pretty helpful Spring Framework error messages as the log output you get in the error case suggests to set proxyTargetClass to false which is exactly what you have in place and thus probably leaves you puzzled as nothing leaves a trace that some Boot default affects this and we've spent years training users to expect their custom configuration trump Boot's defaults.

@wilkinsona
Copy link
Member

wilkinsona commented Feb 23, 2018

Regarding the "aren't needed in a Boot app" statement. I don't think I can agree with this.

I meant that they aren't needed as both will be auto-configured for you. By contrast, if you want to enable caching you have to use @EnableCaching as it won't be auto-configured.

shipping a configuration class that enforces JDK proxies is a reasonable option

As far as I can tell, it's actually impossible to enforce JDK proxies. As soon as anyone uses an annotation with proxyTargetClass=true on it, any attempt to enforce JDK proxies will fail. That's true in a pure Spring Framework app as well as in a Spring Boot app.

I haven't been too happy with the switch to CGLib proxies by default in the first place as CGLib still imposes quite severe limitations on the code being written.

That's a shame, and it would have been useful to have this feedback in mid-2016 when it was asked for. It's too late to change this in 2.0.

Even worse, we break the usually pretty helpful Spring Framework error messages as the log output you get in the error case suggests to set proxyTargetClass to false

We might be able to use a FailureAnalyzer to produce a more helpful error message. I've opened #12197 to investigate.

We've spent years training users to expect their custom configuration trump Boot's defaults.

Unfortunately, a user's custom configuration can only trump Boot's defaults when it's clear what the custom configuration is intended to do. In terms of beans in the context relating to proxying, @EnableCaching, @EnableTransactionManagement, and @EnableAspectJAutoProxy all look the same. For example, if using @EnableCaching meant that spring.aop.proxy-target-class was suddenly ignored, I'm pretty sure someone would raise a bug.

One possibility could be if Framework provided some sort of marker that allowed us to identify that @EnableAspectJAutoProxy had been used explicitly, rather than proxying being configured as a side-effect of the use of @EnableCaching and @EnableTransactionManagement. We could then back off in the presence of @EnableAspectJAutoProxy, while allowing @EnableCaching (for example), to be used without inadvertently changing the whole app's proxying behaviour.

@odrotbohm
Copy link
Member Author

odrotbohm commented Feb 23, 2018

As far as I can tell, it's actually impossible to enforce JDK proxies. As soon as anyone uses an annotation with proxyTargetClass=true on it, any attempt to enforce JDK proxies will fail. That's true in a pure Spring Framework app as well as in a Spring Boot app.

I am not necessarily talking about enforcing but making thinks work the way they need in such a scenario out of the box. If someone can use the annotation to set this to true why to we accept that setting this to false does not work?

That's a shame, and it would have been useful to have this feedback in mid-2016 when it was asked for. It's too late to change this in 2.0.

I did so. Both in exactly the comment below the one previously linked to and also a lot of in person discussions regarding Spring RESTBucks, just when the default was flipped.

Unfortunately, a user's custom configuration can only trump Boot's defaults when it's clear what the custom configuration is intended to do. In terms of beans in the context relating to proxying, @EnableCaching, @EnableTransactionManagement, and @EnableAspectJAutoProxy all look the same. For example, if using @EnableCaching meant that spring.aop.proxy-target-class was suddenly ignored, I'm pretty sure someone would raise a bug.

I'm puzzled. They all have dedicated names. There is a strong contract of what they enable and what now. It might be the case that it's hard to diagnose at runtime which of the switches has been used. But that's something we should then probably bring up with Spring Framework.

And no, I don't think anyone would be surprised that user configuration trumps application properties. No one is surprised that e.g. JDBC settings in application.properties are not applied to a DataSource one just declares manually. If you write custom Spring configuration, you take control. If you don't that's a bug. That's at least the way I've learned to use Boot. And so far, all tickets that I opened that indicated the opposite have been considered bugs and fixed pretty quickly usually.

One possibility could be if Framework provided some sort of marker that allowed us to identify that @EnableAspectJAutoProxy had been used explicitly, rather than proxying being configured as a side-effect of the use of @EnableCaching and @EnableTransactionManagement. We could then back off in the presence of @EnableAspectJAutoProxy, while allowing @EnableCaching (for example), to be used without inadvertently changing the whole app's proxying behavior.

Here we go. I've filed SPR-16532 to improve that.

@ghost
Copy link

ghost commented Jan 1, 2019

I faced similar issue when I added Apache Shiro Web Starter in my project.

    at org.springframework.beans.factory.support.DefaultListableBeanFactory.doResolveDependency(DefaultListableBeanFactory.java:1247)
    at org.springframework.beans.factory.support.DefaultListableBeanFactory.resolveDependency(DefaultListableBeanFactory.java:1164)
    at org.springframework.beans.factory.annotation.AutowiredAnnotationBeanPostProcessor$AutowiredFieldElement.inject(AutowiredAnnotationBeanPostProcessor.java:593)
    at org.springframework.beans.factory.annotation.InjectionMetadata.inject(InjectionMetadata.java:90)
    at org.springframework.beans.factory.annotation.AutowiredAnnotationBeanPostProcessor.postProcessProperties(AutowiredAnnotationBeanPostProcessor.java:374)
    at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.populateBean(AbstractAutowireCapableBeanFactory.java:1378)
    at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.doCreateBean(AbstractAutowireCapableBeanFactory.java:575)
    at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBean(AbstractAutowireCapableBeanFactory.java:498)
    at org.springframework.beans.factory.support.AbstractBeanFactory.lambda$doGetBean$0(AbstractBeanFactory.java:320)
    at org.springframework.beans.factory.support.DefaultSingletonBeanRegistry.getSingleton(DefaultSingletonBeanRegistry.java:222)
    at org.springframework.beans.factory.support.AbstractBeanFactory.doGetBean(AbstractBeanFactory.java:318)
    at org.springframework.beans.factory.support.AbstractBeanFactory.getBean(AbstractBeanFactory.java:199)
    at org.springframework.beans.factory.config.DependencyDescriptor.resolveCandidate(DependencyDescriptor.java:273)
    at org.springframework.beans.factory.support.DefaultListableBeanFactory.doResolveDependency(DefaultListableBeanFactory.java:1237)
    at org.springframework.beans.factory.support.DefaultListableBeanFactory.resolveDependency(DefaultListableBeanFactory.java:1164)
    at org.springframework.beans.factory.annotation.AutowiredAnnotationBeanPostProcessor$AutowiredFieldElement.inject(AutowiredAnnotationBeanPostProcessor.java:593)
    at org.springframework.beans.factory.annotation.InjectionMetadata.inject(InjectionMetadata.java:90)
    at org.springframework.beans.factory.annotation.AutowiredAnnotationBeanPostProcessor.postProcessProperties(AutowiredAnnotationBeanPostProcessor.java:374)
    at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.populateBean(AbstractAutowireCapableBeanFactory.java:1378)
    at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.doCreateBean(AbstractAutowireCapableBeanFactory.java:575)
    at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBean(AbstractAutowireCapableBeanFactory.java:498)
    at org.springframework.beans.factory.support.AbstractBeanFactory.lambda$doGetBean$0(AbstractBeanFactory.java:320)
    at org.springframework.beans.factory.support.DefaultSingletonBeanRegistry.getSingleton(DefaultSingletonBeanRegistry.java:222)
    at org.springframework.beans.factory.support.AbstractBeanFactory.doGetBean(AbstractBeanFactory.java:318)
    at org.springframework.beans.factory.support.AbstractBeanFactory.getBean(AbstractBeanFactory.java:199)
    at org.springframework.beans.factory.support.DefaultListableBeanFactory.preInstantiateSingletons(DefaultListableBeanFactory.java:846)
    at org.springframework.context.support.AbstractApplicationContext.finishBeanFactoryInitialization(AbstractApplicationContext.java:863)
    at org.springframework.context.support.AbstractApplicationContext.refresh(AbstractApplicationContext.java:546)
    at org.springframework.boot.web.servlet.context.ServletWebServerApplicationContext.refresh(ServletWebServerApplicationContext.java:142)
    at org.springframework.boot.SpringApplication.refresh(SpringApplication.java:775)
    at org.springframework.boot.SpringApplication.refreshContext(SpringApplication.java:397)
    at org.springframework.boot.SpringApplication.run(SpringApplication.java:316)
    at org.springframework.boot.SpringApplication.run(SpringApplication.java:1260)
    at org.springframework.boot.SpringApplication.run(SpringApplication.java:1248)
    at com.nusrcraft.flexinics.FlexinicsWebApplication.main(FlexinicsWebApplication.java:37)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at org.springframework.boot.devtools.restart.RestartLauncher.run(RestartLauncher.java:49)

@Transactional was working fine earlier but it started failing as soon as I added Shiro starter

@snicoll
Copy link
Member

snicoll commented Jan 2, 2019

@amol204 please don't comment on a closed issue to get support. If you have a question please ask on StackOverflow or come chat with the community on Gitter. Also, Shiro is not handled here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

No branches or pull requests

4 participants