Return catalogue error responses on non-works pages

catalogue_api/api/src/main/scala/uk/ac/wellcome/platform/api/ContextHelper.scala

@@ -4,6 +4,7 @@ import uk.ac.wellcome.display.models.ApiVersions
 import uk.ac.wellcome.platform.api.models.ApiConfig
 
 object ContextHelper {
-  def buildContextUri(apiConfig: ApiConfig, version: ApiVersions.Value) =
+  def buildContextUri(apiConfig: ApiConfig,
+                      version: ApiVersions.Value = ApiVersions.default) =
     s"${apiConfig.scheme}://${apiConfig.host}${apiConfig.pathPrefix}/$version${apiConfig.contextSuffix}"
 }

catalogue_api/api/src/main/scala/uk/ac/wellcome/platform/api/Server.scala

@@ -45,6 +45,7 @@ class Server extends HttpServer {
       .add[DocsController]
       .add[V1WorksController]
       .add[V2WorksController]
+      .add[MissingPathController]
       .exceptionMapper[GeneralExceptionMapper]
       .exceptionMapper[CaseClassMappingExceptionWrapper]
       .exceptionMapper[ElasticsearchResponseExceptionMapper]

catalogue_api/api/src/main/scala/uk/ac/wellcome/platform/api/controllers/MissingPathController.scala

@@ -0,0 +1,35 @@
+package uk.ac.wellcome.platform.api.controllers
+
+import com.google.inject.{Inject, Singleton}
+import com.twitter.finagle.http.Request
+import com.twitter.finatra.http.Controller
+import uk.ac.wellcome.platform.api.ContextHelper.buildContextUri
+import uk.ac.wellcome.platform.api.models.{ApiConfig, DisplayError, Error}
+import uk.ac.wellcome.platform.api.responses.ResultResponse
+
+/** This controller returns a 404 to any requests for an undefined path.
+  *
+  * Note: it does this by defining a wildcard route for all paths.
+  * Since Finatra resolves routes in the order they're declared, this
+  * controller must be used last, or it will break other routes.
+  *
+  * More info:
+  *   https://twitter.github.io/finatra/user-guide/http/controllers.html#wildcard-parameter
+  *   https://alexwlchan.net/2018/10/finatra-404/
+  *
+  */
+@Singleton
+class MissingPathController @Inject()(apiConfig: ApiConfig) extends Controller {
+  val contextUri: String = buildContextUri(apiConfig = apiConfig)
+
+  get("/:*") { request: Request =>
+    val result = Error(
+      variant = "http-404",
+      description = Some(s"Page not found for URL ${request.uri}")
+    )
+
+    response.notFound.json(
+      ResultResponse(context = contextUri, result = DisplayError(result))
+    )
+  }
+}

catalogue_api/api/src/main/scala/uk/ac/wellcome/platform/api/models/ApiConfig.scala

@@ -4,7 +4,6 @@ case class ApiConfig(
   host: String,
   scheme: String,
   defaultPageSize: Int,
-  name: String,
   pathPrefix: String,
   contextSuffix: String
 )

catalogue_api/api/src/main/scala/uk/ac/wellcome/platform/api/modules/ApiConfigModule.scala

@@ -31,7 +31,6 @@ object ApiConfigModule extends TwitterModule {
       host = host(),
       scheme = scheme(),
       defaultPageSize = defaultPageSize(),
-      name = apiName(),
       pathPrefix = pathPrefix(),
       contextSuffix = contextSuffix()
     )

catalogue_api/api/src/main/scala/uk/ac/wellcome/platform/api/services/ElasticsearchService.scala

@@ -20,7 +20,7 @@ class ElasticsearchService @Inject()(elasticClient: HttpClient,
                      indexName: String): Future[GetResponse] =
     elasticClient
       .execute {
-        get(canonicalId).from(s"${indexName}/$documentType")
+        get(canonicalId).from(s"$indexName/$documentType")
       }
 
   def listResults(sortByField: String,
@@ -29,7 +29,7 @@ class ElasticsearchService @Inject()(elasticClient: HttpClient,
                   from: Int = 0): Future[SearchResponse] =
     elasticClient
       .execute {
-        search(s"${indexName}/$documentType")
+        search(s"$indexName/$documentType")
           .query(termQuery("type", "IdentifiedWork"))
           .sortBy(fieldSort(sortByField))
           .limit(limit)
@@ -42,7 +42,7 @@ class ElasticsearchService @Inject()(elasticClient: HttpClient,
                                indexName: String): Future[SearchResponse] =
     elasticClient
       .execute {
-        search(s"${indexName}/$documentType")
+        search(s"$indexName/$documentType")
           .query(
             must(
               simpleStringQuery(queryString),

catalogue_api/api/src/test/scala/uk/ac/wellcome/platform/api/fixtures/WorksServiceFixture.scala

@@ -15,7 +15,6 @@ trait WorksServiceFixture { this: Suite =>
         host = "example.org",
         scheme = "https",
         defaultPageSize = 10,
-        name = "testing",
         pathPrefix = "/catalogue/works",
         contextSuffix = "/conext.json"
       ),

catalogue_api/api/src/test/scala/uk/ac/wellcome/platform/api/works/ApiContextTest.scala

@@ -4,12 +4,8 @@ import com.twitter.finagle.http.Status
 import com.twitter.finatra.http.EmbeddedHttpServer
 import org.apache.commons.io.IOUtils
 import uk.ac.wellcome.display.models.ApiVersions
-import uk.ac.wellcome.display.models.v1.DisplayV1SerialisationTestBase
-
-class ApiContextTest
-    extends ApiWorksTestBase
-    with DisplayV1SerialisationTestBase {
 
+class ApiContextTest extends ApiWorksTestBase {
   it("returns a context for all versions") {
     ApiVersions.values.toList.foreach { version: ApiVersions.Value =>
       withApiFixtures(apiVersion = version) {
@@ -22,5 +18,4 @@ class ApiContextTest
       }
     }
   }
-
 }

catalogue_api/api/src/test/scala/uk/ac/wellcome/platform/api/works/ApiErrorsTest.scala

@@ -0,0 +1,38 @@
+package uk.ac.wellcome.platform.api.works
+
+import com.twitter.finagle.http.Status
+import com.twitter.finatra.http.EmbeddedHttpServer
+import uk.ac.wellcome.display.models.ApiVersions
+import uk.ac.wellcome.test.fixtures.TestWith
+
+class ApiErrorsTest extends ApiWorksTestBase {
+
+  it("returns a Not Found error if you try to get an API version") {
+    withServer { server =>
+      server.httpGet(
+        path = "/catalogue/v567/works",
+        andExpect = Status.NotFound,
+        withJsonBody = notFound(
+          s"catalogue/${ApiVersions.default.toString}",
+          "Page not found for URL /catalogue/v567/works")
+      )
+    }
+  }
+
+  it("returns a Not Found error if you try to get an unrecognised path") {
+    withServer { server =>
+      server.httpGet(
+        path = "/foo/bar",
+        andExpect = Status.NotFound,
+        withJsonBody = notFound(
+          s"catalogue/${ApiVersions.default.toString}",
+          "Page not found for URL /foo/bar")
+      )
+    }
+  }
+
+  private def withServer[R](testWith: TestWith[EmbeddedHttpServer, R]): R =
+    withServer(indexNameV1 = "index-v1", indexNameV2 = "index-v2") { server =>
+      testWith(server)
+    }
+}

catalogue_api/api/src/test/scala/uk/ac/wellcome/platform/api/works/ApiWorksTestBase.scala

@@ -3,7 +3,7 @@ package uk.ac.wellcome.platform.api.works
 import com.sksamuel.elastic4s.Indexable
 import com.twitter.finatra.http.EmbeddedHttpServer
 import org.scalatest.FunSpec
-import uk.ac.wellcome.display.models.{ApiVersions, DisplaySerialisationTestBase}
+import uk.ac.wellcome.display.models.ApiVersions
 import uk.ac.wellcome.elasticsearch.test.fixtures.ElasticsearchFixtures
 import uk.ac.wellcome.json.JsonUtil._
 import uk.ac.wellcome.models.work.internal.IdentifiedWork
@@ -14,7 +14,6 @@ import uk.ac.wellcome.test.fixtures.TestWith
 trait ApiWorksTestBase
     extends FunSpec
     with ElasticsearchFixtures
-    with DisplaySerialisationTestBase
     with WorksGenerators {
 
   implicit object IdentifiedWorkIndexable extends Indexable[IdentifiedWork] {

catalogue_api/api/src/test/scala/uk/ac/wellcome/platform/api/works/v1/ApiV1ErrorsTest.scala

@@ -2,7 +2,6 @@ package uk.ac.wellcome.platform.api.works.v1
 
 import com.twitter.finagle.http.Status
 import com.twitter.finatra.http.EmbeddedHttpServer
-import uk.ac.wellcome.display.models.ApiVersions
 
 class ApiV1ErrorsTest extends ApiV1WorksTestBase {
 
@@ -276,19 +275,4 @@ class ApiV1ErrorsTest extends ApiV1WorksTestBase {
 
     }
   }
-
-  // TODO figure out what the correct behaviour should be in this case
-  ignore(
-    "returns a Not Found error if you try to get a version that doesn't exist") {
-    withServer(indexNameV1 = "not-important", indexNameV2 = "not-important") {
-      server =>
-        server.httpGet(
-          path = "/catalogue/v567/works?pageSize=100&page=101",
-          andExpect = Status.NotFound,
-          withJsonBody = badRequest(
-            s"catalogue/${ApiVersions.default.toString}",
-            "v567 is not a valid API version")
-        )
-    }
-  }
 }

catalogue_api/terraform/outputs.tf

@@ -1,11 +1,3 @@
-output "romulus_app_uri" {
-  value = "${local.romulus_app_uri}"
-}
-
-output "remus_app_uri" {
-  value = "${local.remus_app_uri}"
-}
-
 output "snapshots_bucket_arn" {
   value = "${module.data_api.snapshots_bucket_arn}"
 }