From b1c843cf1061db41f36fa496adfa1cb664f08aa2 Mon Sep 17 00:00:00 2001
From: Neil Frow <675806+worthydolt@users.noreply.github.com>
Date: Mon, 2 Aug 2021 13:53:49 +0100
Subject: [PATCH] =?UTF-8?q?APID-237:=20Rename=20variables.=20Fail=20immedi?=
=?UTF-8?q?ately=20on=203xx=20and=204xx=20responses=E2=80=A6=20(#55)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
* APID-237: Rename variables. Fail immediately on 3xx and 4xx responses during retrying
* APID-237: refactoring to remove duplication and break down methods into more manageable parts
* APID-237: Reordering code
* APID-237: updated README with description of retrying behaviour
---
README.md | 12 +-
.../apiplatformoutboundsoap/responses.scala | 8 ++
.../services/OutboundService.scala | 133 +++++++++++++-----
.../services/OutboundServiceSpec.scala | 54 +++++++
4 files changed, 164 insertions(+), 43 deletions(-)
diff --git a/README.md b/README.md
index f8077db..dcad2c0 100644
--- a/README.md
+++ b/README.md
@@ -3,11 +3,13 @@
This service allows other HMRC services to send messages to external SOAP web services. It has a retry mechanism whereby if the
CCN2 SOAP service doesn't return a 2xx response, the request will be retried every 60 seconds for 5 minutes by default assuming that the `retry.enabled` property in `application.conf` is set to `true`. The total duration and the interval are both configurable.
The retry logic works as follows:
-Initial request to CCN2 results in a 2xx response -> message goes into a sent state.
-Initial request to CCN2 results in a non 2xx response -> message goes into a retrying state.
-Subsequent request to CCN2 results in a non 2xx response and inside retry duration -> message retried.
-Subsequent request to CCN2 results in a 2xx response and inside retry duration -> message goes into a sent state.
-Subsequent request to CCN2 results in a non 2xx response and outside retry duration -> message goes into failed state.
+ * Initial request to CCN2 results in a 2xx response -> message goes into a sent state.
+ * Initial request to CCN2 results in a 5xx response -> message goes into a retrying state.
+ * Initial request to CCN2 results in a 1xx, 3xx or 4xx response -> message goes into failed state.
+ * Subsequent request to CCN2 results in a 1xx, 3xx or 4xx response and inside retry duration -> message goes into failed state.
+ * Subsequent request to CCN2 results in a 5xx response and inside retry duration -> message retried.
+ * Subsequent request to CCN2 results in a 2xx response and inside retry duration -> message goes into a sent state.
+ * Subsequent request to CCN2 results in a non 2xx response and outside retry duration -> message goes into failed state.
## `POST /message`
Send a SOAP message for the given operation
diff --git a/app/uk/gov/hmrc/apiplatformoutboundsoap/responses.scala b/app/uk/gov/hmrc/apiplatformoutboundsoap/responses.scala
index 9122fa1..bcef6f6 100644
--- a/app/uk/gov/hmrc/apiplatformoutboundsoap/responses.scala
+++ b/app/uk/gov/hmrc/apiplatformoutboundsoap/responses.scala
@@ -26,6 +26,14 @@ object ErrorCode extends Enumeration {
val INTERNAL_SERVER_ERROR = Value("INTERNAL_SERVER_ERROR")
}
+object CcnRequestResult extends Enumeration {
+ type CcnRequestResult = Value
+ val UNEXPECTED_SUCCESS = Value("UNEXPECTED_SUCCESS")
+ val SUCCESS = Value("SUCCESS")
+ val FAIL_ERROR = Value("FAIL_ERROR")
+ val RETRYABLE_ERROR = Value("RETRYABLE_ERROR")
+}
+
object JsErrorResponse {
def apply(errorCode: ErrorCode.Value, message: JsValueWrapper): JsObject =
Json.obj(
diff --git a/app/uk/gov/hmrc/apiplatformoutboundsoap/services/OutboundService.scala b/app/uk/gov/hmrc/apiplatformoutboundsoap/services/OutboundService.scala
index 6ee0193..05fb637 100644
--- a/app/uk/gov/hmrc/apiplatformoutboundsoap/services/OutboundService.scala
+++ b/app/uk/gov/hmrc/apiplatformoutboundsoap/services/OutboundService.scala
@@ -26,12 +26,14 @@ import org.apache.axiom.soap.SOAPEnvelope
import org.apache.axis2.addressing.AddressingConstants.Final.{WSAW_NAMESPACE, WSA_NAMESPACE}
import org.apache.axis2.addressing.AddressingConstants._
import org.apache.axis2.wsdl.WSDLUtil
-import org.apache.http.HttpStatus
import org.joda.time.DateTime
import org.joda.time.DateTimeZone.UTC
import org.joda.time.format.{DateTimeFormatter, ISODateTimeFormat}
import play.api.cache.AsyncCacheApi
import play.api.Logging
+import play.api.http.Status._
+import uk.gov.hmrc.apiplatformoutboundsoap.CcnRequestResult
+import uk.gov.hmrc.apiplatformoutboundsoap.CcnRequestResult._
import uk.gov.hmrc.apiplatformoutboundsoap.config.AppConfig
import uk.gov.hmrc.apiplatformoutboundsoap.connectors.{NotificationCallbackConnector, OutboundConnector}
import uk.gov.hmrc.apiplatformoutboundsoap.models._
@@ -65,8 +67,9 @@ class OutboundService @Inject()(outboundConnector: OutboundConnector,
def sendMessage(message: MessageRequest): Future[OutboundSoapMessage] = {
for {
soapRequest <- buildSoapRequest(message)
- result <- outboundConnector.postMessage(message.addressing.messageId, soapRequest)
- outboundSoapMessage = buildOutboundSoapMessage(message, soapRequest, result)
+ httpStatus <- outboundConnector.postMessage(message.addressing.messageId, soapRequest)
+ outboundSoapMessage = processSendingResult(message, soapRequest, httpStatus)
+ _ <- logCcnSendResult(outboundSoapMessage, httpStatus)
_ <- outboundMessageRepository.persist(outboundSoapMessage)
} yield outboundSoapMessage
}
@@ -79,45 +82,63 @@ class OutboundService @Inject()(outboundConnector: OutboundConnector,
val nextRetryDateTime: DateTime = now.plus(appConfig.retryInterval.toMillis)
val globalId = message.globalId
val messageId = message.messageId
+ def updateStatusAndNotify(newStatus: SendingStatus)(implicit hc: HeaderCarrier) = {
+ outboundMessageRepository.updateSendingStatus(globalId, newStatus) map { updatedMessage =>
+ updatedMessage.map(notificationCallbackConnector.sendNotification)
+ ()
+ }
+ }
+
+ def retryDurationExpired = {
+ message.createDateTime.plus(appConfig.retryDuration.toMillis).isBefore(now.getMillis)
+ }
- outboundConnector.postMessage(messageId, SoapRequest(message.soapMessage, message.destinationUrl)) flatMap { result =>
- if (is2xx(result)) {
- log2xxResult(result, globalId, messageId)
- outboundMessageRepository.updateSendingStatus(message.globalId, SendingStatus.SENT) map { updatedMessage =>
- updatedMessage.map(notificationCallbackConnector.sendNotification)
- ()
- }
- } else {
- if (message.createDateTime.plus(appConfig.retryDuration.toMillis).isBefore(now.getMillis)) {
- logger.error(s"Retried message with global ID ${message.globalId} message ID ${message.messageId} got status code $result " +
- s"and failed on last attempt")
- outboundMessageRepository.updateSendingStatus(message.globalId, SendingStatus.FAILED).map { updatedMessage =>
- updatedMessage.map(notificationCallbackConnector.sendNotification)
- ()
+ outboundConnector.postMessage(messageId, SoapRequest(message.soapMessage, message.destinationUrl)) flatMap { httpStatus =>
+ mapHttpStatusCode(httpStatus) match {
+ case SUCCESS =>
+ logSuccess(httpStatus, globalId, messageId)
+ updateStatusAndNotify(SendingStatus.SENT)
+ case UNEXPECTED_SUCCESS =>
+ logSuccess(httpStatus, globalId, messageId)
+ updateStatusAndNotify(SendingStatus.SENT)
+ case FAIL_ERROR =>
+ logSendingFailure(httpStatus, message.globalId, message.messageId)
+ updateStatusAndNotify(SendingStatus.FAILED)
+ case RETRYABLE_ERROR =>
+ if (retryDurationExpired) {
+ logRetryingTimedOut(httpStatus, globalId, messageId)
+ updateStatusAndNotify(SendingStatus.FAILED)
+ } else {
+ logContinuingRetrying(httpStatus, globalId, messageId)
+ outboundMessageRepository.updateNextRetryTime(message.globalId, nextRetryDateTime).map(_ => ())
}
- } else {
- logger.warn(s"Retried message with global ID ${message.globalId} message ID ${message.messageId} got status code $result and will retry")
- outboundMessageRepository.updateNextRetryTime(message.globalId, nextRetryDateTime).map(_ => ())
- }
+ case _ => Future.unit
}
}
}
- private def buildOutboundSoapMessage(message: MessageRequest, soapRequest: SoapRequest, result: Int): OutboundSoapMessage = {
+ private def processSendingResult(message: MessageRequest, soapRequest: SoapRequest, httpStatus: Int): OutboundSoapMessage = {
val globalId: UUID = randomUUID
val messageId = message.addressing.messageId
- def is3xx(result: Int): Boolean = result >= 300 && result < 400
-
- if (is2xx(result)) {
- log2xxResult(result, globalId, messageId)
- SentOutboundSoapMessage(globalId, messageId, soapRequest.soapEnvelope, soapRequest.destinationUrl, now, result, message.notificationUrl)
- } else if (is3xx(result)|| is4xx(result)) {
- logger.error(s"Message with global ID $globalId message ID $messageId got status code $result and failed")
- FailedOutboundSoapMessage(globalId, messageId, soapRequest.soapEnvelope, soapRequest.destinationUrl, now, result, message.notificationUrl)
- } else {
- logger.warn(s"Message with global ID $globalId message ID $messageId got status code $result and will retry")
+
+ def succeededMessage = {
+ SentOutboundSoapMessage(globalId, messageId, soapRequest.soapEnvelope, soapRequest.destinationUrl, now, httpStatus, message.notificationUrl)
+ }
+
+ def failedMessage = {
+ FailedOutboundSoapMessage(globalId, messageId, soapRequest.soapEnvelope, soapRequest.destinationUrl, now, httpStatus, message.notificationUrl)
+ }
+
+ def retryingMessage = {
RetryingOutboundSoapMessage(globalId, messageId, soapRequest.soapEnvelope, soapRequest.destinationUrl, now,
- now.plus(appConfig.retryInterval.toMillis), result, message.notificationUrl, None, None)
+ now.plus(appConfig.retryInterval.toMillis), httpStatus, message.notificationUrl, None, None)
+ }
+
+ mapHttpStatusCode(httpStatus) match {
+ case SUCCESS => succeededMessage
+ case UNEXPECTED_SUCCESS => succeededMessage
+ case FAIL_ERROR => failedMessage
+ case RETRYABLE_ERROR => retryingMessage
}
}
@@ -217,12 +238,48 @@ class OutboundService @Inject()(outboundConnector: OutboundConnector,
envelope.getBody.addChild(payload)
}
-
- private def log2xxResult(result: Int, globalId: UUID, messageId: String) = {
- if (result != HttpStatus.SC_ACCEPTED) {
- logger.warn(s"Message with global ID $globalId message ID $messageId got status code $result and successfully sent")
+ private def mapHttpStatusCode(httpStatusCode: Int): CcnRequestResult.Value = {
+ if (isSuccessful(httpStatusCode)){
+ httpStatusCode match {
+ case ACCEPTED => SUCCESS
+ case _ => UNEXPECTED_SUCCESS
+ }
+ } else if (isInformational(httpStatusCode) || isRedirect(httpStatusCode) || isClientError(httpStatusCode)) {
+ FAIL_ERROR
} else {
- logger.info(s"Message with global ID $globalId message ID $messageId got status code $result and successfully sent")
+ RETRYABLE_ERROR
+ }
+ }
+
+ private def logSendingFailure(httpStatus: Int, globalId: UUID, messageId: String): Unit = {
+ logger.error(s"Message with global ID $globalId message ID and $messageId, got status code $httpStatus and failed")
+ }
+
+ private def logRetryingTimedOut(httpStatus: Int, globalId: UUID, messageId: String): Unit = {
+ logger.error(s"Retried message with global ID $globalId and message ID $messageId, got status code $httpStatus and failed on last attempt")
+ }
+
+ private def logContinuingRetrying(httpStatus: Int, globalId: UUID, messageId: String): Unit = {
+ logger.warn(s"Retried message with global ID $globalId and message ID $messageId, got status code $httpStatus and will retry")
+ }
+
+ private def logSuccess(httpStatus: Int, globalId: UUID, messageId: String): Unit = {
+ val successLogMsg: String = s"Message with global ID $globalId and message ID $messageId got status code $httpStatus and successfully sent"
+ httpStatus match {
+ case ACCEPTED => logger.info(successLogMsg)
+ case _ => logger.warn(successLogMsg)
+ }
+ }
+
+ private def logCcnSendResult(osm: OutboundSoapMessage, httpStatus: Int): Future[Unit] = {
+ val globalId = osm.globalId
+ val messageId = osm.messageId
+ mapHttpStatusCode(httpStatus) match {
+ case SUCCESS => logSuccess(httpStatus, globalId, messageId)
+ case UNEXPECTED_SUCCESS => logSuccess(httpStatus, globalId, messageId)
+ case FAIL_ERROR => logSendingFailure(httpStatus, globalId, messageId)
+ case RETRYABLE_ERROR => logContinuingRetrying(httpStatus, globalId, messageId)
}
+ Future.unit
}
}
diff --git a/test/uk/gov/hmrc/apiplatformoutboundsoap/services/OutboundServiceSpec.scala b/test/uk/gov/hmrc/apiplatformoutboundsoap/services/OutboundServiceSpec.scala
index 294ef49..70a1e9c 100644
--- a/test/uk/gov/hmrc/apiplatformoutboundsoap/services/OutboundServiceSpec.scala
+++ b/test/uk/gov/hmrc/apiplatformoutboundsoap/services/OutboundServiceSpec.scala
@@ -492,6 +492,60 @@ class OutboundServiceSpec extends AnyWordSpec with Matchers with GuiceOneAppPerS
verify(outboundMessageRepositoryMock).updateNextRetryTime(retryingMessage.globalId, expectedCreateDateTime.plus(appConfigMock.retryInterval.toMillis))
}
+ "retry a message and mark failed when SOAP request received 1xx status" in new Setup {
+ val retryingMessage = RetryingOutboundSoapMessage(randomUUID, "MessageId-A1", "payload",
+ "some url", DateTime.now(UTC), DateTime.now(UTC), httpStatus)
+ when(appConfigMock.parallelism).thenReturn(2)
+ when(appConfigMock.retryDuration).thenReturn(Duration("5s"))
+ when(appConfigMock.retryInterval).thenReturn(Duration("5s"))
+ when(outboundConnectorMock.postMessage(*, *)).thenReturn(successful(CONTINUE))
+ when(outboundMessageRepositoryMock.retrieveMessagesForRetry).
+ thenReturn(fromIterator(() => Seq(retryingMessage).toIterator))
+ when(outboundMessageRepositoryMock.updateSendingStatus(*, *)).thenReturn(successful(None))
+ await(underTest.retryMessages)
+
+ verify(outboundMessageRepositoryMock, never).updateNextRetryTime(*, *)
+ verify(outboundMessageRepositoryMock, never).updateSendingStatus(retryingMessage.globalId, SendingStatus.SENT)
+ verify(outboundMessageRepositoryMock, never).updateSendingStatus(retryingMessage.globalId, SendingStatus.RETRYING)
+ verify(outboundMessageRepositoryMock).updateSendingStatus(retryingMessage.globalId, SendingStatus.FAILED)
+ }
+
+ "retry a message and mark failed when SOAP request received 3xx status" in new Setup {
+ val retryingMessage = RetryingOutboundSoapMessage(randomUUID, "MessageId-A1", "payload",
+ "some url", DateTime.now(UTC), DateTime.now(UTC), httpStatus)
+ when(appConfigMock.parallelism).thenReturn(2)
+ when(appConfigMock.retryDuration).thenReturn(Duration("5s"))
+ when(appConfigMock.retryInterval).thenReturn(Duration("5s"))
+ when(outboundConnectorMock.postMessage(*, *)).thenReturn(successful(TEMPORARY_REDIRECT))
+ when(outboundMessageRepositoryMock.retrieveMessagesForRetry).
+ thenReturn(fromIterator(() => Seq(retryingMessage).toIterator))
+ when(outboundMessageRepositoryMock.updateSendingStatus(*, *)).thenReturn(successful(None))
+ await(underTest.retryMessages)
+
+ verify(outboundMessageRepositoryMock, never).updateNextRetryTime(*, *)
+ verify(outboundMessageRepositoryMock, never).updateSendingStatus(retryingMessage.globalId, SendingStatus.SENT)
+ verify(outboundMessageRepositoryMock, never).updateSendingStatus(retryingMessage.globalId, SendingStatus.RETRYING)
+ verify(outboundMessageRepositoryMock).updateSendingStatus(retryingMessage.globalId, SendingStatus.FAILED)
+ }
+
+ "retry a message and mark failed when SOAP request received 4xx status" in new Setup {
+ val retryingMessage = RetryingOutboundSoapMessage(randomUUID, "MessageId-A1", "payload",
+ "some url", DateTime.now(UTC), DateTime.now(UTC), httpStatus)
+ when(appConfigMock.parallelism).thenReturn(2)
+ when(appConfigMock.retryDuration).thenReturn(Duration("5s"))
+ when(appConfigMock.retryInterval).thenReturn(Duration("5s"))
+ when(outboundConnectorMock.postMessage(*, *)).thenReturn(successful(NOT_FOUND))
+ when(outboundMessageRepositoryMock.retrieveMessagesForRetry).
+ thenReturn(fromIterator(() => Seq(retryingMessage).toIterator))
+ when(outboundMessageRepositoryMock.updateSendingStatus(*, *)).thenReturn(successful(None))
+ await(underTest.retryMessages)
+
+ verify(outboundMessageRepositoryMock, never).updateNextRetryTime(*, *)
+ verify(outboundMessageRepositoryMock, never).updateSendingStatus(retryingMessage.globalId, SendingStatus.SENT)
+ verify(outboundMessageRepositoryMock, never).updateSendingStatus(retryingMessage.globalId, SendingStatus.RETRYING)
+ verify(outboundMessageRepositoryMock).updateSendingStatus(retryingMessage.globalId, SendingStatus.FAILED)
+ }
+
"set a message's status to FAILED when its retryDuration has expired" in new Setup {
val retryDuration: Duration = Duration("30s")
val retryingMessage = RetryingOutboundSoapMessage(randomUUID, "MessageId-A1", "payload",