Repository: SwitchbackTech/compass (ts)

Issue:
Backend throws error during notification attempt if no websocket connection
## Prerequisites

- [x] Using an up-to-date main branch

## Expected Behavior

<!--- What should have happened? -->
Given that the backend has a notification from google about an event change
And the user with the change is not currently connected to compass via websockets
Then the backend should not attempt to emit the update to the user's client
And the backend should not throw and error

## Current Behavior
Backend throws and logs error.

```bash
74|backend | 400 POST /api/sync/gcal/notifications 265.820ms Thu, 03 Oct 2024 15:42:06 GMT

74|backend  | 24-10-03 15:55:08 [debug] app:sync.service.helpers: updating: "💸 Some Event"  | xyz
74|backend  | 24-10-03 15:55:08 [warn] app:websocket.server: Event update not sent to client due to missing userId: some-user-id
74|backend  | 24-10-03 15:55:08 [error] app:sync.controller: Not sure how to handle this error:
74|backend  | 24-10-03 15:55:08 [error] app:sync.controller: Invalid socket id {
74|backend  |   "result": "Event update not sent to client",
74|backend  |   "description": "Invalid socket id",
74|backend  |   "statusCode": 400,
74|backend  |   "isOperational": true,
74|backend  |   "stack": "Error: Invalid socket id\n    at new BaseError (/compass/packages/core/src/errors/errors.base.ts:23:11)\n    at error (/compass/packages/backend/src/common/errors/handlers/error.handler.ts:9:10)\n    at notifyClient (/compass/packages/backend/src/servers/websocket/websocket.server.ts:34:16)\n    at SyncService.<anonymous> (/compass/packages/backend/src/sync/services/sync.service.ts:97:17)\n    at Generator.next (<anonymous>)\n    at fulfilled (/compass/build/node/node_modules/tslib/tslib.js:166:62)\n    at runMicrotasks (<anonymous>)\n    at processTicksAndRejections (node:internal/process/task_queues:96:5)"
74|backend  | }
```

<!--- What went wrong? -->

## Steps to Reproduce

<!-- Add relevant code and/or a live example -->
<!-- Add stack traces -->

1. Close any browser windows with connection to Compass app

2. Make change in Google cal

3. See compass backend logs


## Possible Solution (Not obligatory)
Instead of throwing error in this scenario, just log a debug message
<!--- Suggest a reason for the bug or how to fix it. -->

## Context
This doesn't affect users directly. It's more of a dev inconvenience, because now the logs show errors during normal behavior
<!--- How has this issue affected users? What are they trying to accomplish? -->


Pull request:
♻️ Refactor: convert webSocketServer to class
Closes #146 

This does 2 things

1. Converts the websocker server from a function to class
Converting to a class makes it easier to test, while also preserving the ability to reuse instances of the `webSocketServer` throughout different parts of the app using the singleton pattern. 

2. Updates how google calendar notifications are handled. Previously, they threw an error if the client wasn't connected to Compass at the time of the notification. Throwing that error was unnecessary, as the updated data is still saved in Compass' DB as expected. Now, when Compass gets a notification from google that the user's data has changed and the user isn't connected to Compass it doesn't log an error.

Patch:
diff --git a/packages/backend/src/app.ts b/packages/backend/src/app.ts
index 7749dce7..3465133b 100644
--- a/packages/backend/src/app.ts
+++ b/packages/backend/src/app.ts
@@ -13,16 +13,16 @@ import * as http from "http";
 import { Logger } from "@core/logger/winston.logger";
 
 import { ENV } from "./common/constants/env.constants";
-import { initWebsocketServer } from "./servers/websocket/websocket.server";
 import { initExpressServer } from "./servers/express/express.server";
 import mongoService from "./common/services/mongo.service";
+import { webSocketServer } from "./servers/websocket/websocket.server";
 
 const logger = Logger("app:root");
 mongoService;
 
 const app = initExpressServer();
 const httpServer: http.Server = http.createServer(app);
-initWebsocketServer(httpServer);
+webSocketServer.init(httpServer);
 
 const port = ENV.PORT;
 httpServer.listen(port, () => {
diff --git a/packages/backend/src/common/constants/error.constants.ts b/packages/backend/src/common/constants/error.constants.ts
index 90a597a0..a7a1c466 100644
--- a/packages/backend/src/common/constants/error.constants.ts
+++ b/packages/backend/src/common/constants/error.constants.ts
@@ -129,6 +129,12 @@ export const SocketError = {
     status: Status.BAD_REQUEST,
     isOperational: true,
   },
+  ServerNotReady: {
+    description:
+      "WebSocket server not ready (Did you forget to initialize it?)",
+    status: Status.INTERNAL_SERVER,
+    isOperational: false,
+  },
 };
 
 export const SyncError = {
diff --git a/packages/backend/src/servers/websocket/websocket.server.ts b/packages/backend/src/servers/websocket/websocket.server.ts
index ce8bca1b..9beeaed3 100644
--- a/packages/backend/src/servers/websocket/websocket.server.ts
+++ b/packages/backend/src/servers/websocket/websocket.server.ts
@@ -10,6 +10,8 @@ import {
 import {
   EVENT_CHANGE_PROCESSED,
   EVENT_CHANGED,
+  RESULT_IGNORED,
+  RESULT_NOTIFIED_CLIENT,
 } from "@core/constants/websocket.constants";
 import { Logger } from "@core/logger/winston.logger";
 import { SocketError } from "@backend/common/constants/error.constants";
@@ -20,79 +22,98 @@ import { handleWsError } from "./websocket.util";
 
 const logger = Logger("app:websocket.server");
 
-let wsServer: CompassSocketServer;
-export const connections = new Map<string, string>(); // { userId: socketId }
+export class WebSocketServer {
+  private connections: Map<string, string>;
+  private wsServer?: CompassSocketServer;
 
-export const notifyClient = (userId: string, server?: CompassSocketServer) => {
-  const socketServer = server || wsServer;
-  const socketId = connections.get(userId);
+  constructor() {
+    this.connections = new Map<string, string>();
+  }
+
+  public init(server: HttpServer) {
+    this.wsServer = new SocketIOServer<
+      ClientToServerEvents,
+      ServerToClientEvents,
+      InterServerEvents,
+      SocketData
+    >(server, {
+      cors: {
+        origin: ENV.ORIGINS_ALLOWED,
+        credentials: true,
+      },
+    });
+
+    this.wsServer.use((socket, next) => {
+      if (socket.handshake) {
+        const userId = socket.handshake.query["userId"] as string;
+
+        if (!userId || userId === "undefined" || userId === "null") {
+          const err = error(SocketError.InvalidSocketId, "Connection closed");
+          logger.error("WebSocket Error:\n\t", err);
+          return next(err);
+        }
+      }
 
-  if (!socketId) {
-    logger.warn(
-      `Event update not sent to client due to missing userId: ${userId}`
+      next();
+    });
+
+    this.wsServer.on(
+      "connection",
+      handleWsError((socket) => {
+        const userId = socket.handshake.query["userId"] as SocketData["userId"];
+
+        logger.debug(`Connection made to: ${userId}`);
+        this.connections.set(userId, socket.id);
+        console.log(this.connections);
+
+        socket.on(
+          "disconnect",
+          handleWsError(() => {
+            logger.debug(`Disconnecting from: ${userId}`);
+            this.connections.delete(userId);
+          })
+        );
+
+        socket.on(
+          EVENT_CHANGE_PROCESSED,
+          handleWsError((clientId) => {
+            logger.debug(`Client successfully processed updated: ${clientId}`);
+          })
+        );
+      })
     );
-    throw error(SocketError.InvalidSocketId, "Event update not sent to client");
+
+    this.wsServer.engine.on("connection_error", (err: Error) => {
+      logger.error(`Connection error: ${err.message}`);
+    });
+
+    return this.wsServer;
   }
 
-  socketServer.to(socketId).emit(EVENT_CHANGED);
-};
-
-export const initWebsocketServer = (server: HttpServer) => {
-  wsServer = new SocketIOServer<
-    ClientToServerEvents,
-    ServerToClientEvents,
-    InterServerEvents,
-    SocketData
-  >(server, {
-    cors: {
-      origin: ENV.ORIGINS_ALLOWED,
-      credentials: true,
-    },
-  });
-
-  wsServer.use((socket, next) => {
-    if (socket.handshake) {
-      const userId = socket.handshake.query["userId"] as string;
-
-      if (!userId || userId === "undefined" || userId === "null") {
-        const err = error(SocketError.InvalidSocketId, "Connection closed");
-        logger.error("WebSocket Error:\n\t", err);
-        return next(err);
-      }
+  public handleBackgroundCalendarChange(userId: string) {
+    const socketId = this.connections.get(userId);
+
+    const isClientConnected = socketId !== undefined;
+    if (!isClientConnected) return RESULT_IGNORED;
+
+    this.notifyClient(socketId, EVENT_CHANGED);
+    return RESULT_NOTIFIED_CLIENT;
+  }
+
+  public addConnection(userId: string, socketId: string) {
+    this.connections.set(userId, socketId);
+  }
+
+  public getConnection(userId: string): string | undefined {
+    return this.connections.get(userId);
+  }
+
+  private notifyClient(socketId: string, event: keyof ServerToClientEvents) {
+    if (this.wsServer === undefined) {
+      throw error(SocketError.ServerNotReady, "Client not notified");
     }
+    this.wsServer.to(socketId).emit(event);
+  }
+}
 
-    next();
-  });
-
-  wsServer.on(
-    "connection",
-    handleWsError((socket) => {
-      const userId = socket.handshake.query["userId"] as SocketData["userId"];
-
-      logger.debug(`Connection made to: ${userId}`);
-      connections.set(userId, socket.id);
-      console.log(connections);
-
-      socket.on(
-        "disconnect",
-        handleWsError(() => {
-          logger.debug(`Disconnecting from: ${userId}`);
-          connections.delete(userId);
-        })
-      );
-
-      socket.on(
-        EVENT_CHANGE_PROCESSED,
-        handleWsError((clientId) => {
-          logger.debug(`Client successfully processed updated: ${clientId}`);
-        })
-      );
-    })
-  );
-
-  wsServer.engine.on("connection_error", (err: Error) => {
-    logger.error(`Connection error: ${err.message}`);
-  });
-
-  return wsServer;
-};
+export const webSocketServer = new WebSocketServer();
diff --git a/packages/backend/src/sync/controllers/sync.debug.controller.ts b/packages/backend/src/sync/controllers/sync.debug.controller.ts
index 9f3ca03b..c5350e2d 100644
--- a/packages/backend/src/sync/controllers/sync.debug.controller.ts
+++ b/packages/backend/src/sync/controllers/sync.debug.controller.ts
@@ -2,7 +2,7 @@ import { Request, Response } from "express";
 import { SessionRequest } from "supertokens-node/framework/express";
 import { BaseError } from "@core/errors/errors.base";
 import { Res_Promise, SReqBody } from "@backend/common/types/express.types";
-import { notifyClient } from "@backend/servers/websocket/websocket.server";
+import { webSocketServer } from "@backend/servers/websocket/websocket.server";
 
 import syncService from "../services/sync.service";
 import { getSync } from "../util/sync.queries";
@@ -19,7 +19,7 @@ class SyncDebugController {
       const endDate = new Date(startDate);
       endDate.setDate(startDate.getDate() + 1);
 
-      notifyClient(userId);
+      webSocketServer.handleBackgroundCalendarChange(userId);
       res.sendStatus(200);
     } catch (e) {
       console.error("Error during dispatch:", e);
diff --git a/packages/backend/src/sync/services/sync.service.ts b/packages/backend/src/sync/services/sync.service.ts
index 6b9d0d3a..73279ec4 100644
--- a/packages/backend/src/sync/services/sync.service.ts
+++ b/packages/backend/src/sync/services/sync.service.ts
@@ -17,7 +17,7 @@ import gcalService from "@backend/common/services/gcal/gcal.service";
 import mongoService from "@backend/common/services/mongo.service";
 import { error } from "@backend/common/errors/handlers/error.handler";
 import { findCompassUserBy } from "@backend/user/queries/user.queries";
-import { notifyClient } from "@backend/servers/websocket/websocket.server";
+import { webSocketServer } from "@backend/servers/websocket/websocket.server";
 
 import {
   assembleEventImports,
@@ -94,7 +94,7 @@ class SyncService {
 
     const response = await importEventsByCalendar(userId, syncInfo);
 
-    notifyClient(userId);
+    webSocketServer.handleBackgroundCalendarChange(userId);
 
     return response;
   };
diff --git a/packages/core/src/constants/websocket.constants.ts b/packages/core/src/constants/websocket.constants.ts
index 22743461..45d75f5b 100644
--- a/packages/core/src/constants/websocket.constants.ts
+++ b/packages/core/src/constants/websocket.constants.ts
@@ -1,3 +1,6 @@
 export const EVENT_CHANGE_PROCESSED = "EVENT_CHANGE_PROCESSED";
 export const EVENT_CHANGED = "EVENT_CHANGED";
 export const EVENT_RECEIVED = "EVENT_RECEIVED";
+
+export const RESULT_IGNORED = "IGNORED";
+export const RESULT_NOTIFIED_CLIENT = "NOTIFIED_CLIENT";


Test patch:
diff --git a/packages/backend/src/servers/websocket/websocket.server.errors.test.ts b/packages/backend/src/servers/websocket/websocket.server.errors.test.ts
index e2f381da..d6ba399d 100644
--- a/packages/backend/src/servers/websocket/websocket.server.errors.test.ts
+++ b/packages/backend/src/servers/websocket/websocket.server.errors.test.ts
@@ -4,8 +4,8 @@ import { io as ioc, type Socket as ClientSocket } from "socket.io-client";
 import { Server as IoServer } from "socket.io";
 import { SocketError } from "@backend/common/constants/error.constants";
 
-import { initWebsocketServer } from "./websocket.server";
 import { getServerUri } from "./websocket.util";
+import { WebSocketServer } from "./websocket.server";
 
 describe("WebSocket Server: Error Handling", () => {
   let httpServer: HttpServer;
@@ -15,7 +15,7 @@ describe("WebSocket Server: Error Handling", () => {
 
   beforeAll((done) => {
     httpServer = createServer();
-    wsServer = initWebsocketServer(httpServer);
+    wsServer = new WebSocketServer().init(httpServer);
     httpServer.listen(() => {
       serverUri = getServerUri(httpServer);
       done();
diff --git a/packages/backend/src/servers/websocket/websocket.server.test.ts b/packages/backend/src/servers/websocket/websocket.server.test.ts
index 94fa3995..2289ff00 100644
--- a/packages/backend/src/servers/websocket/websocket.server.test.ts
+++ b/packages/backend/src/servers/websocket/websocket.server.test.ts
@@ -8,39 +8,40 @@ import {
   EVENT_CHANGED,
 } from "@core/constants/websocket.constants";
 
-import { initWebsocketServer } from "./websocket.server";
 import { getServerUri } from "./websocket.util";
+import { WebSocketServer } from "./websocket.server";
 
 describe("WebSocket Server", () => {
   let httpServer: HttpServer;
-  let io: IoServer;
-  let serverSocket: ServerSocket;
-  let clientSocket: ClientSocket;
+  let wsServer: IoServer;
+  let client: ClientSocket;
+  let socket: ServerSocket;
 
   beforeAll((done) => {
     httpServer = createServer();
-    io = initWebsocketServer(httpServer);
+    wsServer = new WebSocketServer().init(httpServer);
     httpServer.listen(() => {
       const uri = getServerUri(httpServer);
       const userId = "testUser123";
 
-      clientSocket = ioc(uri, {
+      client = ioc(uri, {
         query: { userId },
       });
 
-      io.on("connection", (socket) => {
-        serverSocket = socket;
+      wsServer.on("connection", (_socket) => {
+        socket = _socket;
       });
 
-      clientSocket.on("connect", done);
+      client.on("connect", done);
     });
   });
 
   afterAll((done) => {
-    io.close()
+    wsServer
+      .close()
       .then(() => httpServer.close())
       .then(() => {
-        clientSocket.disconnect();
+        client.disconnect();
         done();
       })
       .catch((err) => {
@@ -50,29 +51,27 @@ describe("WebSocket Server", () => {
   });
 
   describe(EVENT_CHANGED, () => {
-    it("emits event payload to client", (done) => {
-      clientSocket.on(EVENT_CHANGED, (arg) => {
+    it("emits eventpayload  to client", (done) => {
+      client.on(EVENT_CHANGED, (arg) => {
         expect(arg).toEqual({ _id: "1", title: "Test Event" });
         done();
       });
 
       const event: Schema_Event = { _id: "1", title: "Test Event" };
-      serverSocket.emit(EVENT_CHANGED, event);
+      socket.emit(EVENT_CHANGED, event);
     });
-
-    it.todo("emits eventChanged after reciving updated event data from Gcal");
   });
 
   describe(EVENT_CHANGE_PROCESSED, () => {
     it("accepts message from client after successful update", (done) => {
       const userId = "client123";
 
-      serverSocket.on(EVENT_CHANGE_PROCESSED, (receivedUserId) => {
+      socket.on(EVENT_CHANGE_PROCESSED, (receivedUserId) => {
         expect(receivedUserId).toBe(userId);
         done();
       });
 
-      clientSocket.emit(EVENT_CHANGE_PROCESSED, userId);
+      client.emit(EVENT_CHANGE_PROCESSED, userId);
     });
   });
 });
diff --git a/packages/backend/src/servers/websocket/websocket.util.test.ts b/packages/backend/src/servers/websocket/websocket.util.test.ts
index 93c35af0..19f247ac 100644
--- a/packages/backend/src/servers/websocket/websocket.util.test.ts
+++ b/packages/backend/src/servers/websocket/websocket.util.test.ts
@@ -1,38 +1,56 @@
 import { Server as SocketIOServer } from "socket.io";
+import { createServer, Server as HttpServer } from "http";
 import { CompassSocketServer } from "@core/types/websocket.types";
 import { EVENT_CHANGED } from "@core/constants/websocket.constants";
-import { SocketError } from "@backend/common/constants/error.constants";
 
-import { connections, notifyClient } from "./websocket.server";
+import { WebSocketServer } from "./websocket.server";
 
 jest.mock("socket.io");
 
-describe("emitEventToUser", () => {
-  beforeEach(() => {
-    (SocketIOServer as unknown as jest.Mock).mockClear();
-  });
+describe("handleBackgroundCalendarChange", () => {
+  let mockHttpServer: jest.Mocked<HttpServer>;
+  let mockIo: jest.Mocked<CompassSocketServer>;
+  let webSocketServer: WebSocketServer;
 
-  it("emits event to the correct socketId", () => {
-    const userId = "existingUser";
-    const socketId = "socket123";
-    const mockIo = {
+  beforeAll(() => {
+    mockIo = {
       to: jest.fn().mockReturnThis(),
       emit: jest.fn(),
-    };
-
+      use: jest.fn(),
+      on: jest.fn(),
+      engine: { on: jest.fn() },
+    } as unknown as jest.Mocked<CompassSocketServer>;
     (SocketIOServer as unknown as jest.Mock).mockImplementation(() => mockIo);
 
-    connections.set(userId, socketId);
-    notifyClient(userId, mockIo as unknown as CompassSocketServer);
+    webSocketServer = new WebSocketServer();
 
+    mockHttpServer = createServer() as jest.Mocked<HttpServer>;
+    webSocketServer.init(mockHttpServer);
+  });
+
+  afterAll(() => {
+    jest.clearAllMocks();
+  });
+
+  it("emits event to the correct socketId", (): void => {
+    const httpServer = createServer();
+    webSocketServer.init(httpServer);
+
+    const userId = "existingUser";
+    const socketId = "socket123";
+    webSocketServer.addConnection(userId, socketId);
+    webSocketServer.handleBackgroundCalendarChange(userId);
+
+    // eslint-disable-next-line @typescript-eslint/unbound-method
     expect(mockIo.to).toHaveBeenCalledWith(socketId);
+    // eslint-disable-next-line @typescript-eslint/unbound-method
     expect(mockIo.emit).toHaveBeenCalledWith(EVENT_CHANGED);
   });
-  it("throws error if socketId not found", () => {
+  it("ignores change if no connection between client and ws server", () => {
     const userId = "nonexistentUser";
 
-    expect(() => notifyClient(userId)).toThrow(
-      SocketError.InvalidSocketId.description
-    );
+    const result = webSocketServer.handleBackgroundCalendarChange(userId);
+
+    expect(result).toEqual("IGNORED");
   });
 });