validate and save not returning the same errors in 2.x

[wavded]

I'm getting no errors when I call validate, but when I call save I get this error:

{
    stack: "Error: key test@test.com must not contain '.'\n    at Error (unknown source)\n    at Function.checkKey (/home/wavded/Projects/arc/node_modules/mongoose/node_modules/mongodb/lib/mongodb/bson/bson.js:1797:11)\n    at Function.serializeWithBufferAndIndex (/home/wavded/Projects/arc/node_modules/mongoose/node_modules/mongodb/lib/mongodb/bson/bson.js:585:16)\n    at [object Object].toBinary (/home/wavded/Projects/arc/node_modules/mongoose/node_modules/mongodb/lib/mongodb/commands/insert_command.js:86:55)\n    at [object Object].send (/home/wavded/Projects/arc/node_modules/mongoose/node_modules/mongodb/lib/mongodb/connection.js:290:37)\n    at [object Object].executeCommand (/home/wavded/Projects/arc/node_modules/mongoose/node_modules/mongodb/lib/mongodb/db.js:664:18)\n    at Collection.insertAll (/home/wavded/Projects/arc/node_modules/mongoose/node_modules/mongodb/lib/mongodb/collection.js:289:13)\n    at Collection.insert (/home/wavded/Projects/arc/node_modules/mongoose/node_modules/mongodb/lib/mongodb/collection.js:110:8)\n    at Array.2 (/home/wavded/Projects/arc/node_modules/mongoose/lib/drivers/node-mongodb-native/collection.js:65:25)\n    at EventEmitter._tickCallback (node.js:126:26)",
    arguments: undefined,
    type: undefined,
    message: "key test@test.com must not contain '.'"
}

Mongoose object looked like this:

{ userId: { test: undefined },
  password: { test: undefined },
  _id: 4e5bac9480416cb13d000005,
  hasPhoto: false,
  contacts: [],
  email: 
   { primary: { 'test@test.com': undefined },
     secondary: [] },
  roles: [] }

Mongoose 1.x worked fine in this case. I noticed values in 2.x have this undefined after them. I can get you more details if you need. I have save middleware disabled.

[wavded]

found out what the issue was, this and sugarjs are conflicting somehow

[aheckmann]

fyi mongodb rejects object keys with periods in them

insert({ 'my@email.address': 1 }) // Error!

[wavded]

gotcha, does mongoose extend native prototypes? seems like something was being triggered that was on the prototype for http://sugarjs.com, I opened an issue over there:

https://github.com/andrewplummer/Sugar/issues/22

[aheckmann]

@wavded no, we just subclass Array, Number, and Buffer.

[ricardobeat]

test case at https://github.com/andrewplummer/Sugar/issues/23

Mongoose's utils.js seems to be the culprit.

in the clone() function:

if (obj.toObject)
  return obj.toObject(options);

Sugar (safely) extends strings' prototypes with .toObject(). Why is mongoose doing this crude detection?

If that code is trying to detect Document instances, wouldn't checking for _doc or _schema more appropriate?

if (typeof obj._schema !== 'undefined')
    return obj.toObject(options)

[aheckmann]

do not extend prototypes. closing

[andrewplummer]

@aheckmann I understand that a lot of people are not comfortable with extending native prototypes. That may be your policy, and I respect that. But it is Sugar's policy to extend native prototypes, and to do it in a way that is as safe as possible. The fact is that people are going to do play with these libs together whether we like it or not.

Sugar in this case is extending Strings with a toObject method. Now as it so happens I don't particularly care for this method in the first place and duck typing issues such as these may be a good argument to remove it, which I will consider.

However, @ricardobeat has a point here that the above clone function is making a very big assumption that every object that passes through it and implements a toObject method will behave in exactly the way that Mongoose defines. Essentially this is the reverse of extending native prototypes. Instead of creating a new method on natives that people may not be expecting, the code is assuming that all natives that pass through it will NOT be implementing a certain method. The effect is the same.

Or let's put it another way. If the Javascript spec were to suddenly change and implement a toObject method on strings, this code would break just as quickly. I'm not expecting you to "endorse" extending native prototypes, but a quick change on your part would not only make our libs play nicely together but would also make Mongoose that much more robust...

[wavded]

I have to second @andrewplummer's comments on this one.

I think something like Sugar is going to be pretty common place for node as it just makes sense and makes things easier to work with and easier to read IMO, not perfect but I think of Rails and ActiveSupport and how helpful those methods can be. Of course we don't want everyone going around tinkering with natives but if its done in secure way which Sugar seems to do, why not?

[aheckmann]

If it was done in a "secure" way this ticket would never have been opened. I'm all for interoperability with other libs, we're just not gonna endorse bad practice to do so.

[andrewplummer]

I suggest having a look at the problem again because that simply isn't the case here. This ticket is a good example of how nothing is absolute. Assumptions about natives can be just as destructive and problematic as extending them, and that is what's happening here.

I will be renaming the toObject method in Sugar. This issue was a factor but actually I had been planning to rename it before this came up. It will effectively fix the issue, but I ask that you reconsider your stance. The above code is clearly making some very big assumptions, and as I said if this comes up again it will break again.

[wavded]

I guess bad practice may be a matter of opinion (similar i guess to semicolons), i agree that native extensions have been abused and have been a source of many errors, but i don't think they are untouchable... My line of thinking is similar to this dude: http://blip.tv/jsconf/jsconf2011-andrew-dupont-everything-is-permitted-extending-built-ins-5211542, either way, a good discussion, doesn't make or break my dev at the moment, cheerio!

[ricardobeat]

@aheckmann sugar safely extends objects using defineProperty with enumerable set to false. This is not the old case of extending prototypes and breaking for in loops.

That any object with a toObject method is an instance of a document is a wrongful assumption, just like assuming that if !window.XMLHttpRequest means the environment is IE6.

Pull request at #485

[andrewplummer]

The issue is effectively resolved with Sugar v0.9.5, which renames the method. @aheckmann I still highly recommend that this code gets fixed, but it's up to you.