-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Should discriminators copy hooks from original schema? #2945
Comments
+1 for NO (hooks defined on original/base schema execute twice which prevents code factoring). |
Discriminators should execute the hooks from the original schema, but they should not be executed twice. |
@antolovich does it get run twice in your case as well? Same situation as #2892 (comment) ? |
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. |
@vkarpov15, I second @antolovich's comment |
I think it's just a misunderstanding - the question is whether mongoose's 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. |
I have an error plugin which I registered globally |
@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 |
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 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'} |
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 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 |
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 |
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.
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:
I'm not sure how my external library would handle 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. |
Why do you need discriminators if you have this sophisticated kind of inheritance system? Discriminators would not support this structure anyway. |
There are two main advantages I can think of off the top of my head for using Mongoose's discriminators:
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. |
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 |
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 ( |
We'll have a fix in 5.0 branch shortly that deduplicates based on functions 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);
}); |
See #2892 (comment)
The text was updated successfully, but these errors were encountered: