From 21577967bd32b701b0347b8f1fa34faf49011357 Mon Sep 17 00:00:00 2001 From: colin-lamed <9568290+colin-lamed@users.noreply.github.com> Date: Thu, 22 Sep 2022 12:41:36 +0100 Subject: [PATCH 1/3] Fix MDC for HttpClientV2 --- .../hmrc/http/client/HttpClientV2Impl.scala | 9 +++-- .../scala/uk/gov/hmrc/http/RetriesSpec.scala | 4 +-- .../hmrc/http/client/HttpClientV2Spec.scala | 33 +++++++++++++++++++ 3 files changed, 42 insertions(+), 4 deletions(-) diff --git a/http-verbs-common/src/main/scala/uk/gov/hmrc/http/client/HttpClientV2Impl.scala b/http-verbs-common/src/main/scala/uk/gov/hmrc/http/client/HttpClientV2Impl.scala index d7fa2097..31e29c6c 100644 --- a/http-verbs-common/src/main/scala/uk/gov/hmrc/http/client/HttpClientV2Impl.scala +++ b/http-verbs-common/src/main/scala/uk/gov/hmrc/http/client/HttpClientV2Impl.scala @@ -25,6 +25,7 @@ import play.api.libs.ws.{BodyWritable, EmptyBody, InMemoryBody, SourceBody, WSCl import play.core.parsers.FormUrlEncodedParser import uk.gov.hmrc.http.{BadGatewayException, BuildInfo, GatewayTimeoutException, HeaderCarrier, HttpReads, HttpResponse, Retries} import uk.gov.hmrc.play.http.BodyCaptor +import uk.gov.hmrc.play.http.logging.Mdc import uk.gov.hmrc.play.http.ws.WSProxyConfiguration import uk.gov.hmrc.http.hooks.{Data, HookData, HttpHook, RequestData, ResponseData} import uk.gov.hmrc.http.logging.ConnectionTracing @@ -175,10 +176,14 @@ final class RequestBuilderImpl( // -- Execution -- override def execute[A](implicit r: HttpReads[A], ec: ExecutionContext): Future[A] = - executor.execute(request, hookDataF, isStream = false, r) + Mdc.preservingMdc( + executor.execute(request, hookDataF, isStream = false, r) + ) override def stream[A](implicit r: StreamHttpReads[A], ec: ExecutionContext): Future[A] = - executor.execute(request, hookDataF, isStream = true, r) + Mdc.preservingMdc( + executor.execute(request, hookDataF, isStream = true, r) + ) } class ExecutorImpl( diff --git a/http-verbs-common/src/test/scala/uk/gov/hmrc/http/RetriesSpec.scala b/http-verbs-common/src/test/scala/uk/gov/hmrc/http/RetriesSpec.scala index 22f4cdb2..24728c43 100644 --- a/http-verbs-common/src/test/scala/uk/gov/hmrc/http/RetriesSpec.scala +++ b/http-verbs-common/src/test/scala/uk/gov/hmrc/http/RetriesSpec.scala @@ -200,7 +200,7 @@ class RetriesSpec _ <- Future.successful(Mdc.putMdc(mdcData)) res <- retries.retryOnSslEngineClosed("GET", "url") { // assert mdc available to block execution - Option(MDC.getCopyOfContextMap).map(_.asScala.toMap).getOrElse(Map.empty) shouldBe mdcData + Mdc.mdcData shouldBe mdcData retries.failFewTimesAndThenSucceed( success = Future.successful(expectedResponse), @@ -209,7 +209,7 @@ class RetriesSpec } } yield { // assert mdc available to continuation - Option(MDC.getCopyOfContextMap).map(_.asScala.toMap).getOrElse(Map.empty) shouldBe mdcData + Mdc.mdcData shouldBe mdcData res } diff --git a/http-verbs-common/src/test/scala/uk/gov/hmrc/http/client/HttpClientV2Spec.scala b/http-verbs-common/src/test/scala/uk/gov/hmrc/http/client/HttpClientV2Spec.scala index 5adb208b..c64f5769 100644 --- a/http-verbs-common/src/test/scala/uk/gov/hmrc/http/client/HttpClientV2Spec.scala +++ b/http-verbs-common/src/test/scala/uk/gov/hmrc/http/client/HttpClientV2Spec.scala @@ -33,8 +33,10 @@ import play.api.libs.json.{Json, Reads, Writes} import play.api.libs.ws.ahc.{AhcWSClient, AhcWSClientConfigFactory} import uk.gov.hmrc.http.{HeaderCarrier, HttpReads, HttpReadsInstances, HttpResponse, Retries, StringContextOps, UpstreamErrorResponse} import uk.gov.hmrc.http.hooks.{Data, HookData, HttpHook, RequestData, ResponseData} +import uk.gov.hmrc.play.http.logging.Mdc import uk.gov.hmrc.http.test.WireMockSupport +import java.util.concurrent.Executors import java.util.concurrent.atomic.AtomicInteger import scala.concurrent.{ExecutionContext, Future} import scala.concurrent.ExecutionContext.Implicits.global @@ -588,6 +590,37 @@ class HttpClientV2Spec res.failed.futureValue shouldBe a[UpstreamErrorResponse] count.get shouldBe 4 } + + "preserve Mdc" in new Setup { + implicit val hc = HeaderCarrier() + + wireMockServer.stubFor( + WireMock.put(urlEqualTo("/")) + .willReturn(aResponse().withBody("\"res\"").withStatus(200)) + ) + + val mdcData = Map("key1" -> "value1") + + implicit val mdcEc = ExecutionContext.fromExecutor(new uk.gov.hmrc.play.http.logging.MDCPropagatingExecutorService(Executors.newFixedThreadPool(2))) + + val res: Future[Map[String, String]] = + for { + _ <- Future.successful(Mdc.putMdc(mdcData)) + _ <- httpClientV2 + .put(url"$wireMockUrl/") + .withBody(Json.toJson(ReqDomain("req"))) + .setHeader("User-Agent" -> "ua2") + .execute[ResDomain] + } yield Mdc.mdcData + + res.futureValue shouldBe mdcData + + wireMockServer.verify( + putRequestedFor(urlEqualTo("/")) + .withRequestBody(equalTo("\"req\"")) + .withHeader("User-Agent", equalTo("ua2")) + ) + } } trait Setup { From 216753d0bab9d4cf8c7b48ae62e755e5c2b90238 Mon Sep 17 00:00:00 2001 From: colin-lamed <9568290+colin-lamed@users.noreply.github.com> Date: Thu, 22 Sep 2022 12:58:27 +0100 Subject: [PATCH 2/3] Update dependencies --- build.sbt | 8 +------- .../src/main/scala/uk/gov/hmrc/http/HttpExceptions.scala | 5 ++--- .../src/main/scala/uk/gov/hmrc/http/HttpReads.scala | 3 --- .../src/main/scala/uk/gov/hmrc/http/HttpResponse.scala | 3 +-- .../src/test/scala/uk/gov/hmrc/http/HeadersSpec.scala | 2 -- .../scala/uk/gov/hmrc/http/HttpErrorFunctionsSpec.scala | 3 +-- .../uk/gov/hmrc/http/HttpReadsLegacyInstancesSpec.scala | 3 +-- .../src/test/scala/uk/gov/hmrc/http/RetriesSpec.scala | 2 -- .../scala/uk/gov/hmrc/play/connectors/ConnectorSpec.scala | 3 +-- .../gov/hmrc/play/http/HeaderCarrierConverterSpec.scala | 3 +-- .../http/logging/MdcLoggingExecutionContextSpec.scala | 3 +-- project/build.properties | 2 +- project/plugins.sbt | 2 +- 13 files changed, 11 insertions(+), 31 deletions(-) diff --git a/build.sbt b/build.sbt index 792caf41..40dde5da 100644 --- a/build.sbt +++ b/build.sbt @@ -8,18 +8,12 @@ Global / concurrentRestrictions += Tags.limitSum(1, Tags.Test, Tags.Untagged) val scala2_12 = "2.12.15" val scala2_13 = "2.13.8" -val silencerVersion = "1.7.8" - lazy val commonSettings = Seq( organization := "uk.gov.hmrc", majorVersion := 14, scalaVersion := scala2_12, isPublicArtefact := true, - scalacOptions ++= Seq("-feature"), - libraryDependencies ++= Seq( - compilerPlugin("com.github.ghik" % "silencer-plugin" % silencerVersion cross CrossVersion.full), - "com.github.ghik" % "silencer-lib" % silencerVersion % Provided cross CrossVersion.full - ) + scalacOptions ++= Seq("-feature") ) lazy val library = (project in file(".")) diff --git a/http-verbs-common/src/main/scala/uk/gov/hmrc/http/HttpExceptions.scala b/http-verbs-common/src/main/scala/uk/gov/hmrc/http/HttpExceptions.scala index 80678d45..d7a9d43e 100644 --- a/http-verbs-common/src/main/scala/uk/gov/hmrc/http/HttpExceptions.scala +++ b/http-verbs-common/src/main/scala/uk/gov/hmrc/http/HttpExceptions.scala @@ -16,7 +16,6 @@ package uk.gov.hmrc.http -import com.github.ghik.silencer.silent import uk.gov.hmrc.http.HttpExceptions._ private object HttpExceptions { @@ -131,7 +130,7 @@ sealed trait UpstreamErrorResponse extends Exception { def upstreamResponseCode: Int // final to help migrate away from upstreamResponseCode (i.e. read only - set via UpstreamErrorResponse.apply) - @silent("deprecated") + @annotation.nowarn("msg=deprecated") final def statusCode: Int = upstreamResponseCode @@ -176,7 +175,7 @@ object UpstreamErrorResponse { headers = Map.empty ) - @silent("deprecated") + @annotation.nowarn("msg=deprecated") def apply(message: String, statusCode: Int, reportAs: Int, headers: Map[String, Seq[String]]): UpstreamErrorResponse = if (HttpErrorFunctions.is4xx(statusCode)) uk.gov.hmrc.http.Upstream4xxResponse( diff --git a/http-verbs-common/src/main/scala/uk/gov/hmrc/http/HttpReads.scala b/http-verbs-common/src/main/scala/uk/gov/hmrc/http/HttpReads.scala index 8fd56f52..cbff5a69 100644 --- a/http-verbs-common/src/main/scala/uk/gov/hmrc/http/HttpReads.scala +++ b/http-verbs-common/src/main/scala/uk/gov/hmrc/http/HttpReads.scala @@ -16,8 +16,6 @@ package uk.gov.hmrc.http -import com.github.ghik.silencer.silent - object HttpReads extends HttpReadsLegacyInstances { def apply[A : HttpReads] = implicitly[HttpReads[A]] @@ -40,7 +38,6 @@ object HttpReads extends HttpReadsLegacyInstances { // compilation priority during implicit resolution. This means, unless // specified otherwise a verb call will return a plain HttpResponse @deprecated("Use uk.gov.hmrc.http.HttpReads.Implicits instead. See README for differences.", "11.0.0") - @silent("deprecated") implicit val readRaw: HttpReads[HttpResponse] = HttpReadsLegacyRawReads.readRaw object Implicits extends HttpReadsInstances diff --git a/http-verbs-common/src/main/scala/uk/gov/hmrc/http/HttpResponse.scala b/http-verbs-common/src/main/scala/uk/gov/hmrc/http/HttpResponse.scala index 83369b8b..5aa6358d 100644 --- a/http-verbs-common/src/main/scala/uk/gov/hmrc/http/HttpResponse.scala +++ b/http-verbs-common/src/main/scala/uk/gov/hmrc/http/HttpResponse.scala @@ -18,7 +18,6 @@ package uk.gov.hmrc.http import akka.stream.scaladsl.Source import akka.util.ByteString -import com.github.ghik.silencer.silent import play.api.libs.json.{JsValue, Json} /** @@ -39,7 +38,7 @@ trait HttpResponse { def allHeaders: Map[String, Seq[String]] // final to help migrate away from allHeaders (i.e. read only - set via HttpResponse.apply) - @silent("deprecated") + @annotation.nowarn("msg=deprecated") final def headers: Map[String, Seq[String]] = allHeaders diff --git a/http-verbs-common/src/test/scala/uk/gov/hmrc/http/HeadersSpec.scala b/http-verbs-common/src/test/scala/uk/gov/hmrc/http/HeadersSpec.scala index 4038a53b..60914148 100644 --- a/http-verbs-common/src/test/scala/uk/gov/hmrc/http/HeadersSpec.scala +++ b/http-verbs-common/src/test/scala/uk/gov/hmrc/http/HeadersSpec.scala @@ -17,7 +17,6 @@ package uk.gov.hmrc.http import akka.actor.ActorSystem -import com.github.ghik.silencer.silent import com.github.tomakehurst.wiremock.client.WireMock._ import com.typesafe.config.Config import org.scalatest.concurrent.{IntegrationPatience, ScalaFutures} @@ -44,7 +43,6 @@ class HeadersSpec private lazy val app: Application = new GuiceApplicationBuilder().build() - @silent("deprecated") private implicit val hc: HeaderCarrier = HeaderCarrier( authorization = Some(Authorization("authorization")), forwarded = Some(ForwardedFor("forwarded-for")), diff --git a/http-verbs-common/src/test/scala/uk/gov/hmrc/http/HttpErrorFunctionsSpec.scala b/http-verbs-common/src/test/scala/uk/gov/hmrc/http/HttpErrorFunctionsSpec.scala index 4894f11e..be42e9d7 100644 --- a/http-verbs-common/src/test/scala/uk/gov/hmrc/http/HttpErrorFunctionsSpec.scala +++ b/http-verbs-common/src/test/scala/uk/gov/hmrc/http/HttpErrorFunctionsSpec.scala @@ -16,7 +16,6 @@ package uk.gov.hmrc.http -import com.github.ghik.silencer.silent import org.scalacheck.Gen import org.scalatest.prop.TableDrivenPropertyChecks import org.scalatest.TryValues @@ -26,7 +25,7 @@ import org.scalatestplus.scalacheck.ScalaCheckDrivenPropertyChecks import scala.util.Try -@silent("deprecated") +@annotation.nowarn("msg=deprecated") class HttpErrorFunctionsSpec extends AnyWordSpec with Matchers diff --git a/http-verbs-common/src/test/scala/uk/gov/hmrc/http/HttpReadsLegacyInstancesSpec.scala b/http-verbs-common/src/test/scala/uk/gov/hmrc/http/HttpReadsLegacyInstancesSpec.scala index 85ec218a..97b1f2fb 100644 --- a/http-verbs-common/src/test/scala/uk/gov/hmrc/http/HttpReadsLegacyInstancesSpec.scala +++ b/http-verbs-common/src/test/scala/uk/gov/hmrc/http/HttpReadsLegacyInstancesSpec.scala @@ -16,14 +16,13 @@ package uk.gov.hmrc.http -import com.github.ghik.silencer.silent import org.scalacheck.Gen import org.scalatestplus.scalacheck.ScalaCheckDrivenPropertyChecks import org.scalatest.wordspec.AnyWordSpec import org.scalatest.matchers.should.Matchers import play.api.libs.json.Json -@silent("deprecated") +@annotation.nowarn("msg=deprecated") class HttpReadsLegacyInstancesSpec extends AnyWordSpec with ScalaCheckDrivenPropertyChecks with Matchers { "RawReads" should { val reads = HttpReads.readRaw diff --git a/http-verbs-common/src/test/scala/uk/gov/hmrc/http/RetriesSpec.scala b/http-verbs-common/src/test/scala/uk/gov/hmrc/http/RetriesSpec.scala index 24728c43..47ce9709 100644 --- a/http-verbs-common/src/test/scala/uk/gov/hmrc/http/RetriesSpec.scala +++ b/http-verbs-common/src/test/scala/uk/gov/hmrc/http/RetriesSpec.scala @@ -27,13 +27,11 @@ import org.mockito.scalatest.MockitoSugar import org.scalatest.concurrent.{IntegrationPatience, ScalaFutures} import org.scalatest.matchers.should.Matchers import org.scalatest.wordspec.AnyWordSpecLike -import org.slf4j.MDC import play.api.libs.json.{JsValue, Json, Writes} import uk.gov.hmrc.http.HttpReads.Implicits._ import uk.gov.hmrc.http.hooks.{HttpHook, HttpHooks} import uk.gov.hmrc.play.http.logging.Mdc -import scala.collection.JavaConverters._ import scala.concurrent.duration._ import scala.concurrent.{ExecutionContext, Future} import scala.util.{Random, Try} diff --git a/http-verbs-common/src/test/scala/uk/gov/hmrc/play/connectors/ConnectorSpec.scala b/http-verbs-common/src/test/scala/uk/gov/hmrc/play/connectors/ConnectorSpec.scala index 3c2af8b8..b38fd6d6 100644 --- a/http-verbs-common/src/test/scala/uk/gov/hmrc/play/connectors/ConnectorSpec.scala +++ b/http-verbs-common/src/test/scala/uk/gov/hmrc/play/connectors/ConnectorSpec.scala @@ -16,14 +16,13 @@ package uk.gov.hmrc.play.connectors -import com.github.ghik.silencer.silent import org.mockito.scalatest.MockitoSugar import org.scalatest.wordspec.AnyWordSpecLike import org.scalatest.matchers.should.Matchers import play.api.test.WsTestClient import uk.gov.hmrc.http._ -@silent("deprecated") +@annotation.nowarn("msg=deprecated") class ConnectorSpec extends AnyWordSpecLike with Matchers with MockitoSugar { WsTestClient.withClient { wsClient => diff --git a/http-verbs-common/src/test/scala/uk/gov/hmrc/play/http/HeaderCarrierConverterSpec.scala b/http-verbs-common/src/test/scala/uk/gov/hmrc/play/http/HeaderCarrierConverterSpec.scala index d2d57723..75fae190 100644 --- a/http-verbs-common/src/test/scala/uk/gov/hmrc/play/http/HeaderCarrierConverterSpec.scala +++ b/http-verbs-common/src/test/scala/uk/gov/hmrc/play/http/HeaderCarrierConverterSpec.scala @@ -16,7 +16,6 @@ package uk.gov.hmrc.play.http -import com.github.ghik.silencer.silent import org.scalatest.BeforeAndAfterAll import org.scalatest.wordspec.AnyWordSpecLike import org.scalatest.matchers.should.Matchers @@ -31,7 +30,7 @@ import uk.gov.hmrc.http._ import scala.concurrent.duration._ -@silent("deprecated") +@annotation.nowarn("msg=deprecated") class HeaderCarrierConverterSpec extends AnyWordSpecLike with Matchers with BeforeAndAfterAll { "Extracting the request timestamp from the session and headers" should { diff --git a/http-verbs-common/src/test/scala/uk/gov/hmrc/play/http/logging/MdcLoggingExecutionContextSpec.scala b/http-verbs-common/src/test/scala/uk/gov/hmrc/play/http/logging/MdcLoggingExecutionContextSpec.scala index 582b4f6a..e0e32ea0 100644 --- a/http-verbs-common/src/test/scala/uk/gov/hmrc/play/http/logging/MdcLoggingExecutionContextSpec.scala +++ b/http-verbs-common/src/test/scala/uk/gov/hmrc/play/http/logging/MdcLoggingExecutionContextSpec.scala @@ -18,7 +18,6 @@ package uk.gov.hmrc.play.http.logging import java.util.concurrent.{CountDownLatch, Executors} -import com.github.ghik.silencer.silent import ch.qos.logback.classic.spi.ILoggingEvent import ch.qos.logback.classic.{Level, Logger => LogbackLogger} import ch.qos.logback.core.AppenderBase @@ -34,7 +33,7 @@ import scala.concurrent.duration._ import scala.concurrent.{Await, ExecutionContext, Future} import scala.reflect._ -@silent("deprecated") +@annotation.nowarn("msg=deprecated") class MdcLoggingExecutionContextSpec extends AnyWordSpecLike with Matchers diff --git a/project/build.properties b/project/build.properties index 8a847661..5e3bdc0a 100644 --- a/project/build.properties +++ b/project/build.properties @@ -14,4 +14,4 @@ # limitations under the License. # -sbt.version=1.5.8 +sbt.version=1.6.2 diff --git a/project/plugins.sbt b/project/plugins.sbt index cde6abc4..b50ff884 100644 --- a/project/plugins.sbt +++ b/project/plugins.sbt @@ -1,4 +1,4 @@ resolvers += MavenRepository("HMRC-open-artefacts-maven2", "https://open.artefacts.tax.service.gov.uk/maven2") resolvers += Resolver.url("HMRC-open-artefacts-ivy2", url("https://open.artefacts.tax.service.gov.uk/ivy2"))(Resolver.ivyStylePatterns) -addSbtPlugin("uk.gov.hmrc" % "sbt-auto-build" % "3.6.0") +addSbtPlugin("uk.gov.hmrc" % "sbt-auto-build" % "3.8.0") From 8aeca60fe21ed1f6584911ff29c248156d3c2319 Mon Sep 17 00:00:00 2001 From: colin-lamed <9568290+colin-lamed@users.noreply.github.com> Date: Thu, 22 Sep 2022 13:11:58 +0100 Subject: [PATCH 3/3] Fix MdcLoggingExecutionContext and tests --- .../logging/MdcLoggingExecutionContext.scala | 7 +- .../scala/uk/gov/hmrc/http/RetriesSpec.scala | 2 + .../hmrc/http/client/HttpClientV2Spec.scala | 2 + .../MdcLoggingExecutionContextSpec.scala | 68 ++++++++----------- .../gov/hmrc/play/http/logging/MdcSpec.scala | 22 +++--- 5 files changed, 50 insertions(+), 51 deletions(-) diff --git a/http-verbs-common/src/main/scala/uk/gov/hmrc/play/http/logging/MdcLoggingExecutionContext.scala b/http-verbs-common/src/main/scala/uk/gov/hmrc/play/http/logging/MdcLoggingExecutionContext.scala index b688f01c..02d01230 100644 --- a/http-verbs-common/src/main/scala/uk/gov/hmrc/play/http/logging/MdcLoggingExecutionContext.scala +++ b/http-verbs-common/src/main/scala/uk/gov/hmrc/play/http/logging/MdcLoggingExecutionContext.scala @@ -29,13 +29,18 @@ class MdcLoggingExecutionContext(wrapped: ExecutionContext, mdcData: Map[String, private class RunWithMDC(runnable: Runnable, mdcData: Map[String, String]) extends Runnable { def run(): Unit = { + val oldMdcData = MDC.getCopyOfContextMap + MDC.clear() mdcData.foreach { case (k, v) => MDC.put(k, v) } try { runnable.run() } finally { - MDC.clear() + if (oldMdcData == null) + MDC.clear() + else + MDC.setContextMap(oldMdcData) } } } diff --git a/http-verbs-common/src/test/scala/uk/gov/hmrc/http/RetriesSpec.scala b/http-verbs-common/src/test/scala/uk/gov/hmrc/http/RetriesSpec.scala index 47ce9709..c933efa4 100644 --- a/http-verbs-common/src/test/scala/uk/gov/hmrc/http/RetriesSpec.scala +++ b/http-verbs-common/src/test/scala/uk/gov/hmrc/http/RetriesSpec.scala @@ -193,6 +193,8 @@ class RetriesSpec val expectedResponse = HttpResponse(404, "") + org.slf4j.MDC.clear() + val resultF = for { _ <- Future.successful(Mdc.putMdc(mdcData)) diff --git a/http-verbs-common/src/test/scala/uk/gov/hmrc/http/client/HttpClientV2Spec.scala b/http-verbs-common/src/test/scala/uk/gov/hmrc/http/client/HttpClientV2Spec.scala index c64f5769..5d6d6bd0 100644 --- a/http-verbs-common/src/test/scala/uk/gov/hmrc/http/client/HttpClientV2Spec.scala +++ b/http-verbs-common/src/test/scala/uk/gov/hmrc/http/client/HttpClientV2Spec.scala @@ -603,6 +603,8 @@ class HttpClientV2Spec implicit val mdcEc = ExecutionContext.fromExecutor(new uk.gov.hmrc.play.http.logging.MDCPropagatingExecutorService(Executors.newFixedThreadPool(2))) + org.slf4j.MDC.clear() + val res: Future[Map[String, String]] = for { _ <- Future.successful(Mdc.putMdc(mdcData)) diff --git a/http-verbs-common/src/test/scala/uk/gov/hmrc/play/http/logging/MdcLoggingExecutionContextSpec.scala b/http-verbs-common/src/test/scala/uk/gov/hmrc/play/http/logging/MdcLoggingExecutionContextSpec.scala index e0e32ea0..a9a55126 100644 --- a/http-verbs-common/src/test/scala/uk/gov/hmrc/play/http/logging/MdcLoggingExecutionContextSpec.scala +++ b/http-verbs-common/src/test/scala/uk/gov/hmrc/play/http/logging/MdcLoggingExecutionContextSpec.scala @@ -27,7 +27,6 @@ import org.scalatest.matchers.should.Matchers import org.slf4j.{LoggerFactory, MDC} import play.core.NamedThreadFactory -import scala.collection.JavaConverters._ import scala.collection.mutable import scala.concurrent.duration._ import scala.concurrent.{Await, ExecutionContext, Future} @@ -35,20 +34,18 @@ import scala.reflect._ @annotation.nowarn("msg=deprecated") class MdcLoggingExecutionContextSpec - extends AnyWordSpecLike - with Matchers - with LoneElement - with Inspectors - with BeforeAndAfter { + extends AnyWordSpecLike + with Matchers + with LoneElement + with Inspectors + with BeforeAndAfter { before { MDC.clear() } "The MDC Transporting Execution Context" should { - - "capture the an MDC map with values in it and put it in place when a task is run" in withCaptureOfLoggingFrom[ - MdcLoggingExecutionContextSpec] { logList => + "capture the MDC map with values in it and put it in place when a task is run" in withCaptureOfLoggingFrom[MdcLoggingExecutionContextSpec] { logList => implicit val ec = createAndInitialiseMdcTransportingExecutionContext(Map("someKey" -> "something")) logEventInsideAFutureUsing(ec) @@ -75,20 +72,18 @@ class MdcLoggingExecutionContextSpec logList.loneElement._2 should be(Map("someKey" -> "something")) } - "clear the MDC map after a task throws an exception" in withCaptureOfLoggingFrom[MdcLoggingExecutionContextSpec] { - logList => - implicit val ec = createAndInitialiseMdcTransportingExecutionContext(Map("someKey" -> "something")) + "clear the MDC map after a task throws an exception" in withCaptureOfLoggingFrom[MdcLoggingExecutionContextSpec] { logList => + implicit val ec = createAndInitialiseMdcTransportingExecutionContext(Map("someKey" -> "something")) - throwAnExceptionInATaskOn(ec) + throwAnExceptionInATaskOn(ec) - MDC.clear() - logEventInsideAFutureUsing(ec) + MDC.clear() + logEventInsideAFutureUsing(ec) - logList.loneElement._2 should be(Map("someKey" -> "something")) + logList.loneElement._2 should be(Map("someKey" -> "something")) } - "log values from given MDC map when multiple threads are using it concurrently by ensuring each log from each thread has been logged via MDC" in withCaptureOfLoggingFrom[ - MdcLoggingExecutionContextSpec] { logList => + "log values from given MDC map when multiple threads are using it concurrently by ensuring each log from each thread has been logged via MDC" in withCaptureOfLoggingFrom[MdcLoggingExecutionContextSpec] { logList => val threadCount = 10 val logCount = 10 @@ -131,36 +126,33 @@ class MdcLoggingExecutionContextSpec ec } - def logEventInsideAFutureUsingImplicitEc(implicit ec: ExecutionContext) { + def logEventInsideAFutureUsingImplicitEc(implicit ec: ExecutionContext): Unit = logEventInsideAFutureUsing(ec) - } - def logEventInsideAFutureUsing(ec: ExecutionContext) { - Await.ready(Future { - LoggerFactory.getLogger(classOf[MdcLoggingExecutionContextSpec]).info("") - }(ec), 2.second) - } + def logEventInsideAFutureUsing(ec: ExecutionContext): Unit = + Await.ready( + Future.apply( + LoggerFactory.getLogger(classOf[MdcLoggingExecutionContextSpec]).info("") + )(ec), + 2.second + ) - def doSomethingInsideAFutureButDontLog(ec: ExecutionContext) { - Await.ready(Future {}(ec), 2.second) - } + def doSomethingInsideAFutureButDontLog(ec: ExecutionContext): Unit = + Await.ready(Future.apply()(ec), 2.second) - def throwAnExceptionInATaskOn(ec: ExecutionContext) { - ec.execute(new Runnable() { - def run(): Unit = - throw new RuntimeException("Test what happens when a task running on this EC throws an exception") - }) - } + def throwAnExceptionInATaskOn(ec: ExecutionContext): Unit = + ec.execute(() => throw new RuntimeException("Test what happens when a task running on this EC throws an exception")) /** Ensures that a thread is already created in the execution context by running an empty future. * Required as otherwise the MDC is transferred to the new thread as it is stored in an inheritable * ThreadLocal. */ - def initialise(ec: ExecutionContext) { - Await.ready(Future {}(ec), 2.second) - } + def initialise(ec: ExecutionContext): Unit = + Await.ready(Future.apply()(ec), 2.second) + + def withCaptureOfLoggingFrom[T: ClassTag](body: (=> List[(ILoggingEvent, Map[String, String])]) => Unit): Unit = { + import scala.collection.JavaConverters._ - def withCaptureOfLoggingFrom[T: ClassTag](body: (=> List[(ILoggingEvent, Map[String, String])]) => Any) = { val logger = LoggerFactory.getLogger(classTag[T].runtimeClass).asInstanceOf[LogbackLogger] val appender = new AppenderBase[ILoggingEvent]() { diff --git a/http-verbs-common/src/test/scala/uk/gov/hmrc/play/http/logging/MdcSpec.scala b/http-verbs-common/src/test/scala/uk/gov/hmrc/play/http/logging/MdcSpec.scala index f91b9310..e0bf3c74 100644 --- a/http-verbs-common/src/test/scala/uk/gov/hmrc/play/http/logging/MdcSpec.scala +++ b/http-verbs-common/src/test/scala/uk/gov/hmrc/play/http/logging/MdcSpec.scala @@ -18,7 +18,7 @@ package uk.gov.hmrc.play.http.logging import akka.dispatch.ExecutorServiceDelegate import org.scalatest.BeforeAndAfter -import org.scalatest.concurrent.ScalaFutures +import org.scalatest.concurrent.{IntegrationPatience, ScalaFutures} import org.scalatest.matchers.should.Matchers import org.scalatest.wordspec.AnyWordSpecLike import org.slf4j.MDC @@ -31,6 +31,7 @@ class MdcSpec extends AnyWordSpecLike with Matchers with ScalaFutures + with IntegrationPatience with BeforeAndAfter { before { @@ -96,19 +97,16 @@ class MdcSpec class MDCPropagatingExecutorService(val executor: ExecutorService) extends ExecutorServiceDelegate { override def execute(command: Runnable): Unit = { - val mdcData = MDC.getCopyOfContextMap - executor.execute(new Runnable { - override def run(): Unit = { - val oldMdcData = MDC.getCopyOfContextMap - setMDC(mdcData) - try { - command.run() - } finally { - // this means any Mdc updates on the ec will not be propagated once it steps out - setMDC(oldMdcData) - } + executor.execute(() => { + val oldMdcData = MDC.getCopyOfContextMap + setMDC(mdcData) + try { + command.run() + } finally { + // this means any Mdc updates on the ec will not be propagated once it steps out + setMDC(oldMdcData) } }) }