2
\$\begingroup\$

I'm currently building a MongoDB which has collections of 2 types:

  1. A series of "dialog" collections
  2. A collection storing the version numbers of each "dialog"

I'm looking to build an API, where-in I increment the version of the dialogs whenever that particular "dialog" is amended.

Basically I'll be updating multiple collections and I want to ensure that my code is appropriate.

On a given POST route I have something like:

router.post('/:dialogName', function(req,res){

    const dialogName = req.params.dialogName;
    const subId = req.body.subID;
    const newArray = req.body.newArray;
    const collection = db.get().collection(dialogName);

    collection.update({
        "synonyms._id": new ObjectId(subId)
    },
        {
            $set: {"synonyms.$.text": newArray}
        }
    ).then(function(doc){
        return updateVersions(dialogName).then(function(){
            console.log('version updated')
        })
        .catch(function(err){
            console.log('version not saved')
        })
    }).then(function(doc){
        res.status(200).end()
    }).catch(function(err){
        res.status(400).send("Couldnt update question")
    })
})

So once I successfully amend a doc, I invoke a promisified function that does this:

const updateVersions = function(dialog){
    return new Promise(function(resolve,reject){
        const versionCollection = db.get().collection('Version')
        versionCollection.update({
            "dialogs.name": dialog
        },{
            $inc: { "dialogs.$.__v": 1 , "__v": 1}
        }).then(function(response){
            resolve('ok')
        }).catch(function(err){
            console.log('errr : ',err)
            reject('err')
        })
    })
}

Is my approach correct?

\$\endgroup\$
2
\$\begingroup\$

I really think your code is having at least two issues. The first one is something that may confuse your service consumers a lot. The second one is probably a bug.


Careful With Status Codes

.catch(function(err){
  res.status(400).send("Couldnt update question")
})

This is not following RESTful approach. This .catch will be invoked if your code could not update a question. Meaning, the service was not able to fulfill a request. The correct HTTP response code for such situation is 500 Internal Server Error.

You should only return 400 Bad Request if the request was validated and there were issues with the payload (request body received). In such case, HTTP 400 is truly the best option, but in order to improve user experience, you should also provide the information about what exactly was wrong with the request. E.g. HTTP 404 + response payload

{
  "error": {
    "referenceId": "TOO_LONG_QUESTION",
    "description": "Question payload should not exceed 1024kb, however received 2048kb."
  }
}

The response format is actually arbitrary. Just think about what would be useful for a developer to know so that they can start troubleshooting in a right place.


Possible Bug

I numbered the code lines to make it easy to refer to them.

( 1)  collection.update({
( 2)    "synonyms._id": new ObjectId(subId)
( 3)  },
( 4)    {
( 5)        $set: {"synonyms.$.text": newArray}
( 6)    }
( 7)  ).then(function(doc){
( 8)    return updateVersions(dialogName).then(function(){
( 9)        console.log('version updated')
(10)    })
(11)    .catch(function(err){
(12)        console.log('version not saved')
(13)    })
(14)  }).then(function(doc){
(15)    res.status(200).end()
(16)  }).catch(function(err){
(17)    res.status(400).send("Couldnt update question")
(18)  })

If versionCollection.update(...) in updateVersions(...) fails, the promise will be rejected and we will fall into lines 11-13 which is good. There is one problem though... The promise created in line 1 (collection.update(...)) will NOT fail even though we failed to update the versionCollection. Try debugging it and you will see what I mean.

I think, what you need to do there is to re-throw the error like this:

(11)    .catch(function(err){
(12)        console.log('version not saved');
(12')       throw err;  <<-------------------------
(13)    })

Asynchronosity Is NOT Easy

Yes, it's not easy at all. Even with promises. I really recommend you to read about await/async feature of ES2017 which simplifies things a lot.

With await/async your code would look much more "regular" imperative code. It reads as synchronous but acts asynchronously. Here's a sketch (not a 100% working code):

router
  .post('/:dialogName', async (req, res) => {
    const dialogName = req.params.dialogName;
    const collection = db.get().collection(dialogName);

    try {
      const doc =  await collection.update(
        { "synonyms._id": new ObjectId(req.body.subID) },
        { $set: {"synonyms.$.text": req.body.newArray} }
      );

      try {
        await updateVersions(dialogName);
        console.log('version updated');
      } catch (err) {
        console.log('version not saved');
        throw err;
      }

      res.status(200).end();
    } catch {
      res.status(400).send("Couldnt update question");
    }
  })
\$\endgroup\$
  • \$\begingroup\$ Thanks for the really detailed analysis :) - i'll include the changes! \$\endgroup\$ – Kannaj Oct 2 '17 at 6:43

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.