Consider the following test case:
@RunWith(SpringRunner.class)
@SpringBootTest(classes = { BindingTest.Config.class })
@EnableConfigurationProperties
@TestPropertySource
public class BindingTest {
@Autowired FooProperties fooProperties;
@Test
public void shouldInjectBar() {
assertThat(fooProperties.getBar()).isEqualTo("baz");
}
@Configuration
public static class Config {
@Bean
public FooProperties fooProperties() {
return new FooProperties();
}
}
@ConfigurationProperties("foo")
@Validated
public static class FooProperties {
@NotNull @Getter @Setter
private String bar;
public FooProperties() {}
public FooProperties(String bar) { // (1)
this.bar = bar;
}
}
}
BindingTest.properties
:
foo.bar=baz
foo=unrelated # (2)
I expected fooProperties.bar
to be populated, or, at least, @NotNull
validation error to be thrown.
However, in presence of single argument constructor (1) and conflicting property (2) (it may be, for example, a totally unrelated system environment variable), I got the following:
* fooProperties.bar
is null
* no validation error is thrown
This happens because Binder
decides to use ObjectToObjectConverter
to initialize FooProperties
using its single argument constructor (despite the fact that FooProperties
is explicitly created in @Bean
method), creates new instance of FooProperties
and applies validation to it, but ConfigurationPropertiesBindingPostProcessor
ignores that new instance.
This applies to Spring Boot 2.0.x, 2.1.x, 2.2.x.
Comment From: wilkinsona
Thanks for the test case. For our future reference, here's a slightly modified version that is standalone and doesn't rely on Lombok:
package com.example.demo;
import static org.assertj.core.api.Assertions.assertThat;
import javax.validation.constraints.NotNull;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.context.properties.ConfigurationProperties;
import org.springframework.boot.context.properties.EnableConfigurationProperties;
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.test.context.TestPropertySource;
import org.springframework.test.context.junit4.SpringRunner;
import org.springframework.validation.annotation.Validated;
@RunWith(SpringRunner.class)
@SpringBootTest(classes = { BindingTest.Config.class })
@EnableConfigurationProperties
@TestPropertySource(properties = {"foo.bar=baz", "foo=unrelated"})
public class BindingTest {
@Autowired
FooProperties fooProperties;
@Test
public void shouldInjectBar() {
assertThat(fooProperties.getBar()).isEqualTo("baz");
}
@Configuration
public static class Config {
@Bean
public FooProperties fooProperties() {
return new FooProperties();
}
}
@ConfigurationProperties("foo")
@Validated
public static class FooProperties {
@NotNull
private String bar;
public FooProperties() {}
public FooProperties(String bar) { // (1)
this.bar = bar;
}
public String getBar() {
return bar;
}
public void setBar(String bar) {
this.bar = bar;
}
}
}
A few comments about the example:
It's unusual for a @ConfigurationProperties
to have a constructor. They're generally expected to be JavaBeans with a default constructor and getters and setters. What purpose is the constructor serving?
Having both foo
and foo.bar
as properties will cause problems with YAML as the following cannot be parsed:
foo: unrelated
bar: baz
We've made this mistake ourselves in the past and intend to correct it in 2.2. See https://github.com/spring-projects/spring-boot/issues/12510 for some details.
With that said, I think it makes sense for the binder to ignore a property named foo
when binding to something that uses foo
as its prefix. We should probably only bind properties that are descendants of foo
and ignore foo
itself.
Comment From: Raheela1024
@wilkinsona I want to work on it. Please let me know if I can start working on it ?
Comment From: snicoll
@Raheela1024 thank you for the offer. Please go ahead and let us know if you need assistance.
Comment From: mbhave
@Raheela1024 How's it going? Let us know if there is anything we can help with.
Comment From: Raheela1024
@mbhave Sorry i was busy but now looking into will update shortly.
Comment From: Raheela1024
@mbhave I need some help regarding the fix should we need to revert
"Renamed
logging.file
tologging.file.name
"
or we need to fix into ConfigurationPropertiesBinder
bind method.
Comment From: snicoll
@Raheela1024 I'd like to help but I don't see the link. We should not revert the change, it's part of an ongoing effort that a property name should not match the name of a group. Can you please expand a bit of what the problem is? You can share what you have on your fork.
Comment From: Raheela1024
@snicoll thanks for your reply. I have debugged the issue by changing test case into ConfigurationPropertiesAutoConfigurationTests
file by setting environment as"foo:"
and observed that in Binder class containsNoDescendantOf
method returns true while I am not providing Descendant fields. Should I need to update this method i am in right direction or not ?
Comment From: mbhave
@Raheela1024 As @wilkinsona mentioned here, we want to ignore any property that matches the prefix of the @ConfigurationProperties
bean that we're binding to. For example, we want to bind foo.*
to something with a foo
prefix, but to ignore a foo
property if one is set. This would be a change in the Binder
class. It looks like the change is a bit more involved than we originally thought as quite a few tests would need to be updated. We really appreciate your offer to help but we'd like to tackle this one ourselves.
Comment From: mbhave
We probably need to consider doing something so that this change only affects @ConfigurationProperties
. A change in the binder looks like it would be a fundamental change to the binder's behavior that involves a lot of test changes.
Comment From: philwebb
I've got a fix for this, but it includes API changes so I'm only going to apply it to 2.5
Comment From: philwebb
Reopening to consider nested properties
Comment From: mbhave
Example for nested properties
public class JavaBeanWithPublicConstructor {
private String value;
private Bar bar = new Bar();
public JavaBeanWithPublicConstructor() {
}
public JavaBeanWithPublicConstructor(String value) {
setValue(value);
}
public String getValue() {
return this.value;
}
public void setValue(String value) {
this.value = value;
}
public Bar getBar() {
return bar;
}
public void setBar(Bar bar) {
this.bar = bar;
}
static class Bar {
private String baz;
public Bar() {
}
public Bar(String baz) {
this.baz = baz;
}
public String getBaz() {
return baz;
}
public void setBaz(String baz) {
this.baz = baz;
}
}
}
Comment From: philwebb
We'll look at nested ones in #25368