Clarification of ConfigSource.getProperties and getPropertyNames

[tevans78]

Not so long ago there was an issue opened in the javaee-samples repo because the Liberty config impl did not work as expected with an "EchoConfigSource".

javaee-samples/microprofile1.2-samples#3
https://github.com/javaee-samples/microprofile1.2-samples/blob/master/config/config-source/src/main/java/org/eclipse/microprofile12/config/configsource/EchoConfigSource.java

The EchoConfigSource has been implemented to simply echo back anything that is requested via ConfigSource.getValue. That means that the returns from getProperties and getPropertyNames are incomplete compared with what getValue will respond to.

If ConfigSource.getPropertyNames is not guaranteed to return a complete set then Config.getPropertyNames can not be guaranteed to be complete either. Without that guarantee then the method isn't of much use beyond perhaps some non-critical tracing.

Either the javadoc for both ConfigSource and Config should be updated to make if clear what can be relied upon and what can not... or the methods should just be removed?!

[smillidge]

+1 for updating the docs. Removal could break backwards compatibility however I would also be in favour of removal.
Both methods could be very expensive depending on the Config Source for example imagine a config source that looks up a key in a Redis or memcached KV store it would need to retrieve all keys to comply with the api signature.

[OndroMih]

The javadoc should say that both methods reflect the current state and results may change during runtime.

[OndroMih]

@Emily-Jiang explained that there's a usecase to find out whether a value exists in a config source or not and the only way to find out now is to call getPropertyNames(). The usecase is motly valid when checking that a config value is present at deploy time.
I think this usecase would be better supported by a new method propertyExists(String).

It can even be a default method like this to avoid breaking existing config sources:

default boolean propertyExists(String propertyName) {
        return getPropertyNames().contains(propertyName);
}

However, I would remove both getProperties() and getPropertyNames() as @smillidge suggested. For most sources, there's no guarantee that the config values won't change during runtime. Even standard system property config source can change values during runtime.

To avoid breaking existing code, we can make them deprecated and return empty set/map by default, so that config sources don't have to implement them.

[dmlloyd]

I ran into a related problem with smallrye/smallrye-config#70 - essentially, custom ConfigSource implementations generally must implement a full Map even if their backing implementation isn't really Map-like. It's a heavy burden to place on implementors.

[ljnelson]

For the record and posterity: please do not remove getPropertyNames(). There are many use cases that depend on discovering at least the names of available properties in the system. Thanks.

[Emily-Jiang]

related #431