search

sksamuel / hoplite

A boilerplate-free Kotlin config library for loading configuration files as data classes
Apache License 2.0
922 stars 74 forks source link

Path normalization breaks decoding data classes inside maps and sets incorrect map keys for property sources #443

Closed rocketraman closed 1 month ago

rocketraman commented 2 months ago

The implementations of AbstractUnnormalizedKeysDecoders which was created in PR https://github.com/sksamuel/hoplite/pull/413 "restore" the unnormalized node keys.

Unfortunately, when a decoder that requires normalization is further down in the tree, such as a data class decoder, it sees the unnormalized values and can therefore fail to read the config correctly.

The solution is to be less heavy-handed about de-normalization for map decoders and the Hikara data source decoder, and have them internally decode nodes only when they need to.

Additionally, the sourceKey was being set incorrectly to null for props-based sources like the CommandLinePropertySource.

rocketraman commented 1 month ago

@sksamuel Would appreciate a review and merge of this asap, because current master is a bit broken.

osoykan commented 1 month ago

I think that with the new version 2.8.0, another problem was introduced in the same area. Map keys from CommandLine was fixed but, now, if it comes from environment variables, the key ends up being lowercasekey. I didn't have the time to write a test case but, just saw on the CI, the behaviour was affected.

rocketraman commented 1 month ago

I think that with the new version 2.8.0, another problem was introduced in the same area. Map keys from CommandLine was fixed but, now, if it comes from environment variables, the key ends up being lowercasekey. I didn't have the time to write a test case but, just saw on the CI, the behaviour was affected.

@osoykan The key is lowercase because all keys are normalized to lowercase now due to the addition of the path normalizer by default, but it should still set the config correctly. Does it?

Also FYI, there is possibly related issue #410 which is fixed by PR #414.

osoykan commented 1 month ago

@rocketraman I've reproduced the case in the commit: https://github.com/osoykan/hoplite/commit/d858187a057f58e9066ef0ca9b63ff2fedbe07cc

When the key is lowercase, then my access strategy needs to be changed, too, which is breaking change.

Could be fixed with your MR, indeed, if you could give it a try with the test-case I've added that would be perfect!

rocketraman commented 1 month ago

@osoykan Oh that's interesting -- the test is fine if addEnvironmentSource (or addCommandLineSource) are used by themselves, and it is also fine if addResourceOrFileSource is used by itself, but if addResourceOrFileSource is combined with either addEnvironmentSource or addCommandLineSource (or both), it fails.

I will look into it.

rocketraman commented 1 month ago

@osoykan I created https://github.com/sksamuel/hoplite/issues/447, and it should be fixed in #448 .