config
config copied to clipboard
lightbend
Added SPI based loading
Defined an SPI so devs can load custom config file formats without this library relying on them as an external dependency.
Also added capability to use numbers in paths so devs can have array-index-like access to config items.
Just a couple of comments. I am not a committer, just an observer.
There is a Scala port of this library that targets JVM, Scala Native, and Scala.js. I try to port PRs across when I can but am quite a few behind.
This PR looks like it would be very difficult to port to targets that don't support reflection if I am reading the ConfigServiceProvider code correctly. If providing yaml is the main goal then perhaps a static provider factory would work. Additional providers could be added in the factory versus a config file.
The diff for Parseable is very large but it seems that quite a bit of the code is the same.
Overall, this looks pretty reasonable.
@ekrich Thanks for the feedback, it is much appreciated. This PR doesn't use any explicit reflection. Since explicit reflection has be greatly restricted in new versions of the JVM decided to use the SPI pattern. I am not sure if the SPI pattern is supported in other JVM languages. If it is this will open up a lot more than just yaml support. A developer could also add support for TOML, INI, etc without requiring any 3rd part dependencies in this library. Once I get the tests passing (unless they aren't passing because of the use of the SPI pattern), I'll be publishing the yaml support library that developers can crib off of for whatever format they intend on using.
@havocp Huge thanks for reviewing this. In the next couple days of I will have multiple PRs to separate the SPI PR and array indexing PR. I'll also be sure to format the code the next time around. Locally, I do have a happy path test cases for both the SPI and array indexing but they are java JUnit tests. Should I write them as Scala unit tests?
Ideally yes I think the tests are best in Scala like the others - note that while the tests are in Scala, the framework is just JUnit. So you could probably port straightforwardly...
@havocp I now have the code passing the build. This gives me a "clean" starting point with my changes from a build stand point (as well as an understanding of everything that I need to have done both test and documentation wise).
Also regarding tests for the SPI feature, the fact that the build is passing is kind of the basic happy path test for it as the code is using the provided implementations to load the respective configuration associated with it (which the tests would fail if that feature wasn't working properly). I have a library that I'm working on outside of the config library that demonstrates that it works properly, but I know the importance of having zero external dependencies for this library is quite crucial.
@jamesratzlaff I guess I saw this file config/src/main/resources/META-INF/services/com.typesafe.config.spi.ConfigProvider and figured they must be loaded via name by the classloader.
This code is ready for review. Made changes based on @havocp's recommendations with various other clean-ups.
@ekrich I believe the ServiceLoader uses the URLClassLoader for that. AFAIK this either uses reflection or MethodHandles. Either way, I'm pretty sure that the system path libraries the jvm loads are exempt from reflection restrictions.
@jamesratzlaff It is really not related to this PR targeting the JVM but Scala.js and Scala Native don't do classloading or reflection for the most part.
Really hoping this feature gets added. Would probably be good to have a test that demonstrated the use of a new (test-only) SPI - config/src/test/resources/META-INF/services/com.typesafe.config.spi.ConfigProvider.
@devinrsmith I have a POC SPI for YAML configs here. Aside from that this pull request actually implements each of the current Config readers (HOCON/.conf, JSON, and .properties) as SPI's and are included,defined as services , and used for loading by default.
If you add the YAML SPI I made, to your project/classpath, you'll be able to load YAML with out changing any of your java code :-) with the caveate that you are using a build of this library that has the SPI changes.
And if memory serves me correct, the include keyword can be used in exactly the same way for loading in non-HOCON/JSON/properties config files as long as there is an SPI implemention/service definition that supports that format. This way you can integrate other config formats within your regular HOCON config file and use a single API to make use of all those configs. :-D
That said, I'd think that since it is passing the current tests and using the included SPIs as well as functioning with an external SPI (as to not add external dependencies to the core project) beyond my expectations, a test-only SPI seems kind of redundant. But if you can think of a use case I may have missed, I will certainly make an effort to add tests for that.
Has this gone cold? Our main reason for using YAML configs is that it integrates nicely with Helm and Kubernetes.
We do not intend to extend the functionality of "Typesafe Config" further. See https://github.com/lightbend/config#maintained-by
