Merge pull request #126 from hmrc/BDOG-1338

BDOG-1338
hmrc · Mar 31, 2021 · 12f5c5b · 12f5c5b
2 parents f14b09f + 22d5670
commit 12f5c5b
Show file tree

Hide file tree

Showing 28 changed files with 271 additions and 247 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -8,6 +8,8 @@
 | HeaderNames package change                                   | Minor      | [Scalafix available](https://github.com/hmrc/scalafix-rules/blob/master/http-verbs-13/rules/src/main/scala/fix/HttpVerbs13RenamePackages.scala) |
 | URLs can now be supplied as `java.net.URL`                   | Minor      | Optional change |
 | Removed deprecated values from `SessionKeys`                 | Minor      | Use auth client retrievals |
+| All sent request headers explicitly passed to HttpHook       | Minor      | Distinction between sent headers and HeaderCarrier is useful|
+| Transport layer explicitly passed all headers (e.g doPost)   | Minor      | All headers can be obtained from HeaderCarrier.headersForUrl |
 
 To run a scalafix rule on your project, please refer to [the usage docs](https://github.com/hmrc/scalafix-rules#usage).
 

diff --git a/build.sbt b/build.sbt
@@ -53,7 +53,7 @@ def sharedSources = Seq(
   Compile / unmanagedSourceDirectories   += baseDirectory.value / "../http-verbs-common/src/main/scala",
   Compile / unmanagedResourceDirectories += baseDirectory.value / "../http-verbs-common/src/main/resources",
   Test    / unmanagedSourceDirectories   += baseDirectory.value / "../http-verbs-common/src/test/scala",
-  Test    / unmanagedResourceDirectories += baseDirectory.value / "../http-verbs-common/src/test/resources",
+  Test    / unmanagedResourceDirectories += baseDirectory.value / "../http-verbs-common/src/test/resources"
 )
 
 def copySources(module: Project) = Seq(

diff --git a/http-verbs-common/src/main/scala/uk/gov/hmrc/http/HeaderCarrier.scala b/http-verbs-common/src/main/scala/uk/gov/hmrc/http/HeaderCarrier.scala
@@ -91,6 +91,16 @@ case class HeaderCarrier(
 }
 
 object HeaderCarrier {
+
+  def headersForUrl(
+    config: Config,
+    url: String,
+    extraHeaders: Seq[(String, String)] = Seq()
+  )(
+    implicit hc: HeaderCarrier
+  ): Seq[(String, String)] =
+    hc.withExtraHeaders(extraHeaders: _*).headersForUrl(config)(url)
+
   case class Config(
     internalHostPatterns    : Seq[Regex]     = Seq.empty,
     headersAllowlist        : Seq[String]    = Seq.empty,

diff --git a/http-verbs-common/src/main/scala/uk/gov/hmrc/http/HttpDelete.scala b/http-verbs-common/src/main/scala/uk/gov/hmrc/http/HttpDelete.scala
@@ -30,10 +30,13 @@ trait HttpDelete
     with HttpHooks
     with Retries {
 
+  private lazy val hcConfig = HeaderCarrier.Config.fromConfig(configuration)
+
   override def DELETE[O](url: String, headers: Seq[(String, String)] = Seq.empty)(implicit rds: HttpReads[O], hc: HeaderCarrier, ec: ExecutionContext): Future[O] =
     withTracing(DELETE_VERB, url) {
-      val httpResponse = retry(DELETE_VERB, url)(doDelete(url, headers))
-      executeHooks(url, DELETE_VERB, None, httpResponse)
+      val allHeaders = HeaderCarrier.headersForUrl(hcConfig, url, headers)
+      val httpResponse = retry(DELETE_VERB, url)(doDelete(url, allHeaders))
+      executeHooks(DELETE_VERB, url"$url", allHeaders, None, httpResponse)
       mapErrors(DELETE_VERB, url, httpResponse).map(rds.read(DELETE_VERB, url, _))
     }
 }
diff --git a/http-verbs-common/src/main/scala/uk/gov/hmrc/http/HttpGet.scala b/http-verbs-common/src/main/scala/uk/gov/hmrc/http/HttpGet.scala
@@ -26,6 +26,8 @@ import scala.concurrent.{ExecutionContext, Future}
 
 trait HttpGet extends CoreGet with GetHttpTransport with HttpVerb with ConnectionTracing with HttpHooks with Retries {
 
+  private lazy val hcConfig = HeaderCarrier.Config.fromConfig(configuration)
+
   override def GET[A](
     url: String,
     queryParams: Seq[(String, String)],
@@ -43,8 +45,9 @@ trait HttpGet extends CoreGet with GetHttpTransport with HttpVerb with Connectio
     val urlWithQuery = url + makeQueryString(queryParams)
 
     withTracing(GET_VERB, urlWithQuery) {
-      val httpResponse = retry(GET_VERB, urlWithQuery)(doGet(urlWithQuery, headers = headers))
-      executeHooks(url, GET_VERB, None, httpResponse)
+      val allHeaders = HeaderCarrier.headersForUrl(hcConfig, url, headers)
+      val httpResponse = retry(GET_VERB, urlWithQuery)(doGet(urlWithQuery, headers = allHeaders))
+      executeHooks(GET_VERB, url"$url", allHeaders, None, httpResponse)
       mapErrors(GET_VERB, urlWithQuery, httpResponse).map(response => rds.read(GET_VERB, urlWithQuery, response))
     }
   }

diff --git a/http-verbs-common/src/main/scala/uk/gov/hmrc/http/HttpPatch.scala b/http-verbs-common/src/main/scala/uk/gov/hmrc/http/HttpPatch.scala
@@ -31,6 +31,8 @@ trait HttpPatch
     with HttpHooks
     with Retries {
 
+  private lazy val hcConfig = HeaderCarrier.Config.fromConfig(configuration)
+
   override def PATCH[I, O](
     url: String,
     body: I,
@@ -40,8 +42,9 @@ trait HttpPatch
       hc: HeaderCarrier,
       ec: ExecutionContext): Future[O] =
     withTracing(PATCH_VERB, url) {
-      val httpResponse = retry(PATCH_VERB, url)(doPatch(url, body, headers))
-      executeHooks(url, PATCH_VERB, Option(HookData.FromString(Json.stringify(wts.writes(body)))), httpResponse)
+      val allHeaders = HeaderCarrier.headersForUrl(hcConfig, url, headers)
+      val httpResponse = retry(PATCH_VERB, url)(doPatch(url, body, allHeaders))
+      executeHooks(PATCH_VERB, url"$url", allHeaders, Option(HookData.FromString(Json.stringify(wts.writes(body)))), httpResponse)
       mapErrors(PATCH_VERB, url, httpResponse).map(response => rds.read(PATCH_VERB, url, response))
     }
 }
diff --git a/http-verbs-common/src/main/scala/uk/gov/hmrc/http/HttpPost.scala b/http-verbs-common/src/main/scala/uk/gov/hmrc/http/HttpPost.scala
@@ -31,6 +31,8 @@ trait HttpPost
     with HttpHooks
     with Retries {
 
+  private lazy val hcConfig = HeaderCarrier.Config.fromConfig(configuration)
+
   override def POST[I, O](
     url: String,
     body: I,
@@ -40,8 +42,9 @@ trait HttpPost
       hc: HeaderCarrier,
       ec: ExecutionContext): Future[O] =
     withTracing(POST_VERB, url) {
-      val httpResponse = retry(POST_VERB, url)(doPost(url, body, headers))
-      executeHooks(url, POST_VERB, Option(HookData.FromString(Json.stringify(wts.writes(body)))), httpResponse)
+      val allHeaders = HeaderCarrier.headersForUrl(hcConfig, url, headers)
+      val httpResponse = retry(POST_VERB, url)(doPost(url, body, allHeaders))
+      executeHooks(POST_VERB, url"$url", allHeaders, Option(HookData.FromString(Json.stringify(wts.writes(body)))), httpResponse)
       mapErrors(POST_VERB, url, httpResponse).map(rds.read(POST_VERB, url, _))
     }
 
@@ -53,8 +56,9 @@ trait HttpPost
       hc: HeaderCarrier,
       ec: ExecutionContext): Future[O] =
     withTracing(POST_VERB, url) {
-      val httpResponse = retry(POST_VERB, url)(doPostString(url, body, headers))
-      executeHooks(url, POST_VERB, Option(HookData.FromString(body)), httpResponse)
+      val allHeaders = HeaderCarrier.headersForUrl(hcConfig, url, headers)
+      val httpResponse = retry(POST_VERB, url)(doPostString(url, body, allHeaders))
+      executeHooks(POST_VERB, url"$url", allHeaders, Option(HookData.FromString(body)), httpResponse)
       mapErrors(POST_VERB, url, httpResponse).map(rds.read(POST_VERB, url, _))
     }
 
@@ -66,8 +70,9 @@ trait HttpPost
       hc: HeaderCarrier,
       ec: ExecutionContext): Future[O] =
     withTracing(POST_VERB, url) {
-      val httpResponse = retry(POST_VERB, url)(doFormPost(url, body, headers))
-      executeHooks(url, POST_VERB, Option(HookData.FromMap(body)), httpResponse)
+      val allHeaders = HeaderCarrier.headersForUrl(hcConfig, url, headers)
+      val httpResponse = retry(POST_VERB, url)(doFormPost(url, body, allHeaders))
+      executeHooks(POST_VERB, url"$url", allHeaders, Option(HookData.FromMap(body)), httpResponse)
       mapErrors(POST_VERB, url, httpResponse).map(rds.read(POST_VERB, url, _))
     }
 
@@ -78,8 +83,9 @@ trait HttpPost
       hc: HeaderCarrier,
       ec: ExecutionContext): Future[O] =
     withTracing(POST_VERB, url) {
-      val httpResponse = retry(POST_VERB, url)(doEmptyPost(url, headers))
-      executeHooks(url, POST_VERB, None, httpResponse)
+      val allHeaders = HeaderCarrier.headersForUrl(hcConfig, url, headers)
+      val httpResponse = retry(POST_VERB, url)(doEmptyPost(url, allHeaders))
+      executeHooks(POST_VERB, url"$url", allHeaders, None, httpResponse)
       mapErrors(POST_VERB, url, httpResponse).map(rds.read(POST_VERB, url, _))
     }
 }
diff --git a/http-verbs-common/src/main/scala/uk/gov/hmrc/http/HttpPut.scala b/http-verbs-common/src/main/scala/uk/gov/hmrc/http/HttpPut.scala
@@ -25,6 +25,8 @@ import scala.concurrent.{ExecutionContext, Future}
 
 trait HttpPut extends CorePut with PutHttpTransport with HttpVerb with ConnectionTracing with HttpHooks with Retries {
 
+  private lazy val hcConfig = HeaderCarrier.Config.fromConfig(configuration)
+
   override def PUT[I, O](
     url: String,
     body: I,
@@ -34,8 +36,9 @@ trait HttpPut extends CorePut with PutHttpTransport with HttpVerb with Connectio
       hc: HeaderCarrier,
       ec: ExecutionContext): Future[O] =
     withTracing(PUT_VERB, url) {
-      val httpResponse = retry(PUT_VERB, url)(doPut(url, body, headers))
-      executeHooks(url, PUT_VERB, Option(HookData.FromString(Json.stringify(wts.writes(body)))), httpResponse)
+      val allHeaders = HeaderCarrier.headersForUrl(hcConfig, url, headers)
+      val httpResponse = retry(PUT_VERB, url)(doPut(url, body, allHeaders))
+      executeHooks(PUT_VERB, url"$url", allHeaders, Option(HookData.FromString(Json.stringify(wts.writes(body)))), httpResponse)
       mapErrors(PUT_VERB, url, httpResponse).map(response => rds.read(PUT_VERB, url, response))
     }
 
@@ -47,8 +50,9 @@ trait HttpPut extends CorePut with PutHttpTransport with HttpVerb with Connectio
       hc: HeaderCarrier,
       ec: ExecutionContext): Future[O] =
     withTracing(PUT_VERB, url) {
-      val httpResponse = retry(PUT_VERB, url)(doPutString(url, body, headers))
-      executeHooks(url, PUT_VERB, Option(HookData.FromString(body)), httpResponse)
+      val allHeaders = HeaderCarrier.headersForUrl(hcConfig, url, headers)
+      val httpResponse = retry(PUT_VERB, url)(doPutString(url, body, allHeaders))
+      executeHooks(PUT_VERB, url"$url", allHeaders, Option(HookData.FromString(body)), httpResponse)
       mapErrors(PUT_VERB, url, httpResponse).map(rds.read(PUT_VERB, url, _))
     }
 }
diff --git a/http-verbs-common/src/main/scala/uk/gov/hmrc/http/HttpTransport.scala b/http-verbs-common/src/main/scala/uk/gov/hmrc/http/HttpTransport.scala
@@ -26,16 +26,14 @@ trait GetHttpTransport {
   def doGet(
     url: String,
     headers: Seq[(String, String)] = Seq.empty)(
-      implicit hc: HeaderCarrier,
-      ec: ExecutionContext): Future[HttpResponse]
+      implicit ec: ExecutionContext): Future[HttpResponse]
 }
 
 trait DeleteHttpTransport {
   def doDelete(
     url: String,
     headers: Seq[(String, String)] = Seq.empty)(
-      implicit hc: HeaderCarrier,
-      ec: ExecutionContext): Future[HttpResponse]
+      implicit ec: ExecutionContext): Future[HttpResponse]
 }
 
 trait PatchHttpTransport {
@@ -44,7 +42,6 @@ trait PatchHttpTransport {
     body: A,
     headers: Seq[(String, String)] = Seq.empty)(
       implicit rds: Writes[A],
-      hc: HeaderCarrier,
       ec: ExecutionContext): Future[HttpResponse]
 }
 
@@ -54,15 +51,13 @@ trait PutHttpTransport {
     body: A,
     headers: Seq[(String, String)] = Seq.empty)(
       implicit rds: Writes[A],
-      hc: HeaderCarrier,
       ec: ExecutionContext): Future[HttpResponse]
 
   def doPutString(
     url: String,
     body: String,
     headers: Seq[(String, String)] = Seq.empty)(
-      implicit hc: HeaderCarrier,
-      ec: ExecutionContext): Future[HttpResponse]
+      implicit ec: ExecutionContext): Future[HttpResponse]
 }
 
 trait PostHttpTransport {
@@ -71,28 +66,24 @@ trait PostHttpTransport {
     body: A,
     headers: Seq[(String, String)] = Seq.empty)(
       implicit wts: Writes[A],
-      hc: HeaderCarrier,
       ec: ExecutionContext): Future[HttpResponse]
 
   def doPostString(
     url: String,
     body: String,
     headers: Seq[(String, String)] = Seq.empty)(
-      implicit hc: HeaderCarrier,
-      ec: ExecutionContext): Future[HttpResponse]
+      implicit ec: ExecutionContext): Future[HttpResponse]
 
   def doEmptyPost[A](
     url: String,
     headers: Seq[(String, String)] = Seq.empty)(
-      implicit hc: HeaderCarrier,
-      ec: ExecutionContext): Future[HttpResponse]
+    implicit ec: ExecutionContext): Future[HttpResponse]
 
   def doFormPost(
     url: String,
     body: Map[String, Seq[String]],
     headers: Seq[(String, String)] = Seq.empty)(
-      implicit hc: HeaderCarrier,
-      ec: ExecutionContext): Future[HttpResponse]
+    implicit ec: ExecutionContext): Future[HttpResponse]
 }
 
 trait HttpTransport

diff --git a/http-verbs-common/src/main/scala/uk/gov/hmrc/http/hooks/HttpHook.scala b/http-verbs-common/src/main/scala/uk/gov/hmrc/http/hooks/HttpHook.scala
@@ -16,14 +16,23 @@
 
 package uk.gov.hmrc.http.hooks
 
+import java.net.URL
+
 import uk.gov.hmrc.http.{HeaderCarrier, HttpResponse}
 
 import scala.concurrent.{ExecutionContext, Future}
 
 trait HttpHook {
-  def apply(url: String, verb: String, body: Option[HookData], responseF: Future[HttpResponse])(
+  def apply(
+    verb: String,
+    url: URL,
+    headers  : Seq[(String, String)],
+    body: Option[HookData],
+    responseF: Future[HttpResponse]
+  )(
     implicit hc: HeaderCarrier,
-    ec: ExecutionContext)
+    ec: ExecutionContext
+  )
 }
 
 sealed trait HookData

diff --git a/http-verbs-common/src/main/scala/uk/gov/hmrc/http/hooks/HttpHooks.scala b/http-verbs-common/src/main/scala/uk/gov/hmrc/http/hooks/HttpHooks.scala
@@ -16,6 +16,8 @@
 
 package uk.gov.hmrc.http.hooks
 
+import java.net.URL
+
 import uk.gov.hmrc.http.{HeaderCarrier, HttpResponse}
 
 import scala.concurrent.{ExecutionContext, Future}
@@ -26,25 +28,27 @@ trait HttpHooks {
   val NoneRequired = Seq(
     new HttpHook {
       def apply(
-        url      : String,
         verb     : String,
+        url      : URL,
+        headers  : Seq[(String, String)],
         body     : Option[HookData],
         responseF: Future[HttpResponse]
-      )(implicit
-       hc: HeaderCarrier,
-       ec: ExecutionContext
-     ) = {}
+      )(
+        implicit hc: HeaderCarrier,
+        ec: ExecutionContext
+     ): Unit = {}
     }
   )
 
   protected def executeHooks(
-    url      : String,
-    verb     : String,
-    body     : Option[HookData],
+    verb   : String,
+    url    : URL,
+    headers: Seq[(String, String)],
+    body   : Option[HookData],
     responseF: Future[HttpResponse]
-  )(implicit
-    hc: HeaderCarrier,
+  )(
+    implicit hc: HeaderCarrier,
     ec: ExecutionContext
   ): Unit =
-    hooks.foreach(_.apply(url, verb, body, responseF))
+    hooks.foreach(_.apply(verb, url, headers, body, responseF))
 }
diff --git a/http-verbs-common/src/main/scala/uk/gov/hmrc/http/logging/ConnectionTracing.scala b/http-verbs-common/src/main/scala/uk/gov/hmrc/http/logging/ConnectionTracing.scala
@@ -16,15 +16,15 @@
 
 package uk.gov.hmrc.http.logging
 
-import org.slf4j.LoggerFactory
-import uk.gov.hmrc.http.{HttpException, Upstream4xxResponse}
+import org.slf4j.{Logger, LoggerFactory}
+import uk.gov.hmrc.http.{HttpException, UpstreamErrorResponse}
 
 import scala.concurrent._
 import scala.util.{Failure, Success, Try}
 
 trait ConnectionTracing {
 
-  lazy val connectionLogger = LoggerFactory.getLogger("connector")
+  lazy val connectionLogger: Logger = LoggerFactory.getLogger("connector")
 
   def withTracing[T](method: String, uri: String)(
     body: => Future[T])(implicit ld: LoggingDetails, ec: ExecutionContext): Future[T] = {
@@ -34,18 +34,18 @@ trait ConnectionTracing {
     f
   }
 
-  def logResult[A](ld: LoggingDetails, method: String, uri: String, startAge: Long)(result: Try[A]) = result match {
-    case Success(ground) => connectionLogger.debug(formatMessage(ld, method, uri, startAge, "ok"))
+  def logResult[A](ld: LoggingDetails, method: String, uri: String, startAge: Long)(result: Try[A]): Unit = result match {
+    case Success(_) => connectionLogger.debug(formatMessage(ld, method, uri, startAge, "ok"))
     case Failure(ex: HttpException) if ex.responseCode == 404 =>
       connectionLogger.info(formatMessage(ld, method, uri, startAge, s"failed ${ex.getMessage}"))
-    case Failure(ex: Upstream4xxResponse) if ex.upstreamResponseCode == 404 =>
-      connectionLogger.info(formatMessage(ld, method, uri, startAge, s"failed ${ex.getMessage}"))
+    case Failure(ex: UpstreamErrorResponse) if ex.statusCode == 404 =>
+      connectionLogger.info(formatMessage(ld, method, uri, startAge, s"failed ${ex.message}"))
     case Failure(ex) => connectionLogger.warn(formatMessage(ld, method, uri, startAge, s"failed ${ex.getMessage}"))
   }
 
   import uk.gov.hmrc.http.logging.ConnectionTracing.formatNs
 
-  def formatMessage(ld: LoggingDetails, method: String, uri: String, startAge: Long, message: String) = {
+  def formatMessage(ld: LoggingDetails, method: String, uri: String, startAge: Long, message: String): String = {
     val requestId    = ld.requestId.getOrElse("")
     val requestChain = ld.requestChain
     val durationNs   = ld.age - startAge