Feature/http requester external client

[skradel]

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Need Doc update	yes

Describe your change

This change adds an additional constructor to ApacheHttpRequester to lift the dependency creation out of the class's private implementation.

What problem is this fixing?

Managing the HttpClient dependency within ApacheHttpRequester precludes using an existing client; e.g., it may be desirable to use an instrumented instance or subclass of CloseableHttpAsyncClient already available within a framework.

[Ant-hem]

Hi @skradel, Thanks for your contribution.

I understood your request, however I think the library already has a feature which is covering this use-case. One can inject his own managed instance of an httpClient within the configuration builder like this:

SearchConfig config =
new SearchConfig.Builder("YourApplicationID", "YourAdminAPIKey")
    .build();

HttpRequester myCustomRequester = new myCustomRequester();

SearchClient client = new SearchClient(config, myCustomRequester);

One requirement is: the requester should implement the following interface: HttpRequester. What you could do is to copy/paste the current implementation you just modified. If the solution I provided is not working for you, let us know and please detail your use-case so we could think on a solution.

[skradel]

Hi @Ant-hem - the reason for this modification is indeed to facilitate the use of using the HttpRequester as a parameter to SearchClient; however, the provided ApacheHttpRequester has dozens of disparate responsibilities (exception mapping, compression, composing requests, etc.) that implement an otherwise-invisible contract. Copy+paste of over 200 lines of source would create an ongoing requirement to review Algolia's implementation of that class on every point release thereafter. Short of refactoring the existing implementation into a set of single-responsibility classes, the option to supply its heavyweight dependency--the HttpClient--seems a good first step.

[Ant-hem]

Thanks for your answer. I understand better your request now.

To achieve that, we would also need to overload DefaultSearchClient/DefaultInsightsClient/DefaultAnalyticsClient.create(..) methods to properly inject a CloseableHttpAsyncClient, because ApacheHttpRequester() constructors are not public. I have a good idea how we could do it in a clean way.

However, before going further I'll look if there would be any side-effects sharing the same CloseableHttpAsyncClient instance across multiple Search/Analytics/Clients instances.

Any concerns about it @aseure & @BenoitPerrot?

[skradel]

@Ant-hem I'd propose it would be okay to leave the Default*Clients as-is and direct users to construct (non-default) SearchClient / InsightsClient / etc. explicitly; however, I failed to notice that ApacheHttpRequester is marked final, which suggests that removing the final keyword and/or increasing ctor visibility to public would be possible cures. Alternately, consider shifting the utility methods (performRequestAsync, buildRequest, etc.) to an abstract superclass.

Interestingly ApacheHttpRequester only accesses its HttpClient in two places--to dispatch an async request, and on shutdown.

[Ant-hem]

Thanks - I'll have a look in the coming days, I am pretty busy with another project ATM. I'll write you some requirements we would love for this enhancement.

[aseure]

Hey team 👋 I hope you're doing well. Could you either close that PR or assign it to someone else. I don't work at Algolia anymore and I still have that PR to review in my assigned PR on Github. Thank you and all the best to you folks :)