-
Notifications
You must be signed in to change notification settings - Fork 12
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
Retry when discovery document fetcher fails #133
Conversation
fa8ffe6
to
eff4315
Compare
eff4315
to
575e810
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach seems reasonable, just a bit confused about the change to the implicits
ws.url(config.discoveryDocumentUrl).get().map(r => DiscoveryDocument.fromJson(r.json)) | ||
|
||
private def discoveryDocument: Future[DiscoveryDocument] = | ||
discoveryDocumentHolder.updateAndGet(ddf => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ddf
isn't a great name but I can't think of anything better.
@@ -45,7 +46,7 @@ class OAuth(config: OAuthSettings, system: String, redirectUrl: String) { | |||
} | |||
|
|||
def redirectToOAuthProvider(antiForgeryToken: String, email: Option[String] = None) | |||
(implicit context: ExecutionContext, request: RequestHeader, ws: WSClient): Future[Result] = { | |||
(implicit context: ExecutionContext): Future[Result] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't immediately obvious to me why you have moved these implicit parameters to the class level
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're not strictly necessary, but it made life a bunch easier to move the implicits for the discoveryDocument
method up to be shared throughout the class, so that I can make the initial discovery doc request at construction. Once they're up at the class level, these implicits become unnecessary, because the values used in the construction of the OAuth class are also the values that are passed to redirectToOAuthProvider
.
@guardian/digital-cms
Motivated by seeing an example where DNS resolution failed for the discovery document, and that failure was held for the rest of the instance's lifetime. If when asking for the discovery document future, we see that the fetch has failed; replace the future with a fresh fetch.