[POC/RFC] Extract Connection logic out of the Client

[merk]

As part of the SOLID principles is the Single Responsibility principle which I try to follow in designing services. It is my believe the Client shouldnt know anything about the internal workings of the ConnectionPool.

In this PR, I've removed quite a bit of "magic" and made creation of Connection objects and the ConnectionPool explicit - but this is about starting a discussion (along with #1069) about direction. I would expect to add helpers to simplify the default setup of these classes for 90% of the use cases encountered.

Is this something I should explore further and get to a working state where tests pass?

[ruflin]

This also somehow overlaps with #944 In the end Elastica itself should not have to handle the complexity of connectors. I'm not even sure if it will need a connector interface, all it needs to do is the forward configuration options.

This PR seems to be from my point of view almost too radical as it seems to break even the simplest usage of Elastica\Client as far as I understand. I would actually hope that the implementation of #944 makes it much more obvious how we could proceed here and remove all the connection dependencies.

[merk]

Yeah, I agree this is radical. I'd expect to back it off and make it simpler to actually use as a next step, it should be possible to achieve without breaking BC for normal users. Mostly just some tinkering I did while sorting out the other PRs.

[ruflin]

Cool, I really like to discuss new ideas based on PR's as it often says more then describing it.

[merk]

I've improved a bit more on this proposal:

First Commit (Interfaces)
It always helps to simplify the relationships between classes by defining interfaces for methods that are required for communication/use. In some cases they could probably be removed if there is never going to be a different implementation of the Interface.

Second Commit (Deprecations)
Theres no point having these methods around forever since the Client isnt the responsible class for managing connections.

Third Commit (Extracting ConnectionPool creation)
This is the meat of the PR - removing the handling of ConnectionPool creation. The Client now accepts a $config array as before or a ConnectionPoolInterface directly. This should be almost 100% BC, but I havent run extensive tests yet.

Fourth Commit (Tests)
The removal of $_config from Client means some of the tests no longer make sense (or ultimately need to be moved). There is a unit test failure in ClientTest that is a BC break - being able to set a host or port on the Elastica Client and expect the Connections to be updated. In my opinion this isnt a good way to proceed and shouldnt be a very big BC break for most users to deal with.

[merk]

@ruflin Did you disable travis as well?

[ruflin]

@merk At least not intentionally :-( I only made changes on codecov.io so it should not have any affects :-(

[merk]

A few pushes here didnt seem to trigger travis builds :\

[ruflin]

Yeah, not sure why :-( Just checked the configs on travis and all looks good.

[merk]

After running the docker tests, there dont appear to be any failures (after some slight adjustments of the test cases).

I've pushed a new commit that fixes test cases and documents the BC break (and removes the offending test). This is the test on master: https://github.com/ruflin/Elastica/blob/master/test/lib/Elastica/Test/ClientTest.php#L1176-1192

[ruflin]

I think we should proceed with these changes. I had a look again and I think it is partially going to overlap with #944 but at the same time it could actually simplify #944 . Not sure yet and also not sure when I get the time to do #944 so we should not be blocked by it.

Is the CHANGELOG still up-to-date? Can you rebase on master? This should hopefully also solve the travis build issue and make starting ES more stable.

[merk]

This does simplify moving to php-elasticsearch because ultimately you'd just need to wrap the php-es Client with a wrapper that implements \Elastica\Connection\ConnectionInterface for the first stage.

I believe this is still up to date, I'll take a look at master when I get home tonight and do a rebase. Only thing to note here is I think this PR should cause a minor bump, not a patch bump.

[merk]

To note, this reverses #1077

[merk]

Rebased.

But something to consider before merging this is that we've removed the $config array methods from the Client which is a hard BC break. These methods don't have an analog anymore since the $config array is immediately consumed in the creation of the ConnectionPool and cannot be changed after initialisation.

We can either implement these methods as a noop with a deprecation/php notice, by throwing an exception with more information, or by leaving them removed.

[ruflin]

@merk Two notes:

• About Added connect() method to allow the client to connect to server when … #1077: I'm not 100% sure what we should do here. Is that still possible through the Connection object?
• How much additional work would it be to introduce the "proxy" functions with deprecated warnings?

[ruflin]

@merk It seems one of the connection tests is broken.

[merk]

Regarding #1077, no - the concept of changing the runtime configuration of the Elastica Client is no longer present at all and the mechanism that connect() was added for is no longer supported as per the documented BC break.

Theoretically, if someone wanted to emulate the behaviour of changing connection details after a Client is created, they would need to implement their own ConnectionPoolInterface which can do whatever the implementor wants, as long as getConnection() returns a ConnectionInterface.

I might be able to do something to keep a bit more BC in terms of the getConfigValue/setConfigValue methods, but it will increase the complexity of the ConnectionPool quite a bit. Give me a few days to think about it.

[ruflin]

@aguvillalba Have a look at the comments above, would be good to have you in this conversation.

@merk An alternative idea here is that instead of having something like connect, is that it is possible to assign a new ConnectionPool to the client. So if someone wants to add / remove connections at a later stage, he has to exchange the complete Connection Pool.

[merk]

If you know ahead of time that you need custom connection pool functionality, it isnt very complicated to pass a ConnectionPoolInterface implementation that is going to handle changing connection details - it isnt really necessary functionality to supply in the default implementation.

We could have a second ConnectionPoolInterface that handles this separately - but my thinking right now to try to avoid BC breaks might revolve around providing a more advanced ConnectionPool that we might need to ship anyway.

[merk]

Another thing to consider is if the Interfaces that have been added are optimal: if we create these interfaces they become codified for use by others. The whole split of Connection/Transport becomes something that needs to be supported.

It might be worth considering a simplified approach?

[ruflin]

As you know, I'm less hesitant to break BC then you are if it is the right way to go in the long run. So the question I ask myself, what is going to happen to Connection Pool and Transport when we get elasticsearch-php in? TBH I haven't thought this part through yet :-(

[aguvillalba]

Hi @ruflin and @merk! I understand and completely agree with the idea of taking out from the Client all the logic to connect to the server.
If I did not misunderstood, from now on, instead of doing:

$client = new Client();
$client->setConfigValue('host',$host);
$client->setConfigValue('port',$port);
$client->connect();

We should do:

$client = new Client();
$connection = new Connection(array('host' => $host, 'port' => $port));
$client->addConnection($connection);

And after this, we could use the $client as usually, is it right?

Or what would be the client's API to create an "empty" client and inject the connection settings later?

Thank you both for these improvements!

[merk]

Right now, this PR would require either setting the config array as previously supported in the Client Constructor:

$client = new Client(['servers' => /*...*/]);

Or setting a ConnectionPool:

$connections = []; // An array of ConnectionInterface objects
$pool = new ConnectionPool($connections)
$client = new Client($pool);

There is no method for creating an empty Client. It is possible to achieve what you're asking for by creating a custom ConnectionPool which can have its connections modified later, but the Client requires a reference to that pool on creation.

[aguvillalba]

But this would make the Client less flexible, wouldn't it? You are forcing the developer to know all the connection details (either the array or the pool) before creating the client instead of making them optional and set the connection details later. There are cases where the developer needs to set the connection details after creating the client.
As far as I see in PR changed files, all the parameters in the Client's constructor are optional, so the pool is not mandatory in the moment of the client's creation.
Besides that, I also think that this approach exposes many internal logic to the outside, I mean: the consumer of the library, in order to create and use just the client, must know about Connection objects, ConnectionPool object and the Client object itself.
I would suggest to allow the developer to create an "empty" client (with an empty ConnectionPool in it) and later inject the connection into the client (which actually injects the connection into the connectionPool) trying to avoid to expose the ConnectionPool to the outside.
What do you guys think?

[aguvillalba]

In order to not make my app dependent on Elastica, I created an ElasticaClientAdapter which implements a SearchClient interface. So my app actually uses a SearchClient (today is Elastica, tomorrow could be some other and I do not have to change my app code, only the adapter).
The adapter takes the Elastica\Client object and MyAppConfig object as its constructor params. When I define this dependencies in my dependencies injection file, I do not know yet the connection parameters (because the come with the Config object), so they are introduced in the Elastica\Client during the execution of the Adapter's constructor based on the parameters defined in the configuration of the app. So:

1. Create the "empty" client (in autoloading for dependency injection)
2. Read connection parameters from app config (in adapter's constructor)
3. Inject connection parameters to the client (in adapter's constructor)
   It's a legitimate circumstance, isn't it? Anyway, since you allow to create a Client without any parameter (all client's constructor parameters are optional), then you should provide an easy way to introduce these parameters later, via setters, and make the client still works.

which is bad in itself - $client->request() will fail to work without those details
Of course it will fail, but what's the problem? Your request is failing because you did not set a connection into the client, it's normal. Throw an exception.

The lack of flexibility is intentional - supporting every use case for every edge is not conducive to maintainable code.
Well, a flexible library does not imply that its code is non-maintainable. It only depends how it is designed.

As I said before, making the library (exactly the Client) more flexible does not imply less maintainable code.
Also, by exposing the ConnectionPool to the user, you are forcing him to know about a Client's detail implementation that he does not need to know. I do not care if the Client uses a ConnectionsPool or other approach. I simply need to know that I can add connections to the client. What if in the future you decide to change this approach to something else? You are creating a backward compatibility problem that could be avoided.

[merk]

Your dependency injection container is the correct place (which will already know everything required) to manage the creation and configuration of the Client and its Connections.

Alternatively, as I've stated multiple times, a custom ConnectionPool can be used. It is no more onerous as your SearchClient. And even then, your SearchClient could manage the lifecycle here itself and create the Elastica client when you have the information required.

And third, as this PR demonstrates - following the SRP and reducing the edge cases that the Client is required to follow results in a reduction of code complexity both in lines and mental model required to maintain the code.

[aguvillalba]

Ok, I do not agree at all with:

as this PR demonstrates - following the SRP and reducing the edge cases that the Client is required to follow results in a reduction of code complexity both in lines and mental model required to maintain the code.

Flexibility does not imply complex code, as I said, it depends on how you design it.

Your dependency injection container is the correct place (which will already know everything required) to manage the creation and configuration of the Client and its Connections.

Based on that, should I also put my connection parameters in the dependency injection container? Not in the app config?

Anyway, @ruflin, it's ok, it's your library and you decide its route map. I think I will stick to the version 3.2 which is the one that works fine for me and maybe it's a good chance to start my own package from now on.

Thank you very much for your effort!
Cheers guys!

[merk]

@ruflin rebased, Callback test fixed

[merk]

I've added a new commit with a LegacyConnectionPool that should theoretically support the getConfig_/setConfig_ methods on the Client in a BC way, but because there is no functional or unit test coverage for the expected behaviours it might be subtly different.

If this is actually something to be merged, test coverage has to be modified and the LegacyConnectionPool will need a full suite of tests depending on if it is a temporary stopgap until a major bump or if it is going to stay around forever.

I think the only thing missing is working through how a new Client should be 'officially' created and tweaking the Client constructor or Builder class to provide a factory if it suits.

[ruflin]

@aguvillalba Thanks a lot for your inputs. I think the client is currently under flux to make it more reusable for others to extend. To go the right path, it is important to gather together the different facts and opinions. I believe only in combining them we will find the best solution. Sometimes if we break stuff, we also have to go backwards and figure out new ways. Please keep pushing for your ideas. :-)

[ruflin]

@merk I really like the addition of the LegacyConnectionPool. I enables us to do the restructuring withouth breaking BC and it shows and example how the ConnectionPool interface can be used. +1 on moving forward here.

[merk]

So I've spent some time thinking about official 'how to start a Client' documentation, a few options:

We leave the $config array the default recommended way of configuring the Client and rely on the LegacyConnectionPool. The deprecated methods on the Client get removed in 4.0 but they can still be accessed on the pool itself.

We have a ClientFactory that returns a configured Client, which could look something like:

class ClientFactory {
    public function create(array $connections = [], $strategy, $logger) {
        // handle creation of a normal pool and default to a
        // localhost connection if no $connections
    }

Introduce developers to the actual way that Elastica works internally (my preferred option)

$connections = [];
$pool = new ConnectionPool($connections);
$client = new Client($pool, $logger);

[merk]

And looking at the Client constructor I want to deprecate the $callback option as well, since if someone wants to have a callback run on failure they can wrap or implement their own ConnectionPool instead.

We could even leave the callback in the LegacyPool

[ruflin]

@merk Not so sure about the settings removal. I see it reasonable to remove most of the options and move it to the connection pool. But for example passing an array of hosts would be really nice. The reason is mainly simplicity as it feels for me quite intuitive that you tell a client just to connect to a list of hosts and the rest happens magically.

An alternative could be to do some magic in the contstructor dependent on which types are passed, means allowing both.

[ruflin]

@merk I think it is now also time to push this one forward again. How should we proceed? Perhaps it still makes sense to do a quick video call to discuss it all in person?

[ruflin]

@merk It hurts me to close this PR as a lot of really good work went into this but I doubt this will be picked up very soon again. And if it does, lets reopen this PR!

@deguif @thePanz Just pinging you here in case you are interested in this work.

[thePanz]

I liked the idea @ruflin , such a shame the PR got lost from 2016!
I'd rather integrate a http://httplug.io library at this point.