Fix compatibility with datejs #646

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
Contributor

cleishm commented Dec 13, 2011

Fixes compatibility with datejs (or any other library that adds a toObject method to the Date prototype).

The fix ensures that mongoose ignores toObject methods for the standard types "Date", "Function" and "Regex" (these were currently checked for after looking for a toObject method).

This fix addresses issue #502.

Collaborator

aheckmann commented Dec 13, 2011

the only problem with this is that it still breaks when someone does Object.prototype.toObject = function (){} but its better than nothing.

I'll test / merge later today. thanks!

Contributor

cleishm commented Dec 13, 2011

I believe it will also cover that case. If the object is an instance of
Date, Function or Regex then it won't even look for, or call, the toObject
method.

I suppose the other primitive types could also be checked for, but that's
probably getting silly. People shouldn't be monkey-patching these things
anyway ;)

Question: you've done the type check by looking at the constructor name
(rather than instanceof) in clone, yet used instanceof in deepEqual. Any
particular reason? I just followed the same pattern.

chris

On Tuesday, December 13, 2011, Aaron Heckmann <
reply@reply.github.com>
wrote:

the only problem with this is that it still breaks when someone does
Object.prototype.toObject = function (){} but its better than nothing.

I'll test / merge later today. thanks!


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

@@ -4719,6 +4732,7 @@ module.exports = {
m.save(function (err) {
should.strictEqual(err, null);
M.remove(function (err) {
+ delete Date.prototype.toObject;
@aheckmann

aheckmann Dec 13, 2011

Collaborator

we'll need a more fine grained test so other tests are not impacted but thats ok, i'll fix it up later

Collaborator

aheckmann commented Dec 13, 2011

re instanceof, its a mistake.

Collaborator

aheckmann commented Dec 14, 2011

tests fail

uncaught: AssertionError: expected 0 to equal 4
    at Object.equal (/home/aaron/test/mongoose/node_modules/should/lib/should.js:306:10)
    at Promise.<anonymous> (/home/aaron/test/mongoose/test/types.array.test.js:359:34)
Contributor

cleishm commented Dec 14, 2011

Platform? I can't reproduce. I'm using master with only this pull request applied, and I've tried with both node 0.4.7 and node 0.6.5.

NPM details:

% npm ls
mongoose@2.4.2 /Users/cleishma/work/mongoose
├── colors@0.5.1
├── gleak@0.2.1
├── hooks@0.1.9
├── mongodb@0.9.7-1.4
└── should@0.2.1

Collaborator

aheckmann commented Dec 14, 2011

i'm on node 0.4.12. the test by itself passes but it causes the other test to fail when running the full suite.
Ubuntu 11.10

Contributor

cleishm commented Dec 14, 2011

Strange. Full suite (505 tests) passes for me.

Contributor

cleishm commented Dec 14, 2011

I'm on OS X (10.7)

Contributor

cleishm commented Dec 17, 2011

Ok - I started up a virtual box and debugged this. There were a few other places in the code that assume a "toObject" method means that it's an embedded doc. Updating the pull request with a fix.

Contributor

cleishm commented Dec 17, 2011

PS. I'm starting to wonder if, perhaps, checking for toObject should be changed to checking for the types of Document or MongooseArray (rather than adding an additional check that it's not a primitive type, as this patch does).

Collaborator

aheckmann commented Dec 20, 2011

merged #655 instead

@aheckmann aheckmann closed this Dec 20, 2011

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