refactor: UserError -> ClientError, ServerError

ClientError will default to 400 HTTP error code.
ServerError will default to 500 HTTP error code.

Following the previous commit, these for now are only being used in
albumsController. More will soon follow.

Additionally fixed existing album names can sometimes be re-used when
editing an album.

controllers/albumsController.js

@@ -8,7 +8,8 @@ const perms = require('./permissionController')
 const uploadController = require('./uploadController')
 const utils = require('./utilsController')
 const apiErrorsHandler = require('./handlers/apiErrorsHandler.js')
-const UserError = require('./utils/UserError')
+const ClientError = require('./utils/ClientError')
+const ServerError = require('./utils/ServerError')
 const config = require('./../config')
 const logger = require('./../logger')
 const db = require('knex')(config.database)
@@ -68,7 +69,7 @@ self.getUniqueRandomName = async () => {
     return identifier
   }
 
-  throw new UserError('Failed to allocate a unique identifier for the album. Try again?', 500)
+  throw new ServerError('Failed to allocate a unique identifier for the album. Try again?')
 }
 
 self.list = async (req, res, next) => {
@@ -179,7 +180,7 @@ self.create = async (req, res, next) => {
       ? utils.escape(req.body.name.trim().substring(0, self.titleMaxLength))
       : ''
 
-    if (!name) return res.json({ success: false, description: 'No album name specified.' })
+    if (!name) throw new ClientError('No album name specified.')
 
     const album = await db.table('albums')
       .where({
@@ -189,7 +190,7 @@ self.create = async (req, res, next) => {
       })
       .first()
 
-    if (album) return res.json({ success: false, description: 'There is already an album with that name.' })
+    if (album) throw new ClientError('Album name already in use.')
 
     const identifier = await self.getUniqueRandomName()
 
@@ -228,7 +229,7 @@ self.disable = async (req, res, next) => {
 
     const id = req.body.id
     const purge = req.body.purge
-    if (!Number.isFinite(id)) return res.json({ success: false, description: 'No album specified.' })
+    if (!Number.isFinite(id)) throw new ClientError('No album specified.')
 
     if (purge) {
       const files = await db.table('files')
@@ -263,7 +264,12 @@ self.disable = async (req, res, next) => {
       .first()
       .then(row => row.identifier)
 
-    await paths.unlink(path.join(paths.zips, `${identifier}.zip`))
+    try {
+      await paths.unlink(path.join(paths.zips, `${identifier}.zip`))
+    } catch (error) {
+      // Re-throw non-ENOENT error
+      if (error.code !== 'ENOENT') throw error
+    }
     return res.json({ success: true })
   } catch (error) {
     return apiErrorsHandler(error, req, res, next)
@@ -278,13 +284,13 @@ self.edit = async (req, res, next) => {
     const ismoderator = perms.is(user, 'moderator')
 
     const id = parseInt(req.body.id)
-    if (isNaN(id)) return res.json({ success: false, description: 'No album specified.' })
+    if (isNaN(id)) throw new ClientError('No album specified.')
 
     const name = typeof req.body.name === 'string'
       ? utils.escape(req.body.name.trim().substring(0, self.titleMaxLength))
       : ''
 
-    if (!name) return res.json({ success: false, description: 'No name specified.' })
+    if (!name) throw new ClientError('No album name specified.')
 
     const filter = function () {
       this.where('id', id)
@@ -302,12 +308,29 @@ self.edit = async (req, res, next) => {
       .first()
 
     if (!album) {
-      return res.json({ success: false, description: 'Could not get album with the specified ID.' })
+      throw new ClientError('Could not get album with the specified ID.')
-    } else if (album.id !== id) {
+    }
-      return res.json({ success: false, description: 'Name already in use.' })
+
-    } else if (req._old && (album.id === id)) {
+    const albumNewState = (ismoderator && typeof req.body.enabled !== 'undefined')
-      // Old rename API
+      ? Boolean(req.body.enabled)
-      return res.json({ success: false, description: 'You did not specify a new name.' })
+      : null
+
+    const nameInUse = await db.table('albums')
+      .where({
+        name,
+        enabled: 1,
+        userid: user.id
+      })
+      .whereNot('id', id)
+      .first()
+
+    if ((album.enabled || (albumNewState === true)) && nameInUse) {
+      if (req._old) {
+        // Old rename API (stick with 200 status code for this)
+        throw new ClientError('You did not specify a new name.', { statusCode: 200 })
+      } else {
+        throw new ClientError('Album name already in use.')
+      }
     }
 
     const update = {
@@ -319,8 +342,8 @@ self.edit = async (req, res, next) => {
         : ''
     }
 
-    if (ismoderator && typeof req.body.enabled !== 'undefined') {
+    if (albumNewState !== null) {
-      update.enabled = Boolean(req.body.enabled)
+      update.enabled = albumNewState
     }
 
     if (req.body.requestLink) {
@@ -342,7 +365,7 @@ self.edit = async (req, res, next) => {
         const newZip = path.join(paths.zips, `${update.identifier}.zip`)
         await paths.rename(oldZip, newZip)
       } catch (error) {
-        // Re-throw error
+        // Re-throw non-ENOENT error
         if (error.code !== 'ENOENT') throw error
       }
 
@@ -368,7 +391,7 @@ self.get = async (req, res, next) => {
   try {
     const identifier = req.params.identifier
     if (identifier === undefined) {
-      return res.status(401).json({ success: false, description: 'No identifier provided.' })
+      throw new ClientError('No identifier provided.')
     }
 
     const album = await db.table('albums')
@@ -379,7 +402,7 @@ self.get = async (req, res, next) => {
       .first()
 
     if (!album || album.public === 0) {
-      return res.status(404).json({ success: false, description: 'The album could not be found.' })
+      throw new ClientError('Album not found.', { statusCode: 404 })
     }
 
     const title = album.name
@@ -421,17 +444,11 @@ self.generateZip = async (req, res, next) => {
 
     const identifier = req.params.identifier
     if (identifier === undefined) {
-      return res.status(401).json({
+      throw new ClientError('No identifier provided.')
-        success: false,
-        description: 'No identifier provided.'
-      })
     }
 
     if (!config.uploads.generateZips) {
-      return res.status(401).json({
+      throw new ClientError('ZIP generation disabled.', { statusCode: 403 })
-        success: false,
-        description: 'ZIP generation disabled.'
-      })
     }
 
     const album = await db.table('albums')
@@ -442,9 +459,9 @@ self.generateZip = async (req, res, next) => {
       .first()
 
     if (!album) {
-      return res.json({ success: false, description: 'Album not found.' })
+      throw new ClientError('Album not found.', { statusCode: 404 })
     } else if (album.download === 0) {
-      return res.json({ success: false, description: 'Download for this album is disabled.' })
+      throw new ClientError('Download for this album is disabled.', { statusCode: 403 })
     }
 
     if ((isNaN(versionString) || versionString <= 0) && album.editedAt) {
@@ -457,18 +474,18 @@ self.generateZip = async (req, res, next) => {
         await paths.access(filePath)
         return res.download(filePath, `${album.name}.zip`)
       } catch (error) {
-        // Re-throw error
+        // Re-throw non-ENOENT error
         if (error.code !== 'ENOENT') throw error
       }
     }
 
     if (self.zipEmitters.has(identifier)) {
       logger.log(`Waiting previous zip task for album: ${identifier}.`)
-      return self.zipEmitters.get(identifier).once('done', (filePath, fileName, json) => {
+      return self.zipEmitters.get(identifier).once('done', (filePath, fileName, clientErr) => {
         if (filePath && fileName) {
           res.download(filePath, fileName)
-        } else if (json) {
+        } else if (clientErr) {
-          res.json(json)
+          apiErrorsHandler(clientErr, req, res, next)
         }
       })
     }
@@ -482,24 +499,18 @@ self.generateZip = async (req, res, next) => {
       .where('albumid', album.id)
     if (files.length === 0) {
       logger.log(`Finished zip task for album: ${identifier} (no files).`)
-      const json = {
+      const clientErr = new ClientError('There are no files in the album.')
-        success: false,
+      self.zipEmitters.get(identifier).emit('done', null, null, clientErr)
-        description: 'There are no files in the album.'
+      throw clientErr
-      }
-      self.zipEmitters.get(identifier).emit('done', null, null, json)
-      return res.json(json)
     }
 
     if (zipMaxTotalSize) {
       const totalSizeBytes = files.reduce((accumulator, file) => accumulator + parseInt(file.size), 0)
       if (totalSizeBytes > zipMaxTotalSizeBytes) {
         logger.log(`Finished zip task for album: ${identifier} (size exceeds).`)
-        const json = {
+        const clientErr = new ClientError(`Total size of all files in the album exceeds ${zipMaxTotalSize} MB limit.`)
-          success: false,
+        self.zipEmitters.get(identifier).emit('done', null, null, clientErr)
-          description: `Total size of all files in the album exceeds the configured limit (${zipMaxTotalSize} MB).`
+        throw clientErr
-        }
-        self.zipEmitters.get(identifier).emit('done', null, null, json)
-        return res.json(json)
       }
     }
 
@@ -522,10 +533,7 @@ self.generateZip = async (req, res, next) => {
       })
     } catch (error) {
       logger.error(error)
-      return res.status(500).json({
+      throw new ServerError(error.message)
-        success: 'false',
-        description: error.toString()
-      })
     }
 
     logger.log(`Finished zip task for album: ${identifier} (success).`)
@@ -589,7 +597,7 @@ self.addFiles = async (req, res, next) => {
 
     ids = req.body.ids
     if (!Array.isArray(ids) || !ids.length) {
-      return res.json({ success: false, description: 'No files specified.' })
+      throw new ClientError('No files specified.')
     }
 
     albumid = parseInt(req.body.albumid)
@@ -608,10 +616,7 @@ self.addFiles = async (req, res, next) => {
         .first()
 
       if (!album) {
-        return res.json({
+        throw new ClientError('Album does not exist or it does not belong to the user.')
-          success: false,
-          description: 'Album does not exist or it does not belong to the user.'
-        })
       }
 
       albumids.push(albumid)
@@ -641,10 +646,7 @@ self.addFiles = async (req, res, next) => {
     return res.json({ success: true, failed })
   } catch (error) {
     if (Array.isArray(failed) && (failed.length === ids.length)) {
-      return res.json({
+      return apiErrorsHandler(new ServerError(`Could not ${albumid === null ? 'add' : 'remove'} any files ${albumid === null ? 'to' : 'from'} the album.`), req, res, next)
-        success: false,
-        description: `Could not ${albumid === null ? 'add' : 'remove'} any files ${albumid === null ? 'to' : 'from'} the album.`
-      })
     } else {
       return apiErrorsHandler(error, req, res, next)
     }

controllers/handlers/apiErrorsHandler.js

@@ -1,4 +1,5 @@
-const UserError = require('./../utils/UserError')
+const ClientError = require('./../utils/ClientError')
+const ServerError = require('./../utils/ServerError')
 const logger = require('./../../logger')
 
 module.exports = (error, req, res, next) => {
@@ -6,21 +7,21 @@ module.exports = (error, req, res, next) => {
     return logger.error(new Error('Missing "res" object.'))
   }
 
-  // Intentional error messages to be delivered to users
+  // Error messages that can be returned to users
-  const isUserError = error instanceof UserError
+  const isClientError = error instanceof ClientError
+  const isServerError = error instanceof ServerError
 
-  // ENOENT or missing file errors, typically harmless, so do not log stacktrace
+  const logStack = (!isClientError && !isServerError) ||
-  const isENOENTError = error instanceof Error && error.code === 'ENOENT'
+    (isServerError && error.logStack)
-
+  if (logStack) {
-  if (!isUserError && !isENOENTError) {
     logger.error(error)
   }
 
-  const statusCode = isUserError
+  const statusCode = (isClientError || isServerError)
     ? error.statusCode
     : 500
 
-  const description = isUserError
+  const description = (isClientError || isServerError)
     ? error.message
     : 'An unexpected error occurred. Try again?'
 

controllers/utils/ClientError.js

@@ -0,0 +1,15 @@
+class ClientError extends Error {
+  constructor (message, options = {}) {
+    super(message)
+
+    const {
+      statusCode
+    } = options
+
+    this.statusCode = statusCode !== undefined
+      ? statusCode
+      : 400
+  }
+}
+
+module.exports = ClientError

controllers/utils/ServerError.js

@@ -0,0 +1,18 @@
+class ServerError extends Error {
+  constructor (message, options = {}) {
+    super(message)
+
+    const {
+      statusCode,
+      logStack
+    } = options
+
+    this.statusCode = statusCode !== undefined
+      ? statusCode
+      : 500
+
+    this.logStack = logStack || false
+  }
+}
+
+module.exports = ServerError

controllers/utils/UserError.js

@@ -1,11 +0,0 @@
-class UserError extends Error {
-  constructor (message, statusCode) {
-    super(message)
-
-    this.statusCode = statusCode !== undefined
-      ? statusCode
-      : 400
-  }
-}
-
-module.exports = UserError