Allow @Bean methods to override definitions in XML [SPR-7028]

[spring-projects-issues]

Dave Syer opened SPR-7028 and commented

There are two ways I might want to do this, and both fail.

1. Use @ImportResource in an @Configuration to load an XML file, then override a bean using @Bean
2. Write an XML file that imports another file and then defines a bean that is @Configuration, and the latter provides a bean with the same name

Neither works.

Case 1 fails because ConfigurationClassBeanDefinitionReader always loads XML imports after the @Bean definitions. This seems like the wrong order, so it would be good to understand why it is implemented that way (I imported before I defined the @Bean, so I expect the latter to win).

This code in ConfigurationClassBeanDefinitionReader prevents the override in case 2:

if (!(existingBeanDef instanceof ConfigurationClassBeanDefinition)) {
	// no -> then it's an external override, probably XML
	// overriding is legal, return immediately
	return;
}

---

Affects: 3.0.1

Issue Links:

• Revised support for bean definition visibility and overriding [SPR-8189] #12839 Revised support for bean definition visibility and overriding ("is depended on by")
• Reconsider whether to generally allow bean overriding by name [SPR-10808] #15434 Reconsider whether to generally allow bean overriding by name
• Bean Configuration Overriding [SPR-5509] #10181 Bean Configuration Overriding
• Support profile exclusivity and/or bean definition finality [SPR-7982] #12637 Support profile exclusivity and/or bean definition finality
• Allow bean definitions from @Configuration classes to override those defined in XML [SPR-7341] #12000 Allow bean definitions from @Configuration classes to override those defined in XML

8 votes, 12 watchers

[spring-projects-issues]

Chris Beams commented

Scheduling for 3.1 timeline. This is certainly an important issue, and will require a bit of thought as to exactly how we implement it. At this point, something user-configurable, probably on ConfigurationClassPostProcessor will be the way to go (think SYSTEM_PROPERTIES_MODE_* on PropertyPlaceholderConfigurer as a rough analogy).

It could be argued that overriding semantics should be exposed on the @Bean or @Configuration annotations, but I'm not sure about this. Annotations are a static thing, and overriding is context-specific, i.e.: in configuration scenario A, I want a @Bean to override XML, but in config scenario B, I do not. For this reason, I think exposing a switch at the PostProcessor (and ultimately through a namespace element) is probably the way to go.

Even this approach has edge case issues, however. It could be that XML exposes <bean/> definitions b1 and b2, and also includes @Configuration classes c1 and c2. It could be that the user wants @Bean c1.b1() to override the XML definition of b1, but wants the XML definition of b2 to override @Bean c2.b2(). This is probably obscure enough not to care about, but good to call out for completeness.

[spring-projects-issues]

Dave Syer commented

I don't think we need to make it complicated. What exactly are the use cases driving the current behaviour anyway?

The semantics are well defined for bean definition readers and bean factories: bean definitions parsed later override those parsed earlier. Let's try and keep that guarantee and work out how to make the ordering of resources (class, xml, etc) easy to reason about. E.g. it seems intuitive to me that an @ImportResource at the top of an @Configuration is before any @Bean defined in the body.

[spring-projects-issues]

Chris Beams commented

test from mylyn. disregard.

[spring-projects-issues]

Neale Upstone commented

+1 on Dave's suggestion here.

I don't exactly love the default behaviour of overriding (personally thinking that nested <beans final='true'> or <beans override='true'> could be helpful for clarifying intent in the same way as the semantics of @Override and final in Java), but to have the meaning of order reversed is even worse.

[spring-projects-issues]

Chris Beams commented

Okay, I've given this some thought. Take a look and please provide feedback. If we can keep the scope simple, this can probably still make it into 3.1 M2.

Dave's "case 1" above deals with a situation like the following:

[some.xml]
<beans>
    <bean id="foo" class="Foo"/>
</beans>

@ImportResource("some.xml")
@Configuration
class Config {
    @Bean
    Foo foo() { return new Foo(); }
}

The question is who should 'win' -- the <bean/> or the @Bean?

The answer could be that the <bean/> should win, because the @Configuration imports the XML file; i.e. the @Configuartion class is first, the XML second by logical definition. Bean definition overriding has always been a 'last parsed' wins scenario, and from this perspective the configuration file should be parsed first, and the XML should be imported and parsed second.

The answer could be that the @Bean should win, because the @Bean is literally declared after the @ImportResource in terms of actual line numbers in the class file. This is perhaps trying to draw too close an analogy between @Configuration classes and XML files. In the latter, an <import/> can be placed anywhere (before or after any <bean/> definition); in the former, @ImportResource can only exist at the top of the file. It's therefore false to think of import order as literally the same in a @Configuration file world. That said, having the @Bean 'win' might be exactly what the user wants, as in Dave's case. And perhaps this is the common case -- it would be hard to know.

In my mind, there's not a clear answer to whether the <bean/> or @Bean should win when using @ImportResource. For this reason, I propose we change the default to have @Bean win when using @ImportResource, and that we provide an attribute on @ImportResource to make this behavior configurable.

@ImportResource(value="some.xml", override=true)
class Config { ... }

The above indicates that "beans imported from the some.xml resource should override @Bean definitions in this @Configuration class and any @Import-ed @Configuration classes.

The default for the override attribute would be false. This technically represents a breaking change, but actual impact is probably quite low, and there's a clear path how to get back to the original behavior by using flipping the attribute to true.

Implementation feasibility for this attribute has not been evaluated. I mainly want to see if the approach would meet the needs discussed here.

Dave's "Case 2" goes something like the following:

<beans>
    <context:annotation-config/>
    <bean id="foo" class="Foo"/>
    <bean class="Config"/>
</beans>

@Configuration
class Config {
    @Bean
    Foo foo() { return new Foo(); }
}

In this case, I agree that the current behavior doesn't make sense. It's pretty clear that the @Bean should win, because the XML is logically 'first' and the @Configuration is logically 'second' from the users point of view. It's less clear if the file looks like this:

<beans>
    <context:annotation-config/>
    <bean class="Config"/>
    <bean id="foo" class="Foo"/>
</beans>

Now, the user's intent might be to override foo in XML because it comes 'after' the Config <bean/> declaration.

I propose the following: Just as above, where @Bean wins by default when using @ImportResource, <bean/> declarations should win when declaring @Configuration classes in XML. Additionally, we can consider providing an attribute on ConfigurationClassPostProcessor similar to the proposed override attribute on @ImportResource. This will allow users to drop down to the post processor if they really need to tweak this default.

Thoughts?

[spring-projects-issues]

Dave Syer commented

I could see some value in your proposal for @ImportResource, but I would name the attribute "overridable" to make it easier to reason about what it is going to do (the beans in this XML are overridable by stuff later in the Java compilation unit).

I see what you are saying but I must admit I don't like the proposal for Case 2. I don't like anything that says "XML always wins" or "Java always wins", and I don't like anything that requires me to modify the context loader in Java (that's like saying to a user he should extend GenericApplicationContext). The existing behaviour of XML where later bean definitions override earlier ones makes sense, so what we should focus on is retaining those semantics and defining carefully the meaning of "later". Including a bean definition of an @Configuration seems like it should be equivalent to using <import/> in the same place. The semantics should match. Is that not possible?

[spring-projects-issues]

Chris Beams commented

Slating this issue to be addressed in 3.2, when we plan to address all 'bean-visibility-and-overriding'-labeled issues (see tracking issue #12839). This will also allow more time for feedback post 3.1 GA to see what actual usage patterns look like.

[spring-projects-issues]

Sam Brannen commented

Chris Beams,

The answer could be that the @Bean should win, because the @Bean is literally declared after the @ImportResource in terms of actual line numbers in the class file.

I think that is completely logical and expected by most users. The fact that it is analogous to the semantics of XML configuration with imports strengthens the argument of going with this as the default semantics for @Configuration classes importing XML config files.

This is perhaps trying to draw too close an analogy between @Configuration classes and XML files.

I disagree. See above. ;)

In the latter, an <import/> can be placed anywhere (before or after any <bean/> definition); in the former, @ImportResource can only exist at the top of the file. It's therefore false to think of import order as literally the same in a @Configuration file world. That said, having the @Bean 'win' might be exactly what the user wants, as in Dave's case. And perhaps this is the common case -- it would be hard to know.

Instead of (or perhaps in addition to) introducing an override or overridable attribute in @ImportResource, how about just introducing an attribute to control when the imported resource is physically imported with respect to the current @Configuration class? For example, by default the imported resource would be imported before the config class, and via the flag the user could opt to have the imported resource imported after the config class. Then the well-known "last bean definition wins" semantics would be straightforward.

Thoughts?

Sam

[spring-projects-issues]

Dave Syer commented

how about just introducing an attribute to control when the imported resource is physically imported with respect to the current @Configuration class?

That works for me and I think it is intuitive for users.

[spring-projects-issues]

thomas menzel commented

this just cost me 5hrs of my life to figure out why my overriding @Bean def. in a @Configuration wasnt being executed!
so absolutely non-obvious to me that this aint working.

and guys: this issue is 5 yrs old!
PS: i would suggest to mark #12000 a dup of this.

[spring-projects-issues]

Conor Gallagher commented

I highly suggest keeping the current behaviour as default, the blast-radius would be far larger than previous comments suggest.

For example, our product has an OOTB wiring via a combination of @Configuration classes, Profiles, and Auto-wiring. We allow our customers to override default (or profile controlled) behaviour via XML using @ImportResource on our @Configuration classes. This works beautifully in our current setup. If a customer wants to override a bean we didn't anticipate they can do so by dropping an XML definition of that bean on the classpath which will take precedence.

If the precedence were to be changed in a future Spring release without us noticing, then the behaviour of our customer deployments would quietly change.

[spring-projects-issues]

Stéphane Nicoll commented

Conor Gallagher you could fix that by having a deterministic order for the product's configuration, making sure that those customizations happens last. I'd argue that you should do this independently of the current mechanism.

[spring-projects-issues]

Conor Gallagher commented

I'm just trying to call out that this would be a massive breaking change, the impact of which could be devastating. Below is simply not true:

This technically represents a breaking change, but actual impact is probably quite low

Imagine a scenario where a developer revs the version of Spring without reading the release notes throughly! :) Everything works as before and all their tests pass, but one subtle thing has changed in production.

Their setup is something along the lines of

@Configuration
@ImportResource("classpath:spring/customer-config.xml")
public class BaseConfig {
    @Bean(name = "paymentProcessor")
    public PaymentProcessor getPaymentProcessor() {
        return new DummyPaymentProcessor();
    }
}

Their individual customers define the appropriate payment processors via xml as follows:

<beans >
    <bean id="paymentProcessor" class="com.visa.VisaPaymentProcessor" />
</beans>

In all their test environments the behaviour of the application is unchanged, but in production the Visa paymentProcessor they defined is no longer getting used. All payments are now being processed by a dummy payment processor!

Obviously this is a contrived example, and I'm not advocating this setup. But please keep this sort of thing in mind before you change the default behaviour.

[spring-projects-issues]

Stéphane Nicoll commented

Conor Gallagher with such a reasoning, we wouldn't be able to evolve anything at all, really. I am not sure that "not reading the release notes" is an argument either. 5.0 is out now and this issue hasn't been assigned so we're not going to change that any time soon. I already gave you an escape path that you can apply right now.

[spring-projects-issues]

Juergen Hoeller commented

The main reason why we have not changed this particular arrangement for many years is exactly the backwards compatibility consideration. We indeed reserve the right to change defaults in some cases... but arguably not in this one. In particular since overriding between XML and Java config arrangements is not recommendable to begin with; it's complicated enough if you override within an XML bean definition arrangement.

In short: The default behavior is not going to change here. We'd only do that if there was another first-class scenario that we couldn't make work otherwise.