diff --git a/archaius2-api/src/main/java/com/netflix/archaius/api/annotations/Configuration.java b/archaius2-api/src/main/java/com/netflix/archaius/api/annotations/Configuration.java index 24a5e83db..bd2e225b7 100644 --- a/archaius2-api/src/main/java/com/netflix/archaius/api/annotations/Configuration.java +++ b/archaius2-api/src/main/java/com/netflix/archaius/api/annotations/Configuration.java @@ -22,8 +22,17 @@ 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. + *

+ * Note that an interface can be bound via the ConfigProxyFactory even if it does NOT have this annotation. + *

+ * 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) @@ -31,37 +40,37 @@ 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; diff --git a/archaius2-core/src/main/java/com/netflix/archaius/ConfigProxyFactory.java b/archaius2-core/src/main/java/com/netflix/archaius/ConfigProxyFactory.java index 674bb6191..e99312847 100644 --- a/archaius2-core/src/main/java/com/netflix/archaius/ConfigProxyFactory.java +++ b/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. *

* For example, * 

@@ -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"
  * }
  * }
  *
* - * 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 @@ * } * * - * 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. * * 
@@ -92,7 +95,10 @@
  *
* * 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. *

* 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 newProxy(final Class 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,7 +303,7 @@ private String derivePrefix(Configuration annot, String prefix) { } @SuppressWarnings({"unchecked", "rawtypes"}) - private MethodInvokerHolder buildInvokerForMethod(Class type, String prefix, Method m, T proxyObject, boolean immutable) { + private MethodInvokerHolder buildInvokerForMethod(Class proxyObjectType, String prefix, Method m, T proxyObject, boolean immutable) { try { final Class returnType = m.getReturnType(); @@ -304,16 +311,8 @@ private MethodInvokerHolder buildInvokerForMethod(Class type, String pref 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, but we can't say that in Java. + final Function 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 MethodInvokerHolder buildInvokerForMethod(Class 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 Function defaultValueSupplierForMethod(Class 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,7 +429,7 @@ private static Function memoize(T value) { } /** A supplier that calls a default method in the proxied interface and returns its output */ - private static Function createDefaultMethodSupplier(Method method, Class type, T proxyObject) { + private static Function createDefaultMethodSupplier(Method method, Class proxyObjectType, T proxyObject) { final MethodHandle methodHandle; try { @@ -402,21 +437,21 @@ private static Function createDefaultMethodSupplier(Method meth Constructor 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) -> { diff --git a/archaius2-core/src/main/java/com/netflix/archaius/DefaultPropertyFactory.java b/archaius2-core/src/main/java/com/netflix/archaius/DefaultPropertyFactory.java index 115d7c2a6..5bc841d22 100644 --- a/archaius2-core/src/main/java/com/netflix/archaius/DefaultPropertyFactory.java +++ b/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 newValue; try { // Get the new value from the supplier. This call could fail. - CachedValue 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,7 +282,7 @@ public synchronized void removeListener(PropertyListener listener) { @Override public Property 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; }); } @@ -288,12 +290,12 @@ public Property orElse(T defaultValue) { @Override public Property 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 keyAndType = this.keyAndType.withKey(key); Property 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(); }); } @@ -301,7 +303,7 @@ public Property orElseGet(String key) { @Override public Property map(Function 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 Property asType(Class type, T defaultValue) { public Property asType(Function 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; + } }); } diff --git a/archaius2-core/src/test/java/com/netflix/archaius/ProxyFactoryTest.java b/archaius2-core/src/test/java/com/netflix/archaius/ProxyFactoryTest.java index d650d64bf..bf60b361a 100644 --- a/archaius2-core/src/test/java/com/netflix/archaius/ProxyFactoryTest.java +++ b/archaius2-core/src/test/java/com/netflix/archaius/ProxyFactoryTest.java @@ -9,7 +9,6 @@ import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.junit.jupiter.api.Assertions.fail; import java.util.ArrayList; import java.util.Arrays; @@ -34,7 +33,6 @@ import com.netflix.archaius.api.annotations.PropertyName; import com.netflix.archaius.api.config.SettableConfig; import com.netflix.archaius.config.DefaultSettableConfig; -import com.netflix.archaius.exceptions.ParseException; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; @@ -86,24 +84,29 @@ public void testDefaultValues() { @Test public void testBadValuesInConfig() { config.setProperty("integer", "a"); + config.setProperty("required", "1"); - RootConfig a = proxyFactory.newProxy(RootConfig.class); + WithRequiredValue a = proxyFactory.newProxy(WithRequiredValue.class); - // We should be able to *build* the config object, but then get an error when trying to get the value - ParseException pe = assertThrows(ParseException.class, a::getInteger); - assertThat(pe.getMessage(), allOf( - containsString("'integer'"), - containsString("'a'"))); + // A bad config value causes the default to be used + assertEquals(0, a.getInteger()); - // Other values do work + // A missing setting returns the default assertEquals(TestEnum.NONE, a.getEnum()); - // But a subsequent, invalid, change to the configs now results in an error + // An updated setting returns the new value + config.setProperty("enum", "A"); + assertEquals(TestEnum.A, a.getEnum()); + + // A second update, to an invalid value, reverts to returning the default. config.setProperty("enum", "NOTINENUM"); - ParseException pe2 = assertThrows(ParseException.class, a::getEnum); - assertThat(pe2.getMessage(), allOf( - containsString("'enum'"), - containsString("'NOTINENUM'"))); + assertEquals(TestEnum.NONE, a.getEnum()); + + assertEquals(1, a.getRequired()); + config.setProperty("required", "a"); + NullPointerException npe = assertThrows(NullPointerException.class, a::getRequired); + // We should get a message that includes the property name + assertThat(npe.getMessage(), containsString("required")); } @Test @@ -131,7 +134,7 @@ public void testAllPropertiesSet() { config.setProperty("prefix.baseBoolean", true); config.setProperty("prefix.intArray", "0,1,2,3"); - RootConfig a = proxyFactory.newProxy(RootConfig.class, "prefix"); + WithRequiredValue a = proxyFactory.newProxy(WithRequiredValue.class, "prefix"); assertThat(a.getStr(), equalTo("str1")); assertThat(a.getInteger(), equalTo(1)); @@ -145,12 +148,9 @@ public void testAllPropertiesSet() { config.setProperty("prefix.subConfig.str", "str3"); assertThat(a.getSubConfig().str(), equalTo("str3")); - try { - a.getRequiredValue(); - fail("should have failed with no value for requiredValue"); - } - catch (Exception expected) { - } + NullPointerException npe = assertThrows(NullPointerException.class, a::getRequired); + // We should get a message that includes the property name + assertThat(npe.getMessage(), containsString("prefix.required")); } @Test @@ -494,13 +494,17 @@ public interface RootConfig extends BaseConfig { @DefaultValue("") Integer[] getIntArray(); - int getRequiredValue(); - default long getOtherLongValueWithDefault() { return 43L; } } + @SuppressWarnings("UnusedReturnValue") + public interface WithRequiredValue extends RootConfig { + // A primitive-valued property will throw an exception if it's not set. + int getRequired(); + } + public interface SubConfig { @DefaultValue("default") String str(); diff --git a/archaius2-core/src/test/java/com/netflix/archaius/property/PropertyTest.java b/archaius2-core/src/test/java/com/netflix/archaius/property/PropertyTest.java index 8bb5fe60c..5b14d3377 100644 --- a/archaius2-core/src/test/java/com/netflix/archaius/property/PropertyTest.java +++ b/archaius2-core/src/test/java/com/netflix/archaius/property/PropertyTest.java @@ -33,7 +33,6 @@ import java.util.function.IntConsumer; import com.netflix.archaius.api.PropertyContainer; -import com.netflix.archaius.exceptions.ParseException; import org.junit.jupiter.api.BeforeEach; import org.mockito.Mockito; @@ -47,9 +46,6 @@ import org.junit.jupiter.api.Test; -import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.allOf; -import static org.hamcrest.Matchers.containsString; import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotEquals; @@ -441,29 +437,43 @@ public void chainedPropertyNotification() { @Test public void testBadValue() { - config.setProperty("first", "bad"); + Property prop = factory.get("first", Integer.class); - Property prop = factory - .get("first", Integer.class); + // No value, property returns null + assertNull(prop.get()); - ParseException pe = assertThrows(ParseException.class, prop::get); - assertThat(pe.getMessage(), allOf( - containsString("'first'"), - containsString("'bad'"))); + // Set to 1 + config.setProperty("first", "1"); + assertEquals(1, prop.get().intValue()); + + // Set to bad value, property reverts to returning null + config.setProperty("first", "bad"); + assertNull(prop.get()); + + // Set to good value again + config.setProperty("first", "2"); + assertEquals(2, prop.get().intValue()); } + @Test public void chainedPropertyBadValue() { - config.setProperty("first", "bad"); + Property prop = factory.get("first", Integer.class).orElse(3); - Property prop = factory - .get("first", Integer.class) - .orElse(3); + // No value, property returns default + assertEquals(3, prop.get()); + + // Set to 1 + config.setProperty("first", "1"); + assertEquals(1, prop.get().intValue()); - ParseException pe = assertThrows(ParseException.class, prop::get); - assertThat(pe.getMessage(), allOf( - containsString("'first'"), - containsString("'bad'"))); + // Set to bad value, property reverts to returning default + config.setProperty("first", "bad"); + assertEquals(3, prop.get()); + + // Set to good value again + config.setProperty("first", "2"); + assertEquals(2, prop.get().intValue()); } @Test @@ -474,12 +484,10 @@ public void testSubscriptionsBadValue() { .orElse(3); Property stringProperty = factory.get("first", String.class); - Consumer integerConsumer = unexpected -> fail("Consumer should not be called. Received argument: " + unexpected); + AtomicInteger integerConsumer = new AtomicInteger(); AtomicReference stringConsumer = new AtomicReference<>(); - // Order is important here! We want the string subscription to be called *after* trying to call the integer - // subscription, which should fail because "a" can not be decoded as an integer - integerProperty.subscribe(integerConsumer); + integerProperty.subscribe(integerConsumer::set); stringProperty.subscribe(stringConsumer::set); // Initial value for the property is 2 @@ -489,7 +497,8 @@ public void testSubscriptionsBadValue() { // Set it to something that's not an integer config.setProperty("first", "a"); - // The integer consumer is never called + // The integer consumer is called with the default value + assertEquals(3, integerConsumer.get()); // The string consumer *is* called with the new value assertEquals("a", stringConsumer.get()); } @@ -520,7 +529,8 @@ public void testSubscriptionsNullValue() { // Set it to something that's not an integer config.setProperty("first", null); - // The integer consumer is never called + // The primitive int consumer is never called. The subscription machinery will get an NPE internally when + // it tries to cast the null to a primitive int. Nevertheless, the rest of the consumers should still be called. // The string consumer *is* called with the new value assertNull(stringConsumer.get()); // ... the boxed integer consumer also gets the new value @@ -545,7 +555,9 @@ public void testSubscriptionsConsumerFails() { AtomicReference stringConsumer = new AtomicReference<>(); // Order is important here! We want the string subscription to be called *after* trying to call the integer - // subscription, which should fail because "a" can not be decoded as an integer + // subscription, which should fail because "a" can not be decoded as an integer. + // This test relies on the fact that the property factory calls listeners in the order they were added. + // THAT IS NOT PART OF THE PUBLIC CONTRACT! integerProperty.subscribe(faultyConsumer); stringProperty.subscribe(stringConsumer::set); @@ -720,14 +732,11 @@ public void testPropertyContainer() { config.setProperty("foo", 3); assertEquals(3, prop.get().intValue()); - // Set to an invalid value + // Setting to an invalid value should cause the default value to be used config.setProperty("foo", "notAnInt"); - ParseException pe = assertThrows(ParseException.class, prop::get); - assertThat(pe.getMessage(), allOf(containsString("foo"), containsString("notAnInt"))); + assertEquals(1, prop.get()); - // Using the custom parser feature should still return a ParseException. Older versions of the - // code would silently return the default value instead. - ParseException pe2 = assertThrows(ParseException.class, () -> container.asType(Integer::parseInt, "1").get()); - assertThat(pe2.getMessage(), allOf(containsString("foo"), containsString("notAnInt"))); + // Using the custom parser feature should return the default value when the value is invalid + assertEquals(1, container.asType(Integer::parseInt, "1").get()); } }