-
Notifications
You must be signed in to change notification settings - Fork 85
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
#215 Add followRedirects support to the Akka Http backend #228
#215 Add followRedirects support to the Akka Http backend #228
Conversation
return CompletableFuture.completedFuture(response); | ||
} | ||
} | ||
|
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.
Should we move this into Akka itself? even if only working on the high level API it'd be nice anyway to have it there?
cc @jrudolph
if (redirectsLeft < 1) { | ||
Future.failed(new IllegalStateException("Maximum redirect reached: " + MaxRedirects)) | ||
} else if (response.status.isRedirection() && config.followRedirects) { | ||
val location = response.header[Location].get.uri |
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.
Probably needs explicit handling of missing Location
header to improve the error message in that case.
val redirectUri = | ||
if (location.isRelative) | ||
if (location.path.startsWithSlash) | ||
request.uri.withPath(location.path) |
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.
You could use location.resolveAgainst(request.uri)
instead.
Closing the akka-http backend related tickets for now. These will be handled in the future. |
This adds followRedirects support to the Akka Http backend.
While adding tests for the followRedirects functionality I noticed some inconsistent behaviour of the Ahc backend. Therefore some of the tests are marked as pending for the Ahc backend.
This PR builds on top of #227 (which builds on top of #208 in itself) and therefore should be rebased and retargeted to master after #227 is merged.
Fixes #215