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

lack of documentation of middleware callback #14305

Closed
2 tasks done
adelbke opened this issue Jan 29, 2024 · 7 comments
Closed
2 tasks done

lack of documentation of middleware callback #14305

adelbke opened this issue Jan 29, 2024 · 7 comments
Labels
help This issue can likely be resolved in GitHub issues. No bug fixes, features, or docs necessary Stale

Comments

@adelbke
Copy link

adelbke commented Jan 29, 2024

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the issue has not already been raised

Issue

Middleware callback functions can have varying parameters; sometimes, pre-hooks have different structures than post-hooks, and query middleware is different than document middleware Yet the structure of the callback lacks clear documentation. I humbly believe it needs to be more evident for newcomers and documented.

The only way to notice this is by inspecting the typescript files on my project, which is slow and time-consuming.
I am ready to contribute to making this improvement in the documentation.

@IslandRhythms IslandRhythms added the docs This issue is due to a mistake or omission in the mongoosejs.com documentation label Jan 31, 2024
@vkarpov15 vkarpov15 added this to the 8.1.2 milestone Feb 1, 2024
@vkarpov15
Copy link
Collaborator

What specifically about the middleware callback is confusing? Our middleware docs show examples of using next vs using async functions, which seems to cover most of what I can think of with the middleware callback.

@vkarpov15 vkarpov15 removed this from the 8.1.2 milestone Feb 7, 2024
@vkarpov15 vkarpov15 added the needs clarification This issue doesn't have enough information to be actionable. Close after 14 days of inactivity label Feb 7, 2024
@adelbke
Copy link
Author

adelbke commented Feb 14, 2024

I'll explain it to you by giving examples of situations I found myself in when working on a task related to this.

Goal:
I am trying to propagate Mongoose schema events to an event-emitter. A one-to-one mapping would be straightforward. Of each event in a given schema to an event name in the event emitter.

Situations:

  • Post Hook of save events (a document middleware) fire twice on each save, and in the first instance, when I access the "this" keyword, it is not a proper document instance as this.constructor.modelName is undefined.
  • in the post Hook of save events, I expect to pass a function that looks like the following
function (doc, next) { }

This is expected and documented, but there's no indication of what happens when I pass an async function; it seems to be working, but I still have to call next somehow (which is not bad; I would prefer if I could have the choice not to do see and return a promise).

  • In Query Middleware, the "pre" function passes everything regarding the query in the this keyword. But Queries are different; sometimes, there can be an update object, a filter Object, or even an Options Object. They are accessible through functions that may or may not be defined depending on the event. If I try to do what I am doing and react to many different events, I would need to do investigative work to determine which type of query this is.

In this case, I think it would be better if the this keyword were more strictly typed to understand what type of object I am manipulating (is this an update or insertMany ? ). If there is already a way for me to type the Query strictly (I believe there is) that I do not understand, then it means it's either hard to find in the documentation or undocumented.
As to my knowledge, the blog post linked in the docs explains clearly that Query is "ambiguous" in middleware and I think that should be updated

  • I also noticed something weird in insertMany mentioned in this issue insertMany middleware #11531. The pre and post middleware functions have different orders of parameters.

  • In all the other document events, I had to add an if statement checking if this.constructor.modelName is defined because of occasional double event firing like the save case mentioned above.

I hope this was clear; thank you and apologies for the delayed response.

Edit: updated paragraph structure

@vkarpov15 vkarpov15 added needs repro script Maybe a bug, but no repro script. The issue reporter should create a script that demos the issue and removed docs This issue is due to a mistake or omission in the mongoosejs.com documentation needs clarification This issue doesn't have enough information to be actionable. Close after 14 days of inactivity labels Feb 19, 2024
@vkarpov15 vkarpov15 added this to the 8.1.4 milestone Feb 19, 2024
@vkarpov15 vkarpov15 modified the milestones: 8.2.1, 8.2.2 Mar 4, 2024
@vkarpov15 vkarpov15 modified the milestones: 8.2.2, 8.2.3, 8.2.4 Mar 15, 2024
@vkarpov15 vkarpov15 modified the milestones: 8.2.4, 8.2.5 Mar 28, 2024
@vkarpov15 vkarpov15 modified the milestones: 8.3.1, 8.3.2 Apr 5, 2024
@vkarpov15
Copy link
Collaborator

Post Hook of save events (a document middleware) fire twice on each save, and in the first instance, when I access the "this" keyword, it is not a proper document instance as this.constructor.modelName is undefined. This is incorrect. The following script shows that post save hooks only execute once, there's only one "post save" printed to the console:

'use strict';

const mongoose = require('mongoose');

mongoose.connect('mongodb://127.0.0.1:27017/mongoose_test');

const schema = mongoose.Schema({ name: String });
schema.post('save', function() {
  console.log('post save');
});
const TestModel = mongoose.model('Test', schema);
const doc = new TestModel({ name: 'test' });
doc.save().then(() => console.log('done'));

This is expected and documented, but there's no indication of what happens when I pass an async function; it seems to be working, but I still have to call next somehow (which is not bad; I would prefer if I could have the choice not to do see and return a promise).

That's fair, we don't have an example in our docs of using async functions with post hooks, just with pre hooks. But post hooks work the same way, and you don't need to call next if you have an async function. For example, the following script shows a post save hook with an async function that waits for 1 sec before completing

'use strict';

const mongoose = require('mongoose');

mongoose.connect('mongodb://127.0.0.1:27017/mongoose_test');

const schema = mongoose.Schema({ name: String });
schema.post('save', async function() {
  console.log('Post save start');
  const start = Date.now();
  await new Promise(resolve => setTimeout(resolve, 1000));
  console.log('post save end', Date.now() - start);
});
const TestModel = mongoose.model('Test', schema);
const doc = new TestModel({ name: 'test' });
doc.save().then(() => console.log('done'));

In this case, I think it would be better if the this keyword were more strictly typed to understand what type of object I am manipulating (is this an update or insertMany ? ). If there is already a way for me to type the Query strictly (I believe there is) that I do not understand, then it means it's either hard to find in the documentation or undocumented.
As to my knowledge, the blog post linked in the docs explains clearly that Query is "ambiguous" in middleware and I think that should be updated

That's why we have schema.pre('updateOne') and schema.pre('insertMany') instead of just schema.pre(/.*/). insertMany isn't even query middleware. I guess you mean just for TypeScript types, not runtime behavior?

@vkarpov15 vkarpov15 added docs This issue is due to a mistake or omission in the mongoosejs.com documentation and removed needs repro script Maybe a bug, but no repro script. The issue reporter should create a script that demos the issue labels Apr 9, 2024
@adelbke
Copy link
Author

adelbke commented Apr 9, 2024

Hi @vkarpov15, thanks for responding.

Post Hook of save events (a document middleware) fire twice on each save, and in the first instance, when I access the "this" keyword, it is not a proper document instance as this.constructor.modelName is undefined. This is incorrect. The following script shows that post save hooks only execute once, there's only one "post save" printed to the console:

I just ran that script and it adds up, thank you for pointing that out. I'll check the other dependencies I had... I may have made a mistake

That's why we have schema.pre('updateOne') and schema.pre('insertMany') instead of just schema.pre(/.*/). insertMany isn't even a query middleware. I guess you mean just for TypeScript types, not runtime behavior?

I was referring to typescript types, not runtime behavior. Especially the meaning and behavior of generics in mongoose that I have a hard time grasping (I don't know if this is my fault or the docs tbh).
at the time of writing the issue I tried to set up a single hook for all queries and set up separation logic myself. But resorted to defining a hook for each operation. As it made more sense. Therefore this comment is moot

@vkarpov15
Copy link
Collaborator

Yeah I would recommend defining a hook for each operation, or at least a hook for each general class of operations like pre(['find', 'findOne']), pre(['updateOne', 'updateMany']), etc.

However, Query instances are the same for all operations, so there isn't anything wrong with a pre('find') hook that adds details to this.getUpdate() other than the trivial perf overhead of updating an object. find() just won't use the update.

Which generics are you specifically confused about? With automatic schema type inference, you frequently don't need generics at all.

vkarpov15 added a commit that referenced this issue Apr 10, 2024
Add documentation for calling `schema.post()` with async function
@vkarpov15 vkarpov15 removed this from the 8.3.2 milestone Apr 15, 2024
@vkarpov15 vkarpov15 added help This issue can likely be resolved in GitHub issues. No bug fixes, features, or docs necessary and removed docs This issue is due to a mistake or omission in the mongoosejs.com documentation labels Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help This issue can likely be resolved in GitHub issues. No bug fixes, features, or docs necessary Stale
Projects
None yet
Development

No branches or pull requests

3 participants