CODEWITHSUNDEEP
node.jsmongodbmongoose
Taran Vohra
Taran Vohra

Reputation: 217

Multiple findOneAndUpdate operations are being skipped

I have a forEach loop where I am querying a document, doing some simple math calculations and then updating a document in the collection and move on to the next iteration.

The problem is, alot of times randomly some of the UPDATE operations will not update the document. I don't know why is it happening. Is it because of the lock?

I have tried logging things just before the update operation. The data is all correct but when it comes to update, it will randomly not update at all. Out of 10 iterations, lets say 8 will correctly work

    const name = "foo_game";
    players.forEach(({ id, team, username }) => {
      let updatedStats = {};
      Users.findOne({ id }).then(existingPlayer => {
        if (!existingPlayer) return;

        const { stats } = existingPlayer;
        const existingStats = stats[pug.name];

        if (!existingStats) return;
        const presentWins = existingStats.won || 0;
        const presentLosses = existingStats.lost || 0;
        updatedStats = {
          ...existingStats,
          won:
            team === winningTeam
              ? presentWins + 1
              : changeWinner
              ? presentWins - 1
              : presentWins,
          lost:
            team !== winningTeam
              ? presentLosses + 1
              : changeWinner
              ? presentLosses - 1
              : presentLosses,
        };

       // THE CALCULATIONS ARE ALL CORRECT TILL THIS POINT
       // THE UPDATE WIILL RANDOMLY NOT WORK
        Users.findOneAndUpdate(
          { id, server_id: serverId },
          {
            $set: {
              username,
              stats: { ...stats, [name]: updatedStats },
            },
          },
          {
            upsert: true,
          }
        ).exec();
      });
    });

Upvotes: 0

Views: 283

Answers (1)

Neil Lunn
Neil Lunn

Reputation: 151132

Basically what you are missing here is the asynchronous operations of both the findOne() and the findOneAndUpdate() are not guaranteed to complete before your foreach() is completed. Using forEach() is not a great choice for a loop with async operations in it, but the other main point here is that it's completely unnecessary since MongoDB has a much better way of doing this and in one request to the server.

In short, instead of "looping" you actually want to provide an array of instructions to bulkWrite():

let server_id = serverId; // Alias one of your variables or just change it's name

Users.bulkWrite(
  players.map(({ id, team, username }) => 
    ({
      "updateOne": {
        "filter": { _id, server_id },
        "update": {
          "$set": { username },
          "$inc": {
            [`stats.${name}.won`]:
              team === winningTeam ?  1 : changeWinner ?  - 1 : 0,
            [`stats.${name}.lost`]:
              team !== winningTeam ?  1 : changeWinner ?  - 1 : 0
          }
        },
        "upsert": true
      }
    })
  )
)
.then(() => /* whatever continuation here */ )
.catch(e => console.error(e))

So rather than looping, that Array.map() produces one "updateOne" statement within the bulk operation for each array member and sends it to the server. The other change of course is you simply do not need the findOne() in order to read existing values. All you really need is to use the $inc operator in order to either increase or decrease the current value. Noting here that if nothing is currently recorded at the specified path, then it would create those with whatever value of 1/-1/0 was determined by the logic and handed to $inc.

Note this is how you actually should be doing things in general, as aside from avoiding uneccesary loops of async calls the main thing here is to actually use the atomic operators like $inc that MongoDB has. Reading data state from the database in order to make changes is an anti-pattern and best avoided.

Upvotes: 2

Related Questions