Open henryx opened 11 months ago
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.
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:
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:
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:
Any suggestion would be appreciated