Filter on __v missing after push() to nested array

[lee-alexander]

Do you want to request a feature or report a bug?
Bug

What is the current behavior?
Calling push() on an array field on an object inside an array does not add a query filter on __v as expected. This is important for concurrency because the indicies of the top-level array may have changed. Setting non-array fields on the nested object correctly adds a filter on __v.

If the current behavior is a bug, please provide the steps to reproduce.
Code:

const PostModel = mongoose.model(
  'Post',
  new mongoose.Schema({
    comments: [{likedBy: [String]}],
  })
);
// ...
const post = await PostModel.findById(postId).exec();
const comment = post.comments[0];
comment.likedBy.push('Some User');
await post.save();

Output:

posts.updateOne(
  { _id: ObjectId("61ba9a5febe81b83ce3945f9") }, 
  { 
    '$push': { 
      'comments.0.likedBy': { '$each': [ 'Some User' ] } 
    }, 
    '$inc': { __v: 1 }}, 
  { session: null })

What is the expected behavior?

posts.updateOne(
  { _id: ObjectId("61ba9a5febe81b83ce3945f9"), __v: 48 }, 
  { 
    '$push': { 
      'comments.0.likedBy': { '$each': [ 'Some User' ] } 
    }, 
    '$inc': { __v: 1 }}, 
  { session: null })

This incorrect behavior is shared by $push, $addToSet, $pullAll, and $pull. We can see that these exit early without checking to see if we are operating on a subpath of array (final else-if)

model.js

  if (op === '$push' || op === '$addToSet' || op === '$pullAll' || op === '$pull') {
    self.$__.version = VERSION_INC;
  } else if (/^\$p/.test(op)) {
    // potentially changing array positions
    increment.call(self);
  } else if (Array.isArray(val)) {
    // $set an array
    increment.call(self);
  } else if (/\.\d+\.|\.\d+$/.test(data.path)) {
    // now handling $set, $unset
    // subpath of array
    self.$__.version = VERSION_WHERE;
  }

What are the versions of Node.js, Mongoose and MongoDB you are using? Note that "latest" is not a version.
Node.js: 14.7.1
MongoDB: 4.2.1
Mongoose: 6.1.2

[IslandRhythms]

const mongoose = require('mongoose');
mongoose.set('debug', true)

const PostModel = mongoose.model(
    'Post',
    new mongoose.Schema({
      comments: [{likedBy: [String]}],
    })
  );
  // ...
  async function run() {
    await mongoose.connect('mongodb://localhost:27017/', {useNewUrlParser: true,
    useUnifiedTopology: true,});
    await mongoose.connection.dropDatabase();
    const entry = await PostModel.create({
        comments: [{likedBy: ['Friends', 'Family']}]
    });
    console.log(entry, entry.comments[0].likedBy)
    const post = await PostModel.findById(entry._id).exec();
    const comment = post.comments[0];
    comment.likedBy.push('Some User');
    await post.save();
    console.log(post, post.comments[0].likedBy);
  }
run();

[lee-alexander]

This is not strictly related to the bug on this issue, but I'd also suggest we fix the code block in question to more safely assign to $__.version to avoid repeated calls to this code wiping out prior bits.

Here:

  if (op === '$push' || op === '$addToSet' || op === '$pullAll' || op === '$pull') {
    // OLD: self.$__.version = VERSION_INC;
    self.$__.version = (self.$__.version || 0) | VERSION_INC;
  }

And here:

  } else if (/\.\d+\.|\.\d+$/.test(data.path)) {
    // OLD: self.$__.version = VERSION_WHERE;
    self.$__.version = (self.$__.version || 0) | VERSION_WHERE;
  }

[vkarpov15]

This is expected behavior, because push() becomes a MongoDB $push operation, which means no risk of unintentional conflicting changes.

If you want to be more defensive in concurrency, you can enable optimistic concurrency.

[lee-alexander]

@vkarpov15 - I'm aware that $push is a safe operation and does not usually require this behavior.

The issue is that the $push is happening to a nested portion of the document:

    '$push': { 
      'comments.0.likedBy': { '$each': [ 'Some User' ] } 
    }, 

If the comments has shifted in the meantime, then the $push will be applied to comments.0 which could be an entirely different item!

This is the exact reason that $set on a nested array item checks __v, and in this case $push / $pull / etc should as well. The specific operation doesn't matter - just that it is happening to a nested array item.

[lee-alexander]

@vkarpov15 - here is a proposed fix according to my comment above.

lee-alexander@c61c910

[vkarpov15]

@lee-alexander at first glance that fix looks good. Feel free to put in a PR 👍

[lee-alexander]

Hey @vkarpov15 - I think 2d6bbf9 introduces a new bug / regression. Now any subpath of array will only set VERSION_WHERE. In many cases we also want the VERSION_INC too if the operation modifies array indices (a $pull for example).

Happy to follow up with a PR next week.