Merge pull request #47 from hmrc/APID-192

APID-192 - handle error scenarios with soap http requests
hmrc · Jul 8, 2021 · 961eadb · 961eadb
2 parents bec5a9d + e36ea6b
commit 961eadb
Show file tree

Hide file tree

Showing 6 changed files with 112 additions and 49 deletions.
diff --git a/app/uk/gov/hmrc/apiplatformoutboundsoap/config/AppConfig.scala b/app/uk/gov/hmrc/apiplatformoutboundsoap/config/AppConfig.scala
@@ -52,4 +52,6 @@ class AppConfig @Inject()(config: Configuration, servicesConfig: ServicesConfig)
   val addressingReplyTo: String = config.getOptional[String]("addressing.replyTo").getOrElse("")
   val addressingFaultTo: String = config.getOptional[String]("addressing.faultTo").getOrElse("")
   val confirmationOfDelivery: Boolean = config.getOptional[Boolean]("confirmationOfDelivery").getOrElse(false)
+  val proxyRequiredForThisEnvironment = config.getOptional[Boolean]("proxy.proxyRequiredForThisEnvironment").getOrElse(false)
+
 }
diff --git a/app/uk/gov/hmrc/apiplatformoutboundsoap/connectors/OutboundConnector.scala b/app/uk/gov/hmrc/apiplatformoutboundsoap/connectors/OutboundConnector.scala
@@ -16,8 +16,10 @@
 
 package uk.gov.hmrc.apiplatformoutboundsoap.connectors
 
+import org.apache.http.HttpStatus
 import play.api.http.HeaderNames.CONTENT_TYPE
-import play.api.{Configuration, Logger, LoggerLike}
+import play.api.{Logger, LoggerLike}
+import uk.gov.hmrc.apiplatformoutboundsoap.config.AppConfig
 import uk.gov.hmrc.apiplatformoutboundsoap.models.SoapRequest
 import uk.gov.hmrc.http.HttpReads.Implicits._
 import uk.gov.hmrc.http.{HttpClient, _}
@@ -27,28 +29,37 @@ import scala.concurrent.{ExecutionContext, Future}
 
 @Singleton
 class OutboundConnector @Inject()(
-               config: Configuration,
-               defaultHttpClient: HttpClient,
-              proxiedHttpClient: ProxiedHttpClient)
-             (implicit ec: ExecutionContext) extends HttpErrorFunctions {
+                                   appConfig: AppConfig,
+                                   defaultHttpClient: HttpClient,
+                                   proxiedHttpClient: ProxiedHttpClient)
+                                 (implicit ec: ExecutionContext) extends HttpErrorFunctions {
 
   val logger: LoggerLike = Logger
   val useProxy: Boolean = useProxyForEnv()
   lazy val httpClient: HttpClient = if (useProxy) proxiedHttpClient else defaultHttpClient
 
   def postMessage(soapRequest: SoapRequest): Future[Int] = {
-    implicit val hc: HeaderCarrier =  HeaderCarrier().withExtraHeaders(CONTENT_TYPE -> "application/soap+xml")
+    implicit val hc: HeaderCarrier = HeaderCarrier().withExtraHeaders(CONTENT_TYPE -> "application/soap+xml")
 
-    httpClient.POSTString[HttpResponse](soapRequest.destinationUrl, soapRequest.soapEnvelope) map { response =>
-      if (!is2xx(response.status)) {
-        logger.warn(s"Attempted request to ${soapRequest.destinationUrl} responded with HTTP response code ${response.status}")
-      }
-      response.status
+    def requestLogMessage(statusCode: Int) = s"Attempted request to ${soapRequest.destinationUrl} responded with HTTP response code $statusCode"
+
+    postHttpRequest(soapRequest).map {
+      case Left(UpstreamErrorResponse(_, statusCode, _, _)) =>
+        logger.warn(requestLogMessage(statusCode))
+        statusCode
+      case Right(response: HttpResponse) =>
+        if (response.status != HttpStatus.SC_ACCEPTED) {
+          logger.warn(requestLogMessage(response.status))
+        }
+        response.status
     }
   }
 
+  def postHttpRequest(soapRequest: SoapRequest)(implicit hc: HeaderCarrier) = {
+    httpClient.POSTString[Either[UpstreamErrorResponse, HttpResponse]](soapRequest.destinationUrl, soapRequest.soapEnvelope)
+  }
 
   def useProxyForEnv(): Boolean = {
-    config.getOptional[Boolean]("proxy.proxyRequiredForThisEnvironment").getOrElse(false)
+    appConfig.proxyRequiredForThisEnvironment
   }
 }
diff --git a/app/uk/gov/hmrc/apiplatformoutboundsoap/services/OutboundService.scala b/app/uk/gov/hmrc/apiplatformoutboundsoap/services/OutboundService.scala
@@ -79,20 +79,20 @@ class OutboundService @Inject()(outboundConnector: OutboundConnector,
     val nextRetryDateTime: DateTime = now.plus(appConfig.retryInterval.toMillis)
     outboundConnector.postMessage(SoapRequest(message.soapMessage, message.destinationUrl)) flatMap { result =>
       if (is2xx(result)) {
-        logger.info(s"Retrying message with global ID ${message.globalId} and message ID ${message.messageId} succeeded")
+        logger.info(s"Retried message with global ID ${message.globalId} and message ID ${message.messageId} and succeeded")
         outboundMessageRepository.updateSendingStatus(message.globalId, SendingStatus.SENT) map { updatedMessage =>
           updatedMessage.map(notificationCallbackConnector.sendNotification)
           ()
         }
       } else {
         if (message.createDateTime.plus(appConfig.retryDuration.toMillis).isBefore(now.getMillis)) {
-          logger.info(s"Retrying message with global ID ${message.globalId} and message ID ${message.messageId} failed on last attempt")
+          logger.info(s"Retried message with global ID ${message.globalId} and message ID ${message.messageId} but failed on last attempt")
           outboundMessageRepository.updateSendingStatus(message.globalId, SendingStatus.FAILED).map { updatedMessage =>
             updatedMessage.map(notificationCallbackConnector.sendNotification)
             ()
           }
         } else {
-          logger.info(s"Retrying message with global ID ${message.globalId} and message ID ${message.messageId} failed")
+          logger.info(s"Retried message with global ID ${message.globalId} and message ID ${message.messageId} but failed")
           outboundMessageRepository.updateNextRetryTime(message.globalId, nextRetryDateTime).map(_ => ())
         }
       }
@@ -105,6 +105,9 @@ class OutboundService @Inject()(outboundConnector: OutboundConnector,
     if (is2xx(result)) {
       logger.info(s"Message with global ID $globalId and message ID $messageId successfully sent")
       SentOutboundSoapMessage(globalId, messageId, soapRequest.soapEnvelope, soapRequest.destinationUrl, now, result, message.notificationUrl)
+    } else if(is3xx(result)|| is4xx(result)) {
+      logger.info(s"Message with global ID $globalId and message ID $messageId failed")
+      FailedOutboundSoapMessage(globalId, messageId, soapRequest.soapEnvelope, soapRequest.destinationUrl, now, result, message.notificationUrl)
     } else {
       logger.info(s"Message with global ID $globalId and message ID $messageId failed on first attempt")
       RetryingOutboundSoapMessage(globalId, messageId, soapRequest.soapEnvelope, soapRequest.destinationUrl, now,
@@ -203,4 +206,6 @@ class OutboundService @Inject()(outboundConnector: OutboundConnector,
     if (message.messageBody.nonEmpty) payload.addChild(stringToOM(message.messageBody))
     envelope.getBody.addChild(payload)
   }
+
+  private def is3xx(result: Int): Boolean = result >= 300 && result < 400
 }
diff --git a/test/uk/gov/hmrc/apiplatformoutboundsoap/connectors/OutboundConnectorSpec.scala b/test/uk/gov/hmrc/apiplatformoutboundsoap/connectors/OutboundConnectorSpec.scala
@@ -16,48 +16,75 @@
 
 package uk.gov.hmrc.apiplatformoutboundsoap.connectors
 
-import org.mockito.MockitoSugar
+import org.mockito.{ArgumentMatchersSugar, MockitoSugar}
 import org.scalatest.matchers.should.Matchers
 import org.scalatest.wordspec.AnyWordSpec
 import org.scalatestplus.play.guice.GuiceOneAppPerSuite
-import play.api.{Application, Configuration}
+import play.api.Application
+import play.api.http.Status.OK
 import play.api.inject.guice.GuiceApplicationBuilder
-import uk.gov.hmrc.http.{HeaderCarrier, HttpClient}
+import play.api.test.Helpers.{INTERNAL_SERVER_ERROR, await, defaultAwaitTimeout}
+import uk.gov.hmrc.apiplatformoutboundsoap.config.AppConfig
+import uk.gov.hmrc.apiplatformoutboundsoap.models.SoapRequest
+import uk.gov.hmrc.http.{HeaderCarrier, HttpClient, HttpResponse, UpstreamErrorResponse}
+
 import scala.concurrent.ExecutionContext.Implicits.global
+import scala.concurrent.Future.successful
 
 
-class OutboundConnectorSpec extends AnyWordSpec with Matchers with GuiceOneAppPerSuite with MockitoSugar {
+class OutboundConnectorSpec extends AnyWordSpec with Matchers with GuiceOneAppPerSuite with MockitoSugar with ArgumentMatchersSugar {
   implicit val hc: HeaderCarrier = HeaderCarrier()
 
   override lazy val app: Application = GuiceApplicationBuilder()
     .configure()
     .build()
 
   trait Setup {
+    val appConfigMock: AppConfig = mock[AppConfig]
     val mockDefaultHttpClient: HttpClient = mock[HttpClient]
     val mockProxiedHttpClient: ProxiedHttpClient = mock[ProxiedHttpClient]
   }
 
   "OutboundConnector" should {
     "use proxy when configured" in new Setup {
-      val config: Configuration = app.configuration ++ Configuration("proxy.proxyRequiredForThisEnvironment" -> true)
-
-      val underTest = new OutboundConnector(config, mockDefaultHttpClient, mockProxiedHttpClient)
+      when(appConfigMock.proxyRequiredForThisEnvironment).thenReturn(true)
+      val underTest = new OutboundConnector(appConfigMock, mockDefaultHttpClient, mockProxiedHttpClient)
       underTest.httpClient shouldBe mockProxiedHttpClient
     }
 
     "use default ws client when proxy is disabled" in new Setup {
-      val config: Configuration = app.configuration ++ Configuration("proxy.proxyRequiredForThisEnvironment" -> false)
-
-      val underTest = new OutboundConnector(config, mockDefaultHttpClient, mockProxiedHttpClient)
+      when(appConfigMock.proxyRequiredForThisEnvironment).thenReturn(false)
+      val underTest = new OutboundConnector(appConfigMock, mockDefaultHttpClient, mockProxiedHttpClient)
       underTest.httpClient shouldBe mockDefaultHttpClient
     }
 
     "use default ws client when proxy is not configured" in new Setup {
-      val config: Configuration = app.configuration
-      val underTest = new OutboundConnector(config, mockDefaultHttpClient, mockProxiedHttpClient)
+      val underTest = new OutboundConnector(appConfigMock, mockDefaultHttpClient, mockProxiedHttpClient)
+      underTest.httpClient shouldBe mockDefaultHttpClient
+    }
+
+    "return valid status code if http post returns 2xx" in new Setup {
+      val soapRequestMock: SoapRequest = mock[SoapRequest]
+      when(soapRequestMock.soapEnvelope).thenReturn("<IE4N03>payload</IE4N03>")
+      when(soapRequestMock.destinationUrl).thenReturn("some url")
+      when(appConfigMock.proxyRequiredForThisEnvironment).thenReturn(false)
+      when(mockDefaultHttpClient.POSTString[Either[UpstreamErrorResponse,HttpResponse]](*,*,*)(*,*,*)).thenReturn(successful(Right(HttpResponse(OK,""))))
+      val underTest = new OutboundConnector(appConfigMock, mockDefaultHttpClient, mockProxiedHttpClient)
       underTest.httpClient shouldBe mockDefaultHttpClient
+      val result: Int = await(underTest.postMessage(soapRequestMock))
+      result shouldBe OK
     }
 
+    "return valid status code if http post returns 5xx" in new Setup {
+      val soapRequestMock: SoapRequest = mock[SoapRequest]
+      when(soapRequestMock.soapEnvelope).thenReturn("<IE4N03>payload</IE4N03>")
+      when(soapRequestMock.destinationUrl).thenReturn("some url")
+      when(appConfigMock.proxyRequiredForThisEnvironment).thenReturn(false)
+      when(mockDefaultHttpClient.POSTString[Either[UpstreamErrorResponse,HttpResponse]](*,*,*)(*,*,*)).thenReturn(successful(Left(UpstreamErrorResponse("unexpected error", INTERNAL_SERVER_ERROR))))
+      val underTest = new OutboundConnector(appConfigMock, mockDefaultHttpClient, mockProxiedHttpClient)
+      underTest.httpClient shouldBe mockDefaultHttpClient
+      val result: Int = await(underTest.postMessage(soapRequestMock))
+      result shouldBe INTERNAL_SERVER_ERROR
+    }
   }
 }
diff --git a/test/uk/gov/hmrc/apiplatformoutboundsoap/controllers/OutboundControllerSpec.scala b/test/uk/gov/hmrc/apiplatformoutboundsoap/controllers/OutboundControllerSpec.scala
@@ -24,7 +24,7 @@ import org.mockito.{ArgumentCaptor, ArgumentMatchersSugar, MockitoSugar}
 import org.scalatest.matchers.should.Matchers
 import org.scalatest.wordspec.AnyWordSpec
 import org.scalatestplus.play.guice.GuiceOneAppPerSuite
-import play.api.{Application, Configuration}
+import play.api.Application
 import play.api.http.Status.{BAD_REQUEST, OK}
 import play.api.inject.guice.GuiceApplicationBuilder
 import play.api.libs.json.{JsBoolean, Json}

diff --git a/test/uk/gov/hmrc/apiplatformoutboundsoap/services/OutboundServiceSpec.scala b/test/uk/gov/hmrc/apiplatformoutboundsoap/services/OutboundServiceSpec.scala
@@ -72,6 +72,7 @@ class OutboundServiceSpec extends AnyWordSpec with Matchers with GuiceOneAppPerS
     when(appConfigMock.addressingReplyTo).thenReturn("ReplyTo")
     when(appConfigMock.addressingFaultTo).thenReturn("FaultTo")
 
+    when(appConfigMock.proxyRequiredForThisEnvironment).thenReturn(false)
     val underTest: OutboundService = new OutboundService(outboundConnectorMock, wsSecurityServiceMock,
       outboundMessageRepositoryMock, notificationCallbackConnectorMock, appConfigMock, cacheSpy) {
       override def now: DateTime = expectedCreateDateTime
@@ -200,20 +201,16 @@ class OutboundServiceSpec extends AnyWordSpec with Matchers with GuiceOneAppPerS
       when(wsSecurityServiceMock.addUsernameToken(*)).thenReturn(expectedSoapEnvelope())
       when(outboundConnectorMock.postMessage(*)).thenReturn(successful(BAD_REQUEST))
       when(outboundMessageRepositoryMock.persist(*)).thenReturn(Future(InsertOneResult.acknowledged(BsonNumber(1))))
-      val expectedInterval = Duration("10s")
-      when(appConfigMock.retryInterval).thenReturn(expectedInterval)
-
       val result: OutboundSoapMessage = await(underTest.sendMessage(messageRequestFullAddressing))
 
-      result.status shouldBe SendingStatus.RETRYING
+      result.status shouldBe SendingStatus.FAILED
       result.soapMessage shouldBe expectedSoapEnvelope()
       result.messageId shouldBe messageId
       result.globalId shouldBe expectedGlobalId
       result.createDateTime shouldBe expectedCreateDateTime
-      result.asInstanceOf[RetryingOutboundSoapMessage].retryDateTime shouldBe expectedCreateDateTime.plus(expectedInterval.toMillis)
     }
 
-    "save the message as SENT when the connector returns 2XX" in new Setup {
+    "save the message as SENT when the connector returns 2xx" in new Setup {
       (200 to 299).foreach { httpCode =>
         when(wsSecurityServiceMock.addUsernameToken(*)).thenReturn(expectedSoapEnvelope())
         when(outboundConnectorMock.postMessage(*)).thenReturn(successful(httpCode))
@@ -226,13 +223,34 @@ class OutboundServiceSpec extends AnyWordSpec with Matchers with GuiceOneAppPerS
         messageCaptor.getValue.messageId shouldBe messageId
         messageCaptor.getValue.globalId shouldBe expectedGlobalId
         messageCaptor.getValue.createDateTime shouldBe expectedCreateDateTime
+        messageCaptor.getValue.ccnHttpStatus shouldBe httpCode
         messageCaptor.getValue.notificationUrl shouldBe messageRequestFullAddressing.notificationUrl
         messageCaptor.getValue.destinationUrl shouldBe "http://example.com:1234/CCN2.Service.Customs.EU.ICS.RiskAnalysisOrchestrationBAS"
       }
     }
 
-    "save the message as RETRYING when the connector returns a non 2XX" in new Setup {
-      (300 to 599).foreach { httpCode =>
+    "save the message as FAILED when the connector returns a 3xx, 4xx" in new Setup {
+      (300 to 499).foreach { httpCode =>
+        when(wsSecurityServiceMock.addUsernameToken(*)).thenReturn(expectedSoapEnvelope())
+        when(outboundConnectorMock.postMessage(*)).thenReturn(successful(httpCode))
+        val messageCaptor: ArgumentCaptor[OutboundSoapMessage] = ArgumentCaptor.forClass(classOf[OutboundSoapMessage])
+        when(outboundMessageRepositoryMock.persist(messageCaptor.capture())).thenReturn(Future(InsertOneResult.acknowledged(BsonNumber(1))))
+
+        await(underTest.sendMessage(messageRequestFullAddressing))
+
+        messageCaptor.getValue.status shouldBe SendingStatus.FAILED
+        messageCaptor.getValue.soapMessage shouldBe expectedSoapEnvelope()
+        messageCaptor.getValue.messageId shouldBe messageId
+        messageCaptor.getValue.globalId shouldBe expectedGlobalId
+        messageCaptor.getValue.createDateTime shouldBe expectedCreateDateTime
+        messageCaptor.getValue.ccnHttpStatus shouldBe httpCode
+        messageCaptor.getValue.notificationUrl shouldBe messageRequestFullAddressing.notificationUrl
+        messageCaptor.getValue.destinationUrl shouldBe "http://example.com:1234/CCN2.Service.Customs.EU.ICS.RiskAnalysisOrchestrationBAS"
+      }
+    }
+
+    "save the message as RETRYING when the connector returns a 5xx" in new Setup {
+      (500 to 599).foreach { httpCode =>
         when(wsSecurityServiceMock.addUsernameToken(*)).thenReturn(expectedSoapEnvelope())
         when(outboundConnectorMock.postMessage(*)).thenReturn(successful(httpCode))
         val messageCaptor: ArgumentCaptor[OutboundSoapMessage] = ArgumentCaptor.forClass(classOf[OutboundSoapMessage])
@@ -277,16 +295,16 @@ class OutboundServiceSpec extends AnyWordSpec with Matchers with GuiceOneAppPerS
     }
 
     "send the SOAP envelope with empty body returned from the security service to the connector" in new Setup {
-        when(wsSecurityServiceMock.addUsernameToken(*)).thenReturn(expectedSoapEnvelopeWithEmptyBodyRequest())
-        when(outboundConnectorMock.postMessage(*)).thenReturn(successful(200))
-        val messageCaptor: ArgumentCaptor[OutboundSoapMessage] = ArgumentCaptor.forClass(classOf[OutboundSoapMessage])
-        when(outboundMessageRepositoryMock.persist(messageCaptor.capture())).thenReturn(Future(InsertOneResult.acknowledged(BsonNumber(1))))
-        await(underTest.sendMessage(messageRequestEmptyBody))
+      when(wsSecurityServiceMock.addUsernameToken(*)).thenReturn(expectedSoapEnvelopeWithEmptyBodyRequest())
+      when(outboundConnectorMock.postMessage(*)).thenReturn(successful(200))
+      val messageCaptor: ArgumentCaptor[OutboundSoapMessage] = ArgumentCaptor.forClass(classOf[OutboundSoapMessage])
+      when(outboundMessageRepositoryMock.persist(messageCaptor.capture())).thenReturn(Future(InsertOneResult.acknowledged(BsonNumber(1))))
+      await(underTest.sendMessage(messageRequestEmptyBody))
 
-        messageCaptor.getValue.status shouldBe SendingStatus.SENT
-        messageCaptor.getValue.soapMessage shouldBe expectedSoapEnvelopeWithEmptyBodyRequest()
-        messageCaptor.getValue.messageId shouldBe messageId
-        messageCaptor.getValue.globalId shouldBe expectedGlobalId
+      messageCaptor.getValue.status shouldBe SendingStatus.SENT
+      messageCaptor.getValue.soapMessage shouldBe expectedSoapEnvelopeWithEmptyBodyRequest()
+      messageCaptor.getValue.messageId shouldBe messageId
+      messageCaptor.getValue.globalId shouldBe expectedGlobalId
 
     }
 
@@ -369,23 +387,23 @@ class OutboundServiceSpec extends AnyWordSpec with Matchers with GuiceOneAppPerS
       exception.getMessage should include("This file was not found: http://example.com/missing")
     }
 
-  "fail when the addressing.to field is empty" in new Setup {
+    "fail when the addressing.to field is empty" in new Setup {
       when(wsSecurityServiceMock.addUsernameToken(*)).thenReturn(expectedSoapEnvelope())
       when(outboundConnectorMock.postMessage(*)).thenReturn(successful(expectedStatus))
 
       val exception: IllegalArgumentException = intercept[IllegalArgumentException] {
-        await(underTest.sendMessage(messageRequestFullAddressing.copy(addressing= addressing.copy(to = ""))))
+        await(underTest.sendMessage(messageRequestFullAddressing.copy(addressing = addressing.copy(to = ""))))
       }
 
       exception.getMessage should include("addressing.to being empty")
     }
 
-  "fail when the addressing.messageId field is empty" in new Setup {
+    "fail when the addressing.messageId field is empty" in new Setup {
       when(wsSecurityServiceMock.addUsernameToken(*)).thenReturn(expectedSoapEnvelope())
       when(outboundConnectorMock.postMessage(*)).thenReturn(successful(expectedStatus))
 
       val exception: IllegalArgumentException = intercept[IllegalArgumentException] {
-        await(underTest.sendMessage(messageRequestFullAddressing.copy(addressing= addressing.copy(messageId = ""))))
+        await(underTest.sendMessage(messageRequestFullAddressing.copy(addressing = addressing.copy(messageId = ""))))
       }
 
       exception.getMessage should include("addressing.messageId being empty")