added new option to updateReview using findOneAndUpdate()

[NatachaKey]

No description provided.

[akosasante]

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]

@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]

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]

@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😊

[akosasante]

btw in your app.js I noticed you still have a route for browser-app.js: https://github.com/NatachaKey/ecommerce/blob/main/app.js#L51-L53

but you can remove that now that you've moved that to the public directory

[akosasante]

A quick note for the frontend, for the review update endpoint; it would be best if the frontend also has some logic to prevent sending empty inputs to the backend. For example, if I only want to update my rating, it currently sends the following: https://github.com/NatachaKey/reactEcommerce/blob/main/src/pages/Reviews.js#L42

Maybe something like:

let requestBody = {}
  
// option 1  
if (formData.rating) {
  requestBody.rating = formData.rating
}
// repeat the if statement for each of the fields (ie: title and comment)
// end option 1

// option 2 - extract into helper functions in util.js so that we don't need to keep repeating if statement
// util.js
const setFieldIfNonEmpty = (destinationObject, sourceObject, field) => {
  if (sourceObject[field]) {
    destinationObject[field] = sourceObject[field]
  }
  return destinationObject
};

const setFieldsFromSourceObject = (destinationObject, sourceObject) => {
  const fields = Object.keys(sourceObject)
  for (field of fields) {
    destinationObject = setFieldIfNonEmpty(destinationObject, sourceObject, field)
  }
  return destinationObject
}

//Reviews.js
// import setFieldsIfNotEmpty at top of file

requestBody = setFieldsFromSourceObject(requestBody, formData)

// end option 2
 
await reviewApi.updateReview(reviewId, requestBody)

[NatachaKey]

@akosasante i think we handled that with hasOwnProperty? in Postman seems to work fine and on the frontend too