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 a prototype bean, if first-time rejected value is null, subsequent value will wrongly be null always #30794

Closed
qiangyt opened this issue Jul 3, 2023 · 4 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
Milestone

Comments

@qiangyt
Copy link

qiangyt commented Jul 3, 2023

Affects: 5.3.28


Below is a test that is able to reproduce the issue.

import org.junit.jupiter.api.Test;

import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.context.annotation.AnnotationConfigApplicationContext;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Scope;

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

public class NotRequiredIssue {
    
    @Test
    void test() {
        AnnotationConfigApplicationContext ctx 
            = new AnnotationConfigApplicationContext(Bean1.class, Bean2Factory.class);
        
        Bean1 bean1A = ctx.getBean(Bean1.class);
        // Bean2Factory.bean2() returns null initially, so Bean1.bean2 got null injected
        assertThat(bean1A.bean2).isNull();
        
        Bean2 bean2 = new Bean2();

        Bean2Factory bean2Factory = ctx.getBean(Bean2Factory.class);
        // ask Bean2Factory.bean2() to return non-null instance
        bean2Factory.bean2 = bean2;
        
        Bean1 bean1B = ctx.getBean(Bean1.class);
        // expects Bean1.bean2 is non-null, however, still got null
        assertThat(bean1B.bean2).isNotNull();
        assertThat(bean1B.bean2).isSameAs(bean2);

        ctx.close();
    }

    @Scope("prototype")
    static class Bean1 {

        @Autowired(required = false) // required=false, otherwise will get exception 
        Bean2 bean2;
        
    }

    static class Bean2 {
    }

    static class Bean2Factory {

        Bean2 bean2;

        @Scope("prototype") @Bean
        Bean2 bean2() {
            return this.bean2;
        }
    }

}
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jul 3, 2023
@jhoeller jhoeller self-assigned this Jul 3, 2023
@jhoeller
Copy link
Contributor

jhoeller commented Jul 3, 2023

An initial note: The Bean2Factory class itself should not declare @Scope("prototype") in this case, otherwise anything set on it will not survive until the next Bean2 retrieval attempt since the factory instance is recreated every time.

With that sorted out, the actual null handling mismatch comes from an explicit check where null values for non-required injection points will indeed remain null forever. We'll have to revisit where that special check came from. Generally speaking, the null in that check is meant to indicate "not resolvable" (a permanent state) rather than "currently not present" (a temporary state), based on no matching bean defined at all as opposed to a matching factory method currently returning null.

For the time being, you could consider using Optional<Bean2> or ObjectProvider<Bean2> for your injection point instead. The required flag is not relevant then, the optional presence checks can happen programmatically on an always-injected handle. Design-wise, that's generally sensible since you do not have to rely on the ambiguous semantics of null.

@sbrannen sbrannen added the in: core Issues in core modules (aop, beans, core, context, expression) label Jul 3, 2023
@qiangyt
Copy link
Author

qiangyt commented Jul 3, 2023

An initial note: The Bean2Factory class itself should not declare @Scope("prototype") in this case, otherwise anything set on it will not survive until the next Bean2 retrieval attempt since the factory instance is recreated every time.

With that sorted out, the actual null handling mismatch comes from an explicit check where null values for non-required injection points will indeed remain null forever. We'll have to revisit where that special check came from. Generally speaking, the null in that check is meant to indicate "not resolvable" (a permanent state) rather than "currently not present" (a temporary state), based on no matching bean defined at all as opposed to a matching factory method currently returning null.

For the time being, you could consider using Optional<Bean2> or ObjectProvider<Bean2> for your injection point instead. The required flag is not relevant then, the optional presence checks can happen programmatically on an always-injected handle. Design-wise, that's generally sensible since you do not have to rely on the ambiguous semantics of null.

Updated the test code.

And I do agree that Optional or ObjectProvider is better, my trouble is that this issue cames from some legacy injection code which a lot of codes depend on it and need to update to Optional<>.

@jhoeller jhoeller added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jul 4, 2023
@jhoeller jhoeller added this to the 6.0.11 milestone Jul 4, 2023
@jhoeller jhoeller added the for: backport-to-5.3.x Marks an issue as a candidate for backport to 5.3.x label Jul 4, 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 Jul 4, 2023
@jhoeller
Copy link
Contributor

jhoeller commented Jul 4, 2023

Addressed through revised caching now, being able to deal with a switch from null to non-null and also the other way round.

jhoeller added a commit that referenced this issue Jul 4, 2023
In addition to the previously addressed removal of bean definitions, this is able to deal with prototype factory methods returning non-null after null or also null after non-null. Stale cached values are getting refreshed rather than bypassed.

Closes gh-30794

(cherry picked from commit 0226580)
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
Projects
None yet
Development

No branches or pull requests

4 participants