Add more Exception messages for Config.getValue() (#504)

* Add more Exception messages for Config.getValue()

Signed-off-by: Joseph Cass <[email protected]>

implementation/src/main/java/io/smallrye/config/ConfigMessages.java

@@ -57,7 +57,8 @@ public interface ConfigMessages {
     @Message(id = 13, value = "No Converter registered for %s")
     IllegalArgumentException noRegisteredConverter(Class<?> type);
 
-    @Message(id = 14, value = "Property %s is required but the value was not found or is empty")
+    // Returns a String rather than a NoSuchElementException for a slight performance improvement as throwing this exception could be quite common.
+    @Message(id = 14, value = "The config property %s is required but it could not be found in any config source")
     String propertyNotFound(String name);
 
     @Message(id = 15, value = "No configuration is available for this class loader")
@@ -131,4 +132,14 @@ public interface ConfigMessages {
 
     @Message(id = 38, value = "Type has no raw type class: %s")
     IllegalArgumentException noRawType(Type type);
+
+    @Message(id = 39, value = "The config property %s with the config value \"%s\" threw an Exception whilst being converted")
+    IllegalArgumentException converterException(@Cause Throwable converterException, String configProperty, String configValue);
+
+    @Message(id = 40, value = "The config property %s is defined as the empty String (\"\") which the following Converter considered to be null: %s")
+    NoSuchElementException propertyEmptyString(String configPropertyName, String converter);
+
+    @Message(id = 41, value = "The config property %s with the config value \"%s\" was converted to null from the following Converter: %s")
+    NoSuchElementException converterReturnedNull(String configPropertyName, String configValue, String converter);
+
 }

implementation/src/main/java/io/smallrye/config/SmallRyeConfig.java

@@ -186,6 +186,23 @@ public <T> T getValue(String name, Class<T> aClass) {
         return getValue(name, requireConverter(aClass));
     }
 
+    /**
+     * 
+     * This method handles calls from both {@link Config#getValue} and {@link Config#getOptionalValue}.<br>
+     * <br>
+     * 
+     * Calls from {@link Config#getValue} should throw an {@link Exception} for each of the following:<br>
+     * 
+     * 1. {@link IllegalArgumentException} - if the property cannot be converted by the {@link Converter} to the specified type
+     * <br>
+     * 2. {@link NoSuchElementException} - if the property is not defined <br>
+     * 3. {@link NoSuchElementException} - if the property is defined as an empty string <br>
+     * 4. {@link NoSuchElementException} - if the {@link Converter} returns {@code null} <br>
+     * <br>
+     * 
+     * Calls from {@link Config#getOptionalValue} should only throw an {@link Exception} for #1
+     * ({@link IllegalArgumentException} when the property cannot be converted to the specified type).
+     */
     @SuppressWarnings("unchecked")
     public <T> T getValue(String name, Converter<T> converter) {
         ConfigValue configValue = getConfigValue(name);
@@ -200,20 +217,35 @@ public <T> T getValue(String name, Converter<T> converter) {
             }
         }
 
-        String value = configValue.getValue();
+        String value = configValue.getValue(); // Can return the empty String (which is not considered as null)
+
         final T converted;
+
         if (value != null) {
-            converted = converter.convert(value);
+            try {
+                converted = converter.convert(value);
+            } catch (IllegalArgumentException e) {
+                throw ConfigMessages.msg.converterException(e, name, value); // 1
+            }
         } else {
             try {
+                // See if the Converter is designed to handle a missing (null) value i.e. Optional Converters
                 converted = converter.convert("");
             } catch (IllegalArgumentException ignored) {
-                throw new NoSuchElementException(ConfigMessages.msg.propertyNotFound(name));
+                throw new NoSuchElementException(ConfigMessages.msg.propertyNotFound(name)); // 2
             }
         }
+
         if (converted == null) {
-            throw new NoSuchElementException(ConfigMessages.msg.propertyNotFound(name));
+            if (value == null) {
+                throw new NoSuchElementException(ConfigMessages.msg.propertyNotFound(name)); // 2
+            } else if (value.length() == 0) {
+                throw ConfigMessages.msg.propertyEmptyString(name, converter.getClass().getTypeName()); // 3
+            } else {
+                throw ConfigMessages.msg.converterReturnedNull(name, value, converter.getClass().getTypeName()); // 4
+            }
         }
+
         return converted;
     }
 

implementation/src/test/java/io/smallrye/config/ConfigValueConversionRulesExceptionsTest.java

@@ -0,0 +1,102 @@
+package io.smallrye.config;
+
+import static io.smallrye.config.KeyValuesConfigSource.config;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+import java.util.NoSuchElementException;
+
+import org.junit.jupiter.api.Test;
+
+/**
+ * 
+ * Test sensible error messages are output when edge case config value conversion Exceptions occur
+ *
+ */
+class ConfigValueConversionRulesExceptionsTest {
+
+    @Test
+    void missingString() {
+        final SmallRyeConfig config = new SmallRyeConfigBuilder().build();
+        final NoSuchElementException exception = assertThrows(NoSuchElementException.class,
+                () -> config.getValue("none.existing.prop", String.class));
+        assertEquals(
+                "SRCFG00014: The config property none.existing.prop is required but it could not be found in any config source",
+                exception.getMessage());
+    }
+
+    @Test
+    void missingStringArray() {
+        final SmallRyeConfig config = new SmallRyeConfigBuilder().build();
+        final NoSuchElementException exception = assertThrows(NoSuchElementException.class,
+                () -> config.getValue("none.existing.array.prop", String[].class));
+        assertEquals(
+                "SRCFG00014: The config property none.existing.array.prop is required but it could not be found in any config source",
+                exception.getMessage());
+    }
+
+    @Test
+    void emptyString() {
+        final SmallRyeConfig config = new SmallRyeConfigBuilder()
+                .withSources(config("empty.string", "")).build();
+        final NoSuchElementException exception = assertThrows(NoSuchElementException.class,
+                () -> config.getValue("empty.string", String.class));
+        assertEquals(
+                "SRCFG00040: The config property empty.string is defined as the empty String (\"\") which the following Converter considered to be null: io.smallrye.config.Converters$BuiltInConverter",
+                exception.getMessage());
+    }
+
+    @Test
+    void emptyStringArray() {
+        final SmallRyeConfig config = new SmallRyeConfigBuilder()
+                .withSources(config("empty.string.array", "")).build();
+        final NoSuchElementException exception = assertThrows(NoSuchElementException.class,
+                () -> config.getValue("empty.string.array", String[].class));
+        assertEquals(
+                "SRCFG00040: The config property empty.string.array is defined as the empty String (\"\") which the following Converter considered to be null: io.smallrye.config.Converters$ArrayConverter",
+                exception.getMessage());
+    }
+
+    @Test
+    void commaStringArray() {
+        final SmallRyeConfig config = new SmallRyeConfigBuilder()
+                .withSources(config("comma.string.array", ",")).build();
+        final NoSuchElementException exception = assertThrows(NoSuchElementException.class,
+                () -> config.getValue("comma.string.array", String[].class));
+        assertEquals(
+                "SRCFG00041: The config property comma.string.array with the config value \",\" was converted to null from the following Converter: io.smallrye.config.Converters$ArrayConverter",
+                exception.getMessage());
+    }
+
+    @Test
+    void doubleCommaStringArray() {
+        final SmallRyeConfig config = new SmallRyeConfigBuilder()
+                .withSources(config("double.comma.string.array", ",,")).build();
+        final NoSuchElementException exception = assertThrows(NoSuchElementException.class,
+                () -> config.getValue("double.comma.string.array", String[].class));
+        assertEquals(
+                "SRCFG00041: The config property double.comma.string.array with the config value \",,\" was converted to null from the following Converter: io.smallrye.config.Converters$ArrayConverter",
+                exception.getMessage());
+    }
+
+    @Test
+    void missingStringWithBadDefault() {
+        final SmallRyeConfig config = new SmallRyeConfigBuilder().withDefaultValue("bad.default.value", "").build();
+        final NoSuchElementException exception = assertThrows(NoSuchElementException.class,
+                () -> config.getValue("bad.default.value", String.class));
+        assertEquals(
+                "SRCFG00040: The config property bad.default.value is defined as the empty String (\"\") which the following Converter considered to be null: io.smallrye.config.Converters$BuiltInConverter",
+                exception.getMessage());
+    }
+
+    @Test
+    void badConversion() throws Exception {
+        final SmallRyeConfig config = new SmallRyeConfigBuilder().withDefaultValue("not.an.Integer", "notInt").build();
+        final Exception exception = assertThrows(IllegalArgumentException.class,
+                () -> config.getValue("not.an.Integer", Integer.class));
+        assertEquals(
+                "SRCFG00039: The config property not.an.Integer with the config value \"notInt\" threw an Exception whilst being converted",
+                exception.getMessage());
+        assertEquals("SRCFG00029: Expected an integer value, got \"notInt\"", exception.getCause().getMessage());
+    }
+}

implementation/src/test/java/io/smallrye/config/CustomConverterTest.java

@@ -122,9 +122,10 @@ void malformedUUID() {
         IllegalArgumentException thrownException = assertThrows(IllegalArgumentException.class, () -> {
             config.getValue("uuid.invalid", UUID.class);
         }, "Malformed UUID should throw exception");
-        assertEquals("SRCFG00026: notauuid cannot be converted into a UUID", thrownException.getMessage(),
-                "Exception message is incorrect");
-
+        assertEquals(
+                "SRCFG00039: The config property uuid.invalid with the config value \"notauuid\" threw an Exception whilst being converted",
+                thrownException.getMessage());
+        assertEquals("SRCFG00026: notauuid cannot be converted into a UUID", thrownException.getCause().getMessage());
     }
 
     private static Config buildConfig(String... keyValues) {