Go back to the old behavior of returning null on ParseExceptions

We discovered that too many consumers depend on the old behavior that avoided throwing exceptions from configuration reading calls. Trying to adapt code paths that put these calls in the middle of critical request-servicing code is too onerous.

This version DOES fix the old bug that caused properties with defaults (from the Property.orElse() method) to fail for this "invalid setting" case. They will now return the default value.

We are also retaining the noisy logging of parsing errors, to assist users with detection and fixing.

archaius2-api/src/main/java/com/netflix/archaius/api/annotations/Configuration.java

@@ -22,46 +22,55 @@
 import java.lang.annotation.Target;
 
 /**
- * Marks a field as a configuration item. Governator will auto-assign the value based
- * on the {@link #value()} of the annotation via the set {@link ConfigurationProvider}.
+ * When applied to an interface, marks it as a candidate for binding via a ConfigProxyFactory (from the archaius2-core
+ * module). For this case, the only relevant attributes are {@link #prefix()}, which sets a shared prefix for all the
+ * properties bound to the interface's methods, and {@link #immutable()}, which when set to true creates a static proxy
+ * that always returns the config values as they were at the moment that the proxy object is created.
+ * <p>
+ * Note that an interface can be bound via the ConfigProxyFactory even if it does NOT have this annotation.
+ * <p>
+ * When applied to a field, marks it as a configuration item, to be injected with the value of the specified property
+ * key. This usage is deprecated in favor of using your DI-framework options for injecting configuration values.
+ * @see PropertyName
+ * @see DefaultValue
  */
 @Documented
 @Retention(java.lang.annotation.RetentionPolicy.RUNTIME)
 @Target({ElementType.TYPE})
 public @interface Configuration
 {
     /**
-     * @return name/key of the config to assign
+     * name/key of the config to assign
      */
     String      prefix() default "";
 
     /**
-     * @return field names to use for replacement
+     * field names to use for replacement
      */
     String[]    params() default {};
 
     /**
-     * @return user displayable description of this configuration
+     * user displayable description of this configuration
      */
     String      documentation() default "";
 
     /**
-     * @return true to allow mapping configuration to fields
+     * true to allow mapping configuration to fields
      */
     boolean     allowFields() default false;
 
     /**
-     * @return true to allow mapping configuration to setters
+     * true to allow mapping configuration to setters
      */
     boolean     allowSetters() default true;
 
     /**
-     * @return Method to call after configuration is bound
+     * Method to call after configuration is bound
      */
     String      postConfigure() default "";
 
     /**
-     * @return If true then properties cannot change once set otherwise methods will be 
+     * If true then properties cannot change once set otherwise methods will be
      * bound to dynamic properties via PropertyFactory.
      */
     boolean     immutable() default false;

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

@@ -42,7 +42,7 @@
 /**
  * Factory for binding a configuration interface to properties in a {@link PropertyFactory}
  * instance.  Getter methods on the interface are mapped by naming convention
- * by the property name may be overridden using the @PropertyName annotation.
+ * by the property name or may be overridden using the @{@link PropertyName} annotation.
  * <p>
  * For example,
  * <pre>
@@ -52,11 +52,14 @@
  *    int getTimeout();     // maps to "foo.timeout"
  *
  *    String getName();     // maps to "foo.name"
+ *
+ *    @PropertyName(name="bar")
+ *    String getSomeOtherName();  // maps to "foo.bar"
  * }
  * }
  * </pre>
  *
- * Default values may be set by adding a {@literal @}DefaultValue with a default value string.  Note
+ * Default values may be set by adding a {@literal @}{@link DefaultValue} with a default value string.  Note
  * that the default value type is a string to allow for interpolation.  Alternatively, methods can
  * provide a default method implementation.  Note that {@literal @}DefaultValue cannot be added to a default
  * method as it would introduce ambiguity as to which mechanism wins.
@@ -82,7 +85,7 @@
  * }
  * </pre>
  * 
- * To override the prefix in {@literal @}Configuration or provide a prefix when there is no 
+ * To override the prefix in {@literal @}{@link Configuration} or provide a prefix when there is no
  * {@literal @}Configuration annotation simply pass in a prefix in the call to newProxy.
  * 
  * <pre>
@@ -92,7 +95,10 @@
  * </pre>
  * 
  * By default, all properties are dynamic and can therefore change from call to call.  To make the
- * configuration static set the immutable attributes of @Configuration to true.
+ * configuration static set {@link Configuration#immutable()}  to true.  Creation of an immutable configuration
+ * will fail if the interface contains parametrized methods or methods that return primitive types and do not have a
+ * value set at the moment of creation, from either the underlying config, a {@link DefaultValue} annotation, or a
+ * default method implementation.
  * <p>
  * Note that an application should normally have just one instance of ConfigProxyFactory
  * and PropertyFactory since PropertyFactory caches {@link com.netflix.archaius.api.Property} objects.
@@ -245,7 +251,8 @@ <T> T newProxy(final Class<T> type, final String initialPrefix, boolean immutabl
 
                 if (immutable) {
                     // Cache the current value of the property and always return that.
-                    // Note that this will fail for parameterized properties!
+                    // Note that this will fail for parameterized properties and for primitive-valued methods
+                    // with no value set!
                     Object value = methodInvokerHolder.invoker.invoke(new Object[]{});
                     invokers.put(method, (args) -> value);
                 } else {
@@ -296,24 +303,16 @@ private String derivePrefix(Configuration annot, String prefix) {
     }
 
     @SuppressWarnings({"unchecked", "rawtypes"})
-    private <T> MethodInvokerHolder buildInvokerForMethod(Class<T> type, String prefix, Method m, T proxyObject, boolean immutable) {
+    private <T> MethodInvokerHolder buildInvokerForMethod(Class<T> proxyObjectType, String prefix, Method m, T proxyObject, boolean immutable) {
         try {
 
             final Class<?> returnType = m.getReturnType();
             final PropertyName nameAnnot = m.getAnnotation(PropertyName.class);
             final String propName = getPropertyName(prefix, m, nameAnnot);
 
             // A supplier for the value to be returned when the method's associated property is not set
-            final Function defaultValueSupplier;
-
-            if (m.getAnnotation(DefaultValue.class) != null) {
-                defaultValueSupplier = createAnnotatedMethodSupplier(m, m.getGenericReturnType(), config, decoder);
-            } else if (m.isDefault()) {
-                defaultValueSupplier = createDefaultMethodSupplier(m, type, proxyObject);
-            } else {
-                // No default specified in proxied interface. Return "empty" for collection types, null for any other type.
-                defaultValueSupplier = knownCollections.getOrDefault(returnType, (ignored) -> null);
-            }
+            // The proper parametrized type for this would be Function<Object[], returnType>, but we can't say that in Java.
+            final Function<Object[], ?> defaultValueSupplier = defaultValueSupplierForMethod(proxyObjectType, m, returnType, proxyObject, propName);
 
             // This object encapsulates the way to get the value for the current property.
             final PropertyValueGetter propertyValueGetter;
@@ -354,6 +353,42 @@ private <T> MethodInvokerHolder buildInvokerForMethod(Class<T> type, String pref
         }
     }
 
+    /**
+     * Build a supplier for the default value to be returned when the underlying property for a method is not set.
+     * Because of the way {@link Property} works, this will ALSO be called if the underlying property is set to null
+     * OR if it's set to a "bad" value that can't be decoded to the method's return type.
+     **/
+    private <PT> Function<Object[], ?> defaultValueSupplierForMethod(Class<PT> proxyObjectType, Method m, Type returnType, PT proxyObject, String propName) {
+        if (m.getAnnotation(DefaultValue.class) != null) {
+            // The method has a @DefaultValue annotation. Decode the string from there and return that.
+            return createAnnotatedMethodSupplier(m, m.getGenericReturnType(), config, decoder);
+        }
+
+        if (m.isDefault()) {
+            // The method has a default implementation in the interface. Obtain the default value by calling that implementation.
+            return createDefaultMethodSupplier(m, proxyObjectType, proxyObject);
+        }
+
+        // No default value available.
+        // For collections, return an empty
+        if (knownCollections.containsKey(returnType)) {
+            return knownCollections.get(returnType);
+        }
+
+        // For primitive return types, our historical behavior of returning a null causes an NPE with no message and an
+        // obscure trace. Instead of that we now use a fake supplier that will still throw the NPE, but adds a message to it.
+        if (returnType instanceof Class && ((Class<?>) returnType).isPrimitive()) {
+            return (ignored) -> {
+                String msg = String.format("Property '%s' is not set or has an invalid value and method %s.%s does not define a default value",
+                        propName, proxyObjectType.getName(), m.getName());
+                throw new NullPointerException(msg);
+            };
+        }
+
+        // For any other return type return nulls.
+        return (ignored) -> null;
+    }
+
     /**
      * Compute the name of the property that will be returned by this method.
      */
@@ -394,29 +429,29 @@ private static <T> Function<Object[], T> memoize(T value) {
     }
 
     /** A supplier that calls a default method in the proxied interface and returns its output */
-    private static <T> Function<Object[], T> createDefaultMethodSupplier(Method method, Class<T> type, T proxyObject) {
+    private static <T> Function<Object[], T> createDefaultMethodSupplier(Method method, Class<T> proxyObjectType, T proxyObject) {
         final MethodHandle methodHandle;
 
         try {
             if (SystemUtils.IS_JAVA_1_8) {
                 Constructor<MethodHandles.Lookup> constructor = MethodHandles.Lookup.class
                         .getDeclaredConstructor(Class.class, int.class);
                 constructor.setAccessible(true);
-                methodHandle = constructor.newInstance(type, MethodHandles.Lookup.PRIVATE)
-                        .unreflectSpecial(method, type)
+                methodHandle = constructor.newInstance(proxyObjectType, MethodHandles.Lookup.PRIVATE)
+                        .unreflectSpecial(method, proxyObjectType)
                         .bindTo(proxyObject);
             }
             else {
                 // Java 9 onwards
                 methodHandle = MethodHandles.lookup()
-                        .findSpecial(type,
+                        .findSpecial(proxyObjectType,
                                 method.getName(),
                                 MethodType.methodType(method.getReturnType(), method.getParameterTypes()),
-                                type)
+                                proxyObjectType)
                         .bindTo(proxyObject);
             }
         } catch (ReflectiveOperationException e) {
-            throw new RuntimeException("Failed to create temporary object for " + type.getName(), e);
+            throw new RuntimeException("Failed to create temporary object for " + proxyObjectType.getName(), e);
         }
 
         return (args) -> {

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

@@ -186,31 +186,33 @@ public T get() {
             // The tricky edge case is if another update came in between the check above to get the version and
             // the call to the supplier. In that case we'll tag the updated value with an old version number. That's fine,
             // since the next call to get() will see the old version and try again.
+            CachedValue<T> newValue;
             try {
                 // Get the new value from the supplier. This call could fail.
-                CachedValue<T> newValue = new CachedValue<>(supplier.get(), currentMasterVersion);
-
-                /*
-                 * We successfully got the new value, so now we update the cache. We use an atomic CAS operation to guard
-                 * from edge cases where another thread could have updated to a higher version than we have, in a flow like this:
-                 * Assume currentVersion started at 1., property cache is set to 1 too.
-                 * 1. Upstream update bumps version to 2.
-                 * 2. Thread A reads currentVersion at 2, cachedValue at 1, proceeds to start update, gets interrupted and yields the cpu.
-                 * 3. Thread C bumps version to 3, yields the cpu.
-                 * 4. Thread B is scheduled, reads currentVersion at 3, cachedValue still at 1, proceeds to start update.
-                 * 5. Thread B keeps running, updates cache to 3, yields.
-                 * 6. Thread A resumes, tries to write cache with version 2.
-                 */
-                CACHED_VALUE_UPDATER.compareAndSet(this, currentCachedValue, newValue);
-
-                return newValue.value;
+                newValue = new CachedValue<>(supplier.get(), currentMasterVersion);
 
             } catch (RuntimeException e) {
-                // Oh, no, something went wrong while trying to get the new value. Log the error and rethrow the exception
-                // so our caller knows there's a problem. We leave the cache unchanged. Next caller will try again.
-                LOG.error("Unable to get current version of property '{}'", keyAndType.key, e);
-                throw e;
+                // Oh, no, something went wrong while trying to get the new value. Log the error and return null.
+                // Upstream users may return that null unchanged or substitute it by a defaultValue.
+                // We leave the cache unchanged, which means the next caller will try again.
+                LOG.error("Unable to update value for property '{}'", keyAndType.key, e);
+                return null;
             }
+
+            /*
+             * We successfully got the new value, so now we update the cache. We use an atomic CAS operation to guard
+             * from edge cases where another thread could have updated to a higher version than we have, in a flow like this:
+             * Assume currentVersion started at 1., property cache is set to 1 too.
+             * 1. Upstream update bumps version to 2.
+             * 2. Thread A reads currentVersion at 2, cachedValue at 1, proceeds to start update, gets interrupted and yields the cpu.
+             * 3. Thread C bumps version to 3, yields the cpu.
+             * 4. Thread B is scheduled, reads currentVersion at 3, cachedValue still at 1, proceeds to start update.
+             * 5. Thread B keeps running, updates cache to 3, yields.
+             * 6. Thread A resumes, tries to write cache with version 2.
+             */
+            CACHED_VALUE_UPDATER.compareAndSet(this, currentCachedValue, newValue);
+
+            return newValue.value;
         }
 
         @Override
@@ -280,28 +282,28 @@ public synchronized void removeListener(PropertyListener<T> listener) {
         @Override
         public Property<T> orElse(T defaultValue) {
             return new PropertyImpl<>(keyAndType, () -> {
-                T value = supplier.get();
+                T value = this.get(); // Value from the "parent" property
                 return value != null ? value : defaultValue;
             });
         }
 
         @Override
         public Property<T> orElseGet(String key) {
             if (!keyAndType.hasType()) {
-                throw new IllegalStateException("Type information lost due to map() operation.  All calls to orElse[Get] must be made prior to calling map");
+                throw new IllegalStateException("Type information lost due to map() operation.  All calls to orElseGet() must be made prior to calling map");
             }
             KeyAndType<T> keyAndType = this.keyAndType.withKey(key);
             Property<T> next = DefaultPropertyFactory.this.get(key, keyAndType.type);
             return new PropertyImpl<>(keyAndType, () -> {
-                T value = supplier.get();
+                T value = this.get(); // Value from the "parent" property
                 return value != null ? value : next.get();
             });
         }
 
         @Override
         public <S> Property<S> map(Function<T, S> mapper) {
             return new PropertyImpl<>(keyAndType.discardType(), () -> {
-                T value = supplier.get();
+                T value = this.get(); // Value from the "parent" property
                 if (value != null) {
                     return mapper.apply(value);
                 } else {
@@ -464,12 +466,14 @@ public <T> Property<T> asType(Class<T> type, T defaultValue) {
         public <T> Property<T> asType(Function<String, T> mapper, String defaultValue) {
             T typedDefaultValue = applyOrThrow(mapper, defaultValue);
             return getFromSupplier(propName, null, () -> {
-                String value = config.getString(propName, null);
-                if (value != null) {
-                        return applyOrThrow(mapper, value);
-                }
+                String stringValue = config.getString(propName, null);
 
-                return typedDefaultValue;
+                try {
+                    return stringValue != null ? applyOrThrow(mapper, stringValue) : typedDefaultValue;
+                } catch (ParseException pe) {
+                    LOG.error("Error parsing value '{}' for property '{}'", stringValue, propName, pe);
+                    return typedDefaultValue;
+                }
             });
         }