Skip to content

Commit

Permalink
fix(api): fix media route timeout (#1170)
Browse files Browse the repository at this point in the history
## 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">
  • Loading branch information
kishore03109 authored Mar 7, 2024
1 parent 4bac8cb commit 12c22bc
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 0 deletions.
11 changes: 11 additions & 0 deletions src/errors/RouteNotFoundError.ts
Original file line number Diff line number Diff line change
@@ -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,
})
}
}
13 changes: 13 additions & 0 deletions src/middleware/catchNonExistentRouteHandler.ts
Original file line number Diff line number Diff line change
@@ -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
9 changes: 9 additions & 0 deletions src/routes/v2/authenticatedSites/__tests__/Media.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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)
})
})
})
3 changes: 3 additions & 0 deletions src/routes/v2/authenticatedSites/media.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -445,6 +446,8 @@ export class MediaRouter {
attachRollbackRouteHandlerWrapper(this.deleteMediaFile)
)

router.use(catchNonExistentRoutesMiddleware)

return router
}
}

0 comments on commit 12c22bc

Please sign in to comment.