Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong Options Used in Model.$__handleSave #8739

Closed
makinde opened this issue Mar 28, 2020 · 2 comments · Fixed by #8764
Closed

Wrong Options Used in Model.$__handleSave #8739

makinde opened this issue Mar 28, 2020 · 2 comments · Fixed by #8764
Labels
confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. has repro script There is a repro script, the Mongoose devs need to confirm that it reproduces the issue
Milestone

Comments

@makinde
Copy link
Contributor

makinde commented Mar 28, 2020

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

What is the current behavior?
When you try to save a document that has no changes, mongoose will run a Model.exists() query to make sure the document is in the DB (I don't know why this is done, but that's a different story). That query is done with a subset of the options that were passed to Document.save() call, not the original options. This breaks my middleware because Model.exists() triggers a Document.findOne() middleware, but the extra options that I have passed in are not sent with this new query.

The relevant line is 316 in lib/model.js.

    } else {
      this.constructor.exists(this.$__where(), saveOptions)
        .then((documentExists)=>{
          if (!documentExists) throw new DocumentNotFoundError(this.$__where(),this.constructor.modelName);

          this.$__reset();
          callback();
        })
        .catch(callback);
      return;
    }

saveOptions there is a stripped down version of the original options passed in. I think options should be used there, or something similar to it.

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

const doc = new MyModel({name: 'john'});
await doc.save({ customOpt: 1 }); // works
await doc.save({ customOpt: 1 }); // customOpt isn't passed to the Model.exists query

This seems like a far fetched case. But I ran into this by having a Document.save() call inside a session.withTransaction() call. withTransaction would then rerun the function after a TransientTransactionError, triggering this edge case where the document is saved twice.

What is the expected behavior?
The custom options should be passed through to the Model.exists() method.

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

@vkarpov15 vkarpov15 added this to the 5.9.8 milestone Apr 2, 2020
@vkarpov15 vkarpov15 added the needs repro script Maybe a bug, but no repro script. The issue reporter should create a script that demos the issue label Apr 2, 2020
@AbdelrahmanHafez
Copy link
Collaborator

This will be fixed by #8097

I'll dig some more into this and see if we can have a fix for this before v6.0.

@AbdelrahmanHafez
Copy link
Collaborator

Reproduction script:

// 8739
'use strict';

const mongoose = require('mongoose');
const { Schema } = mongoose;
const assert = require('assert');

mongoose.connect('mongodb://localhost:27017/test', { useNewUrlParser: true, useUnifiedTopology: true });

const userSchema = new Schema({ name: String });

let optionInMiddleware;

userSchema.pre('findOne', function(next) {
  optionInMiddleware = this.getOptions().customOption;

  return next();
});

const User = mongoose.model('User', userSchema);


async function run() {
  await User.deleteMany();

  const user = await User.create({ name: 'Hafez' });

  await user.save({ customOption: 'test' });

  assert.equal(optionInMiddleware, 'test');

  console.log('Done.');
}


run().catch(console.error);

AbdelrahmanHafez added a commit to AbdelrahmanHafez/mongoose that referenced this issue Apr 3, 2020
@AbdelrahmanHafez AbdelrahmanHafez added has repro script There is a repro script, the Mongoose devs need to confirm that it reproduces the issue confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. and removed needs repro script Maybe a bug, but no repro script. The issue reporter should create a script that demos the issue labels Apr 3, 2020
vkarpov15 added a commit that referenced this issue Apr 3, 2020
[WIP] pass custom options from save to Model.exists(...) when no changes are present
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. has repro script There is a repro script, the Mongoose devs need to confirm that it reproduces the issue
Projects
None yet
3 participants