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

hook is not of type Aggregate #14903

Closed
2 tasks done
dragontaek-lee opened this issue Sep 22, 2024 · 1 comment · Fixed by #14904
Closed
2 tasks done

hook is not of type Aggregate #14903

dragontaek-lee opened this issue Sep 22, 2024 · 1 comment · Fixed by #14904

Comments

@dragontaek-lee
Copy link
Contributor

dragontaek-lee commented Sep 22, 2024

Prerequisites

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

Mongoose version

8.6.1

Node.js version

20.10.0

MongoDB server version

7.0.9

Typescript version (if applicable)

No response

Description

this context in hooks are not of type Aggregate.

This issue arises when custom static methods overwrite built-in aggregate functions. (as seen in the implementation in mongoose-delete)

This issue is similar to the past one (#7790), where the context in hooks was not of type Query. I noticed that it has been fixed in this PR (f848b54), which filters custom static methods that overwrite existing query middleware in applyStaticHooks.js.

Steps to Reproduce

I created a minimum reproduction repository. (https://github.com/dragontaek-lee/mongoose-staticHooks-overwrite-support-more-cases)

I created some test cases to reproduce this issue in the reproduction repository.
The code can be found in index.js, test/aggregate.middleware.test.js, or test/query.middleware.test.js.
Tests and scripts can also be run using the npm commands mentioned in the README.

The summarized reproduction code is as follows:

schema1.statics.aggregate = function(pipeline) {
    let match = { $match: { deleted: { '$ne': false } } };

    if (pipeline.length && pipeline[0].$match) {
        pipeline[0].$match.deleted = { '$ne': false };
    } else {
        pipeline.unshift(match);
    }

    const query = Model.aggregate.apply(this, [pipeline]);
    return query;
};

Assume that the static method overwrites the built-in Mongoose aggregate function.

schema1.pre('aggregate', function(next) {
    console.log(this);
    next();
});

schema1.post('aggregate', function() {
    console.log(this);
});

In this situation, also assume that there are pre and post hooks for aggregate.

Test 1

  • both the custom static method and the built-in Mongoose aggregate hooks are executed and both the pre and post hooks are called twice

Test 2

  • this context differs between the custom static method and the built-in aggregate function.
  • in the first pre-hook and the second post-hook call, this context is not an instance of Aggregate, but in the second pre-hook and the first post-hook call, it becomes an instance of Aggregate.

Expected Behavior

Since the issues with query middleware were fixed in this PR (f848b54), it is expected that if a custom static method overwrites an existing aggregate middleware, the middleware should not be applied by default, just as it works with query middleware.

I’m going to submit the PR right away for this.

@dragontaek-lee
Copy link
Contributor Author

Fix PR: #14904

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