fix: preserve Nacos config file extension across refresh#4312
Open
daguimu wants to merge 2 commits into
Open
Conversation
When a Nacos config update is received, NacosPropertySourceRefreshListener rebuilt the affected NacosPropertySource with a hard-coded "properties" file extension, so yaml/yml/json/xml configs were re-parsed as properties and silently produced broken key/value pairs after each push. - Carry the original file extension on NacosPropertySource via a new constructor overload and expose it through a getter. - NacosPropertySourceBuilder and NacosConfigDataLoader populate the extension when they create the source (using the dataId suffix / builder argument that was already in scope). - The refresh listener now reads the preserved extension from the previous NacosPropertySource and falls back to NacosConfigProperties.getFileExtension(), then to "properties", so behavior for historical sources built via the legacy constructor stays unchanged. Addresses alibaba#4296
Address review feedback on the file-extension-preserving refresh fix: - Make NacosPropertySource#fileExtension final and route every constructor through a single all-args constructor so the field is safely published to the ApplicationEvent dispatch thread that reads it on refresh. - Drop the intermediate fallback to NacosConfigProperties#getFileExtension in the refresh listener: it could re-introduce the same bug for shared-configs / extension-configs that declare a per-entry suffix but run under a NacosConfigProperties whose main file-extension defaults to "properties". Fall through directly to "properties" (historical default) and log a warning so the case is diagnosable. - Use StringUtils#hasText instead of isEmpty so blank values are handled. - Emit the effective fileExtension in the replace log for easier debugging. - Add NacosPropertySourceRefreshListenerTest covering: yaml and json sources keep their format through refresh, a legacy source without extension falls back to "properties", and a missing dataId skips the refresh without calling the backing ConfigService.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When a Nacos config change is pushed to the client,
NacosPropertySourceRefreshListener#handle(NacosConfigRefreshEvent)rebuilds the affectedNacosPropertySourceby calling:The third argument — the file extension — is hard-coded as
"properties".NacosDataParserHandler#parseNacosDatathen selectsPropertiesPropertySourceLoaderregardless of the user's actual format (yaml, yml, json, xml). So after the first refresh, a yaml config likeis re-parsed as a properties file and produces a single bogus key
server:\n portwith value8080, replacing the correctly-parsed yaml source that was installed at startup.Root Cause
NacosPropertySourcenever remembers the extension it was created with. At refresh time the listener therefore has no way to recover the original format and defaults to"properties". This affects both the mainspring.cloud.nacos.config.file-extensionas well asextension-configs/shared-configsthat may declare their own suffix per entry.Fix
fileExtensionfield toNacosPropertySourcewith a new public constructor overload and agetFileExtension()accessor. The existing constructors stay for backward compatibility.NacosPropertySourceBuilder#buildalready receives the file extension from its callers, so it now passes it into the new constructor.NacosConfigDataLoader#doLoadpropagatesNacosItemConfig#getSuffix(), which is already the per-entry suffix, soextension-configs/shared-configswith non-default formats are also preserved.NacosPropertySourceRefreshListener#handlereads the preserved extension from the previousNacosPropertySource, falls back toNacosConfigProperties#getFileExtension()(useful when the source was built via the legacy constructor from third-party code), and finally keeps the historical"properties"default so nothing regresses.Tests Added
fileExtensionunsetNacosPropertySourceTest#fileExtensionIsNullWhenUsingLegacyConstructorNacosPropertySourceTest#fileExtensionIsPreservedWhenProvided,otherPropertiesAreNotAffectedByNewConstructornullwithout side effectsNacosPropertySourceTest#fileExtensionCanBeNullExplicitlyBuilder#buildpropagates the yaml extension into the resultingNacosPropertySourceNacosPropertySourceBuilderTest#builtPropertySourcePreservesFileExtensionBuilder#buildpropagates the properties extensionNacosPropertySourceBuilderTest#builtPropertySourcePreservesPropertiesExtensionImpact
NacosPropertySourceconstructor keeps working; the refresh listener's two-step fallback covers that case.spring-alibaba-nacos-config; no public contract onNacosPropertySourcewas removed.Addresses #4296