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 Beans are not Re-bound on Context Refresh #1372

Closed
AdrianDiemerDev opened this issue Aug 21, 2024 · 11 comments
Closed
Milestone

Comments

@AdrianDiemerDev
Copy link

AdrianDiemerDev commented Aug 21, 2024

The documentation states that @ConfigurationProperties are supposed to be re-bound on environment changes.

As far as I can tell, that's not what's happening, though. Since updates to ConfigurationProperties seem to only be working when using setters (#1547), I think the ConfigurationProperties aren't actually re-bound but only modified which is different from the way the documentation describes it as well as how other beans are handled.

Changing the behavior from modifying to rebinding would also resolve the issue mentioned further down in the documentation that deleted properties are not going to be updated in the ConfigurationProperties beans.

Is this a change that would be accepted as a contribution to the project if we came up with a solution for it?

  • Spring Version: 6.1.11
  • Spring Boot Version: 3.2.8
  • Spring Cloud Version: 2023.0.2
@ryanjbaxter
Copy link
Contributor

Rebinding should be working. If you can provide a sample of when they are not being rebound that would be useful

@AdrianDiemerDev
Copy link
Author

I've created a minimal demo project here: https://github.com/AdrianDiemerDev/SpringCloudPropertiesRebindingDemo

When starting the application, two endpoints will be served:

  • GET http://localhost:8080/constructor-binding
  • GET http://localhost:8080/setter-binding
    exposing the values bound with the respective methods.

The initial value is given in the application.yaml file: 1

Both endpoints will return exactly this value.

If you change that value in the application.yaml to e.g. 2 and make a call to POST http://localhost:8080/actuator/refresh, the value returned by the first endpoint will be unchanged, while the value returned by the second endpoint will indeed be 2.

If you then remove the configuration from the application.yaml completely and execute another refresh, the first endpoint will still return 1 while the second endpoint will again return 2.

This behavior suggests to me that @ConfigurationProperties aren't actually re-bound on environment changes but are instead modified using their setters. Which, as mentioned in my original post, doesn't match what the documentation says and also seems to be different behavior from all other beans.

@ryanjbaxter
Copy link
Contributor

This is as expected. Removing the property will from the configuration source, will not remove the property from the environment. If I remember correctly Boot is just looking for the property and updating its value, but since the property does not exist is leaves the value as is.

However to me this still is effectively rebinding the properties with the updated values.

@AdrianDiemerDev
Copy link
Author

this still is effectively rebinding the properties with the updated values

I guess that's a philosophical question.

The effect, though, is that constructor bound properties or records (which seem to be the intended way to do configuration properties currently) are not supported by the refresh functionality. This seems kind of broken to me.

The fact that deleted properties are not updated is a different point in my opinion. That's really just a consequence of the refresh method. If the configuration properties objects were completely reinitialized (like in my interpretation of the documentation), this issue would go away as well, I think.

What do you think?

@ryanjbaxter
Copy link
Contributor

Ah ok. Could you try adding @ConstructorBinding to the record?

@AdrianDiemerDev
Copy link
Author

I've added record ConfigurationProperties to the demo project in order to demonstrate the issue there as well.

I don't seem to be able to add the @ConstructorBinding annotation to a record as it's restricted to constructors as targets.

@ryanjbaxter
Copy link
Contributor

I think you would have to do something like this

@ConfigurationProperties("demo")
@ConstructorBinding
public record MyConfigurationProperties(
        String myProperty
) {
}

@AdrianDiemerDev
Copy link
Author

When I do it like that the compiler says:

annotation type not applicable to this kind of declaration

@ryanjbaxter ryanjbaxter transferred this issue from spring-cloud/spring-cloud-config Aug 28, 2024
@ryanjbaxter ryanjbaxter added this to the 4.1.5 milestone Aug 28, 2024
@ryanjbaxter
Copy link
Contributor

My mistake, I thought this should be supported but it looks as well it is not. I have added a note to our docs stating such

@AdrianDiemerDev
Copy link
Author

Your note in the documentation only covers records but classes that use constructor binding are affected as well.

Also, I don't feel like a note in the documentation fixes the issue. In my opinion, this is a fundamental flaw in the refresh mechanism. As you can see in the table below, only 2 out of 9 scenarios are supported by the refresh:

Constructor-Bound Class Record Setter-Bound Class
Create Property no no yes
Update Property no no yes
Delete Property no no no

All of this could be fixed by actually reinitializing the object (just like any other bean) instead of relying on setters.

My original question was whether such a change would be accepted as a contribution since there seem to be some maintainers that feel that this (in my opionion broken) behavior is by design.

@ryanjbaxter
Copy link
Contributor

We would certainly take a look at a PR that would support this can consider the enhancement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

No branches or pull requests

3 participants