Skip to content

Support middleware skip option for cursors and custom hooks#16350

Open
AbdelrahmanHafez wants to merge 6 commits into
Automattic:masterfrom
AbdelrahmanHafez:feat/skip-middleware-cursors-statics
Open

Support middleware skip option for cursors and custom hooks#16350
AbdelrahmanHafez wants to merge 6 commits into
Automattic:masterfrom
AbdelrahmanHafez:feat/skip-middleware-cursors-statics

Conversation

@AbdelrahmanHafez

Copy link
Copy Markdown
Collaborator

re #8768, builds on top of mongoosejs/kareem#46

We left some parts uncovered in #15883, namely statics, methods, and cursors (aggregation, and query).

This PR:

  • Applies the existing middleware skip option to query cursors, including both pre find hooks and per-document post find hooks.
  • Applies the skip option to aggregate cursors for pre aggregate hooks. Aggregate cursors still don't run post aggregate hooks, so the cursor-specific TS type only exposes middleware: false and { pre: false }.
  • Strips the Mongoose-only middleware option before passing cursor options to the MongoDB driver.
  • Adds support for skipping middleware on custom statics and methods by reading middleware from the final plain options object.
  • Documents the cursor behavior and the recommended custom static/method signature.

For now this pins kareem to the fork commit that adds wrapper getOptions support for custom statics/methods. Once the Kareem change lands and is released, we can switch this back to a normal version range.

A design decision that was made is how we pass the middleware option to statics/methods, I chose to pass them to the last arg if it's POJO, and the docs now have examples with a pattern where users define the last arg as options, following the same pattern as the other mongoose operations.

@AbdelrahmanHafez
AbdelrahmanHafez requested review from hasezoey and vkarpov15 and removed request for vkarpov15 June 29, 2026 23:46
@AbdelrahmanHafez
AbdelrahmanHafez marked this pull request as ready for review June 29, 2026 23:46

@hasezoey hasezoey left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
I dont really like how the options for custom methods is implicit, though aside from forcing users to somehow handle it (by transferring pre/post running to be manual calls), i dont see another way.

Comment thread test/model.skip.middleware.test.js Outdated
Co-authored-by: hasezoey <hasezoey@gmail.com>
if (connection) {
this.cursor = connection.db.aggregate(agg._pipeline, agg.options || {});
const options = agg.options != null ? { ...agg.options } : {};
delete options.middleware;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend only deleting options.middleware if it is present, delete is relatively slow.

Comment thread docs/middleware.md
Mongoose reads `middleware` from that trailing options object:

```javascript
schema.statics.queueEmail = async function(to, emailOptions, options = {}) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some concerns that function queueEmail(params) will skip middleware if params.middleware is true, but middleware may be a parameter set for some other lib. Could potentially be backwards breaking if someone relies on middleware for statics while also passing middleware as a parameter.

Potential alternatives:

  1. Don't allow skipping middleware on methods and statics
  2. Require a more explicit opt-in by only checking the last parameter if it is named mongooseOptions
  3. Add new mongoose.MiddlewareOptions class and explicitly check for instances of mongoose.MiddlewareOptions

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another alternative, though i dont know how common it is, would be to assign some extra property to those functions, similar to:

schema.statics.someFunction = function(...) {}
schema.statics.someFunction.useMiddleware = true;

This could serve to enable / disable middleware options and could be enabled by default (or at least in a later version of mongoose)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hasezoey I like that, with one tweak: if the flag toggled middleware itself it wouldn't solve the per-operation case, but as a gate for whether Mongoose reads the trailing options object it works well. I'd rename it to something like supportsMiddlewareOption though, useMiddleware sounds like "this static uses middleware" rather than "this static accepts the middleware option".

I think that gives us the best migration path: ship the flag as an explicit opt-in now, which is strictly non-breaking, then in v10 remove the flag entirely and always read the option. The flag becomes a harmless property assignment for anyone who set it, adopters' call sites never change, and they get the same { middleware: false } shape as the built-in operations from day one. The cost is that adopters do a two-step setup until v10, which seems like a fair price for the transition.

The v10 break is also narrower than it looks. A user only breaks if all three are true:

  1. They use statics.
  2. They have middleware registered for that specific static (schema.pre('queueEmail')).
  3. They pass a trailing options object with middleware: false in it for unrelated reasons. Something like middleware: ['e-mail', 'sms'] is truthy, so hooks run exactly as before.

The check is currently falsy-based, I'll change it in this PR to only honor exactly false or the { pre, post } object shape, which narrows that even further.

So my order of preference: supportsMiddlewareOption opt-in now and always-on in v10, then plain middleware directly accepting the narrow risk, then mongooseOptions.middleware. mongoose.MiddlewareOptions feels too verbose, and not supporting statics/methods at all is inconsistent with the rest of the feature.

WDYT?

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