Throw ParseException when a configuration setting can't be parsed as the type requested.

Homogenize the handling of parsing errors. Before this change, some code paths in the default Property implementation would silently swallow parsing errors and either return a defaultValue or null (even for non-nullable methods!) This leads to errors that are hard to debug.

Throwing a ParseException is a behaviour change that will lead to simpler debugging, at the expense of surfacing latent bugs in user code. For example, if bad values were being set in config files, the code will now fail instead of silently returning a different value than expected.

Author: rgallardo-netflix

archaius2-api/src/main/java/com/netflix/archaius/api/PropertyFactory.java

@@ -26,8 +26,8 @@
 @Deprecated
 public interface PropertyFactory extends PropertyRepository {
     /**
-     * Create a property for the property name.
-     * @deprecated Use {@link PropertyRepository#get(String, Type)} instead.
+     * Create a {@link PropertyContainer} for the given name.
+     * @deprecated Use {@link PropertyRepository#get(String, Type)} or {@link PropertyRepository#get(String, Class)} instead.
      */
     @Deprecated
     PropertyContainer getProperty(String propName);

archaius2-archaius1-bridge/src/test/java/com/netflix/archaius/bridge/DynamicPropertyTest.java

@@ -1,13 +1,13 @@
 package com.netflix.archaius.bridge;
 
+import com.netflix.archaius.api.PropertyRepository;
 import org.apache.commons.configuration.AbstractConfiguration;
 
 import com.google.inject.Guice;
 import com.google.inject.Injector;
 import com.google.inject.Key;
 import com.netflix.archaius.api.Config;
 import com.netflix.archaius.api.Property;
-import com.netflix.archaius.api.PropertyFactory;
 import com.netflix.archaius.api.config.SettableConfig;
 import com.netflix.archaius.api.inject.RuntimeLayer;
 import com.netflix.archaius.guice.ArchaiusModule;
@@ -44,7 +44,7 @@ public void after() {
     @Test
     public void settingOnArchaius2UpdateArchaius1(TestInfo testInfo) {
         String methodName = testInfo.getTestMethod().map(Method::getName).orElse("unknown");
-        Property<String> a2prop = injector.getInstance(PropertyFactory.class).getProperty(methodName).asString("default");
+        Property<String> a2prop = injector.getInstance(PropertyRepository.class).get(methodName, String.class).orElse("default");
         DynamicStringProperty a1prop = DynamicPropertyFactory.getInstance().getStringProperty(methodName, "default");
 
         assertEquals("default", a1prop.get());
@@ -63,7 +63,7 @@ public void testNonStringDynamicProperty() {
         config.accept(new PrintStreamVisitor());
         ConfigurationManager.getConfigInstance().setProperty("foo", 123);
 
-        Property<Integer> prop2 = injector.getInstance(PropertyFactory.class).getProperty("foo").asInteger(1);
+        Property<Integer> prop2 = injector.getInstance(PropertyRepository.class).get("foo", Integer.class).orElse(1);
 
         DynamicIntProperty prop = DynamicPropertyFactory.getInstance().getIntProperty("foo", 2);
 

archaius2-core/src/main/java/com/netflix/archaius/DefaultPropertyFactory.java

@@ -6,6 +6,7 @@
 import com.netflix.archaius.api.PropertyContainer;
 import com.netflix.archaius.api.PropertyFactory;
 import com.netflix.archaius.api.PropertyListener;
+import com.netflix.archaius.exceptions.ParseException;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -62,6 +63,7 @@ public DefaultPropertyFactory(Config config) {
         this.config.addListener(this);
     }
 
+    /** @deprecated Use {@link #get(String, Type)} or {@link #get(String, Class)} instead. */
     @Override
     @Deprecated
     @SuppressWarnings("deprecation")
@@ -124,20 +126,24 @@ public <T> Property<T> asType(Class<T> type, T defaultValue) {
 
             @Override
             public <T> Property<T> asType(Function<String, T> mapper, String defaultValue) {
-                T typedDefaultValue = mapper.apply(defaultValue);
+                T typedDefaultValue = applyOrThrow(mapper, defaultValue);
                 return getFromSupplier(propName, null, () -> {
                     String value = config.getString(propName, null);
                     if (value != null) {
-                        try {
-                            return mapper.apply(value);
-                        } catch (Exception e) {
-                            LOG.error("Invalid value '{}' for property '{}'. Will return the default instead.", propName, value);
-                        }
+                            return applyOrThrow(mapper, value);
                     }
 
                     return typedDefaultValue;
                 });
             }
+
+            private <T> T applyOrThrow(Function<String, T> mapper, String value) {
+                try {
+                    return mapper.apply(value);
+                } catch (RuntimeException e) {
+                    throw new ParseException("Invalid value '" + value + "' for property '" + propName + "'.", e);
+                }
+            }
         };
     }
 
@@ -208,23 +214,28 @@ public PropertyImpl(KeyAndType<T> keyAndType, Supplier<T> supplier) {
 
         @Override
         public T get() {
-            int cacheVersion = cache.getStamp();
+            int[] cacheVersion = new int[1];
+            T currentValue = cache.get(cacheVersion);
             int latestVersion  = masterVersion.get();
-            
-            if (cacheVersion != latestVersion) {
-                T currentValue = cache.getReference();
-                T newValue = null;
-                try {
-                    newValue = supplier.get();
-                } catch (Exception e) {
-                    LOG.error("Unable to get current version of property '{}'", keyAndType.key, e);
-                }
-                
-                if (cache.compareAndSet(currentValue, newValue, cacheVersion, latestVersion)) {
-                    // Possible race condition here but not important enough to warrant locking
+
+            if (cacheVersion[0] == latestVersion) {
+                return currentValue;
+            }
+
+            try {
+                T newValue = supplier.get();
+
+                if (cache.compareAndSet(currentValue, newValue, cacheVersion[0], latestVersion)) {
+                    // newValue could be stale here already, if the cache was updated *again* between the CAS and this line
+                    // We don't care enough about this edge case to fix it.
                     return newValue;
                 }
+
+            } catch (RuntimeException e) {
+                LOG.error("Unable to get current version of property '{}'", keyAndType.key, e);
+                throw e; // Rethrow the exception, our caller should know that the property is not available
             }
+
             return cache.getReference();
         }
 

archaius2-core/src/test/java/com/netflix/archaius/ConfigManagerTest.java

@@ -20,7 +20,7 @@
 import java.util.concurrent.atomic.AtomicReference;
 
 import com.netflix.archaius.api.Property;
-import com.netflix.archaius.api.PropertyFactory;
+import com.netflix.archaius.api.PropertyRepository;
 import com.netflix.archaius.config.DefaultCompositeConfig;
 
 import com.netflix.archaius.config.MapConfig;
@@ -45,9 +45,9 @@ public void testBasicReplacement() throws ConfigException {
                         .build()));
 
 
-        PropertyFactory factory = DefaultPropertyFactory.from(config);
+        PropertyRepository factory = DefaultPropertyFactory.from(config);
 
-        Property<String> prop = factory.getProperty("abc").asString("defaultValue");
+        Property<String> prop = factory.get("abc", String.class).orElse("defaultValue");
 
         // Use an AtomicReference to capture the property value change
         AtomicReference<String> capturedValue = new AtomicReference<>("");