HTTP status logic is inadequate

[rickb777]

There is a major bug: the status code can get ignored, doesn't handle 3xx and is not available to client code.

• Redirects are not handled.
• Conditional requests are impossible.
• Depending on usage, client or server errors can get ignored.

Typically, all you get back is a Jackson exception because the error page cannot be parsed; this is quite unsatisfactory.

For example in https://github.com/hmrc/http-verbs/blob/master/src/main/scala/uk/gov/hmrc/play/http/HttpGet.scala

def GET[A](url: String)(implicit rds: HttpReads[A], hc: HeaderCarrier): Future[A] =
  withTracing(GET_VERB, url) {
    val httpResponse = doGet(url)
    executeHooks(url, GET_VERB, None, httpResponse)
    mapErrors(GET_VERB, url, httpResponse).map(response => rds.read(GET_VERB, url, response))
  }

should probably be more like

def GET[A](url: String)(implicit rds: HttpReads[A], hc: HeaderCarrier):
        Future[Either[HttpResponse, A]] =
  withTracing(GET_VERB, url) {
    val httpResponse = doGet(url)
    executeHooks(url, GET_VERB, None, httpResponse)
    mapErrors(GET_VERB, url, httpResponse).<... rewrite this ...>
  }

The same applies to the other methods, POST, PUT etc.

[howyp]

Rick - I think you're right that the status can be rather hard to get at, though it isn't ignored.

The idea of http-verbs is that it translates non-2xx codes into typed exceptions, which can flow back up through your chain of futures, and either be handled by your application code or by an error handler in the default Global objects provided by microservice-bootstrap/frontend-bootstrap.

Whilst Future[Either[HttpResponse, A]] would model some situations better, the nested monads can get unpleasant to handle and we didn't want to force that on everyone. Having said that, catching exceptions to handle situations where you expect to get a non-2xx code isn't very nice either.

So my plan was to improve this a lot with #17, which builds on the introduction of the HttpReads typeclass, and allows clients of http-verbs to configure it to parse responses as any mixture of type, based on status code.

Sadly, although it was merged, it never got released because of other project pressures. If it looks helpful we could resurrect it?

[rickb777]

The problem seems to be one that arises from trying too hard to do too much. If we were to separate the HTTP traffic from the unmarshalling into two separate API steps, it would make it much easier for client code to achieve what it needs to achieve in different response scenarios. It would avoid complex monads too.

[rickb777]

Also, the two implicit parameters caused me trouble.

def GET[A](url: String)(implicit rds: HttpReads[A], hc: HeaderCarrier)

I discovered it's possible to make requests without these set up correctly. Explicit parameters would prevent this, e.g. like this

def GET[A](url: String, rds: HttpReads[A], hc: HeaderCarrier)

[SteveSmithTech]

Hi @rickb777 and @howyp, we would welcome a PR from the two of you on this, based on #17