Plugin hooks are cloned on Model.discriminator

[hasezoey]

Prerequisites

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

Mongoose version

6.6.1

Node.js version

any

MongoDB server version

any

Description

Currently when having a plugin which adds hooks and using discriminator the plugin only gets run twice where as the hooks do not get merged correctly unless they are global functions

Steps to Reproduce

// NodeJS: 18.8.0
// MongoDB: 5.0 (Docker)
// Typescript 4.8.3
import * as mongoose from 'mongoose'; // mongoose@6.6.1

function globalPlugin(schema: any) {
  console.log('PLUGIN');
  schema.pre('save', function nonGlobalHook() {});
}

const baseSchema = new mongoose.Schema({});
baseSchema.plugin(globalPlugin); // plugin gets once run here

const BaseModel = mongoose.model('Base', baseSchema);

const disSchema = new mongoose.Schema({}); // schema with no inheritance from "baseSchema"
disSchema.plugin(globalPlugin); // and plugin gets once run here too

const DisModel = BaseModel.discriminator('Dis', disSchema);

console.log('TEST1', (BaseModel.schema as any).s.hooks._pres.get('save')); // only has 1 "nonGlobalHook"
console.log('TEST2', (DisModel.schema as any).s.hooks._pres.get('save')); // has 2 "nonGlobalHook"

when moving nonGlobalHook to the more global scope (outside of globalPlugin), it works correctly and only gets applied once (deduplicated?)

Reproduction Repository / Branch: https://github.com/typegoose/typegoose-testing/tree/mongooseGh12472

Expected Behavior

The Plugin to only run twice (current behavior) and the hooks to either get merged correctly (only one occurrence) or not merged at all when not extend from original

---

i know this seems like it is intended behavior, but it seems that should be behavior that could be controlled by the user
(i noticed this because typegoose currently uses this approach of constructing a new schema for each class and not using a schema that is already in a model)

related #2945

PS: after reading Apply Plugins Before Compiling Models i would expect to apply all plugins before calling .discriminator regardless of if the schema is cloned or not, though i would also (somehow) expect all required thing to be cloned form the base schema (like discriminator options or the type key)

[hasezoey]

i just noticed that all plugins get overwritten instead of merged, where as the hooks get merged

plugins:

mongoose/lib/helpers/model/discriminator.js

Line 184 in 55112dc

schema.plugins = Array.prototype.slice.call(baseSchema.plugins);

(with no other merge later)

hooks:

mongoose/lib/helpers/model/discriminator.js

Line 182 in 55112dc

schema.s.hooks = model.schema.s.hooks.merge(schema.s.hooks);

example the following second plugin gets overwritten:

// NodeJS: 18.8.0
// MongoDB: 5.0 (Docker)
// Typescript 4.8.3
import * as mongoose from 'mongoose'; // mongoose@6.6.1

function globalPlugin(schema: any) {
  console.log('PLUGIN');
  schema.pre('save', function nonGlobalHook() {});
}

const baseSchema = new mongoose.Schema({});
baseSchema.plugin(globalPlugin);

const BaseModel = mongoose.model('Base', baseSchema);

const disSchema = new mongoose.Schema({});
// @ts-ignore
disSchema.plugin(globalPlugin, { test: 1 });

const DisModel = BaseModel.discriminator(
  'Dis',
  disSchema
);

console.log('TEST', (DisModel.schema as any).plugins); // notice that "opts" is "undefined", like the baseschema plugin and there is no second entry for the plugin

[vkarpov15]

@hasezoey Mongoose does correctly dedupe if you use mongoose.plugin() rather than using schema.plugin() on every schema. For example:

const mongoose = require('mongoose');

function globalPlugin(schema) {
  console.log('PLUGIN');
  schema.pre('save', function nonGlobalHook() {});
}

mongoose.plugin(globalPlugin);

const baseSchema = new mongoose.Schema({});

const BaseModel = mongoose.model('Base', baseSchema);

const disSchema = new mongoose.Schema({}); // schema with no inheritance from "baseSchema"

const DisModel = BaseModel.discriminator('Dis', disSchema);

console.log('TEST1', (BaseModel.schema).s.hooks._pres.get('save')); // only has 1 "nonGlobalHook"
console.log('TEST2', (DisModel.schema).s.hooks._pres.get('save')); // only has 1 "nonGlobalHook"

I think the assumption is that if you're calling schema.plugin(), you mean to apply the plugin to that schema. Whereas mongoose.plugin() is smart enough to avoid discriminator schemas by default. Is there some reason why you can't use mongoose.plugin()?

[hasezoey]

Mongoose does correctly dedupe if you use mongoose.plugin() rather than using schema.plugin() on every schema.

my understanding is that mongoose.plugin is applied to all schemas across a mongoose instance, which is not what i meant with the original code, the original issue code is just to show the minimal required for this example.

if you want a more complex example: i had discovered this issue originally because typegoose creates a new schema for all classes, by walking up the class hierarchy (where the first in that hierarchy creates the schema, and all in between just add the paths) and then on the final schema applying "finishing touches" (like virtual, plugins, indexes, hooks, etc), but this approach did not work correctly because Model.discriminator always merges the base's schema into the discriminated schema, which in this case did not duplicate the plugin (or call it again for the new schema), but the hooks were duplicated (this was encountered with typegoose + typegoose-auto-increment).

i know this is basically a matter of how typegoose does things, but it works for all other cases that are not discriminators (that i know of), but it also does not seem necessary to always merge the schemas when also already having the base already applied (by either baseSchema.clone and adding to that, or a new schema that also incorporates the schema definition).
i see mongoose "fixing" this by either better hooks de-duplicated (which i think is hard) or giving a option to not have properties merged unless absolutely required