Express middleware to validate URL and tags

Question

I am building an API where the client can send a url and some comma-delimited tags:

{
   "url": "https://www.youtube.com/watch?v=pquxHIBx8ks",
   "tags": "JavaScript, Sequelize"
} 

Before processing this data, I want to validate the input. To do that, I wrote a middleware function called validateSubmitScreencastReq:

import config from 'config'
import youtubeUrl from 'youtube-url'
import db from 'sequelize-connect'
import * as youtubeClient from '../../source/util/youtubeClient'

async function validateUrl (url) {
  if (!url) {
    return 'url cannot be undefined'
  }

  if (!youtubeUrl.valid(url)) {
    return 'url must be a valid YouTube URL'
  }

  const client = youtubeClient.create(config.youtubeApiKey)
  if (!await client.videoExists(url)) {
    return 'url must link to an existent, public YouTube video'
  }

  const foundScreencast = await db.models.screencast.findOne({
    where: {
      url: url
    }
  })
  if (foundScreencast !== null) {
    return 'url has already been submitted'
  }

  return undefined
}

function validateTags (tags) {
  if (!tags) {
    return 'tags cannot be undefined'
  }

  if (typeof tags !== 'string') {
    return 'tags must be a string'
  }

  return undefined
}

export async function validateSubmitScreencastReq (req, res, next) {
  try {
    const urlError = await validateUrl(req.body.url)
    const tagError = validateTags(req.body.tags)
    const errors = []
    if (urlError) {
      errors.push({
        field: 'url',
        message: urlError
      })
    }
    if (tagError) {
      errors.push({
        field: 'tags',
        message: tagError
      })
    }
    if (errors.length !== 0) {
      res.status(400).json({errors})
    } else {
      next()
    }
  } catch (error) {
    next(error)
  }
}

I'm not too happy with this code.

The function essentially aggregates error messages, which makes me think of using reduce but I can't see how to make it work elegantly.

What's more, I don't think I'm expressing that the function aggregates errors very clearly.

How can I improve the readability the above code, preferably using functional concepts?

(If it's at all useful, here is the validateSubmitScreencastReq function on GitHub and here are the corresponding unit tests.)

Answer 1

export async function validateSubmitScreencastReq (req, res, next) {
  try {
    const errors = [
        {field: 'url', message: await validateUrl(req.body.url)},
        {field: 'tags', message: validateTags(req.body.tags)}
    ].filter((obj) => obj.message !== undefined);

    if (errors.length !== 0) {
      res.status(400).json({errors})
    } else {
      next()
    }
  } catch (error) {
    next(error)
  }
}

This removes any non-errors from the originally constructed array and should give the same end-result as your code.

You can also improve your validate url/valide tag functions:

const assert = require("assert");//common nodejs package

async function validateUrl (url) {
  try {
    assert(url, "url can not be undefined");
    assert(youtubeUrl.valid(url), 'url must be a valid YouTube URL');

    const client = youtubeClient.create(config.youtubeApiKey)

    assert(await client.videoExists(url), 'url must link to an existent, public YouTube video');    
    assert(!(await db.models.screencast.findOne({where: {url: url}})), 'url has already been submitted');
  } catch (error) {
    /* remove the if, if you want to also return whatever those functions may
       have thrown as an error */
    if (error instanceof AssertionError) {
      return error.message;
    }
  }
  //No return needed if you want to return undefined
};

Answer 2

Avoiding the '!' and nesting your conditions would be good. They would then throw relevant errors.

if (url) {
    if (youtubeUrl.valid(url)) {
        const client = youtubeClient.create(config.youtubeApiKey)
        if (await client.videoExists(url)) {
            const foundScreencast = await db.models.screencast.findOne({
                where: {
                    url: url
                }
            });
            if (foundScreencast === null) {
                return;
            } else {
                return 'url has already been submitted'
            }
        } else {
            return 'url must link to an existent, public YouTube video'
        }
    } else {
        return 'url must be a valid YouTube URL'
    }
} else {
    return 'url cannot be undefined'
}

No errors would be aggregated then, and only a single error will be thrown. That would be relevant enough. You errors array would be very clean as well.