JSON-API correct handling of included entities

[wwwdata]

The implementation currently always loads related entities from the provided url. If there are ids inside a data object in a relation that match the ids of objects inside the included object, those already loaded objects must be taken instead of requesting them from the related url, because that is not necessary.

basic example

{
    "data": [
        {
            "type": "posts",
            "id": "1",
            "attributes": {
                "title": "JSON API paints my bikeshed!"
            },
            "relationships": {
                "author": {
                    "links": {
                        "self": "http://example.com/posts/1/relationships/author",
                        "related": "http://example.com/posts/1/author"
                    },
                    "data": {
                        "type": "people",
                        "id": "9"
                    }
                }
            },
            "links": {
                "self": "http://example.com/posts/1"
            }
        }
    ],
    "included": [
        {
            "type": "people",
            "id": "9",
            "attributes": {
                "first-name": "Dan",
                "last-name": "Gebhardt",
                "twitter": "dgeb"
            },
            "links": {
                "self": "http://example.com/people/9"
            }
        }
    ]
}

here, the author got already provided in the included object and does not need to be fetched.

[bmac]

Looks like we had a regression where the included links were not being loaded properly. I believe this is fixed by #3379.

@wwwdata do you mind trying http://builds.emberjs.com/canary/ember-data.js to see if the issue is fixed?

[wwwdata]

the error is still there. I wrote a small test: wwwdata@36a62a2

the equal(passedUrl.length, 1); fails because a second request was made. It seems that if the jsonapi adapter finds a related link, it always follows it.

[koemeet]

@bmac @wwwdata Is this caused by the following line? https://github.com/emberjs/data/blob/master/packages/ember-data/lib/system/relationships/state/has-many.js#L176

[koemeet]

Also this PR might be related to this issue: #3122

[wwwdata]

@steffenbrem yes, that is the line, when i always use the this.findRecords() my test works but the other tests break that rely on loading stuff from the related urls.

[wecc]

I've looked into this (including #3122) and even though it might seems trivial at first (let local data take precedence over links) I fear it's not.

We have the scenario when you have a link but no data and then fetch the data. So far so good. But if the link gets updated we want to invalidate the cache and refetch the next time we get the records. There's a test for this here. Having local data take precedence over links breaks this.

Another scenario is when you have a link and then create a new local record. This means we do have data but we still haven't fetched the link. In this scenario we want to fetch the link and merge the results with the newly created record and return them together. There's currently a test for this here. Having local data take precedence over links breaks this too.

With JSON-API I'm suspecting it will be more and more common having both links and data returned from backends and in many of those cases the data should take precedence over links. This is absolutely something we should solve, I just don't know how yet.

Edit: With "having data" I'm referring to the internal relationship.hasData boolean.

[koemeet]

@wecc My two cents on how to solve the issues you described above. For the first scenario, couldn't you just keep the dirty state of the link, so only take data presendence if the link is not "dirty". The link will be in a dirty state once it is different for the same relationship.

For the second scenario, I think you could easily track this by checking if the specific relation you are requesting has embedded data loaded, so some property should be set on every relationship that says if is has the data or not.

I don't know if this is realistic, since I am not an Ember Data expert. But I think it shouldn't be that hard to solve it. If needed, I could try to make a PR that implements my points.

[wwwdata]

@steffenbrem 👍 go for it please and make a PR. I think we should just check if there are some reference IDs in the data object on the same level as the link and if all of these IDs can be found in the included object, there is no more need to fetch data from the related link. I would expect from the server to return all the references in that case.

The link however always makes sense, because you might want to check at a later time if there are more referenced entities. For example if you loaded a blog post and just want to reload comments. Maybe ember-data at sometime needs another method to explicitly force loading of related entities via a related link for this special case. But as first step it would be awesome to just take stuff from the included object if possible 😄

[wecc]

@steffenbrem @wwwdata The problem is, as I see it, that we want different outcomes from two different scenarios that we're currently unable to tell apart:

1. link exist (relationship.link) + locally created record exist (relationship.hasData === true), we want to fetch the link
2. link exist (relationship.link) + we have data from server (relationship.hasData === true), we do not want to fetch the link

[koemeet]

@wecc So basically, we need to be able to know if the relationship has its data from the server or it is only local data. Something like that? I think that covers the issue we are having.

So lets say we introduce hasResolvedData on the relationship. It will keep track if the relationship has it's data resolved by the adapter (so data from the API is loaded). Obviously we cannot use hasData, since that also tracks local changes. So I think we need some other new property that tells us if there is data loaded from the server.

1. link exist (relationship.link) + no resolved data from server (relationship.hasResolvedData === false) + locally created record exist,
   we want to fetch the link
2. link exist (relationship.link) + resolved data from server already loaded (relationship.hasResolvedData === true) + locally created record exist,
   we do not want to fetch the link
3. link exist (relationship.link) + resolved data from server already loaded (relationship.hasResolvedData === true) + locally created record exist,
   we do not want to fetch the link

I can't think of another way to solve this properly. I am going to sleep now, but when I have time I will check if it is possible to keep track of data loaded from the server, so we can have such a property as hasResolvedData.

@igorT Do you have any ideas on this maybe?

[krzkrzkrz]

Just to summarise and to help newcomers getting into Ember Data and using the JSONAPIAdapter.

There's a bug where links.related currently takes precedence over data. The issue affects any model associations that contain the { async: true }.

The bug is apparent when the API response returns with data ids and there is already an included resource. What Ember Data does instead, is fetches the data from the links.related url. As suggested by the original poster.

The bug is also apparent in this scenario. coalesceFindRequests is set to true in app/adapters/application.js. The data response looks like:

{
  data: {
    "type": "articles",
    "id": "1",
    "attributes": {
      "title": "Rails is Omakase"
    },
    "relationships": {
      "comments": {
        "links": {
          "self": "http://example.com/articles/1/relationships/comments",
          "related": "http://example.com/articles/1/comments"
        },
        "data": [
          { "type": "comment", "id": "1" },
          { "type": "comment", "id": "2" },
          { "type": "comment", "id": "3" }
        ]
      }
    },
    "links": {
      "self": "http://example.com/articles/1"
    }
  }
}

Instead of Ember Data executing a coalesce (batch) query like:

http://localhost:4099/api/v1/comments?filter[id]=1,2,3

It follows the related.link url and does an n+1 for each comment.

Apply this workaround without having to remove the links.related urls from the API. When the bug is fixed, you just have to remove the extractRelationship function. The workaround removes any relationship.type.related links coming in from the API response.

I believe it is IMPORTANT that the workaround be applied to expect the desired behaviour from Ember Data, in any scenario.

Credit to @wecc. In the application serializer (app/pods/application/serializer.js):

import DS from 'ember-data';

// https://github.com/emberjs/data/issues/3380
// Theres a bug where links.related currently takes precedence over data
// Apply this workaround without having to remove the links.related urls from the API
export default DS.JSONAPISerializer.extend({
  extractRelationship: function(relationshipHash) {
    if (relationshipHash.data && relationshipHash.links && relationshipHash.links.related) {
      delete relationshipHash.links.related;
    }
    return this._super(relationshipHash);
  }
});

[koemeet]

@krzkrzkrz smart workaround. Thanks ;)

[jcope2013]

is there an equivalent method i can override on active model serializer to do something similar? should i file an issue on https://github.com/ember-data/active-model-adapter also?

[wecc]

@jcope2013 I think you should be able to do pretty much the same if you call this._super() first (that will give you a JSON-API relationship back that you can work with)

[jcope2013]

@wecc that doesn't seem to work, relationshipHash comes in to the method as a string of the modelName which seems odd and what it returns when calling this._super() is just an object that looks like this { id: 1, type: 'user' }

import DS from 'ember-data';

export default DS.ActiveModelSerializer.extend({
  isNewSerializerAPI: true,

  extractRelationship: function(relationshipHash) {
    var data = this._super.apply(this, arguments);
        console.log(data);
        return data;
  }
});

[wecc]

relationshipHash comes in to the method as a string of the modelName

yeah, that sound's really odd. can you create a separate issue with a JSBin demonstrating the issue? (preferably using your real payload)

[cdunn]

Would it make sense to also distinguish between data: [] and null, the former representing from the server that there are no related records vs the latter implying that the server doesn't know and should query?

[wwwdata]

any news on the team decision?

In order to really use ember-data with jsonapi 1.0 we need a good way to treat relationships and also make them optional if needed, so that they are requested only if needed from the specified relationship URL.

[greyhwndz]

Any update on the position of the team regarding this?

[smcclstocks]

bump

[xomaczar]

Seeing the same issue with [email protected], not even using relationship links at all (One-to-One).

//models/article.js
DS.Model.extend({
   title: attr('string'),
   comment: belongsTo('comment')
});

JSONAPI payload:

{
  "data": {
    "type": "articles",
    "id": "1",
    "attributes": {
      "title": "Rails is Omakase"
    },
    "relationships": {
      "comment": {
        "data": { "type": "comment", "id": "1" }
      }
    }
  },
 "included": [
      {
            "type": "comments",
            "id": "1",
            "attributes": {
                "author": "Dan",
                "title": "Gebhardt",
            }
        }
    ]
}

Route where the fetch occurs

//app/routes/foo.js
Ember.Route.extend({
  model() {
    return this.store.findRecord('article', 1);
  },

  afterModel(article) {
     return article.get('comment'); //  makes a seperate request /host/namespace/comments/:id instead of using "included". 
  }
}

[wecc]

@xomaczar I'm unable to reproduce this, see http://emberjs.jsbin.com/catohelode/edit?html,js,output

Would you be able to create a JSBin demonstrating the issue? Feel free to open a new issue since this one is old and very long.

[xomaczar]

@wecc here is the failing JSBin: http://emberjs.jsbin.com/cezowa/edit?html,js,console,output
Try adding/removing article.afterModel() hook.
I'll definitely open a new issue if it's an issue after all or my misunderstanding of stuff...

[xomaczar]

@wecc is this issue legit?

[wecc]

@xomaczar not a bug, more unfortunate circumstances :)

article:1 is already loaded when you enter the second route so the promise is resolved straight away using the article in store (without comments). Your template then tries to access comments and triggers a fetch. Meanwhile ED has background reloaded your article (see the new shouldBackgroundReloadRecord hook iirc) and the comments are side loaded and displayed correctly even though you get at "failed" comments fetch.

So, the behavior you're seeing is by design, but not very easy to spot.

Ping me in Slack of you want, should be an easy fix.

[scottkidder]

article:1 is already loaded when you enter the second route so the promise is resolved straight away using the article in store (without comments). Your template then tries to access comments and triggers a fetch. Meanwhile ED has background reloaded your article (see the new shouldBackgroundReloadRecord hook iirc) and the comments are side loaded and displayed correctly even though you get at "failed" comments fetch.

This is precisely what I am currently seeing. Right now I override shouldReloadRecord on the first model adapter to always return true, stopping the promise from being resolved straight away.

What is a better real solution here?