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

Add automatic redirection handling #195

Open
ktoso opened this issue Sep 8, 2016 · 32 comments
Open

Add automatic redirection handling #195

ktoso opened this issue Sep 8, 2016 · 32 comments
Labels
1 - triaged Tickets that are safe to pick up for contributing in terms of likeliness of being accepted play-akka-http-integration t:client Issues related to the HTTP Client

Comments

@ktoso
Copy link
Contributor

ktoso commented Sep 8, 2016

Issue by jrudolph
Monday Sep 29, 2014 at 08:00 GMT
Originally opened as akka/akka#15990


The high-level client-side API should support some form of configurable redirection handling.

Several things need to be taken into account:

  • dealing with headers to be used for the redirected request. This has security implications.
  • user-agent spray-user
  • cookies
  • authorization
  • redirection not allowed for non-idempotent request (already implemented in spray) for some redirection status codes
  • which HostConnector to use for cross-domain redirections

Before starting on this, it would make sense to go through the mailing list / spray tickets to find further cases we might need to support.

A first solution could only support redirections to the same domain, where keeping certain headers may be safer (but: cookies with path constraints set).

Maybe also consider how browsers deal with redirections.

This is part of the bigger initiative to support a high-level HTTP client interface as tracked as #16856.

/cc @sirthias

@ktoso
Copy link
Contributor Author

ktoso commented Sep 8, 2016

Comment by huntc
Thursday Mar 05, 2015 at 02:37 GMT


I've just stumbled across the need for this and will have to of course hand craft it. I just thought that you'd like to know of at least one person who needs this functionality.

@ktoso ktoso added this to the 2.4.x milestone Sep 8, 2016
@ktoso
Copy link
Contributor Author

ktoso commented Sep 8, 2016

Comment by francisdb
Sunday Jul 19, 2015 at 08:23 GMT


Another case to handle would be redirect loops

@ktoso
Copy link
Contributor Author

ktoso commented Sep 8, 2016

Comment by CptnKirk
Wednesday Oct 14, 2015 at 23:26 GMT


Also +1 for this feature. Did tracking of this issue get lost? Was 1.0-RC4, now I see it has no milestone. Yet I don't see that anyone has modified the milestone since sirthias on Jun 24.

@ktoso
Copy link
Contributor Author

ktoso commented Sep 8, 2016

Comment by coreyauger
Wednesday Nov 18, 2015 at 23:26 GMT


Also +1 from me.

@ktoso
Copy link
Contributor Author

ktoso commented Sep 8, 2016

Comment by ktoso
Wednesday Feb 24, 2016 at 14:18 GMT


Logging a +1 ☝️ February 24, 2016 3:11 PM

We'd welcome contributions of this feature, anyone interested in helping out? :-)

@ktoso
Copy link
Contributor Author

ktoso commented Sep 8, 2016

Comment by RomanIakovlev
Monday Feb 29, 2016 at 13:43 GMT


I'd like to try to help with this, since I need it in my project. However, I'm totally new to akka source code, so I'd definitely need some initial guidance. What would be the best channel to ask questions related to this?

@ktoso
Copy link
Contributor Author

ktoso commented Sep 8, 2016

Comment by ktoso
Monday Feb 29, 2016 at 13:57 GMT


Thank a lot for offering help on that :-)
Best discussed here actually as it's nice and async, or gitter (however this week the team is mostly offline, planning the next months of development, so we may be a bit slow to respond - sorry about that).

@ktoso
Copy link
Contributor Author

ktoso commented Sep 8, 2016

Comment by RomanIakovlev
Monday Feb 29, 2016 at 18:10 GMT


Thanks, I'd be happy to help. Just not sure if I'm up to this task, but one never knows unless one tries. :)

Ok, here are the initial questions.

  1. API overview. I imagine adding a couple of properties to the akka.http.scaladsl.model.HttpRequest, like followRedirects: Boolean = false and maxRedirects: Int = 1. Or maybe wrap these two in a single case class. What do you think?
  2. Where the actual redirect handling code should live? So far my best guess is akka.http.impl.engine.client.OutgoingConnectionBlueprint, but I'm in doubts.

This is just to get my thoughts straight before I can begin even thinking of possible solutions.

@ktoso
Copy link
Contributor Author

ktoso commented Sep 8, 2016

Comment by jamesmulcahy
Tuesday Mar 01, 2016 at 00:45 GMT


@RomanIakovlev You probably also need to consider adding an option to decide which headers are re-submitted when the re-direct is followed. Spray used to drop the headers, which is apparently an appealing approach to some, but doesn't suit all scenarios. See spray/spray#1045.

@ktoso
Copy link
Contributor Author

ktoso commented Sep 8, 2016

Comment by RomanIakovlev
Thursday Mar 17, 2016 at 20:43 GMT


I've started working on this issue, and I'm sort of stuck on some questions I can't resolve myself.

My overall idea is to create a GraphStage[BidiShape[HttpRequest, HttpRequest, HttpResponse, HttpResponse]], which will sit on top of the OutgoingConnectionBlueprint. This new stage will keep track of incoming request (In1) and forward them downstream (Out1). When response comes (In2), the stage checks if the response is redirect, and, if it is, sends the new redirected request (Out1). When the "real" response will come, it's forwarded to requester (Out2). I don't want to send the pull request so far, but, if you wish, you can have a look at the current implementation in branch wip-15990-http-client-redirect-RomanIakovlev in my forked Akka repo at https://github.com/RomanIakovlev/akka/

This implementation fails at one of the tests in akka.http.impl.engine.client.HighLevelOutgoingConnectionSpec, concretely on the "catch response stream truncation" one. It's not completely clear to me what's the expected behavior for that test, so maybe someone can clarify?

Besides that, I have 2 questions about this approach in general.

First of all, do you think it makes sense? I mean, the GraphStage[BidiShape] part of it. Maybe there's a better way?

Secondly, if the BidiShape is the way to go, is it supposed to support multiple requests in flight? If yes, then how received response can be mapped to an incoming request? I need this correspondence to be able to copy the headers to the redirected request from the original request (and maybe for something else that I don't know yet). If only single request in flight can be supported, then I'm not sure how the pipelining will be dealt with.

I hope my questions make sense. I'm on vacations now and will be able to dedicate more time to this issue for the next week, especially if I'll have some guidance from the Akka core team. :)

@ktoso
Copy link
Contributor Author

ktoso commented Sep 8, 2016

Comment by RomanIakovlev
Friday Mar 18, 2016 at 12:00 GMT


Another thought has just occurred to me. The existing OutgoungConnectionBlueprint does support pipelining, obviously, but, as far as I can tell, it also guarantees the order of responses to be the same as the order of requests. Is it correct? If yes, then I can rely on this fact to get correspondence between requests and responses in my redirect supporting graph stage to support copying certain headers into redirect requests. Would that work?

@ktoso
Copy link
Contributor Author

ktoso commented Sep 8, 2016

Comment by ktoso
Friday Jul 08, 2016 at 12:45 GMT


This had some progress in akka/akka#20135 but we decided together that it wasn't quite there yet. Would be awesome to see it be picked up again (@RomanIakovlev again perhaps if he has time?)

@ktoso
Copy link
Contributor Author

ktoso commented Sep 8, 2016

Comment by gaydenko
Thursday Aug 18, 2016 at 09:07 GMT


Have you some workaround, please? It is a hard stopper.

@ktoso
Copy link
Contributor Author

ktoso commented Sep 8, 2016

Comment by ktoso
Thursday Aug 18, 2016 at 09:09 GMT


Here's the options:
a) help us deliver this feature - contribute!
b) follow the redirect manually.
c) use a different http client. Play's WS is pretty good.

@ktoso
Copy link
Contributor Author

ktoso commented Sep 8, 2016

Comment by gaydenko
Thursday Aug 18, 2016 at 10:45 GMT


Thanks. I know the a) is the best for akka, but resources are limited at the moment resulting in preferring the b).

@ktoso
Copy link
Contributor Author

ktoso commented Sep 8, 2016

Comment by drewhk
Friday Aug 19, 2016 at 11:37 GMT


Take a look at this for implementing retries: akka/akka-stream-contrib#26

@ktoso
Copy link
Contributor Author

ktoso commented Sep 8, 2016

Comment by drewhk
Friday Aug 19, 2016 at 11:37 GMT


currently in contrib, but seems like useful for many things so we might want to migrate it to akka-streams and consider adding it to the HTTP client API somehow.

@ktoso ktoso added the 1 - triaged Tickets that are safe to pick up for contributing in terms of likeliness of being accepted label Sep 8, 2016
@ktoso ktoso removed this from the 2.4.x milestone Sep 12, 2016
@jrudolph
Copy link
Contributor

Here's a stub for redirection support and how things could be wired:

package akka.http

import akka.actor.ActorSystem
import akka.http.scaladsl.Http
import akka.http.scaladsl.model.headers.Location
import akka.http.scaladsl.model.{ HttpMethods, HttpRequest, HttpResponse, StatusCodes }
import akka.stream.ActorMaterializer

import scala.concurrent.{ ExecutionContext, Future }

object RichHttpClient {
  type HttpClient = HttpRequest  Future[HttpResponse]

  def redirectOrResult(singleRequest: HttpClient)(response: HttpResponse): Future[HttpResponse] =
    response.status match {
      case StatusCodes.Found | StatusCodes.MovedPermanently | StatusCodes.SeeOther 
        val newUri = response.header[Location].get.uri

        // TODO: add debug logging

        // change to GET method as allowed by https://tools.ietf.org/html/rfc7231#section-6.4.3
        // TODO: keep HEAD if the original request was a HEAD request as well?
        // TODO: do we want to keep something of the original request like custom user-agents, cookies
        //       or authentication headers?
        singleRequest(HttpRequest(method = HttpMethods.GET, uri = newUri))
      // TODO: what to do on an error? Also report the original request/response?

      // TODO: also handle 307, which would require resending POST requests
      case _  Future.successful(response)
    }

  def httpClientWithRedirect(client: HttpClient)(implicit ec: ExecutionContext): HttpClient = {
    lazy val redirectingClient: HttpClient =
      req  client(req).flatMap(redirectOrResult(redirectingClient)) // recurse to support multiple redirects

    redirectingClient
  }
}

object SingleRequestWithRedirect extends App {
  implicit val system = ActorSystem()
  import system.dispatcher
  implicit val mat = ActorMaterializer()

  val simpleClient = Http().singleRequest(_: HttpRequest)
  val redirectingClient = RichHttpClient.httpClientWithRedirect(simpleClient)

  val request = HttpRequest(uri = "http://goggle.de")

  simpleClient(request).onComplete(res  println(s"Without redirection: $res"))
  redirectingClient(request).onComplete(res  println(s"With redirection: $res"))
}

I added lots of TODOs where a general solution would have to actually support solutions.

@akashppatel
Copy link

Thank you @jrudolph , for sharing general solution it helped me :)

@muller
Copy link

muller commented Dec 14, 2016

To help answering the TODO: do we want to keep... I like the approach in http://docs.python-requests.org/en/master/user/quickstart/#redirection-and-history where the HttpClient is HttpRequest ⇒ Future[List[HttpResponse]] instead of a single HttpResponse

@ktoso
Copy link
Contributor Author

ktoso commented Dec 14, 2016

The low level in Akka is like that actually. Since Flow[Request, Response] means there can be multiple responses, we simply don't need the List as it's already represented by how Flows work.
I know your commend likely relates to the singleRequest API though.

@gaydenko
Copy link

BTW, I guess recursion must be limited. I use this workaround:

  private val maxRedirCount = 20
  def httpRequire(req: HttpRequest, count: Int = 0)(implicit system: ActorSystem, mat: Materializer): Future[HttpResponse] = {
    implicit val ec: ExecutionContext = system.dispatcher

    Http().singleRequest(req).flatMap { resp =>
      resp.status match {
        case StatusCodes.Found => resp.header[headers.Location].map { loc =>
          val locUri = loc.uri
          val newUri = req.uri.copy(scheme = locUri.scheme, authority = locUri.authority)
          val newReq = req.copy(uri = newUri)
          if (count < maxRedirCount) httpRequire(newReq, count + 1) else Http().singleRequest(newReq)
        }.getOrElse(throw new RuntimeException(s"location not found on 302 for ${req.uri}"))
        case _ => Future(resp)
      }
    }
  }

@jonas jonas added the t:client Issues related to the HTTP Client label Jan 28, 2017
@usamec
Copy link

usamec commented May 24, 2017

val newUri = req.uri.copy(scheme = locUri.scheme, authority = locUri.authority)

what is the reason for only replacing scheme and authority part of the URI?

@gaydenko
Copy link

@usamec It does work at my use cases.

@usamec
Copy link

usamec commented May 24, 2017

@gaydenko And yet it does not work on example from wikipedia: https://en.wikipedia.org/wiki/HTTP_301
(and does not handle 301 status code).

I would copy the whole URI and handle all the 30x status codes.

@gaydenko
Copy link

@usamec You can modify that workaround the way you want. In common case there isn't single rule all redirectors follow to, and I was just focused on the problem in hands. Again, at my case those few line of code do the job. You can use val newReq = req.copy(uri = locUri) if it does work for you.

@manonthegithub
Copy link
Contributor

manonthegithub commented Aug 12, 2017

@jrudolph

val newUri = response.header[Location].get.uri

please note that this won't work for the case when uri in location is relative and you don't use host connection pool (just encountered this case).

Also, as I understand docs, you forgot to dicard redirect response's bytes so I assume, it should look like:

response.discardEntityBytes().future().flatMap(_ =>
   singleRequest(HttpRequest(method = HttpMethods.GET, uri = newUri))
)

@vvasuki
Copy link

vvasuki commented Oct 19, 2017

A slightly modified version of @jrudolph 's code (only new thing added: discardEntityBytes):

object RichHttpClient {
  type HttpClient = HttpRequest ⇒ Future[HttpResponse]

  def redirectOrResult(client: HttpClient)(response: HttpResponse)(implicit materializer: Materializer): Future[HttpResponse] =
    response.status match {
      case StatusCodes.Found | StatusCodes.MovedPermanently | StatusCodes.SeeOther ⇒
        val newUri = response.header[Location].get.uri
        // Always make sure you consume the response entity streams (of type Source[ByteString,Unit]) by for example connecting it to a Sink (for example response.discardEntityBytes() if you don’t care about the response entity), since otherwise Akka HTTP (and the underlying Streams infrastructure) will understand the lack of entity consumption as a back-pressure signal and stop reading from the underlying TCP connection!
        response.discardEntityBytes()
        // TODO: add debug logging

        // change to GET method as allowed by https://tools.ietf.org/html/rfc7231#section-6.4.3
        // TODO: keep HEAD if the original request was a HEAD request as well?
        // TODO: do we want to keep something of the original request like custom user-agents, cookies
        //       or authentication headers?
        client(HttpRequest(method = HttpMethods.GET, uri = newUri))
      // TODO: what to do on an error? Also report the original request/response?

      // TODO: also handle 307, which would require resending POST requests
      case _ ⇒ Future.successful(response)
    }

  def httpClientWithRedirect(client: HttpClient)(implicit ec: ExecutionContext, materializer: Materializer): HttpClient = {
    lazy val redirectingClient: HttpClient =
      req ⇒ client(req).flatMap(redirectOrResult(redirectingClient)) // recurse to support multiple redirects

    redirectingClient
  }
}
  final implicit val materializer: ActorMaterializer = ActorMaterializer(ActorMaterializerSettings(context.system))

  private val simpleClient: HttpRequest => Future[HttpResponse] = Http(context.system).singleRequest(_: HttpRequest)
  private val redirectingClient: HttpClient = RichHttpClient.httpClientWithRedirect(simpleClient)
val responseFuture = redirectingClient(HttpRequest(uri = uri))

Available as:
"com.github.sanskrit-coders" % "scala-utils_2.12" % "0.1"

@qstyler
Copy link

qstyler commented Aug 11, 2020

Hello everyone from 2020.
Was that problem solved?

@nmolenaar
Copy link

Hello everyone from 2021.
Was that problem solved?

@spaniakos
Copy link

spaniakos commented Mar 17, 2021

if anyone has a problem with the redirects in laravel, do check your .htaccess and see if you have any 301 redirect like:
RewriteCond %{REQUEST_FILENAME} !-d
RewriteRule ^(.*)/$ /$1 [L,R=301]

As redirect in akka is not following though, this will terminate the request and wan't process the redirect.

@randers-bit
Copy link

Hello everyone from 2023.
Was that problem solved?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 - triaged Tickets that are safe to pick up for contributing in terms of likeliness of being accepted play-akka-http-integration t:client Issues related to the HTTP Client
Projects
None yet
Development

No branches or pull requests