Skip to content

Added full relations serialization in toJSON #183

Closed
wants to merge 9 commits into from

10 participants

@buzzeante

Hello there,

There are some situations when we need to make toJSON work in
different ways:

  • Communication with server where relations may be reduced to resource_uri
  • Template rendering where we need more information that the one we send to the server

Adding {full: true} to toJSON allows to avoid includeInJSON params and make a full
serialization of a model and its relations.

Related: GH-161.

Cheers,
Javi

@DouweM
Collaborator
DouweM commented Aug 15, 2012

I'm not opposed to this feature per se, but I don't see why you wouldn't just pass the model object itself to the template. As I understand and use it, #toJSON() is really meant to be used with server requests. The keyDestination option that is respected by the method really only makes sense in a server/API context, for example.

@buzzeante

I don't think there is a way of passing a model with all its relations expanded to a template. That's what I am trying to cover here.

You are right about the way toJSON is primarily meant to be used, but that does not mean it can fulfill other use cases. It may need some additional work to solve possible problems with keyDestination.

Cheers,
Javi

@PaulUithol
Owner

What do you mean by "expanded"? Without the need to use the .get notation?

Another note: at the very least, a feature like this would need a big fat very bad for performance; do not, ever, use this in a loop warning, as evidenced by #18.

@buzzeante

I mean that if I have a complex relational model with some relation defined with includeInJSON: false and need it for the template, I believe there is no way I can pass the structure to the template and render it properly.

You are right @PaulUithol , there should be some warning to indicate a proper way of using it. In our case, we use it only occasionaly and never in loops avoiding performance isssues.

Cheers,
Javi

@DouweM
Collaborator
DouweM commented Aug 15, 2012

What templating system are you using? In most I am aware of, you can just pass the Backbone.RelationalModel object and use it like obj.get("attribute") inside the template. Why do you need to pass a raw JS object (dictionary), as returned by toJSON?

@buzzeante

We are using the template system Mustache. Since it is logic-less, I believe we cannot access to javascript functions from the template. Hence, toJSON is an easy way to pass expanded data to the template.

Cheers,
Javi

@buzzeante buzzeante Added full relations serialization in toJSON
There are some situations when we need to make toJSON work in
different ways:
- Communication with server where relations may be reduced to resource_uri
- Template rendering where we need more information that the one we send to the server

Adding `{full: true}` to toJSON allows to avoid `includeInJSON` params and make a full
serialization of a model and its relations.
5e2649a
@DouweM
Collaborator
DouweM commented Aug 17, 2012

All right, that makes sense. I'm not sure adding an option to toJSON is the way to go here though, seeing how toJSON handles stuff like keyDestination. It seems that what you're looking for is just the model's and its children's attributes, recursively, without any server-specific idiosyncrasies like keyDestination. So why not add a method recursiveAttributes() that would do just that?

@buzzeante

That is another perspective we have considered but we decided to use toJSON since its logic is almost the same and also to be able to use it transparently with models that are not RelationalModel.

Cheers,
Javi

@DouweM
Collaborator
DouweM commented Aug 20, 2012

All right, "to be able to use it transparently with models that are not RelationalModel" is a very good argument in favor of doing this through toJSON. Or at least: for allowing this to be done through toJSON.

The imaginary recursiveAttributes and toJSON differ insofar that the former just returns the attributes property with all Backbone.Model objects replaced by their attributes, i.e. a raw object representation of the model, while the latter includes a lot of logic respecting options like _subModelTypeAttribute, includeInJSON and keyDestination that are really only useful when used with a server, and aren't needed when you just want to pass the data to a template, as you do.

Because of this, I think it's best to add the recursiveAttributes method to do what I just described and have toJSON delegate to recursiveAttributes if (options.full).

What do you think of this solution? The biggest (and really the only) issue I have with your implementation is the fact that it still respects keyDestination when options.full == true, which you probably don't want when you're looking for the raw object representation of your model.

Would you be up for making these changes?

@buzzeante

I see your point, but I don't know if an additional method recursiveAttributesis needed. what use cases do you think it would be useful for?

I agree that respecting keyDestination when options.full === true can be a big issue given the use case I mentioned. So, wouldn't it be easier to modify toJSON to support such requirement?

I guess this is more a matter of opinion, so I am open to make the changes in whichever direction you think fits best in the project.

Cheers,
Javi

@DouweM
Collaborator
DouweM commented Aug 22, 2012

recursiveAttributes would be useful for exactly what you're trying to do. The only reason I want to extract it from toJSON is that it doesn't have anything to do with the server-related serialization that toJSON is meant for and has special logic for (keyDestination).

It is definitely a matter of opinion, so here I am paging @PaulUithol to give his opinion, since this is his project first and foremost. Moreover I'll be gone from the internet for the next 6 days, so he'll probably be the one to merge this or some adaptation of this.

Cheers!

@buzzeante

Sorry for the delay! I was caught up in side tasks. As I said before I am up to make the changes needed in the direction you guys believe is the way to go.

So let me know how can I help.

Cheers!

@PaulUithol
Owner

I don't really have a preference here.. I've been trying to decide, but I can't ;).

I'd like to avoid introducing additional methods if possible, but in this case it would be sort of inconsistent for full=true to have additional side effects of changing the output to ignore keyDestination. So maybe a separate one is better here indeed..

@DouweM
Collaborator
DouweM commented Sep 4, 2012

Yeah, I am hesitant to introduce new methods if not absolute necessary, but it seems to me that this is a case where it would be appropriate. @buzzeante, are you up to make the changes?

@DouweM
Collaborator
DouweM commented Nov 3, 2012

@buzzeante Are you still planning to look into this?

@buzzeante

Hello @DouweM . I am really sorry for this disappearance. These couple months have been really crazy.

I plan on doing it this week. I will let you know as soon as I can tackle it :)

Cheers,
Javi

@DouweM
Collaborator
DouweM commented Nov 13, 2012

Perfect!

@asgeo1
asgeo1 commented Nov 21, 2012

I've been having a similar issue.

That's why I added the pull request for adding the options param to the overridden toJSON function (just like the underlying Backbone toJSON has). #216

At least with that in place I can come up with my own solution until you guys work it out.

FWIW, here's my current workaround:

class BaseModel extends Backbone.RelationalModel
  toJSON: (options = {}) ->
    if options.includeRelations? or options.excludeRelations?
      pre = {}

      for relation in @_relations
        pre[relation.options.key] = relation.options.includeInJSON

        if options.includeRelations? and relation.options.key in options.includeRelations
          relation.options.includeInJSON = true

        if options.excludeRelations? and relation.options.key in options.excludeRelations
          relation.options.includeInJSON = false

    json = super options

    if options.includeRelations? or options.excludeRelations?
      if _.keys(pre).length > 0
        for relation in @_relations
          if pre[relation.options.key]?
            relation.options.includeInJSON = pre[relation.options.key]

    return json

It lets me use the default configuration of the model relations as what will get set back to the server when toJSON is called without any options.

But at anytime I can call @model.toJSON includeRelations:['customers', 'forms'] if I want to include (or exclude) one or more relations.

It works OK for what I'm trying to do anyway.

From what I understand reading this thread, you're mainly considering adding a 'full' option to return full nested model serialization.

But like I show above, it would be very useful to have options for including or excluding whatever related models we like when calling toJSON.

Also not keen on the proposed recursiveAttributes method. Backbone already has a toJSON method which people are familiar with and want to use whenever they need a JSON representation of a model. It just needs more options for working with backbone-relational and nested models :) Presumably, that's why there's an options param in the underlying Backbone.js library, even though it currently doesn't do anything.

@buzzeante buzzeante Added support to avoid `keyDestination` if `full=true`
toJSON method will avoid relational configuration `includeInJSON` and `keyDestination`
in case option `full=true` is included when calling the method.
4ebc7a6
@buzzeante

Hello @DouweM,

First of all, I am really sorry for this huge delay. At last I could take some time to continue with what I started.

Taking into consideration all we talked about some time ago I just made some small changes in order to ignore keyDestination when full=true so that it works in the way we considered.

I started doing a function recursiveAttributes but it was mainly copying and simplifying what toJSON did so it didn't make much sense in my opinion. This way it is just a couple more additions to the method. I couldn't think of a use case where the recursiveAttributes approach was really needed; but again, if you feel it is the way to go I can do a workaround (I promise in less time this time ;))

I also included a test to check it works properly.

Cheers!
Javi

@DouweM
Collaborator
DouweM commented Dec 20, 2012

Hi @buzzeante, hi Javi,

Well, I guess I'll have to apologize for a sizable delay as well! Relational has had to take a back seat for the last couple of weeks, unfortunately.

I'm still very much in favor of recursiveAttributes(), please read my comments above for my explanation :wink:, so if you could make the change, that'd be great!

Cheers,
Douwe

@philfreo
Collaborator

So just to be clear, the proposal is to have model.recursiveAttributes() which would allow the following (if it was passed as the template context)

For a "car" template, you should be able to say {{color}} as well as {{owner.name}}, rather than {{owner.attributes.name}}

@DouweM
Collaborator
DouweM commented Dec 21, 2012

Yup, model.recursiveAttributes() should really just return attrs = model.attributes with attrs[relation.key] = relation.related.recursiveAttributes() for each relation.

@DouweM
Collaborator
DouweM commented Dec 21, 2012

So: model.recursiveAttributes() is what you'll be passing to your templates, instead of model.toJSON() which, with its support for keyDestination and includeInJSON, really isn't fit for that purpose.

@buzzeante

Hey @DouweM, no problem, these dates are also pretty busy by default ;)

Ok, I am on it and hope to have it soon enough. However I have to insist on the option full=true which I believe is necessary to allow a transparent integration with Backbone models. As I said previously, at a template level you don't necessary need to know if a model is Relational.

In order to use recursiveAttributes on templates you need a previous check whether it is a relational model or not; which at the same time makes it unnecessary complex.

Cheers,
Javi

@DouweM
Collaborator
DouweM commented Dec 21, 2012

As mentioned 4 months ago:

Because of this, I think it's best to add the recursiveAttributes method to do what I just described and have toJSON delegate to recursiveAttributes if (options.full).

With this, toJSON({ full: true }) would work for both relational and non-relational models. My issue was with the logic for recursiveAttributes being part of the toJSON method.

@buzzeante buzzeante New recursiveAttributes function
It encapsulates toJSON(full=true) functionality where
keyDestination is omitted
b8438b5
@buzzeante

Hey @DouweM I finally got the time to include recursiveAttribute method. Let me know your thoughts on it in case there is something more that can be made :)

Cheers,
Javi

@DouweM
Collaborator
DouweM commented Jan 9, 2013

What's the deal with all of the changes in #toJSON?

@buzzeante

It's an identation fix. Sorry, didn't realise it was going to screw the diff in such way.

Cheers

@DouweM
Collaborator
DouweM commented Jan 9, 2013

Ah all right. Changes look good to me. Could you rebase it on top of master so I can easily merge it?

buzzeante added some commits Aug 15, 2012
@buzzeante buzzeante Added full relations serialization in toJSON
There are some situations when we need to make toJSON work in
different ways:
- Communication with server where relations may be reduced to resource_uri
- Template rendering where we need more information that the one we send to the server

Adding `{full: true}` to toJSON allows to avoid `includeInJSON` params and make a full
serialization of a model and its relations.
b23ef3a
@buzzeante buzzeante Added support to avoid `keyDestination` if `full=true`
toJSON method will avoid relational configuration `includeInJSON` and `keyDestination`
in case option `full=true` is included when calling the method.
ad1842e
@buzzeante buzzeante New recursiveAttributes function
It encapsulates toJSON(full=true) functionality where
keyDestination is omitted
1b6e1ec
@buzzeante buzzeante Adapting recursiveAttributes to changes in toJSON dd17d02
@buzzeante buzzeante Merge branch 'feature/full-toJSON' of github.com:TwoApart/Backbone-re…
…lational into feature/full-toJSON

Conflicts:
	backbone-relational.js
8b686e5
@buzzeante

There you are! It's the first time I do a rebase so it may be a bit of a mess I'm afraid.

@DouweM
Collaborator
DouweM commented Jan 10, 2013

Haha. Something definitely went wrong, there's 10 commits in the pull request now. No problem though, I'll clean it up manually before merging.

@buzzeante

Thanks @DouweM!

@buzzeante

Hello @DoweM,

I just realized there is a huge bug in the Pull Request when there is a HasMany relationship since Backbone.Collection does not have an implementation of recursiveAttributes.

I can see 2 solutions for it: Go back to previous implementation or extend Backbone.Collection with a recursiveAttributes derived from its implementation of toJSON.

What do you think?

Cheers,
Javi

@DouweM
Collaborator
DouweM commented Feb 6, 2013

Hi @buzzeante,

Ah damn, you're right. I think there's a third solution though: add a simple if/else statement to check if value instanceof Backbone.Collection in which case we loop over value and call .recursiveAttributes on each model. So pretty much what happens in toJSON already: https://github.com/PaulUithol/Backbone-relational/blob/master/backbone-relational.js#L1405

I'm sorry by the way for the month of no response, I'm so busy these days it probably isn't healthy.

Cheers,
Douwe

@buzzeante

No worries @DouweM :)

I already solved it in my branch with your approach. I will write some test so that it checks it runs and push it.

Cheers,
Javi

@buzzeante

Hi yet again. Added the tests, and the bug fix for the issue we mentioned. I am really sorry for such delays, I am stuffed up with work too :S

@DouweM
Collaborator
DouweM commented Feb 25, 2013

Looks good. Now it's up to me to consolidate the commits.

@josx
josx commented Jun 5, 2013

Please rebase against master, because you have worked on 0.7 version

@glanchow
glanchow commented Sep 2, 2013

Ping @buzzeante, nice work, I'd like to see it merged.

@mtsr
mtsr commented Nov 26, 2013

This is a very useful change for those of us who are used to using toJSON for serialization of models for use in rendering templates. Is there any particular reason why this wasn't merged?

@rodneyrd

Thanks a lot for this solution. Needed it a model to its previous to previous state with nested collections. @mtsr Up. Would it be possible to fix the conflicts and merge it?

@rodneyrd

I allowed myself to duplicate the merge request. So there is no longer merge conflicts.
See: #534

@bpatram bpatram added the duplicate label Mar 28, 2016
@bpatram
Collaborator
bpatram commented Mar 28, 2016

Closing this in favor of the cleaned up PR #534

@bpatram bpatram closed this Mar 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.