deltaspike
deltaspike copied to clipboard
apache
Added Support for YAML Configuration
This adds a new module under org.apache.deltaspike.modules:deltaspike-yaml-module-impl, which includes an implementation of ConfigSource for YAML support, and depends on snakeyaml.
I've rebased with master, revised version numbers, docs, and fixed a bug where nested YAML arrays with only null values yielded unexpected behavior.
Similarly to before, rebased with master, resolving any issues along the way.
CI failure appears to be a misconfiguration. :thinking:
Hoping both my PRs can be reviewed soon? ^-^' I appreciate you're all busy, but it'd be great to get these merged before more updates are pushed out.
Any hope to drop snakeyaml dep? It has, as jackson, some cve open and a hard time to fix them so it is a nightmare for projects consuming it.
Rest looks ok to me.
We could drop it if that's desirable, but we'd need to either replace or DIY it.
I thought SnakeYAML was a solid choice for two reasons:
- It's reasonably popular and still maintained.
- I can see that it's used in other Apache projects, namely commons-configuration.
There are other libraries endorsed on the YAML website that we could consider, excluding potential use of JNI:
- snakeyaml/snakeyaml-engine
- snakeyaml/snakeyaml
- EsotericSoftware/yamlbeans
- decorators-squad/eo-yaml
- OpenHFT/Chronicle-Wire
Or alternatively, we could make something from scratch, though that'd be a lot of work. ^-^'
Well just the parsing side is not a lot of work, in particular cause we don't care about the perf enhancements a lib like that can bring (thinking to memory optimization/buffer/cache).
That said a very valid option for me would be to not do a yaml module but a generic Map<String, Object> flattenization module, then the user would load the file as he wants (protected abstract Map<String, Object> load();in the abstractGenericMapModel` class) - including whatever yaml lib he uses - and we would flatten the map (visiting object and supporting most of common default types: map, list recursively but String, primitives, BigDecimal, BigInteger as scalars).
This enables to not bring any drawback and still be a one liner (+spi file) for end user which is probably saner.
The main issue with libs like that is the fact it enables mapping as well as generic deserialization - the only we care, right. The mapping (object) brings issues and CVE we don't want - actually we probbaly don't want to justify more than explaining we we can't exploit some of them.
Can it be a compromise?
Else +1 to implement our own light parser.
Finally catching up with personal projects again, and reviewing old PRs related to them.
For now, I've just rebased the changes with master and ensured the project still compiles and passes tests. I have not dropped the SnakeYAML dependency yet.
I'll have to look back at this and see what's the best approach. :thinking:
Else +1 to implement our own light parser.
@rmannibucau Do you think implementing a lightweight/read-only YAML parser that only loads properties into a Map is still worthwhile? Reading your comment again, I do appreciate the motivation and benefits that come from it. I think it's sensible, but figured I'd get confirmation before I commit time to work on it.
@SethFalco I think it makes sense but while snakeyaml is extracted I'm also ok - btw v2 is out and fixes 1.33 CVE ;). A yaml-light support - a bit better than https://github.com/yupiik/fusion/blob/master/fusion-httpclient-parent/fusion-kubernetes-client/src/main/java/io/yupiik/fusion/kubernetes/client/internal/LightYamlParser.java which is limited to one kind of file - would make sense but due to the EE path I'm not sure it is worth the investment so I'll let you judge now.
I think it makes sense but while snakeyaml is extracted I'm also ok - btw v2 is out and fixes 1.33 CVE ;)
Ahh nice, I tried to check Maven Central before, but it was down when I was working on this. Thanks for pointing that out!
I'm not sure it is worth the investment so I'll let you judge now.
If we're willing to use SnakeYAML, I would much prefer that, over the intricacies of writing the parser myself, even if derived from another project.
But if other maintainers have any problem with this, do raise it, and I'll be willing to evaluate other options. I'm on the side of reducing maintenance by using the library, but I do appreciate the benefit of reducing code surface and dependency by going DIY.
For now, I've updated this PR to the latest SnakeYAML version. For the APIs this project uses, there were no breaking changes, so all code remains the same and tests pass.