Suggestion for single embedded document support

[pmlopes]

Hi,

One of my challenges with mongoose is to have a single embedded doc in a doc, this patch allows this kind of schema declaration:

var mongoose = require('mongoose'),
Schema = mongoose.Schema;

mongoose.connect('mongodb://localhost/mongoose');

var PersonSchema = new Schema({
FirstName: {type: String, required: true},
LastName: String
});

var BlogPostSchema = new Schema({
creator: PersonSchema
});

/**

• Define model.
  */

var BlogPost = mongoose.model('BlogPost', BlogPostSchema);
var Person = mongoose.model('Person', PersonSchema);

var person = new Person({FirstName: 'Paulo', LastName: 'Lopes'});

var post = new BlogPost({title: 'test', creator: person});

post.save(function(err) {
if (err) return console.log(err);
BlogPost.find({}, function(err, docs) {
if(err) return console.log(err);
console.log(docs[0].creator);
docs[0].creator = null;
docs[0].save(function(err) {
if(err) return console.log(err);
mongoose.disconnect();
});
});
});

I don't know much about the internals of mongoose but this seems to work for me... could someone review and give some feedback? Thanks

[aheckmann]

looking at this pull request it appears that this allows one to pass a schema for a property of another schema. the reason we haven't added this support in the past is b/c this leaves us wondering if all pre-hooks will be executed the same way for the pseudo-child document as well as it implies that we can call save() on that child.

example:

var Child = new Schema({ name: String });
var Par = new Schema({ child: Child })
...
var p = new Par({ child: c })
p.c.save(); ???
p.c.remove(); ???
p.save(); // does this trigger all hooks on the child too?

Its pretty vague, so we've taken the approach that its better to just pass in the tree directly when creating the parent schema and avoid the confusion altogether.

[mlegenhausen]

Why not simply adding a variable to the schema that only returns the schema definition given to the schema object.

Example:

var Child = new Schema({ name: String });
var Par = new Schema({ child : Child.Type }); // Child.Type returns { name: String }

So the resulting child has no save or remove methods and it is a nice shortcut for accessing the schema definition.

[pmlopes]

You can do that already like this:

var Child = new Schema({ name: String });
var Par = new Schema({ child : Child.tree });

However... :) this becomes what mongoose calls a MixedType and no
validation is performed as well you must notify of changes on the
subtype by calling markModified.

e.g.:

ParModel.child.name = 'Paulo';
ParModel.save(cb);

the above code does not save the changed name you must do:

ParModel.child.name = 'Paulo';
ParModel.markModified('child');
ParModel.save(cb);

Now it saves...

But all i wanted was automated validation :( now i need to do what i
expected mongoose to do for me...

[aheckmann]

"However... :) this becomes what mongoose calls a MixedType and no validation is performed"

Not the case: https://gist.github.com/1345540

As for using Child.Type or Child.tree, neither will give you what you want reliably ATM. We could probably add support for that pretty easily tho.

[timoxley]

" as well as it implies that we can call save() on that child."

I currently see that same confusion regularly with people calling save() on embedded docs in an array, not realizing they can only save() on the parent, so the confusion we're trying to avoid already exists in one form.

Perhaps consistency > confusion until something better is figured out?

[szimek]

Is it possible to have this patch merged? I need to embed a single document with its own validations and I really don't want to store it in an array...

[aheckmann]

oh, I thought this was closed already. we're not adding support for this. we should really remove the save method for embedded docs but that seems to be a placeholder so that hooks work on them.

[tarqd]

Why not some logic like this in save()?

if(this.isEmbedded)
  throw new Error('You can not call save on embedded models. Call save on the parent model');

Pre-hooks should definitely be executed when you call save on the parent. If you're embedding the model, it's implying you're after all the bells and whistles that go with it (hooks, validation, etc). If you didn't want that functionality you'd just go with a normal object.

[aheckmann]

currently it is necessary for the save method on the embedded doc to not throw b/c hooks.js relies on it. the hooks system needs to be rewritten

[rycfung]

As for using Child.Type or Child.tree, neither will give you what you want reliably ATM. We could probably add support for that pretty easily tho.

So is it going to be added? This seems to be a good workaround for the time being. Currently, there's no way to retrieve that plain object from the Schema and set it to other Schemas.

[aheckmann]

its not a high priority but it would be nice. i'll accept a pull request for it.

[aheckmann]

nevermind, wrong issue. this would be confusing.

[neverfox]

Essentially, it seems like what people want (myself included) is support in Mongoose for a sort of "partial" for Schemas so that they can write DRY code. A common example is Address, which might be used by several Schemas in your data model, but never really needs to live on its own.

I was all ready to suggest literally adding a Partial class to Mongoose that would be just like a Schema but would have no life of its own. Then I realized that would just be a plain JS object, wouldn't it? So I'm wondering why aren't others just creating plain JS objects for their reusable definitions? Is it just because they also want those partials to be stand-alone documents? In the first example here, would anyone ever be storing a generic Person in the database? If not, why wrap it in a Schema? I admit I haven't tried the plain JS object approach, but it seems like it would get you where you want to go.

[yields]

+1 for embedded docs.

[amarcadet]

+1

[kevindente]

+1 for this feature.

[gabrielmancini]

+1

[Jeff-Lewis]

+1

[ElliotChong]

👍 +1 Sharing the virtuals, methods, and hooks from a well-defined schema in a single embedded document would be really useful. Throwing an error on 'save' for a collection / single embedded document to alert the user that it's a noop should clear up any confusion there.

[aheckmann]

pull request welcome.

[cyberwombat]

@aheckmann can you explain this? https://gist.github.com/aheckmann/1345540 You posted that in reference to validating mixed types but I don't see how that can work. 'Child' schema is never used in this code

[chopachom]

+1 for this feature

[csrx]

Another +1

[ghost]

+1

[Reggino]

+1

[leoneparise]

👍

[jonlil]

👍

[ashaffer]

@Jeff-Lewis @vkarpov15

I agree that we should try to define the scope of this feature, but the reason I was requesting a place to discuss 4.x more generally was that I think the reason this feature hasn't been implemented yet, and the reason that it is so difficult to fit into the existing codebase is that mongoose has grown pretty byzantine over time.

As such, I think any approach to implementing a complex additional feature like this should probably begin with modularizing and simplifying the existing codebase. I.e. pulling features like population and mquery which are relatively orthogonal to the core of mongoose out into official plugins, etc.

[Jeff-Lewis]

@ashaffer Thanks. I suspected this should be considered a larger undertaking than possibly expressed in this thread. If implementing this carries the baggage of splitting mongoose core, those of us with +1's should not expect anything for a while, if ever.

AFAIK, 4.0 Release notes are here.

[vkarpov15]

Don't worry too much about mquery - I'm inclined to stop using mquery in mongoose in the future, because IMO its not an effective abstraction. Does more to obfuscate the internal code than anything else.

Re: this issue, I don't see it as being dependent on splitting the core at all. The change in the API is relatively simple, I just have not experimented enough to see what roadblocks there are.

[ashaffer]

Ya, my point was more that those things should probably be configurable by the end user, rather than included as core features of mongoose. And that if that stuff is pulled out of the core, it'll be easier to extend the actual core features of the models like this in a way that makes sense now and in the future.

E.g.

mongoose.plugin(require('mongoose-populate'))
mongoose.plugin(require('mongoose-mquery'));

Then people can build their own query engines or population strategies either replacing the official plugins or building on top of them, and the core codebase will be far simpler to understand.

Also, mquery and population are just two examples that came to mind, there are probably a number of other features that this could be done with.

[drewtunes]

+1

[suceava]

+1

[bertramn]

+1

[ramSeraph]

+1 This was a rude shock.

[dozoisch]

👍 !

[alejandromagnorsky]

+1

[sirhoe]

+1

[josencv]

+1

[jeremyml]

+1

[julienboulay]

+1

[netusco]

+1

[molinto]

+1

[v1k4s]

+1

[adamreisnz]

+1

[zaun]

@vkarpov15 - Is there any status on this? Its version 4.1.0 but when I set a property to a schema I get errors, it still needs to be in an array of length 1. It's also a closed issue, should I assume its just not going to be done for the foreseeable future/ever?

[vkarpov15]

The reason this is closed is that its a pull request that will never get merged. If you look at the thread you'll see that this issue has been moved over to #2689

[adamreisnz]

Thanks @vkarpov15 .
It seems this feature is scheduled for the 4.2 milestone. Good to hear.

[zaun]

Thank for the info @vkarpov15
I'm glad to see it'll be added but disappointed it'll be so far out. I can't complain though, I'm not the one working on the project.

[willerup]

I ran into this issue and then found this thread. I updated my mongoose to 4.2 and "voila" it now works as expected. Thank you mongoosers!

[vkarpov15]

@willerup glad you like it!

For posterity's sake, mongoose >= 4.2.0 supports this behavior.

[joniba]

I'm on 4.2.5 and still getting this MongooseError: "Did you try nesting Schemas? You can only nest using refs or arrays."

Is there an example or documentation somewhere of this functionality?

[vkarpov15]

@joniba single nested subdoc docs

[jeremyml]

Great feature - thank you!

[vkarpov15]

@jeremyml glad you find it useful 🍻