Metadata provided by ImportAware ignores conditions and is dependent on the order of the configuration classes [SPR-12128]

[spring-projects-issues]

Andy Wilkinson opened SPR-12128 and commented

This is related to #16410. It's probably best illustrated with the attached tests.

There are two configuration classes involved: ConfigurationOne and ConfigurationTwo. Both import SomeConfiguration via @EnableSomeConfiguration, however ConfigurationTwo is skipped due to a condition. The problem appears to be that the logic that drives setImportMetadata doesn't consider multiple imports or that some of them may have been skipped: it always passes in the metadata from the last import that was processed. This is illustrated by the two tests in the attached file.

In the event of a single unskipped imported, I'd expect it to provide the metadata and, therefore, for the order in which the configuration classes are processed to make no difference.

---

Affects: 4.0.6

Reference URL: spring-projects/spring-boot#1451

Attachments:

• ImportMetadataTests.java (3.51 kB)

Issue Links:

• @Conditional may prevent an import from taking effect [SPR-11788] #16410 @Conditional may prevent an import from taking effect
• Consider backporting SPR-12128 (ImportAware with conditions) [SPR-12201] #16815 Consider backporting SPR-12128 (ImportAware with conditions)

[spring-projects-issues]

Juergen Hoeller commented

Andy Wilkinson, did you specifically mean to set a condition for the REGISTER_BEAN phase? It seems to be working for a condition in the PARSE_CONFIGURATION phase. At the time of REGISTER_BEAN evaluation, imports will have been processed already, so I'm afraid such conditions don't actually prevent an import to begin with...

Juergen

[spring-projects-issues]

Andy Wilkinson commented

I did. It's a stand-in for Spring Boot's OnBeanCondition which is processed in the REGISTER_BEAN phase.

I think the key thing is that, in the context of ImportMetadataTests, the SomeConfiguration bean is created solely due to ConfigurationOne – if ConfigurationOne is removed from the annotated classes used to create the context, a bean for SomeConfiguration is never created. It therefore feels wrong to me that when one is created, it's configured with the metadata from ConfigurationTwo.

[spring-projects-issues]

Juergen Hoeller commented

I've been discussing this with Phil Webb a bit already, and it seems that configuration classes and their imports have to be processed as usual if not excluded by a PARSE_CONFIGURATION condition... So we'd have to introduce some extra rules to post-process ImportRegistry's content based on REGISTER_BEAN conditions later on.

Phil and I will have another pass - maybe something reveals itself at second glance :-)

Juergen

[spring-projects-issues]

Juergen Hoeller commented

The simplest solution is to evaluate the condition with a REGISTER_BEAN phase in ConfigurationClassParser.processImports, analogous to how we evaluate before processing @ComponentScan declarations. This is technically a change in behavior but should lead to the same end result in all regular cases, and fixes the ImportAware problem.

Juergen

[spring-projects-issues]

Juergen Hoeller commented

Phil Webb, Andy Wilkinson, I've committed the simplest possible change along the lines above to master. Please give it a try with Boot...

In practice, the entire REGISTER_BEAN distinction is only really used by Boot, so that's exactly what it needs to be fine-tuned for.

Juergen

[spring-projects-issues]

Phil Webb commented

Juergen Hoeller I don't see the commit, did you push it?

[spring-projects-issues]

Juergen Hoeller commented

andyw, Phil Webb, how important do you consider this for a backport to 4.0.7? I'm going to push it to master in a few minutes (now for real) but will wait with a backport decision until tomorrow...

Juergen

[spring-projects-issues]

Andy Wilkinson commented

The fix doesn't work if a "proper" missing bean condition is used:

private static final class OnMissingBeanCondition implements ConfigurationCondition {

	@Override
	public boolean matches(ConditionContext context, AnnotatedTypeMetadata metadata) {
		return context.getBeanFactory().getBeanNamesForType(MetadataHolder.class,
				true, false).length == 0;
	}

	@Override
	public ConfigurationPhase getConfigurationPhase() {
		return ConfigurationPhase.REGISTER_BEAN;
	}
}

Apologies, I'm at least partly to blame as my simplified tests masked this behaviour due to their condition that would never match.

AFAICT, the problem is that the new condition check occurs before there's been a chance for the bean to be registered. This means that the condition always matches so we're back to both imports being processed and considered as metadata sources, making their ordering key.

[spring-projects-issues]

Juergen Hoeller commented

Hmm, I more or less expected that. I guess that means we'll have to do a rather convoluted late-removal solution.in the ImportRegistry. Alright, I'll give that a try then with the revised condition implementation in the test...

Thanks for the quick turnaround!

Juergen

[spring-projects-issues]

Phil Webb commented

I think I've fixed this now by improving the tracking logic in the ImportRegistry. Juergen Hoeller should this one also be backported?

[spring-projects-issues]

Juergen Hoeller commented

Phil Webb, Andy Wilkinson, I was actually asking you guys for input on the backport decision above :-)

I've slightly revised the latest variant, promoting ImportRegistry to a package-visible top level interface. The general approach looks fine to me; it's the best we can do if we have to remove ImportRegistry entries so late in the game. However, from my perspective, I'd rather not backport this unless it is critical to have in the 4.0.x line. Note that we need to make that decision now; 4.0.8 is not planned before November/December.

Juergen

[spring-projects-issues]

Andy Wilkinson commented

There's a workaround for the specific case that Artem described in the original Boot issue (setting spring.jmx.default_domain) so, based on that, it's difficult to argue that it's critical. Perhaps Artem has other cases that can't be worked around?