Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Remove hook not triggered when removing documents #1241

Closed
SirPepe opened this Issue · 22 comments

8 participants

@SirPepe

The pre/post hooks for remove are only triggered when using the document instance's remove() method, not when the model's methods are used:

var mongoose = require("mongoose");
var db = mongoose.createConnection('mongodb://localhost/experiment');
db.once('open', function(){

  testSchema = new mongoose.Schema({ title: String });
  testSchema.post('remove', function(removed){
    console.log('removed', removed.title);
  });
  var Test = db.model('Test', testSchema);

  // > "removed A"
  new Test({ title: 'A' }).save(function(err, created){
    created.remove();
  });

  // Nothing :(
  Test({ title: 'B' }).save(function(err, created){
    Test.remove({ title: 'B' }).exec();
  });

  // Nothing :(
  Test({ title: 'C' }).save(function(err, created){
    Test.find({ title: 'C' }).remove();
  });

});
@aheckmann
Owner

this works as expected. its the same thing for Model.update. no middleware is called b/c middleware only execute on mongoose documents which are not involved when sending a remove command directly to MongoDB through the use of Model.remove().

I'll leaving this open for now until I fix the Model.remove docs to be inline with Model.update. Thanks for the heads up.

@SirPepe

I see. Is there any way to hook into all remove operations?

@aheckmann
Owner

not presently.

@SirPepe

Ok. Thank you!

@aheckmann aheckmann closed this in 80500d6
@aheckmann
Owner

well you cooould if you monkey patched Model.remove but thats up to you :)

@intech

Not working with findOneAndRemove

@jValdron

Also doesn't seem to work when you call Model.remove(_id).exec();

@ramseydsilva

This is how I got around the issue:

Post.findOneAndRemove({'id': data.post, 'user.uid': socket.handshake.user.id}, function(err, post) {
    post.remove();
});

models.js

postSchema.post('remove', function(doc) {
    console.log('removed');
});

// prints 'removed' once

The remove hook is not fired on findOneAndRemove, however, it works when I call remove in the callback

@Darksheep42

Ok but Why ? iam sorry i didnt get why the hook is executed on Model.remove() and not on Query.remove(). Now why should i use the hooks if they arent called everytime ?

what's the use ?

@Darksheep42

I mean : Should i use the hooks and avoid Query.remove or not use the hooks and always remove manually ?

@vkarpov15
Owner

@Darksheep42 if you want to use hooks, use doc.remove(). The reason why things like MyModel.remove(); doesn't support hooks is that the removed document may not exist in memory, so you would first have to query mongodb for the document and then tell mongodb to remove it for hooks to work properly.

@gregthegeek

Would it be possible to add support for Query.remove() hooks now with the query middleware in 4.0?

@vkarpov15
Owner

Not right now - that's very tricky because there's already middleware for Model.remove()

@gregthegeek

Would there be any issue other than the name conflict? Couldn't you just call the hook 'remove-query' or something instead of 'remove'?

@gregthegeek

Or how about do what you seem to do with update - call the query hook even when the doc version of the function is used. Of course this would break backwards-compatibility, but it's an idea.

@vkarpov15
Owner

Seems like a reasonable idea, but I'm hesitant to break API unless there's a really good reason to. Right now the remove hook operates on a document, so the context in the middleware is the document. For general Query.remove(), the document might not even be in memory. I suppose we could name it something else like 'remove-query, but that's nasty. What do you think about adding an extra options arg topre()andpost()`, something like:

schema.pre('remove', { query: true }, function() {
  // This is only attached as query middleware
});

schema.pre('remove', { doc: true }, function() {
  // This will not run on Query.remove(), only doc.remove()
});
@gregthegeek

I suppose that could work. It's probably better than just using a different name and it doesn't break the API. So I guess if someone did

schema.pre('remove', { query: true, doc: true }, function() {

});

The hook would be called on both query and doc requests. I guess a user could do this if they didn't care about the context. If you do this, though, you'd probably want to also change the update hook to match this new style, and that could potentially break people's code.

@vkarpov15
Owner

Yeah the tricky bit in that context is "what's the default?" In order to preserve API, we'd have to make it so that update is a query middleware by default, but remove is a document middleware by default. That will be very weird. I think we should just open up a separate issue for 5.0 and worry about it later :)

@gregthegeek

How about, just for the next version or some version in the near future, we can just call the hook 'remove-query' or something? It doesn't break the API and AFAIK should be pretty easy to implement. We could really use this feature, as right now you can't capture 100% of remove requests via middleware and we'd much rather do this than enforce certain techniques in our codebase. In 5.0 we can do it the "right way" but I'd love to just see this quick addition before v5.

@vkarpov15
Owner

Alternative approach: how about we just make a preQuery() and postQuery() function?

@gregthegeek

So instead of writing

schema.pre('remove', function() {
    // What is this? A doc hook? A query hook? Idk man
});

you'd write

schema.preQuery('remove', function() {
    // This is definitely a query hook
});
schema.pre('remove', function() {
    // This must be a doc hook
});

So would pre be reserved for docs? Also I guess you could put this into a 4.x version without breaking API, but we'd probably want to remove the old pre style for query middleware in 5.0.

@vkarpov15
Owner

Yeah preQuery() would be a good way to preserve the API until we can figure out a cleaner API. I would imagine preQuery() might be deprecated immediately though, because I'm not quite thrilled with the syntax.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.