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

@ConfigurationProperties @ConstructorBinding sets missing Optional to null #21795

Closed
candrews opened this issue Jun 9, 2020 · 3 comments
Closed
Assignees
Labels
status: invalid An issue that we don't feel is valid

Comments

@candrews
Copy link
Contributor

candrews commented Jun 9, 2020

With immutable configuration properties as described at https://docs.spring.io/spring-boot/docs/current/reference/html/spring-boot-features.html#boot-features-external-config-constructor-binding Optional doesn't work as expected.

Using Spring Boot 2.3.0:

@ConfigurationProperties(prefix = "monitor")
@ConstructorBinding
public class MonitorConfiguration {
	private final String environmentName;
	private final Optional<String> managerUrl;
	public MonitorConfiguration(final String environmentName,
			final Optional<String> managerUrl) {
		this.environmentName = environmentName;
		this.managerUrl = managerUrl;
	}
}

Using application.properties:

monitor.environment-name=test

Expected:
monitorConfiguration.managerUrl should be Optional.EMPTY

This behavior would match the non-immutable ConfigurationProperties behavior described in #5110

Actual:
monitorConfiguration.managerUrl is null

Note that #18917 says this issue should have already been fixed in Spring Boot 2.3.0, but that's not what I'm seeing.

If I add @DefaultValue to the Optional<String> managerUrl parameter in the contructor, then I get the expected behavior - but IMHO, the @DefaultValue should not be necessary.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 9, 2020
@philwebb philwebb added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 9, 2020
@philwebb philwebb added this to the 2.3.x milestone Jun 9, 2020
@philwebb philwebb added for: team-attention An issue we'd like other members of the team to review status: waiting-for-triage An issue we've not yet triaged and removed type: bug A general bug labels Jun 9, 2020
@philwebb philwebb removed this from the 2.3.x milestone Jun 10, 2020
@philwebb philwebb self-assigned this Jun 10, 2020
@philwebb philwebb added status: waiting-for-triage An issue we've not yet triaged and removed for: team-attention An issue we'd like other members of the team to review status: waiting-for-triage An issue we've not yet triaged labels Jun 10, 2020
@philwebb
Copy link
Member

The current behavior of injecting null for the optional unless @DefaultValue is used is consistent with other parameter types. Although it's highly unlikely that you want a null Optional, we think that it's best that we keep things consistent.

The use of Optional in both fields and constructor arguments isn't something we recommend. Although it works, we generally think it's a bit of an anti-pattern. I've opened #21868 so that we can add a note about this to the reference documentation.

@philwebb philwebb added status: invalid An issue that we don't feel is valid and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 10, 2020
@dreis2211
Copy link
Contributor

I think IntelliJ even warns you about Optional fields. It's considered an antipattern by many and wasn't the design goal. Brian Goetz once stated:

Of course, people will do what they want. But we did have a clear intention when adding this feature, and it was not to be a general purpose Maybe or Some type, as much as many people would have liked us to do so. Our intention was to provide a limited mechanism for library method return types where there needed to be a clear way to represent "no result", and using null for such was overwhelmingly likely to cause errors.

So it's mainly about having a more expressive way for returns with "no result". Since Optional isn't Serializable there is also the problem that it shouldn't be used in bean like classes.

@dreis2211
Copy link
Contributor

https://www.youtube.com/watch?v=Ej0sss6cq14 is a good talk by Stuart Marks about Optional in general.
Particularly, the sections starting at minutes 40:40 & 54:03

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: invalid An issue that we don't feel is valid
Projects
None yet
Development

No branches or pull requests

4 participants