Reset bean to defaults before rebinding values#1680
Reset bean to defaults before rebinding values#1680shanman190 wants to merge 2 commits intospring-cloud:mainfrom
Conversation
0d76586 to
286733d
Compare
Signed-off-by: Shannon Pamperl <shanman190@gmail.com>
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.
.../main/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinder.java
Outdated
Show resolved
Hide resolved
.../main/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinder.java
Outdated
Show resolved
Hide resolved
.../main/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinder.java
Show resolved
Hide resolved
| private Map<String, Object> findTestProperties() { | ||
| for (PropertySource<?> source : this.environment.getPropertySources()) { | ||
| if (source.getName().toLowerCase().contains("test")) { | ||
| @SuppressWarnings("unchecked") | ||
| Map<String, Object> map = (Map<String, Object>) source.getSource(); | ||
| return map; | ||
| } | ||
| } | ||
| throw new IllegalStateException("Could not find test property source"); | ||
| } |
There was a problem hiding this comment.
This lookup is brittle: it may match multiple property sources whose names contain "test" and it assumes getSource() is always a Map, which can cause mis-selection or ClassCastException depending on Boot/environment internals. Prefer selecting a specific known property source (by exact name) and/or verifying source.getSource() instanceof Map before casting (or use a MapPropertySource/EnumerablePropertySource check) to make the tests stable across Spring Boot versions.
|
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 <shanman190@gmail.com>
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.
| if (AopUtils.isAopProxy(bean)) { | ||
| Object target = ProxyUtils.getTargetObject(bean); | ||
| appContext.getAutowireCapableBeanFactory().destroyBean(target); | ||
| Object freshTarget = BeanUtils.instantiateClass(targetClass); | ||
| appContext.getAutowireCapableBeanFactory().initializeBean(freshTarget, name); | ||
| ((Advised) bean).setTargetSource(new SingletonTargetSource(freshTarget)); | ||
| } |
There was a problem hiding this comment.
In the AOP-proxy branch, the code unconditionally casts bean to Advised and replaces its TargetSource. AopUtils.isAopProxy(bean) does not guarantee the proxy implements Advised (e.g., opaque proxies), which can lead to a ClassCastException. Also, BeanUtils.instantiateClass(targetClass) can fail (no default ctor / non-instantiable target) and currently aborts the whole rebind path instead of handling the “cannot restore defaults” edge case gracefully. Consider guarding with instanceof Advised, and if a fresh instance cannot be created, log a warning and fall back to rebinding/resetting the existing target instance (or skip just this bean).
| Object freshTarget = BeanUtils.instantiateClass(targetClass); | ||
| appContext.getAutowireCapableBeanFactory().initializeBean(freshTarget, name); |
There was a problem hiding this comment.
When creating a new target for a proxied bean, the code uses instantiateClass(...) + initializeBean(...). initializeBean runs BeanPostProcessors/init callbacks but does not perform dependency injection/property population like a full bean creation, so a proxied @ConfigurationProperties bean with injected collaborators could end up partially initialized. Consider using AutowireCapableBeanFactory#createBean(targetClass) (or autowireBean + initializeBean) to ensure the fresh target is constructed and wired consistently with normal container semantics.
| Object freshTarget = BeanUtils.instantiateClass(targetClass); | |
| appContext.getAutowireCapableBeanFactory().initializeBean(freshTarget, name); | |
| Object freshTarget = appContext.getAutowireCapableBeanFactory().createBean(targetClass); |
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.