From 7b659095225cef72d0696aff0ff7a993c59a5294 Mon Sep 17 00:00:00 2001 From: Oliver Eyton-Williams Date: Sat, 5 Jun 2021 14:20:40 +0200 Subject: [PATCH] feat: make api only return json (#42320) * fix(api): return json for delete + reset-progress * refactor(api): make .json calls explicit These .send() calls had objects as arguments. Using .json() makes it explicit without changing behaviour. * fix: return json or redirectWithFlash We should never be getting requests for plain text, but if we do they should be redirected back to the client. * fix: prioritize JSON responses If accepted, respond with JSON. If not, it's probably a bad request. --- api-server/src/server/boot/challenge.js | 4 ++-- api-server/src/server/boot/donate.js | 4 ++-- api-server/src/server/boot/randomAPIs.js | 2 +- api-server/src/server/boot/user.js | 4 ++-- api-server/src/server/boot/z-not-found.js | 14 +++++-------- .../src/server/middlewares/error-handlers.js | 21 +++++++------------ 6 files changed, 20 insertions(+), 29 deletions(-) diff --git a/api-server/src/server/boot/challenge.js b/api-server/src/server/boot/challenge.js index a0bdcc5602..544c2ce395 100644 --- a/api-server/src/server/boot/challenge.js +++ b/api-server/src/server/boot/challenge.js @@ -286,7 +286,7 @@ function projectCompleted(req, res, next) { }) ); return Observable.fromPromise(updatePromise).doOnNext(() => { - return res.send({ + return res.json({ alreadyCompleted, points: alreadyCompleted ? user.points : user.points + 1, completedDate: completedChallenge.completedDate @@ -320,7 +320,7 @@ function backendChallengeCompleted(req, res, next) { }) ); return Observable.fromPromise(updatePromise).doOnNext(() => { - return res.send({ + return res.json({ alreadyCompleted, points: alreadyCompleted ? user.points : user.points + 1, completedDate: completedChallenge.completedDate diff --git a/api-server/src/server/boot/donate.js b/api-server/src/server/boot/donate.js index 40a39b8982..099908de47 100644 --- a/api-server/src/server/boot/donate.js +++ b/api-server/src/server/boot/donate.js @@ -20,7 +20,7 @@ export default function donateBoot(app, done) { if (!user || !body) { return res .status(500) - .send({ error: 'User must be signed in for this request.' }); + .json({ error: 'User must be signed in for this request.' }); } return Promise.resolve(req) .then( @@ -31,7 +31,7 @@ export default function donateBoot(app, done) { .then(() => res.status(200).json({ isDonating: true })) .catch(err => { log(err.message); - return res.status(500).send({ + return res.status(500).json({ type: 'danger', message: 'Something went wrong.' }); diff --git a/api-server/src/server/boot/randomAPIs.js b/api-server/src/server/boot/randomAPIs.js index 16c7b5f3f5..5d9d163248 100644 --- a/api-server/src/server/boot/randomAPIs.js +++ b/api-server/src/server/boot/randomAPIs.js @@ -193,7 +193,7 @@ module.exports = function (app) { pulls === parseInt(pulls, 10) && issues ? Object.keys(JSON.parse(issues)).length - pulls : "Can't connect to GitHub"; - return res.send({ + return res.json({ issues: issues, pulls: pulls }); diff --git a/api-server/src/server/boot/user.js b/api-server/src/server/boot/user.js index f361aca32d..1caec372ff 100644 --- a/api-server/src/server/boot/user.js +++ b/api-server/src/server/boot/user.js @@ -185,7 +185,7 @@ function postResetProgress(req, res, next) { if (err) { return next(err); } - return res.sendStatus(200); + return res.status(200).json({}); } ); } @@ -199,7 +199,7 @@ function createPostDeleteAccount(app) { } req.logout(); removeCookies(req, res); - return res.sendStatus(200); + return res.status(200).json({}); }); }; } diff --git a/api-server/src/server/boot/z-not-found.js b/api-server/src/server/boot/z-not-found.js index 310d9602cb..1c84138388 100644 --- a/api-server/src/server/boot/z-not-found.js +++ b/api-server/src/server/boot/z-not-found.js @@ -4,20 +4,16 @@ import { getRedirectParams } from '../utils/redirection'; export default function fourOhFour(app) { app.all('*', function (req, res) { const accept = accepts(req); - const type = accept.type('html', 'json', 'text'); + // prioritise returning json + const type = accept.type('json', 'html', 'text'); const { path } = req; const { origin } = getRedirectParams(req); - if (type === 'html') { + if (type === 'json') { + return res.status('404').json({ error: 'path not found' }); + } else { req.flash('danger', `We couldn't find path ${path}`); return res.redirectWithFlash(`${origin}/404`); } - - if (type === 'json') { - return res.status('404').json({ error: 'path not found' }); - } - - res.setHeader('Content-Type', 'text/plain'); - return res.send('404 path not found'); }); } diff --git a/api-server/src/server/middlewares/error-handlers.js b/api-server/src/server/middlewares/error-handlers.js index 03b6d6dbb5..573e817077 100644 --- a/api-server/src/server/middlewares/error-handlers.js +++ b/api-server/src/server/middlewares/error-handlers.js @@ -37,7 +37,8 @@ export default function prodErrorHandler() { // parse res type const accept = accepts(req); - const type = accept.type('html', 'json', 'text'); + // prioritise returning json + const type = accept.type('json', 'html', 'text'); const redirectTo = handled.redirectTo || `${origin}/`; const message = @@ -48,22 +49,16 @@ export default function prodErrorHandler() { console.error(errTemplate(err, req)); } - if (type === 'html') { + if (type === 'json') { + return res.json({ + type: handled.type || 'errors', + message + }); + } else { if (typeof req.flash === 'function') { req.flash(handled.type || 'danger', message); } return res.redirectWithFlash(redirectTo); - // json - } else if (type === 'json') { - res.setHeader('Content-Type', 'application/json'); - return res.send({ - type: handled.type || 'errors', - message - }); - // plain text - } else { - res.setHeader('Content-Type', 'text/plain'); - return res.send(message); } }; }