2

I have built a simple MERN app where users can rate phone numbers. Users just fill in the phone number, choose rating (1 - 5 star rating), their city & short text. The app has search function with filter & sorting options. It all works good enough ATM but I think it might break when multiple concurrent users use the website because I update the phone number model (mobileSchema) after a rating (messageSchema) has been submitted - using Mongoose middlewares (post hooks).

For example, I need to calculate number of ratings (messagesCount) for phone number. I use Message.countDocuments({ mobile: mobile._id }) for that. However, I also need to update other properties of phone number (mobileSchema - lastMessageDate, globalRating, averageRating) so that operation takes some time. I believe the number of ratings might not be right when 2 users submit rating at the same time - it will increment the number of ratings (messagesCount) by 1 instead of 2.

Is there a better approach? Can a post hook be fired after the previous post hook already finished?

Sample code:

const mobileSchema = new Schema({
    number: { type: String, required: true },
    plan: { type: String, required: true },
    date: { type: Date, default: Date.now, required: true, index: 1 },
    messagesCount: { type: Number, default: 0, index: 1 },
    lastMessageDate: { type: Date, index: 1 },
    // normal mean
    globalRating: { type: Number, default: 0, index: 1 },
    // weighted mean
    averageRating: { type: Number, default: 0, index: 1 }
});

const messageSchema = new Schema({
    comment: { type: String, required: true },
    city: { type: Schema.Types.ObjectId, ref: 'City', required: true, index: 1 },
    rating: { type: Number, required: true, index: 1 },
    date: { type: Date, default: Date.now, required: true, index: 1 },
    mobile: { type: Schema.Types.ObjectId, ref: 'Mobile', required: true },
    user: { type: Schema.Types.ObjectId, ref: 'User', required: true }
});

messageSchema.post('save', function (message, next) {
    const messageModel = this.constructor;
    return updateMobile(messageModel, message, next, 1);
});

const updateMobile = (messageModel, message, next, addMessage) => {
    const { _id } = message.mobile;
    const cityId = message.city._id;
    const lastMessageDate = message.date;
    let mobile;
    hooks.get(Mobile, { _id })
        .then(mobileRes => {
            mobile = mobileRes;
            return Message.countDocuments({ mobile: mobile._id })
        })
        .then(messagesCount => {
            if (messagesCount <= 0) {
                const deleteMobile = Mobile.findOneAndDelete({ _id: mobile._id })
                const deleteSeen = SeenMobile.findOneAndDelete({ mobile: mobile._id, user: message.user._id })
                const cityMobile = updateCityMobile(messageModel, mobile, cityId)
                Promise.all([deleteMobile, deleteSeen, cityMobile])
                    .then(() => {
                        return next();
                    })
                    .catch((err) => {
                        console.log(err);
                        return next();
                    })
            }
            else {
                if (addMessage === -1) lastMessageDate = mobile.lastMessageDate;
                const ratings = hooks.updateGlobalRating(mobile, messageModel)
                    .then(() => hooks.updateAverageRating(mobile, messageModel))
                    .then(() => {
                        return new Promise((resolve, reject) => {
                            mobile.set({
                                messagesCount,
                                lastMessageDate
                            });
                            mobile.save((err, mobile) => {
                                if (err) return reject(err);
                                resolve();
                            });
                        })
                    })
                const cityMobile = updateCityMobile(messageModel, mobile, cityId)
                Promise.all([ratings, cityMobile])
                    .then(([ratings, cityMobile]) => {
                        return next();
                    })
                    .catch(err => console.log(err))
            }
        })
        .catch(err => {
            console.log(err);
        })
}
Jihlavanka
  • 370
  • 2
  • 10
  • 1
    Not directly mongoose related but [here](https://stackoverflow.com/questions/17456671/to-what-level-does-mongodb-lock-on-writes-or-what-does-it-mean-by-per-connec) is a discussion about write operations. Based on that information I would say the post hook fires after the write operation is finished but I didn't check mongoose documentation. – Janne Dec 01 '18 at 14:04

3 Answers3

3

I think you are always going to run into async issues with your approach. I don't believe you can "synchronize" the hooks; seems to go against everything that is true about MongoDB. However, at a high level, you might have more success grabbing the totals/summaries at run-time, rather than trying to keep them always in sync. For instance, if you need the total number of messages for a given mobile device, why not:

Messages.find({mobile: mobile._id})

and then count the results? That will save you storing the summaries and keeping them updated. However, I also think your current approach could work, but you probably need to scrap the "countDocuments". Something a bit more async friendly, like:

Mobile.aggregation([
    { $match: { _id: mobile._id } },
    { $add: [ "$mobile.messagesCount", 1 ] }
]);

Ultimately I think your design would be strengthened if you stored Messages as an array inside of Mobile, so you can just push the message on it. But to answer the question directly, the aggregation should keep everything tidy.

piisexactly3
  • 779
  • 7
  • 16
  • "grabbing the totals/summaries at run-time": I do not think that would be performance friendly. I would have to calculate all properties (messagesCount, lastMessageDate, globalRating, averageRating...) on each request from client. So simple filter & sort would take too long. "stored Messages as an array inside of Mobile": I am afraid that it could grow above max MongoDB document size - 16 MB. – Jihlavanka Dec 01 '18 at 14:40
  • 1
    Fair enough. The 2nd point is well taken, but regarding the 1st point, have you considered using caching like Redis? Caching in the database seems like a lot of strain. – piisexactly3 Dec 01 '18 at 14:54
0

I found this answer: Locking a document in MongoDB

I will calculate all the values I need (messagesCount, globalRating etc.) in post hook and then I will check if the mobile document has the same __v (version) value during final findOneAndUpdate operation (because this operation locks the document and can increment __v). If it has different __v then I will call the post hook again to ensure it will calculate the right values.

Jihlavanka
  • 370
  • 2
  • 10
0

First we need to fix some database structure here

Mobile schema

const mobileSchema = new Schema({
  number: { type: String, required: true },
  plan: { type: String, required: true },
  date: { type: Date, default: Date.now, required: true, index: 1 },
  //messagesCount: { type: Number, default: 0, index: 1 },
  //lastMessageDate: { type: Date, index: 1 },
  // normal mean
  //globalRating: { type: Number, default: 0, index: 1 },
  // weighted mean
  //averageRating: { type: Number, default: 0, index: 1 }
});

Message schema

const messageSchema = new Schema({
  comment: { type: String, required: true },
  city: { type: Schema.Types.ObjectId, ref: 'City', required: true, index: 1 },
  //rating: { type: Number, required: true, index: 1 },
  date: { type: Date, default: Date.now, required: true, index: 1 },
  mobile: { type: Schema.Types.ObjectId, ref: 'Mobile', required: true },
  user: { type: Schema.Types.ObjectId, ref: 'User', required: true }
});

Rating system (take all rating or make them a set) (numerator & denominator after 100 ratings it is difficult to read every single one) can also check for the mobile

const ratingSchema = new Schema({
  mobile: { type: String, required: true },
  commmentId:{type:String, required: true, index: 1}
  rate: { type: Number required: true,  },
  //rating: { type: Number, required: true, index: 1 },
  timestamp: { type: Date, default: Date.now, required: true, index: 1 }
  denominator:{ type: Number},
  numerator:{type:Number}
});

Thanks

Vinayk93
  • 353
  • 1
  • 6