From bc0ea9932f29da9ced7a323f75741cd8b33dbe38 Mon Sep 17 00:00:00 2001 From: Emily Jiang Date: Mon, 26 Oct 2020 21:19:56 +0000 Subject: [PATCH] Clarify the behaviour when a converter returns null (#618) Signed-off-by: Emily Jiang --- .../eclipse/microprofile/config/Config.java | 5 +- .../config/inject/ConfigProperty.java | 20 ++-- .../src/main/asciidoc/configexamples.asciidoc | 4 + spec/src/main/asciidoc/converters.asciidoc | 2 + .../DefaultConfigSourceOrdinalTest.java | 1 - ...ConvertedNullValueBrokenInjectionTest.java | 78 +++++++++++++++ .../convertToNull/ConvertedNullValueTest.java | 96 +++++++++++++++++++ 7 files changed, 196 insertions(+), 10 deletions(-) create mode 100644 tck/src/main/java/org/eclipse/microprofile/config/tck/converters/convertToNull/ConvertedNullValueBrokenInjectionTest.java create mode 100644 tck/src/main/java/org/eclipse/microprofile/config/tck/converters/convertToNull/ConvertedNullValueTest.java diff --git a/api/src/main/java/org/eclipse/microprofile/config/Config.java b/api/src/main/java/org/eclipse/microprofile/config/Config.java index 6e167857..c90a5e23 100644 --- a/api/src/main/java/org/eclipse/microprofile/config/Config.java +++ b/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 getValue(String propertyName, Class 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 List getValues(String propertyName, Class propertyType) { @SuppressWarnings("unchecked") diff --git a/api/src/main/java/org/eclipse/microprofile/config/inject/ConfigProperty.java b/api/src/main/java/org/eclipse/microprofile/config/inject/ConfigProperty.java index 4042ecf3..13ba74c1 100644 --- a/api/src/main/java/org/eclipse/microprofile/config/inject/ConfigProperty.java +++ b/api/src/main/java/org/eclipse/microprofile/config/inject/ConfigProperty.java @@ -70,8 +70,8 @@ * *

* 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}. * * 

  * @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.
      * 

-     * 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
diff --git a/spec/src/main/asciidoc/configexamples.asciidoc b/spec/src/main/asciidoc/configexamples.asciidoc
index 18782641..f66c33e2 100644
--- a/spec/src/main/asciidoc/configexamples.asciidoc
+++ b/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.
diff --git a/spec/src/main/asciidoc/converters.asciidoc b/spec/src/main/asciidoc/converters.asciidoc
index 17aca0b9..9e6105cb 100644
--- a/spec/src/main/asciidoc/converters.asciidoc
+++ b/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.
diff --git a/tck/src/main/java/org/eclipse/microprofile/config/tck/configsources/DefaultConfigSourceOrdinalTest.java b/tck/src/main/java/org/eclipse/microprofile/config/tck/configsources/DefaultConfigSourceOrdinalTest.java
index 514b0535..1cfb88d7 100644
--- a/tck/src/main/java/org/eclipse/microprofile/config/tck/configsources/DefaultConfigSourceOrdinalTest.java
+++ b/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
diff --git a/tck/src/main/java/org/eclipse/microprofile/config/tck/converters/convertToNull/ConvertedNullValueBrokenInjectionTest.java b/tck/src/main/java/org/eclipse/microprofile/config/tck/converters/convertToNull/ConvertedNullValueBrokenInjectionTest.java
new file mode 100644
index 00000000..a18e034e
--- /dev/null
+++ b/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;
+        }
+    }
+
+}
diff --git a/tck/src/main/java/org/eclipse/microprofile/config/tck/converters/convertToNull/ConvertedNullValueTest.java b/tck/src/main/java/org/eclipse/microprofile/config/tck/converters/convertToNull/ConvertedNullValueTest.java
new file mode 100644
index 00000000..ef3052fa
--- /dev/null
+++ b/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 myPizza;
+
+        public Pizza getPizza() {
+            return myPizza.isPresent() ? myPizza.get() : null;
+        }
+    }
+
+}