Should discriminators copy hooks from original schema?

[vkarpov15]

See #2892 (comment)

[jlchereau]

+1 for NO (hooks defined on original/base schema execute twice which prevents code factoring).

[antolovich]

Discriminators should execute the hooks from the original schema, but they should not be executed twice.

[vkarpov15]

@antolovich does it get run twice in your case as well? Same situation as #2892 (comment) ?

[antolovich]

Yes, the hooks run twice in exactly the way you described in #2892 (comment). The hooks only execute once if I attach them to schemas created from the base schema constructor, .

In the aforementioned comment, you said this was not a bug, but instead was a design decision. I understand the reasoning for not declaring it a bug, but I can see no possible situation in which one would want the exact same hooks to run multiple times, especially when discriminators are used as a form of inheritance.

[jlchereau]

@vkarpov15, I second @antolovich's comment

[vkarpov15]

I think it's just a misunderstanding - the question is whether mongoose's discriminator() function should copy hooks for you, or whether you should be responsible for doing so yourself. There is no reason why you would want the same hooks to run twice, but the reason why the hooks run twice is because discriminator() copies the base schema hooks and then your BaseSchema constructor adds the same hook to the child schema. The workaround here is to have your child schema not inherit from the same base class as the parent schema and rely on discriminators to handle the inheritance:

var parentSchema = new ActivityBaseSchema();

var model = mongoose.model('Activity', parentSchema);

var commentSchema = new Schema({
    text    : { type: String, required: true }
});

var D = model.discriminator('Comment', commentSchema)

instead of

var parentSchema = new ActivityBaseSchema();

var model = mongoose.model('Activity', parentSchema);

var commentSchema = new ActivityBaseSchema({ // <-- duplicates hooks because `discriminator()` copies them over
    text    : { type: String, required: true }
});

var D = model.discriminator('Comment', commentSchema);

I'm open to arguments to the contrary but right now I don't see the benefit of supporting this behavior.

[evilive3000]

I have an error plugin which I registered globally mongoose.plugin(mongooseErrors).
On discriminator models errorHandler fires two times. Is this refers to this issue and is it designed behavior?

[vkarpov15]

@evilive3000 can you provide a code sample? I'd need to see whether you're using the same inheritance pattern as the one I described in my Feb 6, 2017 comment

[evilive3000]

const mongoose = require('mongoose');

mongoose.Promise = global.Promise;
mongoose.set('debug', true);

mongoose.plugin((schema) => {
  schema.post('save', (err, doc, next) => {
    console.log({ plugin: err.message });
    return next(new Error('custom error'));
  });
}, { deduplicate: true });

(async () => {
  await mongoose.connect('', { useMongoClient: true });

  const Mother = mongoose.model('Mother', new mongoose.Schema({_id: String}));
  const Daughter = Mother.discriminator('Daughter', new mongoose.Schema({_id: String}));

  // clear collection
  await Daughter.remove();

  // first insert ok
  await Daughter.create({ _id: 'id-1' });

  // second will trigger Duplicate Key Error
  await Daughter.create({ _id: 'id-1' });
})()
  .catch(e => console.log({ catch: e.message }))
  .then(() => { mongoose.disconnect() });

Output:

Mongoose: mothers.remove({ __t: 'Daughter' }, {})
Mongoose: mothers.insert({ _id: 'id-1', __t: 'Daughter', __v: 0 })
Mongoose: mothers.insert({ _id: 'id-1', __t: 'Daughter', __v: 0 })
{plugin: 'E11000 duplicate key error collection: mothers index: _id_ dup key: {:"id-1"}'}
{plugin: 'custom error'}
{catch: 'custom error'}

If I change Daughter models to Mother the error handler will trigger once:

await Mother.remove();
await Mother.create({ _id: 'id-1' });
await Mother.create({ _id: 'id-1' });

Output:

Mongoose: mothers.remove({}, {})
Mongoose: mothers.insert({ _id: 'id-1', __v: 0 })
Mongoose: mothers.insert({ _id: 'id-1', __v: 0 })
{plugin: 'E11000 duplicate key error collection: mothers index: _id_ dup key: {:"id-1"}'}
{catch: 'custom error'}

[c1moore]

I think hooks should be inherited. Let's look at this from an OO POV. Hooks work as instance methods since they deal with instances (documents) as opposed to static methods that interact with the model (class). Thinking of hooks in this light suggests they should be inherited, but let's look at this from a different point of view.

Let's say we define ParentSchema which defines a private field called foo. Before an instance of ParentSchema is saved, foo needs to be validated and transformed somehow. So ParentSchema defines a pre-save hook. Now ChildSchema inherits from ParentSchema, but it doesn't know about foo (foo is a private, implementation-specific detail of ParentSchema!). Technically speaking, there is nothing in Mongoose (or even JS, for that matter) that would stop us from just adding the pre-save hook for foo in ChildSchema, but this is bad practice (what if we get rid of foo? what if foo changes? what if how we need to transform foo changes? etc.).

However, I have created a plugin (private at the moment, but I hope to make it public eventually) that enhances schema inheritance among other things and I definitely see the issue discussed above and in #2892 being a problem there too. I think a better approach would be to leave it up to the Schema. For example, we could define a schema option for the child schema called inheritHooks that takes a Boolean value (true => inherit hooks; false => don't inherit hooks). The default value, for reasons stated above, would be true. Then plugins like mine could automatically add the inheritHooks option and set it to false.

[vkarpov15]

I think you're seeing the same problem I am: it seems like discriminator() should copy schema hooks, but that makes things confusing if you try to use schema inheritance with discriminator(). However, I think the answer here is to just say that if you're going to use schemaB as a discriminator() for schemaA, then schemaB should not inherit from schemaA because discriminator() has its own inheritance mechanism. Does that make sense to you @c1moore ?

[c1moore]

Yes, I think we see eye to eye on the problem. I think I understand what you're suggesting. You're saying that the external library shouldn't inherit the hooks and instead leave that up to Mongoose. There are 3 problems I see with that approach.

1. My library doesn't work with Schemas, it works with JS classes (or class functions). The way the class is defined and certain special fields on the class allow a Schema to be rendered for the class when necessary. This gives me a lot of flexibility in my system and allows me to do neat things with inheritance. This isn't a problem in itself, but lends to the next two problems.
2. Avoiding inheritance isn't an option for this library. It supports multi-level inheritance, which Mongoose is not yet capable of doing. It would get really messy trying to determine what should inherit what and when it should inherit it, making the library a lot more heavyweight than I would like it to be.
3. This library doesn't really know how the parent/child schemas are going to be used. The parent schema could be a simple class that is not meant to be saved to the DB, or it could be a true schema (e.g. Automobile in the example below is not really a Model, just a class).

I could see adding an option on my library to determine when to inherit what, but I think adding the option to Mongoose would be cleaner, especially if other people are running into the same issues (which it sounds like they are). That way, the inheritance mechanism wouldn't have to change halfway through the inheritance chain.

As I mentioned before, my library supports multi-level inheritance. So I'm not certain how it would work on my library when the inheritance skips one or more levels. For example, given the following inheritance chain:

        Vehicle
        /     \
  Automobile  Boat
 /    |     \
Car  Truck  Van

I'm not sure how my external library would handle Car being a discriminator of Vehicle and still inherit Automobile if my library was responsible for determining when/where to use inheritance.

Of course, supporting an external library really isn't Mongoose's concern, but making it easier for external libraries like this will help keep Mongoose's core slimmer.

[vkarpov15]

Why do you need discriminators if you have this sophisticated kind of inheritance system? Discriminators would not support this structure anyway.

[c1moore]

There are two main advantages I can think of off the top of my head for using Mongoose's discriminators:

1. Most importantly, discriminators allow me to store several discriminated things in the same collection and query them under the same Model. In the example above, I can store Cars, Trucks, Vans, and Boats all in the same collection if I set Vehicle as the discriminated Model and each of them as discriminators. Later, I can then execute something like Vehicle.findByVin(), which will return the Vehicle with the specified VIN, regardless of whether that Vehicle is a Boat or Car. Remember, my library just renders the appropriate schema, it does not interact directly with MongoDB, replace Mongoose, or otherwise create Models. It simply renders a Schema that can later be used to create a Model, if necessary; used as a sub-schema; treated as a normal class (i.e. it never touches the database); or a combination of the above.
2. Sometimes I need a field where I can store a Vehicle or a list of Vehicles as an embedded array The best/easiest way to support this behavior without taking over Mongoose is by defining a field of type Vehicle then adding the Car, Truck, Van, and Boat as discriminators for that field.

I had a third, but forgot it. I'll add it if I remember.

The main problem I see with inheriting hooks, unlike everything else, is there is no way to determine which hooks perform the same duty (are overriding another hook) and which extend the parent schema. Literally everything else has this mechanism in play (instance methods, schema fields, virtuals, schema options). Thinking about it, I don't think this problem is unique just to libraries like mine. It probably occurs in situations where somebody wants to extend a hook created in a super-schema but can't because of this limitation. Maybe inheriting hooks themselves isn't the problem, but rather the inability to distinguish which ones perform the same job or extend previous behavior. My library could easily add that sort of behavior, but it would again require Mongoose to allow us to turn off hook inheritance.

However, I don't think a nuclear option is the right approach. For libraries like mine and when you want to extend a parent-schema's hooks, you want to turn off hook inheritance. For basic usage, you would want to keep hook inheritance turned on.

[vkarpov15]

With schema fields, methods, and virtuals this not really an issue because there its clear that the child schema overrides the base schema. Hooks are tricky because the overwrite doesn't really apply, child schema hooks get concatted onto the base schema's hooks. Another possibility is deduplicating like we optionally do with plugins - if a child schema hook function is === a base schema's hook function, we don't re-apply that hook twice. Perhaps that could solve the problem?

[c1moore]

Agreed, it's fairly easy for fields, methods, and virtuals. Re-reading my post it wasn't clear, but that was my point. Adding a mechanism like naming the hooks could allow for overriding and extending inherited hooks. This could be achieved either internally or externally with a library similar to mine as long as Mongoose supports someway for such libraries to be clever enough to somehow remove inherited hooks.

Using strict equality (===) could definitely solve at least part of the problem and arguably the most important aspect of the problem, so sounds like a good idea to me. If I get clever and use enough indirection, it might even solve the problem with overriding inherited hooks using an external library.

[vkarpov15]

We'll have a fix in 5.0 branch shortly that deduplicates based on functions === each other. The below script will only print 'validate2' once against the 5.0 branch:

var mongoose = require('mongoose');
var util = require('util');

mongoose.set('debug', true);
mongoose.connect('mongodb://localhost:27017/gh2892');

function middleware(next) {
    console.log('validate2');
    next();
}

function ActivityBaseSchema() {
    mongoose.Schema.apply(this, arguments);
    this.options.discriminatorKey = 'type';
    this.add({
      name: String
    });
    this.pre('validate', middleware);
}
util.inherits(ActivityBaseSchema, mongoose.Schema);

var parentSchema = new ActivityBaseSchema();

var model = mongoose.model('Activity', parentSchema);

var commentSchema = new ActivityBaseSchema({
    text    : { type: String, required: true }
});

var D = model.discriminator('Comment', commentSchema);

var d = new D({ text: 'abc' });
d.save(function(err) {
  console.log(err);
});