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

@EnableTransactionManagement proxyTargetClass not control by spring.aop.proxyTargetClass #8434

Closed
Dreampie opened this issue Feb 28, 2017 · 6 comments
Assignees
Labels
type: bug A general bug
Milestone

Comments

@Dreampie
Copy link

Dreampie commented Feb 28, 2017

if use @Transactional,and set spring.aop.proxyTargetClass=false, this class also create by cglib not jdk proxy,you must config @EnableTransactionManagement(proxyTargetClass=false) to use jdk proxy with @Transactional

@Service
public class DemoServiceImpl implements DemoService {

    @Transactional
    public void test() {
        System.out.println("Test");
    }
}

And @EnableTransactionManagement proxyTargetClass default value is false, why spring boot change to true?

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 28, 2017
@philwebb
Copy link
Member

This was changed in 1.4 (see #5423). We've generally found cglib proxies less likely to cause unexpected cast exceptions.

@philwebb philwebb added status: invalid An issue that we don't feel is valid and removed status: waiting-for-triage An issue we've not yet triaged labels Feb 28, 2017
@philwebb philwebb reopened this Feb 28, 2017
@philwebb
Copy link
Member

Sorry, I misunderstood your initial comment. You're saying you add spring.aop.proxyTargetClass=false to application.properties but we ignore that and use cglib regardless?

@philwebb philwebb added status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged and removed status: invalid An issue that we don't feel is valid labels Feb 28, 2017
@Dreampie
Copy link
Author

yes,@snicoll know this question,https://github.com/Dreampie/spring-boot-demo

@philwebb philwebb removed the status: waiting-for-feedback We need additional information before we can continue label Feb 28, 2017
@mdeinum
Copy link
Contributor

mdeinum commented Mar 5, 2017

the spring.aop.proxy-target-class is only used to toggle the configuration in AopAutoConfiguration for other proxy creation it is being ignored, like for @EnableTransactionManagement and the configuration of the PersistenceExceptionTranslationPostProcessor.

I noticed this during introducing Spring Boot into a project which used quite a lot of final classes (or classes with final methods). I ran into it in those areas but maybe there are a few more (like JMX for instance).

@philwebb philwebb added priority: normal type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Mar 6, 2017
@philwebb philwebb added this to the 1.4.6 milestone Mar 6, 2017
@snicoll
Copy link
Member

snicoll commented Mar 13, 2017

While we're working on it, we may want to share that thing in its own auto-configuration and order that in a sensible manner, see #8587

snicoll added a commit to snicoll/spring-boot that referenced this issue Mar 17, 2017
Previously to this commit, transaction management was only enabled when
a `DataSource` is configured. The processing of `@Transactional`
annotations are now enabled as long as a `PlatformTransactionManager` is
present.

Also, the `spring.aop.proxy-target-class` is now honoured if set, still
defaulting to CGLIB mode.

Closes spring-projectsgh-8434
@snicoll
Copy link
Member

snicoll commented Mar 17, 2017

@philwebb I've worked on a solution in b20dff1 that is a bit larger than what is foreseen here. I am not sure if that belongs to 1.4 but a couple of thoughts:

  1. Our default for this is CGLIB which basically upgrade the whole context to go CGLIB. IMO we should change the default of spring.aop.proxy-target-class to true
  2. So far we only enabled transaction management when a DataSource is present. That led to the Neo4j problem (Auto-configure @EnableTransactionManagement with Neo4J #8587) but any data tech that supports PlatformTransactionManager is also affected. IMO it makes more sense to enable it once such a bean is found in the context

I think that 1. is pretty bad because we won't be able to change it in 1.4 or 1.5 and currently we're doing that effectively as soon as you use @Transactional in your app.

@philwebb philwebb self-assigned this Mar 20, 2017
@philwebb philwebb added the for: team-attention An issue we'd like other members of the team to review label Mar 20, 2017
@philwebb philwebb modified the milestones: 1.5.3, 1.4.6 Mar 31, 2017
@philwebb philwebb removed the for: team-attention An issue we'd like other members of the team to review label Mar 31, 2017
@snicoll snicoll self-assigned this Mar 31, 2017
snicoll added a commit that referenced this issue Apr 3, 2017
This commit makes sure that `@EnableTransactionManagement` is
auto-configured with Neo4j. It actually reuses what was done in #8434,
making sure that the `Neo4jDataAutoConfiguration` is ordered properly.

Closes gh-8587
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

5 participants