Duplicate bean id allowed in same XML file by using name attribute [SPR-2285]

[spring-projects-issues]

Colin Sampaleanu opened SPR-2285 and commented

The bean factory and application context contract clearly states (as per our docs) that within one definition xml file a bean id must be unique.

This is enforced by the XML parser when using the "id" attribute exclusively because this is flagged as an XML IDREF attribute. However, by combining the use of the id attribute and the name attribute, or just using the name attribute exclusively, it is possible to have 2 or more bean definitions in the same xml file with the same id. This is silently accepted with the last bean definition winning out.

The bean definition reader must basically track ids, however they are supplied, and enforce the same semantics from usage of the "name" attribute as from usage of the "id" attribute.

A related question is what should happen in the case of <import>? An argument could be made either way here.

---

Affects: 2.0 RC2

Attachments:

• spring2285_testcase_and_patch.zip (2.67 kB)

[spring-projects-issues]

Rob Harrop commented

The bean name issue is now solved. The <import> issue was reported some time ago and we decided to leave it as is.

Rob

[spring-projects-issues]

Andrew Perepelytsya commented

Guys,

I would like your feedback on this issue. The way it's been fixed in 2.0-rc2 doesn't seem to play well (appending #1 to the duplicate bean). I'm currently upgrading Mule support to Spring 2.x and ran into that issue. In short, the real solution would be to 'scope' the beans probably, if possible at all. Check this config:

http://svn.mule.codehaus.org/browse/mule/trunk/mule/mule-extras/spring/src/test/resources/test-mule-autowire-app-context.xml?r=2233#l156

This is a global endpoint with the name 'orangeEndpoint'. Later in this config:

http://svn.mule.codehaus.org/browse/mule/trunk/mule/mule-extras/spring/src/test/resources/test-mule-autowire-app-context.xml?r=2233#l252

Again, 'orangeEndpoint' which is local to the enclosing element, the mule-descriptor. There are API calls which specifically lookup either a global one (spreading multiple config files) or local call on a descriptor's router. The lookup is performed by endpoint's name, which is either id or name of the bean. This worked with Spring 1.2.x There are specific assertions in the test cases to validate global vs local endpoints (local has an extra transformer reference applied).

WDYT?

[spring-projects-issues]

Andrew Perepelytsya commented

I have a possible solution. org.springframework.beans.factory.support.BeanDefinitionValueResolver has the private adaptInnerBeanName() method appending this suffix. While this is absolutely correct for top-level beans, I don't think this is the right and desired behaviour for inner beans. In Mule, after I removed the calls to adaptInnerBeanName() from the class, all tests finished successfully. This means the regression was fixed at least in the scope of Mule, which for Spring-based configs defines core objects as prototypes, not singletons.

Now, why I think it may be safe to remove this call. While hacking the code and debugging it, right after config has been finished, I did some test lookups by bean names and bean types (getBean() & getBeansOfType()). I found that ONLY top-level beans were actually accessible via those calls, meaning IT IS safe not to add the #N suffix to INNER bean names. Type lookup returned zero results as well (inboundRouter is an inner bean of the component descriptor).

Is my assumption correct?

[spring-projects-issues]

Andrew Perepelytsya commented

Filed a related request: #7040

[spring-projects-issues]

Andrew Perepelytsya commented

Attached is an archive with a testcase proving that inner beans are never accessible via direct bean factory calls, and thus do not need this #N suffix appended.

The archive includes a patch to the BeanDefinitionValueResolver against the latest CVS version (1.13 at the moment). It removes the adaptInnerBeanName() method and reverts the behaviour for inner beans to the previous state.

[spring-projects-issues]

Juergen Hoeller commented

I agree that inner bean names do not have to be adapted there if they are not singletons, since they're not exposed to the registry in any way then. However, for inner singletons, we do need a unique identifier for shutdown order purposes - the only option would be to expose the plain bean name to BeanNameAware, while using the adapted bean name for the registry. I'd like to avoid that as far as possible, though. And well, inner beans that rely on BeanNameAware are rather rare anyway.

I have modified BeanDefinitionValueResolver accordingly, so that it only adapts an inner bean name in case of a singleton now.

Juergen

[spring-projects-issues]

Andrew Perepelytsya commented

Hi guys, nice to meet you again :) Hate to say that, but Spring 2.0.1 has got a regression, and the testcase I'd attached originally is failing now. I agree that the issue summary is a bit misleading, so we can re-classify it and file a new one for tracking. Just a heads up, Juergen has fixed the bug before, check our archives: http://www.nabble.com/Fwd%3A-Spring-Core-Regression-t2081928.html#a5736008

The Spring 2.0.1 release notes (http://static.springframework.org/spring/docs/2.0.x/changelog.txt) had this change, when it merges the outer and inner bean definitions and try to be smart with singletons. Apparently, something is not clicking here :(

You can ping us back at http://mule.mulesource.org/jira/browse/MULE-1258 in case a new issue is filed, I'm watching the current one.

Cheers!

[spring-projects-issues]

Juergen Hoeller commented

Thanks for pointing this out, Andrew! This unfortunate regression was caused by an optimization in the merged bean definition caching: We use the bean name as key there in 2.0.1, instead of the BeanDefinition object in 2.0... Which means that the bean name must be unique in 2.0.1, else the same merged definiton will be used for all beans of the same name.

I've addressed this in 2.0.2 through using the BeanDefinition object as key again, but this time keeping the merged definition in an identity map (which should be as fast as using the bean name in a standard hash map). After all, the original intent of the optimization was to avoid expensive "equals" calls on the bean definition... which should still be fine.

Juergen

[spring-projects-issues]

Juergen Hoeller commented

This regression fix should make it into the next nightly 2.0.2 snapshot (http://www.springframework.org/snapshots). Please give it a try and let me know whether it works for you!

Juergen

[spring-projects-issues]

Andrew Perepelytsya commented

Juergen, that did the trick, kudos for the super-fast turnover! :D

In lieu of a snapshot m2 repository with Spring artifacts I had to manually put into our repo, however, will be waiting for an official release. Cheers!