Support middleware skip option for cursors and custom hooks#16350
Support middleware skip option for cursors and custom hooks#16350AbdelrahmanHafez wants to merge 6 commits into
middleware skip option for cursors and custom hooks#16350Conversation
hasezoey
left a comment
There was a problem hiding this comment.
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.
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; |
There was a problem hiding this comment.
I recommend only deleting options.middleware if it is present, delete is relatively slow.
| Mongoose reads `middleware` from that trailing options object: | ||
|
|
||
| ```javascript | ||
| schema.statics.queueEmail = async function(to, emailOptions, options = {}) { |
There was a problem hiding this comment.
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:
- Don't allow skipping middleware on methods and statics
- Require a more explicit opt-in by only checking the last parameter if it is named
mongooseOptions - Add new
mongoose.MiddlewareOptionsclass and explicitly check for instances ofmongoose.MiddlewareOptions
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
@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:
- They use statics.
- They have middleware registered for that specific static (
schema.pre('queueEmail')). - They pass a trailing options object with
middleware: falsein it for unrelated reasons. Something likemiddleware: ['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?
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:
middlewareskip option to query cursors, including both prefindhooks and per-document postfindhooks.aggregatehooks. Aggregate cursors still don't run postaggregatehooks, so the cursor-specific TS type only exposesmiddleware: falseand{ pre: false }.middlewareoption before passing cursor options to the MongoDB driver.middlewarefrom the final plain options object.For now this pins
kareemto the fork commit that adds wrappergetOptionssupport 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
middlewareoption 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.