javascriptnode.jsmongodbexpressmongoose

CRUD operations using mongoose and express


I am creating an express app using mongoose with the intention of connecting this to React for the frontend.

I have listed some CRUD operations for a customer controller below but there are a few things I do not like about this approach.

  1. When using Customer.findById with a valid ObjectID that is not found, it returns null with a 200 response code. I want this to return 404 if no customer was found. I realise I could change the catch response to a 404, but I want to have some generic error handling incase the server goes down during the request or an invalid ObjectId was provided, which brings me to my next item.
  2. If I provide an invalid ObjectId I want to provide some meaningful message, is 500 the right response code?
  3. Error handling: Am I returning errors the correct way? currently errors return a string with the error message. Should I return JSON instead? e.g. res.status(500).json({error: error.message). I am planning on connecting this to react (which I am still learning) and I assume the UI will need to display these messages to the user?
  4. findById is repeated in getCustomerById, updateCustomer, and deleteCustomer. I feel this is bad practice and there must be a more streamlined approach?
  5. I want to have one function that validates if the ObjectId is valid. I am aware that I can do this is the routes using router.params but I'm not sure if checking for a valid id should be in the routes file as it seems like something the controller should be handling? See routes example below from another project I did.

What are the best practices and suggested ways to improve my code, based on the above? I have read the documentation from mongoose, mozilla, and stackoverflow Q&A but they don't seem to address these issues (at least I could not find it).

I am really after some guidance or validation that what I am doing is correct or wrong.

customer.controller.js

const Customer = require("../models/customer.model");

exports.getCustomers = async (req, res) => {
  try {
    const customers = await Customer.find();
    res.status(200).json(customers);
  } catch (error) {
    res.status(500).send(error.message);
  }
};

exports.getCustomerById = async (req, res) => {
  try {
    const customer = await Customer.findById(req.params.id);
    res.status(200).json(customer);
  } catch (error) {
    res.status(500).send(error.message);
  }
};

exports.addCustomer = async (req, res) => {
  try {
    const customer = new Customer(req.body);
    await customer.save().then(res.status(201).json(customer));
  } catch (error) {
    res.status(500).send(error.message);
  }
};

exports.updateCustomer = async (req, res) => {
  try {
    const customer = await Customer.findById(req.params.id);
    Object.assign(customer, req.body);
    customer.save();
    res.status(200).json(customer);
  } catch (error) {
    res.status(500).send(error.message);
  }
};

exports.deleteCustomer = async (req, res) => {
  try {
    const customer = await Customer.findById(req.params.id);
    await customer.remove();
    res.status(200).json(customer);
  } catch (error) {
    res.status(500).send(error.message);
  }
};

Router.params example This is a routes file (not related to my current app) and is provided as an example of how I have used router.params in the past.

const express = require("express");
const router = express.Router();
const mongoose = require("mongoose");
const Artist = require("../models/Artist");
const loginRequired = require("../middleware/loginRequired");

const {
  getArtists,
  addArtist,
  getArtistById,
  updateArtist,
  deleteArtist,
} = require("../controllers/artistController");

router
  .route("/")
  .get(loginRequired, getArtists) // Get all artists
  .post(loginRequired, addArtist); // Create a new artist

router
  .route("/:id")
  .get(loginRequired, getArtistById) // Get an artist by their id
  .put(loginRequired, updateArtist) // Update an artist by their id
  .delete(loginRequired, deleteArtist); // Delete an artist by their id

router.param("id", async (req, res, next, id) => {
  // Check if the id is a valid Object Id
  if (mongoose.isValidObjectId(id)) {
    // Check to see if artist with valid id exists
    const artist = await Artist.findOne({ _id: id });
    if (!artist) res.status(400).json({ errors: "Artist not found" });

    res.locals.artist = artist;
    res.locals.artistId = id;

    next();
  } else {
    res.status(400).json({ errors: "not a valid object Id" });
  }
});

module.exports = router;

Solution

  • i personly like to make error handeling more global so i would write something like

    constPrettyError = require('pretty-error')
    const pe = new PrettyError()
    
    const errorHandler = (err, req, res, next) => {
        if (process.env.NODE_ENV !== 'test') {
            console.log(pe.render(err))
        }
        return res
            .status(err.status || 500)
            .json({ error: { message: err.message || 'oops something went wrong' } })
    }
    
    module.exports = errorHandler
    

    as a handler

    the in your index / server file

    app.use(errorHandler)
    

    then in your handlers just

      } catch (err) {
        next(err);
      }
    

    as an example

     if (!artist) next({ message: "Artist not found" ,status:404 });
    

    also, note that you can customize this error handler to switch case (or object) a custom error per status as well if you want

    const errorHandler = (err, req, res, next) => {
        if (process.env.NODE_ENV !== 'test') {
            console.log(pe.render(err))
        }
        const messagePerStatus = {
            404: 'not found',
            401: 'no authorization'
        }
        const message = messagePerStatus[err.status]
        return res
            .status(err.status || 500)
            .json({
                error: { message: message || err.message || 'oops something went wrong' }
            })
    }
    

    then just

     if (!artist) next({status:404 });