Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

saveUpdates function in server controllers does not handle null properly #2052

Closed
wkevwang opened this issue Jul 11, 2016 · 3 comments
Closed

Comments

@wkevwang
Copy link

wkevwang commented Jul 11, 2016

In the promise chain:

return Model.findById(req.params.id).exec()
  .then(handleEntityNotFound(res))
  .then(saveUpdates(req.body))
  .then(respondWithResult(res))
  .catch(handleError(res));

If the entity is not found, handleEntityNotFound passes null to saveUpdates.

function saveUpdates(updates) {
  return function(entity) {
    var updated = _.merge(entity, updates);
    return updated.save()
      .then(updated => {
        return updated;
      });
  };
}

But if the entity is null, then the return of the merge (the updated variable) will not be a Mongoose doc. Then, when updated.save() is called, it will throw a save is a not a function of null error. To fix, saveUpdates should be wrapped in an if statement:

function saveUpdates(updates) {
  return function(entity) {
    if (entity) {
      var updated = _.merge(entity, updates);
      return updated.save()
        .then(updated => {
          return updated;
        });
    }
  };
}
@Awk34
Copy link
Member

Awk34 commented Jul 11, 2016

handleEntityNotFound shouldn't be returning null

@wkevwang
Copy link
Author

If the entity is not found, the query returns null, and then handleEntityNotFound will return null as seen here:

function handleEntityNotFound(res) {
  return function (entity) {
    if (!entity) {
      res.status(404).end();
      return null;
    }
    return entity;
  };
}

@Awk34
Copy link
Member

Awk34 commented Jul 13, 2016

Hmm, the way this is set up might be need to be rethought... but for now, I think your suggestion is best. I would welcome a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants