Client should not have final method nor classes

[cezarykluczynski]

Describe the feature: Most of the methods on RestHighLevelClient are final. That makes them hard to mock, and hard to create decorators. What's the reason behind that? If there is no good reason to keep those methods final, I request that this constraint be dropped. Additionally, that's not the case with native protocol AbstractClient, and almost no methods there are final, despite having the same function as the RestHighLevelClient final methods.

[elasticmachine]

Pinging @elastic/es-core-infra

[javanna]

The final modifier is used for methods that are not supposed to be overridden by subclasses. The fact that such methods are hard to mock is an odd consequence. On one hand testing should be made easy, but I am not sure that this should convert into never using the final modifier. We will discuss this with the team.

[cezarykluczynski]

I get that the final methods should not be overidden. I just wonder why this specific method should not be overriden. I would make sense for stateful objects, that carefully manages their own state, or for some core Java API, but this client is neither.

For an object that is a single entry point to a complicated system, changing it's behaviour is exactly what you want to do, at least some of the time.

When writing clients myself, I sometime spend some time thinking what would be the minimal API that's still does the job (using means like package-private constructors, final methods, not having interface for it), and as the time passes, it become obvious that the nice use cases I had in mind when restricting access to the API aren't enough to fit all the real-world scenarios, and that it's best to leave all methods and classes public and non-final. The client is used in the wild by many different parties, with their different requirements, and they will bother you less when the client is flexible. And if they misuse it, it's their problem to fix it anyway.

[javanna]

@cezarykluczynski are you referring to some specific method? From your initial description of the issue I thought it was a general concern on all of the methods that are currently final.

And if they misuse it, it's their problem to fix it anyway.

We tend to look at this a bit differently as a team these days. But as I said, I marked this for discussion and we will take your concerns into account.

[cezarykluczynski]

@javanna Not a specific method, I'm still talking general here.

I'd be happy to see what the discussion brings.

[nik9000]

For what it is worth I've never liked mocking clients. I feel like lots of the time mocking client's turns tests into a big "mock dance" that doesn't really prove anything. I like wrapping clients in higher level abstractions and mocking those in some tests and having tests for those abstractions that run against the real service. I think that is the "right" way to write Java. But I also like vim more than Emacs so I'm clearly crazy. And folks have asked for this more than once.

I feel like if we wanted to enable mocking we should go all the way and make an interface that the client implements and users can mock that interface. And we can continue to declare our implementations final. I don't like having an interface with only one implementation but in this case I think it is defensible. The interfaces will be pretty big and they return one another which will kill a lot fairies.

:man_shrugging:

I don't really feel like we should go out of our way to enable mocking the client because I feel like mocking the client makes your testing worse. But I really don't have any right to tell you how to write software so actively stopping you from mocking the client also feels silly. If I could declare the client "closed for extension but open for mocking" then I would. But I think the only way to do that is with an interface. Which seems like a fair bit of work just to enable mocking. Which I don't like doing for clients in the first place.

I'm pretty conflicted.

[Garnethil]

@nik9000, i think you are right on creating a interface instead removing the final modifier on the methods. Leaving the implementation separate, opening the door for consumers to mock and validate their code.

When attempting to create DAOs or wrappers around the ES functionality, one should be able to perform unit tests and validate that in fact the parameters sent to the client are what they expect. When testing the interactions with a client, usually it goes into: 1) Do I send what I think I send? 2) Does my code support failure cases from my dependency?

Creating the interface apart from enabling mocks, unlocks using other design patterns for development, i.e. @cezarykluczynski cited decorators - I can think of wanting a more specific logging decorator on top of the client.

[alpar-t]

@nik9000 perfectly agree with the "higher level abstractions" approach that you outline. That's how it should be. The problem with is that more often often than not one modifies code rather than starting from scratch, and sometimes the code that needs modifications might not have great tests. Thus the abstraction is hard to add and leads to a chicken egg problem where it's hard to add any tests, because the client can't be mocked and it's risky to add the abstraction because there are no tests.

That being said, it looks like mocking final methods is incubating in mockito 2.1 - it does come with a performance hit, but at least it is doable.

[dakrone]

Closing this as we've removed the high level rest client in favor of the Java client.