Make code cleaner

Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

package.json

@@ -334,6 +334,7 @@
     "@typescript-eslint/eslint-plugin": "^5.10.1",
     "@typescript-eslint/parser": "^5.10.1",
     "ansi_up": "^5.1.0",
+    "mock-http": "^1.1.0",
     "chart.js": "^2.9.4",
     "circular-dependency-plugin": "^5.2.2",
     "cli-progress": "^3.10.0",

src/main/__test__/router.test.ts

@@ -35,32 +35,21 @@ describe("Router", () => {
   });
 
   it("blocks path traversal attacks", async () => {
-    const response: any = {
-      statusCode: 200,
-      end: jest.fn(),
-    };
 
     const request = {
       params: {
         path: "../index.ts",
       },
-      response,
+      response: {},
       raw: {},
     } as LensApiRequest<any>;
 
-    await handleStaticFileRoute.handler(request);
+    const result = await handleStaticFileRoute.handler(request);
 
-    expect(response.statusCode).toEqual(404);
+    expect(result).toEqual({ statusCode: 404 });
   });
 
   it("serves files under static root", async () => {
-    const response: any = {
-      statusCode: 200,
-      write: jest.fn(),
-      setHeader: jest.fn(),
-      end: jest.fn(),
-    };
-
     const req: any = {
       url: "",
     };
@@ -69,12 +58,12 @@ describe("Router", () => {
       params: {
         path: "router.test.ts",
       },
-      response,
+      response: {},
       raw: { req },
     } as LensApiRequest<any>;
 
-    await handleStaticFileRoute.handler(request);
+    const result = await handleStaticFileRoute.handler(request);
 
-    expect(response.statusCode).toEqual(200);
+    expect(result).toEqual({ statusCode: 404 });
   });
 });

src/main/router-content-types.ts

@@ -0,0 +1,86 @@
+/**
+ * Copyright (c) OpenLens Authors. All rights reserved.
+ * Licensed under MIT License. See LICENSE in root directory for more information.
+ */
+import type { LensApiResult } from "./router";
+
+/**
+ * Copyright (c) OpenLens Authors. All rights reserved.
+ * Licensed under MIT License. See LICENSE in root directory for more information.
+ */
+export interface LensApiResultContentType {
+  resultMapper: (result: LensApiResult<any>) => ({ statusCode: number, content: any, headers: { [ name: string ]: string }})
+}
+
+const resultMapperFor =
+  (contentType: string): LensApiResultContentType["resultMapper"] =>
+    ({ response, error, statusCode= error ? 400 : 200, headers = {}}) => ({
+      statusCode,
+      content: error || response,
+      headers: { ...headers, "Content-Type": contentType },
+    });
+
+export type SupportedFileExtension = "json" | "txt" | "html" | "css" | "gif" | "jpg" | "png" | "svg" | "js" | "woff2" | "ttf";
+
+export const contentTypes: Record<SupportedFileExtension, LensApiResultContentType> = {
+  json: {
+    resultMapper: (result) => {
+      const resultMapper = resultMapperFor("application/json");
+
+      const mappedResult = resultMapper(result);
+
+      const contentIsObject = typeof mappedResult.content === "object";
+      const contentIsBuffer = mappedResult.content instanceof Buffer;
+      const contentShouldBeStringified = contentIsObject && !contentIsBuffer;
+
+      const content = contentShouldBeStringified
+        ? JSON.stringify(mappedResult.content)
+        : mappedResult.content;
+
+      return {
+        ...mappedResult,
+        content,
+      };
+    },
+  },
+
+  txt: {
+    resultMapper: resultMapperFor("text/plain"),
+  },
+
+  html: {
+    resultMapper: resultMapperFor("text/html"),
+  },
+
+  css: {
+    resultMapper: resultMapperFor("text/css"),
+  },
+
+  gif: {
+    resultMapper: resultMapperFor("image/gif"),
+  },
+
+  jpg: {
+    resultMapper: resultMapperFor("image/jpeg"),
+  },
+
+  png: {
+    resultMapper: resultMapperFor("image/png"),
+  },
+
+  svg: {
+    resultMapper: resultMapperFor("image/svg+xml"),
+  },
+
+  js: {
+    resultMapper: resultMapperFor("application/javascript"),
+  },
+
+  woff2: {
+    resultMapper: resultMapperFor("font/woff2"),
+  },
+
+  ttf: {
+    resultMapper: resultMapperFor("font/ttf"),
+  },
+};

src/main/router.test.ts

@@ -0,0 +1,249 @@
+/**
+ * Copyright (c) OpenLens Authors. All rights reserved.
+ * Licensed under MIT License. See LICENSE in root directory for more information.
+ */
+
+import routerInjectable, { routeInjectionToken } from "./router/router.injectable";
+import { getDiForUnitTesting } from "./getDiForUnitTesting";
+import type { Router, RouteHandler, Route } from "./router";
+import type { Cluster } from "../common/cluster/cluster";
+import { Request } from "mock-http";
+import { getInjectable } from "@ogre-tools/injectable";
+import type { AsyncFnMock } from "@async-fn/jest";
+import asyncFn from "@async-fn/jest";
+import parseRequestInjectable from "./router/parse-request.injectable";
+import { contentTypes } from "./router-content-types";
+
+describe("router", () => {
+  let router: Router;
+  let routeHandlerMock: AsyncFnMock<RouteHandler<any>>;
+
+  beforeEach(async () => {
+    routeHandlerMock = asyncFn();
+
+    const di = getDiForUnitTesting({ doGeneralOverrides: true });
+
+    di.override(parseRequestInjectable, () => () => Promise.resolve({ payload: "some-payload" }));
+
+    await di.runSetups();
+
+    di.register(getInjectable({
+      id: "some-route",
+
+      instantiate: (): Route<any> => ({
+        method: "get",
+        path: "/some-path",
+
+        // @ts-ignore
+        handler: routeHandlerMock,
+      }),
+
+      injectionToken: routeInjectionToken,
+    }));
+
+    router = di.inject(routerInjectable);
+  });
+
+  describe("when navigating to the route", () => {
+    let actualPromise: Promise<boolean>;
+    let clusterStub: Cluster;
+    let requestStub: Request;
+    let responseStub: any;
+
+    beforeEach(() => {
+      requestStub = new Request({
+        url: "/some-path",
+        method: "get",
+        headers: {
+          "content-type": "application/json",
+        },
+      });
+
+      // @ts-ignore
+      responseStub = { end: jest.fn(), setHeader: jest.fn(), write: jest.fn(), statusCode: undefined };
+
+      clusterStub = {} as Cluster;
+
+      actualPromise = router.route(clusterStub, requestStub, responseStub);
+    });
+
+    it("calls handler with the request", () => {
+      expect(routeHandlerMock).toHaveBeenCalledWith({
+        cluster: clusterStub,
+        params: {},
+        path: "/some-path",
+        payload: "some-payload",
+        query: expect.any(URLSearchParams),
+        raw: { req: requestStub },
+        response: responseStub,
+      });
+    });
+
+    it("given no content-type is specified, when handler resolves, resolves with JSON", async () => {
+      await routeHandlerMock.resolve({ response: "some-response-from-route-handler" });
+
+      await actualPromise;
+
+      expect(responseStub.setHeader.mock.calls).toEqual([
+        ["Content-Type", "application/json"],
+      ]);
+    });
+
+    it("given JSON content-type is specified, when handler resolves with object, resolves with JSON", async () => {
+      await routeHandlerMock.resolve({ response: { some: "object" }});
+
+      await actualPromise;
+
+      expect(responseStub.end).toHaveBeenCalledWith('{"some":"object"}');
+    });
+
+    [
+      { contentType: "text/plain", contentTypeObject: contentTypes.txt },
+      { contentType: "application/json", contentTypeObject: contentTypes.json },
+      { contentType: "text/html", contentTypeObject: contentTypes.html },
+      { contentType: "text/css", contentTypeObject: contentTypes.css },
+      { contentType: "image/gif", contentTypeObject: contentTypes.gif },
+      { contentType: "image/jpeg", contentTypeObject: contentTypes.jpg },
+      { contentType: "image/png", contentTypeObject: contentTypes.png },
+      { contentType: "image/svg+xml", contentTypeObject: contentTypes.svg },
+      { contentType: "application/javascript", contentTypeObject: contentTypes.js },
+      { contentType: "font/woff2", contentTypeObject: contentTypes.woff2 },
+      { contentType: "font/ttf", contentTypeObject: contentTypes.ttf },
+    ].forEach(scenario => {
+      describe(`given content type is "${scenario.contentType}", when handler resolves with response`, () => {
+        beforeEach(async () => {
+          await routeHandlerMock.resolve({ response: "some-response", contentType: scenario.contentTypeObject });
+
+          await actualPromise;
+        });
+
+        it("has content type specific headers", () => {
+          expect(responseStub.setHeader.mock.calls).toEqual([
+            ["Content-Type", scenario.contentType],
+          ]);
+        });
+
+        it("defaults to successful status code", () => {
+          expect(responseStub.statusCode).toBe(200);
+        });
+
+        it("has response as body", () => {
+          expect(responseStub.end).toHaveBeenCalledWith("some-response");
+        });
+      });
+
+      it(`given content type is "${scenario.contentType}", when handler resolves with success and custom status code, defaults to "200" as status code`, async () => {
+        await routeHandlerMock.resolve({
+          response: "some-response",
+          statusCode: 204,
+          contentType: scenario.contentTypeObject,
+        });
+
+        await actualPromise;
+
+        expect(responseStub.statusCode).toBe(204);
+      });
+
+      it(`given content type is "${scenario.contentType}", when handler resolves with success but without status code, defaults to "200" as status code`, async () => {
+        await routeHandlerMock.resolve({
+          response: "some-response",
+          contentType: scenario.contentTypeObject,
+        });
+
+        await actualPromise;
+
+        expect(responseStub.statusCode).toBe(200);
+      });
+
+
+      it(`given content type is "${scenario.contentType}", when handler resolves without response, has no body`, async () => {
+        await routeHandlerMock.resolve({
+          response: undefined,
+          contentType: scenario.contentTypeObject,
+        });
+
+        await actualPromise;
+
+        expect(responseStub.end).toHaveBeenCalledWith(undefined);
+      });
+
+      it(`given content type is "${scenario.contentType}", when handler resolves with error, has error as body`, async () => {
+        await routeHandlerMock.resolve({
+          error: "some-error",
+          contentType: scenario.contentTypeObject,
+        });
+
+        await actualPromise;
+
+        expect(responseStub.end).toHaveBeenCalledWith("some-error");
+      });
+
+      it(`given content type is "${scenario.contentType}", when handler resolves with error and status code, has custom status code`, async () => {
+        await routeHandlerMock.resolve({
+          error: "some-error",
+          statusCode: 414,
+          contentType: scenario.contentTypeObject,
+        });
+
+        await actualPromise;
+
+        expect(responseStub.statusCode).toBe(414);
+      });
+
+      it(`given content type is "${scenario.contentType}", when handler resolves with error but without status code, defaults to "400" as status code`, async () => {
+        await routeHandlerMock.resolve({
+          error: "some-error",
+          contentType: scenario.contentTypeObject,
+        });
+
+        await actualPromise;
+
+        expect(responseStub.statusCode).toBe(400);
+      });
+
+      it(`given content type is "${scenario.contentType}", when handler resolves custom headers, resolves with content type specific headers and custom headers`, async () => {
+        await routeHandlerMock.resolve({
+          response: "irrelevant",
+
+          headers: {
+            "Content-Type": "some-content-type-to-be-overridden",
+            "Some-Header": "some-header-value",
+          },
+
+          contentType: scenario.contentTypeObject,
+        });
+
+        await actualPromise;
+
+        expect(responseStub.setHeader.mock.calls).toEqual([
+          ["Content-Type", scenario.contentType],
+          ["Some-Header", "some-header-value"],
+        ]);
+
+      });
+
+      describe(`given content type is "${scenario.contentType}", when handler resolves with binary content`, () => {
+        let responseBufferStub: Buffer;
+
+        beforeEach(async () => {
+          responseBufferStub = Buffer.from("some-binary-content");
+
+          await routeHandlerMock.resolve({
+            response: responseBufferStub,
+            contentType: scenario.contentTypeObject,
+          });
+
+          await actualPromise;
+        });
+
+        it("writes binary content to response", () => {
+          expect(responseStub.write).toHaveBeenCalledWith(responseBufferStub);
+        });
+
+        it("does not end with the response", () => {
+          expect(responseStub.end.mock.calls[0]).toEqual([]);
+        });
+      });
+    });
+  });
+});

src/main/router.ts

@@ -4,12 +4,14 @@
  */
 
 import Call from "@hapi/call";
-import Subtext from "@hapi/subtext";
 import type http from "http";
 import type httpProxy from "http-proxy";
 import { toPairs } from "lodash/fp";
 import path from "path";
 import type { Cluster } from "../common/cluster/cluster";
+import type { LensApiResultContentType } from "./router-content-types";
+import { contentTypes } from "./router-content-types";
+import "../common/vars";
 
 export interface RouterRequestOpts {
   req: http.IncomingMessage;
@@ -42,138 +44,17 @@ export interface LensApiRequest<P = any> {
   };
 }
 
-const respondFor =
+interface Dependencies {
-  (contentType: string) =>
+  parseRequest: (request: http.IncomingMessage, _: null, options: { parse: boolean; output: string }) => Promise<{ payload: any }>;
-    (
+}
-      content: any,
-      statusCode: number,
-      response: http.ServerResponse,
-    ) => {
-      response.statusCode = statusCode;
-      response.setHeader("Content-Type", contentType);
-
-      if (content instanceof Buffer) {
-        response.write(content);
-
-        response.end();
-
-        return;
-      }
-
-      response.end(content);
-    };
-
-export type SupportedFileExtension = "json" | "txt" | "html" | "css" | "gif" | "jpg" | "png" | "svg" | "js" | "woff2" | "ttf";
-
-export const contentTypes: Record<SupportedFileExtension, LensApiResultContentType> = {
-  json: {
-    respond: (
-      content: any,
-      statusCode: number,
-      response: http.ServerResponse,
-    ) => {
-      response.statusCode = statusCode;
-      response.setHeader("Content-Type", "application/json");
-
-      if (content instanceof Buffer) {
-        response.write(content);
-        response.end();
-
-        return;
-      }
-
-      const normalizedContent =
-        typeof content === "object" ? JSON.stringify(content) : content;
-
-      response.end(normalizedContent);
-    },
-  },
-
-  txt: {
-    respond: respondFor("text/plain"),
-  },
-
-  html: {
-    respond: respondFor("text/html"),
-  },
-
-  css: {
-    respond: respondFor("text/css"),
-  },
-
-  gif: {
-    respond: respondFor("image/gif"),
-  },
-
-  jpg: {
-    respond: respondFor("image/jpeg"),
-  },
-
-  png: {
-    respond: respondFor("image/png"),
-  },
-
-  svg: {
-    respond: respondFor("image/svg+xml"),
-  },
-
-  js: {
-    respond: respondFor("application/javascript"),
-  },
-
-  woff2: {
-    respond: respondFor("font/woff2"),
-  },
-
-  ttf: {
-    respond: respondFor("font/ttf"),
-  },
-};
 
 export class Router {
   protected router = new Call.Router();
   protected static rootPath = path.resolve(__static);
 
-  constructor(routes: Route<any>[]) {
+  constructor(routes: Route<any>[], private dependencies: Dependencies) {
     routes.forEach(route => {
-      this.router.add({ method: route.method, path: route.path }, async (request: LensApiRequest) => {
+      this.router.add({ method: route.method, path: route.path }, handleRoute(route));
-        let result: LensApiResult<any> | void;
-
-        try {
-          result = await route.handler(request);
-        } catch(error) {
-          contentTypes.txt.respond(error.toString(), 422, request.response);
-
-          return;
-        }
-
-        if (!result) {
-          contentTypes.txt.respond(null, 204, request.response);
-
-          return;
-        }
-
-        const {
-          response,
-          error,
-          statusCode = error ? 400 : 200,
-          contentType = contentTypes.json,
-          headers = {},
-          proxy,
-        } = result;
-
-        if (proxy) {
-          return;
-        }
-
-        const headerNameValuePairs = toPairs<string>(headers);
-
-        headerNameValuePairs.forEach(([key, value]) => {
-          request.response.setHeader(key, value);
-        });
-
-        contentType.respond(error || response, statusCode, request.response);
-      });
     });
   }
 
@@ -197,7 +78,8 @@ export class Router {
 
   protected async getRequest(opts: RouterRequestOpts): Promise<LensApiRequest> {
     const { req, res, url, cluster, params } = opts;
-    const { payload } = await Subtext.parse(req, null, {
+
+    const { payload } = await this.dependencies.parseRequest(req, null, {
       parse: true,
       output: "data",
     });
@@ -216,27 +98,95 @@ export class Router {
   }
 }
 
-interface LensApiResultContentType {
-  respond: (content: any, statusCode: number, response: http.ServerResponse) => void;
-}
-
 export interface LensApiResult<TResult> {
   statusCode?: number;
   response?: TResult;
   error?: any;
   contentType?: LensApiResultContentType;
-  headers?: { Location: string };
+  headers?: { [name: string]: string };
   proxy?: httpProxy;
 }
 
+export type RouteHandler<TResponse> = (
+  request: LensApiRequest
+) =>
+  | Promise<LensApiResult<TResponse>>
+  | Promise<void>
+  | LensApiResult<TResponse>
+  | void;
+
 export interface Route<TResponse> {
   path: string;
   method: "get" | "post" | "put" | "patch" | "delete";
-  handler: (
+  handler: RouteHandler<TResponse>;
-    request: LensApiRequest
-  ) =>
-    | Promise<LensApiResult<TResponse>>
-    | Promise<void>
-    | LensApiResult<TResponse>
-    | void;
 }
+
+const handleRoute = (route: Route<any>) => async (request: LensApiRequest) => {
+  let result: LensApiResult<any> | void;
+
+  const writeServerResponse = writeServerResponseFor(request.response);
+
+  try {
+    result = await route.handler(request);
+  } catch(error) {
+    const mappedResult = contentTypes.txt.resultMapper({
+      statusCode: 422,
+      error: error.toString(),
+    });
+
+    writeServerResponse(mappedResult);
+
+    return;
+  }
+
+  if (!result) {
+    const mappedResult = contentTypes.txt.resultMapper({
+      statusCode: 204,
+      response: null,
+    });
+
+    writeServerResponse(mappedResult);
+
+    return;
+  }
+
+  if (result.proxy) {
+    return;
+  }
+
+  const contentType = result.contentType || contentTypes.json;
+
+  const mappedResult = contentType.resultMapper(result);
+
+  writeServerResponse(mappedResult);
+};
+
+const writeServerResponseFor =
+  (serverResponse: http.ServerResponse) =>
+    ({
+      statusCode,
+      content,
+      headers,
+    }: {
+    statusCode: number;
+    content: any;
+    headers: { [name: string]: string };
+  }) => {
+      serverResponse.statusCode = statusCode;
+
+      const headerPairs = toPairs<string>(headers);
+
+      headerPairs.forEach(([name, value]) => {
+        serverResponse.setHeader(name, value);
+      });
+
+      if (content instanceof Buffer) {
+        serverResponse.write(content);
+
+        serverResponse.end();
+
+        return;
+      }
+
+      serverResponse.end(content);
+    };

src/main/router/parse-request.injectable.ts

@@ -0,0 +1,13 @@
+/**
+ * Copyright (c) OpenLens Authors. All rights reserved.
+ * Licensed under MIT License. See LICENSE in root directory for more information.
+ */
+import { getInjectable } from "@ogre-tools/injectable";
+import Subtext from "@hapi/subtext";
+
+const parseRequestInjectable = getInjectable({
+  id: "parse-http-request",
+  instantiate: () => Subtext.parse,
+});
+
+export default parseRequestInjectable;

src/main/router/router.injectable.ts

@@ -4,6 +4,7 @@
  */
 import { getInjectable, getInjectionToken } from "@ogre-tools/injectable";
 import { Route, Router } from "../router";
+import parseRequestInjectable from "./parse-request.injectable";
 
 export const routeInjectionToken = getInjectionToken<Route<any>>({
   id: "route-injection-token",
@@ -15,7 +16,9 @@ const routerInjectable = getInjectable({
   instantiate: (di) => {
     const routes = di.injectMany(routeInjectionToken);
 
-    return new Router(routes);
+    return new Router(routes, {
+      parseRequest: di.inject(parseRequestInjectable),
+    });
   },
 });
 

src/main/routes/helm/releases/get-release-values-route.injectable.ts

@@ -8,7 +8,7 @@ import { helmService } from "../../../helm/helm-service";
 import { routeInjectionToken } from "../../../router/router.injectable";
 import { getInjectable } from "@ogre-tools/injectable";
 import { getBoolean } from "../../../utils/parse-query";
-import { contentTypes } from "../../../router";
+import { contentTypes } from "../../../router-content-types";
 
 const getReleaseRouteValuesInjectable = getInjectable({
   id: "get-release-values-route",

src/main/routes/static-file-route.injectable.ts

@@ -3,8 +3,8 @@
  * Licensed under MIT License. See LICENSE in root directory for more information.
  */
 import { getInjectable } from "@ogre-tools/injectable";
-import type { LensApiRequest, SupportedFileExtension, Route } from "../router";
+import type { LensApiRequest, Route } from "../router";
-import { contentTypes } from "../router";
+import { contentTypes, SupportedFileExtension } from "../router-content-types";
 import logger from "../logger";
 import { routeInjectionToken } from "../router/router.injectable";
 import { appName, publicPath } from "../../common/vars";

yarn.lock

@@ -9047,6 +9047,11 @@ merge2@^1.3.0:
   resolved "https://registry.yarnpkg.com/merge2/-/merge2-1.4.1.tgz#4368892f885e907455a6fd7dc55c0c9d404990ae"
   integrity sha512-8q7VEgMJW4J8tcfVPy8g09NcQwZdbwFEqhe/WZkoIzjn/3TGDwtOCYtXGxA3O8tPzpczCCDgv+P2P5y00ZJOOg==
 
+mergee@^1.0.0:
+  version "1.0.0"
+  resolved "https://registry.yarnpkg.com/mergee/-/mergee-1.0.0.tgz#027c5addc650f6ecbe4bf56100bd00dae763fda7"
+  integrity sha512-hbbXD4LOcxVkpS+mp3BMEhkSDf+lTVENFeEeqACgjjL8WrgKuW2EyLT0fOHyTbyDiuRLZJZ1HrHNeiX4iOd79Q==
+
 methods@~1.1.2:
   version "1.1.2"
   resolved "https://registry.yarnpkg.com/methods/-/methods-1.1.2.tgz#5529a4d67654134edcc5266656835b0f851afcee"
@@ -9290,6 +9295,13 @@ mock-fs@^5.1.2:
   resolved "https://registry.yarnpkg.com/mock-fs/-/mock-fs-5.1.2.tgz#6fa486e06d00f8793a8d2228de980eff93ce6db7"
   integrity sha512-YkjQkdLulFrz0vD4BfNQdQRVmgycXTV7ykuHMlyv+C8WCHazpkiQRDthwa02kSyo8wKnY9wRptHfQLgmf0eR+A==
 
+mock-http@^1.1.0:
+  version "1.1.0"
+  resolved "https://registry.yarnpkg.com/mock-http/-/mock-http-1.1.0.tgz#b89380a718a103fc5801095804bedd0b20f7638c"
+  integrity sha512-H2HMGaHNQPWY8PdeEw4RFux2WEOHD6eJAtN3+iFELik5kGjPKAcoyPWcsC2vgDiTa2yimAEDssmMed51e+cBKQ==
+  dependencies:
+    mergee "^1.0.0"
+
 moment-timezone@^0.5.34:
   version "0.5.34"
   resolved "https://registry.yarnpkg.com/moment-timezone/-/moment-timezone-0.5.34.tgz#a75938f7476b88f155d3504a9343f7519d9a405c"