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

Any schema with a domain attribute fails to save. #1338

Closed
jaredhanson opened this issue Feb 10, 2013 · 11 comments
Closed

Any schema with a domain attribute fails to save. #1338

jaredhanson opened this issue Feb 10, 2013 · 11 comments
Labels
confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. docs This issue is due to a mistake or omission in the mongoosejs.com documentation

Comments

@jaredhanson
Copy link

If I define a Schema with a domain property, and then attempt to save a model instance, it fails with the following error:

events.js:84
        this.domain.enter();
                    ^
TypeError: Object github.com has no method 'enter'
    at model.EventEmitter.emit (events.js:84:21)
    at model.save (/Users/jaredhanson/Projects/website/node_modules/mongoose/lib/model.js:383:10)
    at model.module.exports.hook.proto.(anonymous function)._done (/Users/jaredhanson/Projects/website/node_modules/mongoose/node_modules/hooks/hooks.js:59:24)
    at module.exports.hook.proto.(anonymous function)._next (/Users/jaredhanson/Projects/website/node_modules/mongoose/node_modules/hooks/hooks.js:52:28)
    at fnWrapper (/Users/jaredhanson/Projects/website/node_modules/mongoose/node_modules/hooks/hooks.js:159:8)
    at model.module.exports (/Users/jaredhanson/Projects/node/mongoose-timestamps/lib/timestamps.js:22:5)
    at module.exports.hook.proto.(anonymous function)._next (/Users/jaredhanson/Projects/website/node_modules/mongoose/node_modules/hooks/hooks.js:50:30)
    at fnWrapper (/Users/jaredhanson/Projects/website/node_modules/mongoose/node_modules/hooks/hooks.js:159:8)
    at complete (/Users/jaredhanson/Projects/website/node_modules/mongoose/lib/document.js:920:5)
    at Document.validate.err (/Users/jaredhanson/Projects/website/node_modules/mongoose/lib/document.js:911:20)

In this case, "github.com" is the value I assigned to the domain property with a schema that looks like:

var AccountSchema = new Schema({
    userId      : { type: Schema.ObjectId, required: true }
  , domain      : String
  , uid         : String
  , username    : String
  , displayName : String
  , name: {
      familyName : String
    , givenName  : String
    , middleName : String
  }
  , credentials : [ CredentialSchema ]
});

Note, this is actually a conflict with Node core, which has a colliding domain property with anything that inherits from EventEmitter. I have a running discussion with @isaacs about it here:

nodejs/node-v0.x-archive#3922

It may help to get more input from Mongoose developers.

@isaacs
Copy link

isaacs commented Feb 10, 2013

Just curious, What happens if you do this?

var BorkSchema = new Schema({
    userId: { type: Schema.ObjectId, required: true }
  , domain: String
  , _events: String
  , emit: String
  , addListener: String
  , on: String
  , removeAllListeners: String
  , removeListener: String
  , once: String
  , _maxListeners: String
  , listeners: String
});

@jaredhanson
Copy link
Author

It would fail, of course. That's not debated.

Inheriting EventEmitter is, and will continue, happening, so expectations need to be made clear with regards to how to extend it while minimizing the possibility of future breakage. Clearly, everyone understands that overriding emit, on, etc will cause conflict. No one is recommending putting underscored _propertyNames in a Schema, so those are relatively safe from conflict as well (and checks could be put in place to enforce this).

What happened is that Node core introduced a domain property (not even underscored) to EventEmitter. If the policy is that any property can be introduced to EventEmitter by Node core then no properties of an inherited class can ever be considered safe (in any module, not just Mongoose).

Given how widespread inheriting from EventEmitter is, expectations should be set. If Node's policy is that EventEmitter should not be extended, that is fine. For my purposes, I'm unlikely to continue inheriting from Node's core EventEmitter and switch to a third-party implementation that will guarantee a more "strict" policy (basically, the interface that is already defined and nothing more, ever). I'd consider this a sorry state of affairs for Node core to be in, though.

@aheckmann
Copy link
Collaborator

this has come up before and Isaac makes a great case. we could either

  1. add "domain" and friends to the reserved names list
  2. write our own event emitter
  3. change documents so they are no longer event emitters

At this time, option 1 seems like the best choice. In 4x I want to revisit the document model and explore getting rid of methods like save etc and pushing that logic onto the Model where it belongs. Would also allow not inheriting from EE.

@isaacs
Copy link

isaacs commented Feb 11, 2013

Can you not just put Document keys on a different data object? Or do you depend on being able to do document.foo = 'bar' and also doing document.on('blah', blerg)?

@aheckmann
Copy link
Collaborator

Side note: prefixing ObjectId properties with an underscore is very common, similar to _id.

@aheckmann
Copy link
Collaborator

Hey Isaac, thanks for exploring this issue. Yes, we have getters/setters on the document itself which push data to the internal _doc property. The getters/setters can not be used as document properties if they conflict with our list of reserved names (which needs to be updated with domain). Documents being event emitters is an implementation detail and haven't been documented as such (though its likely folks depend on this now since its been around so long). This may change in the future.

We take security very seriously. If you have any more observations we'd love to hear more / discuss further.

aheckmann added a commit that referenced this issue Feb 11, 2013
throw friendly errors when `emit` or `_events` are used in schema.

closes #1338
aheckmann added a commit that referenced this issue Feb 15, 2013
throw friendly errors when `emit` or `_events` are used in schema.

#1338
@timlesallen
Copy link

This is still an issue: Any document with a domain property breaks things. We've stopped using mongoose for this reason.

@toddpi314
Copy link

nice

@xizhao
Copy link

xizhao commented Aug 21, 2014

Can we make a note of these in the docs as well?

@James1x0
Copy link

Boo.

@vkarpov15
Copy link
Collaborator

Not an issue in 4.0 anymore, since Document no longer inherits from EventEmitter to avoid this particular mess. See #1451.

As a side note, I really hope ES7 provides better control over encapsulation and domains go away before Node 1.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. docs This issue is due to a mistake or omission in the mongoosejs.com documentation
Projects
None yet
Development

No branches or pull requests

8 participants