ImportSelector#getExclusionFilter does not exclude matching candidates with import selector

[snicoll]

ImportSelector#getExclusionFilter is a way to let an import selector apply an exclude to classes that are imported. We use this in Spring Boot to filter out classes that are about to be imported and that have conditions we can run statically eagerly and to avoid loading unnecessary classes.

CacheAutoConfiguration has an import selector that loads the various configuration candidates. That's handled primarily by the following code:

spring-framework/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassParser.java

Lines 581 to 583 in df588e0

	String[] importClassNames = selector.selectImports(currentSourceClass.getMetadata());
	Collection<SourceClass> importSourceClasses = asSourceClasses(importClassNames, exclusionFilter);
	processImports(configClass, currentSourceClass, importSourceClasses, exclusionFilter, false);

The candidates for the import selector are as follows:

0 = "org.springframework.boot.autoconfigure.cache.GenericCacheConfiguration"
1 = "org.springframework.boot.autoconfigure.cache.JCacheCacheConfiguration"
2 = "org.springframework.boot.autoconfigure.cache.EhCacheCacheConfiguration"
3 = "org.springframework.boot.autoconfigure.cache.HazelcastCacheConfiguration"
4 = "org.springframework.boot.autoconfigure.cache.InfinispanCacheConfiguration"
5 = "org.springframework.boot.autoconfigure.cache.CouchbaseCacheConfiguration"
6 = "org.springframework.boot.autoconfigure.cache.RedisCacheConfiguration"
7 = "org.springframework.boot.autoconfigure.cache.CaffeineCacheConfiguration"
8 = "org.springframework.boot.autoconfigure.cache.SimpleCacheConfiguration"
9 = "org.springframework.boot.autoconfigure.cache.NoOpCacheConfiguration"

The exclusion filter is going to match for a number of them. As a result the importSourceClasses collection is the following:

0 = {org.springframework.context.annotation.ConfigurationClassParser$SourceClass@3426} "org.springframework.boot.autoconfigure.cache.GenericCacheConfiguration"
1 = {org.springframework.context.annotation.ConfigurationClassParser$SourceClass@2464} "java.lang.Object"
2 = {org.springframework.context.annotation.ConfigurationClassParser$SourceClass@2464} "java.lang.Object"
3 = {org.springframework.context.annotation.ConfigurationClassParser$SourceClass@2464} "java.lang.Object"
4 = {org.springframework.context.annotation.ConfigurationClassParser$SourceClass@2464} "java.lang.Object"
5 = {org.springframework.context.annotation.ConfigurationClassParser$SourceClass@2464} "java.lang.Object"
6 = {org.springframework.context.annotation.ConfigurationClassParser$SourceClass@2464} "java.lang.Object"
7 = {org.springframework.context.annotation.ConfigurationClassParser$SourceClass@2464} "java.lang.Object"
8 = {org.springframework.context.annotation.ConfigurationClassParser$SourceClass@3427} "org.springframework.boot.autoconfigure.cache.SimpleCacheConfiguration"
9 = {org.springframework.context.annotation.ConfigurationClassParser$SourceClass@3428} "org.springframework.boot.autoconfigure.cache.NoOpCacheConfiguration"

Those 7 "configuration classes" are then later on processed. It leads ultimately to a java.lang.Object bean to be defined.

The reason for this is because of a shortcut when the exclude filter matches:

spring-framework/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassParser.java

Lines 684 to 686 in df588e0

	if (className == null || filter.test(className)) {
	return this.objectSourceClass;
	}

Perhaps an improvement would be a way to not contribute the SourceClass at all so that it's not processed?

[injae-kim]

        // @return null when exclustionFilter matches className
        @Nullable
	SourceClass asSourceClass(@Nullable String className, Predicate<String> exclusionFilter) throws IOException {
		if (className == null)
			return this.objectSourceClass;
		}
                if (exclusionFilter.test(className) { // 👈👈 should be excluded
			return null
                }
         }

        // usage example
	private Collection<SourceClass> asSourceClasses(String[] classNames, Predicate<String> filter) throws IOException {
		List<SourceClass> annotatedClasses = new ArrayList<>(classNames.length);
		for (String className : classNames) {
                        SourceClass sourceClass = asSourceClass(className, filter);
                        if (sourceClass != null) { // 👈👈 should exclude sourceClass
                            annotatedClasses.add(sourceClass);
                        }
		}
		return annotatedClasses;
	}

(just idea) maybe we can explicitly return null on asSourceClass() when exclusionFilter matches className.
and exclude it properly on caller side like above asSourceClasses? 🤔

[snicoll]

@injae-kim I don't know. I think the first step is to reproduce this scenario in a test here and then investigate. Import selector is quite involved already.

[injae-kim]

I think the first step is to reproduce this scenario in a test here and then investigate

aha~ I understood. if you don't start to investigate this issue yet, may I investigate this more?

I'll check this case and share the result to you within next friday~!

[snicoll]

Thanks for asking. Yes, I am interested as I am busy on other issues at the moment.

[injae-kim]

Investigation

aa4e56b Optimize @configuration class parsing a little

       private final SourceClass objectSourceClass = new SourceClass(Object.class);

	SourceClass asSourceClass(@Nullable Class<?> classType) throws IOException {
		if (classType == null || classType.getName().startsWith("java.lang.annotation")) { // 👈
			return this.objectSourceClass;
		}

(based on commit message) I think at first, we just want to skip java.lang.annotation types which were often processed but would never provide useful results.
Also use a single shared immutable SourceClass instance to represent Object.class.

d93303c ImportSelector.getCandidateFilter() for transitive filtering of classes

After that, we introduced ImportSelector#getExclusionFilter().

         SourceClass asSourceClass(@Nullable String className, Predicate<String> filter) throws IOException {
		if (className == null || filter.test(className)) { // 👈
			return this.objectSourceClass;
		}

spring-framework/spring-context/src/main/java/org/springframework/context/annotation/ImportSelector.java

Lines 73 to 75 in 4f16297

	* <p>If this predicate returns {@code true} for a given fully-qualified
	* class name, said class will not be considered as an imported configuration
	* class, bypassing class file loading as well as metadata introspection.

But unlike above comment, we don't bypass when candidates match exclusionFilter.
Instead we just return this.objectSourceClass so "java.lang.Object" bean is defined as you comment on issue.

Proposal

injae-kim@753668b

	@Nullable
	SourceClass asSourceClass(@Nullable Class<?> classType, Predicate<String> filter) throws IOException {
		if (filter.test(classType.getName())) {
			return null;  // 👈
		}

So my proposal is, explicitly return null when exclusionFilter matches and don't process it. (I checked test passed on above commit)

spring-framework/spring-context/src/test/java/org/springframework/context/annotation/ImportSelectorTests.java

Lines 368 to 370 in 4f16297

	public Predicate<String> getExclusionFilter() {
	return className -> className.endsWith("ImportedSelector1");
	}

But I'm not sure how to test it 😅 maybe above test can cover this change?

@snicoll PTAL when you have some times, and feel free to share your opinion and how can we test it. thanks!