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

feat: add skipMiddlewareFunction() and overwriteMiddlewareResult() for skipping and modifying middleware results #11927

Merged
merged 4 commits into from
Jun 17, 2022

Conversation

vkarpov15
Copy link
Collaborator

Fix #11426
Fix #8393

Summary

Right now there's no way to overwrite the result of a function in a post() hook. For example, there's no way to use filter() to modify a find() result in a post('find') hook.

There's also no way to "cancel" an operation in a pre hook without throwing an error. Which is inconvenient, because there are some use cases where it would be great to skip executing the underlying function (local caching, memorizing results, etc.)

This PR adds overwriteMiddlewareResult() for the former, and skipMiddlewareFunction() for the latter, from mongoosejs/kareem#30 .

@AbdelrahmanHafez @Uzlopak what do you think about the naming and overall idea?

Examples

See tests

…` for skipping and modifying middleware results

Fix #11426
Fix #8393
@vkarpov15 vkarpov15 self-assigned this Jun 13, 2022
@Uzlopak
Copy link
Collaborator

Uzlopak commented Jun 14, 2022

I think skipMiddleware is better than skipMiddlewareFunction
But it raises the question: are you skipping the middleware or are you skipping the hook? skipMiddleware would tell me that all the potential following hooked functions are also skipped. skipHook would indicate that only this function gets skipped.

So it would be imho good to show in a unit test what happens with following hooks. Are they also skipped or not.

@Uzlopak
Copy link
Collaborator

Uzlopak commented Jun 14, 2022

When reading this again, it makes more sense to me, that the all following hooks will get skipped. Right?
Still a unit test were multiple hooks are set should be added.

@vkarpov15
Copy link
Collaborator Author

@Uzlopak following hooks will not be skipped. The purpose of these two functions is to run the entire middleware stack with a modified result. skipMiddlewareFunction() is for use cases where you want to skip the wrapped function but execute the rest of pre and post middleware with a precomputed result (think caching). overwriteMiddlewareResult() is for cases where you want to execute the rest of post middleware with a different result - think using filter() / map() for modifying results and making findOne() return null if the returned result is somehow invalid in a way that you can only compute in JS.

vkarpov15 added a commit to mongoosejs/kareem that referenced this pull request Jun 16, 2022
@vkarpov15 vkarpov15 merged commit 96edf20 into 6.4 Jun 17, 2022
@vkarpov15 vkarpov15 deleted the vkarpov15/gh-11426 branch June 17, 2022 02:29
@junmiu
Copy link

junmiu commented May 16, 2023

@vkarpov15 skipMiddlewareFunction only works for Query not QueryCursor, here is an example

// setup a pre hook for find
schema.pre('find', function (next) {
  next(skipMiddlewareFunction([]));
});
// err is { "args": [[]] }, an instance of skipWrappedFunction
mongooseModel.find().cursor().on('error', function (err) { console.log(err); });

The reason is in lib/cursor/QueryCursor.js, it called like this

// here may want to call query._find()
// or wrap this with kareem.createWrapper()
model.hooks.execPre('find', query, (err) => {
  if (err != null) {
    _this._markError(err);
    _this.listeners('error').length > 0 && _this.emit('error', err);
    return;
  }
  // do other things
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants