From c2961b19578acb707cfff69710658924c8218058 Mon Sep 17 00:00:00 2001
From: Shayne Fletcher <shayne@shaynefletcher.org>
Date: Wed, 3 Jun 2020 14:56:51 -0400
Subject: [PATCH] Make the stop endpoint more robust and test (#6217)

* Make the stop endpoint more robust and test

changelog_begin
changelog_end

* Stopping an unknown trigger gives 404
---
 .../daml/lf/engine/trigger/ServiceMain.scala  | 31 ++++++++++-----
 .../daml/lf/engine/trigger/ServiceTest.scala  | 39 +++++++++++++++++++
 2 files changed, 61 insertions(+), 9 deletions(-)

diff --git a/triggers/service/src/main/scala/com/digitalasset/daml/lf/engine/trigger/ServiceMain.scala b/triggers/service/src/main/scala/com/digitalasset/daml/lf/engine/trigger/ServiceMain.scala
index 635779ab28..91bb6cb0a5 100644
--- a/triggers/service/src/main/scala/com/digitalasset/daml/lf/engine/trigger/ServiceMain.scala
+++ b/triggers/service/src/main/scala/com/digitalasset/daml/lf/engine/trigger/ServiceMain.scala
@@ -103,8 +103,8 @@ class Server(dar: Option[Dar[(PackageId, Package)]], jdbcConfig: Option[JdbcConf
     }
   }
 
-  private def getRunningTrigger(uuid: UUID): RunningTrigger = {
-    triggers(uuid) // TODO: Improve as might throw NoSuchElementException.
+  private def getRunningTrigger(uuid: UUID): Option[RunningTrigger] = {
+    triggers.get(uuid)
   }
 
   private def addRunningTrigger(t: RunningTrigger): Either[String, Unit] = {
@@ -234,16 +234,21 @@ object Server {
       JsObject(("triggerId", triggerInstance.toString.toJson))
     }
 
-    def stopTrigger(uuid: UUID, token: (Jwt, JwtPayload)): JsValue = {
+    def stopTrigger(uuid: UUID, token: (Jwt, JwtPayload)): Option[JsValue] = {
       //TODO(SF, 2020-05-20): At least check that the provided token
       //is the same as the one used to start the trigger and fail with
       //'Unauthorized' if not (expect we'll be able to do better than
       //this).
-      val runningTrigger = server.getRunningTrigger(uuid)
-      runningTrigger.runner ! TriggerRunner.Stop
-      server.logTriggerStatus(runningTrigger, "stopped: by user request")
-      server.removeRunningTrigger(runningTrigger)
-      JsObject(("triggerId", uuid.toString.toJson))
+      server
+        .getRunningTrigger(uuid)
+        .map(
+          runningTrigger => {
+            runningTrigger.runner ! TriggerRunner.Stop
+            server.logTriggerStatus(runningTrigger, "stopped: by user request")
+            server.removeRunningTrigger(runningTrigger)
+            JsObject(("triggerId", uuid.toString.toJson))
+          }
+        )
     }
 
     def listTriggers(jwt: Jwt): (StatusCode, JsObject) = {
@@ -373,7 +378,15 @@ object Server {
                     unauthorized =>
                       complete(
                         errorResponse(StatusCodes.UnprocessableEntity, unauthorized.message)),
-                    triggerInstance => complete(successResponse(triggerInstance))
+                    triggerInstance =>
+                      triggerInstance match {
+                        case Some(stoppedTriggerId) => complete(successResponse(stoppedTriggerId))
+                        case None =>
+                          complete(
+                            errorResponse(
+                              StatusCodes.NotFound,
+                              "Unknown trigger: '" + uuid.toString + "'"))
+                    }
                   )
             }
         }
diff --git a/triggers/service/src/test/scala/com/digitalasset/daml/lf/engine/trigger/ServiceTest.scala b/triggers/service/src/test/scala/com/digitalasset/daml/lf/engine/trigger/ServiceTest.scala
index 14a3cf0cc4..adddd3fec1 100644
--- a/triggers/service/src/test/scala/com/digitalasset/daml/lf/engine/trigger/ServiceTest.scala
+++ b/triggers/service/src/test/scala/com/digitalasset/daml/lf/engine/trigger/ServiceTest.scala
@@ -500,4 +500,43 @@ class ServiceTest extends AsyncFlatSpec with Eventually with Matchers with Postg
       } yield succeed
   }
 
+  it should "stopping a trigger without providing a token should be unauthorized" in withHttpService(
+    None) { (uri: Uri, client: LedgerClient, ledgerProxy: Proxy) =>
+    val uuid: String = "ffffffff-ffff-ffff-ffff-ffffffffffff"
+    val req = HttpRequest(
+      method = HttpMethods.DELETE,
+      uri = uri.withPath(Uri.Path(s"/v1/stop/$uuid")),
+    )
+    for {
+      resp <- Http().singleRequest(req)
+      body <- responseBodyToString(resp)
+      JsObject(fields) = body.parseJson
+      _ <- fields.get("status") should equal(Some(JsNumber(422)))
+      _ <- fields.get("errors") should equal(
+        Some(JsArray(JsString("missing Authorization header with OAuth 2.0 Bearer Token"))))
+    } yield succeed
+  }
+
+  it should "stopping a trigger that can't parse as a UUID gives a 404 response" in withHttpService(
+    None) { (uri: Uri, client: LedgerClient, ledgerProxy: Proxy) =>
+    for {
+      resp <- stopTrigger(uri, "No More Mr Nice Guy", "Alice")
+      _ <- assert(resp.status.isFailure() && resp.status.intValue() == 404)
+    } yield succeed
+  }
+
+  it should "stopping an unknown trigger gives an error response" in withHttpService(None) {
+    (uri: Uri, client: LedgerClient, ledgerProxy: Proxy) =>
+      val uuid: String = "ffffffff-ffff-ffff-ffff-ffffffffffff"
+      for {
+        resp <- stopTrigger(uri, uuid, "Alice")
+        _ <- assert(resp.status.isFailure() && resp.status.intValue() == 404)
+        body <- responseBodyToString(resp)
+        JsObject(fields) = body.parseJson
+        _ <- fields.get("status") should equal(Some(JsNumber(404)))
+        _ <- fields.get("errors") should equal(
+          Some(JsArray(JsString("Unknown trigger: '" + uuid.toString + "'"))))
+      } yield succeed
+  }
+
 }