added overloads for Schema.pre/post with different values for SchemaPreOptions

[jpilgrim]

Summary

You can call the updateOne method with either the model class or a model instance (i.e. document) as receiver:

Depending on how you call it, different pre-hooks are called and this is set differently:

// never called:
userSchema.pre("updateOne", { query: false, document: false }, async function () {
    console.log(`"updateOne",  query: false, document: false, this: ${this.constructor.name}`);
});

// called with this = model
userSchema.pre("updateOne", { query: false, document: true }, async function () {
    console.log(`"updateOne",  query: false, document: true, this: ${this.constructor.name}`);
});

// called with this = Query
userSchema.pre("updateOne", { query: true, document: false }, async function () {
    console.log(`"updateOne",  query: true, document: false, this: ${this.constructor.name}`);
});

// called with this = Query or Model, depending on receiver
userSchema.pre("updateOne", { query: true, document: true }, async function () {
    console.log(`"updateOne",  query: true, document: true, this: ${this.constructor.name}`);
});

In mongoose version 6.7.2 a breaking change has been introduced (compared with 6.7.1. !) with commit 74af5f5b34cc7fcf631e1e323f18df36c74f6201 (by Luca Pizzini) in types/index.d.ts, by moving the overloads for this=Query ahead of the overloads for this=Document (HydratedDocument).

Examples

The following code worked in 6.7.1

userSchema.pre("updateOne", { query: false, document: true }, async function () {
    if (this.isModified("password")) {
        // I know, this will never happen
    }
});

but it produces an error in 6.7.2: Property 'isModified' does not exist on type 'Query<any, any, {}, any>'.ts(2339)

In order to fix that problem, I would suggest to add more overloads, which do not use the general SchemaPreOptions type alias, but concrete values for document and query.

[vkarpov15]

Couple of fixes. Also, please run npm run lint -- --fix

[vkarpov15]

Thanks 👍

[vkarpov15]

Took a closer look and have a couple of comments

[jpilgrim]

Completely refactored my changes.
I added a tests with registers all kind of hooks and checks at runtime the this type. The result of this test is basically how I ended up with the definitions. Added pre and post definitions. Merged latest master.

Checked the following scripts:

• npm run test-tsd
• npm run lint
• npm run test -- here I get 4 failures (1x aggregate.test.js, 3x timestamps.test.js) -- aggregate.test.ts was failing w/o my test as well, and running all three (my tests, aggregate and timestamps) is working. So I assume there is another reason for that failure.

[jpilgrim]

Will have a look at the reported issues

[jpilgrim]

I hopefully fixed the linter issues (I have a different formatter setting...).

I also added a new test, generated from the existing one, which actually tests the type annotations (and found some problems, which I fixed). Now we can sure that the type annotations is in sync with the real thing.

I assume that the tests which failed on Linux failed due to an older Node version? I removed the optional chaining construct.

I'm not quite sure about the ts-benchmark. What is actually measured and what can I do there? How can I check my specific parts? Also I'm not quite sure if the benchmark simply takes longer because there are more lines to parse?

[vkarpov15]

@jpilgrim ts-benchmark measures the output of tsc --extendedDiagnostics on our TypeScript benchmark projects. Right now we don't have very sophisticated TypeScript benchmarking, but we use this to help us avoid severe regressions because TypeScript compiler performance is extremely unpredictable.

[jpilgrim]

@vkarpov15 I have (again) merged the latest master. There were conflicts I have solved (by deleting the changed parts since they were exactly the parts I have newly defined.

Are there any changes required now from my side?

I have no idea what to do about the failing ts benchmark test except increasing the expected time (or whatever is measured). Of course my changes will increase the time, as significantly more overloads are defined now -- which is necessary if you want to have correct definitions.

Github says "1 change requested" -- I think that has been resolved.

[jpilgrim]

@vkarpov15 Any updates?

[vkarpov15]

Sorry for taking so long to review this, we've been busy working on Mongoose 7. Have a couple of comments, but otherwise LGTM

[vkarpov15]

validate is another one that can be both Query and Document middleware

[jpilgrim]

According to https://mongoosejs.com/docs/middleware.html, validate is only a document middleware.
Anyway, how could it be triggered via a query?

[vkarpov15]

We added a Query.validate() method. Mongoose calls this method when runValidators is set on a query, for example:

'use strict';

const mongoose = require('mongoose');

const schema = new mongoose.Schema({ name: String });

schema.pre('validate', () => {
  console.log('Regular pre validate');
});
schema.pre('validate', { query: true, document: false }, () => {
  console.log('Pre query validate');
});

const Test = mongoose.model('Test', schema);

run().catch(err => console.log(err));

async function run() {
  await mongoose.connect('mongodb://localhost:27017/test');

  const doc = new Test();
  await doc.validate();

  console.log('----');
  await Test.updateOne({}, { name: 'bar' }).setOptions({ runValidators: true });
}

[vkarpov15]

| 'validate'

[jpilgrim]

I assume this is the same issue again?

[vkarpov15]

Yep same issue

[jpilgrim]

@vkarpov15 I have merged the (your) latest master. There seems to be some methods have been removed:

• Model.update
• Document.remove

After removing these calls in my test, I got test failures since all pre/post-middlewares for 'remove' and 'update' are not called anymore.

Does that mean that these middleware-strings ('remove' and 'update') are to be removed from types/middleware.d.ts as well, as they cannot be triggered anymore? And from https://mongoosejs.com/docs/middleware.html as well?

[jpilgrim]

@vkarpov15 I assume update and remove have been removed. Thus I updated the tests and type definitions accordingly. As you might note, I had to fix another test.

From my point of view the PR should be ready to be merged. Tests are all passing.

However, I cannot run npm run ts-benchmark: "tarball tarball data for mongoose@file:../../../mongoose.tgz (null) seems to be corrupted."

[vkarpov15]

LGTM, going to merge this into master to release with 7.1. I will make a couple of related changes in master: 1) bumping up the ts-benchmark limit slightly so ts-benchmark passes, 2) undo adding 'validate' to MongooseQueryAndDocumentMiddleware, because of the if (!(this instanceof Query)) change in document.test.ts.