Upgrade from deprecated app-search package to enterprise-search

jakxnz commented 2 years ago

Raising this draft for Travis CI perks.

I am conscious that https://github.com/silverstripe/silverstripe-search-service/pull/69 exists.

chrispenny commented 2 years ago

Trying as best I can to share my learnings and difficulties below.

Elastic Client contract has changed

As part of our EnterpriseSearchService we instantiate an Elastic Client.
This Elastic Client used to contain all of the methods that we interact with (EG: addDocuments()). This is no longer the case
The Elastic Client now has an extra level of abstraction.
We now need to access $client->appSearch() or $client->enterpriseSeearch() before we are able to access the original methods.
Furthermore, the methods used to take in basic params, whereas they now need us to instantiate a Request and pass that through.

Practical example

Before:

$this->getClient()->indexDocuments(static::environmentizeIndex($indexName), $docsToAdd);

After:

$this->getClient()->appSearch()
    ->indexDocuments(new IndexDocuments(static::environmentizeIndex($indexName), $docsToAdd))
    ->asArray();

Most of these should be updated already as part of my commit (though I've only validated the configure method so far)

At first, this doesn't seem like a big deal, and to be honest, for the practical application it isn't a big deal. However, because of the way that our unit tests are mocked, I think this is a significant paradigm shift that is going to completely invalidate the way that we create our test mocks.

I think our Unit Test paradigm is going to have to switch significantly

I apologise in advanced if I don't explain this very well... I barely understand it all myself.

EnterpriseSearchServiceTest currently mocks out the Elastic Client using SearchServiceTest::mockClient();. This method is creating a MockBuilder that mocks particular methods, such as indexDocuments, deleteDocuments, etc.

The problem is... These methods are no longer present in the Elastic Client like they were before, they're now in that next level of abstraction (appSearch).

I don't think we can "mock" appSearch methods using this MockBuilder.

I also don't know if we actually want to use the MockBuilder even if we could? From what I can tell, this mocks the response from a method (EG: it mocks the response from the indexDocuments() method?), as apposed to what (I think) we want to do, which is to instead mock the response from the API (wherever it happens).

So how do we mock the API responses

Yea... I'm struggling with this one a bit. Here's what I have so far. Untested, but I think it'd work.

When we instantiate an Elastic Client, we have the ability to pass in some $config. Any time we access Client::appSearch() or Client::enterpriseSearch() it instantiates (what they call) a TransportBuilder and it passes this builder the $config that we have for this Client.

Here is the method where the TransportBuilder is instantiated. We cannot inject over this, however the initBuilder() and build() methods both have some interesting/useful stuff.

private function buildTransport(array $config): Transport
{
    $builder = TransportBuilder::create();
    $this->initBuilder($builder, $config); // important
    $transport = $builder->build(); // important
    $this->initTransport($transport, $config);

    return $transport;
}

(Removed some stuff to focus in on what is important).

private function initBuilder(TransportBuilder $builder, array $config): void
{
    ...
    foreach ($config as $key => $value) {
        $set = 'set' . ucfirst($key);
        if (method_exists($builder, $set)) {
            $builder->$set($value);
            unset($config[$key]);
        }
    }
}

Ok, so it looks through all of our $config and checks to see if there are matching methods on the TransportBuilder. The key method that does exist on the TransportBuilder is setClient().

public function setClient(ClientInterface $client): self
{
    $this->client = $client;
    return $this;
}

And then when build() is called...

public function build(): Transport
{
    $connectionPool = $this->connectionPool ?? new SimpleConnectionPool;
    $connectionPool->setHosts($this->hosts);

    $transport = new Transport(
        $this->client ?? new GuzzleClient,
        $connectionPool,
        $this->logger ?? new NullLogger
    );
    return $transport;
}

Yaaaay! There's our Transport, and if we have set a client (say, to a GuzzleClient that has a MockHandler and HandlerStack already instantiated), then we should be able to mock responses that are being called through our service???

Ok, so practical example?

Untested, but I think it'd look something like this:

class SharepointCoordinatorTest extends SapphireTest
{

    protected ?MockHandler $mock;

    protected EnterpriseSearchService $searchService;

    protected function setUp(): void
    {
        parent::setUp();

        // Set up a mock handler/client so that we can feed in mock responses that we expected to get from the API
        $this->mock = new MockHandler([]);
        $handler = HandlerStack::create($this->mock);
        $client = new GuzzleClient(['handler' => $handler]);

        $config = [
            'host' => 'https://api.elastic.com',
            'app-search' => [
                'token' => 'test-token',
            ],
            'Client' => $client,
        ];

        $elasticClient = new ElasticClient($config);
        $indexConfiguration = Injector::inst()->get(IndexConfiguration::class, true, 'dev-test');
        $documentBuilder = Injector::inst()->get(DocumentBuilder::class);

        $this->searchService = EnterpriseSearchService::create($elasticClient, $indexConfiguration, $documentBuilder);
    }

    public function testSomething(): void
    {
        $headers = [
            'Content-Type' => 'application/json;charset=utf-8',
        ];
        $body = json_encode([]);

        $this->mock->append(new Response(200, $headers, $body));

        // Start invoking methods that trigger API calls, and they should respond with the mock above
        // This is a "stack" as well, so you can add more than one response if you need to. They get "popped"
        // in the order that they are added
    }

}

jakxnz commented 2 years ago

After a chat with @chrispenny , we designated the direction of this PR's changes to be earmarked as a silverstripe-search-service@2.0, which takes us from abandoned dependencies to the latest alternative to those dependencies

e.g from elastic/app-search@7.6 to elastic/enterprise-search@^7.17

chrispenny commented 2 years ago

Closed in favour of #76

silverstripe / silverstripe-search-service