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

Discriminator support for Sub-Documents #1856

Closed
ElliotChong opened this Issue Dec 30, 2013 · 49 comments

Comments

Projects
None yet
@ElliotChong

ElliotChong commented Dec 30, 2013

Starting a feature request to allow discriminator use within sub-documents.

Playing around with a possible API:

class EmployeeSchema extends Schema
    constructor: ->
        super

        @add
            name: { type: String, required: true }
            salary: { type: Number }

class ManagerSchema extends EmployeeSchema
    constructor: ->
        super

        @add
            subordinates: { type: Number }

        @method
            salaryPerSubordinate: ->
                @salary / @subordinates

# Discriminators can only be set once a Model has been defined; however, sub-documents
# only use Schemas.
# Not sure what the best way to handle this is since defining it at the Model level makes
# sense when not worrying about sub-documents and the distinction between the two
# methods wouldn't be intuitive.
EmployeeSchema.discriminator 'Manager', ManagerSchema

class CompanySchema extends Schema
    @add
        name: { type: String, required: true }
        employees: [ new EmployeeSchema() ]

Company = mongoose.model 'Company', new CompanySchema()
acmeInc = new Company { name: "Acme Inc." }

# Discriminator is inferred through the definition of the dsciriminatorKey '__t'
acmeInc.employees.push { __t: "Manager", name: "Roger Rabbit", salary: 0.01, subordinates: 5 }

# No discriminator is inferred, defaults to Employee
acmeInc.employees.push { name: "Road Runner", salary: 2 }

# Perhaps the 'create' method accepts a discriminator as an optional parameter, or we could
# fall back to requiring the discriminatorKey to be manually defined like in the push example
coyote = acmeInc.employees.create "Manager", { name: "Coyote", salary: 500, subordinates: 3 }
console.log coyote.salaryPerSubordinate()

acmeInc.employees.push coyote

# Sub-documents are cast to their respective discriminated Schema
for employee in acmeInc.employees
    switch employee.__t
        when "Manager"
            console.log employee.name, employee.salary, employee.salaryPerSubordinate()
        else
            console.log employee.name, employee.salary
@dylan-baskind

This comment has been minimized.

Show comment
Hide comment
@dylan-baskind

dylan-baskind Feb 3, 2014

+1 for this feature!

dylan-baskind commented Feb 3, 2014

+1 for this feature!

@pavel-kudinov

This comment has been minimized.

Show comment
Hide comment
@pavel-kudinov

pavel-kudinov May 26, 2014

+1 for this feature!

pavel-kudinov commented May 26, 2014

+1 for this feature!

@alexahdp

This comment has been minimized.

Show comment
Hide comment
@alexahdp

alexahdp May 26, 2014

+1 for this feature!

alexahdp commented May 26, 2014

+1 for this feature!

@mmoulton

This comment has been minimized.

Show comment
Hide comment
@mmoulton

mmoulton May 27, 2014

+1 for this feature!

mmoulton commented May 27, 2014

+1 for this feature!

@scarych

This comment has been minimized.

Show comment
Hide comment
@scarych

scarych May 28, 2014

+1 for this feature!

scarych commented May 28, 2014

+1 for this feature!

@xuntaka

This comment has been minimized.

Show comment
Hide comment
@xuntaka

xuntaka May 28, 2014

+1 for this feature!

xuntaka commented May 28, 2014

+1 for this feature!

@AndreeVG

This comment has been minimized.

Show comment
Hide comment
@AndreeVG

AndreeVG May 28, 2014

+1 for this feature!

AndreeVG commented May 28, 2014

+1 for this feature!

@cynovg

This comment has been minimized.

Show comment
Hide comment
@cynovg

cynovg May 28, 2014

+1 for this feature!

cynovg commented May 28, 2014

+1 for this feature!

@Bravo13

This comment has been minimized.

Show comment
Hide comment
@Bravo13

Bravo13 May 28, 2014

+1 for this feature!

Bravo13 commented May 28, 2014

+1 for this feature!

@kyaroslav

This comment has been minimized.

Show comment
Hide comment
@kyaroslav

kyaroslav May 28, 2014

+1 for this feature!

kyaroslav commented May 28, 2014

+1 for this feature!

@demetr1us

This comment has been minimized.

Show comment
Hide comment
@demetr1us

demetr1us May 28, 2014

+1 for this feature!

demetr1us commented May 28, 2014

+1 for this feature!

@mxmCherry

This comment has been minimized.

Show comment
Hide comment
@mxmCherry

mxmCherry May 28, 2014

+1 for this feature!

mxmCherry commented May 28, 2014

+1 for this feature!

@equinox7

This comment has been minimized.

Show comment
Hide comment
@equinox7

equinox7 May 28, 2014

+1 for this feature!

equinox7 commented May 28, 2014

+1 for this feature!

@TimonKK

This comment has been minimized.

Show comment
Hide comment
@TimonKK

TimonKK May 28, 2014

+1 for this feature!

TimonKK commented May 28, 2014

+1 for this feature!

@izdesenko

This comment has been minimized.

Show comment
Hide comment
@izdesenko

izdesenko May 28, 2014

+1 for this feature!

izdesenko commented May 28, 2014

+1 for this feature!

@dionys

This comment has been minimized.

Show comment
Hide comment
@dionys

dionys May 28, 2014

+1 for this feature.

dionys commented May 28, 2014

+1 for this feature.

@kevindente

This comment has been minimized.

Show comment
Hide comment
@kevindente

kevindente commented Jun 13, 2014

+1

@Coderah

This comment has been minimized.

Show comment
Hide comment
@Coderah

Coderah commented Jun 13, 2014

+1

@MOuli90

This comment has been minimized.

Show comment
Hide comment
@MOuli90

MOuli90 Aug 6, 2014

+1 for this feature!

MOuli90 commented Aug 6, 2014

+1 for this feature!

@UltCombo

This comment has been minimized.

Show comment
Hide comment
@UltCombo

UltCombo Jan 23, 2015

+1 for this feature!

UltCombo commented Jan 23, 2015

+1 for this feature!

@vkarpov15 vkarpov15 added this to the 4.2 milestone Apr 7, 2015

@derickbailey

This comment has been minimized.

Show comment
Hide comment
@derickbailey

derickbailey Apr 7, 2015

@vkarpov15 thanks for putting this on the roadmap! this is something that will make my life significantly easier, as I'm dealing with varying data structures in sub-documents.

derickbailey commented Apr 7, 2015

@vkarpov15 thanks for putting this on the roadmap! this is something that will make my life significantly easier, as I'm dealing with varying data structures in sub-documents.

@vbogdanov

This comment has been minimized.

Show comment
Hide comment
@vbogdanov

vbogdanov commented Apr 14, 2015

+1

@unusualbob

This comment has been minimized.

Show comment
Hide comment
@unusualbob

unusualbob commented May 26, 2015

+1

@jonstorer

This comment has been minimized.

Show comment
Hide comment
@jonstorer

jonstorer commented Jun 2, 2015

nudge

@vkarpov15 vkarpov15 modified the milestones: 4.3, 4.2 Sep 21, 2015

@mathieug

This comment has been minimized.

Show comment
Hide comment
@mathieug

mathieug commented Oct 22, 2015

+1

@ducdigital

This comment has been minimized.

Show comment
Hide comment
@ducdigital

ducdigital Nov 23, 2015

+1, this need to be done <3

ducdigital commented Nov 23, 2015

+1, this need to be done <3

@ducdigital

This comment has been minimized.

Show comment
Hide comment
@ducdigital

ducdigital Nov 23, 2015

By the way any progress for this? Is there any other work around or method that can implement this behavior without touching the mongoose core?

Thanks!

ducdigital commented Nov 23, 2015

By the way any progress for this? Is there any other work around or method that can implement this behavior without touching the mongoose core?

Thanks!

@gflandre

This comment has been minimized.

Show comment
Hide comment
@gflandre

gflandre Nov 24, 2015

+1 for this feature!

gflandre commented Nov 24, 2015

+1 for this feature!

@ducdigital

This comment has been minimized.

Show comment
Hide comment
@ducdigital

ducdigital Dec 10, 2015

@vkarpov15: I did take liberty to create a small module to have this feature implement on custom Schema:

Here is the gist: https://gist.github.com/ducdigital/cb0926f1a27127369813 (contains the module and test file)

The sample usage is in the test file.

This works in most of the case except there are some downsides:

  • Cannot have required validation in children attributes
  • Cannot have different type on the same attribute name.

Let me know what do you think about this module.
My main concern right now is to able to have custom validation on different schema. I am thinking of generating .pre('save') to validate those document. Are there any better way?

ducdigital commented Dec 10, 2015

@vkarpov15: I did take liberty to create a small module to have this feature implement on custom Schema:

Here is the gist: https://gist.github.com/ducdigital/cb0926f1a27127369813 (contains the module and test file)

The sample usage is in the test file.

This works in most of the case except there are some downsides:

  • Cannot have required validation in children attributes
  • Cannot have different type on the same attribute name.

Let me know what do you think about this module.
My main concern right now is to able to have custom validation on different schema. I am thinking of generating .pre('save') to validate those document. Are there any better way?

@vkarpov15

This comment has been minimized.

Show comment
Hide comment
@vkarpov15

vkarpov15 Dec 10, 2015

Collaborator

I think pre('save'); is the only way to do it. That's how we do validation for subdocs anyway

Collaborator

vkarpov15 commented Dec 10, 2015

I think pre('save'); is the only way to do it. That's how we do validation for subdocs anyway

@mitsos1os

This comment has been minimized.

Show comment
Hide comment
@mitsos1os

mitsos1os Feb 14, 2016

+1 for this feature

mitsos1os commented Feb 14, 2016

+1 for this feature

@vkarpov15 vkarpov15 modified the milestones: 4.6, 4.5 Feb 19, 2016

@Fonger

This comment has been minimized.

Show comment
Hide comment
@Fonger

Fonger Mar 20, 2016

Contributor

+1

Contributor

Fonger commented Mar 20, 2016

+1

@davebaol

This comment has been minimized.

Show comment
Hide comment
@davebaol

davebaol Apr 21, 2016

+1
I'd say that this feature is crucial for any non-trivial data model :)

davebaol commented Apr 21, 2016

+1
I'd say that this feature is crucial for any non-trivial data model :)

@vkarpov15

This comment has been minimized.

Show comment
Hide comment
@vkarpov15

vkarpov15 Apr 22, 2016

Collaborator

@davebaol hahah you and I have very different ideas about what non-trivial means :)

Collaborator

vkarpov15 commented Apr 22, 2016

@davebaol hahah you and I have very different ideas about what non-trivial means :)

@davebaol

This comment has been minimized.

Show comment
Hide comment
@davebaol

davebaol Apr 22, 2016

@vkarpov15
Well, in this context non-trivial is clearly a hyperbole 😉
And regardless of the context it's a litotes too :bowtie:

Anyways, figures of speech aside, I really appreciate your hard work on mongoose. 👍
Just wanted to say that this is likely the most important feature among missing ones. 😇

davebaol commented Apr 22, 2016

@vkarpov15
Well, in this context non-trivial is clearly a hyperbole 😉
And regardless of the context it's a litotes too :bowtie:

Anyways, figures of speech aside, I really appreciate your hard work on mongoose. 👍
Just wanted to say that this is likely the most important feature among missing ones. 😇

@vkarpov15

This comment has been minimized.

Show comment
Hide comment
@vkarpov15

vkarpov15 Apr 23, 2016

Collaborator

Thanks for the kind words 🍻 it's definitely a priority, just not for the next 2 months.

Collaborator

vkarpov15 commented Apr 23, 2016

Thanks for the kind words 🍻 it's definitely a priority, just not for the next 2 months.

@davebaol

This comment has been minimized.

Show comment
Hide comment
@davebaol

davebaol Apr 23, 2016

Great! Sounds like a plan 😎

davebaol commented Apr 23, 2016

Great! Sounds like a plan 😎

@whitef0x0

This comment has been minimized.

Show comment
Hide comment
@whitef0x0

whitef0x0 May 31, 2016

Is this close to completion?

whitef0x0 commented May 31, 2016

Is this close to completion?

@vkarpov15

This comment has been minimized.

Show comment
Hide comment
@vkarpov15

vkarpov15 Jun 1, 2016

Collaborator

@whitef0x0 no progress thus far, contributions welcome.

Collaborator

vkarpov15 commented Jun 1, 2016

@whitef0x0 no progress thus far, contributions welcome.

@davebaol

This comment has been minimized.

Show comment
Hide comment
@davebaol

davebaol Jan 30, 2017

@vkarpov15
Sweet!
Is there any documentation on this?

davebaol commented Jan 30, 2017

@vkarpov15
Sweet!
Is there any documentation on this?

@vkarpov15

This comment has been minimized.

Show comment
Hide comment
@vkarpov15

vkarpov15 Feb 2, 2017

Collaborator

Working on a blog post about it to consolidate my thoughts and then going to put that into doc form. https://github.com/Automattic/mongoose/pull/4910/files#diff-391b023b3deb2ee3a2fe9a2ffe802070 has a test case that you can extrapolate the general idea from.

Collaborator

vkarpov15 commented Feb 2, 2017

Working on a blog post about it to consolidate my thoughts and then going to put that into doc form. https://github.com/Automattic/mongoose/pull/4910/files#diff-391b023b3deb2ee3a2fe9a2ffe802070 has a test case that you can extrapolate the general idea from.

@davebaol

This comment has been minimized.

Show comment
Hide comment

davebaol commented Feb 6, 2017

@vkarpov15

This comment has been minimized.

Show comment
Hide comment
@vkarpov15

vkarpov15 Feb 12, 2017

Collaborator

Thanks for posting @davebaol

Collaborator

vkarpov15 commented Feb 12, 2017

Thanks for posting @davebaol

@meammeiam

This comment has been minimized.

Show comment
Hide comment
@meammeiam

meammeiam Feb 19, 2017

I tried this tonight from the blog and was wondering how you add another event once the Batch is created?

I tried pushing an event, but it saves an empty item.

`Batch.create(batch).then(function(doc) {

doc.events.push({ kind: 'Purchased', product: 24 });
doc.save().then(function(doc) {
    console.log(doc);
});

});`

I also tried create and it returns an empty item.

console.log(doc.events.create({ kind: 'Purchased', product: 24 }));

meammeiam commented Feb 19, 2017

I tried this tonight from the blog and was wondering how you add another event once the Batch is created?

I tried pushing an event, but it saves an empty item.

`Batch.create(batch).then(function(doc) {

doc.events.push({ kind: 'Purchased', product: 24 });
doc.save().then(function(doc) {
    console.log(doc);
});

});`

I also tried create and it returns an empty item.

console.log(doc.events.create({ kind: 'Purchased', product: 24 }));

@vkarpov15

This comment has been minimized.

Show comment
Hide comment
@vkarpov15

vkarpov15 Feb 20, 2017

Collaborator

@meammeiam that should work, if it doesn't that's a bug. Please open up a separate issue

Collaborator

vkarpov15 commented Feb 20, 2017

@meammeiam that should work, if it doesn't that's a bug. Please open up a separate issue

@meammeiam

This comment has been minimized.

Show comment
Hide comment
@meammeiam

meammeiam Feb 21, 2017

Okay, well maybe I need to upgrade mongoDB. I'm at version 3.3.6. I'll have to compile it from source. Mongoose is at 4.8.4. If it's still not working, I'll open a bug. Thank you!

meammeiam commented Feb 21, 2017

Okay, well maybe I need to upgrade mongoDB. I'm at version 3.3.6. I'll have to compile it from source. Mongoose is at 4.8.4. If it's still not working, I'll open a bug. Thank you!

@vkarpov15

This comment has been minimized.

Show comment
Hide comment
@vkarpov15

vkarpov15 Feb 21, 2017

Collaborator

Don't use mongodb 3.3.x, that's an unstable dev release. Please use 3.2.x or 3.4.x

Collaborator

vkarpov15 commented Feb 21, 2017

Don't use mongodb 3.3.x, that's an unstable dev release. Please use 3.2.x or 3.4.x

@c1moore

This comment has been minimized.

Show comment
Hide comment
@c1moore

c1moore Jul 15, 2017

Is there a similar feature for normal embedded documents that do not appear in an array? Using the notation in your blog, I would want something like the below.

const EventSchema = new Schema({ message: String });
const SomeSchema = new Schema({ event: EventSchema });

SomeSchema.path('event').discriminator('Clicked', new Schema({
  element: {
    type: String,
    required: true
  }
}));

SomeSchema.path('event').discriminator('Purchased', new Schema({
  product: {
    type: String,
    required: true
  }
}));

const Something = mongoose.model('Something', SomeSchema);

Should I open a new issue for that?

c1moore commented Jul 15, 2017

Is there a similar feature for normal embedded documents that do not appear in an array? Using the notation in your blog, I would want something like the below.

const EventSchema = new Schema({ message: String });
const SomeSchema = new Schema({ event: EventSchema });

SomeSchema.path('event').discriminator('Clicked', new Schema({
  element: {
    type: String,
    required: true
  }
}));

SomeSchema.path('event').discriminator('Purchased', new Schema({
  product: {
    type: String,
    required: true
  }
}));

const Something = mongoose.model('Something', SomeSchema);

Should I open a new issue for that?

@davebaol

This comment has been minimized.

Show comment
Hide comment
@davebaol

davebaol Jul 16, 2017

@c1moore

Is there a similar feature for normal embedded documents that do not appear in an array?

No, currently there's no such feature as discriminators in single nested subdocuments.

Should I open a new issue for that?

No, you should not because there is an open issue already, see #5244
Maybe add your comment or just a '+1' there 😉

davebaol commented Jul 16, 2017

@c1moore

Is there a similar feature for normal embedded documents that do not appear in an array?

No, currently there's no such feature as discriminators in single nested subdocuments.

Should I open a new issue for that?

No, you should not because there is an open issue already, see #5244
Maybe add your comment or just a '+1' there 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment