Server which writes to mongo and makes http request

Question

This is simple server which basically just writes something to mongo after it accepts POST request, and then initiates a new HTTP request. I would appreciate feedback especially if I am doing correctly error handling etc. The code does its job though.

const MongoClient = require('mongodb').MongoClient;
const express = require('express')
const app = express()
var bodyParser = require('body-parser');
var http = require('http');


/* Connection URL */
const URL = 'mongodb://localhost:27017';

/* Database Name */
const DB_NAME = 'config-insert';

/* Handle to the connection */
let db = null;

/* Middlewares */
app.use(bodyParser.json());


/* 
 * Use connect method to connect to the server 
 */
MongoClient.connect(URL, function (err, client) {
    if (err) {
        throw new Error("Error connecting to mongo");
    }

    db = client.db(DB_NAME);



});

/*
 * Insert one document to mongo database.
 * 
 * Parameters
 *   data - what to insert.
 *   successCallback - success callback.
 * 
 */
function insertOne(data, successCallback) {

    if (!db) {
        throw new Error("There is no connection");

    }

    /* Insert a single document */
    db.collection('config-history').insertOne(data, {
        w: 'majority'
        , wtimeout: 10000
        , serializeFunctions: true
    }, function (err, r) {

        if (err) {
            throw new Error("Error inserting");

        }

        successCallback(r);

    });
}




/* 
 * Handler for post request 
 */
app.post('/', function (req, res) {

    let data = req.body;

    insertOne(JSON.parse(JSON.stringify(data)) /* use this otherwise mongo was modifying data variable */, function success() {

        let req = http.request({
            host: '10.1.1.41',
            path: `/category/${data.id}`,
            method: 'PUT',
            port: 9202
        }, function (response) {

            var resp = '';
            response.on('data', function (d) {
                resp += d;
            });
            response.on('end', function () {
                if (JSON.parse(resp)._shards.successful == 1) {
                    res.status(200).json({ status: "ok" })
                }


            });

        });

        req.on('error', (e) => {
            console.error(`problem with request: ${e.message}`);
        });

        req.write(JSON.stringify(data));
        req.end();

    })

})



/* 
 * Listen for connections. 
 */
app.listen(4000, () => console.log('Example app listening on port 4000!'))

Answer 1

Some comments:

1. Code is not formatted properly. There is a bunch of unnecessary whitespace, and there is some inconsistent use of var/let.
2. Most of the comments just add noise, and can be removed.
3. You're not taking advantage of promises or async/await (both for your expressjs route, the mongodb client, and for the outgoing http requests).
4. There is no general error handling middleware, or any other error handling in the express route.
5. The server accepts requests before the database is connected. A side-effect of this is that the db global needs to be null-checked in every function that uses it.
6. It's hard to test
7. The naming is a bit lacking. From DB_NAME and other clues I gather this is about some sort of config, but other than that it's hard to tell

I did some quick refactoring (not tested):

const MongoClient = require('mongodb').MongoClient
const express = require('express')
const app = express()
const bodyParser = require('body-parser')
const rp = require('request-promise')
const URL = 'mongodb://localhost:27017'
const DB_NAME = 'config-insert'

let db = null
async function connectDb(){
    let client = await MongoClient.connect(URL)
    db = client.db(DB_NAME)
}

async function insertOne(db, data){
    data = JSON.parse(JSON.stringify(data)) // use this otherwise mongo was modifying data variable

    return db.collection('config-history').insertOne(data, {
        w: 'majority',
        wtimeout: 10000,
        serializeFunctions: true
    })
}

async function putCategory(data){
    let response = await rp({
        uri: `http://10.1.1.41/category/${data.id}`,
        method: 'PUT',
        port: 9202,
        body: data,
        json: true
    })

    if(response._shards.successful !== 1){
        throw new Error(`Could not put ${data.id}`)
    }
}

const promiseRoute = route => async (req, res, next) => {
    try{
        await route(req, res)
    }catch(err){
        next(err)
    }
}

app.use(bodyParser.json())

app.post('/', promiseRoute(async (req, res) => {
    let data = req.body

    await insertOne(db, data)
    await putCategory(data)

    res.status(200).json({status: 'ok'})
}))

app.use((err, req, res, next) => {
    console.error(err)
    res.status(500).json({status: 'not nok'})
})

connectDb()
    .then(() => {
        app.listen(4000, () => console.log('Example app listening on port 4000!'))
    })
    .catch(err => {
        console.log(err)
    })

What I have done is:

1. Format the code
2. Remove almost all the comments
3. Used async await instead of callbacks. Notice I have wrapped the expressjs route in a helper middleware that just passes any uncaught errors on to the general error handler. For mongodb, the driver already supports promises, and for outgoing http requests we have the request-promise library.
4. Added error handling middleware that ensures you get a proper response even if a request fails
5. Not starting the http server before the db is connected. Notice how the db null check is gone.
6. Injecting db into insertOne to allow for easier testing in the future.
7. Turned the _shards.successful response into a more general error case since it was not handled anyway.
8. Moved the json-parse trick closer to where it is needed (you could probably use an Object.assign here as well if you wanted)

Answer 2

I would try to be consistent and completely switch to utilizing ES6 const variable declarations for those require statements for example:

const express = require('express'); 
const bodyParser = require('body-parser');
const app = express();

I would not have added the MongoDB address at all. We don't want to commit that URI to Github by accident. Rather than pasting that link, you can add that address to a config/keys.js file that you can use to store all your secret configuration data which you can then require like so: const keys = require('./config/keys');

Then you can also install and require Mongoose to help out like so: const mongoose = require('mongoose'); and then implement mongoose.connect(keys.mongoURI);

Where did I get mongoURI from? From your config/keys.js file which would look something like this:

mongoURI:'mongodb://localhost:27017'

For your ports, if you plan on pushing to production especially a service like Heroku, I would configure like so:

if (process.env.NODE_ENV === 'production') {
  // Express will serve up production assets
  // like main.js or main.css
  app.use(express.static('client/build'));

  // Express will serve up the index.html file if
  // it doesnt recognize the route
  const path = require('path');
  app.get('*', (req, res) => {
    res.sendFile(path.resolve(__dirname, 'client', 'build', 'index.html'));
  });
}

const PORT = process.env.PORT || 5000;
app.listen(PORT);