Avoid duplicate JCacheOperationSource bean registration in <cache:annotation-driven /> by nivolg · Pull Request #27499 · spring-projects/spring-framework

nivolg · 2021-09-30T19:42:09Z

Hello.

In our application we use XML context and <cache:annotation-driven mode="aspectj"/> declaration in combination with JCache API. Also we disable bean definition overrides by setting GenericApplicationContext.setAllowBeanDefinitionOverriding(false) in an ApplicationContextInitializer.

Such combination leads to BeanDefinitionOverrideException because the bean "org.springframework.cache.jcache.interceptor.DefaultJCacheOperationSource" is registered twice:

String sourceName = parserContext.getReaderContext().registerWithGeneratedName(sourceDef);
parserContext.registerBeanComponent(new BeanComponentDefinition(sourceDef, sourceName));

In our application we use xml context and <cache:annotation-driven /> declaration. Also we disable bean definition duplication by setting GenericApplicationContext.setAllowBeanDefinitionOverriding(false) in ApplicationContextInitializer. Such combination leads to BeanDefinitionOverrideException because bean "org.springframework.cache.jcache.interceptor.DefaultJCacheOperationSource" is registered twice: String sourceName = parserContext.getReaderContext().registerWithGeneratedName(sourceDef); parserContext.registerBeanComponent(new BeanComponentDefinition(sourceDef, sourceName));

pivotal-cla · 2021-09-30T19:42:11Z

@nivolg Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

nivolg · 2021-09-30T19:44:20Z

@pivotal-cla This is an Obvious Fix

pivotal-cla · 2021-09-30T19:46:02Z

@nivolg This Pull Request contains an obvious fix. Signing the Contributor License Agreement is not necessary.

snicoll · 2021-10-01T05:12:21Z

Thanks for the report and PR @nivolg. I wasn't able to reproduce the problem with a vanilla project started from start.spring.io. Please look at it and let us know what is missing.

If it turns out that a change is required in Spring Framework, I unfortunately disagree that this is an obvious fix so you'll have to sign the CLA.

pivotal-cla · 2021-10-01T06:23:40Z

@nivolg Thank you for signing the Contributor License Agreement!

nivolg · 2021-10-01T06:26:03Z

Thanks for quick response. I can't access your vanilla project repopsitory. But I made example project to reproduce the issue. I missed an important details in description: aspectj and jcache are used together.

Also I signed CLA.

snicoll · 2021-10-01T06:29:54Z

@nivolg bummer, it got created as private by default, sorry about that. Thank you for the sample, I missed the aspectJ bits.

sbrannen · 2021-10-01T17:16:54Z

Thanks for the repro project.

I have confirmed that the following exception occurs.

Exception in thread "main" org.springframework.beans.factory.support.BeanDefinitionOverrideException: Invalid bean definition with name 'org.springframework.cache.jcache.interceptor.DefaultJCacheOperationSource#0' defined in null: Cannot register bean definition [Root bean: class [org.springframework.cache.jcache.interceptor.DefaultJCacheOperationSource]; scope=; abstract=false; lazyInit=null; autowireMode=0; dependencyCheck=0; autowireCandidate=true; primary=false; factoryBeanName=null; factoryMethodName=null; initMethodName=null; destroyMethodName=null] for bean 'org.springframework.cache.jcache.interceptor.DefaultJCacheOperationSource#0': There is already [Root bean: class [org.springframework.cache.jcache.interceptor.DefaultJCacheOperationSource]; scope=; abstract=false; lazyInit=null; autowireMode=0; dependencyCheck=0; autowireCandidate=true; primary=false; factoryBeanName=null; factoryMethodName=null; initMethodName=null; destroyMethodName=null] bound.
	at org.springframework.beans.factory.support.DefaultListableBeanFactory.registerBeanDefinition(DefaultListableBeanFactory.java:995)
	at org.springframework.beans.factory.support.BeanDefinitionReaderUtils.registerBeanDefinition(BeanDefinitionReaderUtils.java:164)
	at org.springframework.beans.factory.xml.ParserContext.registerBeanComponent(ParserContext.java:125)
	at org.springframework.cache.config.AnnotationDrivenCacheBeanDefinitionParser$JCacheCachingConfigurer.registerCacheAspect(AnnotationDrivenCacheBeanDefinitionParser.java:256)
	at org.springframework.cache.config.AnnotationDrivenCacheBeanDefinitionParser$JCacheCachingConfigurer.access$100(AnnotationDrivenCacheBeanDefinitionParser.java:206)
	at org.springframework.cache.config.AnnotationDrivenCacheBeanDefinitionParser.registerCacheAspect(AnnotationDrivenCacheBeanDefinitionParser.java:100)
	at org.springframework.cache.config.AnnotationDrivenCacheBeanDefinitionParser.parse(AnnotationDrivenCacheBeanDefinitionParser.java:87)
	at org.springframework.beans.factory.xml.NamespaceHandlerSupport.parse(NamespaceHandlerSupport.java:74)
	at org.springframework.beans.factory.xml.BeanDefinitionParserDelegate.parseCustomElement(BeanDefinitionParserDelegate.java:1391)
	at org.springframework.beans.factory.xml.BeanDefinitionParserDelegate.parseCustomElement(BeanDefinitionParserDelegate.java:1371)
	at org.springframework.beans.factory.xml.DefaultBeanDefinitionDocumentReader.parseBeanDefinitions(DefaultBeanDefinitionDocumentReader.java:179)
	at org.springframework.beans.factory.xml.DefaultBeanDefinitionDocumentReader.doRegisterBeanDefinitions(DefaultBeanDefinitionDocumentReader.java:149)
	at org.springframework.beans.factory.xml.DefaultBeanDefinitionDocumentReader.registerBeanDefinitions(DefaultBeanDefinitionDocumentReader.java:96)
	at org.springframework.beans.factory.xml.XmlBeanDefinitionReader.registerBeanDefinitions(XmlBeanDefinitionReader.java:511)
	at org.springframework.beans.factory.xml.XmlBeanDefinitionReader.doLoadBeanDefinitions(XmlBeanDefinitionReader.java:391)
	at org.springframework.beans.factory.xml.XmlBeanDefinitionReader.loadBeanDefinitions(XmlBeanDefinitionReader.java:338)
	at org.springframework.beans.factory.xml.XmlBeanDefinitionReader.loadBeanDefinitions(XmlBeanDefinitionReader.java:310)
	at org.springframework.beans.factory.support.AbstractBeanDefinitionReader.loadBeanDefinitions(AbstractBeanDefinitionReader.java:188)
	at org.springframework.beans.factory.support.AbstractBeanDefinitionReader.loadBeanDefinitions(AbstractBeanDefinitionReader.java:224)
	at org.springframework.beans.factory.support.AbstractBeanDefinitionReader.loadBeanDefinitions(AbstractBeanDefinitionReader.java:195)
	at org.springframework.beans.factory.support.AbstractBeanDefinitionReader.loadBeanDefinitions(AbstractBeanDefinitionReader.java:257)
	at org.springframework.context.support.AbstractXmlApplicationContext.loadBeanDefinitions(AbstractXmlApplicationContext.java:128)
	at org.springframework.context.support.AbstractXmlApplicationContext.loadBeanDefinitions(AbstractXmlApplicationContext.java:94)
	at org.springframework.context.support.AbstractRefreshableApplicationContext.refreshBeanFactory(AbstractRefreshableApplicationContext.java:130)
	at org.springframework.context.support.AbstractApplicationContext.obtainFreshBeanFactory(AbstractApplicationContext.java:671)
	at org.springframework.context.support.AbstractApplicationContext.refresh(AbstractApplicationContext.java:553)

But I have not yet confirmed the proposed fix.

We'll look into this.

sbrannen · 2021-10-12T10:00:56Z

 				Object eleSource = parserContext.extractSource(element);
+
+				BeanDefinition sourceDef = createJCacheOperationSourceBeanDefinition(element, eleSource);
+				String sourceName = parserContext.getReaderContext().registerWithGeneratedName(sourceDef);


Suggested change

String sourceName = parserContext.getReaderContext().registerWithGeneratedName(sourceDef);

String sourceName = parserContext.getReaderContext().generateBeanName(sourceDef);

This would be a simpler fix, since it avoids the undesired side effect of registering the bean when generating the name (and the rest of the code can remain unchanged), but I think your CompositeComponentDefinition proposal is more in line with how components are handled for XML namespaces. So we'll go with that.

Thanks for review. I understand what you mean.
I thought about such simpler fix but read the methods above and decided that CompositeComponentDefinition is required for some reasons I don't know.

The CompositeComponentDefinition is not technically required.

It's really just there for tooling support. Without it, the application would work the same.

sbrannen · 2021-10-12T10:37:08Z

This has been merged in 50ccb1b and revised in 5bd9053.

Applied to 5.3.x and main (6.0); to be backported to 5.2.x as well (#27547).

Thanks

…otation-driven /> In our application we use XML context and <cache:annotation-driven /> declaration. Also we disable bean definition duplication by setting GenericApplicationContext.setAllowBeanDefinitionOverriding(false) in an ApplicationContextInitializer. This combination leads to a BeanDefinitionOverrideException because the DefaultJCacheOperationSource bean is registered twice. - once for: parserContext.getReaderContext().registerWithGeneratedName(sourceDef); - once for: parserContext.registerBeanComponent(new BeanComponentDefinition(sourceDef, sourceName)); This commit refactors JCacheCachingConfigurer.registerCacheAspect(...) so that the JCacheOperationSource bean is registered only once. Closes spring-projectsgh-27499

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Sep 30, 2021

snicoll added the status: waiting-for-feedback We need additional information before we can continue label Oct 1, 2021

spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Oct 1, 2021

sbrannen added in: core Issues in core modules (aop, beans, core, context, expression) type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged or decided on status: feedback-provided Feedback has been provided labels Oct 1, 2021

sbrannen added this to the 5.3.11 milestone Oct 1, 2021

sbrannen self-assigned this Oct 11, 2021

sbrannen reviewed Oct 12, 2021

View reviewed changes

sbrannen changed the title ~~Avoid bean name duplication in <cache:annotation-driven /> definition parser~~ Avoid duplicate JCacheOperationSource bean registration in <cache:annotation-driven /> Oct 12, 2021

sbrannen added a commit that referenced this pull request Oct 12, 2021

Introduce test for gh-27499 and polish contribution

5bd9053

sbrannen added the for: backport-to-5.2.x label Oct 12, 2021

spring-projects-issues added status: backported An issue that has been backported to maintenance branches and removed for: backport-to-5.2.x labels Oct 12, 2021

spring-projects-issues mentioned this pull request Oct 12, 2021

Avoid duplicate JCacheOperationSource bean registration in <cache:annotation-driven /> #27547

Closed

sbrannen closed this in 50ccb1b Oct 12, 2021

sbrannen added a commit to sbrannen/spring-framework that referenced this pull request Oct 12, 2021

Introduce test for spring-projectsgh-27499 and polish contribution

6d4dfed

This was referenced Oct 14, 2021

Bump spring.version from 5.3.9 to 5.3.11 liquibase/liquibase-hibernate#363

Closed

Bump spring.version from 5.3.9 to 5.3.12 liquibase/liquibase-hibernate#365

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid duplicate JCacheOperationSource bean registration in <cache:annotation-driven />#27499

Avoid duplicate JCacheOperationSource bean registration in <cache:annotation-driven />#27499
nivolg wants to merge 1 commit into
spring-projects:mainfrom
nivolg:patch-1

nivolg commented Sep 30, 2021 •

edited by sbrannen

Loading

Uh oh!

pivotal-cla commented Sep 30, 2021

Uh oh!

nivolg commented Sep 30, 2021

Uh oh!

pivotal-cla commented Sep 30, 2021

Uh oh!

snicoll commented Oct 1, 2021

Uh oh!

pivotal-cla commented Oct 1, 2021

Uh oh!

nivolg commented Oct 1, 2021 •

edited

Loading

Uh oh!

snicoll commented Oct 1, 2021 •

edited

Loading

Uh oh!

sbrannen commented Oct 1, 2021

Uh oh!

sbrannen Oct 12, 2021

Uh oh!

nivolg Oct 12, 2021

Uh oh!

sbrannen Oct 12, 2021

Uh oh!

sbrannen commented Oct 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	String sourceName = parserContext.getReaderContext().registerWithGeneratedName(sourceDef);
	String sourceName = parserContext.getReaderContext().generateBeanName(sourceDef);

Conversation

nivolg commented Sep 30, 2021 • edited by sbrannen Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pivotal-cla commented Sep 30, 2021

Uh oh!

nivolg commented Sep 30, 2021

Uh oh!

pivotal-cla commented Sep 30, 2021

Uh oh!

snicoll commented Oct 1, 2021

Uh oh!

pivotal-cla commented Oct 1, 2021

Uh oh!

nivolg commented Oct 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snicoll commented Oct 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbrannen commented Oct 1, 2021

Uh oh!

sbrannen Oct 12, 2021

Choose a reason for hiding this comment

Uh oh!

nivolg Oct 12, 2021

Choose a reason for hiding this comment

Uh oh!

sbrannen Oct 12, 2021

Choose a reason for hiding this comment

Uh oh!

sbrannen commented Oct 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nivolg commented Sep 30, 2021 •

edited by sbrannen

Loading

nivolg commented Oct 1, 2021 •

edited

Loading

snicoll commented Oct 1, 2021 •

edited

Loading