Merge pull request #735 from Netflix/type-error-handling

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

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<>("");