Clarify the behaviour when a converter returns null (#618)

Signed-off-by: Emily Jiang <[email protected]>

api/src/main/java/org/eclipse/microprofile/config/Config.java

@@ -128,7 +128,7 @@ public interface Config {
      * @throws IllegalArgumentException
      *             if the property cannot be converted to the specified type
      * @throws java.util.NoSuchElementException
-     *             if the property is not defined or is defined as an empty string
+     *             if the property is not defined or is defined as an empty string or the converter returns {@code null}
      */
     <T> T getValue(String propertyName, Class<T> propertyType);
 
@@ -169,7 +169,8 @@ public interface Config {
      * @throws java.lang.IllegalArgumentException
      *             if the property values cannot be converted to the specified type
      * @throws java.util.NoSuchElementException
-     *             if the property isn't present in the configuration
+     *             if the property isn't present in the configuration or is defined as an empty string or the converter
+     *             returns {@code null}
      */
     default <T> List<T> getValues(String propertyName, Class<T> propertyType) {
         @SuppressWarnings("unchecked")

api/src/main/java/org/eclipse/microprofile/config/inject/ConfigProperty.java

@@ -70,8 +70,8 @@
  *
  * <p>
  * Contrary to natively injecting, if the property is not specified, this will not lead to a DeploymentException. The
- * following code injects a Long value to the {@code my.optional.long.property}. If the property does not exist, the
- * value {@code 123} will be assigned. to {@code injectedLongValue}.
+ * following code injects a Long value to the {@code my.optional.long.property}. If the property is not defined, the
+ * value {@code 123} will be assigned to {@code injectedLongValue}.
  *
  * <pre>
  * &#064;Inject
@@ -139,12 +139,18 @@
     String name() default "";
 
     /**
-     * The default value if the configured property value does not exist. Empty string as the default value will be
-     * ignored, which is same as not setting the default value.
+     * The default value if the configured property does not exist. This value acts as a configure source with the
+     * lowest ordinal.
      * <p>
-     * If the target Type is not String a proper {@link org.eclipse.microprofile.config.spi.Converter} will get applied.
-     * That means that any default value string should follow the formatting rules of the registered Converters.
-     *
+     * If the target Type is not String, a proper {@link org.eclipse.microprofile.config.spi.Converter} will get
+     * applied.
+     * 
+     * Empty string as the default value will be ignored, which is same as not setting the default value. That means
+     * that any default value string should follow the formatting rules of the registered Converters.
+     * 
+     * If a property has been emptied by a config source with a higher ordinal by setting an empty configuration value
+     * or by using a value causing the used converter returning {@code null}, the default value will not be used.
+     * 
      * @return the default value as a string
      */
     @Nonbinding

spec/src/main/asciidoc/configexamples.asciidoc

@@ -370,6 +370,10 @@ The table below defines the conversion rules, including some special edge case s
 
 |===
 
+=== Remove config properties
+Sometimes, there is a need to remove a property. This can be done by setting an empty value or a value causing the corresponding converter returning `null` in a config source. 
+When injecting a property that has been deleted, `DeploymentException` will be thrown unless the return type is `Optional`. 
+
 === Aggregate related properties into a CDI bean
 
 When injecting a number of related configuration properties, it can be tedious to repeat the statement of `ConfigProperty` in scatter places.

spec/src/main/asciidoc/converters.asciidoc

@@ -102,6 +102,8 @@ If no built-in nor custom `Converter` exists for a requested Type `T`, an implic
 * The target type `T` has a `public static T parse(CharSequence)` method, or
 * The target type `T` has a public Constructor with a `String` parameter
 
+If a converter returns `null` for a given config value, the property will be treated as being deleted. If it is a required property, `NoSuchElementException` will be thrown. Even if `defaultValue` is specified on the property injection, the `defaultValue` will not be used.
+
 === Cleaning up a Converter
 
 If a `Converter` implements the `java.lang.AutoCloseable` interface  then the `close()` method will be called when the underlying `Config` is being released.

tck/src/main/java/org/eclipse/microprofile/config/tck/configsources/DefaultConfigSourceOrdinalTest.java

@@ -82,7 +82,6 @@ public void checkSetup() {
             Assert.fail(
                     "Before running this test, the system property \"customer.hobby\" must be set with the value of Tennis");
         }
-
     }
 
     @Test

tck/src/main/java/org/eclipse/microprofile/config/tck/converters/convertToNull/ConvertedNullValueBrokenInjectionTest.java

@@ -0,0 +1,78 @@
+/*
+ * Copyright (c) 2020 Contributors to the Eclipse Foundation
+ *
+ * See the NOTICE file(s) distributed with this work for additional
+ * information regarding copyright ownership.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * You may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+package org.eclipse.microprofile.config.tck.converters.convertToNull;
+
+import javax.enterprise.context.ApplicationScoped;
+import javax.enterprise.inject.spi.DeploymentException;
+import javax.inject.Inject;
+
+import org.eclipse.microprofile.config.Config;
+import org.eclipse.microprofile.config.inject.ConfigProperty;
+import org.eclipse.microprofile.config.spi.Converter;
+import org.eclipse.microprofile.config.tck.converters.Pizza;
+import org.eclipse.microprofile.config.tck.converters.PizzaConverter;
+import org.jboss.arquillian.container.test.api.Deployment;
+import org.jboss.arquillian.container.test.api.ShouldThrowException;
+import org.jboss.arquillian.testng.Arquillian;
+import org.jboss.shrinkwrap.api.Archive;
+import org.jboss.shrinkwrap.api.ShrinkWrap;
+import org.jboss.shrinkwrap.api.asset.EmptyAsset;
+import org.jboss.shrinkwrap.api.asset.StringAsset;
+import org.jboss.shrinkwrap.api.spec.WebArchive;
+import org.testng.annotations.Test;
+
+/**
+ * This is to test that when a converter returns null for some config value, an exception should be thrown and the
+ * defaultValue should not be used.
+ */
+public class ConvertedNullValueBrokenInjectionTest extends Arquillian {
+    private @Inject Config config;
+    private @Inject MyBean myBean;
+
+    @Deployment
+    @ShouldThrowException(DeploymentException.class)
+    public static Archive deployment() {
+        return ShrinkWrap.create(WebArchive.class, "ConvertedNullValueBrokenInjectionTest.war")
+                .addClasses(ConvertedNullValueBrokenInjectionTest.class, Pizza.class, PizzaConverter.class,
+                        MyBean.class)
+                .addAsResource(
+                        new StringAsset(
+                                "partial.pizza=cheese"),
+                        "META-INF/microprofile-config.properties")
+                .addAsServiceProvider(Converter.class, PizzaConverter.class)
+                .addAsWebInfResource(EmptyAsset.INSTANCE, "beans.xml");
+
+    }
+
+    @Test
+    public void test() {
+    }
+
+    @ApplicationScoped
+    public static class MyBean {
+
+        private @Inject @ConfigProperty(name = "partial.pizza", defaultValue = "medium:chicken") Pizza myPizza;
+
+        public Pizza getPizza() {
+            return myPizza;
+        }
+    }
+
+}

tck/src/main/java/org/eclipse/microprofile/config/tck/converters/convertToNull/ConvertedNullValueTest.java

@@ -0,0 +1,96 @@
+/*
+ * Copyright (c) 2020 Contributors to the Eclipse Foundation
+ *
+ * See the NOTICE file(s) distributed with this work for additional
+ * information regarding copyright ownership.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * You may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+package org.eclipse.microprofile.config.tck.converters.convertToNull;
+
+import java.util.NoSuchElementException;
+import java.util.Optional;
+
+import javax.enterprise.context.ApplicationScoped;
+import javax.inject.Inject;
+
+import org.eclipse.microprofile.config.Config;
+import org.eclipse.microprofile.config.inject.ConfigProperty;
+import org.eclipse.microprofile.config.spi.Converter;
+import org.eclipse.microprofile.config.tck.converters.Pizza;
+import org.eclipse.microprofile.config.tck.converters.PizzaConverter;
+import org.jboss.arquillian.container.test.api.Deployment;
+import org.jboss.arquillian.testng.Arquillian;
+import org.jboss.shrinkwrap.api.Archive;
+import org.jboss.shrinkwrap.api.ShrinkWrap;
+import org.jboss.shrinkwrap.api.asset.EmptyAsset;
+import org.jboss.shrinkwrap.api.asset.StringAsset;
+import org.jboss.shrinkwrap.api.spec.WebArchive;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+/**
+ * This is to test that when a converter returns null for some config value, null will be assigned. This means the
+ * property still exists but the value is set to null on purpose.
+ */
+public class ConvertedNullValueTest extends Arquillian {
+    private @Inject Config config;
+    private @Inject MyBean myBean;
+
+    @Deployment
+    public static Archive deployment() {
+        return ShrinkWrap
+                .create(WebArchive.class, "ConvertedNullValueTest.war")
+                .addClasses(ConvertedNullValueTest.class, Pizza.class, PizzaConverter.class, MyBean.class)
+                .addAsResource(
+                        new StringAsset(
+                                "partial.pizza=cheese"),
+                        "META-INF/microprofile-config.properties")
+                .addAsServiceProvider(Converter.class, PizzaConverter.class)
+                .addAsWebInfResource(EmptyAsset.INSTANCE, "beans.xml");
+
+    }
+
+    @Test
+    public void testDefaultValueNotUsed() {
+        // The converter returns null as the converter returns null if the property does not contain :
+        // This will treat as if the property has been removed. The defaultValue will not be used.
+        Assert.assertNull(myBean.getPizza());
+    }
+
+    @Test
+    public void testGetValue() {
+        // The converter returns null as the converter returns null if the property does not contain :
+        // Therefore, it will be treated as non-existence.
+        Assert.assertThrows(NoSuchElementException.class, () -> config.getValue("partial.pizza", Pizza.class));
+    }
+
+    @Test
+    public void testGetOptionalValue() {
+        // The converter returns null as the converter returns null if the property does not contain :
+        // Therefore, it will be treated as non-existence.
+        Assert.assertFalse(config.getOptionalValue("partial.pizza", Pizza.class).isPresent());
+    }
+
+    @ApplicationScoped
+    public static class MyBean {
+
+        private @Inject @ConfigProperty(name = "partial.pizza", defaultValue = "medium:chicken") Optional<Pizza> myPizza;
+
+        public Pizza getPizza() {
+            return myPizza.isPresent() ? myPizza.get() : null;
+        }
+    }
+
+}