Proposal for NullClient

[chloelbn]

Several conversations have been around about how to handle during development or test the calls made to Algolia.

After some thoughts the solution I propose would be to have a new settings in the algolia_search parameters that would be http_client. This would give the opportunity for the customer to pass any requester they like, including a NullClient, and solve the issue of Algolia calls on the lowest level.

Based on a NullClient written by @ostrolucky in this issue, I came up with an example client that handles roughly the expected response formats:

<?php

namespace App\Client;

use Algolia\AlgoliaSearch\Http\HttpClientInterface;
use Algolia\AlgoliaSearch\Http\Psr7\Response;
use Psr\Http\Message\RequestInterface;

class NullClient implements HttpClientInterface
{
    /**
     * {@inheritDoc}
     */
    public function sendRequest(RequestInterface $request, $timeout, $connectTimeout)
    {
        return $this->formatResponse($request);
    }

    private function formatResponse(RequestInterface $request): Response
    {
        if (false !== strpos($request->getUri()->getPath(), 'query')) {
            return new Response(200, [], '{"hits":[]}');
        }

        if ($request->getMethod() === 'DELETE' || false !== strpos($request->getBody()->getContents(), 'deleteObjects')) {
            return new Response(200, [], '{"deletedAt":\'2019-09-09T15:33:13.556Z\', "taskID": 999}');
        }

        if (false !== strpos($request->getBody()->getContents(), 'addObjects')) {
            return new Response(200, [], '{"createdAt":\'2019-09-09T15:33:13.556Z\', "taskID": 999, "objectID": 999}');
        }

        return new Response(200, [], '{"updatedAt":\'2019-09-09T15:33:13.556Z\', "taskID": 999}');
    }
}

and the config would be as follow:

# config/packages/{dev, test, ...}/algolia_search.yaml
algolia_search:
  http_client: 'App\Client\NullClient'

IMHO, the drawbacks of the different other solutions were as follow:

• NullSearchService : too much code duplication to handle the communication with DB
• NullEngine : the bundle would not be Engine-agnostic anymore, which is not what we wanted.
• Changing the instantiation of ClusterHosts as proposed in #596 : the ApiWrapper shouldn't rely on the bundle config. Moreover, Algolia::setHttpClient already lets you set a new http client, so I'm not sure what are the advantages of wanting to set a whole new Client with a whole new ApiWrapper.

WDYT?

[ostrolucky]

Would you mind explaining advantage of following in config_test.yml

algolia_search:
    http_client: 'App\Client\NullClient'

over doing following in services_test.yml?

services:
    algolia.http_client: 'App\Client\NullClient'

I don't see it.

Anyways, doing algolia/algoliasearch-client-php#596 is a prerequisite to do either of these, otherwise where would you put Algolia::setHttpClient call?

[chloelbn]

@ostrolucky It's just to have a dedicated algolia_search.yml file in each environment, the same way it's done for doctrine for example. If you rather do it in services.yml I don't think it makes a difference as long as it's declared only once in each env.

As for the call to Algolia::setHttpClient, I intended to do it in this file.

[ostrolucky]

Services are not available in that file, only their definitions. And I don't know how would you prepare definition for random static call. Result of this call is not a service, so it can't be service definition. Hence I think we should do this properly, inject correct stuff from beginning and not rely on Algolia::setHttpClient

I don't think it makes a difference as long as it's declared only once in each env.

It doesn't make much difference for user, but does make difference for bundle maintainers, as new configuration requires maintainance of extra code. As for userland, it is slightly more clear, but IMHO it makes more sense to teach users how to override services for specific environment, which is universal for any bundle, without having to rely on bundle maintainers to provide options and is pretty easy in Symfony, just not very well known. Heck, you don't even need extra services file, user can just as well put

services:
    algolia.http_client: 'App\Client\NullClient'

to services_test.yml too.

[chloelbn]

And so where would you put the call to setup Algolia with its new client?

[ostrolucky]

This needs to be split into more services

search-bundle/src/Resources/config/services.xml

Lines 18 to 22 in 7b02cea

	<service id="search.client" class="Algolia\AlgoliaSearch\SearchClient" public="true" lazy="true">
	<factory class="Algolia\AlgoliaSearch\SearchClient" method="create" />
	<argument key="$appId">%env(ALGOLIA_APP_ID)%</argument>
	<argument key="$apiKey">%env(ALGOLIA_API_KEY)%</argument>
	</service>

instead of using create factory method.

Following is what is currently required from userspace to do - that's the wiring difficulty I was talking in #317 (comment)

<service id="AppBundle\Test\Algolia\TestClient" />
<service id="Algolia\AlgoliaSearch\Config\SearchConfig" />
<service id="search.client" class="Algolia\AlgoliaSearch\SearchClient" public="true" lazy="true">
    <argument type="service">
        <service class="Algolia\AlgoliaSearch\RetryStrategy\ApiWrapper">
            <argument id="AppBundle\Test\Algolia\TestClient" type="service"/>
            <argument id="Algolia\AlgoliaSearch\Config\SearchConfig" type="service" />
            <argument type="service">
                <service class="Algolia\AlgoliaSearch\RetryStrategy\ClusterHosts">
                    <factory class="Algolia\AlgoliaSearch\RetryStrategy\ClusterHosts" method="createFromAppId" />
                    <argument> </argument>
                </service>
            </argument>
        </service>
    </argument>
    <argument id="Algolia\AlgoliaSearch\Config\SearchConfig" />
</service>

Goal is for bundle to define http client service, so userspace needs to override just that one, instead of whole chain of dependencies, as user currently needs to override search.client.

[chloelbn]

Okay, I see what you want to achieve and while this is not a bad idea, I think we're drifting away from our initial implementation. Plus I don't want to update the PHP Client to achieve this particular need for the Symfony integration.
We need to move on and deliver the bundle so I'll stay with my original idea of letting the user create a new class implementing the SearchServiceInterface and propose an example NullSearchService class to guide them. After all it's Algolia as a whole that needs to be abstracted, not the requester.

@ostrolucky you were really helpful in here and thanks a lot for all your ideas and suggestions! Please keep on sharing your thoughts with us.

I'm closing this issue.

[chloelbn]

See #320