Replace callable with Endpoint factory

[kimpepper]

Description

As part of #233 we want to modernise the API in terms of getting endpoints.

This PR replaces the callable for getting endpoints with a strongly typed EndpointFactory.

In addition, the key for the endpoints is the fully qualified class name. This allows us to use the ::class constant (e.g. \OpenSearch\Endpoints\Bulk::class) which provides an improved DX, autocompletion, phpstan type checks etc.

The use of generics in the method docblock allows IDEs and phpstan to correctly assign the return type

    /**
     * Gets an endpoint.
     *
     * @phpstan-template T of AbstractEndpoint
     * @phpstan-param class-string<T> $class
     * @phpstan-return T
     */
    public function getEndpoint(string $class): AbstractEndpoint;

Issues Resolved

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[dblock]

Same as the other PR, let's look at the changes that are just the non-generated code? I merged #235 so you can rebase, but still make sure you're not modifying any generated code, only the templates used to generate it. If the project doesn't build/test anymore without those, at least make two separate commits (one of the change, the other for re-generating specs) so we can see the actual code changes in the diff.

[kimpepper]

@dblock done ✔️

I'm not sure how the automated generation cron job works. Does it work on PRs too? Do you need to merge first?

It would be great if you could generate a specific commit hash of the spec rather that only the latest.

[shyim]

I think we need to support both callback and your new factory. Otherwise it's an break

[kimpepper]

OK we should we trigger a deprecation error then, and specify we will drop support of callable in the next major ie. 3.0.0?

[shyim]

yes sounds good

[kimpepper]

Added BC support for the deprecated $endpoints property.

[kimpepper]

Thanks for your review. I've addressed your feedback and provided repllies where I disagree.

[JoshuaBehrens]

Thank you for approaching this. It looks really helpful. I just had to add a default parameter to bulk and index this week. With my approach (which is super hacky) I had to build up a lot of technical debt. Now I can just decorate the factory. And can use the shared serializer without making up one my own as the one used in a built Client instance is not retrievable.

Super hacky code

<?php

declare(strict_types=1);

namespace App\Test\Support\ElasticSearch;

use OpenSearch\Client;
use OpenSearch\ClientBuilder;
use OpenSearch\Endpoints\Bulk;
use OpenSearch\Endpoints\Index;
use OpenSearch\Serializers\SmartSerializer;
use Psr\Log\LoggerInterface;
use Shopware\Elasticsearch\Framework\ClientFactory;

/**
 * This factory ensures, that bulk and index steps are waiting to be completed on elasticsearch side.
 * This way the next request can expect to find the indexed documents in a search.
 *
 * @deprecated This is a bare copy with just a slight adjustment on the builder. Re-evaluate in case you update.
 */
final class WaitForBulkClientFactory extends ClientFactory
{
    /**
     * @param array{verify_server_cert: bool, cert_path?: string, cert_key_path?: string} $sslConfig
     */
    #[\Override]
    public static function createClient(string $hosts, LoggerInterface $logger, bool $debug, array $sslConfig): Client
    {
        $hosts = \array_filter(\explode(',', $hosts));

        $clientBuilder = ClientBuilder::create();
        $clientBuilder->setHosts($hosts);

        if ($debug) {
            $clientBuilder->setTracer($logger);
        }

        $clientBuilder->setLogger($logger);

        if ($sslConfig['verify_server_cert'] === false) {
            $clientBuilder->setSSLVerification(false);
        }

        if (isset($sslConfig['cert_path'])) {
            $clientBuilder->setSSLCert($sslConfig['cert_path'], $sslConfig['cert_password'] ?? null);
        }

        if (isset($sslConfig['cert_key_path'])) {
            $clientBuilder->setSSLKey($sslConfig['cert_key_path'], $sslConfig['cert_key_password'] ?? null);
        }

        // BEGIN CHANGE
        $clientBuilder->setEndpoint(function ($class) {
            $serializer = new SmartSerializer();

            if ($class === 'Bulk') {
                return new class($serializer) extends Bulk {
                    #[\Override]
                    public function setParams(array $params): void
                    {
                        // this is where the magic happens
                        $params['refresh'] ??= 'wait_for';

                        parent::setParams($params);
                    }
                };
            }

            if ($class === 'Index') {
                return new class() extends Index {
                    #[\Override]
                    public function setParams(array $params): void
                    {
                        // this is where the magic happens
                        $params['refresh'] ??= 'wait_for';

                        parent::setParams($params);
                    }
                };
            }

            $fullPath = '\\OpenSearch\\Endpoints\\' . $class;

            $reflection = new \ReflectionClass($fullPath);
            $constructor = $reflection->getConstructor();

            if ($constructor && $constructor->getParameters()) {
                return new $fullPath($serializer);
            } else {
                return new $fullPath();
            }
        });
        // END CHANGE

        return $clientBuilder->build();
    }
}

@shyim would you mind following up to do a change in shopware/elasticsearch to inject the endpoint factory via DI? Or shall I go there :)

[dblock]

I came here to say that I am so grateful to @kimpepper for picking the PSR work up and for @stof foe the CRs and @shyim for your support.

[dblock]

@stof looking at you to comment when you’re satisfied with the PR, @kimpepper iterate to green

[stof]

To make unit tests pass, you will need to regenerate the client and namespaces in this PR instead of changing only the templates. And you also need to remove the CI jobs running on PHP 7 due to the new PHP requirement.

[kimpepper]

@stof

To make unit tests pass, you will need to regenerate the client and namespaces in this PR instead of changing only the templates. And you also need to remove the CI jobs running on PHP 7 due to the new PHP requirement.

I think we should merge #238 first to avoid a load of unrelated changes.

PHP 7 was recently removed in #239

[dblock]

I think we should merge #238 first to avoid a load of unrelated changes.

Just did. That was a clean update from spec. Rebase.

[kimpepper]

OK pushed latest generated code and all tests are green ✔️

[dblock]

This looks good, thanks @kimpepper! I'm going to merge this, @shyim do take a look in case something was amiss. @stof much thanks for the iterations/CRs.