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

Confusing and incomplete failure analysis when ConfigurationProperties uses constructor for beans #17750

Closed
snicoll opened this issue Aug 1, 2019 · 2 comments

Comments

@snicoll
Copy link
Member

commented Aug 1, 2019

Consider the following:

@ConfigurationProperties("example")
@Component
public class ExampleProperties {

   private String api;

   private final MyBean myBean

   public ExampleProperties(MyBean bean) { ... }

   // getter setter for api

}

This code is wrong in the first place as it does config binding and some logic with a bean but the upgrade to 2.2 can provide an annoying experience.

First a user would get this:

***************************
APPLICATION FAILED TO START
***************************

Description:

example.ExampleProperties is annotated with @ConfigurationProperties and @Component. This may cause the @ConfigurationProperties bean to be registered twice.

Action:

Remove @Component from example.ExampleProperties or consider disabling automatic @ConfigurationProperties scanning.

When they do that, they remove the actual nature of the component and since there is one constructor, the binding attempts to use it for binding. So it's now trying to bind example.bean and are completely ignore example.api.

Once @Component is removed, we get another failure

***************************
APPLICATION FAILED TO START
***************************

Description:

Failed to bind properties under 'example' to example.ExampleProperties:

    Reason: Failed to bind properties under 'example' to example.ExampleProperties:

Action:

Update your application's configuration

We should improve the first failure to add a note about the mix of the two stereotype. If there is a single constructor with at least one argument, we should warn it's going to be used by default for constructor binding.

It would be nice to see if the failure analysis of the second case can be improved as it's rather cryptic at the moment. This may deserve a separate issue.

@L00kian

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

@snicoll The example for the second case seems to be a bit incomplete.

@ConfigurationProperties(value = "example", ignoreUnknownFields = false)
public class ExampleProperties {

	private String api;

	private final MyBean myBean;

	public ExampleProperties(MyBean bean) { ... }

	// getter setter for api

}

without ignoreUnknownFields=false I didn't get any error for the 'api' field binding failure (which can be even more confusing as I would expect this field to be populated anyway)

when ignoreUnknownFields=false the error description looks a bit different and give at least some sense in what could have gone wrong (still confusing as there is an 'api' field but not as confusing as when field binding is silently ignored)

***************************
APPLICATION FAILED TO START
***************************

Description:

Binding to target [Bindable@2650f79 type = com.example.demo.ExampleProperties, value = 'none', annotations = array<Annotation>[@org.springframework.boot.context.properties.ConfigurationProperties(ignoreInvalidFields=false, ignoreUnknownFields=false, prefix=example, value=example)]] failed:

    Property: example.api
    Value: Hi, I am a value!
    Origin: class path resource [application.properties]:1:13
    Reason: The elements [example.api] were left unbound.

Action:

Update your application's configuration
@snicoll

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2019

@L00kian I didn't experience that but I know the failure analysis can be different when this flag is set. It probably requires a bit more work to account for the constructor argument use case. I'll probably move this to a separate issue once I am done with the first failure analysis. Thanks for the extra information!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.