Must a Config actually use Converters?

[ljnelson]

The Config interface of course does not refer to them at all.

The ConfigBuilder interface accepts them but there does not seem to be a requirement anywhere I can find that these Converters actually be used by a Config implementation.

By contrast, ConfigSources, which are also accepted by a ConfigBuilder, clearly show up in the Config interface (although they give rise to another issue whose comment section apparently augments the specification).

The specification around Converters also mentions, at the bottom:

If a Converter implements the java.lang.AutoCloseable interface then the close() method will be called when the underlying Config is being released.

What does "underlying" mean?

Also this implies that an autocloseable Converter must be used in some unspecified way by exactly one Config instance, or it will be at risk of being closed improperly from the viewpoint of some other Config instance. This doesn't seem to be spelled out anywhere.

For clarity: clearly I understand the intent is that conversion must happen using Converter instances during the Config#getValue(String, Class) method in some hazy way. But it seems to me there is no such requirement anywhere.

If such conversion must be used, is it reasonable to provide a getConverters() method on Config, much as there is a getConfigSources() method on Config?

Is it therefore also reasonable to suggest that there be a full specification of how conversion must behave when the getValue(String, Class) and getOptionalValue(String, Class) methods are invoked?

[dmlloyd]

The behavior of converters is more or less laid out here: https://github.com/eclipse/microprofile-config/blob/master/spec/src/main/asciidoc/converters.asciidoc

I've proposed the following improvements/clarifications to the JavaDoc: https://github.com/eclipse/microprofile-config/pull/445/files#diff-cba20e499de1f727fdbf90650c680ce9R36 (see the result here:

microprofile-config/api/src/main/java/org/eclipse/microprofile/config/spi/Converter.java

Lines 35 to 108 in 74a8c9f

	* <p>Interface for converting configured values from String to any Java type.
	*
	* <h3>Global Converters</h3>
	*
	* Converters may be global to a {@code Config} instance. Global converters are automatically applied to types that
	* match the converter's type.
	*
	* <p>Global converters may be <em>built in</em>. Such converters are provided by the implementation. A compliant
	* implementation must provide built-in converters for at least the following types:
	* <ul>
	* <li>{@code boolean} and {@code Boolean}, returning {@code true} for at least the following values (case insensitive):
	* <ul>
	* <li>{@code true}</li>
	* <li>{@code yes}</li>
	* <li>{@code y}</li>
	* <li>{@code on}</li>
	* <li>{@code 1}</li>
	* </ul>
	* <li>{@code int} and {@code Integer}, accepting (at minimum) all values accepted by the {@link Integer#parseInt(String)} method</li>
	* <li>{@code long} and {@code Long}, accepting (at minimum) all values accepted by the {@link Long#parseLong(String)} method</li>
	* <li>{@code float} and {@code Float}, accepting (at minimum) all values accepted by the {@link Float#parseFloat(String)} method</li>
	* <li>{@code double} and {@code Double}, accepting (at minimum) all values accepted by the {@link Double#parseDouble(String)} method</li>
	* <li>{@code java.lang.Class} based on the result of {@link java.lang.Class#forName}</li>
	* <li>{@code java.lang.String}</li>
	* </ul>
	*
	* <p>Custom global converters will get picked up via the {@link java.util.ServiceLoader} mechanism and and can be registered by
	* providing a file named {@code META-INF/services/org.eclipse.microprofile.config.spi.Converter}
	* which contains one or more fully qualified {@code Converter} implementation class names as content.
	*
	* <p>A custom global {@code Converter} implementation class may specify a {@code javax.annotation.Priority}.
	* If no priority is explicitly assigned, the value of 100 is assumed.
	* If multiple Converters are registered for the same type, the one with the highest priority will be used. Highest number means highest priority.
	*
	* <p>Custom global converters may also be registered programmatically to a configuration builder via the
	* {@link ConfigBuilder#withConverters(Converter[])} or {@link ConfigBuilder#withConverter(Class, int, Converter)} methods.
	*
	* <p>All built in converters have a {@code javax.annotation.Priority} of 1.
	* A Converter should handle null values returning either null or a valid Object of the specified type.
	*
	* <p>Converters may return {@code null}, indicating that the result of the conversion is an empty value.
	*
	* <h3>Implicit Converters</h3>
	*
	* <p>If no global converter could be found for a certain type,
	* the {@code Config} implementation must provide an <em>Implicit Converter</em>, if:
	* <ul>
	* <li>the target type {@code T} has a {@code public static T of(String)} method, or</li>
	* <li>the target type {@code T} has a {@code public static T valueOf(String)} method, or</li>
	* <li>the target type {@code T} has a public constructor with a {@code String} parameter, or</li>
	* <li>the target type {@code T} has a {@code public static T parse(CharSequence)} method</li>
	* <li>the target type {@code T} is an array of any type that has an installed explicit, built-in, or implicit converter</li>
	* </ul>
	*
	* The implicit array converter uses a comma ({@code U+002C ','}) as a delimiter. To allow a comma to be embedded within
	* individual elements, it may be preceded by a backslash ({@code U+005C '\'}) character. Any such escaped comma will be
	* included within a single element (the backslash will be discarded).
	* <p>
	* The implicit array converter <em>must not</em> include empty segments within the conversion result. An empty segment
	* is defined as a segment for which the element converter has returned {@code null}. If no elements are included, the
	* value must be considered empty and {@code null} returned.
	*
	* <h3>Empty Values</h3>
	*
	* A {@code null} conversion result indicates that the converted value is empty. All implicit converters defined
	* here <em>must</em> return {@code null} when converting empty values. All built-in global converters defined here
	* <em>must</em> return {@code null} when converting empty values. Other built-in global converters <em>should</em>
	* return {@code null} when converting empty values.
	* <p>
	* Customized user converters and certain special built-in converters may return other values to represent empty. However, this
	* may be unexpected so the cases for such behavior should be clearly documented for each converter.
	* <p>
	* Most converters will consider an empty string value ({@code ""}) to be empty. Some converters may yield an empty value
	* for other inputs.

)

I would agree that at the very least there needs to be a <T> Converter<T> getConverter(Class<T> clazz) method on Config which returns the converter that would be used for the given class. Having a getConverters method is a bit hazy because it would need to be decided whether built-in or implicit converters are included, and those are generally created on demand so if those are included, the returned collection might confusingly change over time.

[ljnelson]

I truly understand the intent, I really do.

But I urge you and others to read the specification with "robot implementor" eyes. Here, for example, I don't think there is anything that says when Converters should be used, how they should be used, how many of them there can be, what the builder must do to "install" them into the Config if indeed they must live "inside" the Config, what happens if they're shared between Configs, whether they're thread safe, etc. etc. etc. etc. Language matters.

I think my main beef in this area in particular is that I can probably implement Config properly without ever referencing Converter at all. I don't think that's the intent.

I always look to the CDI specification as a good example that splits the difference between having an answer for everything where one is needed, readability, and clear identification of where implementations may do what they wish. I find nothing like that in this specification in many different areas and have been trying to file issues (as advised) where I've noticed gaps so that in my own implementation I can document all the non-specified choices I've had to take as an implementor with references to this project's issue board.

[dmlloyd]

I agree that the specification quality is very sub-par. This has caused us many headaches as well maintaining SmallRye Config and Quarkus. I'm hoping that if we can get past #443 and #445, we can follow up with some proposals which would greatly improve the specification language.

[Emily-Jiang]

@ljnelson
The specification around Converters also mentions, at the bottom:

If a Converter implements the java.lang.AutoCloseable interface then the close() method will be called when the underlying Config is being released.

What does "underlying" mean?

The above sentence means the config object constructed via the Builder pattern where you supplied the converters.

I am not sure how useful it is to list the converters at all. Can you explain why do you need this method?

[ljnelson]

The above sentence means the config object constructed via the Builder pattern where you supplied the converters.

But there is no requirement that the converters supplied to the builder are actually used by the Config. If you think there is such a requirement, please point to the text where it says that a Config object must use Converters supplied to the builder.

[dmlloyd]

In the converters part of the spec, the text currently reads:

A custom Converter for a target type of any of the built-in Converters will overwrite the default Converter.

I think it is meant to say "override" not "overwrite". In a follow-up section it says:

If no built-in nor custom Converter exists for a requested Type T, an implicit Converter is automatically provided if the following conditions are met:

I think this implies that custom converters will be used for a given type if available.

But I agree again that the spec language is not clear and needs refinement.

[Emily-Jiang]

@dmlloyd please come up with a PR to clarify this.

[Emily-Jiang]

@dmlloyd I'm going to create a PR for this in a minute so that it can be included in the Config 1.4 release.

[dmlloyd]

I've been working on an overarching JavaDoc cleanup PR which I hope to submit today, but there will likely be conflicts.

[ljnelson]

What was the final result here? Was the specification text altered?

[kenfinnigan]

@ljnelson #498 contains the changes