-
Notifications
You must be signed in to change notification settings - Fork 115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Establish consistent behavior for empty value handling #541
Conversation
This first draft only addresses documentation. TCK testing will come in a follow-up commit in this PR before it is taken out of "draft" state. |
@@ -94,18 +95,22 @@ | |||
* The configuration value is not guaranteed to be cached by the implementation, and may be expensive | |||
* to compute; therefore, if the returned value is intended to be frequently used, callers should consider storing | |||
* rather than recomputing it. | |||
* <p> | |||
* The result of this method is identical to the result of calling {@code getOptionalValue(propertyName, propertyType).get()}. | |||
* In particular, if the value element of the given property name does not exist, a {@link NoSuchElementException} is thrown. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about:
If the given property name or the value element of the give property name does not exist, a {@link NoSuchElementException} is thrown.
* @throws java.util.NoSuchElementException if the property isn't present in the configuration | ||
* @return the resolved property value as an instance of the requested type (not {@code null}) | ||
* @throws IllegalArgumentException if the property cannot be converted to the specified type | ||
* @throws NoSuchElementException if the property does not exist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NoSuchElementException if the property name or the value element of the given property name does not exist.
@@ -33,7 +33,7 @@ A higher `ordinal` means that the values taken from this `ConfigSource` will ove | |||
This allows a configuration to be customized from outside a binary, assuming that external `ConfigSource` s have higher `ordinal` values than the ones whose values originate within the release binaries. | |||
|
|||
It can also be used to implement a drop-in configuration approach. | |||
Simply create a jar containing a `ConfigSource` with a higher ordinal and override configuration values in it. | |||
Simply create a jar containing a `ConfigSource` with a higher ordinal and override or clear configuration values in it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also add here that the empty string is considered an empty value for effects of clearing a configuration?
Let me see if I can summarise the new semantics: If a value is given or "overridden" with the empty string the outcome of a lookup is the same as the value not being present. Does that sound like the intention here? |
* @throws java.util.NoSuchElementException if the property isn't present in the configuration | ||
* @return the resolved property value as an instance of the requested type (not {@code null}) | ||
* @throws IllegalArgumentException if the property cannot be converted to the specified type | ||
* @throws NoSuchElementException if the property does not exist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does 'does not exist' mean? of course if the key is not contained in the config. But what happens with an empty string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has the same semantical meaning, meanwhile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See Converter.java#107.
@@ -102,12 +102,28 @@ | |||
* <em>Implicit</em> converters are only created when no other converter was found; therefore, they do not have a | |||
* priority. | |||
* | |||
* <h3 id="empty">Empty values</h3> | |||
* | |||
* For all converters, the empty string {@code ""} <em>must</em> be considered an empty value. Some converters <em>may</em> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not fully reviewed whether this changes any logic. But please be aware, that changing this logic will also have an effect on default values within mp-config but also business apps. It might be a hidden API change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does change logic. But it's not a hidden change; on the contrary it's spelled out clearly for the first time.
@@ -30,7 +30,7 @@ All this must be possible without the need to re-package the whole application b | |||
|
|||
MicroProfile Config provides a way to achieve this goal by aggregating configuration from many different <<configsource,ConfigSources>> and presents a single merged view to the user. | |||
This allows the application to bundle default configuration within the application. | |||
It also allows to override the defaults from outside, e.g. via an environment variable a Java system property or via a container like Docker. | |||
It also allows to override or clear the defaults from outside, e.g. via an environment variable a Java system property or via a container like Docker. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change is redundant. To 'override' already includes clearing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is deliberate, in order to make the specification clearer.
That is exactly the intent. In addition to enabling this ability, this change also fixes several inconsistencies and contradictions. |
* consider other values to be empty as well. | ||
* <p> | ||
* When a conversion results in an empty value, most converters should normally return {@code null}. All of the built-in | ||
* converters <em>must</em> return {@code null} for an empty value, <em>except</em> for the built-in converters for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about the primitive types, e.g. int?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no primitive type converters, only boxed type converters. And boxed type converters must also return null
for empty values. If a CDI-style property has a "primitive" type, it will unbox for you, but it cannot be empty by nature (because primitive values have no empty representation).
Don't confuse conversion result with the result of the property retrieval operation. If a conversion returns null
and getValue
or an equivalent (e.g. CDI injection of a non-optional type) is used, then NoSuchElementException
would be thrown (as these operations can never yield a null
value), because the value is empty and the type does not allow empty values.
Types such as OptionalInt
allow an emptiable int
value to be represented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand from the implementation point of view. I directly comment on the above text. In the converter spec, we list int
under Built-in converters. I think people will be confused when they saw the above statement.
The following Converters are provided by MicroProfile Config by default:
boolean and java.lang.Boolean , values for true (case insensitive) "true", "1", "YES", "Y" "ON". Any other value will be interpreted as false
byte and java.lang.Byte
short and java.lang.Short
int, java.lang.Integer, and java.util.OptionalInt
long, java.lang.Long, and java.util.OptionalLong
float and java.lang.Float; a dot '.' is used to separate the fractional digits
double, java.lang.Double, and java.util.OptionalDouble; a dot '.' is used to separate the fractional digits
char and java.lang.Character
java.lang.Class based on the result of Class.forName
Maybe in the built-in converters section, we can add the primitive convertion is done via the boxed type.
I think we should move this for the next version (2.0). We don't have much time left. |
Signed-off-by: Emily Jiang <emijiang6@googlemail.com>
…tion Signed-off-by: Emily Jiang <emijiang6@googlemail.com>
…tion Signed-off-by: Emily Jiang <emijiang6@googlemail.com>
Moved to #598 |
Fixes #446, Fixes #531, Fixes #532, Fixes #397.