From 0aeb8cd8f5cf8f4f812a3eeebd22d3512ecfa6ee Mon Sep 17 00:00:00 2001 From: Jeff Mesnil Date: Fri, 15 Feb 2019 11:15:33 +0100 Subject: [PATCH] Speficy handling of empty config property * [Spec] add section explaining how the Config API handles a config property with missing or empty value * [Spec] mention that it is possible to set an empty value to reset a property from a lower-ordinal config source. * [TCK] add EmptyConfigPropertyTest to test all cases of empty config property Signed-off-by: Jeff Mesnil --- spec/src/main/asciidoc/converters.asciidoc | 41 ++++ .../config/tck/EmptyConfigPropertyBean.java | 57 +++++ .../config/tck/EmptyConfigPropertyTest.java | 206 ++++++++++++++++++ .../config/tck/EmptyValuesTest.java | 40 +++- .../configsources/KeyValuesConfigSource.java | 81 +++++++ .../META-INF/microprofile-config.properties | 5 +- 6 files changed, 417 insertions(+), 13 deletions(-) create mode 100644 tck/src/main/java/org/eclipse/microprofile/config/tck/EmptyConfigPropertyBean.java create mode 100644 tck/src/main/java/org/eclipse/microprofile/config/tck/EmptyConfigPropertyTest.java create mode 100644 tck/src/main/java/org/eclipse/microprofile/config/tck/configsources/KeyValuesConfigSource.java diff --git a/spec/src/main/asciidoc/converters.asciidoc b/spec/src/main/asciidoc/converters.asciidoc index 08b9dc0f..1e2d7d55 100644 --- a/spec/src/main/asciidoc/converters.asciidoc +++ b/spec/src/main/asciidoc/converters.asciidoc @@ -104,3 +104,44 @@ If no built-in nor custom `Converter` exists for a requested Type `T`, an implic If a `Converter` implements the `java.lang.AutoCloseable` interface then the `close()` method will be called when the underlying `Config` is being released. +=== Empty value + +A config property can be configured with an empty value (the empty String `""`). + +The config API interprets an empty string as a missing property and throws a `NoSuchElementException` when the output type is a `String`. + +[NOTE] +This behaviour allows to _unconfigure_ a property. If a property is configured in a lower-ordinal ConfigSource with +an undesirable value, you can set the property value to an empty string in a higher-ordinal ConfigSource to discard the lower-ordinal value. + +The Config API also interprets an empty string as an empty array when the output type is `String[]`. +For consistency, the Config API also discards any leading and trailing commas `","` when the output type is a `String[]`` + +[[empty_value_table]] +.Examples of Config API +[options="header"] +|======================= +| Property value | Output type | `Config` method | Output result +| missing | `String` | `getValue` | throws `NoSuchElementException` +| `""` (empty string) | `String` | `getValue` | throws `NoSuchElementException` +| `" "` | `String` | `getValue` | `" "` +| `"foo"` | `String` | `getValue` | `"foo"` +| missing | `String[]` | `getValue` | `{ }` (empty array) +| `""` | `String[]` | `getValue` | `{ }` +| `" "` | `String[]` | `getValue` | `{ " " }` +| `","` | `String[]` | `getValue` | `{ }` +| `",,"` | `String[]` | `getValue` | `{ }` +| `"foo"` | `String[]` | `getValue` | `{ "foo" }` +| `"foo,bar"` | `String[]` | `getValue` | `{ "foo", "bar" }` +| `"foo,"` | `String[]` | `getValue` | `{ "foo" }` +| `",bar"` | `String[]` | `getValue` | `{ "bar" }` +| `",bar,"` | `String[]` | `getValue` | `{ "bar" }` +| missing | `String` |`getOptionalValue` | `Optional.empty()` +| `""` | `String` | `getOptionalValue` | `Optional.empty()` +| `" "` | `String` | `getOptionalValue` | `Optional.of(" ")` +| `"foo"` | `String` | `getOptionalValue` | `Optional.of("foo")` +| missing | `String[]` | `getOptionalValue` | `Optional.empty()` +| `""` | `String[]` | `getOptionalValue` | `Optional.empty()` +| `","` | `String[]` | `getOptionalValue` | `Optional.empty()` +| `",,"` | `String[]` | `getOptionalValue` | `Optional.empty()` +| `"foo,bar"` | `String[]` | `getOptionalValue` | `Optional.of({ "foo", "bar" })` diff --git a/tck/src/main/java/org/eclipse/microprofile/config/tck/EmptyConfigPropertyBean.java b/tck/src/main/java/org/eclipse/microprofile/config/tck/EmptyConfigPropertyBean.java new file mode 100644 index 00000000..21cb0459 --- /dev/null +++ b/tck/src/main/java/org/eclipse/microprofile/config/tck/EmptyConfigPropertyBean.java @@ -0,0 +1,57 @@ +/* + * Copyright (c) 2019 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; + +import static org.eclipse.microprofile.config.tck.EmptyConfigPropertyTest.EMPTY_PROP; + +import java.util.List; +import java.util.Set; + +import javax.enterprise.context.Dependent; +import javax.inject.Inject; + +import org.eclipse.microprofile.config.inject.ConfigProperty; + +@Dependent +public class EmptyConfigPropertyBean { + + @Inject + @ConfigProperty(name = EMPTY_PROP) + private String[] myStrings; + + @Inject + @ConfigProperty(name = EMPTY_PROP) + private List myStringList; + + @Inject + @ConfigProperty(name = EMPTY_PROP) + private Set myStringSet; + + public String[] getMyStrings() { + return myStrings; + } + + public List getMyStringList() { + return myStringList; + } + + public Set getMyStringSet() { + return myStringSet; + } +} diff --git a/tck/src/main/java/org/eclipse/microprofile/config/tck/EmptyConfigPropertyTest.java b/tck/src/main/java/org/eclipse/microprofile/config/tck/EmptyConfigPropertyTest.java new file mode 100644 index 00000000..46bb132c --- /dev/null +++ b/tck/src/main/java/org/eclipse/microprofile/config/tck/EmptyConfigPropertyTest.java @@ -0,0 +1,206 @@ +/* + * Copyright (c) 2016-2017 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; + +import static org.eclipse.microprofile.config.tck.base.AbstractTest.addFile; +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.fail; + +import java.util.NoSuchElementException; +import java.util.Optional; + +import javax.inject.Inject; + +import org.eclipse.microprofile.config.Config; +import org.eclipse.microprofile.config.spi.ConfigProviderResolver; +import org.eclipse.microprofile.config.tck.configsources.KeyValuesConfigSource; +import org.jboss.arquillian.container.test.api.Deployment; +import org.jboss.arquillian.testng.Arquillian; +import org.jboss.shrinkwrap.api.ShrinkWrap; +import org.jboss.shrinkwrap.api.asset.EmptyAsset; +import org.jboss.shrinkwrap.api.spec.JavaArchive; +import org.jboss.shrinkwrap.api.spec.WebArchive; +import org.testng.Assert; +import org.testng.annotations.Test; + +public class EmptyConfigPropertyTest extends Arquillian { + + @Deployment + public static WebArchive deploy() { + JavaArchive testJar = ShrinkWrap + .create(JavaArchive.class, "EmptyConfigPropertyTest.jar") + .addClass(EmptyConfigPropertyBean.class) + .addClass(KeyValuesConfigSource.class) + .addAsManifestResource(EmptyAsset.INSTANCE, "beans.xml") + .as(JavaArchive.class); + + addFile(testJar, "META-INF/microprofile-config.properties"); + + WebArchive war = ShrinkWrap + .create(WebArchive.class, "EmptyConfigPropertyTest.war") + .addAsLibrary(testJar); + return war; + } + + + private @Inject + Config config; + + @Inject + private EmptyConfigPropertyBean bean; + + static final String EMPTY_PROP = "tck.config.empty"; + static final String MISSING_PROP = "tck.config.this.property.is.not.defined.anywhere"; + + @Test + public void testEmptyPropertyWithConfig() { + // | Property value | Output type | `Config` method | Output result + // | `""` (empty string) | `String` | `getValue` | throws `NoSuchElementException` + try { + config.getValue(EMPTY_PROP, String.class); + Assert.fail("An empty string is considered as missing and must raise a NoSuchElementException"); + } + catch (NoSuchElementException e) { + } + try { + config.getValue(EMPTY_PROP, Double.class); + Assert.fail(("an empty String is considered missing regardless of the ouput type")); + } + catch (NoSuchElementException e) { + } + + // but if it is accessed an array, it will returns an empty array + // | `""` | `String[]` | `getValue` | `{ }` + assertEquals(config.getValue(EMPTY_PROP, String[].class).length, 0); + // regardless of the output type + assertEquals(config.getValue(EMPTY_PROP, Double[].class).length, 0); + + + // | `""` | `String` | `getOptionalValue` | `Optional.empty()` + assertEquals(config.getOptionalValue(EMPTY_PROP, String.class), Optional.empty()); + assertEquals(config.getOptionalValue(EMPTY_PROP, Double.class), Optional.empty()); + + // | `""` | `String[]` | `getOptionalValue` | `Optional.empty()` + assertEquals( config.getOptionalValue(EMPTY_PROP, String[].class), Optional.empty()); + assertEquals( config.getOptionalValue(EMPTY_PROP, Double[].class), Optional.empty()); + } + + @Test + public void testMissingProperty() { + // | Property value | Output type | `Config` method | Output result + // | missing | `String` | `getValue` | throws `NoSuchElementException` + try { + config.getValue(MISSING_PROP, String.class); + Assert.fail("A missing property must raise a NoSuchElementException"); + } + catch (NoSuchElementException e) { + } + // | missing | `String[]` | `getValue` | `{ }` (empty array) + assertEquals(config.getValue(MISSING_PROP, String[].class).length, 0); + // regardless of the output type + assertEquals(config.getValue(MISSING_PROP, Double[].class).length, 0); + + // | missing | `String` |`getOptionalValue` | `Optional.empty()` + assertEquals( config.getOptionalValue(MISSING_PROP, String.class), Optional.empty()); + // | missing | `String[]` | `getOptionalValue` | `Optional.empty()` + assertEquals( config.getOptionalValue(MISSING_PROP, String[].class), Optional.empty()); + } + + @Test + public void testArrayCommasRules() { + Config config = KeyValuesConfigSource.buildConfig( + "arrayWithSpace", " ", + "arrayWithSingleComma", ",", + "arrayWithTwoCommas", ",,", + "arrayWithSingleValue", "foo", + "arrayWithTwoValues", "foo,bar", + "arrayWithTrailingComma", "foo,", + "arrayWithLeadingComma", ",bar", + "arrayWithLeadingAndTrailingCommas", ",bar,"); + + // | Property value | Output type | `Config` method | Output result + // | `" "` | `String[]` | `getValue` | `{ " " }` + assertEquals(config.getValue("arrayWithSpace", String[].class), new String[] { " " }); + // | `","` | `String[]` | `getValue` | `{ }` + assertEquals(config.getValue("arrayWithSingleComma", String[].class).length, 0); + // | `",,"` | `String[]` | `getValue` | `{ }` + assertEquals(config.getValue("arrayWithTwoCommas", String[].class).length, 0); + // | `"foo"` | `String[]` | `getValue` | `{ "foo" }` + assertEquals(config.getValue("arrayWithSingleValue", String[].class), new String[] { "foo" }); + // | `"foo,bar"` | `String[]` | `getValue` | `{ "foo", "bar" }` + assertEquals(config.getValue("arrayWithTwoValues", String[].class), new String[] { "foo", "bar" }); + // | `"foo,"` | `String[]` | `getValue` | `{ "foo" }` + assertEquals(config.getValue("arrayWithTrailingComma", String[].class), new String[] { "foo" }); + // | `",bar"` | `String[]` | `getValue` | `{ "bar" }` + assertEquals(config.getValue("arrayWithLeadingComma", String[].class), new String[] { "bar" }); + // | `",bar,"` | `String[]` | `getValue` | `{ "bar" }` + assertEquals(config.getValue("arrayWithLeadingAndTrailingCommas", String[].class), new String[] { "bar" }); + + // | `","` | `String[]` | `getOptionalValue` | `Optional.empty()` + assertEquals(config.getOptionalValue("arrayWithSingleComma", String[].class), Optional.empty()); + // | `",,"` | `String[]` | `getOptionalValue` | `Optional.empty()` + assertEquals(config.getOptionalValue("arrayWithTwoCommas", String[].class), Optional.empty()); + } + + @Test + public void testEmptyPropertyWithConfigAccessor() { + try { + config.access(EMPTY_PROP, String.class).build().getValue(); + Assert.fail("An empty string is considered as missing and must raise a NoSuchElementException"); + } + catch (NoSuchElementException e) { + + } + assertEquals(config.access(EMPTY_PROP, String.class).build().getOptionalValue(), Optional.empty()); + + assertEquals(config.access(EMPTY_PROP, String[].class).build().getValue().length, 0); + assertEquals(config.access(EMPTY_PROP, String[].class).build().getOptionalValue(), Optional.empty()); + } + + @Test + public void testEmptyPropertyWithInjectedConfigProperty() { + assertEquals(bean.getMyStrings().length, 0); + assertEquals(bean.getMyStringList().size(), 0); + assertEquals(bean.getMyStringSet().size(), 0); + } + + @Test + public void testEmptyPropertyResetsLowerOrdinalProperty() { + Config config = ConfigProviderResolver.instance().getBuilder() + .withSources( + // lower-ordinal configures a prop + KeyValuesConfigSource.config(10, + "my.prop", "foo"), + // higher-ordinal resets it with an empty string + KeyValuesConfigSource.config(20, + "my.prop", "")) + .build(); + + try { + config.getValue("my.prop", String.class); + fail("The property must be reset by the empty string in the higher-ordinal config source"); + } + catch (NoSuchElementException e) { + } + + assertEquals(config.getOptionalValue("my.prop", String.class), Optional.empty()); + } + +} diff --git a/tck/src/main/java/org/eclipse/microprofile/config/tck/EmptyValuesTest.java b/tck/src/main/java/org/eclipse/microprofile/config/tck/EmptyValuesTest.java index 99b4efdf..2e4d890b 100644 --- a/tck/src/main/java/org/eclipse/microprofile/config/tck/EmptyValuesTest.java +++ b/tck/src/main/java/org/eclipse/microprofile/config/tck/EmptyValuesTest.java @@ -19,6 +19,12 @@ */ package org.eclipse.microprofile.config.tck; +import static org.testng.Assert.assertEquals; + +import java.util.NoSuchElementException; + +import javax.inject.Inject; + import org.eclipse.microprofile.config.Config; import org.jboss.arquillian.container.test.api.Deployment; import org.jboss.arquillian.testng.Arquillian; @@ -30,10 +36,6 @@ import org.jboss.shrinkwrap.api.spec.WebArchive; import org.testng.annotations.Test; -import javax.inject.Inject; - -import static org.testng.Assert.assertEquals; - public class EmptyValuesTest extends Arquillian { private static final String EMPTY_PROPERTY = "my.empty.property"; @@ -63,17 +65,31 @@ public void testEmptyStringValues() { assertEquals(emptyValuesBean.getStringValue(), ""); } - @Test + @Test(expectedExceptions = NoSuchElementException.class) public void testEmptyStringProgrammaticLookup() { - System.setProperty(EMPTY_PROPERTY, ""); - String stringValue = config.getValue(EMPTY_PROPERTY, String.class); - assertEquals(stringValue, ""); - System.clearProperty(EMPTY_PROPERTY); + try { + System.setProperty(EMPTY_PROPERTY, ""); + config.getValue(EMPTY_PROPERTY, String.class); + } + finally { + System.clearProperty(EMPTY_PROPERTY); + } } - @Test + @Test(expectedExceptions = NoSuchElementException.class) public void testEmptyStringPropertyFromConfigFile() { - String stringValue = config.getValue(PROP_FILE_EMPTY_PROPERTY, String.class); - assertEquals(stringValue, ""); + config.getValue(PROP_FILE_EMPTY_PROPERTY, String.class); + } + + @Test + public void testEmptyStringLookupAsArray() { + try { + System.setProperty(EMPTY_PROPERTY, ""); + String[] result = config.getValue(EMPTY_PROPERTY, String[].class); + assertEquals(result.length, 0); + } + finally { + System.clearProperty(EMPTY_PROPERTY); + } } } diff --git a/tck/src/main/java/org/eclipse/microprofile/config/tck/configsources/KeyValuesConfigSource.java b/tck/src/main/java/org/eclipse/microprofile/config/tck/configsources/KeyValuesConfigSource.java new file mode 100644 index 00000000..37b703b1 --- /dev/null +++ b/tck/src/main/java/org/eclipse/microprofile/config/tck/configsources/KeyValuesConfigSource.java @@ -0,0 +1,81 @@ +/* + * Copyright (c) 2016-2019 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.configsources; + +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; + +import org.eclipse.microprofile.config.Config; +import org.eclipse.microprofile.config.spi.ConfigProviderResolver; +import org.eclipse.microprofile.config.spi.ConfigSource; + +public class KeyValuesConfigSource implements ConfigSource { + + + private final Map properties = new HashMap<>(); + private final int ordinal; + + private KeyValuesConfigSource(Map properties, int ordinal) { + this.properties.putAll(properties); + this.ordinal = ordinal; + } + + @Override + public Map getProperties() { + return Collections.unmodifiableMap(properties); + } + + @Override + public String getValue(String propertyName) { + return properties.get(propertyName); + } + + @Override + public int getOrdinal() { + return ordinal; + } + + @Override + public String getName() { + return "KeyValuesConfigSource"; + } + + public static ConfigSource config(String... keyValues) { + return config(DEFAULT_ORDINAL, keyValues); + } + + public static ConfigSource config(int ordinal, String... keyValues) { + if (keyValues.length %2 != 0) { + throw new IllegalArgumentException("keyValues array must be a multiple of 2"); + } + + Map props = new HashMap<>(); + for (int i = 0; i < keyValues.length; i += 2) { + props.put(keyValues[i], keyValues[i+1]); + } + return new KeyValuesConfigSource(props, ordinal); + } + + public static Config buildConfig(String... keyValues) { + return ConfigProviderResolver.instance().getBuilder() + .withSources(KeyValuesConfigSource.config(keyValues)) + .build(); + } +} diff --git a/tck/src/main/resources/internal/META-INF/microprofile-config.properties b/tck/src/main/resources/internal/META-INF/microprofile-config.properties index 3705142f..10b45c54 100644 --- a/tck/src/main/resources/internal/META-INF/microprofile-config.properties +++ b/tck/src/main/resources/internal/META-INF/microprofile-config.properties @@ -159,4 +159,7 @@ tck.config.test.javaconfig.converter.class.array=org.eclipse.microprofile.config # variable replacement rests tck.config.variable.baseHost = some.host.name tck.config.variable.firstEndpoint = http://${tck.config.variable.baseHost}/endpointOne -tck.config.variable.secondEndpoint = http://${tck.config.variable.baseHost}/endpointTwo \ No newline at end of file +tck.config.variable.secondEndpoint = http://${tck.config.variable.baseHost}/endpointTwo + +# empty config property test +tck.config.empty=