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

toJSON implementation following Mongoose DBRef pattern, and unit tests #123

Merged
merged 1 commit into from
Sep 12, 2016
Merged

toJSON implementation following Mongoose DBRef pattern, and unit tests #123

merged 1 commit into from
Sep 12, 2016

Conversation

moxious
Copy link
Contributor

@moxious moxious commented Aug 24, 2016

See this issue for a full discussion: #110

@brett19
Copy link
Contributor

brett19 commented Aug 29, 2016

Hey @moxious ,
What is the difference between the DBRef implementation you've done here, and the reference format we already use within Ottoman?
Cheers, Brett

@moxious
Copy link
Contributor Author

moxious commented Aug 29, 2016

DBRef format looks like this:

{ 
  "$ref" : "MyModel", 
  "$id" : "someuuid", 
}

Internal ottoman representation looks like this:

{
   "$ref": "some-uuid",
   "_type": "MyModel"
}

So they're quite similar. In previous discussion with you though, you had indicated that it's undesirable to expose these ottoman internals. There's also the _type issue discussed a lot lately, which makes exposing _type definitely an unsafe bet.

Note that without this change, the ottoman internal format doesn't get exposed properly at all because it's an instance of a function and not an actual JSON object in memory.

See issue #110 linked for more details, and code examples of why this approach.

@brett19
Copy link
Contributor

brett19 commented Aug 29, 2016

This looks good other than that I believe it will break backwards compatibility. I'm going to have to think about the consequences of that before we merge.

@moxious
Copy link
Contributor Author

moxious commented Aug 29, 2016

I don't think this will break backwards compatibility -- or at least anyone in the past who depend ended on the toJSON function was making a mistake. There were no unit tests for this function before, and in its previous implementation -- as I pointed out it returned a complex object with functions, not JSON.

Since toJSON before just returned the object, if people wrote code to this then they were already coupling to all sorts of internals that they shouldn't have, e.g. the $ on the model would have been defined, since it was only returning this. Variables like that are not part of any reasonable JSON serialization of a model.

By all means think about it and discuss, but even if this implementation isn't right, still something substantiatively different than what's there now is needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants