Skip to content

Commit be2c765

Browse files
thehabesthehabes
authored
Error Catching and Logging In Routes (#423)
* Error catching and logging * touchup * small tweak from testing and reviewing * Stop this unhandled 500 * Stop this 500 * 500s from other routes * Error handler as a middleware util * stop this 500 * Stop that 500 * returns * returns * use respondWithError in app.js too * touchup * touchup * touchup * touchup from copilot review * touchup from copilot review * touchup from copilot review * oof too much of a logic change, this is what we actually wanted.
1 parent 2dae7bf commit be2c765

22 files changed

Lines changed: 328 additions & 236 deletions

‎app.js‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@ import userProfileRouter from './userProfile/index.js'
2121
import privateProfileRouter from './userProfile/privateProfile.js'
2222
import proxyRouter from './utilities/proxy.js'
2323
import feedbackRouter from './feedback/feedbackRoutes.js'
24+
import routeErrorHandler from './utilities/routeErrorHandler.js'
25+
import { respondWithError } from './utilities/shared.js'
2426

2527
let app = express()
2628

@@ -65,10 +67,11 @@ app.use(express.static(path.join(__dirname, 'public')))
6567
*/
6668
app.all('*_', (req, res, next) => {
6769
if (process.env.DOWN === 'true') {
68-
return res.status(503).json({
69-
message:
70-
'TPEN3 services are down for updates or maintenance at this time. We apologize for the inconvenience. Try again later.'
71-
})
70+
return respondWithError(
71+
res,
72+
503,
73+
'TPEN3 services are down for updates or maintenance at this time. We apologize for the inconvenience. Try again later.'
74+
)
7275
}
7376
next()
7477
})
@@ -82,9 +85,12 @@ app.use('/my', privateProfileRouter)
8285
app.use('/proxy', proxyRouter)
8386
app.use('/beta', feedbackRouter)
8487

88+
// Centralized error handling middleware
89+
app.use(routeErrorHandler)
90+
8591
//catch 404 because of an invalid site path
8692
app.use('*_', (req, res) => {
87-
res.status(404).json({ message: res.statusMessage ?? 'This page does not exist' })
93+
return respondWithError(res, 404, res.statusMessage ?? 'This page does not exist')
8894
})
8995

9096
export { app as default }

‎auth/index.js‎

Lines changed: 55 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -30,70 +30,67 @@ function auth0Middleware() {
3030
try {
3131
const uid = agent.split("id/")[1]
3232
const user = new User(uid)
33-
user.getSelf().then(async (u) => {
34-
if(!u || !u?.profile) {
35-
const email = payload.name
33+
const u = await user.getSelf()
34+
if(!u || !u?.profile) {
35+
const email = payload.name
3636

37-
// Check if a temporary user exists with this email
38-
let existingUser = null
39-
try {
40-
existingUser = await user.getByEmail(email)
41-
} catch (err) {
42-
// No user found - that's fine, continue
43-
}
37+
// Check if a temporary user exists with this email
38+
let existingUser = null
39+
try {
40+
existingUser = await user.getByEmail(email)
41+
} catch (err) {
42+
// No user found - that's fine, continue
43+
}
4444

45-
if (existingUser && existingUser.inviteCode) {
46-
// Found a temporary user - merge their memberships into this new user
47-
user.data = {
48-
_id: uid,
49-
agent,
50-
_sub: payload.sub,
51-
email: email,
52-
profile: { displayName: payload.nickname },
53-
}
54-
await user.mergeFromTemporaryUser(existingUser)
55-
await user.save()
56-
req.user = user
57-
next()
58-
return
59-
} else if (existingUser) {
60-
// Non-temporary user with same email - this is a conflict
61-
const err = new Error(`User with email ${email} already exists. Please contact TPEN3 administrators for assistance.`)
62-
err.status = 409
63-
next(err)
64-
return
65-
} else {
66-
// No existing user - create new
67-
user.data = {
68-
_id: uid,
69-
agent,
70-
_sub: payload.sub,
71-
email: email,
72-
profile: { displayName: payload.nickname },
73-
}
74-
await user.save()
75-
req.user = user
76-
next()
77-
return
45+
if (existingUser && existingUser.inviteCode) {
46+
// Found a temporary user - merge their memberships into this new user
47+
user.data = {
48+
_id: uid,
49+
agent,
50+
_sub: payload.sub,
51+
email: email,
52+
profile: { displayName: payload.nickname },
53+
}
54+
await user.mergeFromTemporaryUser(existingUser)
55+
await user.save()
56+
req.user = user
57+
next()
58+
return
59+
} else if (existingUser) {
60+
// Non-temporary user with same email - this is a conflict
61+
const err = new Error(`User with email ${email} already exists. Please contact TPEN3 administrators for assistance.`)
62+
err.status = 409
63+
next(err)
64+
return
65+
} else {
66+
// No existing user - create new
67+
user.data = {
68+
_id: uid,
69+
agent,
70+
_sub: payload.sub,
71+
email: email,
72+
profile: { displayName: payload.nickname },
7873
}
74+
await user.save()
75+
req.user = user
76+
next()
77+
return
7978
}
79+
}
80+
// Ensure no inviteCode on authenticated user
81+
delete u.inviteCode
8082

81-
// If user exists but has wrong _sub (e.g., from temp user), update it
82-
if (u._sub !== payload.sub) {
83-
u._sub = payload.sub
84-
// Remove inviteCode if present - this user is now fully authenticated
85-
delete u.inviteCode
86-
const userObj = new User(uid)
87-
userObj.data = u
88-
await userObj.update()
89-
}
83+
// If user exists but has wrong _sub (e.g., from temp user), update it
84+
if (u._sub !== payload.sub) {
85+
u._sub = payload.sub
86+
const userObj = new User(uid)
87+
userObj.data = u
88+
await userObj.update()
89+
}
9090

91-
// Ensure no inviteCode on authenticated user (belt and suspenders)
92-
delete u.inviteCode
93-
req.user = u
94-
next()
95-
return
96-
})
91+
req.user = u
92+
next()
93+
return
9794
} catch (error) {
9895
next(error)
9996
}

‎feedback/feedbackController.js‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,9 @@ import { isSuspiciousValueString } from "../utilities/checkIfSuspicious.js"
1010
* @returns 200 if feedback is submitted successfully, 204 if no feedback is provided, or an error response if the submission fails.
1111
*/
1212
export async function submitFeedback(req, res) {
13+
if (!req.body || typeof req.body !== 'object') {
14+
return respondWithError(res, 400, "Request body is required")
15+
}
1316
const user = req.user ? `${req.user.profile.displayName} (${req.user._id})` : 'Anonymous'
1417
const { page, feedback } = req.body
1518
if (!feedback) return res.status(204).send()
@@ -18,7 +21,7 @@ export async function submitFeedback(req, res) {
1821
await createGitHubIssue('Feedback', `Feedback from ${user}`, `Page: ${page}\n\nFeedback: ${sanitizeUserInput(feedback)}`)
1922
res.status(200).json({ message: 'Feedback submitted successfully' })
2023
} catch (error) {
21-
respondWithError(res, error.status ?? 500, error.message ?? 'Failed to submit feedback')
24+
return respondWithError(res, error.status ?? 500, error.message ?? 'Failed to submit feedback')
2225
}
2326
}
2427

@@ -30,6 +33,9 @@ export async function submitFeedback(req, res) {
3033
* @returns 200 if the bug report is submitted successfully, 204 if no bug description is provided, or an error response if the submission fails.
3134
*/
3235
export async function submitBug(req, res) {
36+
if (!req.body || typeof req.body !== 'object') {
37+
return respondWithError(res, 400, "Request body is required")
38+
}
3339
const user = req.user ? `${req.user.profile.displayName} (${req.user._id})` : 'Anonymous'
3440
const { page, bugDescription } = req.body
3541
if (!bugDescription) return res.status(204).send()
@@ -38,7 +44,7 @@ export async function submitBug(req, res) {
3844
await createGitHubIssue('Bug Report', `Bug reported by ${user}`, `Page: ${page}\n\nBug: ${sanitizeUserInput(bugDescription)}`)
3945
res.status(200).json({ message: 'Bug report submitted successfully' })
4046
} catch (error) {
41-
respondWithError(res, error.status ?? 500, error.message ??'Failed to submit bug report')
47+
return respondWithError(res, error.status ?? 500, error.message ??'Failed to submit bug report')
4248
}
4349
}
4450

‎index.js‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,7 @@ router
3434
.get(function (_req,res) {
3535
fs.readFile('API.md', 'utf8', (err, data) => {
3636
if (err) {
37-
respondWithError(res, 500, 'Failed to read API.md')
38-
return
37+
return respondWithError(res, 500, 'Failed to read API.md')
3938
}
4039
res.format({
4140
html: () => res.send(makeCleanFileFromMarkdown(data))
@@ -49,7 +48,7 @@ router
4948
respondWithHTML(res)
5049
})
5150
.all((_req, res, next) => {
52-
respondWithError(res, 404, 'There is nothing for you here.')
51+
return respondWithError(res, 404, 'There is nothing for you here.')
5352
})
5453

5554
export { router as default }

‎layer/index.js‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,7 @@ router.route('/:layerId')
2020
try {
2121
const layer = await findLayerById(layerId, projectId, true)
2222
if (!layer) {
23-
respondWithError(res, 404, 'No layer found with that ID.')
24-
return
23+
return respondWithError(res, 404, 'No layer found with that ID.')
2524
}
2625
if (layer.id?.startsWith(process.env.RERUMIDPREFIX)) {
2726
// If the page is a RERUM document, we need to fetch it from the server
@@ -70,9 +69,8 @@ router.route('/:layerId')
7069
else layer[key] = null
7170
}
7271
})
73-
if (providedPages?.length === 0) providedPages = undefined
7472
let pages = []
75-
if (providedPages && providedPages.length) {
73+
if (providedPages && Array.isArray(providedPages) && providedPages.length > 0) {
7674
pages = await Promise.all(providedPages.map(p => findPageById(p.split("/").pop(), projectId) ))
7775
layer.pages = pages
7876
}
@@ -84,12 +82,15 @@ router.route('/:layerId')
8482
}
8583
})
8684
.all((req, res) => {
87-
respondWithError(res, 405, 'Improper request method. Use GET instead.')
85+
return respondWithError(res, 405, 'Improper request method. Use GET instead.')
8886
})
8987

9088
// Route to create a new layer within a project
9189
router.route('/').post(auth0Middleware(), screenContentMiddleware(), async (req, res) => {
9290
const { projectId } = req.params
91+
if (!req.body || typeof req.body !== 'object') {
92+
return respondWithError(res, 400, 'Request body is required')
93+
}
9394
const { label, canvases } = req.body
9495
if (!projectId) return respondWithError(res, 400, 'Project ID is required')
9596
if (!label || !Array.isArray(canvases) || canvases.length === 0) {

0 commit comments

Comments
 (0)