Refactor AuthProviderResolver [Breaking Change] #333
Conversation
lbalmaceda
changed the title
| * @return the current builder instance | ||
| */ | ||
| public Builder withProviderResolver(@NonNull AuthProviderResolver resolver) { | ||
| ProviderResolverManager.set(resolver); | ||
| public Builder withAuthHandlers(@NonNull List<AuthHandler> handlers) { |
lbalmaceda
force-pushed
the
refactor-provider-resolver
branch
from
fbea2ed to
b0d6d87
Compare
lbalmaceda
force-pushed
the
refactor-provider-resolver
branch
from
0cabd4b to
6366748
Compare
| * a strategy and connection name. | ||
| * When no AuthProvider is matched, it will return null | ||
| */ | ||
| public abstract class AuthResolver { |
There was a problem hiding this comment.
I don't know what are the best practices for java, but I would have used a regular class with a private constructor instead of an abstract class.
There was a problem hiding this comment.
@nikolaseu Both options are good. Although using a private constructor has the advantage of not letting the user to extend the abstract class and instantiate objects. I will change it. 👍
| * @return an AuthProvider to handle the authentication or null if no providers are available. | ||
| */ | ||
| @Nullable | ||
| public static AuthProvider providerFor(String strategy, String connection) { |
There was a problem hiding this comment.
since there are nullable/nonnull annotations everywhere, add them here too?
| assertThat(resolver, notNullValue()); | ||
| assertThat(resolver, equalTo(customResolver)); | ||
| assertThat(AuthResolver.providerFor("", ""), nullValue()); | ||
| assertThat(AuthResolver.providerFor("a", "a"), is(equalTo(provider))); |
There was a problem hiding this comment.
since the param is a list, maybe test at least two providers? "a" and "b" ?
| break; | ||
| } | ||
| } | ||
| return provider; |
There was a problem hiding this comment.
maybe it's simpler this way?
for (AuthHandler p : authHandlers) {
final AuthProvider provider = p.providerFor(strategy, connection);
if (provider != null) {
return provider;
}
}
return null;
There was a problem hiding this comment.
I think both are acceptable and clear.. But the one I did has 1 return point at the end, maybe that helps a bit reading the code?
| public void onConnectionClicked(String connectionName) { | ||
| lockWidget.onSocialLogin(new SocialConnectionEvent(connectionName)); | ||
| public void onAuthenticationRequest(@NonNull String strategy,@NonNull String connection) { | ||
| lockWidget.onSocialLogin(new SocialConnectionEvent(strategy, connection)); |
There was a problem hiding this comment.
this is just a comment, but maybe the name of this method should better reflect what it does? now it's clear that an authentication has been requested, but the onSocialLogin is not that clear (onSocialLoginRequest?).
Previously, the user needed to define it's own
AuthProviderResolverwhen he was notified of authentication events, asking for anAuthProvidergiven aconnectionName. To simplify that, now the user calls the builder passing handlers:Each handler can decide whether it can or not handle a given strategy/connection name. Because it's a List, the order matters and the first match will return an
AuthProvider. If no provider is found, Lock will default to theWebAuthProvider.This way we can also provide our own handlers in the social native libs.