From a63c067e8d83321974d3e95cd73205edf406b044 Mon Sep 17 00:00:00 2001
From: Jonathan Cohen <cohenjon@google.com>
Date: Mon, 29 Jul 2019 16:31:58 -0400
Subject: [PATCH] manager_service cleanup (#460)

* Remove magic number responses from manager_service

* add 'new'

* De-nest branches in setShareMetrics

* Cleanup manager_service

* Fix error checking

* unformat

* Respond to review comments

Force merging because the travis tests passed but the github ui isn't updating.
---
 src/shadowbox/server/manager_service.ts | 53 +++++++++++++++----------
 1 file changed, 32 insertions(+), 21 deletions(-)

diff --git a/src/shadowbox/server/manager_service.ts b/src/shadowbox/server/manager_service.ts
index 72fca736..8e164246 100644
--- a/src/shadowbox/server/manager_service.ts
+++ b/src/shadowbox/server/manager_service.ts
@@ -60,6 +60,11 @@ interface ResponseType {
   send(code: number, data?: {}): void;
 }
 
+enum HttpSuccess {
+  OK = 200,
+  NO_CONTENT = 204,
+}
+
 export function bindService(
     apiServer: restify.Server, apiPrefix: string, service: ShadowsocksManagerService) {
   apiServer.put(`${apiPrefix}/name`, service.renameServer.bind(service));
@@ -93,18 +98,18 @@ export class ShadowsocksManagerService {
   public renameServer(req: RequestType, res: ResponseType, next: restify.Next): void {
     const name = req.params.name;
     if (typeof name !== 'string' || name.length > 100) {
-      res.send(400);
-      next();
+      next(new restify.InvalidArgumentError(
+          `Requested server name should be a string <= 100 characters long.  Got ${name}`));
       return;
     }
     this.serverConfig.data().name = name;
     this.serverConfig.write();
-    res.send(204);
+    res.send(HttpSuccess.NO_CONTENT);
     next();
   }
 
   public getServer(req: RequestType, res: ResponseType, next: restify.Next): void {
-    res.send(200, {
+    res.send(HttpSuccess.OK, {
       name: this.serverConfig.data().name || this.defaultServerName,
       serverId: this.serverConfig.data().serverId,
       metricsEnabled: this.serverConfig.data().metricsEnabled || false,
@@ -122,7 +127,7 @@ export class ShadowsocksManagerService {
       response.accessKeys.push(accessKeyToJson(accessKey));
     }
     logging.debug(`listAccessKeys response ${response}`);
-    res.send(200, response);
+    res.send(HttpSuccess.OK, response);
     return next();
   }
 
@@ -149,7 +154,7 @@ export class ShadowsocksManagerService {
       if (!this.accessKeys.removeAccessKey(accessKeyId)) {
         return next(new restify.NotFoundError(`No access key found with id ${accessKeyId}`));
       }
-      res.send(204);
+      res.send(HttpSuccess.NO_CONTENT);
       return next();
     } catch (error) {
       logging.error(error);
@@ -164,7 +169,7 @@ export class ShadowsocksManagerService {
       if (!this.accessKeys.renameAccessKey(accessKeyId, req.params.name)) {
         return next(new restify.NotFoundError(`No access key found with id ${accessKeyId}`));
       }
-      res.send(204);
+      res.send(HttpSuccess.NO_CONTENT);
       return next();
     } catch (error) {
       logging.error(error);
@@ -189,7 +194,7 @@ export class ShadowsocksManagerService {
       if (!success) {
         return next(new restify.NotFoundError(`No access key found with id ${accessKeyId}`));
       }
-      res.send(204);
+      res.send(HttpSuccess.NO_CONTENT);
       return next();
     } catch (error) {
       logging.error(error);
@@ -205,7 +210,7 @@ export class ShadowsocksManagerService {
       if (!success) {
         return next(new restify.NotFoundError(`No access key found with id ${accessKeyId}`));
       }
-      res.send(204);
+      res.send(HttpSuccess.NO_CONTENT);
       return next();
     } catch (error) {
       logging.error(error);
@@ -215,7 +220,7 @@ export class ShadowsocksManagerService {
 
   public async getDataUsage(req: RequestType, res: ResponseType, next: restify.Next) {
     try {
-      res.send(200, await this.managerMetrics.get30DayByteTransfer());
+      res.send(HttpSuccess.OK, await this.managerMetrics.get30DayByteTransfer());
       return next();
     } catch (error) {
       logging.error(error);
@@ -224,22 +229,28 @@ export class ShadowsocksManagerService {
   }
 
   public getShareMetrics(req: RequestType, res: ResponseType, next: restify.Next): void {
-    res.send(200, {metricsEnabled: this.metricsPublisher.isSharingEnabled()});
+    res.send(HttpSuccess.OK, {metricsEnabled: this.metricsPublisher.isSharingEnabled()});
     next();
   }
 
   public setShareMetrics(req: RequestType, res: ResponseType, next: restify.Next): void {
-    const params = req.params as SetShareMetricsParams;
-    if (typeof params.metricsEnabled === 'boolean') {
-      if (params.metricsEnabled) {
-        this.metricsPublisher.startSharing();
-      } else {
-        this.metricsPublisher.stopSharing();
-      }
-      res.send(204);
-    } else {
-      res.send(400);
+    if (!req.params) {
+      return next(
+          new restify.BadRequestError(`No params attached to request.  Instead got ${req}`));
     }
+    const enabledType = typeof req.params.metricsEnabled;
+    if (enabledType !== 'boolean') {
+      return next(
+          new restify.BadRequestError(`Expected metricsEnabled to be boolean.  Instead got ${
+              req.params.metricsEnabled}, with type ${enabledType}.`));
+    }
+    const enabled = req.params.metricsEnabled;
+    if (enabled) {
+      this.metricsPublisher.startSharing();
+    } else {
+      this.metricsPublisher.stopSharing();
+    }
+    res.send(HttpSuccess.NO_CONTENT);
     next();
   }
 }