Kong / unirest-java

Unirest in Java: Simplified, lightweight HTTP client library.
http://kong.github.io/unirest-java/
MIT License
2.59k stars 593 forks source link

Hard dependency on Gson #396

Closed rakeshk15 closed 2 years ago

rakeshk15 commented 3 years ago

Config class holds a field private Optional<ObjectMapper> objectMapper = Optional.of(new JsonObjectMapper());

and JsonObjectMapper is based on Gson api and if Gson is missing in OSGi environment because most of the time we have Jackson, the class loading will fail. Of course we can install the Gson bundle into the OSGi runtime and it will work but when we already have Jackson present why would someone wants Gson.

I know there is an extension module available for Jackson but that does not remove the hard dependency on Gson, so a way to provide the kong.unirest.ObjectMapper implementation like kong.unirest.JacksonObjectMapper at runtime would be good to have.

There must be very good reasons to defaults to Gson but I still thought it is good to have a pluggable mechanism without a hard dependency on any JSON library.

JJWT - https://github.com/jwtk/jjwt has this approach, please have a look.

Thanks, Rakesh

ryber commented 3 years ago

Gson is not only used by the ObjectMapper it is also (and primarily) used by the native asJson method.

Originally Unirest used (and had a hard dependency) on org.json and returns that libraries JSONObject for it's vanilla json methods. org.json has a stipulation in it's license that it "cannot be used for evil". This prevented Unirest from being included in many open source projects and repositories.

Google got around this for Android by writing their own clean room implementation of org.json but this is not well maintained and is far behind the reference implementation. So we wrote our own clean room implementation, Gson was chosen as the backing engine for this as I was not terribly interested in writing en entire JSON parsing engine just for unirest.

So, in order to do what you are asking, we would need the option of subbing in Jackson for Gson in that. This is not as easy as the ObjectMappers. It include half a dozen classes with hundreds of methods.

Gson is also used in a few other places in Unirest like the JsonPatch implementations. I'm not opposed to breaking it out like this but it IS a lot of work and the pom changes would be a breaking change for most consumers so it would need to go into the next major release.

If only Java would finally get a native std lib json parser!

rakeshk15 commented 3 years ago

hi @ryber, thanks for the prompt response.

I was searching for a clean REST client library and came to know about unirest-java and i liked it much for its cleaner design 👍

Now I understand the rationale behind Gson and seems removing it bring a lot of breaking changes which obviously not desirable for a stable code.

+1 for JDK 11 HttpClient based version 4.0.0-RC1 which is lightweight and almost no dependency(except Gson), may be this would be a pluggable version where we would be able to plugin in a Jackson or Gson based ObjectMapper at runtime.

If only Java would finally get a native std lib json parser!

Doesn't seem to be a priority there because of Jackson mainly i guess.

Also want to know about release of other clients based implementations such as Apache HC5 etc., will this be available in maven as a separate artifact or based on a classifier?

Thanks, Rakesh

ryber commented 3 years ago

I've tried several times to get HC5 to work with Unirest's interfaces. The regular client works fine, but they radically changed the async client and I've never been able to get it to work the way Unirest wants it to work. It's all reactive streams now and anything beyond very simple use cases is quite difficult, especially when you are trying to abstract it to be generic.

Unless someone else wants to give it a wack I'm not considering ever supporting HC5. The Java11 client does most of the same things and does it pretty cleanly.

rakeshk15 commented 3 years ago

Good to know about HC5 issue here.

I also liked the JDK11 HttpClient based version, no extra dependencies, sometimes in OSGi extra dependencies make life hell. Just want to know if you have a performance benchmark b/w JDK 11 and Apache HttpClient based versions. In general how is the performance of JDK11 HttpClient, if you have measured in past?

+1 for JDK 11 HttpClient based version 4.0.0-RC1 which is lightweight and almost no dependency(except Gson), may be this would be a pluggable version where we would be able to plugin in a Jackson or Gson based ObjectMapper at runtime.

Thoughts here?

I am making myself familiar with the code, once I have good understanding i'll be able to help in making JDK11 based version free from any hard dependency, if you agree here.

ryber commented 3 years ago

I have not done any performance testing with the two clients. I don't really have the equipment necessary to do a good one and I figure someone out there is going to be doing it (probably Oracle)

If super performance is someone's goal they probably shouldn't use Unirest anyway. Why add another abstraction on top of an abstraction.

I'm ok with the concept of making the json dependency a optional plugin like jjwt (which I use all the time). But first we need to build out the abstraction so it's pluggable. Which is going to mean some additional interfaces and some kind of implementation factory. Right now JSONElement and it's inheritors are fully self contained and would need to remain so. They would need to figure out how to get the proper backing engine based on runtime.

ryber commented 3 years ago

The way I see it the refactoring could go like this:

  1. Extract interfaces for JSONObject, JSONElement,J SONArray etc
  2. Move Gson implementations into a different package.
  3. Create new implementation that delegates to a different implementation (gson) of it's own interface
  4. Create alternative Jackson implementations.

This could (and should) be done in the mainline. The only difference between the main branch and the RC branch is that in RC you must declare your implementation choice. while in main you just get gson handed to you (but you COULD override it with jackson and exclude the gson dependency.

ryber commented 3 years ago

ok, question, if we extract an interface for JSONObject what do we call it?

rakeshk15 commented 3 years ago

JsonObject sounds ok to me.

ryber commented 3 years ago

Tried hacking on this a bit since i t's like -3 outside. This is going to be hard. What I outlined above isn't going to work. I'm going to need to abstract out gson itself

ryber commented 2 years ago

This is complete as part of Unirest-4 which is in maybe the final release candidate, see https://github.com/Kong/unirest-java/discussions/429