abstract away everything related to http

[niemannd]

This PR abstracts away everything to do with http requests.
This way we can change the underlying http client without the need for a full rewrite.

Most of the former MeiliSearchHttpRequest class moved into the DefaultHttpClient, turning MeiliSearchHttpRequest into a wrapper to not break existing code.

ApacheHttpClient is a HttpClient basend on the Apache HttpComponents library. I added the Library as a compiletime dependency. This way it doesn't get bundled in the final jar.

RequestFactory is an abstraction to make the request creation more flexible, just in case some special use case needs to overwrite the BasicHttpRequest that is used as a default request class.

Everything combined, the user could change the whole http stack without changes to the sdk code.

Currently only MeiliSearchHttpRequest uses it. Long term it would be better to create a single Instance in the Client that is used by all created handlers.

This is also a candidate for #7

[kination]

@niemannd why I've used pure HttpURLConnection is

• To reduce dependency
• As I remember there was some security issue in ApacheHttpClient(not sure...)

But I'm not sure which is a good way to use, so let's think about it.

Also, how about using OkHttp?

[niemannd]

To reduce dependency

all additional http clients are compileOnly and are NOT bundled with the final artifact. If some user wants to use the Apache Http Client or OkHttp, the user has to add the respective library to his gradle/maven file.

As I remember there was some security issue in ApacheHttpClient(not sure...)

https://snyk.io/vuln/search?q=+org.apache.httpcomponents&type=maven
Only one affecting the used version 5+. I pushed the client to version 5.0.3 - vulnerability fixed.

Also, how about using OkHttp?

Just added a client powered by OkHttp. As you can see, its easy to write an integration for most available http clients.

[eskombro]

From reading the code, this LGTM!

I just wanted to add a few comments:

• PUT request is never used in MeiliSearch API, but still useful to have it implemented already, but if it ever becomes an extra maintaining effort or whatever, it is completely fine to remove it IMHO.

• As I am no expert in Java projects at all, can I ask you what are the steps for a SDK user to integrate a custom HttpClient? Because it would also be very useful to document it in the README if possible. I can do it after the PR is merged !

• Finally, can't we rename OkHttpHttpClient -> OkHttpClient? (not really important, just a suggestion)

[eskombro]

Oh I wanted to comment instead of approving, my bad, as it is in Draft and I don't know if you wanted it checked yet, sorry @niemannd

[niemannd]

@eskombro as i wrote, to not break any existing code, i hardwired the DefaultHttpClient into the MeiliSearchHttpRequest class.
My vision would be to
a) get rid of the MeiliSearchHttpRequest class, as it now only acts as a wrapper for the DefaultHttpClient.
b) make every api class (IndexHandler, etc) accept a HttpClient instance directly or indirectly. This way the Client class can manage a single instance of the HttpClient and supply it to every handler.
c) refactor the Client class to make it easier to configure everything incl. the HttpClient, a future centralized serialization and deserialization system, etc

With this in place, a user could just implement his own HttpClient like i did with Apache and OkHttp and supply it to the Client at creation time and the Client supplies it to every handler it creates internally.

[niemannd]

Finally, can't we rename OkHttpHttpClient -> OkHttpClient? (not really important, just a suggestion)

The Problem was that the Client of OkHttp is also called OkHttpClient. It would not be the problem to name the classes the same, but i believe this would cause some confusion. I'm not satisfied with the current name either. Do you have any other suggestions for a name? 😄

[eskombro]

@eskombro as i wrote, to not break any existing code, i hardwired the DefaultHttpClient into the MeiliSearchHttpRequest class.
My vision would be to
...

Great, thanks for the explanation @niemannd , I like it! In that case my only concern would be to have a default HTTP client that is set unless the user wants to explicitely replace it, having for goal to make it as accessible and easy as possible to use. Do you think Apache should be the default client?

[eskombro]

Do you have any other suggestions for a name? 😄

Huh... not an easy one then! Can't find a good option, was thinking of replacing the 2nd 'Http' with 'Request', 'MeiliSearch', 'Meili', 'Custom'... feels a little bit strained.

[niemannd]

Do you think Apache should be the default client?

I'm not saying we should use Apache specifically, but we should use a Client that allows for ConnectionPooling. The current DefaultHttpClient doesn't do that and therefor is not recommended for production use.

Huh... not an easy one then!

InternalOkHttpClient? CustomOkHttpClient?

[eskombro]

Do you think Apache should be the default client?

I'm not saying we should use Apache specifically, but we should use a Client that allows for ConnectionPooling. The current DefaultHttpClient doesn't do that and therefor is not recommended for production use.

Unless there is a specific reason against it (I'm thinking of security issues etc...), we could go with Apache by default. Seems like a good choice and it is widely used and maintained, so it feels like we can rely on it. We can always change it as it will be abstract now. And there is also the option of using a custom one, so as long as this doesn't generate any conflict, I don't see any reason against making that choice. 👍

InternalOkHttpClient? CustomOkHttpClient?

Haaaaaa, I have the feeling that no option will make us happy, CustomOkHttpClient seems reasonable :)

[eskombro]

Hey @niemannd is this PR ready to merge? If so can you rebase please?

Thanks!

[niemannd]

rebase done.
ready to merge, captain!