In a node (express) route using mongoose, which of these 2 methods is superior to ensure that an object is added to a collection?
29 views
Skip to first unread message
Sivi Luke
Jan 23, 2019, 7:04:55 AM1/23/19
to nodejs
Hi,
But then a colleague who was testing it told me that for him on every browser except Chrome, it wasn't working - he's not a tech person so difficult for me to ascertain the precise error, but I'm pretty sure the problem was that the object just wasn't adding to the collection. My best guess is that the 'project.save()' command was 'winning the race' against the unshift command. So I changed my code to include a try/catch block and async/await, and it worked fine for me (but it worked fine for me before). So this is my new code:
Sorry for the long question, but can anyone help me with this, is the second way of doing it better, as in, more likely to ensure that the newInput variable is actually added to the mongo collection? Is there a difference between the two methods (with and without 'async/await') that makes one superior to the other for my purposes?
I have a route in node/express that takes some req.body info from a form and objectifies it and adds it to a mongodb collection. I had it all coded up and it was working for me both locally and when I uploaded it. This is the code (using a promise, I believe):
router.post("/input/:id", ensureAuthenticated, async (req, res) => { Project.findOne({ _id: req.params.id }).then(project => { const newInput = { inputTitle: req.body.inputTitle, inputValue: req.body.inputValue, }; // Add to array - I think this was causing the issue project.inputs.unshift(newInput);
project.save().then(project => { res.redirect(`/projects/output/${project.id}`); }); }); });But then a colleague who was testing it told me that for him on every browser except Chrome, it wasn't working - he's not a tech person so difficult for me to ascertain the precise error, but I'm pretty sure the problem was that the object just wasn't adding to the collection. My best guess is that the 'project.save()' command was 'winning the race' against the unshift command. So I changed my code to include a try/catch block and async/await, and it worked fine for me (but it worked fine for me before). So this is my new code:
router.post("/input/:id", ensureAuthenticated, async (req, res) => { try { const newInput = { inputTitle: req.body.inputTitle, inputValue: req.body.inputValue, }; const project = await Project.findOne({ _id: req.params.id }); project.inputs.unshift(newInput);
await project.save(); res.redirect(`/projects/output/${project.id}`); } catch (e) { res.send(e); } });Sorry for the long question, but can anyone help me with this, is the second way of doing it better, as in, more likely to ensure that the newInput variable is actually added to the mongo collection? Is there a difference between the two methods (with and without 'async/await') that makes one superior to the other for my purposes?
Thanks.
Zlatko
Jan 24, 2019, 3:19:04 AM1/24/19
to nodejs
Hello Sivi,
Your problem was likely not related to the unshift method at all. That method is synchronous, there is no race. Something else must have caused the issue: front-end bug, missing content type header, the database was down or your API server crashed, cross domain request, but not this. In that respect, there's no difference between the two methods.
On the other hand, I find the second approach better, because while functionally equivalent, it's easier to understand what's going on at a glance.
There are a few things you could do to improve it, though.
For one, that error handler at the end. You should really add a different status code. And for typical clients, you would also want to send this as JSON payload, not raw string - it would be easier to handle the error. Furthermore, this way you might be exposing unneeded details about your server to the clients, while at the same time, _,not_ providing useful info. E.g if I was building a client that used this API, I wouldn't know if the error is mine (e.g. Missing field or some validation), yours (e.g. Server had disk full and can't work any more), or database error (e.g. unique constraint on an index was violated).
Another point would be that you're making an unneeded call to the database (this is MongoDB, right?). You could do the whole transaction in one go, find and update with a single query (since you don't seem to be checking for e.g. Duplicated entries anyway).
The third thing would be that you could create a light service layer and take all these database shenanigans out of your endpoint route. The router would get the id parameter from the url and the title and content from the body, and pass it on to a service that hid all this stuff. It adds the overhead of an extra file, but brings in a much easier to maintain code, reusability etc.
Hope that helps.
Zlatko
Your problem was likely not related to the unshift method at all. That method is synchronous, there is no race. Something else must have caused the issue: front-end bug, missing content type header, the database was down or your API server crashed, cross domain request, but not this. In that respect, there's no difference between the two methods.
On the other hand, I find the second approach better, because while functionally equivalent, it's easier to understand what's going on at a glance.
There are a few things you could do to improve it, though.
For one, that error handler at the end. You should really add a different status code. And for typical clients, you would also want to send this as JSON payload, not raw string - it would be easier to handle the error. Furthermore, this way you might be exposing unneeded details about your server to the clients, while at the same time, _,not_ providing useful info. E.g if I was building a client that used this API, I wouldn't know if the error is mine (e.g. Missing field or some validation), yours (e.g. Server had disk full and can't work any more), or database error (e.g. unique constraint on an index was violated).
Another point would be that you're making an unneeded call to the database (this is MongoDB, right?). You could do the whole transaction in one go, find and update with a single query (since you don't seem to be checking for e.g. Duplicated entries anyway).
The third thing would be that you could create a light service layer and take all these database shenanigans out of your endpoint route. The router would get the id parameter from the url and the title and content from the body, and pass it on to a service that hid all this stuff. It adds the overhead of an extra file, but brings in a much easier to maintain code, reusability etc.
Hope that helps.
Zlatko
Sivi Luke
Jan 24, 2019, 7:52:56 AM1/24/19
to nodejs
Zdravo Zlatko,
Thank you very much for your reply, that is very helpful, I really needed to know if one method was doing something better than the other (I suspected they were doing the same thing in a different way).
I will try and amalgamate the mongodb queries into one (to 'findAndUpdate' etc), I just have a big worry that there is a 'hole' in my code somewhere that means that something is not being added where it should be (sometimes), and I want to make sure that it is, and that if it isn't that an error is thrown. The application is currently hosted on Heroku's free server and mlab's free mongodb, so I'm wondering if maybe that has something to do with it - I'm currently in development but I am noobish and have a great fear of releasing this app to production (because it works for me), and there being a significant problem with it.
Hvala.
Reply all
Reply to author
Forward
0 new messages