Add onAuthenticationFailure callback to Authenticator#1610
Add onAuthenticationFailure callback to Authenticator#1610iyogi wants to merge 1 commit intoapache:4.xfrom
Conversation
|
Thanks for the PR @iyogi! Have you signed the Contributor License Agreement for contributions to DataStax open source projects? If not you can find it at https://cla.datastax.com/. Thanks! |
Yes, have just signed it. |
absurdfarce
left a comment
absurdfarce
left a comment
There was a problem hiding this comment.
Overall I like this change quite a bit. The enhancement of the symmetry of Authenticator is probably worth it by itself. I do think we need to make the impl more consistent with the rest of Authenticator but hopefully that shouldn't be an enormous change.
Thanks again @iyogi !
| * | ||
| * @param exception contains authentication error message | ||
| */ | ||
| default void onAuthenticationFailure(AuthenticationException exception) {} |
There was a problem hiding this comment.
This should return a CompletionStage in order to better fit in with the other methods on this interface. I agree with making it a default method (since that avoids breaking existing impls) but the default should return something like new CompletableFuture().complete(null)
| ((Error) response).message))); | ||
| ((Error) response).message)); | ||
| authenticator.onAuthenticationFailure(cause); | ||
| fail(cause); |
There was a problem hiding this comment.
Keep in mind that Authenticator.onAuthenticationFailure() could itself throw an exception. We should handle that case gracefully here and throw a different AuthenticationException if that occurs. That's the point of this code pattern elsewhere within ProtocolInitHandler:
authenticator
.onAuthenticationSuccess(token)
.whenCompleteAsync(
(ignored, error) -> {
if (error != null) {
fail(
new AuthenticationException(
endPoint,
String.format(
"Authenticator.onAuthenticationSuccess(): stage completed exceptionally (%s)",
error),
error));
} else {
step = Step.GET_CLUSTER_NAME;
send();
}
},
channel.eventLoop())
.exceptionally(UncaughtExceptions::log);We should probably do something similar here in order to clearly distinguish between these two cases. Note that the API change re: making onAuthenticationFailure() return a CompletionStage facilitates such a change.
| // verify that onAuthenticationFailure callback was invoked | ||
| assertThat(authenticator.authFailure).isTrue(); | ||
| } | ||
|
|
There was a problem hiding this comment.
This test winds up being almost identical to should_fail_to_initialize_if_server_sends_auth_error(). I guess for me it's probably cleaner to add a test for this functionality into the existing test (by adding an impl of onAuthenticationFailure() to the mock) and changing it's name to something that indicates we're validating all behaviours in the AuthError case rather than just the failure to initialize.
Does that make sense?
2 participants
Add
onAuthenticationFailure()callback to the Authenticator lifecycle methods so that authenticator instances are made aware of authentication failures as well - which would truly complete the lifecycle callbacks.