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

Added Support for YAML Configuration #110

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SethFalco
Copy link
Contributor

@SethFalco SethFalco commented Aug 14, 2020

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.

@tandraschko
Copy link
Member

@struberg could also review this one?

@SethFalco
Copy link
Contributor Author

SethFalco commented Jul 17, 2021

I've rebased with master, revised version numbers, docs, and fixed a bug where nested YAML arrays with only null values yielded unexpected behavior.

@SethFalco SethFalco force-pushed the yaml-support branch 3 times, most recently from 5da434e to dbe6949 Compare December 26, 2022 23:24
@SethFalco
Copy link
Contributor Author

SethFalco commented Dec 26, 2022

Similarly to before, rebased with master, resolving any issues along the way.
CI failure appears to be a misconfiguration. 🤔

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.

@rmannibucau
Copy link
Contributor

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.

@SethFalco
Copy link
Contributor Author

SethFalco commented Dec 27, 2022

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:

Or alternatively, we could make something from scratch, though that'd be a lot of work. ^-^'

@rmannibucau
Copy link
Contributor

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.

@SethFalco
Copy link
Contributor Author

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. 🤔

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.

@rmannibucau
Copy link
Contributor

@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.

@SethFalco
Copy link
Contributor Author

SethFalco commented Aug 31, 2023

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.

@SethFalco
Copy link
Contributor Author

Hey hey!

Is there anything I can do to help get this merged soon?

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.

3 participants