From 37ced64f17cfab0c8bd3c1e9b2b11ff3cb385efc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Xavier=20Verg=C3=A9s?=
 <460418+xverges@users.noreply.github.com>
Date: Wed, 2 Jun 2021 15:28:35 +0200
Subject: [PATCH] Beter 404 for plots-tables (#140)

* Beter 404 for plots-tables

* Friendlier logging of expected errors

* Broken if omg

* Better logging

* Log hook execution

* Wrong condition, again :-(

* Take care of falsy parsedMessage
---
 src/api/general-services/pipeline-status.js |  2 +-
 src/api/routes/gem2s.js                     | 24 +++++++++++----------
 src/api/routes/pipelines.js                 | 22 ++++++++++---------
 src/api/routes/plots-tables.js              |  4 +++-
 src/specs/api.yaml                          |  6 ++++++
 src/utils/hookRunner.js                     |  3 +++
 src/utils/parse-sns-message.js              |  2 ++
 7 files changed, 40 insertions(+), 23 deletions(-)

diff --git a/src/api/general-services/pipeline-status.js b/src/api/general-services/pipeline-status.js
index 6d0161034..614ee82b7 100644
--- a/src/api/general-services/pipeline-status.js
+++ b/src/api/general-services/pipeline-status.js
@@ -144,7 +144,7 @@ const getPipelineStatus = async (experimentId, processName) => {
     /* eslint-enable no-await-in-loop */
 
     completedSteps = getStepsFromExecutionHistory(events);
-    logger.log(`ExecutionHistory for ARN ${executionArn}: ${events.length} events, ${completedSteps.length} completed steps`);
+    logger.log(`ExecutionHistory(${processName}) for ARN ${executionArn}: ${events.length} events, ${completedSteps.length} completed steps`);
   }
 
   const response = {
diff --git a/src/api/routes/gem2s.js b/src/api/routes/gem2s.js
index 20bde28f7..d29b8bbbf 100644
--- a/src/api/routes/gem2s.js
+++ b/src/api/routes/gem2s.js
@@ -32,17 +32,19 @@ module.exports = {
     }
 
     const { io, parsedMessage } = result;
-
-    try {
-      await gem2sResponse(io, parsedMessage);
-    } catch (e) {
-      logger.error(
-        'Pipeline response handler failed with error: ', e,
-      );
-
-      AWSXRay.getSegment().addError(e);
-      res.status(200).send('nok');
-      return;
+    const isSnsNotification = parsedMessage !== undefined;
+    if (isSnsNotification) {
+      try {
+        await gem2sResponse(io, parsedMessage);
+      } catch (e) {
+        logger.error(
+          'gem2s pipeline response handler failed with error: ', e,
+        );
+
+        AWSXRay.getSegment().addError(e);
+        res.status(200).send('nok');
+        return;
+      }
     }
 
     res.status(200).send('ok');
diff --git a/src/api/routes/pipelines.js b/src/api/routes/pipelines.js
index c0abe6599..cdb088aa5 100644
--- a/src/api/routes/pipelines.js
+++ b/src/api/routes/pipelines.js
@@ -39,17 +39,19 @@ module.exports = {
     }
 
     const { io, parsedMessage } = result;
+    const isSnsNotification = parsedMessage !== undefined;
+    if (isSnsNotification) {
+      try {
+        await pipelineResponse(io, parsedMessage);
+      } catch (e) {
+        logger.error(
+          'qc pipeline response handler failed with error: ', e,
+        );
 
-    try {
-      await pipelineResponse(io, parsedMessage);
-    } catch (e) {
-      logger.error(
-        'Pipeline response handler failed with error: ', e,
-      );
-
-      AWSXRay.getSegment().addError(e);
-      res.status(200).send('nok');
-      return;
+        AWSXRay.getSegment().addError(e);
+        res.status(200).send('nok');
+        return;
+      }
     }
 
     res.status(200).send('ok');
diff --git a/src/api/routes/plots-tables.js b/src/api/routes/plots-tables.js
index 91e28abfc..b40c81baa 100644
--- a/src/api/routes/plots-tables.js
+++ b/src/api/routes/plots-tables.js
@@ -1,4 +1,6 @@
 const PlotsTablesService = require('../route-services/plots-tables');
+const { NotFoundError } = require('../../utils/responses');
+
 
 const plotsTablesService = new PlotsTablesService();
 
@@ -16,7 +18,7 @@ module.exports = {
       .then((response) => res.json(response))
       .catch((e) => {
         if (e.message.includes('not found')) {
-          res.status(404).send('');
+          throw (new NotFoundError('Plot not found'));
         } else {
           throw e;
         }
diff --git a/src/specs/api.yaml b/src/specs/api.yaml
index c7b083dfb..6c09ad7f7 100644
--- a/src/specs/api.yaml
+++ b/src/specs/api.yaml
@@ -523,6 +523,12 @@ paths:
             application/json:
               schema:
                 $ref: ./models/HTTPError.v1.yaml
+        '404':
+          description: Experiment/plot not found.
+          content:
+            application/json:
+              schema:
+                $ref: ./models/HTTPError.v1.yaml
         '200':
           description: OK
           content:
diff --git a/src/utils/hookRunner.js b/src/utils/hookRunner.js
index 445912eae..49fbcc24b 100644
--- a/src/utils/hookRunner.js
+++ b/src/utils/hookRunner.js
@@ -1,3 +1,5 @@
+const logger = require('./logging');
+
 class hookRunner {
   constructor() {
     this.hooks = {};
@@ -27,6 +29,7 @@ class hookRunner {
       // eslint-disable-next-line no-await-in-loop
       this.results[taskName].push(await this.hooks[taskName][idx](payload));
     }
+    logger.log(`Completed ${this.results[taskName].length} hooks for pipeline task ${taskName}`);
 
     return this.results;
   }
diff --git a/src/utils/parse-sns-message.js b/src/utils/parse-sns-message.js
index d422de280..997ea8fd4 100644
--- a/src/utils/parse-sns-message.js
+++ b/src/utils/parse-sns-message.js
@@ -53,6 +53,8 @@ const parseSNSMessage = async (req) => {
       logger.error('Error parsing message:', e);
       throw e;
     }
+  } else {
+    logger.log(`SNS message is of type "${msg.Type}", ignoring it`);
   }
 
   return {};