From aa5d3f09076acf5aba525c755aff4ffa91d98acd Mon Sep 17 00:00:00 2001 From: Rebeca Gallardo Date: Thu, 23 May 2024 16:15:26 -0700 Subject: [PATCH] Handle numeric conversions for non-string raw values. For implementations of AbstractConfig that return non-string values from getRawProperty(), be smarter about doing numeric conversions. Without this, users are forced to guess if the property source will interpret a given number as an integer or a long and code to that, instead of to the types that make sense for their own use case. Fixes: #653 --- .../archaius/config/AbstractConfig.java | 63 ++++++++++++-- .../archaius/config/AbstractConfigTest.java | 84 +++++++++++++++++-- .../archaius/config/MapConfigTest.java | 9 ++ 3 files changed, 141 insertions(+), 15 deletions(-) diff --git a/archaius2-core/src/main/java/com/netflix/archaius/config/AbstractConfig.java b/archaius2-core/src/main/java/com/netflix/archaius/config/AbstractConfig.java index 9e0c7064..b00153d5 100644 --- a/archaius2-core/src/main/java/com/netflix/archaius/config/AbstractConfig.java +++ b/archaius2-core/src/main/java/com/netflix/archaius/config/AbstractConfig.java @@ -254,27 +254,74 @@ protected T getValue(Type type, String key) { } } + @SuppressWarnings("unchecked") protected T getValueWithDefault(Type type, String key, T defaultValue) { Object rawProp = getRawProperty(key); + + // Not found. Return the default. if (rawProp == null) { return defaultValue; } + + // raw prop is a String. Decode it or fail. if (rawProp instanceof String) { try { String value = resolve(rawProp.toString()); return decoder.decode(type, value); - } catch (NumberFormatException e) { + } catch (RuntimeException e) { return parseError(key, rawProp.toString(), e); } - } else if (type instanceof Class) { + } + + if (type instanceof Class) { + // Caller wants a simple class. Class cls = (Class) type; - if (cls.isInstance(rawProp) || cls.isPrimitive()) { + + // The raw object is already of the right type + if (cls.isInstance(rawProp)) { + return (T) rawProp; + } + + // Caller wants a string + if (String.class.isAssignableFrom(cls)) { + return (T) rawProp.toString(); + } + + // Caller wants an unwrapped boolean. + if (rawProp instanceof Boolean && cls == boolean.class) { return (T) rawProp; } + + // Caller wants a number AND we have one. Handle widening and narrowing conversions. + // Char is not included here. It's not a Number and the semantics of converting it to/from a number or a + // string have rough edges. Just ask users to avoid it. + if (rawProp instanceof Number + && ( Number.class.isAssignableFrom(cls) + || ( cls.isPrimitive() && cls != char.class ))) { // We handled boolean above, so if cls is a primitive and not char then it's a number type + if (cls == int.class || cls == Integer.class) { + return (T) Integer.valueOf(((Number) rawProp).intValue()); + } + if (cls == long.class || cls == Long.class) { + return (T) Long.valueOf(((Number) rawProp).longValue()); + } + if (cls == double.class || cls == Double.class) { + return (T) Double.valueOf(((Number) rawProp).doubleValue()); + } + if (cls == float.class || cls == Float.class) { + return (T) Float.valueOf(((Number) rawProp).floatValue()); + } + if (cls == short.class || cls == Short.class) { + return (T) Short.valueOf(((Number) rawProp).shortValue()); + } + if (cls == byte.class || cls == Byte.class) { + return (T) Byte.valueOf(((Number) rawProp).byteValue()); + } + } } + // Nothing matches (ie, caller wants a ParametrizedType, or the rawProp is not easily cast to the desired type) return parseError(key, rawProp.toString(), - new NumberFormatException("Property " + rawProp.toString() + " is of wrong format " + type.getTypeName())); + new IllegalArgumentException("Property " + rawProp + " is not convertible to " + type.getTypeName())); } @Override @@ -284,7 +331,7 @@ public String resolve(String value) { @Override public T resolve(String value, Class type) { - return getDecoder().decode(type, resolve(value)); + return getDecoder().decode((Type) type, resolve(value)); } @Override @@ -384,14 +431,15 @@ public List getList(String key, Class type) { return notFound(key); } String[] parts = value.split(getListDelimiter()); - List result = new ArrayList(); + List result = new ArrayList<>(); for (String part : parts) { - result.add(decoder.decode(type, part)); + result.add(decoder.decode((Type) type, part)); } return result; } @Override + @SuppressWarnings("rawtypes") // Required by legacy API public List getList(String key) { String value = getString(key); if (value == null) { @@ -402,6 +450,7 @@ public List getList(String key) { } @Override + @SuppressWarnings("rawtypes") // Required by legacy API public List getList(String key, List defaultValue) { String value = getString(key, null); if (value == null) { diff --git a/archaius2-core/src/test/java/com/netflix/archaius/config/AbstractConfigTest.java b/archaius2-core/src/test/java/com/netflix/archaius/config/AbstractConfigTest.java index fcb89735..b69ce92d 100644 --- a/archaius2-core/src/test/java/com/netflix/archaius/config/AbstractConfigTest.java +++ b/archaius2-core/src/test/java/com/netflix/archaius/config/AbstractConfigTest.java @@ -16,7 +16,9 @@ package com.netflix.archaius.config; import java.util.Collections; +import java.util.HashMap; import java.util.Iterator; +import java.util.Map; import java.util.function.BiConsumer; import org.junit.jupiter.api.Test; @@ -27,9 +29,20 @@ public class AbstractConfigTest { private final AbstractConfig config = new AbstractConfig() { + private final Map entries = new HashMap<>(); + + { + entries.put("foo", "bar"); + entries.put("byte", (byte) 42); + entries.put("int", 42); + entries.put("long", 42L); + entries.put("float", 42.0f); + entries.put("double", 42.0d); + } + @Override public boolean containsKey(String key) { - return "foo".equals(key); + return entries.containsKey(key); } @Override @@ -39,30 +52,28 @@ public boolean isEmpty() { @Override public Iterator getKeys() { - return Collections.singletonList("foo").iterator(); + return Collections.unmodifiableSet(entries.keySet()).iterator(); } @Override public Object getRawProperty(String key) { - if ("foo".equals(key)) { - return "bar"; - } - return null; + return entries.get(key); } @Override public void forEachProperty(BiConsumer consumer) { - consumer.accept("foo", "bar"); + entries.forEach(consumer); } }; @Test - public void testGet() throws Exception { + public void testGet() { assertEquals("bar", config.get(String.class, "foo")); } @Test public void getExistingProperty() { + //noinspection OptionalGetWithoutIsPresent assertEquals("bar", config.getProperty("foo").get()); } @@ -70,4 +81,61 @@ public void getExistingProperty() { public void getNonExistentProperty() { assertFalse(config.getProperty("non_existent").isPresent()); } + + @Test + public void testGetRawNumerics() { + // First, get each entry as its expected type and the corresponding wrapper. + assertEquals(42, config.get(int.class, "int")); + assertEquals(42, config.get(Integer.class, "int")); + assertEquals(42L, config.get(long.class, "long")); + assertEquals(42L, config.get(Long.class, "long")); + assertEquals((byte) 42, config.get(byte.class, "byte")); + assertEquals((byte) 42, config.get(Byte.class, "byte")); + assertEquals(42.0f, config.get(float.class, "float")); + assertEquals(42.0f, config.get(Float.class, "float")); + assertEquals(42.0d, config.get(double.class, "double")); + assertEquals(42.0d, config.get(Double.class, "double")); + + // Then, get each entry as a string + assertEquals("42", config.get(String.class, "int")); + assertEquals("42", config.get(String.class, "long")); + assertEquals("42", config.get(String.class, "byte")); + assertEquals("42.0", config.get(String.class, "float")); + assertEquals("42.0", config.get(String.class, "double")); + + // Then, narrowed types + assertEquals((byte) 42, config.get(byte.class, "int")); + assertEquals((byte) 42, config.get(byte.class, "long")); + assertEquals((byte) 42, config.get(byte.class, "float")); + assertEquals((byte) 42, config.get(byte.class, "double")); + assertEquals(42.0f, config.get(double.class, "double")); + + // Then, widened + assertEquals(42L, config.get(long.class, "int")); + assertEquals(42L, config.get(long.class, "byte")); + assertEquals(42L, config.get(long.class, "float")); + assertEquals(42L, config.get(long.class, "double")); + assertEquals(42.0d, config.get(double.class, "float")); + + // On floating point + assertEquals(42.0f, config.get(float.class, "int")); + assertEquals(42.0f, config.get(float.class, "byte")); + assertEquals(42.0f, config.get(float.class, "long")); + assertEquals(42.0f, config.get(float.class, "double")); + + // As doubles + assertEquals(42.0d, config.get(double.class, "int")); + assertEquals(42.0d, config.get(double.class, "byte")); + assertEquals(42.0d, config.get(double.class, "long")); + assertEquals(42.0d, config.get(double.class, "float")); + + // Narrowed types in wrapper classes + assertEquals((byte) 42, config.get(Byte.class, "int")); + assertEquals((byte) 42, config.get(Byte.class, "long")); + assertEquals((byte) 42, config.get(Byte.class, "float")); + + // Widened types in wrappers + assertEquals(42L, config.get(Long.class, "int")); + assertEquals(42L, config.get(Long.class, "byte")); + } } diff --git a/archaius2-core/src/test/java/com/netflix/archaius/config/MapConfigTest.java b/archaius2-core/src/test/java/com/netflix/archaius/config/MapConfigTest.java index 5379b1a2..cb0968f5 100644 --- a/archaius2-core/src/test/java/com/netflix/archaius/config/MapConfigTest.java +++ b/archaius2-core/src/test/java/com/netflix/archaius/config/MapConfigTest.java @@ -183,6 +183,15 @@ public void numericInterpolationShouldWork() { assertEquals(123L, (long) config.getLong("value")); } + @Test + public void numericInterpolationShouldWork_withNonStringValues() { + Config config = MapConfig.builder() + .put("default", 123) + .put("value", "${default}") + .build(); + assertEquals(123L, (long) config.getLong("value")); + } + @Test public void getKeys() { Map props = new HashMap<>();