Added full relations serialization in toJSON

[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]

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]

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]

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

[DouweM]

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]

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]

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]

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]

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]

@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]

Perfect!

[asgeo1]

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]

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]

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 😉, so if you could make the change, that'd be great!

Cheers,
Douwe

[philfreo]

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]

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

[DouweM]

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]

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]

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]

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]

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

[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]

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]

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]

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

[josx]

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

[glanchow]

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

[mtsr]

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?

[bbbouvier]

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?

[bbbouvier]

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

[bpatram]

Closing this in favor of the cleaned up PR #534