[QUESTION] Should I remove JSON parser code?

[henryx]

According to codebase, the JSON parser code is injected from json-minimal project. Although this permits to maintain the zero-dependency promise, this creates two problems:

1. This code is out of scope of the library, because it is used only to parse and manage JSON sent and received from Vault.
2. Project is not maintained. Last commit was 5 years ago, and 2 years ago was declared unmaintained.

For these reasons, I want to break the zero-dependency promise and remove this code. As substitude, I want to use one of these two libraries:

1. JSON-Java
2. Jakarta JSON API

Choice of one of these two libraries is made considering the low impact in terms of third party dependencies that they have. Major concerns are:

1. Jakarta JSON API is most preferrable because it is part of the Jakarta Project, but I have doubt of the impact of the license (GPL + Classpath Exception)
2. JSON-Java is a good choice and is most followed, and is released as public domain, so this remove all doubt concerning license, but my doubt it's if your integration can be have impact on third parties project

Any suggestion would be appreciated

[benoitgravitee]

Hey there,
I really like this project. But to be fait, I was considering reimplementing (or fork, or why not contribute) a Vault's client because of two main reasons:
One was have no shared TCP connections. But now is now fixed, Having httpClient injection is quite nice and this ability to reuse connections removes is a huge gain, it really was a show stopper beyond spike choices. Nice work! I'll use it as soon as this comment is sent...
Second is obviously JSON parsing, so short version is: yes please remove it.
Main reasons imo are security and trust. We don't depend on a third party dependency now, nice, but in case a new bug or CVE arise, there is no quick solution.
imho, having an interface for marshalling/unmarshalling and let users choose implementation (that they might want to implement) is a better design.

This is a proposal, where you could keep (and maybe simplify) your JsonObject

interface JSONParser {
   JsonObject marshall(byte[] bytes) throws JsonParsingError;
   byte[] unmarshall(JsonObject jsonObject);
}

For instance I'm using Vert.x and Jackson. I'd really like to have the ability to add Jackson in my classpath and provide VaultConfig a Jackson JSON parser impl.

var parser = new JacksonJSONParser();
vaultConfig.jsonParser(parser);

For this project, it is a provided dependency for out-of-box supported parser (you mentioned two), and yes an implementation. For the user it is adding a dependency of there choice (and possibly a default one) in order to use it.

You could apply the same for Rest, you are using JDK's which is nice, but many use other ones in their project (okhttp, apache, vertx etc.) and might want to keep it this way for consistency and configuration sometimes. I used AWS SDK, they leave a choice on which http client to use (including JDK's), which I found quite nice. But I agree it is probably a bigger undertaking.

Happy to chat about it if you want.

[jon5477]

I second @benoitgravitee and believe an interface would provide the best compatibility without breaking existing implementations. I would propose keeping the current json-minimal code within the library but implementing an additional interface that would allow people to configure with a JSON library they wish to use (be it Jackson Databind, GSON) while defaulting to json-minimal if no external JSON library is configured.

I'm happy to contribute some code that will test this and implement the appropriate interface and drivers for the Jackson Databind library and json-minimal library.