Automattic / mongoose

MongoDB object modeling designed to work in an asynchronous environment.
https://mongoosejs.com
MIT License
26.96k stars 3.84k forks source link

pre, post middleware are not executed on findByIdAndUpdate #964

Closed skotchio closed 9 years ago

skotchio commented 12 years ago

Because a findAndUpdate method is presented to cut down the following code:

 Model.findById(_id, function (err, doc) {
      if (doc) {
          doc.field = 'value';
          doc.save(function (err) {
                 // do something;
          });
      }
 });

to this:

Model
   .findByIdAndUpdate(_id, {$set: {field: 'value'}}, function (err, doc) {
        // do something 
    });

We need to use pre, post middleware exactly the same. At now pre, post middleware are not executed when I make findByIdAndUpdate.

vkarpov15 commented 9 years ago

I should add more docs clarifying this - the document that's being updated may not exist in memory when you call findOneAndUpdate, so the this object refers to the query rather than the document in query middleware. Try this.update({ $set: { updatedAt: Date.now() } });

aggied commented 9 years ago

@vkarpov15 I could not get this.update({ $set: { updatedAt: Date.now() } }); to work, but I was able to successfully update the doc on the findOneAndUpdate hook with this: this._update['$setOnInsert'].updatedAt=Date.now();.

vkarpov15 commented 9 years ago

I would highly recommend not depending on tweaking internal state like that. I'm pretty surprised that you couldn't get this.update() to work - can you show me what your hook looks like?

aggied commented 9 years ago

Sure! (Note that I'm using 4.0.2)

tagSchema.pre('findOneAndUpdate',function(next){
  var self = this;

  //NOTE THAT 'this' in the findOneAndUpdate hook refers to the query, not the document
  //https://github.com/Automattic/mongoose/issues/964

  geoData.country.findOne({'_id':self._update['$setOnInsert'].countryCode}).select('_id name cca2 cca3 ccn3').lean().exec(function(err,country){
    if (err){throw err;}
    if (!country){throw 'no coutnry';}
    self._update['$setOnInsert'].country=country;
    next();
  });
});

I could obviously handle this when I initialize the document elsewhere in my app, but it's nice to have it all contained right there in Mongoose. Welcome any thoughts!

vkarpov15 commented 9 years ago

Yeah I was confused and thought you were using update. The below script

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

mongoose.connect('mongodb://localhost:27017/gh964');

var TodoSchema = new mongoose.Schema({
  name: {type: String, required: true},
  note: String,
  completed: {type: Boolean, default: false},
  updatedAt: {type: Date, default: Date.now},
  user: {
    type: mongoose.Schema.ObjectId,
    ref: 'Users'
  }
});
TodoSchema.pre('findOneAndUpdate', function() {
  this.findOneAndUpdate({}, { updatedAt: Date.now() });
});

var Todo = mongoose.model('Todo', TodoSchema);

Todo.findOneAndUpdate({}, { note: "1" }, function(err) {
  if (err) {
    console.log(err);
  }
  console.log('Done');
  process.exit(0);
});

Works correctly and executes the desired query:

Mongoose: todos.findAndModify({}) [] { '$set': { note: '1', updatedAt: new Date("Thu, 07 May 2015 20:36:39 GMT") } } { new: false, upsert: false }
Done

So please use

TodoSchema.pre('findOneAndUpdate', function() {
  this.findOneAndUpdate({}, { updatedAt: Date.now() });
});

Instead of manually manipulating mquery's internal state - that's usually a bad idea unless you really really know what you're doing.

agjs commented 9 years ago

Is this working or not ? Only hook that works for me is 'save', rest is completely ignored. I'm running on 4.0.6. Thanks

vkarpov15 commented 9 years ago

@agjs provide code example please.

agjs commented 9 years ago
UserController.prototype.updateAvatar = function (req, res) {
    return new Promise(function (resolve, reject) {
        CompanyDetails.update({
            _author: req.user._id
        }, {
            avatarPath: req.files.file
        }, function (error, updated) {
            if (error) {
                reject(error);
            } else {
                resolve(updated);
            }
        });
    }).then(function (resolved) {
        res.sendStatus(204).send(updated);
    }).catch(function (error) {
        next(error);
    })

};
CompanyAvatarSchema.pre('update', function (next) {
    console.log('pre save');
    let VirtualModel = this,
        parent = this.ownerDocument(),
        PATH = path.normalize('./public/images/uploads/avatars/' + parent._id);

    mkdirp(PATH, function (error) {
        if (error) {
            next(error);
        } else {
            fs.rename(VirtualModel.path, path.join(PATH, VirtualModel.name), function (error2) {
                if (error2) {
                    next(error2);
                } else {
                    next();
                }
            });
        }
    });

});
agjs commented 9 years ago

I have another pre-hook on another model with model.create and pre-save and it works normally.

With update whatsoever, It simply doesn't see the hook, doesn't even console.log it. I tried findOneAndUpdate too etc, doesn't really work... Funny thing I've went through this entire thread and as I usually do, checked official documentation and you guys claim even there that it works.

vkarpov15 commented 9 years ago

What's the relationship between CompanyAvatarSchema and CompanyDetails in the above code?

agjs commented 9 years ago

Company details have CompanyAvatar.schema as a subdocument

avatarPath: {
        type: [CompanyAvatar.schema],
        required: true
    }
agjs commented 9 years ago

Also, its not only pre-hook but validation gets ignored completely as well. This subdocument gets populated but ignores both, validation and pre-hook. I've googled everything, also tried THIS but nothing seams to work. When I just for test change my query to create and invoke model with new aka var parent = new Parent(), it does work.

vkarpov15 commented 9 years ago

You're calling CompanyDetails.update() but the pre hook is defined on a separate schema. Query middleware doesn't fire the nested schema's pre('update') atm.

Also, please provide a more thorough code example for your 'validation gets ignored completely' case as well.

agjs commented 9 years ago

Here is my company avatar schema made for validation and pre-hooks for users who are updating their profile (avatar photo):

'use strict';

let mongoose = require('mongoose'),
    mkdirp = require('mkdirp'),
    fs = require('fs'),
    path = require('path'),
    Schema = mongoose.Schema;

let CompanyAvatarSchema = new Schema({
    name: String,
    width: Number,
    height: Number,
    size: Number,
    type: String
});

CompanyAvatarSchema.path('type').validate(function (type) {
    return /^image\//.test(type);
}, 'Image type not allowed!');

CompanyAvatarSchema.path('size').validate(function (size) {
    return size < 5;
}, 'Image too big!');

CompanyAvatarSchema.virtual('path').set(function (path) {
    return this._path = path;
}).get(function () {
    return this._path;
});

CompanyAvatarSchema.virtual('public_path').get(function () {
    var parent = this.ownerDocument();
    var PATH = path.normalize('images/uploads/avatars/' + parent._id);
    if (this.name) {
        return path.join(PATH, this.name);
    }
});

CompanyAvatarSchema.set('toJSON', {
    getters: true
});

CompanyAvatarSchema.pre('findOneAndUpdate', function (next) {
    console.log('pre save');
    let VirtualModel = this,
        parent = this.ownerDocument(),
        PATH = path.normalize('./public/images/uploads/avatars/' + parent._id);

    mkdirp(PATH, function (error) {
        if (error) {
            next(error);
        } else {
            fs.rename(VirtualModel.path, path.join(PATH, VirtualModel.name), function (error2) {
                if (error2) {
                    next(error2);
                } else {
                    next();
                }
            });
        }
    });

});

let runValidatorsPlugin = function (schema, options) {
    schema.pre('findOneAndUpdate', function (next) {
        this.options.runValidators = true;
        next();
    });
};

CompanyAvatarSchema.plugin(runValidatorsPlugin);

let CompanyAvatar = mongoose.model('CompanyAvatar', CompanyAvatarSchema);
module.exports = CompanyAvatar;

Here is the company_details schema where company_avatar is a subdocument :

let CompanyDetailsSchema = new mongoose.Schema({
    _author: [{
        type: Schema.Types.ObjectId,
        ref: 'CompanyAccount'
    }],
    company_name: {
        type: String,
        es_indexed: true,
        es_boost: 2.0
    },
    contact_email: {
        type: String,
        es_indexed: true
    },
    website: {
        type: String,
        es_indexed: true
    },
    country: {
        type: String,
        es_indexed: true
    },
    industry: {
        type: String,
        es_indexed: true
    },
    address: {
        type: String,
        es_indexed: true
    },
    about: {
        type: String,
        es_indexed: true
    },
    avatarPath: {
        type: [CompanyAvatar.schema],

    }
});

And here is update profile controller and the avatarPath which should be validated / hooked before this update performs:

UserController.prototype.updateAvatar = function (req, res, next) {
    let updates = {
        $set: {
            avatarPath: req.files.file
        }
    };
    return new Promise(function (resolve, reject) {
        CompanyDetails.findOneAndUpdate({
            _author: req.user._id
        }, updates, function (error) {
            if (error) {
                reject(error);
            } else {
                resolve('done');
            }
        });
    }).then(function () {
        res.sendStatus(204);
    }).catch(function (error) {
        next(error);
    });

};

Basically, my mongodb gets populated with the fields from req.files.file but other then that, validation gets ignored and no hooks are working.

vkarpov15 commented 9 years ago

The issue is that the pre('findOneAndUpdate') middleware is defined on a nested schema. Right now, mongoose only fires query middleware for top level schemas, so middlewares defined on CompanyDetailsSchema will get fired re: #3125

tennessine commented 9 years ago

+1

willemmulder commented 9 years ago

@vkarpov15 I get that

TodoSchema.pre('findOneAndUpdate', function() {
  this.findOneAndUpdate({}, { updatedAt: Date.now() });
});

works to set a property to a hard value, but how would I read and change a property, e.g. hash a password?

TodoSchema.pre('findOneAndUpdate', function() {
  this.findOneAndUpdate({}, { password: hashPassword(.....?....) });
});

Any thoughts? This is a pretty common usecase, right? Or do people usually find() and then separate save() ?

vkarpov15 commented 9 years ago
TodoSchema.pre('findOneAndUpdate', function() {
  this.findOneAndUpdate({}, { password: hashPassword(this.getUpdate().$set.password) });
});

That should work @willemmulder.

willemmulder commented 9 years ago

@vkarpov15 perfect, thanks! I'll be trying that tonight. That should also work for pre('update') right?

willemmulder commented 9 years ago

@vkarpov15 So I just tried it using

schema.pre('update', function(next) {
this.update({}, { $set : { password: bcrypt.hashSync(this.getUpdate().$set.password) } });
next();
});

and if I console.log this I do get

{ '$set':
    { password: '$2a$10$CjLYwXFtx0I94Ij0SImk0O32cyQwsShKnWh1248BpYsJLIHh7jb66',
        postalAddress: [Object],
        permissions: [Object],
        firstName: 'Willem',
        lastName: 'Mulder',
        email: '...@...',
        _id: 55ed4e8b6de4ff183c1f98e8 } },

which seems fine (and I even tried setting that property directly before) but in the end, it doesn't actually write the hashed value to the database, but simply the 'raw' value. Is there anything I could try?

vkarpov15 commented 9 years ago

That's strange. You can try enabling mongoose debug mode with require('mongoose').set('debug', true); and see what the query that's getting sent to the db is, that could shed some light.

willemmulder commented 9 years ago

Thanks for the suggestion. Just did that:

I run this:

schema.pre('update', function(next) {
    this.update({}, { password: bcrypt.hashSync(this.getUpdate().$set.password) } );
    console.log(this.getUpdate());
    next();
});

which returns this for the console.log

{ '$set':
   { password: '$2a$10$I1oXet30Cl5RUcVMxm3GEOeTFOLFmPWaQvXbr6Z5368zbfpA8nFEK',
     postalAddress: { street: '', houseNumber: '', zipCode: '', city: '', country: '' },
     permissions: [ '' ],
     __v: 0,
     lastName: '',
     firstName: '',
     email: 'test@test.nl',
     _id: 563b0410bd07ce2030eda26d } }

and then this for the Mongoose debug

Mongoose: users.update({ _id: ObjectId("563b0410bd07ce2030eda26d") }) { '$set': { password: 'test', postalAddress: { street: '', houseNumber: '', zipCode: '', city: '', country: '' }, permissions: [ '\u001b[32m\'\'\u001b[39m' ], __v: 0, lastName: '', firstName: '', email: 'test@test.nl', _id: ObjectId("563b0410bd07ce2030eda26d") } } { overwrite: false, strict: true, multi: false, upsert: false, safe: true }
Mongoose: users.findOne({ _id: ObjectId("563b0410bd07ce2030eda26d") }) { fields: { password: 0 } }

Any clue?

vkarpov15 commented 9 years ago

Not sure, opened up a new issue to track.

willemmulder commented 9 years ago

@vkarpov15 Thanks, will track the other issue.

akoskm commented 8 years ago

@vkarpov15 I think the right way to set options for the ongoing query in pre hook would be something like:

  finishSchema.pre('findOneAndUpdate', function (next) {
    this.setOptions({
      new: true,
      runValidators: true
    });
    this.update({}, {
      lastEdited: Date.now()
    });
    next();
  });

but the documentation, http://mongoosejs.com/docs/api.html#query_Query-setOptions doesn't mention any of those options. If this considered to be a hackish solutions, what would be more appropriate?

vkarpov15 commented 8 years ago

That's an issue with the docs, that code you describe looks like it should work at first glance

vkarpov15 commented 8 years ago

Can you open up a separate issue for that?

akoskm commented 8 years ago

@vkarpov15 Yes, it does work. I think I wasn't clear enough.

setOptions applies new and runValidators correctly, I was merely asking if setting those options through setOptions should be preferred over this.options.

vkarpov15 commented 8 years ago

setOptions() is preferable IMO, but both should work. Or you could just do

this.update({}, { lastEdited: Date.now() }, { new: true, runValidators: true });
nlonguit commented 8 years ago
schema.pre('update', function(next) {
this.update({}, { $set : { password: bcrypt.hashSync(this.getUpdate().$set.password) } });
next();
});

This will update password on every update() call. So if i just change the value of other properties, i.e. name or age, then password will be updated also which is not correct!?

akoskm commented 8 years ago

@nlonguit I guess it will. But you can access through this the fields that are going to be updated and you could do something like:

if (this._fields.password) { // <- I'm sure about this one, check in debugger the properties of this 
    this.update({}, { $set : { password: bcrypt.hashSync(this.getUpdate().$set.password) } });
}
nlonguit commented 8 years ago

if (this._update.$set.password) { this.update({}, { $set: { password: bcrypt.hashSync(this.getUpdate().$set.password)} }); }

This code is working well for me. Thanks @akoskm

mickyginger commented 8 years ago

I wonder if it would be possible to add a pre hook for findByIdAndUpdate as well. Would be nice to have both hooks available.

vkarpov15 commented 8 years ago

@mickyginger we have hooks for findOneAndUpdate, and findByIdAndUpdate() just calls findOneAndUpdate() under the hood

keegho commented 8 years ago

I did it this way and it is working: Just findById then save without updating any fields then use the findByIdAndUpdate method:

dbModel.findById(barId, function (err, bar) {
        if (bar) {

            bar.save(function (err) {
                if (err) throw err;
            });
        }
    });
    dbModel.findByIdAndUpdate(barId, {$set:req.body}, function (err, bar) {
        if (err) throw err;
        res.send('Updated');
    });`
zilions commented 8 years ago

I'm trying to set a property to have the length of an array.

schema.post('findOneAndUpdate', function(result) {
    console.log(result.comments.length);
    this.findOneAndUpdate({}, { totalNumberOfComments: result.comments.length });
});

The correct length is logged, although the query never sets totalNumberOfComments, and the field stays at 0 (since the schema references default: 0).

When I console.log(this) at the end of the hook, I can see that my query contains the following:

_update: { '$push': { comments: [Object] }, totalNumberOfComments: 27 }

Although when I turn on debug mode, a query is never logged by Mongoose.

Is there something I'm doing wrong, or is this a bug?

vkarpov15 commented 8 years ago

@zilions this.findOneAndUpdate({}, { totalNumberOfComments: result.comments.length }).exec(); need to actually execute the query :) Just be careful, you're gonna get an infinite recursion there because your post save hook will trigger another post save hook

zilions commented 8 years ago

@vkarpov15 Ahhhh right! Then I can just use this.update({} {....}).exec() instead :) Question though, when using this, it sets the totalNumberOfComments field perfectly, although performs the original update of the findOneAndUpdate too.

For example:

Post.findOneAndUpdate({_id: fj394hri3hfj}, {$push: {comments: myNewComment}})

Will trigger the following hook:

schema.post('findOneAndUpdate', function(result) {
    this.update({}, {
        totalNumberOfComments: result.comments.length
    }).exec();
}));

Although, the hook will $push to comments the myNewComment again, therefore making a duplicate entry.

vkarpov15 commented 8 years ago

Because you're technically executing the same query -

schema.post('findOneAndUpdate', function(result) {
    this.update({}, {
        totalNumberOfComments: result.comments.length
    }).exec();
}));

is essentially the same as

var query = Post.findOneAndUpdate({_id: fj394hri3hfj}, {$push: {comments: myNewComment}});
query.update({}, {
        totalNumberOfComments: result.comments.length
    }).exec();
query.findOneAndUpdate().exec();

If you want to create a new query from scratch, just do

schema.post('findOneAndUpdate', function(result) {
    this.model.update({}, { // <--- `this.model` gives you access to the `Post` model
        totalNumberOfComments: result.comments.length
    }).exec();
}));
moparlakci commented 8 years ago

just dont touch your pre save hook,

router.put('/:id', jsonParser, function(req, res, next) {

  currentCollection.findByIdAndUpdate(req.params.id, req.body, function (err, item) {
    if (err) {
        res.status(404);
        return res.json({'error': 'Server Error', 'trace': err});
    }
    item.save(); // <=== this is were you save your data again which triggers the pre hook :)
    res.status(200); 
    return res.json({'message': 'Saved successfully'});
  });
});
nicky-lenaers commented 7 years ago

I found that the order matters in which you define a model and define a pre hook. Allow me to demonstrate:

Does not work:

// Create Model
let model = Database.Connection.model(`UserModel`, this._schema, `users`);

// Attach Pre Hook
this._schema.pre(`findOneAndUpdate`, function(next) {
    console.log('pre update');
    return next();
});

Does work:

// Attach Pre Hook
this._schema.pre(`findOneAndUpdate`, function(next) {
    console.log('pre update');
    return next();
});

// Create Model
let model = Database.Connection.model(`UserModel`, this._schema, `users`);

Hope this helps anyone!

albert-92 commented 7 years ago

I just found out the same thing as @nicky-lenaers.

It works just fine with 'safe'. 'delete'. etc. if you define the hooks after the model is defined.

Is there a workaround do define a 'findOneAndUpdate' hook after the model is defined?

vkarpov15 commented 7 years ago

@albert-92 no not at the moment

marcusjwhelan commented 7 years ago

For anyone trying to get something that used to be like

SCHEMA.pre('validate', function(done) {
    // and here use something like 
    this.yourNestedElement 
    // to change a value or maybe create a hashed character    
    done();
});

This should work

SCHEMA.pre('findOneAndUpdate', function(done){
    this._update.yourNestedElement
    done();
});
johndeyrup commented 7 years ago

I can't get post hooks to update the document in the collection.

`module.exports = function (mongoose) { var mySchema = mongoose.Schema({ id: { type: Number, index: { unique: true } }, field1: { type: String }, field2: { type: String} }, { collection: "mySchema", versionKey: false });

mySchema.post('findOneAndUpdate', function (result) {
    this.model.update({}, {
        field2: 'New Value'
    }).exec();
});
return mySchema;

}`

mySchema.findOneAndUpdate({id: 1}, {field1: 'test'}, {new: true});

Sets field in collection to { id:1, field1: 'test' ) but should be {id: 1, field1: 'test', field2:'New Value'} Not sure what I am doing wrong

I can change change the result of the findOneAndUpdate by doing this mySchema.post('findOneAndUpdate', function (result) { result.field2 = 'something' });

marcusjwhelan commented 7 years ago

I think it might be that you are trying to update the model with an element that already exists on the model. Or possibly that you are selecting it wrong. Try printing out "this" in your mySchema.post. Also you don't seem to have a done() or next() in your post. I'm not very knowledgable on the subject but I know printing out this will at least give you an idea of what you are dealing with.

johndeyrup commented 7 years ago

Isn't the point of update to change an existing document in your model?

this is a query object

You don't need done or next in post hooks as far as I understand.

marcusjwhelan commented 7 years ago

Well you have this.model.update which is the schema not the model of the object. I think.. Which means you would have to use

mySchema.post('findOneAndUpdate', function (result) {
    this.model.update({}, {
        $set: { field2: 'New Value'}
    }).exec();
});
return mySchema;

This seems a little backwards to be calling a model function inside of the model. Since you could just use parts of the "this" object that is given to you. you might be better off just using the findOneAndUpdate instead of calling it and then calling another model function on top of it. in a manager

var data = yourNewData;
self.findOneAndUpdate({id: something._id}, data, {safe: false, new: true})
    .exec()
    .then(resolve)
    .catch(reject);

In my example above I used this._update because that was the update object that needed to be used off of this.

johndeyrup commented 7 years ago

I have tried using $set. It still doesn't change my document in the collection.

jcyh0120 commented 7 years ago

Where can I find the all available pre and post hooks?