Mongoose: pre() and post() options to switch between query middleware and document middleware

[vkarpov15]

Some hooks, like 'update' and 'remove', can be triggered by Document.update() as well as Query.update(). Re: https://github.com/Automattic/mongoose/issues/1241#issuecomment-108936495, it would be good to be able to do the following:

schema.pre('remove', { query: true }, function() {
  // This is only attached as query middleware, context will be query in all cases,
  // but will be triggered by both doc.remove() and query.remove()
});

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

Question is, what should be the default? Right now 'update' is only a query middleware, but 'remove' is only a document middleware. Either way will break someone's code.

[gregcarlin]

How about for default, call the hook on both queries and documents? The context (this) would be different for each, but I think it would be a good default. A user may not need the document or query, and if they do they can of course limit the hook to either doc or query, or they can use instanceof on this. With this, a opt-out style may make more sense.

schema.pre('remove', { doc: false }, function() {
  // This is only attached as query middleware, context will be query in all cases,
  // but will be triggered by both doc.remove() and query.remove()
});

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

This would be the same as writing

schema.pre('remove', { doc: false, query: true }, function() {
  // This is only attached as query middleware, context will be query in all cases,
  // but will be triggered by both doc.remove() and query.remove()
});

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

[vkarpov15]

If we do both by default though, that means that schema.pre('remove') would be called twice by default, once as doc middleware and once as query middleware. Not a sensible default IMO. One or the other is probably a better choice.

[gregcarlin]

Are you saying that it would be called twice for a given command? Like running

Model.removeById(id).exec();

Would cause two calls to schema.pre('remove')? Is this caused by something with the internal implementation of the function?

[vkarpov15]

Not for Model.removeById(), but doc.remove() is implemented as a wrapper around Query.remove(), so if you specified both doc and query, the middleware would get fired twice on doc.remove().

[MForever78]

Yeah this is really a feature in need. I debugged for a long time only to figure out that remove from model doesn't trigger the pre hook...

[Jeff-Lewis]

Just taking a fresh look at this, @vkarpov15 from an API perspective, it's important that all Mongoose hooks(events) fire at both the document and schema level when possible. It should depend on how the app chooses to consume the events, either at the doc level or schema level when hook(event) gets fired. I think separating the hooks(events) in the API based on doc or query is tied too much to the implementation aspects of MongoDB and should be abstracted slightly.

As for the hook(event) information passed to the callback(handler), we should try to make it consistent as possible given the limitations with needing to be backward compatible.

One idea is to leave the 'this' alone for backward compatibility and include a new "hook context" object as arguments passed to callback which contains multiple properties of useful information. This also allow better extensibility of the "hook context" object without breaking callback interfaces.

We could leverage a lot of the ideas from StrongLoop Loopbacks Operation Hooks.

var schema = new Schema(..);
schema.pre('save', function(next, context) {
  // Now we can get all the information we need.
  context.Model;
  context.query;
  context.options;
  context.instance;
  // do stuff
  next();
});

Ideally, the context would go first but this would be backward breaking. :)
schema.pre('save', function(context, next) {

[vkarpov15]

That's an interesting idea. My beef with that approach is that 'context' objects tend to be huge, bloated, and difficult to grasp (like loopback's operation hooks :) ), and either way the 'context' object is going to have to be different for query middleware vs document middleware. This also wouldn't really help with the remove() case as well, because it's going to get quite confusing when doc.remove() fires the generic 'remove' middleware twice...

[muratsu]

Is there a decision on this one? I may have some free time & help.

[vkarpov15]

@muratsu yeah, I think the general idea is something like schema.pre('remove', { kind: 'query' }, function(next)); If you want to help go for it, would be much appreciated :)

[c1moore]

In #1241 you mentioned using something like preQuery() instead of the 3 argument approach discussed here. IMO using the preQuery() approach is semantically more meaningful and, therefore, I feel a better approach. To remain backwards compatibility, we could continue to support pre('query', function(next) {}) approach for existing query middleware, but warn the user that they should start using preQuery('query', function(){});.

Under the hood, pre() would just call preQuery() if the hooked method is a _currently supported_ query. If the user wants to take advantage of new query hooks (e.g. remove), they would have to use preQuery(). That avoids having to break existing code and makes it easier to get this issue done.

[vkarpov15]

Right now i prefer pre(, { query: true }) instead of preQuery, but not sold either way. I like being backwards compatible and favoring more consolidation between query and document middleware

[mtnptrsn]

Will this feature be implemented?

[vkarpov15]

Yes but no concrete timeline currently.

[nove1398]

I am a bit late to the party but, I think having query remove hooks would be a great default, I am recently coming across the need for this sort of hook. Any updates?

[vkarpov15]

Not yet, we'll implement it for 5.3.

[nove1398]

Understood, looking forward to it. :)

[andfk]

Hi! Something new into this? :)

[lineus]

@andfk it's slated for the 5.3 milestone which is currently set for mid September shown here

[jmarchello]

Any help needed with this? I'd love to contribute.

[vkarpov15]

@jmarchello please do! Make a PR against the 5.3 branch