APIS-7008 - Make responses contain errors and log errors (#110)

* APIS-7008 - Make responses contain errors and log errors

* APIS-7008 - PR Bot comments

app/uk/gov/hmrc/apipublisher/controllers/PublisherController.scala

@@ -19,6 +19,7 @@ package uk.gov.hmrc.apipublisher.controllers
 import java.nio.charset.StandardCharsets
 import java.util.Base64
 import javax.inject.{Inject, Singleton}
+import scala.concurrent.Future.successful
 import scala.concurrent.{ExecutionContext, Future}
 
 import org.everit.json.schema.ValidationException
@@ -124,6 +125,10 @@ class PublisherController @Inject() (
         publisherResponse     <- E.liftF(publishApiAndScopes(apiAndScopes))
       } yield publisherResponse
     )
+      .leftSemiflatTap { err: PublishError =>
+        logger.error(s"Failed to publish api and scopes due to ${err.message}")
+        successful(err) // Thrown away
+      }
       .leftMap(mapBusinessErrorsToResults)
       .merge
       .recover(recovery(FAILED_TO_PUBLISH))
@@ -136,10 +141,14 @@ class PublisherController @Inject() (
         apiAndScopes <- ER.fromEither(validateRequestPayload[ApiAndScopes])
         validation   <- ER.fromEitherF(
                           publisherService.validation(apiAndScopes, validateApiDefinition = true)
-                            .map(_.toRight(apiAndScopes).map(x => BadRequest(Json.toJson(x)))
-                              .swap)
+                            .map(_.toRight(NoContent))
                         )
-      } yield NoContent
+                          .swap
+                          .leftMap { jsv =>
+                            logger.error(s"Failed to publish api and scopes due to ${jsv.toString}")
+                            BadRequest(Json.toJson(jsv))
+                          }
+      } yield validation
     )
       .merge
       .recover(recovery(FAILED_TO_VALIDATE))

app/uk/gov/hmrc/apipublisher/services/DefinitionService.scala

@@ -19,6 +19,7 @@ package uk.gov.hmrc.apipublisher.services
 import javax.inject.Inject
 import scala.concurrent.Future.successful
 import scala.concurrent.{ExecutionContext, Future}
+import scala.util.control.NonFatal
 
 import cats.implicits._
 
@@ -88,18 +89,32 @@ class DefinitionService @Inject() (
     val ramlVD = ramlVersionDefinitionService.getDetailForVersion(serviceLocation, context, version)
       .orElse(successful(List.empty))
 
-    val oasVD = oasVersionDefinitionService.getDetailForVersion(serviceLocation, context, version)
-      .orElse(successful(List.empty))
+    val oasVD: Future[Either[Throwable, List[Endpoint]]] =
+      oasVersionDefinitionService.getDetailForVersion(serviceLocation, context, version)
+        .map(_.asRight[Throwable])
+        .recover {
+          case NonFatal(t) => Left(t)
+        }
 
     ramlVD.flatMap { raml =>
       oasVD.map { oas =>
         (raml, oas) match {
-          case (Nil, Nil)                                                                   => throw new IllegalStateException(s"No endpoints defined for $version of ${serviceLocation.serviceName}")
-          case (ramlEndpoints, Nil)                                                         => logger.info(s"${describeService} : Using RAML to publish"); (ramlEndpoints, ApiVersionSource.RAML)
-          case (Nil, oasEndpoints)                                                          => logger.info(s"${describeService} : Using OAS to publish"); (oasEndpoints, ApiVersionSource.OAS)
-          case (ramlEndpoints, oasEndpoints) if (ramlEndpoints.toSet == oasEndpoints.toSet) =>
-            logger.info(s"${describeService} : Both RAML and OAS match for publishing"); (oasEndpoints, ApiVersionSource.OAS)
-          case (ramlEndpoints, oasEndpoints)                                                => logger.warn(s"${describeService} : Mismatched RAML <$ramlEndpoints>  OAS <$oasEndpoints>"); (ramlEndpoints, ApiVersionSource.RAML)
+          case (Nil, Right(Nil))                                                                   =>
+            throw new IllegalStateException(s"No endpoints defined for $version of ${serviceLocation.serviceName}")
+          case (Nil, Left(t))                                                                      =>
+            throw new IllegalStateException(s"No endpoints defined for $version of ${serviceLocation.serviceName} due to failure in OAS Parsing - [${t.getMessage()}]")
+          case (ramlEndpoints, Right(Nil))                                                         =>
+            logger.info(s"${describeService} : Using RAML to publish")
+            (ramlEndpoints, ApiVersionSource.RAML)
+          case (ramlEndpoints, Left(t))                                                            =>
+            logger.info(s"${describeService} : Using RAML to publish due to failure in OAS Parsing - [${t.getMessage()}]")
+            (ramlEndpoints, ApiVersionSource.RAML)
+          case (Nil, Right(oasEndpoints))                                                          => logger.info(s"${describeService} : Using OAS to publish"); (oasEndpoints, ApiVersionSource.OAS)
+          case (ramlEndpoints, Right(oasEndpoints)) if (ramlEndpoints.toSet == oasEndpoints.toSet) =>
+            logger.info(s"${describeService} : Both RAML and OAS match for publishing")
+            (oasEndpoints, ApiVersionSource.OAS)
+          case (ramlEndpoints, Right(oasEndpoints))                                                =>
+            logger.warn(s"${describeService} : Mismatched RAML <$ramlEndpoints>  OAS <$oasEndpoints>"); (ramlEndpoints, ApiVersionSource.RAML)
         }
       }
     }

conf/logback.xml

@@ -14,19 +14,6 @@
         </encoder>
     </appender>
 
-    <appender name="STDOUT_IGNORE_NETTY" class="ch.qos.logback.core.ConsoleAppender">
-        <encoder class="ch.qos.logback.classic.encoder.PatternLayoutEncoder">
-            <pattern>%date{ISO8601} level=[%level] logger=[%logger] thread=[%thread] rid=[not-available] user=[not-available] message=[%message] %replace(exception=[%xException]){'^exception=\[\]$',''}%n</pattern>
-        </encoder>
-    </appender>
-
-    <appender name="ACCESS_LOG_FILE" class="ch.qos.logback.core.FileAppender">
-        <file>logs/access.log</file>
-        <encoder>
-            <pattern>%message%n</pattern>
-        </encoder>
-    </appender>
-
     <appender name="CONNECTOR_LOG_FILE" class="ch.qos.logback.core.FileAppender">
         <file>logs/connector.log</file>
         <encoder>
@@ -35,14 +22,6 @@
     </appender>
 
 
-    <logger name="accesslog" level="INFO" additivity="false">
-        <appender-ref ref="ACCESS_LOG_FILE" />
-    </logger>
-
-    <logger name="com.ning.http.client.providers.netty" additivity="false">
-        <appender-ref ref="STDOUT_IGNORE_NETTY" />
-    </logger>
-
     <logger name="uk.gov" level="INFO"/>
 
     <logger name="com.google.inject" level="WARN"/>

project/build.properties

@@ -1 +1 @@
-sbt.version=1.9.7
+sbt.version=1.9.9

project/plugins.sbt

@@ -1,7 +1,7 @@
 resolvers += "HMRC-open-artefacts-maven-2" at "https://open.artefacts.tax.service.gov.uk/maven2"
 resolvers += Resolver.url("HMRC-open-artefacts-ivy", url("https://open.artefacts.tax.service.gov.uk/ivy2"))(Resolver.ivyStylePatterns)
 
-addSbtPlugin("uk.gov.hmrc"        %  "sbt-auto-build"         % "3.20.0")
+addSbtPlugin("uk.gov.hmrc"        %  "sbt-auto-build"         % "3.22.0")
 addSbtPlugin("uk.gov.hmrc"        %  "sbt-distributables"     % "2.5.0")
 addSbtPlugin("org.playframework"  %  "sbt-plugin"             % "3.0.1")
 addSbtPlugin("org.scoverage"      %  "sbt-scoverage"          % "2.0.9")

test/uk/gov/hmrc/apipublisher/services/DefinitionServiceSpec.scala

@@ -17,7 +17,7 @@
 package uk.gov.hmrc.apipublisher.services
 
 import scala.concurrent.ExecutionContext.Implicits.global
-import scala.concurrent.Future.successful
+import scala.concurrent.Future.{failed, successful}
 
 import org.mockito.{ArgumentMatchersSugar, MockitoSugar}
 import utils.AsyncHmrcSpec
@@ -66,6 +66,10 @@ class DefinitionServiceSpec extends AsyncHmrcSpec {
       primeRamlFor(version)
       primeOasFor(version, endpoints: _*)
     }
+
+    def primeOasFailure(version: String, throwable: Throwable) = {
+      when(oasVDS.getDetailForVersion(*, *, eqTo(version))).thenReturn(failed(throwable))
+    }
   }
 
   "getDefinition" should {
@@ -86,7 +90,21 @@ class DefinitionServiceSpec extends AsyncHmrcSpec {
       intercept[IllegalStateException] {
         await(service.getDefinition(aServiceLocation))
       }
-        .getMessage startsWith "No endpoints defined for"
+        .getMessage shouldBe "No endpoints defined for 1.0 of test"
+    }
+
+    "handle api and scopes with bad OAS" in new Setup {
+      val api    = json[JsObject]("/input/api_no_endpoints_one_version.json")
+      val scopes = json[JsArray]("/input/scopes.json")
+      MicroserviceConnectorMock.GetAPIAndScopes.returns(ApiAndScopes(api, scopes))
+
+      primeRamlFor("1.0")
+      primeOasFailure("1.0", new RuntimeException("Boom"))
+
+      intercept[IllegalStateException] {
+        await(service.getDefinition(aServiceLocation))
+      }
+        .getMessage startsWith "No endpoints defined for 1.0 of test due to failure in OAS Parsing"
     }
 
     "handle api and scopes with RAML data only" in new Setup {
@@ -115,6 +133,20 @@ class DefinitionServiceSpec extends AsyncHmrcSpec {
       result.value shouldBe ApiAndScopes(expected, scopes)
     }
 
+    "handle api and scopes with bad OAS data only" in new Setup {
+      val api    = json[JsObject]("/input/api_no_endpoints_one_version.json")
+      val scopes = json[JsArray]("/input/scopes.json")
+      MicroserviceConnectorMock.GetAPIAndScopes.returns(ApiAndScopes(api, scopes))
+
+      primeRamlFor("1.0")
+      primeOasFailure("1.0", new RuntimeException("Boom"))
+
+      intercept[IllegalStateException] {
+        await(service.getDefinition(aServiceLocation))
+      }
+        .getMessage startsWith "No endpoints defined for 1.0 of test due to failure in OAS Parsing"
+    }
+
     "handle api and scopes with both RAML and OS data that matches" in new Setup {
       val api    = json[JsObject]("/input/api_no_endpoints_one_version.json")
       val scopes = json[JsArray]("/input/scopes.json")