Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AuthenticationClient methods imply Async #14

Open
mfulgo opened this issue Jul 18, 2018 · 3 comments
Open

AuthenticationClient methods imply Async #14

mfulgo opened this issue Jul 18, 2018 · 3 comments
Assignees
Labels

Comments

@mfulgo
Copy link

mfulgo commented Jul 18, 2018

The method signatures on the AuthenticationClient seem a bit conflicting to me: The methods both return the AuthenticationResponse as well as feeding that value to the AuthenticationStateHandler before returning. Typically, when passing in a handler such as this, I'd expect the method to be asynchronous and to return void.

What I'd propose is to restructure it a bit so a) the methods do not take the handler and b) the case logic lives in the handler or a separate class.

AuthenticationStateHandler handler = createMyHandler();
AuthenticationResponse response = client.authenticate(user, pass, relayState);
handler.handle(response);
 // or response.process(handler); or something else

Perhaps there's a reason for this approach, but I didn't find one in the commit history. If there's a better place to have this discussion, please feel free to redirect me and close this.

@bdemers
Copy link
Contributor

bdemers commented Jul 19, 2018

@mfulgo Thanks! this is a great place for that discussion. This thought had crossed my mind, but I wasn't sure how developers would prefer to consume it. My preference would be returning void with the handler as an argument. Though your suggestion above is nice too.

Neither of these options require the method to be async, it all depends on how the method is called. This lib could be used in a CLI or a webapp.

What type of application were you thinking about using this lib with?

Great feed back, thanks!

@mfulgo
Copy link
Author

mfulgo commented Jul 19, 2018

@bdemers Perfect. So, in my particular use case, I'm implementing a proxy/broker. For me, the problem with the handler is that it actually gets in the way of returning a response, but a void return would be worse.

var response: AuthenticationResponse = null
val handler = new AuthenticationStateHandlerAdapter {
  override def handleUnknown(authResp: AuthenticationResponse) = {
    response = tweakAuthResponse(authResp)
  }
}
client.authenticate(user, pass, relayState) // Assuming it returns void
ok(response)

However, if it returns the AuthenticationResponse, it simplifies quite a bit:

val authResp = client.authenticate(user, pass, relayState)
val response = tweakAuthResponse(authResp)
ok(response)
// Or just...
ok(tweakAuthResponse(client.authenticate(user, pass, relayState)))

This gets a little more complicated when dealing with Futures and thread pools, but I think the second approach gives you more control over that as well. (i.e. What context is executing the http request vs handling the response. If the client methods take in the handler, the only option is to have the client thread execute the handler methods.)

@bdemers
Copy link
Contributor

bdemers commented Jul 19, 2018

Thanks @mfulgo great examples and points! I'll chew this over a bit more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants