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

ConstructorResolver will generate NPE in case of no factory method found when there are explicit args [SPR-11517] #16142

Closed
spring-projects-issues opened this issue Mar 5, 2014 · 9 comments
Assignees
Labels
in: core status: backported type: bug
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Mar 5, 2014

Joe Gamache opened SPR-11517 and commented

You can find this bug by inspection if you simply look at the code. So please do not turn around and ask me for a test case. In fact, there should be a Unit Test case added internally for this usage as I do not believe that there can be one given the code.

Look at org.springframwork.beans.factory.support.ConstructorResolver in the 'instantiateusingFactoryMethod'. If there are 'explicitArgs' all is fine until the if statement at line: 440:
if (explicitArgs != null)
In this usage, "resolvedValues" IS NEVER INITIALIZED!!!

This guarantees a NullPointerException on line 533:

boolean hasArgs = (resolvedValues.getArgumentCount() > 0)

The problem can also be stated in English: If explicitArgs are used they are never loaded into the resolvedValues data structure from which the code extracts them, thus this can not work.


Affects: 3.2.8, 4.0.2

Attachments:

Backported to: 3.2.9

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 5, 2014

Juergen Hoeller commented

Good catch that there will be an NPE in said line:

boolean hasArgs = (resolvedValues.getArgumentCount() > 0)

However, that line only executes when we fail to find a factory method. So instead of a proper BeanCreationException, we end up throwing an NPE.

That clearly is a bug but it's just minor, since it only leads to bad error reporting in case of a bean definition with an invalid factoryMethodName.

Juergen

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 5, 2014

Joe Gamache commented

That's not how I found it though. I came in via this constructor (trying to create a prototype bean with explicit args....):

/**
 * Return an instance, which may be shared or independent, of the specified bean.
 * <p>Allows for specifying explicit constructor arguments / factory method arguments,
 * overriding the specified default arguments (if any) in the bean definition.
 * @param name the name of the bean to retrieve
 * @param args arguments to use if creating a prototype using explicit arguments to a
 * static factory method. It is invalid to use a non-null args value in any other case.
 * @return an instance of the bean
 * @throws NoSuchBeanDefinitionException if there is no such bean definition
 * @throws BeanDefinitionStoreException if arguments have been given but
 * the affected bean isn't a prototype
 * @throws BeansException if the bean could not be created
 * @since 2.5
 */
Object getBean(String name, Object... args) throws BeansException;

If this is the case and that entire usage model doesn't work, wouldn't that be fairly major? (he asked hopefully ;) )

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 5, 2014

Juergen Hoeller commented

In the meantime, I've added explicit introspection of explicitArgs to that spot in ConstructorResolver, reflecting the arguments in the exception's message just like we do for the pre-resolved case.

As for your scenario, were you calling getBean for a factory method there, and did you potentially mis-specify that factory method - i.e. not matching the name or not matching the parameter types? In that case, you would have run into the NPE.

For scenarios with a matching factory method, we have rather comprehensive FactoryMethodTests in the test suite, so that model should work fine in general. I've added a few further tests for non-matching factory methods now, checking that we throw a meaningful BeanCreationException in that case.

Juergen

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 5, 2014

Juergen Hoeller commented

Resolving this for the time being, assuming that the exception-related fix addresses the underlying problem. This will be available in the next 4.0.3 snapshot, and in a 3.2.9 snapshot later this week.

Juergen

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 5, 2014

Joe Gamache commented

I am really sorry if I did wrong by trying to re-open this! I knew I should have bitten the proverbial bullet and did an example. But if you couldn't tell from my earlier remarks - I hate doing that, it really does take a lot of time!

So here is the usage I am trying via java config (this is from Spring 3 Demo code I hacked up to show you):

    @Bean
    @Scope(value = "prototype")
    public TransferService myFirstService(AccountRepository serviceBean, String param) {
        System.out.println("value of count:" + count++);
        return new TransferServiceImpl(serviceBean, param);
    }

    @Bean
    @Scope(value = "prototype")
    public TransferService mySecondService() {
        System.out.println("value of count:" + count++);
        return new TransferServiceWithFeesImpl(accountRepository());
    }

    @Bean
    public AccountRepository aSingletonBean() {
        System.out.println("value of count:" + count++);
        return new InMemoryAccountRepository();
    }

When I try to test this with this test:

@Test
public void prototypeTest() {
    // create the spring container using the ServiceConfig @Configuration class
    ApplicationContext ctx = new AnnotationConfigApplicationContext(ServiceConfig.class);
    Object singleton = ctx.getBean("aSingletonBean");
    System.out.println(singleton.toString());
    singleton = ctx.getBean("aSingletonBean");
    System.out.println(singleton.toString());
    TransferService transferService = (TransferService) ctx.getBean("myFirstService", TransferService.class, "simulated Dynamic Parameter One");
    System.out.println(transferService.toString());
    transferService = (TransferService) ctx.getBean("myFirstService", TransferService.class, "simulated Dynamic Parameter Two");
    System.out.println(transferService.toString());
}

Then the NPE shows up. The problem then is that I cannot create a 'prototype' bean, via Java Config, that takes parameters. So am I doing something wrong or is it a bug? I have attached all the source as a zip file with a gradle build script if you need it.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 5, 2014

Joe Gamache commented

per comment above.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 5, 2014

Juergen Hoeller commented

From a quick glance, it seems that your myFirstService factory method declares two parameters but you're only specifying one argument in your getBean call in the test. Note that, if you're specifying parameters, they need to be complete; you can't selectively specify a single parameter. So you'll need to provide that AccountRepository reference in some other way; maybe through an accountRepository() call like in the mySecondService factory method?

In any case, that's exactly the sort of problem that will lead into an NPE here since ConstructorResolver can't find a factory method with matching parameters. Once the getBean arguments and the target parameters actually match, it should work fine.

Juergen

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 5, 2014

Joe Gamache commented

Yes you are correct. If I take out that parameter and pass it in indirectly by calling the accountRespository() method (which IS how it should be done anyway!), everything works fine! Thanks for your help!

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 5, 2014

Juergen Hoeller commented

No problem - thanks for finding that NPE, even if it is 'just' in our error reporting!

Juergen

@spring-projects-issues spring-projects-issues added type: bug status: backported in: core labels Jan 11, 2019
@spring-projects-issues spring-projects-issues added this to the 4.0.3 milestone Jan 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core status: backported type: bug
Projects
None yet
Development

No branches or pull requests

2 participants