Reset bean to defaults before rebinding values#1680
Reset bean to defaults before rebinding values#1680ryanjbaxter merged 6 commits intospring-cloud:mainfrom
Conversation
shanman190
force-pushed
the
bugfix/null-refresh-property
branch
from
0d76586 to
286733d
Compare
Signed-off-by: Shannon Pamperl <[email protected]>
shanman190
force-pushed
the
bugfix/null-refresh-property
branch
from
286733d to
b4c685c
Compare
|
Does this address a specific issue? If so can you link it? If not can you describe the issue you are addressing in the PR description? |
|
Hey @ryanjbaxter, sorry about that! 😅 I had started to edit the description earlier and entirely forgot to hit save. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes ConfigurationPropertiesRebinder behavior so that when properties are removed during refresh/rebind, previously bound values don’t linger on existing bean instances.
Changes:
- Enable a previously disabled list rebinding integration test.
- Add new integration tests covering clearing/restore-to-default behavior on property removal.
- Reset bean state to class defaults prior to
initializeBean()during rebinding.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| spring-cloud-context/src/main/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinder.java | Add “reset to defaults” step before reinitializing a rebound bean |
| spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderListIntegrationTests.java | Enable testReplaceProperties by removing @Disabled |
| spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderClearIntegrationTests.java | Add coverage for clearing to null/primitive defaults and restoring field initializers when properties are removed |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
There is a related PR here for non proxy beans: #1662 It would be nice if this PR could also address the non proxy beans as well and solve this problem completely |
Signed-off-by: Shannon Pamperl <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
shanman190
force-pushed
the
bugfix/null-refresh-property
branch
from
939d71e to
e3fd27b
Compare
Signed-off-by: Shannon Pamperl <[email protected]>
shanman190
force-pushed
the
bugfix/null-refresh-property
branch
from
e3fd27b to
ba34b47
Compare
|
@ryanjbaxter , I think I've got all of the feedback addressed now. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Shannon Pamperl <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Shannon Pamperl <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Shannon Pamperl <[email protected]>
|
Thanks @ryanjbaxter for the reviews and the merge of this! |
Problem
I noticed that when a property is removed from the
Environmentwith a property refresh resulting in a specific property going from having a configured value to having no value, theConfigurationPropertiesRebindereffectively only calls the lifecycle methods on the bean, but doesn't reset the values. This results in@RefreshScopeannotated beans not clearing their configuration leaving potentially orphaned configuration from the prior state.Solution
Reset the bean to its default state as if it were going to be a new instance, so that when performing
initializeBean()on the new instance it is bound with a fresh state.I also covered the case where if restoring the bean to its initial state is impossible, the code will be able to gracefully handle that edge case.
Alternative
Since we're resetting the bean behind a JDK proxy, we could replace the internal value with the brand new instance rather than just a simple value reset.
Related
ConfigurationPropertiesRebinderListIntegrationTestsalso had a@Disabledtest --testReplaceProperties-- which illustrated this inability as well.