Spring-boot: Unexpected constructor-based initialization of @ConfigurationProperties leads to inconsistent behavior

[axtavt]

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.

[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.

[Raheela1024]

@wilkinsona I want to work on it. Please let me know if I can start working on it ?

[snicoll]

@Raheela1024 thank you for the offer. Please go ahead and let us know if you need assistance.

[mbhave]

@Raheela1024 How's it going? Let us know if there is anything we can help with.

[Raheela1024]

@mbhave Sorry i was busy but now looking into will update shortly.

[Raheela1024]

@mbhave I need some help regarding the fix should we need to revert

"Renamed logging.file to logging.file.name"

or we need to fix into ConfigurationPropertiesBinderbind method.

[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.

[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 ?

[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.

[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.