Auth.java: Inconsistent usage of the final keyword degrades usability.

jonastr commented 5 years ago

In Auth.java, we find several final methods, but also non-final methods.

Examples final:

Examples non-final:

This makes it overly complicated to write unit tests for our integration with Onelogin using Mocks (cannot mock final methods). For sure, there are ways to work around this, but before we start working around this circumstance, I would like to understand the rationale behind and/or whether you are willing to adapt this (preferably remove 'final').

Thx a lot!

pitbulk commented 5 years ago

Is just to prevent overrides on some methods that should not be overridden.

Related to unit test, The toolkit contains a lot of them. What are you trying to achieve?

whitelake-city commented 5 years ago

It's basically about Unit tests. If I have code like:

public void login() {
  Saml2Settings settings = buildSamlSettings();
  Auth auth = authenticateUsingResponse(settings);
  if (!auth.isAuthenticated()) {
    // do sth
  }
}

And I want to write a Unit test, I would usually use a mock of the Auth class. And just make sure that isAuthenticated is called. Since the isAuthenticated method is final I can't do that. At least not with Mockito. I could work around the issue by using an incubating feature of Mockito or by using another library or reflection. But that isn't really a nice way to solve the problem.

I see your point with not overriding some core api methods, but that makes for me only sense for projects not used by external parties, as in my own project I can still decide to remove the final keyword. I guess for public libs it doesn't make to much sense in my eyes.

PS: Don't get me wrong, for the core classes this would make much more sense to me. And I do use final in internal projects too.

SAML-Toolkits / java-saml

Auth.java: Inconsistent usage of the final keyword degrades usability. #221