Merge pull request #738 from Netflix/rollback-exception-propagation

Go back to the old behavior of returning null on ParseExceptions

Author: rgallardo-netflix

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;
+                }
             });
         }