MP Conf - config sources - dynamic or static?

[Arjan Tijms]

Hi,

I encountered a somewhat troublesome issues with this MP Config test:

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

The source is a dynamic source, meaning it will accept any property key and echoes it back. This test however failed on Liberty, while it passes on Payara and WildFly.

Now it seems that the source of the problem is with the getProperties() map. Emily reported that here:https://github.com/javaee-samples/microprofile1.2-samples/issues/3

However, after internal discussion with Steve, I think the root source of the problem is the interpretation of getProperties(). Liberty seems to interpret this as a kind of validation constraint; it enforces that the source only ever accepts property keys that are initially returned by the getProperties() map. In Payara, and I guess WildFly too, the method is seen as optional, i.e. for if someone would need all keys from a source, and not as a constraint that has to be enforced all the time.

In the Payara and potentially WildFly interpretation, sources can thus always be dynamic, in the Liberty interpretation sources can only ever provide the set of keys which are known and/or available at deployment time and are thus static.

I see a couple of issues with the Liberty interpretation:

1. Where in the spec does it actually say getProperties() or getPropertyNames() is a constraint that needs to be enforced?

2. The set of properties may not be known at deployment time, which is especially true for more dynamic sources

3. The method should be optionally any way, as it may be impossible to support without risking overloading the server's memory if the source has a very large amount of keys (thinks e.g. modelling FaceBook users as properties)

Thoughts?

Kind regards,

Arjan Tijms

[Mark Struberg]

getProperty might return values which getProperties() (or inmp getPropertyNames() ) does not contain.

This is because not every ConfigSource is 'scannable'.

In DeltaSpike we had an own flag per ConfigSource [1]. But I left this off because in practice it has just informal nature anyway. You cannot do anything programmatic in that situation.

An example would be some ConfigSource which is built on a remote tree structure. 

Or much simpler: JNDI. Here it might simply be too expensive to poll JNDI for all values.

So no, getPropertyNames is not a constraint for getValue

hth.

LieGrue,

strub

[Mark Struberg]

Damn, where is the edit button ;)

Here is the mentioned link to isScannable

[1] https://github.com/apache/deltaspike/blob/master/deltaspike/core/api/src/main/java/org/apache/deltaspike/core/spi/config/ConfigSource.java#L99

[Arjan Tijms]

Hi,

>So no, getPropertyNames is not a constraint for getValue

Okay, thanks for the clarification!

@Emilly, shall I restore the test to its original version, and can you then remove this constraint validation from Liberty?

Might also be a good idea to clarify this in the spec, and add a TCK test for it?

Kind regards,

Arjan Tijms

[Emily Jiang]

In the javadoc of ConfigSource, it says clearly on the getProperties and getPropertyNames: https://github.com/eclipse/microprofile-config/blob/master/api/src/main/java/org/eclipse/microprofile/config/spi/ConfigSource.java



The javadoc on getProperties is

/**

* Gets all property names known to this config source, without evaluating the values.

*

* For backwards compatibility, there is a default implementation that just returns the keys of {@code getProperties()}

* slower ConfigSource implementations should replace this with a more performant implementation

*

* @return the set of property keys that are known to this ConfigSource

*/


It is clear according to the spec, the getProperties returns the properties in the config source.

/**

* Return the properties in this config source

* @return the map containing the properties in this config source

*/

Map<String, String> getProperties();

For dynamic config sources, if a property does not exist during the startup, you supply a default value.


I am afraid the current release clearly says what getProperties and getPropertyNames should do. Having this said, we can relax this contract for the following release in light of what you described in your scenario?

Can you raise an issue in the config to get javadoc updated and the rule loosened?

Thanks
Emily

[Arjan Tijms]

Hi,

Mark can probably clarify this better than I can, but doesn't "Gets all property names known to this config source" mean "Gets all property names known to this config source [at the time of calling]"?

It doesn't explicitly say either that the property names can never change again and can not be different in a second call, and it certainly doesn't say anywhere that the property names obtained from the first call should be used as explicit constraint validation of what getValue(...) is allowed to be queried for.

So IMHO both readings would be within what the spec currently asks for. As Mark basically established the "non constrained" reading is the right one, and both Payara and Swarm already implement it that way. I feel Liberty could still just remove that check that is in place now without violating any spec contract. Currently Liberty behaves different from the other implementations, which I think is not helpful to anyone.

Then indeed, for the next version of the spec this should be explicitly clarified (as I asked for before).

[Emily Jiang]

We are discussing about MP Config not DeltaSpike . Unfortunately, until now, this "non constrained" was not discussed anywhere in MP Config. We should not operate based on what Wildfly and Payara do. I think the javadoc is fairly clear on the contract about what getProperties and getPropertyNames should behave.

I still think a better approach is to raise an issue and have the limitation relaxed by adding a couple of sentences on getProperties and getPropertyNames.

We can release this feature in the upcoming release.

Thanks
Emily

[Arjan Tijms]

Hi,

I think Mark explained how things should be in MP Config and just used DeltaSpike as an historical example. But it's perhaps better to let Mark speak for himself ;)

In my personal opinion, if a spec has two possible interpretations, then it's best to align with how all other implementations are doing it. If Liberty lifts the constraint, it would not violate the spec, since the spec doesn't demand the constraint, and Liberty would also still be compliant, since it would still pass the TCK. Lifting the constraint now also means preparing for the future, as no other implementation is going to add the same constraint, but you mentioned Liberty may lift the constraint when the current spec text is made more explicit.

It's of course up to you, but I would say just remove the constraint now. Nothing demands that constraint to be there, you won't help any user with this constraint, and the constraint will be removed soon anyway. Why keep it for now?

Kind regards,

Arjan Tijms

[Emily Jiang]

ok, Arjan! Since it is a good thing to go forward and what you have said, I will agree with majority. We will fix it in the current MP Config feature and also will make spec more explicit to go forward.

Thanks
Emily

[Arjan Tijms]

Hi Emily,

Sounds good, thanks a lot!

Kind regards,

Arjan

[Mark Struberg]

> "Gets all property names known to this config source [at the time of calling]"?

yes exactly that one. Imagine a database backed ConfigSource. Depending on what's in the database you might get different values now and in 5 minutes.

> t doesn't explicitly say either that the property names can never change again and can not be different in a second call

Well it actually pretty much says exactly that they CAN change ;)

See the specification:

"A `Config` instance provides no caching but iterates over all `ConfigSources` for each `getValue(String)` operation."

This means that the names also can change dynamically.

Of course we could make this even more clear. So +1 for a clarification.

txs and LieGrue,

strub