Closed
Description
Now that all methods on the RestHighLevelClient
are marked as final, it is not possible to be able to effectively decorate the RestHighLevelClient
class. The use case I have for decorating this class is to provide some high level metrics around the usage of the client from our application code.
One possibility would be to introduce some interface that the RestHighLevelClient
implements would allow the class to be wrapped and ensure that all methods are accounted for if there is a addition or removal of a method.
This issue comes from a conversation from #27238 (comment) that changed all of the modifiers to be final.
Activity
elasticmachine commentedon Jun 4, 2018
Pinging @elastic/es-core-infra
Edvinas01 commentedon Jun 11, 2018
This would also be very useful when creating mocks / writing tests. As now in order to validate
RestHighLevelClient
calls, I have to write my own non-final wrapper which could be substituted and validated against in tests.nik9000 commentedon Jun 11, 2018
About a dozen contributors got together to talk about open issues and this came up. We decided against making this change. Here goes our reasoning:
@Override
ing them would be super confusing.final
feels like the right choice for them.getLowLevelClient
,close
,indices
?So we came to conclusion that folks should make their own delegates and mock them if they need mocking, that way folks don't have a huge interface with weird stuff on it that they never use floating around.
I admit a good deal of personal bias here: I've never liked mocking storage systems in tests. The tests just end up as brittle rituals that don't actually prove anything. But I've found mocking a reasonably high level construct around the storage system to be quite useful in testing the rest of my application. My experience with this kind of thing is fairly out of date but the other Elasticsearch contributors agreed so I have that going for me.
purple52 commentedon Jan 28, 2019
The original reporter of this issue essentially needs to be able to proxy
RestHighLevelClient
for gathering metrics. This is the same need that brought me here. The discussion referred to above seems to completely miss the point, focusing on mocking. If it's so difficult to decide what should go in an interface, then maybe the purpose of the class needs a bit of a rethink? Having to write a delegate class yourself makes using the client in reusable way in library or framework code impossible.Edvinas01 commentedon Jan 29, 2019
@purple52 indeed mocking is only a tiny portion of what an interface would offer to its users and maintainers of this utility.
For example, if an interface were used and some factory or builder which would return instance of this interface, the maintainers would have the freedom to do what ever they want with
RestHighLevelClient
class (change its naming, constructor signature, return types - assuming that they would be interfaces as well). Now if you try to do any of those, quite a few people will be quite unhappy when upgrading.clakech commentedon Jan 24, 2020
Hi, here are my 2 cents after a discussion on twitter https://twitter.com/cyril_lakech/status/1219356360766316545
You should not try to avoid confusion for your users by blocking them. Document your recommandations/warnings and if users want to extend the client, it is their problems, not yours.
Unit tests are not Integration tests. Unit tests provide quick feedbacks, integration tests are slower.
This is not easy to mock final in Java (performance are so bad on mockito on this). Yes this is a Java problem. Still you provide a Java driver and should take care of ecosystem capabilities from the trenches and not only take care of the "good" design. A perfect design is not the good one here.
I am ok to create a delegate for the client, but how to be able to use final SearchHit in unit tests ? I can't.