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

added support for discriminator mapping (closes #1003) #1647

Merged
merged 11 commits into from Sep 9, 2013
Merged

added support for discriminator mapping (closes #1003) #1647

merged 11 commits into from Sep 9, 2013

Conversation

j
Copy link
Contributor

@j j commented Aug 15, 2013

So I gave discriminator mapping a shot. After coding this out, it feels pretty natural and seems to fit well with the current API of mongoose.

Anyway, a quick example of how it works:

function BaseSchema() {
    Schema.apply(this, arguments);

    this.add({
        name: String,
        createdAt: { type: Date, default: Date.now }
    });
}
util.inherits(BaseSchema, Schema);

var EventSchema = new BaseSchema();

var ImpressionEvent = new BaseSchema();

var ConversionEvent = new BaseSchema({
    revenue: Number
});

var Base = mongoose.model('Event', EventSchema),
var Impression = Base.discriminator('Impression', ImpressionEvent),
var Conversion = Base.discriminator('Conversion', ConversionEvent);

Then assuming we have inserted a single document for each event in the order of Base, Impression, and Conversion:

Base.find({}, function(err, events) {
    expect.ok(events[0] instanceof Base);       // true
    expect.ok(events[1] instanceof Impression); // true
    expect.ok(events[2] instanceof Conversion); // true
    expect.ok(events[2] instanceof Base);       // false

    // however
    expect.ok(events[2].schema instanceof BaseSchema); // true
});

Lastly, if you take a the specific discriminator type model, it will query for only the documents of that type:

// find all conversion events
Conversion.find({}, function(err, events) {
    expect.equal(events.length, 1);
});

// or a single document
Conversion.findOne({}, function(err, event) {
    expect.ok(event instanceof Conversion); // true
});

// or querying for a specific id which is an invalid type (produces query { _id: ..., __t: 'Conversion' })
Conversion.findOne({ _id: impression.id }}, function(err, event) {
    expect.equal(event, null); // true
});

Just one thing before moving too far forward: I introduced a new method "discriminator" to be clear of what you're doing. It creates a model in the same connection and returns it and does some other stuff behind the scenes. We could also make the static method Model.model do the same (or alias / remove discriminator method) if the community would rather have the following instead:

var Base = mongoose.model('Event', EventSchema);
var Impression = Base.model('Impression', ImpressionEvent),
var Conversion = Base.model('Conversion', ConversionEvent);

Another note, in my example, I'm using util.inherit to simply show that you can have a base model. This is completely optional. You can technically store whatever schema's you want and they don't have to inherit from one another. I'm also +1 for creating an extend method in Schema to shortcut schema inheritance.

Anyway, here you go! :) 🏄

Todos

  • Reference mapping validation (throw error when reference type doesn't match)
  • More regression tests (reference mapping, ...)
  • Force discriminator schemas to inherit root (?)
  • Model inheritance (?)
  • ...
Off topic:

After this, we can discuss adding "discriminator" style mapping to embedded documents. They are sort of unrelated in the scope of this PR... Unless someone can think of a way to combine the two in a mongoose style fashion :P

@j
Copy link
Contributor Author

j commented Aug 15, 2013

ping @whitecolor @pongells

just in case you're interested

@aheckmann
Copy link
Collaborator

At first glance this looks good. Detailed feedback tomorrow.

@j
Copy link
Contributor Author

j commented Aug 15, 2013

Awesome. Yeah, I tried to simplify it and make it feel natural. I did most of this at 1am this morning, so I can probably dig more into the core to find cleaner ways. Looking forward to your opinion :)

On Wed, Aug 14, 2013 at 7:35 PM, Aaron Heckmann notifications@github.com
wrote:

At first glance this looks good. Detailed feedback tomorrow.

Reply to this email directly or view it on GitHub:
#1647 (comment)

@wclr
Copy link

wclr commented Aug 15, 2013

Looks nice for the first level docs. Can it work for embedded some docs in some way?

@j
Copy link
Contributor Author

j commented Aug 15, 2013

@whitecolor yeah, I have a use case for that currently, but have done it manually. I based this PR off the other attempts I've seen out there which affect the root level and give you the ability to do "instanceof" on the model/document level which isn't possible on the schema level (unless you create a custom type which casts to objects. That and it seems that most ORMs just do this on the root level.

If someone can think of a good way to do both at the same time, I can jump on that..

I was thinking of the following schema level options:

  1. Allow "enum" to be schema objects which automatically track types.
  2. Create a Schema.Types.DiscriminatorType.

Anyway, I'll wait thinking about that until I get feed back on this current implementation of root level discriminator mapping.

@pongells
Copy link

Looks nice. By the way, I found this plugin which seems to do something similar mongoose-schema-extend.

@j
Copy link
Contributor Author

j commented Aug 15, 2013

@pongells yeah I checked that out and got some of the motivation from it for developing this version.

I wanted to be able to do more of a natural way of doing this by having the ability to do the following:

var FooSchema = new Schema();

var BarSchema = new Schema();
util.inherits(BarSchema, FooSchema);

var Foo = mongoose.model('Foo', FooSchema);
var Bar = mongoose.model('Bar', BarSchema); // this would create a discriminator type 'Bar' automatically

but you can't inherit that way, also, most people may want a base schema to inherit from but not live in the same collection, which is why I demonstrated in my example of this PR by extending Schema to create a "base" schema.

The "Model" class already has a .model() method and a prototype.model() method, which could replace the .discriminator() method I created which would create a discriminator type if a Schema is passed into it.

It's current implementation just shortcuts fetching of a Model which is weird :P

So we could create an extend function in the Schema, but that seems misleading b/c someone may just want to literally extend the schema to copy it to create a separate and unrelated model:

i.e)

var BaseSchema = new Schema({ createdAt: Date, updatedAt: Date, deletedAt: Date });

var UserSchema = BaseSchema.extend({ firstName: String });
var PostSchema = BaseSchema.extend({ content: String });

So naturally I'd think of extending something as a factory method for inheriting and creating an instance of what I extend.. so an extend method may end up confusing people.

Also, the route I took, you don't have to extend anything. In fact all objects can be completely unrelated to each other and live in the same collection.

So my current stance is:

  • The "discriminator" method provides the most clarity.
  • Moving "discriminator" functionality into the "model" method makes even more sense, however, may not be BC because users "may" (but shouldn't be doing) user.model('Friends', FriendSchema) or Model.model('Friends', FriendSchema) (because Schema's can't be passed into these methods).. so Model.model / model.model would be cool if it only worked for discriminator types / nested models instead of referencing models in the connection.

Thoughts?

@@ -154,6 +155,7 @@ Schema.prototype.defaultOptions = function (options) {
, bufferCommands: true
, capped: false // { size, max, autoIndexId }
, versionKey: '__v'
, discriminatorKey: null // discriminator keys are '__t' when left null
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets set this to __t by default and remove the check for existence on https://github.com/LearnBoost/mongoose/pull/1647/files#L0R641

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

silly, that's what I had from the get-go but thought you wouldn't like it since it's not a discriminator schema :P changing now.

@aheckmann
Copy link
Collaborator

It might be nice if

Base.find({}, function(err, events) {
    expect.ok(events[2] instanceof Base);       // false

was true. We might be able to do some prototype hackery but may not be worth the trouble.

@j
Copy link
Contributor Author

j commented Aug 15, 2013

@aheckmann yeah, i agree. i'll look into that later.

if you like the direction of this, we can keep this as a WIP obviously to get feedback and perfect this way of doing discriminator types.

assert.ifError(err);
conversionEvent.save(function(err) {
assert.ifError(err);
BaseEvent.find({}, function(err, docs) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should add .sort('name') to ensure docs return in consistent order (varies on linux)

@aheckmann
Copy link
Collaborator

Really like the direction of this. I want to play around with it to get a user experience opinion and some more feedback yet.

done();
});

it('throws error when discriminator when discriminator with taken name is added', function(done) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/when discriminator with/with/

@j
Copy link
Contributor Author

j commented Aug 15, 2013

I agree with that statement. Just writing the tests made me like the feeling of the other attempted methods so far. I'd like to hear input.

@j
Copy link
Contributor Author

j commented Aug 16, 2013

@aheckmann is this fine? j/mongoose@31b958a

i'm assuming merge gets called on all query preparations

see the regression test (j/mongoose@2c2d591) i've added to show that findOneAndUpdate() works

@j
Copy link
Contributor Author

j commented Aug 16, 2013

Alright, I'm going to call it a night for now.. I'm assuming you work in the NY 10gen offices, so you're going to be up early (earlier than us lazy west coasters!). Here's some food for thought:

This implementation doesn't force schemes to be extended / inherited... meaning, you can create a "discriminator" and they have absolutely nothing to do with the "base" model's schema. They just share the same collection and that's it. However, most ORMs use discriminator mapping alongside inheritance.

For example, using PHP's Doctrine MongoDB ODM (https://github.com/doctrine/mongodb-odm) which I contribute to:

<?php

/**
 * @Document()
 * @DiscriminatorField(fieldName="__t")
 * @DiscriminatorMap({"person"="Person", "employee"="Employee"})
 */
class Person
{
    // ...
}

/**
 * @Document()
 */
class Employee extends Person
{
    // ...
}

So, should we do the same? I'm starting to think that it makes sense to do so b/c that's the main use-case of it.. and if you don't want to have any shared schema properties, the root Schema can be empty...

Then if we want to tackle the model inheritance, the hydrated document will always assert.ok(discriminatorDocument instanceof RootModel)

@aheckmann
Copy link
Collaborator

It would be nice if instanceof worked but not a showstopper.

@aheckmann
Copy link
Collaborator

West-coasters BTW :)

@j
Copy link
Contributor Author

j commented Aug 16, 2013

haha, I'll try hacking on the instanceof part. but with that said, do you think schemas should always inherit the root schema's properties or keep them unrelated?

also, what do you think of the method discriminator? what about "type", "embed" or putting this logic in the "model" method?.. or something else?

Here's some sudo-coding of options:

1.) Current implementation (does not inherit base schema)

var TransportationSchema = new Schema({ maxSpeed: Number });
var CarSchema = new Schema();
var BusSchema = new Schema();

var Transportation = mongoose.model('Transportation', TransportationSchema);
var Car = Transportation.discriminator('Car', CarSchema);
var Bus = Transportation.discriminator('Car', BusSchema);
// .. or
var Car = Transportation.model('Car', CarSchema);
// .. or
var Bus = Transportation.type('Car', BusSchema);

var ferrari = new Car(); // does not inherit from base schema and does not have maxSpeed field

// .. but users can do:

function BaseSchema() {}
util.inherits(BaseSchema, Schema);
var CarSchema = new BaseSchema();

// another note:

console.log(ferrari instanceof Transportation); // ???

// leads to "sub-option" of 1:
// * my personal opinion is that this only makes sense if schemas are related *
//     a.) Should `(ferrari instanceof Transportation` === true if schemas aren't related?
//     b.) Should `(ferrari instanceof Transportation` === false if schemas aren't related?

2.) Inherits base schema under the hood:

var TransportationSchema = new Schema({ maxSpeed: Number });
var CarSchema = new Schema();
var BusSchema = new Schema();

var Transportation = mongoose.model('Transportation', TransportationSchema);
var Car = Transportation.discriminator('Car', CarSchema);
var Bus = Transportation.discriminator('Car', BusSchema);
// .. or
var Car = Transportation.model('Car', CarSchema);
// .. or
var Bus = Transportation.type('Car', BusSchema);

var ferrari = new Car({ maxSpeed: 340 }); // automatically inherits base schema behind the scenes
console.log(ferrari instanceof Transportation); // true

3.) Move logic to Schema (like https://github.com/briankircho/mongoose-schema-extend)

var TransportationSchema = new Schema({ maxSpeed: Number });
var CarSchema = TransportationSchema.extend();
var BusSchema = TransportationSchema.extend();

var Transportation = mongoose.model('Transportation', TransportationSchema);
var Car = Transportation.discriminator('Car', CarSchema);
var Bus = Transportation.discriminator('Car', BusSchema);
// .. or
var Car = Transportation.model('Car', CarSchema);
// .. or
var Bus = Transportation.type('Car', BusSchema);

var ferrari = new Car({ maxSpeed: 340 }); // Schema.prototype.extend does this automatically
console.log(ferrari instanceof Transportation); // true

4.) Combination of extend and others

// create extend function or similar that ONLY extends and has nothing to do with discriminator mapping
var TransportationSchema = new Schema({ maxSpeed: Number });
var CarSchema = TransportationSchema.extend();
var BusSchema = TransportationSchema.extend();

// or perhaps add Schema.inherits
var TransportationSchema = new Schema({ maxSpeed: Number });
var CarSchema = new Schema();
Schema.inherits(CarSchema, TransportationSchema);
// or try to make this work: (?? i haven't looked into this yet ??)
util.inherits(CarSchema, TransportationSchema)

// then using a `Transportation.discriminator` creates the discriminator mapping
var Transportation = mongoose.model('Transportation', TransportationSchema);
var Car = Transportation.discriminator('Car', CarSchema);
var Bus = Transportation.discriminator('Car', BusSchema);

4.) Or... just vomiting stuff out (seems kinda cool though)

var CarSchema = new Schema();
var BusSchema = new Schema();
var TransportationSchema = new Schema(
    CarSchema,
    BusSchema,
    { discriminatorKey: '_type', discriminatorMap: { Car: CarSchema, Bus, BusSchema }}
);

var Transportation = mongoose.model('Transportation', TransportationSchema);
var Car = Transportation.type('Car');
var Bus = Transportation.type('Bus');

All the options that have extend / inheritance in the schema (minus auto discriminator mapping) are unrelated to this PR and should be separate.

@pongells
Copy link

I personally like option 2. It seems very straight forward.

var TransportationSchema = new Schema({ maxSpeed: Number });
var CarSchema = new Schema();

var Transportation = mongoose.model('Transportation', TransportationSchema);
var Car = Transportation.model('Car', CarSchema);

I'd like having the ".model" - it makes it clear Car is a model that comes from Transportation. I also think inheriting the super-schema is good.. since Car comes from Transportation, it should have a maxSpeed by default.

@j
Copy link
Contributor Author

j commented Aug 16, 2013

@pongells yeah, I've been leaning towards that also. I haven't implemented the automatic inheritance yet.

We're going to have to make sure we adopt the methods, statics, validations, etc, from the base schema.

I'm also fond of Transportation.model over Transportation.discriminator because of the reason you said. It's clear.

HOWEVER, I'm not sure if anyone is currently abusing Model.model (see https://github.com/LearnBoost/mongoose/blob/master/lib/model.js#L602)

It's basically a shortcut for getting a model from the same connection instance... I'd rather force Model.model to be for discriminator types instead of shortcutting this.

For example:

var UserSchema = new Schema({ /** ... */ });

var TransportationSchema = new Schema({ /** ... */ });
var CarSchema = new Schema({ /** ... */ });

var UserSchema = mongoose.model('User', UserSchema);
var Transportation = mongoose.model('Transportation', TransportationSchema);
Transportation.model('Car', CarSchema);

var Car = Transportation.model('Car');
var User = Transportation.model('User'); // should throw exception in my opinion

If my example above can be implemented, then discriminators would suddenly become very clear as to what's happening. It's clean.

Thoughts @aheckmann @pongells ?

@pongells
Copy link

Woah, so atm:

var test = mongoose.model('User', UserSchema).model('Transportation')
console.log(test instanceof Transportation); // true?

@j
Copy link
Contributor Author

j commented Aug 16, 2013

Yeah, as long as they share the same connection... lol.

@pongells
Copy link

Well, I'd say I agree with you. However, I am quite new around here.. maybe there is a reason for that Model.model..

By the way, how would this work?

    var TransportationSchema = new Schema({ maxSpeed: Number });
    var CarSchema = new Schema({});

    var Transportation = mongoose.model('Transportation', TransportationSchema);
    var Car = Transportation.model('Car', CarSchema);

    var ParkSchema = new Schema({
        transportations: [TransportationSchema]
    })

@nikmartin
Copy link
Contributor

Are there any documents on how to use this feature other than this thread? I desperately need schema inheritance, but am not sure where to start.

@j
Copy link
Contributor Author

j commented Nov 25, 2013

Check out one of the latest PRs.

On Mon, Nov 25, 2013 at 12:24 PM, Nik Martin notifications@github.com
wrote:

Are there any documents on how to use this feature other than this thread? I desperately need schema inheritance, but am not sure where to start.

Reply to this email directly or view it on GitHub:
#1647 (comment)

@nikmartin
Copy link
Contributor

Got it! And I hate jade too.

@paulgration
Copy link

Having some troubles with schema inheritance with discriminators and trying to populate using 3.8 - http://stackoverflow.com/questions/20430484/mongoose-schema-inheritance-and-model-populate
Any ideas?

@nikmartin
Copy link
Contributor

replied on SO for that sweet sweet karma

On Fri, Dec 6, 2013 at 12:37 PM, pmgration notifications@github.com wrote:

Having some troubles with schema inheritance with discriminators and
trying to populate using 3.8 -
http://stackoverflow.com/questions/20430484/mongoose-schema-inheritance-and-model-populate
Any ideas?


Reply to this email directly or view it on GitHubhttps://github.com//pull/1647#issuecomment-30017733
.

@tjmehta
Copy link

tjmehta commented Jan 26, 2014

I notice the following error:

Uncaught Error: Discriminator options are not customizable (except toJSON & toObject)

When settings auto index to false:

var ModelSchema = new Schema({...});
ModelSchema.set('autoIndex', false); // removing this line prevents error
var model = new Base.discriminator('Model', ModelSchema);

Removing the autoIndex:false line prevents the error.

@jlchereau
Copy link

Troubling facts...

Let's say we have done:

var Transportation = mongoose.model('Transportation', TransportationSchema),
       Car = Transportation.discriminator('Car', CarSchema);
  1. The following does not work (although var Transportation = mongoose.model('Transportation'); works):
var Car = Transportation.discriminator('Car');
  1. The following works with unexpected results: a transportations and a cars collections are created:
var Car =  mongoose.model('Car');
  1. It seems the only correct way to reference Car is:
var Car = Transportation.discriminators.Car;

or

var Car = Transportation.discriminators['Car'];

This is inconsistent and one would expect at least 1) to work (and possibly 2)

@jlchereau
Copy link

More troubling facts....

Let's says we have done:

var Transportation = mongoose.model('Transportation', TransportationSchema),
    Car = Transportation.discriminator('Car', CarSchema),
    Train = Transportation.discriminator('Train', TrainSchema);

then

new Train({...}).save(function(error, data){
    expect(data).to.be.an.instanceof(Transportation); //passes successfully
    expect(data).to.be.an.instanceof(Car); //passes successfully (Oops!)
    expect(data).to.be.an.instanceof(Train); //passes successfully
});

@techjeffharris
Copy link

Is current documentation available for these features...?

I am not finding any mention here: http://mongoosejs.com/docs/

@vkarpov15
Copy link
Collaborator

Unfortunately discriminators aren't really covered in the docs right now @techjeffharris .

@techjeffharris
Copy link

Thanks, @vkarpov15. If you were to use the node.js stability index, about where would the polymorphic schema API be?

I would like to implement core-features if the API is relatively stable (in which case, I'll help with the documentation--albeit later this month), but if its still going to be a while, I'll just use the mongoose-schema-extend module in the interim.

Thanks for all the hard work, everyone!

@vkarpov15
Copy link
Collaborator

Hi @techjeffharris, I unfortunately am not familiar enough with discriminators right now to make such an estimate. However, I'm reasonably certain that the API won't change in the next major version.

@RangerMauve
Copy link

Still no docs?

@loudwinston
Copy link

+1 for adding some documentation on this. Seems like a great feature but I don't feel confident using it until there's some docs.

@gkaran
Copy link

gkaran commented Sep 30, 2014

+1 for documentation too. It is a great feature but the lack of documentation make it (sadly) useless...

@hartca
Copy link

hartca commented Oct 1, 2014

Example in the documentation working nicely:

Person
✓ should be an instance of Person
✓ should not be an instance of Boss

Boss
✓ should be an instance of Person
✓ should be an instance of Boss

@jlchereau
Copy link

@hartca, if you are referring to my comment of May 30, you need 2 discriminators to check my point. Add NoBoss, and you will realize that Boss is an instance of NoBoss and vice versa (unless it has been corrected in Mongoose since May 30).

@guiltry
Copy link

guiltry commented Oct 20, 2014

What if we want to extend more than once ?
ex: Thing -> Human -> Woman

@RangerMauve
Copy link

@guiltry Querying Human won't get Woman in that case, sadly. :[ You only really get one level of inheritance because discriminators only hold one value.

Maybe there could be a way to have functionality like that by having multiple descriptors, or maybe having paths for the descriptor and using regex. (regex is super slow though)

@guiltry
Copy link

guiltry commented Oct 20, 2014

Yeah, I've tried it.
Do we have a solution for this feature ?
It's gonna be very very useful. 😁

@RangerMauve
Copy link

If the discriminator was an array, we could store all of the types that it matches against, so Woman in your example might have a discriminator that would look like

["thing","human","woman"]

Then you could query for any thing for any thing that's also human and for any human that's a woman
At this point it might be easier to just roll your own solution to this rather than wait for Mongoose. Then you might be able to contribute it pack into the project. :D

@FREEZX
Copy link

FREEZX commented Oct 21, 2014

I have a base schema called User, and two other schemas that inherit from User, called Person and Company.
I instantiate a User.
I would like to add some more data to User and change its type to either Person or Company.
Anyone has any idea how i could do this?

@FREEZX
Copy link

FREEZX commented Oct 21, 2014

For anyone else that might need to do something similar, use the Model.hydrate method.
If you need a repo with the 3.8.17 version with only the hydrate method added, you can use this:
https://github.com/A8tech/mongoose

The way to use it is as follows(pseudocode):
var user = User.findById('youruseridhere');
var person = Person.hydrate(user);
person.save();

Now you have casted the user into a person.

@hartca
Copy link

hartca commented Oct 21, 2014

👍

@ericsaboia
Copy link

Well done, @j!

I just get an issue using discriminators: If you have some references in your child model, you can't populate it using the base model:

Child:

new BaseSchema({
  user: { type: mongoose.Schema.Types.ObjectId, ref: 'User' }
});

This works as expected:

Child.find().populate('user');

But this does not populate referenced users:

Base.find().populate('user');

@vkarpov15
Copy link
Collaborator

@ericsaboia can you open up a new issue for that? Makes it much easier to track :)

@ericsaboia
Copy link

@vkarpov15 sure.

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

Successfully merging this pull request may close these issues.

None yet