DocumentNotFoundError instead of VersionError during save

[mjfwebb]

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

What is the current behavior?
This is the same issue as described in #10974

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

Using the same script created by @IslandRhythms in #10974 (comment)

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

const robotSchema = new mongoose.Schema({
  name: String
}, { optimisticConcurrency: true });

const Robot = new mongoose.model('Robot', robotSchema);

async function run() {
  mongoose.connect('mongodb://localhost:27017/', {
    useNewUrlParser: true,
    useUnifiedTopology: true,
  });
  await mongoose.connection.dropDatabase();
  const entry = await Robot.create({ name: 'WallE' });
  const changes = await Robot.findOne({ _id: entry._id }).orFail().exec();
  const other = await Robot.findOne({ _id: entry._id }).orFail().exec();
  changes.name = 'John';
  await changes.save();
  other.name = 'WallE';
  await other.save();
}

run();

I am getting a DocumentNotFound:

DocumentNotFoundError: No document found for query "{ _id: new ObjectId("61f578fc226164d0c2fb411e"), __v: 0 }" on model "Robot"

What is the expected behavior?

A VersionError should be thrown, not a DocumentNotFound error.

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

Mongoose: 6.1.8
Node: v16.6.1

[twhy]

Although this has been fixed, I want to explain the cause of this bug and another possible fix, in case someone else is interested at this.

The cause of this bug is the line of code below. It assigns the same name value as created to other.name.
Remove this line and you still get the same error(DocumentNotFoundError ).

other.name = 'WallE'; 

If you assign a different value to other.name, you'll get VersionError as expected, because the original code covered this case.

other.name = 'Steve';   // get VersionError as expected

But why?
Because other.name = 'WallE'; didn't change other, so mongoose doesn't think other is 'dirty'.
But other.name = 'Steve' does change other, so Mongoose will know other is dirty and handle this differently.

https://github.com/Automattic/mongoose/blob/master/lib/model.js#L314

Model.prototype.$__handleSave = function(options, callback) {
// ...
  const delta = this.$__delta();
  if (delta) {
    // model has some changes, this case works well
  } else {
    // model has no changes, may fix this bug in this else block
  }

Mongoose use this.$__delta() to produces a special query document of the modified properties used in updates.
In the case of other.name = 'WallE';, the value of delta is undefined because other has no changes.
But in the case of other.name = 'Steve', delta is the array below.

[
  { _id: new ObjectId("..."), __v: 0 },
  { '$set': { name: 'Steve' }, '$inc': { __v: 1 } }
]

Since Mongoose has handled this case well, let's see how it handles the other.name = 'WallE'; case.
https://github.com/Automattic/mongoose/blob/master/lib/model.js#L338-L360

  } else {
    const optionsWithCustomValues = Object.assign({}, options, saveOptions);
    const where = this.$__where();
    if (this.$__schema.options.optimisticConcurrency) {
      const key = this.$__schema.options.versionKey;
      const val = this.$__getValue(key);
      if (val != null) {
        where[key] = val;
      }
    }
    // where = { _id: new ObjectId("..."), __v: 0 }
    this.constructor.exists(where, optionsWithCustomValues).
      then((documentExists) => {
        // Check whether document has beed changed before updating
        if (!documentExists) {
          // document has been changed, in this case, __v is 1 in the database
          const matchedCount = 0;
          return callback(null, { $where: where, matchedCount });
        }

        const matchedCount = 1;
        callback(null, { $where: where, matchedCount });
      }).
      catch(callback);
    return;
  }

Later in $__save() method, $__handleSave() is called.

Model.prototype.$__save = function(options, callback) {
  // ...
  if (this.$__.version && !this.$__.inserting) {   // This is the line of code before the fix
    // this.$__.version is undefined so it goes to the next if block
    // We want it go into this if block after the fix
    // ...
    if (numAffected <= 0) {
      // the update failed. pass an error back
      this.$__undoReset();
      const err = this.$__.$versionError ||
        new VersionError(this, version, this.$__.modifiedPaths);
      return callback(err);
    }
    // ...
  }
  if (result != null && numAffected <= 0) {
    // result = { $where: where, matchedCount } in the code block above
    // and hence DocumentNotFoundError instead of VersionError
    error = new DocumentNotFoundError(result.$where,
      this.constructor.modelName, numAffected, result);
    // ...
  }
}

Below is my fix of this bug, and it passes all the latest related tests, so I consider it a possible fix.
Correct me if I was wrong.

  } else {
    const optimisticConcurrency = this.$__schema.options.optimisticConcurrency
    const optionsWithCustomValues = Object.assign({}, options, saveOptions);
    const where = this.$__where();
    if (optimisticConcurrency) {
      // ...
    }
    this.constructor.exists(where, optionsWithCustomValues).
      then((documentExists) => {
        if (!documentExists) {
          if (optimisticConcurrency) { // the fix here
            this.$__.version = VERSION_INC;
          }
          const matchedCount = 0;
          return callback(null, { $where: where, matchedCount });
        }
        // ...
      }).
      catch(callback);
    return;
  }

Basically it sets this.$__.version = VERSION_INC; if document has been changed and and optimisticConcurrency is true. It makes sure that the code goes into the right if block in $__save() method and generate a VersionError when necessary.