toJSON can discard relation if it has not been fetched with fetchRelated(relationName)

[dwt]

Given a toOne relation like this:

class Favorite extends Backbone.Model
  relations: [
    {
      key: 'favorized'
      keySource: 'favorized_id'
      keyDestination: 'favorized_id'
      includeInJSON: '_id'
      type: Backbone.HasOne
      relatedModel: 'Spine.Model'
    }
  ]


model = new Favorite({favorized_id: 'fnord'});
model.save()

reloadedModel = new Favorite()
reloadedModel.fetch()
// wait for it to fetch
expect(reloadedModel.get('favorized_id')).toBe('fnord') // fails

The problem seems to be the save() call. There the toJSON implementation of BackboneRelational expects the value (favorized in this case) to always be included in the superclasses return value for toJSON.

However it is null if it hasn't yet been fetchd with fetchRelated()

Thus the value for favorized_id is lost.

See: https://github.com/PaulUithol/Backbone-relational/blob/master/backbone-relational.js#L1326

[dwt]

Here's some tests and the workaround we tried to apply to this problem.

I'm quite uncomfortable marking this as a patch as I do not understand the reationale behind this behavior of Backbone.Relational - I only understand that it is messing our app.

Here's my understanding:

• Backbone.Relational does not present the contents of related fields to the app before they have been loaded.
• It does so by presenting empty Backbone.Collections for to many relations and null for to one relations

This makes sense to me and makes it easy for my app to load minimal data from the server for each context a model might get displayed in.

Here's where it gets weird though: On re-serialization time (after some aspect of a model has been edited/ changed) toJSON does not contain any of the original data that was 'hidden' by Backbone.Relational.

And this reeks like a bug to me, because I would assume that the contract of toJSON is to always return the same thing that you used to create the object from. It certainly does in Backbone, and Backbone.Relational certainly changes this.

So what gives? Is this a bug in Backbone.Relational or not?

Here's some unit tests I used to isolate the problem:

describe "Backbone Relational integration", ->

  describe "Backbone.Relational bugs", ->

    beforeEach ->
      class @Model extends Backbone.RelationalModel
        relations: [
          {
            key: 'parent',
            includeInJSON: '_id',
            type: Backbone.HasOne,
            relatedModel: @
          }
          {
            key: 'categories',
            includeInJSON: '_id',
            type: Backbone.HasMany,
            relatedModel: @,
            collectionType: 'Backbone.Collection'
          }
        ]


    it "should serialize related data of to one relations even if it is not loaded", ->
      @model = new @Model(_id: 'foo', parent: 'parent_id')
      @json = @model.toJSON()
      expect(@json.parent).toBe('parent_id')

    it "should serialize related data of to many relations even if it has not loaded", ->
      @model = new @Model(_id: 'foo', categories: ['first', 'second'])
      @json = @model.toJSON()
      expect(@json.categories).toEqual(['first', 'second'])

And here's the workaround I applied:

Backbone.RelationalModel::toJSON = ((original) ->
  ->
    json = original.apply(@, arguments)
    # loop over relations
    _(@_relations).each (relation) ->
      # add missing
      json[relation.key] = relation.keyContents
    json
)(Backbone.RelationalModel::toJSON)

What makes me uncomfortable mostly is that I am not sure what type of guarantees Backbone.Relational makes about keyContents and if it can just be used like this.

The actual bug seems to stem the toJSON implementation of Backbone.RelationalModel, where it does serialize collections, models and then just null, and is missing the case where it serializes the keyContents if it has not been loaded yet.

[dwt]

Oh, and a short/ quick reply would be greatly appreciated as I'm really unsure whether I want to apply this workaround or what other assumptions this breaks in Backbone.Relational

[philfreo]

@dwt looks like I'm describing the same issue over in #222

i'm not positive if your workaround will work properly in the cases where the relation/object is there. in those cases the id should show up already in the json, and you don't want to clobber it with keyContents if that's not correct.

my workaround (in toJSON) and yours solve some of the simple cases but definitely want to get @PaulUithol or @DouweM to help come up with a more comprehensive solution for this. would be nice to have access to the ID in both model.get('foo_id') and model.toJSON()

[dwt]

@philfreo: yes, would be very welcome to have the heavyweights weight in - I've since refined that workaround to this:

# Workaround for bug https://github.com/PaulUithol/Backbone-relational/issues/191
# Not feeling too good with this, as it's quite deep a change.
# However it does restore the property that json pushed into a model is the same that comes out of toJSON()
Backbone.RelationalModel::toJSON = ((original) ->
  ->
    json = original.apply(@, arguments)
    # loop over relations
    _(@_relations).each (relation) =>
      # add missing if not serialized
      superContent = json[relation.keySource]
      shouldFixupJSON = null == superContent or _(superContent).isEqual([])
      if shouldFixupJSON
        json[relation.keySource] = relation.keyContents
    json
)(Backbone.RelationalModel::toJSON)

Though it still dies when the last object of a to many relation has been removed in the app, because I haven't found the key to discerning that situation from a relation that has not yet been loaded. :-(

[DouweM]

Hi guys, you haven't been forgotten, I'll weigh in later tonight! (CEST)

[DouweM]

All right, I understand the problem you guys are running into, and I agree that we could handle this better.

How about the following setup:

• We'll consider a relation fetched if get(keySource) == null (i.e. the relation is actually empty) OR get(keySource) contains an object OR (get(keySource) contains an ID AND the relation was then fetch()ed) OR the relation was explicitly set to an object OR the relation was explicitly set to null.

• If the relation is fetched and get(key) == null, we'll include keyDestination: null in the JSON.

• If the relation is fetched and get(key) != null, we'll include keyDestination: includeInJSON in the JSON.

• If the relation is fetched and get(key) == undefined (i.e. the key was explicitly deleted), we won't include keyDestination in the JSON.

• If the relation isn't fetched and get(keySource) contains an ID AND includeInJSON == idAttribute (so id or _id, depending on config), we'll include keyDestination: get(keySource) in the JSON.

• If the relation isn't fetched and get(keySource) contains an ID AND includeInJSON != idAttribute, we won't include keyDestination in the JSON.

• If the relation isn't fetched and get(keySource) == undefined (i.e. it wasn't set by the server), we won't include keyDestination in the JSON.

I think this would cover all cases, such as still allowing us to destroy a relation with set(key, null). For has-many relations you can just substitute "null or [] or an empty Backbone.Collection" for "null", and it should still make sense.

I also agree it would be nice to keep get(keySource) in sync with get(key).id (if keySource != key of course), but it won't be included in the JSON as such, that behavior is laid out above.

What do you think?

/cc @PaulUithol

[philfreo]

From a quick read it sounds great to me. All the tests mentioned in #222 should then pass, right?

[DouweM]

@philfreo Yessir, those tests were very helpful in getting the desired behavior clear.

How about you, @dwt, would these changes solve your problems?

[dwt]

Thinking about this I'm pretty confident that this works for ToOne relations. However for ToMany relations I'm unsure if this works if you remove the last element of a ToMany relation.

Here's my understanding: If a ToMany relation is serialized to an empty array (with the current code) it is either not fetched or the last element of the relation has been removed. (This is derived from my problem definition, which is "Discern when a relation was fetched and when it was unfetched, so that the original key-content can be restored in toJSON on unfetched relations")

Under the rules laid out above, I'm not sure those could actually discern between those two situations?

Here's the problem table I used to think about this:

I see the following problem with ToOne relations, if the are null they can be either

• unfetched, in which case keySource holds the correct information
• or explicitly removed, in which case I have no idea where this should be tracked

The rules you mentioned above seem to cover this.

For ToMany relations I see a problem when they are empty, which means they are either

• not fetched. In this case keySource holds the correct information
• or the last element was removed - in which case keySource is wrong as it still contains the content of the original json

How is that covered from the rules you outlined above?

Additionally I would find it very helpful if the meta information if a relation was already fetched or not was exposed in the relation api somewhere. I'm thinking about an API that I can use to discern the three states involved here, i.e. was never fetched, fetch has started and fetch was completed. If I can bind events to this information that would allow me to give rich feedback to the user as to the status of the data fetch. Could be that this is actually something that needs to happen in Backbone itself, but at least Backbone.Relational should ensure that this information can be gathered in the meantime and is equaly availeable from to many relations and the objects contained therein.

[DouweM]

The rules outlined above specify a difference between "unfetched-null" and "explicitly removed-null", as well as "unfetched-[]" and "explicitly emptied-[]". Keeping that in mind, I think the rules work. How we're actually going to accomplish detecting that difference is for us to worry about, I'm just trying to outline how it should work. As for implementation we have it a little easier than you did because we don't have to monkey patch toJSON and we can add extra flags in Backbone.Relational etc.

That meta information about a relation being fetched is exactly that implementation I mention, I'll see if a public API for that is viable. Ideally, something like that would be part of Backbone itself, but for now we'll have to implement this ourselves.

[dwt]

Very nice!

Oh, and when can we have that? :-D

[DouweM]

Haha, you're excited like a little boy before Christmas; let's make it a Christmas then. I'll have a crack at it in the weekend.

[dwt]

That would be amazing! :-D

[philfreo]

@DouweM how's it coming? :)

[DouweM]

Not well I'm afraid; looks like I was too optimistic as to my free time: haven't had time to look into it yet :(

[DouweM]

Hahaha, I've got a GitHub email in my inbox saying there's a $100 bounty ;) But whatever the price, Monday is not going to work for me, seeing as I'm on holiday right now, due to be back on Monday. Anyone else who likes the sound of $50 is encouraged to have a crack at it though! Unfortunately, with work and school I'll have little time in the next couple of weeks.

[philfreo]

@DouweM or @PaulUithol just a friendly reminder that we'd love to see happen if you've got time! I've got some bugs related to when foo vs. foo_id is populated and I think this might solve them.

[PaulUithol]

0f7a719 should make implementing this a lot easier. Looks like relation.keyId(s) corresponds to the "missing" models of a relation that should be added back in on toJSON.

[PaulUithol]

Just to be clear, this issue is just about the toJSON, and how it treats "unknown" models, meaning those that did not appear in the store at any time during it's lifetime. For that purpose, it doesn't matter if this didn't happen because they were supposed to be fetched or explicitly created.