Conversation
ContextRefresher used reference equality for property changes, so ConfigTreePropertySource values were always reported as changed. Fixes spring-cloudgh-1687 Signed-off-by: arimu1 <19286898+arimu1@users.noreply.github.com>
| if (Objects.equals(one, two)) { | ||
| return true; | ||
| } | ||
| if (one instanceof ConfigTreePropertySource.Value oneValue |
There was a problem hiding this comment.
I think we need to be a little more careful here since we are dealing with files on K8S and those files can be updated/removed between the time the refresh occurs and the comparison happens. If one or both of the files is gone when the comparison happens an IllegalStateException may occur so we should wrap this in a try/catch to deal with that case and return false. We should probably add a test to cover this if possible.
| } | ||
| if (one instanceof ConfigTreePropertySource.Value oneValue | ||
| && two instanceof ConfigTreePropertySource.Value twoValue) { | ||
| return oneValue.toString().contentEquals(twoValue); |
There was a problem hiding this comment.
I think we might want to compare bytes rather than use toString() since some of these values may be binary data (certs/keystores)
Use byte-level comparison for ConfigTreePropertySource values so binary mounts (certs/keystores) are handled correctly. Treat missing or unreadable files during comparison as a change instead of failing refresh. Signed-off-by: arimu1 <19286898+arimu1@users.noreply.github.com>
1daf0f4 to
e57b4f7
Compare
|
Thanks for the review @ryanjbaxter — addressed both points in e57b4f7. Missing K8s files during comparison Added Byte comparison (binary mounts) Added Tests run locally CI note (run 34726626743) |
ryanjbaxter
left a comment
ryanjbaxter
left a comment
There was a problem hiding this comment.
Could you submit this against the 5.0.x branch?
This branch has not been deployed
3 participants
Summary
ContextRefresherwhen the environment uses Spring BootConfigTreePropertySource.ConfigTreePropertySource.Value(includingPropertyFileContent) does not implementequals/hashCode, so every refresh incorrectly reported all config tree keys as changed.CharSequence/toString()instead of reference equality.Fixes gh-1687
Test plan
./mvnw -pl spring-cloud-context -am testContextRefresherConfigTreeTestscovering unchanged config tree refresh (no spurious keys) and changed content (key reported).