spring-projects / spring-framework

Spring Framework
https://spring.io/projects/spring-framework
Apache License 2.0
56.63k stars 38.13k forks source link

Allow to map null value from yaml [SPR-15425] #19986

Open spring-projects-issues opened 7 years ago

spring-projects-issues commented 7 years ago

Simon Stratmann opened SPR-15425 and commented

The following issue occured to me while using Spring Boot but is caused by Spring code.

With Spring Boot I have a String property that is mapped to an application.yml by using @ConfigurationProperties. The value in the yml file is null (or ~). But the values are initialized with "" (empty String). According to YAML specification it does allow null values.

The culprit is org.springframework.beans.factory.config.YamlProcessor#buildFlattenedMap which executes result.put(key, (value != null ? value : "")).

I've extended the file and configured it to be used and this seems to work fine. But I have to take over a lot of final and private code which makes it a bit brittle. I think the class should support null values, explicitly remark that their converted to "" or make a change in behavior a bit more comfortable.

Code excerpts:

test:
    setting: ~

@ConfigurationProperties("test")
public class Test {

    String setting;

    public void setSetting(String setting) {
        this.setting = setting; //Will set "setting" to "" instead of null
    }
}

Thanks for your time.


Affects: 4.3.7

Attachments:

Issue Links:

4 votes, 5 watchers

spring-projects-issues commented 6 years ago

Craig commented

https://github.com/spring-projects/spring-framework/pull/1798

spring-projects-issues commented 6 years ago

Stéphane Nicoll commented

I am not keen to support null values in the loader but I am ok to provide a hook point that allows you to do so if you want. 

We want things to be consistent in Spring Boot (and you can't set null in properties) so even if this was implemented as a default, we'd convert it back to empty string for consistency.

Can you review your PR to offer a hook point that allows you to define that strategy with an extension?

spring-projects-issues commented 6 years ago

Andy Wilkinson commented

FWIW, I disagree with Stéphane here. Looking at things purely from the perspective of YAML processing, I think that the current behaviour of YamlProcessor is wrong. null is a first-class citizen in YAML and Java supports null values, but YamlProcessor currently coerces a null to an empty String. Given that Java is capable of representing null values, I think YamlProcessor should support them rather than the current lossy conversion.

Unfortunately, supporting null values may be easier said than done. YamlProcessor.MatchCallback and YamlProcessor.DocumentMatcher both use Properties in public API and Properties does not support null values. It looks like API changes will be necessary to fully support null values. It may be possible for those changes to be non-breaking by judicious use of default methods on the relevant interfaces but changing this probably isn't as easy as it first appears.

 

 

spring-projects-issues commented 6 years ago

Stéphane Nicoll commented

Thanks Andy Wilkinson.

The purpose of my initial comment was largely targeted at the Spring Boot situation and I do agree that we should not push those considerations too restrictively. The proposed PR can't be merged as is anyway. 

Craig would you be willing to rework your PR with what has been considered here?

spring-projects-issues commented 6 years ago

Bruno Orsi Berton commented

I was working with this issue as well, I was trying to do the same solution as Craig and I came across the same problem of the Properties class not allowing null values, but my opinion is that a change in the code to replace the Properties class is not going to be feasible. Since this is one of my first issues in Spring, I am not familiar with the code, @Stéphane Nicoll, can you explain a little more your solution? I really wanna try to solve this issue, since I am new as a contributor.

spring-projects-issues commented 6 years ago

Stéphane Nicoll commented

Hey Bruno, thanks for asking. I did ask Craig if he wanted to rework his PR but he didn't reply so I am going to decline it. I don't have any solution at the moment I am afraid, I need to look in the API and see if there is a way to do this in a backward compatible way.

andrei-ivanov commented 4 years ago

any chance for a fix? 😕

andrei-ivanov commented 4 years ago

@snicoll any suggestions on how this could be implemented?

incodemode commented 3 years ago

So was this dismissed or fixed? just wanting to know, in my case, the solution is simple for my particular project if(!null|!empty). but reading that spring is the new hot topic, I was wondering if this was fixed at some point

sbrannen commented 3 years ago

@incodemode, as @wilkinsona hinted at in https://github.com/spring-projects/spring-boot/issues/4877#issuecomment-782233621, this issue is still open and waiting for triage (i.e., waiting for further consideration/investigation).

The original PR for this issue (#1798) was declined as explained in https://github.com/spring-projects/spring-framework/issues/19986#issuecomment-453453915 above.

snicoll commented 3 years ago

I think it is important to put this issue in context. While it is true that YamlProcessor is mapping null to an empty String too high in the hierarchy, we took that relaxed approach because Spring's Environment does not have the capability of storing that information at the moment anyway (null is not handled).

Fixing this bug, in the current state, would only help users that are using YamlProcessor directly. A user of, say Spring Boot, wouldn't see a single difference if we fixed it for the reason I've expressed above.

szpak commented 1 year ago

We've encountered that problem trying to resolve an environment property or null if not provided (to fail the validation with the Jakarta JSR303 annotations). As SpEL is not evaluated in @ConfigurationProperties, something like ${FOO_BAR:#{null}} in YAML doesn't work.

Having an ability to pass a null value (also as a default value) in YAML would probably solve also our issue (without any other post processors).

djchapm commented 1 year ago

This would also be helpful if you're the client of a spring boot app and trying to override one of the properties delivered with the app using -Dspring.config.additional-location to null out just that one entry that is set with a default value when you need it to be null.

Sroca3 commented 7 months ago

I'm a bit confused about the lack of ability to fix this. YamlProcessor is clearly dedicated to processing yaml files right? The implementation doesn't support parts of clearly defined yaml functionality. Whether or not that breaks additional callers of it is irrelevant, I would think. If Environment or Properties doesn't support null, then the extra data massaging should happen in a different place than the YamlProcessor.

Stefan4112 commented 3 months ago

My workaround is to remove (comment) the property, because then the value is null.

test:
    #setting: ~

Maybe it is a good idea to remove properties with null values before YamlProcessor#buildFlattenedMap is called? Then they would be null. I don't know if it is a problem, because it is not exactly the same.