SpringBoot NPE in BeanDefinitionLoader when loading non-Class sources and XML support is disabled

Hi,

I was playing with spring.xml.ignore and found an NPE when loading CharSequence sources.

The following application snippet should reproduce the error

@SpringBootApplication
public class Application {

    public static void main(String[] args) {
        SpringApplication app = new SpringApplication(Application.class);
        app.setSources(Set.of("test", "source"));
        app.run(args);
    }

}

When running java -Dspring.xml.ignore=true -jar app.jar the following NPE occurs:

java.lang.NullPointerException: null
        at org.springframework.boot.BeanDefinitionLoader.load(BeanDefinitionLoader.java:192)
        at org.springframework.boot.BeanDefinitionLoader.load(BeanDefinitionLoader.java:155)
        at org.springframework.boot.BeanDefinitionLoader.load(BeanDefinitionLoader.java:136)
        at org.springframework.boot.SpringApplication.load(SpringApplication.java:691)
        at org.springframework.boot.SpringApplication.prepareContext(SpringApplication.java:392)
        at org.springframework.boot.SpringApplication.run(SpringApplication.java:314)

This is due to the fact that xmlReader is null when XML support is disabled. As the reader is only used to get the Environment, I think we can workaround this by just using the scanner field. They should have the same Environment here.

Unfortunately, it's a bit cumbersome to write a test for this. I would need to set the final xmlReader field to null via reflection, which is what I try to avoid usually. Nonetheless, let me know If I should add a test like described that mimics XML support being disabled.

Cheers, Christoph

P.S.: I tried setting spring.xml.ignore in my application.properties first, which didn't work. I think we would need to use Binder facilities to make this work. Maybe that's worth evaluating.

Comment From: wilkinsona

Unfortunately, it's a bit cumbersome to write a test for this.

We talked a bit about this last week. We're increasingly uncomfortable with these Graal-specific changes that are hard to test. Our assumption had been that any problems would be detected by the Spring Graal Native project but it appears to have been missed there too.

@sdeleuze I think we need to revisit our approach here. We either need to figure how how to test these changes in Boot (without relying on Graal) or we need a commitment on your side to test things.

Comment From: wilkinsona

With a change to make ModifiedClasspathExtension public, I think we can test this with the following:

package org.springframework.boot;

import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;

import org.springframework.beans.factory.BeanDefinitionStoreException;
import org.springframework.boot.testsupport.classpath.ModifiedClassPathExtension;
import org.springframework.context.support.StaticApplicationContext;

import static org.assertj.core.api.Assertions.assertThatExceptionOfType;

@ExtendWith(ModifiedClassPathExtension.class)
class IgnoringXmlBeanDefinitionLoaderTests {

    @BeforeAll
    static void ignoreXml() {
        System.setProperty("spring.xml.ignore", "true");
    }

    @AfterAll
    static void enableXml() {
        System.clearProperty("spring.xml.ignore");
    }

    @Test
    void whenXmlSupportIsDisabledXmlSourcesAreRejected() {
        assertThatExceptionOfType(BeanDefinitionStoreException.class)
                .isThrownBy(() -> new BeanDefinitionLoader(new StaticApplicationContext(),
                        "classpath:org/springframework/boot/sample-beans.xml").load())
                .withMessage("Cannot load XML bean definitions when XML support is disabled");
    }

}

Comment From: wilkinsona

Thanks very much, @dreis2211.

Comment From: dreis2211

That's a neat trick to use ModifiedClassPathExtension, @wilkinsona . Given that I wrote this thing, I could have come up with that as well, I guess. ;-)

Comment From: dreis2211

I wonder if we should rather introduce a new annotation ForkedClassPath instead of making ModifiedClassPathExtension public btw.

Comment From: wilkinsona

I wonder if we should rather introduce a new annotation ForkedClassPath instead of making ModifiedClassPathExtension public btw.

I was in two minds about that, and I still am really. I don't have any objection to introducing a custom annotation. I guess we'd want to prevent @ForkedClassPath being used alongside @ClassPathExclusions or @ClassPathOverrides to make it clear that it is intended for the situation where you just need a "copy" of the class path rather than a modified variant of it.

Comment From: dreis2211

Mind if I experiment with that if I find the time?

Comment From: wilkinsona

Not at all. That'd be great!