Skip to content

added new option to updateReview using findOneAndUpdate() #4

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.

Sign up for GitHub

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

Open
NatachaKey wants to merge 1 commit into
base: main
Choose a base branch
from
Open

added new option to updateReview using findOneAndUpdate() #4

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 41 additions & 20 deletions controllers/reviewController.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,29 +78,50 @@ const updateReview = async (req, res) => {
// await review.save();
// res.status(StatusCodes.OK).json({ review });

//OPTION 2
const { id: reviewId } = req.params; //TAKE ID FROM REQ.PARAMS AND ASSIGN IT TO REVIEWiD
//GENERAL !!!!! OPTION 2
// const { id: reviewId } = req.params; //TAKE ID FROM REQ.PARAMS AND ASSIGN IT TO REVIEWiD

const review = await Review.findOne({ _id: reviewId });
// const review = await Review.findOne({ _id: reviewId });

// if (!review) {
// throw new CustomError.NotFoundError(`No review with id ${reviewId}`);
// }

// checkPermissions(req.user, review.user);
// // can simplify with: review.rating = req.body?.rating || review.rating (but hasOwnProperty is better because then that allows us to send/unset values like this: {rating: null})
// review.rating = req.body.hasOwnProperty('rating')
// ? req.body.rating
// : review.rating;
// review.title = req.body.hasOwnProperty('title')
// ? req.body.title
// : review.title;
// review.comment = req.body.hasOwnProperty('comment')
// ? req.body.comment
// : review.comment;

// await review.save();
// res.status(StatusCodes.OK).json({ review });

//option 2.o with findoneandupdate()
const { id: reviewId } = req.params;
const review = await Review.findOne({ _id: reviewId });
if (!review) {
throw new CustomError.NotFoundError(`No review with id ${reviewId}`);
}

checkPermissions(req.user, review.user);
// can simplify with: review.rating = req.body?.rating || review.rating (but hasOwnProperty is better because then that allows us to send/unset values like this: {rating: null})
review.rating = req.body.hasOwnProperty('rating')
? req.body.rating
: review.rating;
review.title = req.body.hasOwnProperty('title')
? req.body.title
: review.title;
review.comment = req.body.hasOwnProperty('comment')
? req.body.comment
: review.comment;

await review.save();
res.status(StatusCodes.OK).json({ review });
throw new CustomError.NotFoundError(`No review with id ${reviewId}`);
}

const updatedReview = await Review.findOneAndUpdate(
{ _id: reviewId },
{
$set: {
rating: req.body.hasOwnProperty('rating') ? req.body.rating : review.rating,
title: req.body.hasOwnProperty('title') ? req.body.title : review.title,
comment: req.body.hasOwnProperty('comment') ? req.body.comment : review.comment
}
},
{ new: true } // This option returns the updated document
);

res.status(StatusCodes.OK).json({ review: updatedReview });
Comment on lines +105 to +124

@akosasante akosasante Jul 12, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job with the refactor! Now after seeing this and trying it out I'm seeing a few things that make me think you should actually just stick to the previous version after all. First a quick explanation on the difference between the two:

  • findOneAndUpdate will do a single database call where it will use the first argument to the function to filter which database rows it needs to update, and then use the second argument to make those updates at once. So it is more efficient and performant; and avoids any race conditions if someone else is making a update to the same database rows at the same time.
  • findOne + save will do two database calls. One for the findOne query, and one for the save query. So it's less performant (though in simpler cases like here this probably won't make much of a user-visible difference in the speed of the query. It's more something to think about when working with large databases). As well, you do more fine-grained updates or manipulate the document you get from the findOne call with this approach, so this approach is more flexible.

In this particular case, one thing I forgot about here is that you want to do two things with the result of the find:

  1. check if the review exists in the db, if not then throw an error
  2. check if the request has permissions to update the review

For (1), because findOneAndUpdate will try to update based on the filter query; if it doesn't find a result it will just return null. So you could do something like the following (and this would allow us to remove the earlier findOne query so that we're not still doing 2 queries when we could just do 1):

const updateReview = async (req, res) => {
  const { id: reviewId } = req.params;
  
  const updatedReview = await Review.findOneAndUpdate(
    { _id: reviewId }, // first argument tells Mongo how to filter the database for the rows we want to update
    req.body, // second argument is an object that has the keys as the columns mongo should update, and the values as what it should set those columns to. By default, if we don't include $set, mongo will just assume that operation and Mongoose will wrap it for us
    { new: true } // This option returns the updated document
  );
  
  if (!updatedReview) {
    throw new CustomError.NotFoundError(`No review with id ${reviewId}`);
  }
  
  return res.status(StatusCodes.OK).json({ review: updatedReview });
}

For (2), because the findOneAndUpdate happens all in one query, we need to figure out a way to either stop the query from in the middle (not possible) or revert it after it's complete if the user shouldn't have had permission to do the update. In the hooks, we have access to the original document by fetching it from the database in a pre hook (but now we're back to doing 2 database queries, one for the original findOneAndUpdate and one for the findOne inside the hook). But the hook has no idea about the requesting user, so we need to get that info back to the controller, where we can do the checkPermissions call. Also, in the controller if the permissions check fails, then we need to undo the update:

// models/Review.js

ReviewSchema.pre('findOneAndUpdate', async function () {
  // in pre/post hooks for `findOneAndUpdate` "this" refers to the Query object, NOT to the Document object like in pre/post-save hooks
  // Fetch and store the original document in the "this" object so that we can check it in the post middleware
  this._originalDoc = await this.model.findOne(this.getQuery());
})

ReviewSchema.post('findOneAndUpdate', async function (updatedReview) {
  if (updatedReview) {
    // store the original document in the updatedReview object so that we can check it in the controller
    updatedReview._originalDoc = this._originalDoc
  }
})

// reviewController.js
const updateReview = async (req, res) => {
  const { id: reviewId } = req.params;
  
  const updatedReview = await Review.findOneAndUpdate(
    { _id: reviewId }, // first argument tells Mongo how to filter the database for the rows we want to update
    req.body, // second argument is an object that has the keys as the columns mongo should update, and the values as what it should set those columns to. By default, if we don't include $set, mongo will just assume that operation and Mongoose will wrap it for us
    { new: true } // This option returns the updated document
  );
  
  if (!updatedReview) {
    throw new CustomError.NotFoundError(`No review with id ${reviewId}`);
  }
  
  try {
    checkPermissions(req.body, updatedReview.user);
  } catch (unauthorizedError) {
    console.log('Check permissions failed, user cannot update this review, undo-ing the applied update');
    updatedReview = await Review.findOneAndUpdate(
      { _id: reviewId },
      updatedReview._originalDoc,
      { new: true },
    );
  }
  
  return res.status(StatusCodes.OK).json({ review: updatedReview });
}

As you can see, this is becoming more complex, which is why I think in this case, the findOne + save approach is probably what we should stick to. 

If you do choose to stay with the findOneAndUpdate in this case, then don't forget to make sure that you're updating the product's average rating in a findOneAndUpdate post-hook instead of a save post-hook now:

// We shouldn't remove this one, since it still applies for when a new review is created (Review.create)
ReviewSchema.post('save', async function () {
  // we are calling the static method calculateAverageRating that schema has:
  await this.constructor.calculateAverageRating(this.product); //with this. we access actual schema
});

ReviewSchema.post('findOneAndUpdate', async function (updatedReview) {
  // copying this if statement from the earlier example for permissions check
  if (updatedReview) {
    updatedReview._originalDoc = this._originalDoc
    await this.model.calculateAverageRating(updatedReview.product) // only update the average rating  if we found a review to update
  }
})

@NatachaKey NatachaKey Jul 12, 2023 •
edited
Loading

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akosasante thank you for showing different approaches,I definetly will stick to findOne + save one, //need time to figure out what's happening here. I won't merge this PR, just leave it to learn about the options. one question : shoul we always use POST requests while dealing with schemas? it will always be post- with all kings of events in remove/save/update/create... ? -----> ReviewSchema.post('findOneAndUpdate', async function (updatedReview) {....

@akosasante akosasante Jul 13, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one question : shoul we always use POST requests while dealing with schemas? it will always be post- with all kings of events in remove/save/update/create... ? -----> ReviewSchema.post('findOneAndUpdate', async function (updatedReview) {....

To clarify, here you're asking about the pre and post hooks in mongoose right? Not about POST HTTP requests?

https://mongoosejs.com/docs/6.x/docs/middleware.html

Both are useful depending on the case. pre hooks are for logic that you want to occur before the query actually happens in the database. The example cases the doc gives are:

  • complex validation
  • removing dependent documents (removing a user removes all their blogposts)
  • asynchronous defaults
  • asynchronous tasks that a certain action triggers

post hooks are for logic that you want to occur after the query happens in the database but before returning the document/result to the caller. Some example cases are:

  • logging information about your query results
  • doing additional logic based on the saved/fetched document(s)
  • maybe something like sending an email after successfully updating an order to 'paid'

So it all just depends on your use-case. In many cases you may not need any middleware at all 😄

Please let me know if I misunderstood your question!

@NatachaKey NatachaKey Jul 13, 2023

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akosasante I mixed up all the concepts! In this case we are dealing with the db and mongoose Schema and use it's hooks, i thought that we were making simple http request- my bad!
Thank you so much for such a great, clear explanation with examples, it's clear now😊


//option 3
//When we do the object destructuring (const { rating, title, comment } = req.body;), then any fields that didn't exist in req.body will just be saved to the variables on the left-hand-side as undefined.
Expand Down