Standard DBRef support #525

Closed
wants to merge 2 commits into
from

Projects

None yet

6 participants

@nekaab
nekaab commented Sep 16, 2011

As mentioned in #489 and #491, Refs in mongoose are not standard DBRefs.

This pull request is not finished, unit tests have to be added and checked and one TODO is still on the list, but the solution has worked for me so far. Populating works as expected and the values written back to the DB now are standard DBRefs (i.e.:

    {
      "$ref": "fooCollection",
      "$id": ObjectId("4e5f7a73326a0206c9400000"),
      "$db": "barDB"
    }
@nekaab nekaab DBRef support updated, use standard DBRefs, order of ref, id and db m…
…ust be right, added db as a field (not supported yet), various fixes
bee0cd8
@aheckmann
Collaborator

looking real good man

@nekaab
nekaab commented Sep 21, 2011

Is there a way to ensure the order of values being written to the database? I need this for the DBRef implementation.

@aheckmann
Collaborator

array order is preserved when written. i take it you're seeing smth else?

@nekaab
nekaab commented Sep 21, 2011

I have an object, not an array. The DBRef is basically an object.

@aheckmann
Collaborator

object key order is not preserved in javascript at all.

@nekaab
nekaab commented Sep 21, 2011

Oh. true. Well how the hell am I supposed to keep the order intact for the DBRef then?

This is a mongodb issue, sorry to bother you. Will ask for advice there.

@nekaab nekaab closed this Sep 21, 2011
@nekaab nekaab reopened this Sep 21, 2011
@aheckmann
Collaborator

kinda surprised that order matters?

@nekaab
nekaab commented Sep 21, 2011

http://www.mongodb.org/display/DOCS/Database+References#DatabaseReferences-DBRef

"The ordering for DBRefs does matter, fields must be in the order specified above."

@aheckmann
Collaborator

probably post this to the mongodb-native driver list. i believe Christian has checks for this in place somewhere.

http://groups.google.com/group/node-mongodb-native

@aheckmann
Collaborator

@nekaab what is the state of one?

@aheckmann
Collaborator

err what is the state of this one?

@aheckmann aheckmann and 1 other commented on an outdated diff Oct 17, 2011
lib/schema/dbref.js
+ */
+
+DBRef.prototype.cast = function (value, scope, init) {
+ if (this.options
+ && this.options.ref
+ && init
+ && value && value._id && value._id instanceof oid) {
+ return value;
+ }
+
+ if (value === null)
+ return value;
+
+ if (value && value.oid instanceof oid && typeof value.namespace == 'string') {
+ return {
+ "$db" : mongoose.connection.name,
@aheckmann
aheckmann Oct 17, 2011 Collaborator

where is mongoose coming from? also, mongoose.connection is the default but what about if i create another connection using createConnection?

@nekaab
nekaab Oct 19, 2011

Thanks for your comments, I am not familiar enough with mongoose to notice those problems.

Can we maybe have a chat about how we could finalize this? Gonna @ you on twitter about that.

@Sija Sija commented on the diff Nov 14, 2011
lib/schema/index.js
@@ -19,4 +19,6 @@ exports.Date = require('./date');
exports.ObjectId = require('./objectid');
+exports.ObjectId = require('./dbref');
@Sija
Sija Nov 14, 2011 Contributor

Should be exports.DBRef i guess.

@nekaab
nekaab Jan 2, 2012

No, my implementation implements dbref and objectid still separately as an objectid is part of a dbref but can also be used individually (e.g. as _id)

@nekaab
nekaab Feb 6, 2012

I just read my comment again and can in no way reconstruct why I said that.

You are, of course, completely right. It should be exports.DBRef.

@Sija
Sija Feb 6, 2012 Contributor

Our thoughts are taking incomprehensible routes sometimes, perhaps it was some stale-cache-thought ;)

@nekaab
nekaab commented Jan 2, 2012

@aheckmann This is now "worksforme".

Unfortunately, I do not have a test environment with more thatn one database and there are no tests written yet. I don't know the framework well enough to see what's next.

How do we progress on this one?

@aheckmann
Collaborator

when i get a chance i'll take a look

@Almad
Almad commented Aug 8, 2012

+1 for this, I am now wrestling with mongoose-dbref package which is broken for 3.0.

@asabhaney

+1

What is the status of this? Is any help needed to move this forward?

Thanks!

@isstaif
isstaif commented Feb 6, 2013

Another +1 ! I'm wresting with mongoose-dbref too. Are there any updates about this pull request?

@aheckmann
Collaborator

we need to make sure it's compatible with 3.6x branch.

@aheckmann aheckmann closed this Mar 18, 2013
@Almad
Almad commented Mar 19, 2013

@aheckmann Is this closed because of fixed or wontfix? ,)

(Ran into this yesterday; with current implementation, one cannot have "or" in references)

@aheckmann
Collaborator

@Almad few have ever requested support for this and it looks like it could be implemented as a plugin.

@Almad
Almad commented Mar 19, 2013

While I disagree (IMO this needlessly breaks compatibility with other mongo tools), I respect this + we are probably going to collaborate on this.

@aheckmann
Collaborator

It's not any more non-compatible than storing functions, regexp, longs, etc. Rarely used things are best left out of core IMO.

@asabhaney

Fair enough. IMO the best reason for it to be in the core is because then you could natively use mongoose's populate function to populate a DBRef.

@aheckmann
Collaborator

Population is pretty flexible. We should be able to come up with something I think.

@aheckmann aheckmann referenced this pull request in mongodbinc-interns/mongoose Jun 3, 2013
Open

goals #1

9 of 11 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment