Skip to content
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

Update MP config to 1.3 #537

Merged
merged 27 commits into from
Apr 11, 2019
Merged

Update MP config to 1.3 #537

merged 27 commits into from
Apr 11, 2019

Conversation

batsatt
Copy link
Contributor

@batsatt batsatt commented Mar 28, 2019

Fixes #432 and includes changes to SE for improved symmetry.

Unfortunately, the different models for handling config sources in SE and MP have made symmetry for environment variables a bit complicated.

In SE, the contents of the config sources are merged during the config build and not consulted during a search. In order to support environment variables taking precedence over application config entries, an optimistic mapping must take place prior to the merge; see EnvironmentVariables.expand(). This approach produces key aliases that are highly likely to match search keys, but is limited in what it can do.

I've been referring to this approach as the "static" mapping.

In MP, the config sources are consulted during the search and the MpcSourceEnvironmentVariables class can easily implement the mapping required by the spec. Prior to the current change, it was (mostly) sufficient to just do the same static mapping; however, the one place where this falls down is with unusual characters in keys (e.g. slash in app.my/property, or a Unicode character). The spec wants any non-alphanumeric character other than underscore mapped to underscore, and the 1.3 config TCK checks for this case.

To fix this, the current change switches to use a "dynamic" mapping within MpcSourceEnvironmentVariables; see its use of EnvironmentVariableAliases.

So far, so good, except... SE does not handle unusual characters, so it isn't symmetric in this case. We can't simply switch to the static mapping or else mapped environment variables can't take precedence. We can't enhance the static mapping because the set of unusual characters is vast and would obviously result in an unacceptable explosion of entries.

So we can either live with the asymmetry or try to address it. I've attempted to address it here, using the dynamic mapping as a last resort when a search fails; see aliasGenerator usage in ConfigFactory. If the config key with the odd characters is actually present in the config, you get that value; if it is not, but one of the MP aliases is present you get that. Note a few things here:

  1. The value with the aliased key is not restricted to the env variables source (e.g. you could have "FOO_BAR" in an application source and find it using "foo.bar" as the key.
  2. Since this lookup happens only after searching everything else, it doesn't support overriding an existing value.
  3. This behavior is enabled only when the default environment variable source is enabled.
  4. It adds a small cost for every lookup miss, but it is one-time since the result is cached.

Like I said... complicated. It is clearly debatable whether this fallback search is worth it.

@batsatt batsatt self-assigned this Mar 28, 2019
@tomas-langer
Copy link
Member

I would vote for asymmetry here.
Helidon SE config is supposed to be a snapshot of the config tree.
MP config is a dynamic map.

@batsatt batsatt requested review from tomas-langer and tjquinno April 1, 2019 19:08
@batsatt
Copy link
Contributor Author

batsatt commented Apr 1, 2019

I'm leaning your way here Tomas. If it handled overrides I would feel differently, but as it is the return for the extra complexity isn't great.

At the moment, I'm focusing on correctly supporting converters, and will get back to this.

batsatt added 2 commits April 1, 2019 12:27
…rsions supported by Helidon.

This currently breaks the TCK since ObjectConfigMapperProvider supports "conversion" using only a default ctor; this needs to be addressed somehow.
@batsatt
Copy link
Contributor Author

batsatt commented Apr 1, 2019

@tomas-langer I added tests for the injection of types using implicit converters, which highlighted the fact that MpConfig.hasConverter(Class) is overly simple. It seems to be present to filter out some of the converters supported by SE, is only used by ConfigCdiExtension, but doesn't match what MpConfig.convert(Class, String) will do.

I've removed that filter, and, to pass the TCK, changed GenericConfigMapper to fail if there are no bean properties. The TCK expects that a conversion to a type that has only a default ctor will fail, and this change (along with some exception handling) causes it to do so.

@tjquinno
Copy link
Member

tjquinno commented Apr 2, 2019

I'm a little late to the comment stream here.

Earlier, Bryan, you described what you called the "static" and "dynamic" techniques for handling "weird" characters in keys, and that an option to maintain symmetric behavior between SE and MP would be to introduce dynamic weird character handling into SE.

Tomas, your comment about "SE config is supposed to be a snapshot of the config tree" to me at least sounds as if you have interpreted Bryan's description of the alternative to mean it would introduce dynamic config behavior into SE.

IIUC that is not what is suggested here. Rather, it is just when and how the handling of special characters in keys is done.

batsatt added 3 commits April 3, 2019 11:06
…ties. Along with some exception handling in ConfigCdiExtension, this solves the TCK problem where it expects a conversion to a type that only has a default ctor to fail.
@batsatt batsatt changed the title WIP: First pass at MP config 1.3 update. Update MP config to 1.3 Apr 4, 2019
@tomas-langer
Copy link
Member

I have gone through the changes and it looks good.
There are a few comments (in review).
I agree with what you said Bryan about the hasConverter and your solution seems to handle that nicely (and as I hope I understood correctly from the code without backward incompatible changes).

Please correct me if I am wrong, the only backward incompatibility is in a case where a class has a public default constructor and no public setters (as in previous version, we would just create an instance, now we fail), right?

@batsatt
Copy link
Contributor Author

batsatt commented Apr 10, 2019

Yes, you're correct about that backward incompatibility. (Supporting that case seems wrong anyway; what is the point of a "conversion" that ignores the input?)

@tomas-langer
Copy link
Member

Yes, I agree and in that case I do not think this is a real backward incompatible change, rather a bugfix ;)

@batsatt batsatt merged commit b995061 into helidon-io:master Apr 11, 2019
@batsatt batsatt deleted the config-to-1.3 branch April 11, 2019 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Microprofile Config 1.3
3 participants