Skip to content

Commit 023217c

Browse files
Shawyeokclaudemergify[bot]
authored
fix: ConfigService.getConfig(appId, namespace) returns wrong app's config (#140)
* fix: appId dropped when creating PropertiesCompatibleFileConfigRepository for non-default appId DefaultConfigFactory.createPropertiesCompatibleFileConfigRepository() received an appId parameter but called ConfigService.getConfigFile(namespace, format) — the two-arg overload that ignores appId and resolves against the default app.id from app.properties. Fixes the bug by: 1. Adding ConfigService.getConfigFile(appId, namespace, format) that delegates to the already-correct ConfigManager.getConfigFile(appId, namespace, format). 2. Updating DefaultConfigFactory to call the new three-arg overload so the caller-specified appId is preserved. Adds tests: - DefaultConfigFactoryTest.testCreatePropertiesCompatibleFileConfigRepositoryForwardsCustomAppId: verifies ConfigManager is invoked with the supplied appId, never the default. - ConfigServiceTest.testGetConfigFileWithCustomAppId: verifies the new ConfigService.getConfigFile(appId, ns, format) overload returns a ConfigFile whose getAppId() equals the requested appId. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test: add regression test for custom appId on properties-compatible namespace Add ConfigServiceTest.testGetConfigWithCustomAppIdForPropertiesCompatibleNamespace, which drives the real DefaultConfigFactory path (create -> createPropertiesCompatibleFileConfigRepository -> ConfigService.getConfigFile(appId, namespace, format)) for a .yml namespace. The existing custom-appId tests either used a properties namespace or called the new getConfigFile overload directly, so neither would catch DefaultConfigFactory.createPropertiesCompatibleFileConfigRepository dropping the custom appId again. The new test stubs only createConfigFile and echoes the received appId into the resulting Config, so it fails if the appId is dropped. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
1 parent c31e574 commit 023217c

3 files changed

Lines changed: 131 additions & 1 deletion

File tree

apollo-client/src/main/java/com/ctrip/framework/apollo/ConfigService.java

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,19 @@ public static ConfigFile getConfigFile(String namespace, ConfigFileFormat config
101101
return s_instance.getManager().getConfigFile(namespace, configFileFormat);
102102
}
103103

104+
/**
105+
* Get the config file instance for the appId and namespace.
106+
*
107+
* @param appId the appId of the config
108+
* @param namespace the namespace of the config without file extension, e.g. "application"
109+
* @param configFileFormat the config file format
110+
* @return config file instance
111+
*/
112+
public static ConfigFile getConfigFile(String appId, String namespace,
113+
ConfigFileFormat configFileFormat) {
114+
return s_instance.getManager().getConfigFile(appId, namespace, configFileFormat);
115+
}
116+
104117
public static ConfigMonitor getConfigMonitor(){
105118
return s_instance.getMonitor();
106119
}

apollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigFactory.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,7 @@ PropertiesCompatibleFileConfigRepository createPropertiesCompatibleFileConfigRep
163163
String appId, String namespace, ConfigFileFormat format) {
164164
String actualNamespaceName = trimNamespaceFormat(namespace, format);
165165
PropertiesCompatibleConfigFile configFile = (PropertiesCompatibleConfigFile) ConfigService
166-
.getConfigFile(actualNamespaceName, format);
166+
.getConfigFile(appId, actualNamespaceName, format);
167167

168168
return new PropertiesCompatibleFileConfigRepository(configFile);
169169
}

apollo-client/src/test/java/com/ctrip/framework/apollo/ConfigServiceTest.java

Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@
2020

2121
import com.ctrip.framework.apollo.core.MetaDomainConsts;
2222
import com.ctrip.framework.apollo.enums.ConfigSourceType;
23+
import com.ctrip.framework.apollo.spi.DefaultConfigFactory;
24+
import java.util.Properties;
2325
import java.util.Set;
2426

2527
import org.junit.After;
@@ -103,6 +105,60 @@ public void testMockConfigFactoryForConfigFile() throws Exception {
103105
assertEquals(someNamespaceFileName + ":" + someConfigFileFormat.getValue(), configFile.getContent());
104106
}
105107

108+
@Test
109+
public void testGetConfigWithCustomAppId() throws Exception {
110+
String customAppId = "customAppId";
111+
String someNamespace = "mock";
112+
String someKey = "someKey";
113+
MockInjector.setInstance(ConfigFactory.class, someNamespace, new MockConfigFactory());
114+
115+
Config config = ConfigService.getConfig(customAppId, someNamespace);
116+
117+
assertEquals(customAppId + ConfigConsts.CLUSTER_NAMESPACE_SEPARATOR + someNamespace + ":" + someKey,
118+
config.getProperty(someKey, null));
119+
}
120+
121+
@Test
122+
public void testGetConfigFileWithCustomAppId() throws Exception {
123+
String customAppId = "customAppId";
124+
String someNamespace = "mock";
125+
ConfigFileFormat someConfigFileFormat = ConfigFileFormat.YML;
126+
String someNamespaceFileName =
127+
String.format("%s.%s", someNamespace, someConfigFileFormat.getValue());
128+
MockInjector.setInstance(ConfigFactory.class, someNamespaceFileName, new MockConfigFactory());
129+
130+
ConfigFile configFile = ConfigService.getConfigFile(customAppId, someNamespace, someConfigFileFormat);
131+
132+
assertEquals(customAppId, configFile.getAppId());
133+
assertEquals(someNamespaceFileName, configFile.getNamespace());
134+
}
135+
136+
@Test
137+
public void testGetConfigWithCustomAppIdForPropertiesCompatibleNamespace() throws Exception {
138+
String customAppId = "customAppId";
139+
String someNamespace = "mock";
140+
ConfigFileFormat someConfigFileFormat = ConfigFileFormat.YML;
141+
String someNamespaceFileName =
142+
String.format("%s.%s", someNamespace, someConfigFileFormat.getValue());
143+
144+
// Exercise the real DefaultConfigFactory path for a non-properties namespace, i.e.
145+
// create(appId, "mock.yml") -> createPropertiesCompatibleFileConfigRepository(...) ->
146+
// ConfigService.getConfigFile(appId, "mock", YML). Only createConfigFile is stubbed (to avoid
147+
// hitting a remote repository); it echoes the appId it receives into the resulting properties so
148+
// the assertion below fails if DefaultConfigFactory ever drops the custom appId again.
149+
MockInjector.setInstance(ConfigFactory.class, someNamespaceFileName, new DefaultConfigFactory() {
150+
@Override
151+
public ConfigFile createConfigFile(String appId, String namespace,
152+
ConfigFileFormat configFileFormat) {
153+
return new MockPropertiesCompatibleConfigFile(appId, namespace, configFileFormat);
154+
}
155+
});
156+
157+
Config config = ConfigService.getConfig(customAppId, someNamespaceFileName);
158+
159+
assertEquals(customAppId, config.getProperty("appId", null));
160+
}
161+
106162
private static class MockConfig extends AbstractConfig {
107163
private final String m_appId;
108164
private final String m_namespace;
@@ -213,6 +269,67 @@ public ConfigFile createConfigFile(String appId, String namespace, ConfigFileFor
213269
}
214270
}
215271

272+
private static class MockPropertiesCompatibleConfigFile implements PropertiesCompatibleConfigFile {
273+
private final String m_appId;
274+
private final String m_namespace;
275+
private final ConfigFileFormat m_configFileFormat;
276+
277+
public MockPropertiesCompatibleConfigFile(String appId, String namespace,
278+
ConfigFileFormat configFileFormat) {
279+
m_appId = appId;
280+
m_namespace = namespace;
281+
m_configFileFormat = configFileFormat;
282+
}
283+
284+
@Override
285+
public Properties asProperties() {
286+
Properties properties = new Properties();
287+
// echo the appId so it is observable through the resulting Config
288+
properties.setProperty("appId", m_appId);
289+
return properties;
290+
}
291+
292+
@Override
293+
public String getContent() {
294+
return null;
295+
}
296+
297+
@Override
298+
public boolean hasContent() {
299+
return true;
300+
}
301+
302+
@Override
303+
public String getAppId() {
304+
return m_appId;
305+
}
306+
307+
@Override
308+
public String getNamespace() {
309+
return m_namespace;
310+
}
311+
312+
@Override
313+
public ConfigFileFormat getConfigFileFormat() {
314+
return m_configFileFormat;
315+
}
316+
317+
@Override
318+
public void addChangeListener(ConfigFileChangeListener listener) {
319+
320+
}
321+
322+
@Override
323+
public boolean removeChangeListener(ConfigFileChangeListener listener) {
324+
return false;
325+
}
326+
327+
@Override
328+
public ConfigSourceType getSourceType() {
329+
return ConfigSourceType.REMOTE;
330+
}
331+
}
332+
216333
public static class MockConfigUtil extends ConfigUtil {
217334
@Override
218335
public String getAppId() {

0 commit comments

Comments
 (0)