diff --git a/app/uk/gov/hmrc/apiplatformoutboundsoap/config/AppConfig.scala b/app/uk/gov/hmrc/apiplatformoutboundsoap/config/AppConfig.scala index 3d449fd..b1d56a9 100644 --- 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 index a7eeeb3..fc1cff3 100644 --- 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 index 69c11f1..b2c66b4 100644 --- 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 index 70cb599..d7fa29c 100644 --- a/test/uk/gov/hmrc/apiplatformoutboundsoap/connectors/OutboundConnectorSpec.scala +++ b/test/uk/gov/hmrc/apiplatformoutboundsoap/connectors/OutboundConnectorSpec.scala @@ -16,17 +16,23 @@ 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() @@ -34,30 +40,51 @@ class OutboundConnectorSpec extends AnyWordSpec with Matchers with GuiceOneAppPe .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("payload") + 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("payload") + 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 index 241e2f5..b772efc 100644 --- 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 index ae16d6e..c7a9279 100644 --- 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")