Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

For @Bean method that returns null, @Autowired injects NullBean instead of null for cached arguments #30485

Closed
qiangyt opened this issue May 14, 2023 · 12 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) status: backported An issue that has been backported to maintenance branches type: bug A general bug type: documentation A documentation task
Milestone

Comments

@qiangyt
Copy link

qiangyt commented May 14, 2023

Affects: 5.x


See below test case:

import org.testng.annotations.Test;

import org.testng.Assert;
import org.springframework.context.annotation.AnnotationConfigApplicationContext;
import org.springframework.context.annotation.Bean;

public class NullBeanIssue {

    @Test
    public void test_shouldGetNull() {
        var ctx = new AnnotationConfigApplicationContext();
        ctx.register(MyFactoryBean.class);
        ctx.refresh();

        var myBean = ctx.getBean("myBean");

        // expects null because MyFactoryBean.myBean returns null,
        // however, I got a NullBean instance here
        Assert.assertNull(myBean);

        ctx.close();
    }

}

class MyBean {
}

class MyFactoryBean {

    @Bean
    public MyBean myBean() {
        return null; // in real project cases, return value might be null or not null that depends.
    }

}
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label May 14, 2023
@quaff
Copy link
Contributor

quaff commented May 15, 2023

Have you tried latest version? IIRC this is fixed long long ago.

@quaff
Copy link
Contributor

quaff commented May 15, 2023

I can verify injection works fine but get bean doesn't work, tested with v6.0.8.

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

import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.context.annotation.AnnotationConfigApplicationContext;
import org.springframework.context.annotation.Bean;
import org.springframework.test.context.ContextConfiguration;
import org.springframework.test.context.junit.jupiter.SpringExtension;

@ContextConfiguration(classes = NullBeanTests.Config.class)
@ExtendWith(SpringExtension.class)
public class NullBeanTests {

	@Autowired
	private String myBean;

	@Test
	void testInjection() { // will pass
		assertThat(myBean).isNull();
	}

	@Test
	public void testGetBean() { // will fail
		try (var ctx = new AnnotationConfigApplicationContext()) {
			ctx.register(Config.class);
			ctx.refresh();
			assertThat(ctx.getBean("myBean")).isNull();
		}
	}

	static class Config {

		@Bean
		public String myBean() {
			return null;
		}

	}
}

@qiangyt qiangyt closed this as completed May 15, 2023
@qiangyt qiangyt reopened this May 15, 2023
@qiangyt
Copy link
Author

qiangyt commented May 15, 2023

Have you tried latest version? IIRC this is fixed long long ago.

Not yet tried 6.x, but all 5.x have this issue.

@qiangyt
Copy link
Author

qiangyt commented May 15, 2023

Injection works, but applicationContext.getBean() doesn't work. I updated the issue title.

@qiangyt qiangyt changed the title For @Autowired(required=false) dependencies, expects null if dependence is null, but got a NullBean instance actually For @Bean factory method that returns null, expects null if but got a NullBean instance actually May 15, 2023
@qiangyt qiangyt changed the title For @Bean factory method that returns null, expects null if but got a NullBean instance actually For @Bean factory methods that returns null, expects null if but got a NullBean instance actually May 15, 2023
@qiangyt qiangyt changed the title For @Bean factory methods that returns null, expects null if but got a NullBean instance actually For @Bean factory methods that returns null, ApplicationContext.getBean() returns a NullBean instead of expected real null May 15, 2023
@sbrannen
Copy link
Member

sbrannen commented May 15, 2023

@qiangyt, thanks for reporting this.

@quaff, thanks for providing the autowiring/injection test case.

Interestingly, it turns out that autowiring does not consistently work either.

Here's a modified version of the combined tests which demonstrates that.

import org.junit.jupiter.api.MethodOrderer.OrderAnnotation;
import org.junit.jupiter.api.Order;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.TestMethodOrder;

import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.context.annotation.AnnotationConfigApplicationContext;
import org.springframework.context.annotation.Bean;
import org.springframework.test.context.junit.jupiter.SpringJUnitConfig;

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

@SpringJUnitConfig(NullBeanTests.Config.class)
@TestMethodOrder(OrderAnnotation.class)
class NullBeanTests {

	@Autowired
	String myBean;

	@Test
	@Order(1)
	void fieldInjection() {
		assertThat(myBean).isNull();
	}

	@Test
	@Order(2)
	void parameterInjection(@Autowired String myBean) {
		assertThat(myBean).isNull();
	}

	@Test
	@Order(99)
	void getBean() {
		try (var ctx = new AnnotationConfigApplicationContext(Config.class)) {
			Object bean = ctx.getBean("myBean");
			System.err.println(bean != null ? bean.getClass().getName() : null);
			assertThat(bean).isNull();
		}
	}

	// @Configuration
	static class Config {

		@Bean
		String myBean() {
			return null;
		}

	}

}

getBean() always fails.

If you run fieldInjection() or parameterInjection() by itself, each will pass.

If you run the NullBeanTests above unmodified, fieldInjection() will pass but only because it runs first. The other two test methods fail.

If you change the @Order of fieldInjection() to 10, parameterInjection() will pass and fieldInjection() will fail.

If you change the @Order of getBean() to 0, all three tests will fail.

If you only change the NullBeanTests above so that String myBean field is annotated with @Autowired(required = false), then fieldInjection() and parameterInjection() will both pass.

So, there is something interesting going on here, and it's definitely a bug (and possibly a regression).

We'll look into it.

@sbrannen sbrannen added type: bug A general bug in: core Issues in core modules (aop, beans, core, context, expression) and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels May 15, 2023
@sbrannen sbrannen added this to the 6.0.10 milestone May 15, 2023
@sbrannen sbrannen changed the title For @Bean factory methods that returns null, ApplicationContext.getBean() returns a NullBean instead of expected real null For @Bean method that returns null, ApplicationContext.getBean() and @Autowired result in NullBean instead of null May 15, 2023
@jhoeller
Copy link
Contributor

In terms of intended behavior, @Autowired should consistently resolve to null wherever necessary (so there may indeed be some subtle bug here) but getBean calls are actually designed to return a NullBean instance as of 5.0, whereas the behavior was strictly speaking undefined before. We recommend avoiding null beans completely, but if you keep returning null from a factory method, that's the container behavior that you'll get as of 5.0. This has been the case for more than five years already.

So for getBean calls, we do not support null return values there anymore, whereas we do support resolving injection points such as @Autowired to null. If you'd like to programmatically test for a NullBean instance received from getBean, you may call .equals(null) on it, but you're not going to receive a plain null value from getBean directly anymore.

@sbrannen
Copy link
Member

getBean calls are actually designed to return a NullBean instance as of 5.0

I knew we used NullBean internally and that we sometimes adapt it to null, but I don't know if I ever knew that we literally return NullBean instances to the application.

Is that documented anywhere?

@sbrannen sbrannen changed the title For @Bean method that returns null, ApplicationContext.getBean() and @Autowired result in NullBean instead of null For @Bean method that returns null, @Autowired sometimes injects a NullBean instead of null May 15, 2023
@qiangyt
Copy link
Author

qiangyt commented May 15, 2023

NullBean is an internal class (the class is not public/protected), it is a bit strange to be the result of ApplicationContext.getBean() API.

@qiangyt qiangyt closed this as completed May 15, 2023
@qiangyt qiangyt reopened this May 15, 2023
@qiangyt
Copy link
Author

qiangyt commented May 15, 2023

sorry for making mistake of close/re-open the issue.

@qiangyt
Copy link
Author

qiangyt commented May 15, 2023

Another case is, for a factory method that returns null, ApplicationContext.getBean("myBean", MyBean.class) will raise a BeanNotOfRequiredTypeException with a message "Bean named 'myBean' is expected to be of type 'org.springframework.beans.factory.support.MyBean' but was actually of type 'org.springframework.beans.factory.support.NullBean", I feel it strange for users.

@jhoeller
Copy link
Contributor

jhoeller commented May 15, 2023

Good point, Sam, I'm afraid it might not be fully documented.

In this view, a "bean" is never null. Only an injection point can resolve to null, or rather remain unresolved and therefore evaluate to null. This also gives strict non-null guarantees to getBean callers, not forcing them to apply defensive null checks, which includes bean post-processors and other infrastructure components.

Stubbing out a bean instance with null can only happen from factory methods and is arguably not great practice to begin with. We leniently tolerate it as an indicator for an optional bean and that's ok. However, use an @Autowired injection point or BeanFactory.getBeanProvider to resolve such beans, not hard getBean calls.

As for NullBean being an internal type, that's a bit unusual indeed. However, there is nothing to do with the type in any case. It's a stub with .equals(null) behavior and a "null" toString output that does not fail with an NPE when doing checks against it. Making it public would not provide practical value beyond that.

@sbrannen sbrannen added the type: documentation A documentation task label May 15, 2023
@jhoeller jhoeller self-assigned this May 26, 2023
@jhoeller
Copy link
Contributor

This turns out to be a bug in shortcut resolution for cached field/method arguments. We're replacing the full dependency resolution algorithm with a getBean call there which does not leniently resolve to null as the original resolution algorithm would.

In addition, I'll also add a few documentation notes towards not expecting null from getBean calls, hinting at getBeanProvider usage instead.

@jhoeller jhoeller changed the title For @Bean method that returns null, @Autowired sometimes injects a NullBean instead of null For @Bean method that returns null, @Autowired injects NullBean instead of null for cached arguments May 26, 2023
@jhoeller jhoeller added the for: backport-to-5.3.x Marks an issue as a candidate for backport to 5.3.x label May 26, 2023
@github-actions github-actions bot added status: backported An issue that has been backported to maintenance branches and removed for: backport-to-5.3.x Marks an issue as a candidate for backport to 5.3.x labels May 26, 2023
jhoeller added a commit that referenced this issue May 26, 2023
Includes getBean documentation against NullBean values.

Closes gh-30485

(cherry picked from commit 8b8d147)
mdeinum pushed a commit to mdeinum/spring-framework that referenced this issue Jun 29, 2023
Includes getBean documentation against NullBean values.

Closes spring-projectsgh-30485
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) status: backported An issue that has been backported to maintenance branches type: bug A general bug type: documentation A documentation task
Projects
None yet
Development

No branches or pull requests

5 participants