@ImportAutoConfiguration and @EnableAutoConfiguration import ordering

[bclozel]

While testing for #12318, @snicoll found a limitation in the current ordering between @ImportAutoConfiguration and @EnableAutoConfiguration.

Consider the following test application:

@RunWith(SpringRunner.class)
@SpringBootTest(classes = {
		WebTestClientSpringBootTestIntegrationTests.TestConfiguration.class,
		ExampleWebFluxApplication.class })
@AutoConfigureWebTestClient
public class WebTestClientSpringBootTestIntegrationTests {
}

This test starts a Spring Boot application with all auto-configurations and use the @AutoConfigureWebTestClient test annotation. The current issue is that all auto-configurations imported by @AutoConfigureWebTestClient cannot rely on @AutoConfigureAfter/@AutoConfigureBefore, since things are processed in two different phases.

Currently:

• @EnableAutoConfiguration is using AutoConfigurationImportSelector, which is ordered at Ordered.LOWEST_PRECEDENCE - 1
• @ImportAutoConfiguration is using ImportAutoConfigurationImportSelector (itself extending from AutoConfigurationImportSelector), which is ordered at super.getOrder() - 1, so Ordered.LOWEST_PRECEDENCE - 2.

Because of this, ImportAutoConfigurationImportSelector auto-configurations are processed before Spring Boot auto-configurations.

Solutions

Setting ImportAutoConfigurationImportSelector::getOrder to Ordered.LOWEST_PRECEDENCE fixes the issue, and the whole Spring Boot build is green.

Doing that change would imply that:

1. Ordering and bean conditions (like @AutoConfigureBefore/@AutoConfigureAfter) can only be applied to a set of auto-configurations imported by the same selector. So for @ImportAutoConfiguration, those are only applied within the scope of test slicing annotations.
2. In the case of @SpringBootTest, we agree that @ImportAutoConfiguration should always run after the Spring Boot auto-configurations and that ordering and bean conditions on test auto-configurations won't apply to auto-configurations contributed by @EnableAutoConfiguration

Flagging this as for:team-attention.

[snicoll]

There is an alternative solution that would require a change in the framework. If we had a new callback interface to be able to identify and merge DeferredImportSelector implementations, then we wouldn't need to order them at all. We'd just merge the two sets and order them in one go.

IMO, it would be better of the current situation:

1. If you are using a slice test, then a restricted list of auto-configurations are imported and you can order them any way you like
2. If you are using a test infrastructure with a dedicated annotation (such as @ AutoConfigureWebTestClient), then ordering with other auto-configurations is ignored. If we implement the suggestion above, it wouldn't be a problem for @AutoConfigureAfter (always true) but it would make @AutoConfigureBefore impossible to implement

The fact that ordering has different semantic in slice test and "integration test" is a serious issue even it hasn't surfaced so far.

[philwebb]

I think that there might be some issues if we adopt an approach similar to snicoll@3eff5dc. The ConfigurationClassParser.processDeferredImportSelectors method currently calls back into processImports using the config class as the currentSourceClass. I'm worried that if we add an additional DeferredImportSelector it might end up importing classes from the wrong source. This might not be an issue, but I think it could mess up conditions.

[snicoll]

I've managed to break ordering.

BatchAutoConfiguration should be configured after HibernateJpaAutoConfiguration and HibernateJpaAutoConfiguration should be configured after DataSourceAutoConfiguration. This changes sorts auto-configurations at the very end.

If Hibernate is not on the classpath, the auto-configuration in the middle is removed too early and we lose that link between BatchAutoConfiguration and DataSourceAutoConfiguration.

I'd argue that the fact the auto-configuration "in the middle" must be present in the list for the ordering to work properly can be seen as a bug of the ordering algorithm.

[snicoll]

This test reproduces the issue in the sorting algorithm

@Test
public void useAnnotationWithNoDirectLink() throws Exception {
	MetadataReaderFactory readerFactory = mock(MetadataReaderFactory.class);
	this.autoConfigurationMetadata = getAutoConfigurationMetadata(A, B, D);
	this.sorter = new AutoConfigurationSorter(readerFactory,
			this.autoConfigurationMetadata);
	List<String> actual = this.sorter.getInPriorityOrder(Arrays.asList(D,B));
	assertThat(actual).containsExactly(B, D);
}

[wilkinsona]

I think that test should fail due to a cycle between A, B, and D. That's a bug in itself, but sounds different to the one that broke the build.