What must the set of property names returned by Config#getPropertyNames() consist of?

[ljnelson]

The javadocs for Config#getPropertyNames() say, in their entirety, and I see no other language related to this topic in the specification documents:

Return a collection of property names.
Returns:
the names of all configured keys of the underlying configuration.

It is hard to know what to make of this. Does this mean:

1. The set of property names returned by Config#getPropertyNames() must consist only of the combination of the sets of property names returned by each ConfigSource present in the return value of Config#getConfigSources() and nothing else
2. The set of property names returned by Config#getPropertyNames() may contain a subset of the combination of the sets of property names returned by each ConfigSource present in the return value of Config#getConfigSources()
3. The set of property names returned by Config#getPropertyNames() can be anything the implementor wants, regardless of whether it is represented in part or in whole by any ConfigSource notionally contained by the Config
   …? It must be one of these. The specification does not say, and there is no TCK test.

I am assuming that (1) is the proper answer (modulo any information related to #370). However, given that the specification says nothing and there is no TCK¹, (3) is the de facto answer at the moment.

If (1) is the proper answer, should a TCK test exist for this?

If (1) is not the proper answer, why does this method exist?

¹ There is

microprofile-config/tck/src/main/java/org/eclipse/microprofile/config/tck/ConfigProviderTest.java

Lines 170 to 183 in 828ee2f

	@Test
	public void testGetPropertyNames() {
	String configKey = "some.arbitrary.key";
	String configValue = "value";
	System.setProperty(configKey, configValue);
	AtomicBoolean foundKey = new AtomicBoolean(false);
	config.getConfigSources().forEach(c -> {
	if(c.getPropertyNames().contains(configKey)) {
	foundKey.set(true);
	}
	});
	Assert.assertTrue(foundKey.get(), "Unable to find property "+configKey);
	}

which indirectly tests only that Config#getPropertyNames() must contain at least one key returned by the getPropertyNames() methods of the ConfigSource instances returned by getConfigSources() at the moment of the Config#getPropertyNames() call.

[ljnelson]

Related: #370, #371, #395

[Emily-Jiang]

@ljnelson It should be the following interpretation:

The set of property names returned by Config#getPropertyNames() must consist only of the combination of the sets of property names returned by each ConfigSource present in the return value of Config#getConfigSources() and nothing else

[ljnelson]

@Emily-Jiang Thanks very much. Shall I contribute a TCK test for this?

[Emily-Jiang]

@ljnelson please feel free to contribute. Thank you!

[Emily-Jiang]

Let's decide the real usage for this method as we plan to deprecate ConfigSource.getPropertiesNames and ConfigSource.getProperties.

[dmlloyd]

If you have a configuration which can detect invalid properties within a namespace, as we do in Quarkus, you need a way to know what configuration properties are present. The only way to do that in the MP Config API is by traversing all the property names of all the sources (ConfigSource#getPropertyNames), which is exactly what Config#getPropertyNames does.

Given that there is a clear use case, it does not make sense to talk about deprecation. You'd need an overpoweringly significant reason to justify doing so.

[dmlloyd]

The question of deprecating org.eclipse.microprofile.config.spi.ConfigSource#getProperties is very different. Nothing uses it in the specification (other than the default implementation of the getPropertyNames method), and nothing in our SmallRye implementation uses it either.

[ljnelson]

@Emily-Jiang Please do not deprecate Config#getPropertyNames(). There are many use cases where it is used and significant. Additionally, given your earlier comment please do not deprecate ConfigSource#getProperties() either. Or at least please don't deprecate ConfigSource#getPropertyNames().

[jmesnil]

Given that there are use case for ConfigSource.getPropertyNames(), we should keep the method.
It seems however there is no objection to deprecate (and plan the removal) of ConfigSource.getProperties().

This would impact ConfigSource.getPropertyNames() which provides a default implementation:

default Set<String> getPropertyNames() {
        return getProperties().keySet();
}

The default implementation should be eventually removed and let the implementation of ConfigSource provides it.

There is also a question about the relevance of Config.getPropertyNames().
Does this method add anything to the API that would not be achievable by iterating on the Config.getConfigSources() and collect their propertyNames?

Finally, if we keep both Config.getPropertyNames() and ConfigSource.getPropertyNames(), would it make sense to align them and use the same types for both. For now, Config.getPropertyNames() returns Iterable<String> (which may contain duplicate property names) while ConfigSource.getPropertyNames() returns a Set<String>.

Shouldn't Config.getPropertyNames() return a Set<String> too so that the collection of property names controlled by this Config does not contain duplicates by construction?

[Emily-Jiang]

@jmesnil please see the comment from @ljnelson regarding the usage of getProperties. With that, I would leave them along.
As for changing the return type, we can discuss when it is time for a breaking change.

[dmlloyd]

@ljnelson can you please explain why you would need ConfigSource#getProperties() in addition to ConfigSource#getPropertyNames()? I read it as a miscommunication but maybe I am wrong?

[dmlloyd]

FWIW changing the return type doesn't have to be a breaking change. A second method could be added under a different name, with the original delegating to it, or a bridge could be introduced by introducing a non-public superinterface that only has the Iterable<String> variant, with Config then extending that hidden interface overriding the method with one returning Set<String>, and you've preserved binary compatibility as well as source compatibility.

[ljnelson]

@dmlloyd What I was referring to was a comment that @Emily-Jiang made in an earlier issue where it was explained that what was intended was that Config#getProperties() should be defined in terms of ConfigSource#getProperties(). Obviously if you remove ConfigSource#getProperties() then you cannot define Config#getProperties() in terms of it.

If we are talking about removing both Config#getProperties() and ConfigSource#getProperties() I have no problem with that.

As mentioned previously please keep Config#getPropertyNames() and define it (I suppose?) in terms of ConfigSource#getPropertyNames().

[dmlloyd]

Ah I see. Well there is no such method as Config#getProperties() so I think we're safe on that account. Our implementation (SmallRye) doesn't actually ever use ConfigSource#getProperties() for any purpose.

[ljnelson]

Guess I read carelessly; carry on.

[dmlloyd]

I guess everyone did; no worries :)

@Emily-Jiang I think this means that there is no further reason to prevent deprecation of ConfigSource#getProperties() for removal as @jmesnil has proposed.

[OndroMih]

I see there's a usecase for ConfigSource.getPropertyNames in some cases, especially during initialization. However, we should ensure that the result is not constant and shouldn't be cached for a long time. I'm not sure this is ensured now, os it?
ConfigSources can be mutable and new properties can be added dynamically, so thry don't know the list of all their props at initialization. There are also good usecases to support completely dynamic sources that derive the value for some properties from their name, so a new property can be created as late as an app asks for it.

If we deprecate getProperties and later remove it, we could change the default impl for getPropertyNames to return an empty set. This would indicate that it's OK to return an empty set of properties and still provide values later. Expensive config sources could decide to return an empty or incomplete set of property names and still work.

Even if we leave getProperties, we can provide a default impl that returns an empty set.

I would also consider how calling getPropertyNames/getProperties can slow down the app. If we allow configsources to return an empty/incomplete set of props and still provide more values later, that might solve this problem too.

[smillidge]

These methods are very useful for tooling. For example the Payara administration console uses these methods to display all known properties in some config sources for runtime configuration. I don't believe any of these methods should be deprecated.

[dmlloyd]

I see there's a usecase for ConfigSource.getPropertyNames in some cases, especially during initialization. However, we should ensure that the result is not constant and shouldn't be cached for a long time. I'm not sure this is ensured now, os it?

No this is not ensured. The set should at least be safe to access indefinitely though, and I don't think there's any reason not to specify that it must be thread safe also. But then, there isn't really any reasonable way to put an upper bound on how long users may cache it. To me the simplest implementation in the presence of mutable configuration (discussed in a different topic) will always be a snapshot as it satisfies all the basic requirements here.

If we deprecate getProperties and later remove it, we could change the default impl for getPropertyNames to return an empty set. This would indicate that it's OK to return an empty set of properties and still provide values later. Expensive config sources could decide to return an empty or incomplete set of property names and still work.

Even if we leave getProperties, we can provide a default impl that returns an empty set.

Good idea!

I would also consider how calling getPropertyNames/getProperties can slow down the app. If we allow configsources to return an empty/incomplete set of props and still provide more values later, that might solve this problem too.

I agree we definitely should explicitly allow getPropertyNames to return a subset of the properties.

[dmlloyd]

These methods are very useful for tooling. For example the Payara administration console uses these methods to display all known properties in some config sources for runtime configuration. I don't believe any of these methods should be deprecated.

@smillidge is there a use case in Payara for getProperties that cannot be easily satisfied by getPropertyNames + getValue?

[smillidge]

Yes if you want to display all the values getProperties is more efficient than the iteration. However in our case the number of properties is likely small.

[dmlloyd]

Understood, so using the getPropertyNames approach does work but might be slower for some implementations. This is reasonable.

Maybe we can find a middle ground between having to support a full, random-access Map and having to look up each property value: like an iterable sequence of key-value pairs. Then the getPropertyNames method could (by default) be implemented in terms of that method, and the getProperties method could be deprecated with a default implementation which constructs e.g. a LinkedHashMap consisting of all of the entries from the new method?

[Emily-Jiang]

Back to drawing board:
Based on today's hangout with @dmlloyd @kenfinnigan , one idea is to introduce a better performant method e.g. getNameValuePairs() in this release. In Config 2.0, we can delete getProperties() method. Thoughts?

[Emily-Jiang]

I've done some javadoc update under #333 . Let's revisit this in Config 2.0 release in June to properly fix this.

[Emily-Jiang]

I'll redo the PR I did last time.

[Emily-Jiang]

I believe this issue can be closed. Please object in the next couple of days if you think otherwise.

[ljnelson]

May I request a one sentence summary of the resolution at the bottom of this issue before it is closed? I am still not sure what the final outcome is, even with #534 being merged. Are there requirements or restrictions on the elements that can be returned in the Set? That must be returned in the Set? Is the Set immutable? Is the method deterministic? Etc.

[dmlloyd]

I do think this needs a little more clarification. I'll work up a more specific PR.

[dmlloyd]

#538 contains my attempt to clarify the thread safety requirements.