Merge pull request #421 from elandau/bugfix/overrides

config: Fix isLoaded not set in the correct order
Netflix · May 21, 2019 · 1b30bd7 · 1b30bd7
2 parents 5356690 + 7084f65
commit 1b30bd7
Show file tree

Hide file tree

Showing 3 changed files with 79 additions and 14 deletions.
diff --git a/ribbon-archaius/src/main/java/com/netflix/client/config/ArchaiusPropertyResolver.java b/ribbon-archaius/src/main/java/com/netflix/client/config/ArchaiusPropertyResolver.java
@@ -44,8 +44,6 @@ private static void invokeAction(Runnable action) {
 
     @Override
     public <T> Optional<T> get(String key, Class<T> type) {
-        LOG.debug("Loading property {}", key);
-
         if (Integer.class.equals(type)) {
             return Optional.ofNullable((T) config.getInteger(key, null));
         } else if (Boolean.class.equals(type)) {
@@ -70,7 +68,8 @@ public <T> Optional<T> get(String key, Class<T> type) {
                                     .orElseThrow(() -> new IllegalArgumentException("Unable to convert value to desired type " + type));
                         }
                     });
-        }    }
+        }
+    }
 
     @Override
     public void forEach(String prefix, BiConsumer<String, String> consumer) {

diff --git a/ribbon-core/src/main/java/com/netflix/client/config/ReloadableClientConfig.java b/ribbon-core/src/main/java/com/netflix/client/config/ReloadableClientConfig.java
@@ -59,12 +59,24 @@ public abstract class ReloadableClientConfig implements IClientConfig {
 
     private boolean isLoaded = false;
 
+    /**
+     * @deprecated Use {@link #ReloadableClientConfig(PropertyResolver, String, String)}
+     */
+    @Deprecated
     protected ReloadableClientConfig(PropertyResolver resolver) {
-        this.clientName = DEFAULT_CLIENT_NAME;
-        this.resolver = resolver;
+        this(resolver, DEFAULT_CLIENT_NAME);
     }
 
+    /**
+     * @deprecated Use {@link #ReloadableClientConfig(PropertyResolver, String, String)}
+     */
+    @Deprecated
     protected ReloadableClientConfig(PropertyResolver resolver, String clientName) {
+        this(resolver, DEFAULT_NAMESPACE, DEFAULT_CLIENT_NAME);
+    }
+
+    protected ReloadableClientConfig(PropertyResolver resolver, String namespace, String clientName) {
+        this.namespace = namespace;
         this.clientName = clientName;
         this.resolver = resolver;
     }
@@ -106,14 +118,11 @@ public final void setNameSpace(String nameSpace) {
 
     @Override
     public void loadProperties(String clientName) {
-        Preconditions.checkState(isLoaded == false, "Config '{}' can only be loaded once", clientName);
-        if (!isLoaded) {
-            loadDefaultValues();
-            this.isLoaded = true;
-            resolver.onChange(this::reload);
-        }
-
+        Preconditions.checkState(!isLoaded, "Config '{}' can only be loaded once", clientName);
         this.clientName = clientName;
+        this.isLoaded = true;
+        loadDefaultValues();
+        resolver.onChange(this::reload);
     }
 
     @Override
@@ -202,8 +211,6 @@ interface ReloadableProperty<T> extends Property<T> {
     }
 
     private <T> ReloadableProperty<T> getClientDynamicProperty(IClientConfigKey<T> key) {
-        LOG.debug("Get dynamic property key={} ns={} client={}", key.key(), getNameSpace(), clientName);
-
         return createProperty(
                 () -> resolveFinalProperty(key),
                 key::defaultValue);
@@ -295,18 +302,24 @@ public <T> Optional<T> getIfSet(IClientConfigKey<T> key) {
 
     @Override
     public final <T> Property<T> getDynamicProperty(IClientConfigKey<T> key) {
+        LOG.debug("Get dynamic property key={} ns={} client={}", key.key(), getNameSpace(), clientName);
+
         return getClientDynamicProperty(key);
     }
 
     @Override
     public <T> Property<T> getScopedProperty(IClientConfigKey<T> key) {
+        LOG.debug("Get dynamic property key={} ns={} client={}", key.key(), getNameSpace(), clientName);
+
         return (Property<T>) dynamicProperties.computeIfAbsent(key, ignore -> createProperty(
                 () -> resolverScopedProperty(key),
                 key::defaultValue));
     }
 
     @Override
     public <T> Property<T> getPrefixMappedProperty(IClientConfigKey<T> key) {
+        LOG.debug("Get dynamic property key={} ns={} client={}", key.key(), getNameSpace(), clientName);
+
         return (Property<T>) dynamicProperties.computeIfAbsent(key, ignore -> createProperty(
                 getPrefixedMapPropertySupplier(key),
                 key::defaultValue));

diff --git a/ribbon-core/src/test/java/com/netflix/client/config/ReloadableClientConfigTest.java b/ribbon-core/src/test/java/com/netflix/client/config/ReloadableClientConfigTest.java
@@ -0,0 +1,53 @@
+package com.netflix.client.config;
+
+import com.netflix.config.ConfigurationManager;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TestName;
+
+public class ReloadableClientConfigTest {
+    @Rule
+    public TestName testName = new TestName();
+
+    private CommonClientConfigKey<Integer> testKey;
+
+    @Before
+    public void before() {
+        this.testKey = new CommonClientConfigKey<Integer>(testName.getMethodName(), -1) {};
+    }
+
+    @Test
+    public void testOverrideLoadedConfig() {
+        final DefaultClientConfigImpl overrideconfig = new DefaultClientConfigImpl();
+        overrideconfig.set(testKey, 123);
+
+        final DefaultClientConfigImpl config = new DefaultClientConfigImpl();
+        config.loadDefaultValues();
+        config.applyOverride(overrideconfig);
+
+        Assert.assertEquals(123, config.get(testKey).intValue());
+    }
+
+    @Test
+    public void setBeforeLoading() {
+        ConfigurationManager.getConfigInstance().setProperty("ribbon." + testKey.key(), "123");
+
+        final DefaultClientConfigImpl config = new DefaultClientConfigImpl();
+        config.loadProperties("foo");
+
+        Assert.assertEquals(123, config.get(testKey).intValue());
+    }
+
+    @Test
+    public void setAfterLoading() {
+        final DefaultClientConfigImpl config = new DefaultClientConfigImpl();
+        config.loadProperties("foo");
+        config.set(testKey, 456);
+
+        ConfigurationManager.getConfigInstance().setProperty("ribbon." + testKey.key(), "123");
+
+        Assert.assertEquals(123, config.get(testKey).intValue());
+    }
+}