From 379ffa22acee319a911220d1b4bfd624e49cb87f Mon Sep 17 00:00:00 2001 From: Kaushik Narayan R Date: Mon, 5 Aug 2024 23:59:09 +0530 Subject: [PATCH] refactor: one-at-a-time spotify api calls, some other modifications and logging reordering --- boilerplates/controller.js | 3 +- controllers/apiCall.js | 53 +++++++++++++++++ controllers/operations.js | 119 ++++++++++++++++++++----------------- controllers/playlists.js | 16 +++-- middleware/authCheck.js | 10 ++-- typedefs.js | 3 + utils/axios.js | 35 +---------- 7 files changed, 140 insertions(+), 99 deletions(-) create mode 100644 controllers/apiCall.js diff --git a/boilerplates/controller.js b/boilerplates/controller.js index e6578c4..94594e8 100644 --- a/boilerplates/controller.js +++ b/boilerplates/controller.js @@ -10,8 +10,9 @@ const __controller_func = async (req, res) => { try { } catch (error) { + res.sendStatus(500); logger.error('__controller_func', { error }); - return res.sendStatus(500); + return; } } diff --git a/controllers/apiCall.js b/controllers/apiCall.js new file mode 100644 index 0000000..5588616 --- /dev/null +++ b/controllers/apiCall.js @@ -0,0 +1,53 @@ + +const logger = require("../utils/logger")(module); + +const typedefs = require("../typedefs"); + +const { axiosInstance } = require("../utils/axios"); + +/** + * Spotify API - one-off request handler + * @param {typedefs.Req} req needed for auto-placing headers from middleware + * @param {typedefs.Res} res handle failure responses here itself + * @param {typedefs.AxiosMethod} method HTTP method + * @param {string} path request path + * @param {typedefs.AxiosRequestConfig} config request params, headers, etc. + * @param {any} data request body + * @param {boolean} inlineData true if data is to be placed inside config + * @returns {Promise<{ success: boolean, resp?: any }>} + */ +const singleRequest = async (req, res, method, path, config = {}, data = null, inlineData = false) => { + let resp; + config.headers = { ...config.headers, ...req.sessHeaders }; + try { + if (!data || (data && inlineData)) { + if (data) + config.data = data ?? null; + resp = await axiosInstance[method.toLowerCase()](path, config); + } else { + resp = await axiosInstance[method.toLowerCase()](path, data, config); + } + + if (resp.status >= 400 && resp.status < 500) { + res.status(resp.status).send(resp.data); + logger.debug("4XX Response", { resp }); + return { success: false }; + } + else if (resp.status >= 500) { + res.sendStatus(resp.status); + logger.warn("5XX Response", { resp }); + return { success: false }; + } + + logger.debug("Response received."); + return { success: true, resp }; + } catch (error) { + res.sendStatus(500); + logger.error("Request threw an error.", { error }); + return { success: false }; + } +} + +module.exports = { + singleRequest, +} \ No newline at end of file diff --git a/controllers/operations.js b/controllers/operations.js index 1aab18e..2f66a30 100644 --- a/controllers/operations.js +++ b/controllers/operations.js @@ -6,6 +6,7 @@ const myGraph = require("../utils/graph"); const { parseSpotifyLink } = require("../utils/spotifyURITransformer"); const { Op } = require("sequelize"); +const { singleRequest } = require("./apiCall"); /** @type {typedefs.Model} */ const Playlists = require("../models").playlists; /** @type {typedefs.Model} */ @@ -344,11 +345,14 @@ const populateMissingInLink = async (req, res) => { fromPl = parseSpotifyLink(req.body["from"]); toPl = parseSpotifyLink(req.body["to"]); if (fromPl.type !== "playlist" || toPl.type !== "playlist") { - return res.status(400).send({ message: "Link is not a playlist" }); + res.status(400).send({ message: "Link is not a playlist" }); + logger.warn("non-playlist link provided", { from: fromPl, to: toPl }); + return; } } catch (error) { + res.status(400).send({ message: "Invalid Spotify playlist link" }); logger.error("parseSpotifyLink", { error }); - return res.status(400).send({ message: "Invalid Spotify playlist link" }); + return; } // check if exists @@ -362,32 +366,33 @@ const populateMissingInLink = async (req, res) => { } }); if (!existingLink) { + res.sendStatus(409); logger.error("link does not exist"); - return res.sendStatus(409); + return; } let checkFields = ["collaborative", "owner(id)"]; - const checkFromData = await axiosInstance.get( + + const checkResp = await singleRequest(req, res, + "GET", `/playlists/${fromPl.id}/`, { params: { fields: checkFields.join() }, - headers: req.sessHeaders - } - ); - if (checkFromData.status >= 400 && checkFromData.status < 500) - return res.status(checkFromData.status).send(checkFromData.data); - else if (checkFromData.status >= 500) - return res.sendStatus(checkFromData.status); + }); + if (!checkResp.success) return; + + const checkFromData = checkResp.resp.data; // editable = collaborative || user is owner - if (checkFromData.data.collaborative !== true && - checkFromData.data.owner.id !== uID) { - logger.error("user cannot edit target playlist"); - return res.status(403).send({ + if (checkFromData.collaborative !== true && + checkFromData.owner.id !== uID) { + res.status(403).send({ message: "You cannot edit this playlist, you must be owner/playlist must be collaborative" }); + logger.error("user cannot edit target playlist"); + return; } let initialFields = ["tracks(next,items(is_local,track(uri)))"]; @@ -508,25 +513,25 @@ const populateMissingInLink = async (req, res) => { const logNum = toTrackURIs.length; - // add in batches of 100 + // append to end in batches of 100 while (toTrackURIs.length) { const nextBatch = toTrackURIs.splice(0, 100); - const addResponse = await axiosInstance.post( + const addResponse = await singleRequest(req, res, + "POST", `/playlists/${fromPl.id}/tracks`, - { uris: nextBatch }, - { headers: req.sessHeaders } + {}, + { uris: nextBatch }, false ); - if (addResponse.status >= 400 && addResponse.status < 500) - return res.status(addResponse.status).send(addResponse.data); - else if (addResponse.status >= 500) - return res.sendStatus(addResponse.status); + if (!addResponse.success) return; } + res.status(200).send({ message: `Added ${logNum} tracks.` }); logger.info(`Backfilled ${logNum} tracks`); - return res.sendStatus(200); + return; } catch (error) { + res.sendStatus(500); logger.error('populateMissingInLink', { error }); - return res.sendStatus(500); + return; } } @@ -556,11 +561,14 @@ const pruneExcessInLink = async (req, res) => { fromPl = parseSpotifyLink(req.body["from"]); toPl = parseSpotifyLink(req.body["to"]); if (fromPl.type !== "playlist" || toPl.type !== "playlist") { - return res.status(400).send({ message: "Link is not a playlist" }); + res.status(400).send({ message: "Link is not a playlist" }); + logger.warn("non-playlist link provided", { from: fromPl, to: toPl }); + return } } catch (error) { + res.status(400).send({ message: "Invalid Spotify playlist link" }); logger.error("parseSpotifyLink", { error }); - return res.status(400).send({ message: "Invalid Spotify playlist link" }); + return; } // check if exists @@ -574,32 +582,33 @@ const pruneExcessInLink = async (req, res) => { } }); if (!existingLink) { + res.sendStatus(409); logger.error("link does not exist"); - return res.sendStatus(409); + return } let checkFields = ["collaborative", "owner(id)"]; - const checkToData = await axiosInstance.get( + + const checkToResp = await singleRequest(req, res, + "GET", `/playlists/${toPl.id}/`, { params: { fields: checkFields.join() }, - headers: req.sessHeaders - } - ); - if (checkToData.status >= 400 && checkToData.status < 500) - return res.status(checkToData.status).send(checkToData.data); - else if (checkToData.status >= 500) - return res.sendStatus(checkToData.status); + }); + + if (!checkToResp.success) return; + const checkToData = checkToResp.resp.data; // editable = collaborative || user is owner - if (checkToData.data.collaborative !== true && - checkToData.data.owner.id !== uID) { - logger.error("user cannot edit target playlist"); - return res.status(403).send({ + if (checkToData.collaborative !== true && + checkToData.owner.id !== uID) { + res.status(403).send({ message: "You cannot edit this playlist, you must be owner/playlist must be collaborative" }); + logger.error("user cannot edit target playlist"); + return; } let initialFields = ["snapshot_id", "tracks(next,items(is_local,track(uri)))"]; @@ -717,39 +726,37 @@ const pruneExcessInLink = async (req, res) => { const fromTrackURIs = fromPlaylist.tracks.map(track => track.uri); let indexedToTrackURIs = toPlaylist.tracks; - // forEach doesn't execute in given order, not sure what it uses to order indexedToTrackURIs.forEach((track, index) => { track.position = index; }); - let indexes = indexedToTrackURIs.filter(track => !fromTrackURIs.includes(track.uri)); // only ones missing from the 'from' playlist + let indexes = indexedToTrackURIs.filter(track => !fromTrackURIs.includes(track.uri)); // only those missing from the 'from' playlist indexes = indexes.map(track => track.position); // get track positions const logNum = indexes.length; - // remove in batches of 100 (from reverse, to preserve positions) + // remove in batches of 100 (from reverse, to preserve positions while modifying) let currentSnapshot = toPlaylist.snapshot_id; while (indexes.length) { const nextBatch = indexes.splice(Math.max(indexes.length - 100, 0), 100); - const delResponse = await axiosInstance.delete( + const delResponse = await singleRequest(req, res, + "DELETE", `/playlists/${toPl.id}/tracks`, - { - headers: req.sessHeaders, - data: { positions: nextBatch, snapshot_id: currentSnapshot }, // delete method doesn't have separate arg for body - } - ); - if (delResponse.status >= 400 && delResponse.status < 500) - return res.status(delResponse.status).send(delResponse.data); - else if (delResponse.status >= 500) - return res.sendStatus(delResponse.status); - currentSnapshot = delResponse.data.snapshot_id; + {}, + // axios delete method doesn't have separate arg for body so hv to put it in config + { positions: nextBatch, snapshot_id: currentSnapshot }, true + ) + if (!delResponse.success) return; + currentSnapshot = delResponse.resp.data.snapshot_id; } + res.status(200).send({ message: `Removed ${logNum} tracks.` }); logger.info(`Pruned ${logNum} tracks`); - return res.sendStatus(200); + return; } catch (error) { + res.sendStatus(500); logger.error('pruneExcessInLink', { error }); - return res.sendStatus(500); + return; } } diff --git a/controllers/playlists.js b/controllers/playlists.js index e753ddb..843f7a8 100644 --- a/controllers/playlists.js +++ b/controllers/playlists.js @@ -71,8 +71,9 @@ const getUserPlaylists = async (req, res) => { delete userPlaylists.next; + res.status(200).send(userPlaylists); logger.debug("Fetched user's playlists", { num: userPlaylists.total }); - return res.status(200).send(userPlaylists); + return; } catch (error) { logger.error('getUserPlaylists', { error }); return res.sendStatus(500); @@ -97,11 +98,14 @@ const getPlaylistDetails = async (req, res) => { try { uri = parseSpotifyLink(req.query.playlist_link) if (uri.type !== "playlist") { - return res.status(400).send({ message: "Link is not a playlist" }); + res.status(400).send({ message: "Link is not a playlist" }); + logger.warn("non-playlist link provided", { uri }); + return; } } catch (error) { + res.status(400).send({ message: "Invalid Spotify playlist link" }); logger.error("parseSpotifyLink", { error }); - return res.status(400).send({ message: "Invalid Spotify playlist link" }); + return; } const response = await axiosInstance.get( @@ -178,11 +182,13 @@ const getPlaylistDetails = async (req, res) => { delete playlist.next; + res.status(200).send(playlist); logger.info("Fetched playlist tracks", { num: playlist.tracks.length }); - return res.status(200).send(playlist); + return; } catch (error) { + res.sendStatus(500); logger.error('getPlaylistDetails', { error }); - return res.sendStatus(500); + return; } } diff --git a/middleware/authCheck.js b/middleware/authCheck.js index 9636031..f30d592 100644 --- a/middleware/authCheck.js +++ b/middleware/authCheck.js @@ -18,17 +18,19 @@ const isAuthenticated = (req, res, next) => { } else { const delSession = req.session.destroy((err) => { if (err) { + res.sendStatus(500); logger.error("Error while destroying session.", { err }); - return res.sendStatus(500); + return; } else { - logger.info("Session invalid, destroyed.", { sessionID: delSession.id }); res.clearCookie(sessionName); - return res.sendStatus(401); + res.sendStatus(401); + logger.debug("Session invalid, destroyed.", { sessionID: delSession.id }); + return; } }); } } module.exports = { - isAuthenticated + isAuthenticated, } diff --git a/typedefs.js b/typedefs.js index e2f8548..e2ade3e 100644 --- a/typedefs.js +++ b/typedefs.js @@ -5,6 +5,9 @@ * @typedef {import('express').Response} Res * @typedef {import('express').NextFunction} Next * + * @typedef {import('axios').Method} AxiosMethod + * @typedef {import('axios').AxiosRequestConfig} AxiosRequestConfig + * * @typedef {import("sequelize").Sequelize} Sequelize * @typedef {import("sequelize").Model} Model * @typedef {import("sequelize").QueryInterface} QueryInterface diff --git a/utils/axios.js b/utils/axios.js index 5b74d35..92b4aab 100644 --- a/utils/axios.js +++ b/utils/axios.js @@ -32,40 +32,9 @@ axiosInstance.interceptors.request.use(config => { axiosInstance.interceptors.response.use( (response) => response, (error) => { - if (error.response && error.response.status === 429) { - // Rate limiting - logger.warn("Spotify API: Too many requests"); + logger.warn("AxiosError", { req: error.config }); + if (error.response) return error.response; - } - else if (error.response) { - // Server has responded - logger.error( - "Spotify API: Error", { - response: { - status: error.response.status, - statusText: error.response.statusText, - data: error.response.data - } - }); - return error.response; - } else if (error.request) { - // The request was made but no response was received - logger.error( - "Spotify API: No response", { - request: { - url: error.request?.url, - params: error.request?.params, - } - }); - } else { - // Something happened in setting up the request that triggered an Error - logger.error( - "Spotify API: Request error", { - error: { - message: error.message, - } - }); - } return Promise.reject(error); } );