From 12c22bc1b561198afcad5664a44d4fc584500682 Mon Sep 17 00:00:00 2001
From: Kishore <42832651+kishore03109@users.noreply.github.com>
Date: Thu, 7 Mar 2024 15:08:37 +0800
Subject: [PATCH] fix(api): fix media route timeout (#1170)

## Problem

We had an issue where a request such as "media/images%20blah/pages"  timeing out, see incident [here](https://opengovproducts.slack.com/archives/CK68JNFHR/p1708651983715799). while we do have an overall catch all in our main router, there are quite a number of calls in our subrouter that fails.

## Solution

for the media subrouter, this should return a 404. this is not applied across all the routes as some of the routes depend on the fall through behaviour to function. therefore, this pr is only scoped to return only for the media subrouter

# Other solutions that did not work
1. adding a `next()` to every subrouter
2. adding an error handler to every subrouter such that if subrouter captures a group and does not handle it, an error should be thrown. unfortunately, this caused quite a number of functionality to break, rather this hotfix go in first while and revisit this when we get timeouts again

**Breaking Changes**

<!-- Does this PR contain any backward incompatible changes? If so, what are they and should there be special considerations for release? -->

- [ ] Yes - this PR contains breaking changes
  - Details ...
- [X] No - this PR is backwards compatible with ALL of the following feature flags in this [doc](https://www.notion.so/opengov/Existing-feature-flags-518ad2cdc325420893a105e88c432be5)



## Tests
- [ ] Get call an authenticated request (ie you have assess to the site with the relevant valid cookie) to the backend
```
GET http://localhost:8081/v2/sites/kishore-test-dev-gh/media/images%2Fblah.png/pages
```
- [ ] Assert that you receive a res not found error rather than a timeout

<img width="1211" alt="Screenshot 2024-03-04 at 8 21 16 AM" src="https://github.com/isomerpages/isomercms-backend/assets/42832651/a88d2115-d4fc-47d3-b264-fbb5b1138953">
---
 src/errors/RouteNotFoundError.ts                    | 11 +++++++++++
 src/middleware/catchNonExistentRouteHandler.ts      | 13 +++++++++++++
 .../v2/authenticatedSites/__tests__/Media.spec.ts   |  9 +++++++++
 src/routes/v2/authenticatedSites/media.ts           |  3 +++
 4 files changed, 36 insertions(+)
 create mode 100644 src/errors/RouteNotFoundError.ts
 create mode 100644 src/middleware/catchNonExistentRouteHandler.ts

diff --git a/src/errors/RouteNotFoundError.ts b/src/errors/RouteNotFoundError.ts
new file mode 100644
index 000000000..fbeeee926
--- /dev/null
+++ b/src/errors/RouteNotFoundError.ts
@@ -0,0 +1,11 @@
+import { BaseIsomerError } from "./BaseError"
+
+export default class RouteNotFoundError extends BaseIsomerError {
+  constructor(meta = {}) {
+    super({
+      status: 404,
+      code: "Route Not Found Error",
+      meta,
+    })
+  }
+}
diff --git a/src/middleware/catchNonExistentRouteHandler.ts b/src/middleware/catchNonExistentRouteHandler.ts
new file mode 100644
index 000000000..087427718
--- /dev/null
+++ b/src/middleware/catchNonExistentRouteHandler.ts
@@ -0,0 +1,13 @@
+import { NextFunction, Request, Response } from "express"
+
+import RouteNotFoundError from "@root/errors/RouteNotFoundError"
+
+export const catchNonExistentRoutesMiddleware = (
+  req: Request,
+  res: Response,
+  next: NextFunction
+) => {
+  next(new RouteNotFoundError())
+}
+
+export default catchNonExistentRoutesMiddleware
diff --git a/src/routes/v2/authenticatedSites/__tests__/Media.spec.ts b/src/routes/v2/authenticatedSites/__tests__/Media.spec.ts
index d891b7569..9735d12e6 100644
--- a/src/routes/v2/authenticatedSites/__tests__/Media.spec.ts
+++ b/src/routes/v2/authenticatedSites/__tests__/Media.spec.ts
@@ -7,6 +7,7 @@ import {
   mockGithubSessionData,
 } from "@root/fixtures/sessionData"
 import { MOCK_REPO_NAME_ONE } from "@root/fixtures/sites"
+import catchNonExistentRoutesMiddleware from "@root/middleware/catchNonExistentRouteHandler"
 import { attachReadRouteHandlerWrapper } from "@root/middleware/routeHandler"
 import { MediaDirectoryService } from "@root/services/directoryServices/MediaDirectoryService"
 import { MediaFileService } from "@root/services/fileServices/MdPageServices/MediaFileService"
@@ -80,6 +81,8 @@ describe("Media Router", () => {
     attachReadRouteHandlerWrapper(router.deleteMediaFile)
   )
 
+  subrouter.use(catchNonExistentRoutesMiddleware)
+
   const app = generateRouterForDefaultUserWithSite(subrouter)
   const siteName = MOCK_REPO_NAME_ONE
   const directoryName = "directoryName"
@@ -438,4 +441,10 @@ describe("Media Router", () => {
       )
     })
   })
+
+  describe("catchNonExistentRoutes", () => {
+    it("returns 404 for non-existent routes", async () => {
+      await request(app).get(`/${siteName}/media/imagesblah/pages`).expect(404)
+    })
+  })
 })
diff --git a/src/routes/v2/authenticatedSites/media.ts b/src/routes/v2/authenticatedSites/media.ts
index 3d09d9e87..5f20c61e1 100644
--- a/src/routes/v2/authenticatedSites/media.ts
+++ b/src/routes/v2/authenticatedSites/media.ts
@@ -11,6 +11,7 @@ import {
 import GithubSessionData from "@classes/GithubSessionData"
 import UserWithSiteSessionData from "@classes/UserWithSiteSessionData"
 
+import { catchNonExistentRoutesMiddleware } from "@root/middleware/catchNonExistentRouteHandler"
 import type {
   MediaDirOutput,
   MediaFileOutput,
@@ -445,6 +446,8 @@ export class MediaRouter {
       attachRollbackRouteHandlerWrapper(this.deleteMediaFile)
     )
 
+    router.use(catchNonExistentRoutesMiddleware)
+
     return router
   }
 }