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

Constructor-based configuration property binding fails silently when parameter name information is not available #16928

Closed
thekalinga opened this issue May 21, 2019 · 11 comments

Comments

Projects
None yet
5 participants
@thekalinga
Copy link

commented May 21, 2019

I tried using @ConfigurationProperties with an immutable class, but the object has all values set to default null values

I used 2.2.0-BUILD-SNAPSHOT version to test this

Please find the test project in the attachment

spring-boot-playground.zip

@wilkinsona

This comment has been minimized.

Copy link
Member

commented May 21, 2019

Thanks for the sample. Unfortunately, I haven't been able to reproduce the behaviour that you have described. Here's the output from running ./gradlew bootRun:

[$]: ./gradlew bootRun

> Task :bootRun

  .   ____          _            __ _ _
 /\\ / ___'_ __ _ _(_)_ __  __ _ \ \ \ \
( ( )\___ | '_ | '_| | '_ \/ _` | \ \ \ \
 \\/  ___)| |_)| | | | | || (_| |  ) ) ) )
  '  |____| .__|_| |_|_| |_\__, | / / / /
 =========|_|==============|___/=/_/_/_/
 :: Spring Boot ::  (v2.2.0.BUILD-SNAPSHOT)

2019-05-21 14:24:36.138  INFO 99177 --- [           main] com.acme.playground.DemoApplication      : Starting DemoApplication on Andys-MacBook-Pro.local with PID 99177 (/Users/awilkinson/Downloads/spring-boot-playground/build/classes/java/main started by awilkinson in /Users/awilkinson/Downloads/spring-boot-playground)
2019-05-21 14:24:36.140  INFO 99177 --- [           main] com.acme.playground.DemoApplication      : No active profile set, falling back to default profiles: default
2019-05-21 14:24:36.551  INFO 99177 --- [           main] com.acme.playground.DemoApplication      : Config: ExternalSystemConfig(username=alpha, password=password, timeoutInSecs=2)
2019-05-21 14:24:36.636  INFO 99177 --- [           main] com.acme.playground.DemoApplication      : Started DemoApplication in 0.781 seconds (JVM running for 1.057)

As you can see, username, password, and timeoutInSecs are all non-null.

Can you please describe the steps that must be taken with the sample to reproduce the behaviour that you have described?

@allanneves

This comment has been minimized.

Copy link

commented May 21, 2019

Hi @thekalinga, @wilkinsona
I opened the project in IntelliJ IDEA and was able to replicate it. I do not know what causes the binding to fail in IntelliJ but it may be related to Lombok and Annotation processing. The app ran perfectly on the terminal in all attempts:

image
However, when I switched to IntelliJ it kept failing. I made that work in IntelliJ using Lombok and enabling Annotation Processing only after commenting out the @builder and @AllArgsConstructor annotations and adding a @Setter to it:

image
Please check if that is your case. I hope that helps.

Regards,
Allan

@thekalinga

This comment has been minimized.

Copy link
Author

commented May 22, 2019

@allanneves @wilkinsona Thanks for these two observations

Yes.. Looks like the issue has something to do with Intellij. I went to intellij settings & delegated build actions to gradle & the application works as expected. If I uncheck this box & run the app with intellij, then issue is consistently reproducible

@thekalinga

This comment has been minimized.

Copy link
Author

commented May 22, 2019

This is the setting I am referring to

Delegate to gradle

@thekalinga thekalinga closed this May 22, 2019

@thekalinga

This comment has been minimized.

Copy link
Author

commented May 22, 2019

Raised an issue against intellij

https://youtrack.jetbrains.com/issue/IDEA-214730

@philwebb philwebb reopened this May 22, 2019

@philwebb

This comment has been minimized.

Copy link
Member

commented May 22, 2019

I'm going to reopen this just to check that there's not a work-around that we can put in place.

@wilkinsona

This comment has been minimized.

Copy link
Member

commented May 22, 2019

Thank you, @allanneves and @thekalinga. I've reproduced the problem in IntelliJ IDEA.

The cause is the names of the constructor parameters which are arg0, arg1, and arg2. Adding -parameters as an additional command line parameter to the compiler configuration fixes the problem. It does not occur when building with Gradle as Spring Boot's Gradle plugin automatically adds -parameters to all CompileJava tasks. This configuration isn't picked up by the IDE when importing the project, hence the problem being specific to compiling and running in the IDE.

We should see if we can improve the diagnostics in the ConstructorParametersBinder to detect when parameter name information is not available. We should also improve the documentation to note the need for -parameters as we already do for endpoint operations.

@wilkinsona wilkinsona changed the title @ConfigurationProperties is not working for immutable classes Constructor-based configuration property binding fails silently when parameter name information is not available May 22, 2019

@wilkinsona wilkinsona modified the milestones: 2.2.x, 2.2.0.M4 May 22, 2019

@wilkinsona

This comment has been minimized.

Copy link
Member

commented May 28, 2019

I've switched to using DefaultParameterNameDiscoverer which aligns with parameter name discovery for actuator endpoint operations. This means that -parameters won't strictly be necessary as it delegates to LocalVariableTableParameterNameDiscoverer which will read debug information from the .class file if the names are not available via standard reflection.

@thekalinga

This comment has been minimized.

Copy link
Author

commented May 28, 2019

@wilkinsona Does this work with @ConstructorProperties({....})?

@wilkinsona

This comment has been minimized.

Copy link
Member

commented May 28, 2019

No, I don't believe it does. If that's something that you think Framework's parameter name discovery should take into consideration, please open a Framework issue.

@thekalinga

This comment has been minimized.

Copy link
Author

commented May 29, 2019

@wilkinsona Will open an issue. Thanks

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