1
\$\begingroup\$

I'm practicing back-end programming and NodeJS. As an exercise, I'm attempting to build a REST API for a MongoDB collection. I'm also learning to use the Express and Mongoose middle-wares, so that's what we'll use for the server and database respectively. Also practicing async / await to handle promises.

The requirements for this basic REST API and exercise are:

  • Support get and delete on individual resources.
  • Support get and post on the resource collection.
  • Apply generalization and separation of concerns.
  • Protect against Mongo injection.
  • Use async / await to handle promises.

This is the current working implementation:


app.js

const express = require('express')
const mongoose = require('mongoose')
const morgan = require('morgan')

const songRouter = require('./routes/song-router.js')

const mongurl = 'mongodb://localhost:27017/library'
const port = 3000

const app = express()
app.use(morgan('combined'))
app.use('/songs', songRouter)

mongoose.connect(mongurl, () => {
  console.log(`\n    >> Mongoose connected to ${mongurl}`)
})

app.listen(port, () => {
  console.log(`\n    >> Node listening to port ${port}`)
})

models/song-model.js

const mongoose = require('mongoose')

const song = {
  name: {
    type: String,
    required: true
  },

  author: {
    type: String,
    required: true
  },

  key: String
}

const options = {
  timestamps: true
}

const schema = new mongoose.Schema(song, options)

module.exports = mongoose.model('song', schema)


routes/song-router.js

const express = require('express')

const control = require('../controllers/song-control.js')

const router = express.Router()
router.use(express.json())

router
  .route('/')
  .get(control.getAll)
  .post(control.postOne)

router
  .route('/:songId')
  .get(control.getOne)
  .delete(control.deleteOne)

module.exports = router


controllers/song-control.js (version 1, without generalization)

const songModel = require('../models/song-model.js')

exports.getAll = async (req, res, nxt) => {
  try {
    const allSongs = await songModel.find({})
    res.status(200).json(allSongs)
  } catch (err) {
    nxt(err)
  }
}

exports.getOne = async (req, res, nxt) => {
  try {
    const oneSong = await songModel.findById(req.params.songId)
    res.status(200).json(oneSong)
  } catch (err) {
    nxt(err)
  }
}

exports.postOne = async (req, res, nxt) => {
  try {
    const postedSong = await songModel.create(req.body)
    res.status(200).json(postedSong)
  } catch (err) {
    nxt(err)
  }
}

exports.deleteOne = async (req, res, nxt) => {
  try {
    const deletedSong = await songModel.findByIdAndDelete(req.params.songId)
    res.status(200).json(deletedSong)
  } catch (err) {
    nxt(err)
  }
}


controllers/song-control.js (version 2, first attempt at generalization)

const songModel = require('../models/song-model.js')

exports.getAll = buildMongoFunction('find')

exports.getOne = buildMongoFunction('findById', true)

exports.postOne = buildMongoFunction('create', false)

exports.deleteOne = buildMongoFunction('findByIdAndDelete', true)

function buildMongoFunction (funName, argIsParam) {
  return async (req, res, nxt) => {
    const arg = argIsParam ? req.params.songId : req.body

    try {
      const reply = await songModel[funName](arg)
      res.status(200).json(reply)
    } catch (err) {
      nxt(err)
    }
  }
}



I'm looking forward to all kinds and types of feedback: style, bugs, anti-patterns, ways to do this more concise / maintainable / redeable, conventions, best practices; whatever you think can be improved, please share.

I have some specific questions, but please feel free to ignore these and comment on something else!

  • The generalization of controllers/song-control.js feels hacky. Is there a better way to implement the generalization of that pattern? How'd you do it?

  • How well are these concepts being applied: generalization, separation of concerns? Would you separate responsibilities even further? Or are they too separated? Can something be further generalized?

  • How well is async / await being used?

  • Should I sanitize inputs? Or is enforcing Mongoose models and schemas protection enough against Mongo injections?

  • Seems that Mongoose queries do not return promises. Is the async / await code here doing any actual asynchronous job?

  • What would you recommend doing in a different way?

\$\endgroup\$
2
+100
\$\begingroup\$

This is not a full response to your question but what I could quickly jot down while looking at your post. Sorry, no insights to share. I would discourage you from choosing this answer in case someone with a more fleshed out response decides to come along and provide more value.

I looked over the song controller. I considered style and organization choices and decided on the following. This is more about javascript styling in general, not necessarily about node, or mongoose, or server-side technology.

Please take a look and see if it helps you think differently about anything you are looking forward to understanding.

const buildMongoFunction = name => async (req, res, nxt) => {
  const { params: { songId }, body } = req;
  
  try {
    const needsSongID = ['findById', 'findByIdAndDelete'].includes(name);
    res.status(200).json(await songModel[name](needsSongID ? songID : body));
  } catch (error) { nxt(error); }
};

const toExports = ['find', 'findById', 'create', 'findByIdAndDelete']
  .reduce((toExport, name) => ({ ...toExport, [name]: buildMongoFunction(name) }), {});

Object.assign(exports, toExports);
| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.