hmrc / http-verbs Goto Github PK

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.

As a user of http-verbs, I have my project open in IntelliJ but I cannot browse the source code without wasting time finding out which repo it is in, then only having web-page views of the source code (i.e. not clickable content within IntelliJ).

Can you provide the source jars automatically?

Currently the timeout setting for services in application.conf is ignored. Instead, http-verbs (via ws) uses the Play's default setting (20 seconds).

https://github.com/hmrc/http-verbs/blob/master/src/main/resources/reference.conf#L15

We should enable to pass an (implicit) timeout parameter to all the http verbs, so that users can define different timeout values per REST call.

On one of builds https://travis-ci.org/hmrc/http-verbs/jobs/497152958 there is a lot of warnings about deprecation

[warn] /home/travis/build/hmrc/http-verbs/src/main/play-26/uk/gov/hmrc/play/http/ws/WSPost.scala:39: object Execution in package concurrent is deprecated: Please see https://www.playframework.com/documentation/2.6.x/Migration26#play.api.libs.concurrent.Execution-is-deprecated

This could lead lot of problems in migration 2.7

WSRequest.scala:44 (and possible other code) relies on deprecations in play.

As a result it's not possible to run parts of the code without running Play Appliacion. Isolation and Encapsulation is broken.

Example. Below code throws Exception in thread "main" java.lang.RuntimeException: There is no started application

object Demo extends App {
  val ws = new uk.gov.hmrc.play.http.ws.WSHttp {
    override val hooks: Seq[HttpHook] = Nil
  }
  implicit val hc = HeaderCarrier()

  ws.GET[HttpResponse]("https://google.com") //BOOM
}

Exception in thread "main" java.lang.RuntimeException: There is no started application
	at scala.sys.package$.error(package.scala:27)
	at play.api.Play$$anonfun$current$1.apply(Play.scala:86)
	at play.api.Play$$anonfun$current$1.apply(Play.scala:86)
	at scala.Option.getOrElse(Option.scala:121)
	at play.api.Play$.current(Play.scala:86)
	at uk.gov.hmrc.play.http.ws.WSRequest$class.buildRequest(WSRequest.scala:44)
...

When performing http requests, it is difficult to obtain the response body and headers if the status is not 2xx. The exceptions thrown if the response is not 2xx only contain a message String. The only way to obtain the response body is to possibly perform some regex on the message to extract it out. What would be nicer if a user was able to write something like this:

http.POST(url, body).map{ response: HttpResponse =>
  response.status match {
    case 200 => /** do something */
    case 401 => /** do something else */
    ... 
  }
}

This is impossible however as exception are being thrown before we get the map in the example above. Doing this:

http.POST(url, body).recover{
  case e: BadRequestException => /** do something */
}

is problematic as the exception only contains a message String. There is no easy way of using the body of the response during error handling.

Could the code be refactored so it doesn't throw exceptions if it's returning a HttpResponse or can the exceptions be made to include more easily accessible information?

uk.gov.hmrc.http.HttpPost methods have a contract to return a Future response. However they can actually throw IllegalArgumentExceptions (rather that returning a failed Future as per their contract) if you pass an invalid 'url' String parameter to it.

Obviously you should not be passing invalid URLs to one of these methods but sometimes this is not within your control (when you get URLs from another service for instance).

It is very hidden that these methods can throw exceptions (which is itself very un-scala like) so it will catch out developers who are rightly expecting a Future to be returned.

One solution to this issue would be for these methods to accept a java.net.URL as the type of the 'url' parameter instead of String (thus enforcing the good url requirement via the type system). Obviously this would then change the signature of all the methods (or result in another set of methods with different signatures).

The alternate solution would be for the hmrc implementations of HttpPost to catch any such exceptions (caused by invalid url Strings) and return the exception in a failed Future (as per contract).

It would be nice to have an unapply method on the HttpResponse type so that we can do pattern matching on response objects