fix: resolve config refresh failure with spring.config.import on 2025.0.x - #4335
fix: resolve config refresh failure with spring.config.import on 2025.0.x#4335wushiyuanmaimob wants to merge 4 commits into
Conversation
Verify two defects in NacosPropertySourceRefreshListener: 1. PropertySource name mismatch between ConfigData path (group@dataId) and bootstrap path (dataId,group) 2. File extension hardcoded as 'properties' instead of using actual suffix (yml/json/xml) Related to alibaba#4331 Signed-off-by: wushiyuan <wushiyuanwork@outlook.com>
….0.x Fix two defects in NacosPropertySourceRefreshListener that prevented dynamic config refresh when using spring.config.import=nacos: 1. PropertySource name mismatch: ConfigData path uses 'group@dataId' naming while RefreshListener looked for 'dataId,group'. Now tries both naming conventions. 2. File extension hardcoded: RefreshListener always used 'properties' extension, breaking yml/json/xml configs. Now reads actual suffix from NacosPropertySource. Changes: - Add suffix field to NacosPropertySource to preserve file extension - Update NacosPropertySourceBuilder and NacosConfigDataLoader to pass suffix - Update NacosPropertySourceRefreshListener to handle both naming formats and use actual file extension Fixes alibaba#4331 Signed-off-by: wushiyuan <wushiyuanwork@outlook.com>
The tests were adding plain MapPropertySource to propertySources, but the production code checks for NacosPropertySource type. This caused the refresh logic to never execute, resulting in test failures where values weren't updated. Fixed by: 1. Adding NacosPropertySource wrapper to propertySources instead of inner MapPropertySource 2. Specifying "yml" suffix in NacosPropertySource constructor to match test data Signed-off-by: sywu14 <wushiyuanwork@outlook.com>
58c013c to
05418be
Compare
… tests - YAML parser returns Integer for numeric values, use String.valueOf() - After replace, new NacosPropertySource uses standard dataId,group naming
|
Will the current PR see any further progress? |
| NacosPropertySourceBuilder nacosPropertySourceBuilder = new NacosPropertySourceBuilder(nacosConfigManager.getConfigService(), nacosConfigManager.getNacosConfigProperties() | ||
| .getTimeout()); | ||
| String sourceName = String.join(NacosConfigProperties.COMMAS, event.dataId, event.group); | ||
|
|
There was a problem hiding this comment.
Based on the fix in #4341, it looks like the naming mismatch issue should no longer occur.
oss-sentinel-ai
left a comment
There was a problem hiding this comment.
Summary
This PR fixes two real defects in the config refresh path when using spring.config.import=nacos:: (1) PropertySource name mismatch between ConfigData (group@dataId) and bootstrap (dataId,group) naming conventions, and (2) hardcoded "properties" suffix during refresh. The dual-name lookup approach is pragmatic. However, there are a few concerns worth addressing before merge.
Findings
- [Critical]
NacosPropertySourceRefreshListener.java:125— When the ConfigData path is taken,sourceNameis set toconfigDataName(group@dataId), but the rebuiltNacosPropertySourcefromnacosPropertySourceBuilder.build()constructs its internal name asdataId,group(bootstrap convention). Aftertarget.replace(sourceName, newProperSource), the map key isgroup@dataIdbut the object'sgetName()returnsdataId,group. This inconsistency can cause subtle bugs in downstream code that relies onNacosPropertySource.getName()matching its key in thePropertySourcesmap. Consider passing the originalsourceNameinto the builder or using a constructor that accepts an explicit name. - [Warning] This PR and #4349 both fix the same hardcoded-suffix bug with different approaches (immutable constructor param vs. mutable setter). They will conflict if both are merged. Please coordinate with the other PR author to align on a single approach.
- [Info] Test file path
src/test/java/com.alibaba.cloud.nacos/refresh/uses dots instead of slashes for the package directory. This may cause test discovery issues depending on the build configuration.
Suggestions
// In NacosPropertySourceRefreshListener, when rebuilding for ConfigData path:
// Ensure the rebuilt source name matches the map key
String sourceName = bootstrapName;
NacosPropertySource existing = (NacosPropertySource) target.get(sourceName);
if (existing == null) {
sourceName = configDataName;
existing = (NacosPropertySource) target.get(sourceName);
}
// existing.getSuffix() now carries the correct extension — good.
// But verify the rebuilt source name matches sourceName.Automated review by github-manager-bot
|
Thanks for the contribution. I rechecked the current
Please rebase this PR onto the latest The #4349 commit cannot be cherry-picked into
After these adjustments, please make sure the tests cover yml, json, the null-suffix fallback, and consecutive refreshes. We can continue the review once the PR has been updated. |
oss-sentinel-ai
left a comment
There was a problem hiding this comment.
Thanks @uuuyuqi for the detailed guidance. Updating my earlier review in light of it:
- [Warning] overlap with #4349 — resolved: the maintainers decided this PR should be revised into a backport of #4349, so no coordination between competing approaches is needed anymore.
- [Critical] rebuilt source name mismatch — superseded: since #4346 already landed the ConfigData replacement fix on
2025.0.x(merged 2026-06-21), the dualdataId,group/group@dataIdlookup in this PR is no longer needed once rebased. - [Info] test directory path — still applies:
src/test/java/com.alibaba.cloud.nacos/refresh/uses dots instead of slashes; if the test is carried over into the backport, please usesrc/test/java/com/alibaba/cloud/nacos/refresh/so test discovery works reliably.
Remaining work per the maintainer's request: rebase onto the latest 2025.0.x and revise this PR into a backport of #4349 (merged on 2025.1.x 2026-07-31) — preserve the original suffix on each replacement NacosPropertySource, retain the consecutive-refresh regression test, adapt for the missing namespace constructor argument and the absent JSpecify @Nullable dependency, follow the 2025.0.x Checkstyle copyright headers, and skip the #4319 snapshot regression test. Test coverage should include yml, json, the null-suffix fallback, and consecutive refreshes.
Happy to re-review once the PR is updated.
Automated review by github-manager-bot
Describe what this PR does / why we need it
Fix two defects in
NacosPropertySourceRefreshListenerthat prevented dynamic config refresh when usingspring.config.import=nacos:on the 2025.0.x branch.Defect 1: PropertySource name mismatch
group@dataIdnaming (NacosConfigDataLoader.java:146)dataId,group(NacosPropertySourceRefreshListener.java:103)target.get(sourceName)always returned null, so refresh never happenedDefect 2: File extension hardcoded
"properties"as file extension (NacosPropertySourceRefreshListener.java:108)NacosItemConfig.suffixbut not propagatedDoes this pull request fix one issue?
Fixes #4331
Describe how you did it
suffixfield toNacosPropertySourceto preserve file extensionNacosPropertySourceBuilderandNacosConfigDataLoaderto pass suffix when creatingNacosPropertySourceNacosPropertySourceRefreshListenerto:dataId,groupand ConfigDatagroup@dataId)NacosPropertySource.getSuffix()instead of hardcoding"properties"Describe how to verify it
spring.config.import=nacos:test-config.ymlRegression tests added in
NacosPropertySourceRefreshListenerTest.Special notes for reviews