Path normalization via new NodeTransformer interface
#413
Conversation
rocketraman
force-pushed
the
rg/node-normalization
branch
from
cd1e359
to
90662ef
Compare
|
I think this might be a better approach in general than the parameter mappers that are applied at runtime. It might also make env support easier as we can just layer the whole set of env vars over the final node (if we decide that THIS_FORMAT_IS_THE_CANONICAL_FORMAT) |
rocketraman
force-pushed
the
rg/node-normalization
branch
from
90662ef
to
3caf6f6
Compare
It looks like this PR solves #395 already, simply with the addition of the And yes, I believe the If you want me to make those changes in this PR I can, or we can do it as a follow-up. UPDATE: I did it in this PR.
Yeah, my next PR (#414) builds on this to address the env var stuff. I tried to implement it in a way that maintains backward compatibility. If you had a different idea in mind with "layer the whole set of env vars over the final node" can you explain further? #368 is also relevant I think. |
rocketraman
force-pushed
the
rg/node-normalization
branch
from
3caf6f6
to
b9d0b35
Compare
I just mean that we can take the System.envvar map and make it an always registered property source that is applied last. |
rocketraman
force-pushed
the
rg/node-normalization
branch
from
b9d0b35
to
e9b26ff
Compare
The purpose of the path normalizer is to normalize all inbound paths by making them lower case, and removing any "-" characters. Normalizing paths means sources with different idiomatic approaches to defining key values will all map correctly to the defined config classes. The only downside to this is multiple config attributes in the same class that differ only by case can no longer be disambiguated. This should be a rare case and the advantages are more than worth losing this "feature". We also add a LowercaseParameterMapper by default which can handle the normalized paths.
With path normalization (and even before path normalization), it was possible for the reported path to not match the input value. For example, with environment variables, the environment variable may have been `FOO__BAR`, but the report showed `FOO.BAR`, which makes it harder for users to trace the report back to the original source. We now add a `sourceKey` attribute to `Node` and report on it, to show user's the key value as it was originally present in the source.
rocketraman
force-pushed
the
rg/node-normalization
branch
from
e9b26ff
to
eac8a0b
Compare
Remove both the `SnakeCaseParamMapper` and `KebabCaseParamMapper` by default, add the `PathNormalizer` by default. Add removal of `_` to path normalizer. Fix some issues with the `HikariDataSourceDecoder` when enabling path normalization by default -- that decoder requires the original key case as its props are case-sensitive. Create an abstract `UnnormalizedKeysDecoder` which has the ability to restore the case of keys via the `sourceKey`. Fix breaking explicit sealed types with normalization because the discriminator field defaults to `_type` which normalizes to `type`. Disable normalization if the field name matches the discriminator field name, and the node is a `MapNode`. Fix reporting for strict mode to use the `sourceKey` value, so that reporting matches the source value, not the normalized value. Update Preprocessor implementations to correctly copy the source key when new Map and Array nodes are created.
rocketraman
force-pushed
the
rg/node-normalization
branch
from
084895f
to
80c6ed8
Compare
|
@sksamuel Just a ping on this PR, and the follow-up PRs that are in draft waiting for this one. Is there any plan to review and/or merge? |
|
I've been slow :) |
|
Just wondering, does this new node transformer also help to implement #358? |
Add a generic node transformer interface
NodeTransformer, which can be used to transform nodes in arbitrary ways at configuration load time.Add an implementation of this called
PathNormalizer.The purpose of the path normalizer is to normalize all inbound paths by making them lower case, and removing any "-" and "_" characters. Normalizing paths means sources with different idiomatic approaches to defining key values will all map correctly to the defined config classes. It makes it possible to implement #410.
The only downside to this is multiple config attributes in the same class that differ only by case can no longer be disambiguated. This should be a rare case and the advantages are more than worth losing this "feature".
Therefore, in my opinion this should be installed by default, but I have not done that in this PR until I hear some feedback from @sksamuel.The feature is thus installed by default, and the now unnecessary parameter mappers removed by default.We also add a
LowercaseParameterMapperby default which can handle the normalized paths.This PR is stacked on top of #412 and will be rebased and moved out of draft once that PR is merged.