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

Ember Data is using the links in relationships, not using data in relationships #5574

Closed
fengshuo opened this issue Aug 17, 2018 · 11 comments
Closed

Comments

@fengshuo
Copy link

fengshuo commented Aug 17, 2018

Hi,

I have an jsonapi response (/contents) with this format:

attributes: {},
id: 'XXX',
relationships: {
  provider: {
    data: { type: "providers", id: "99"},
    links: { self: "linkA", related: "linkB"}
  }
}

one provider may have many contents, the models has the hasMany and belongsTo relationship setup. The issue is that when requesting providers information, ember data is not using the data inside relationships, but using the links in relationships.

I read in several places that ember data will use the data inside relationship if it exists, such as here and here, I wonder if ember data 3.3 is still working like this?

Thanks.

@runspired
Copy link
Contributor

@fengshuo if you give us a link, we will always use the link when fetching.

I read in several places that ember data will use the data inside relationship if it exists

This is still true; however, this probably doesn't mean what you think. It refers to when you have previously loaded a record with that type and id, which includes sideloading the resource via included: [] in the same API response.

@mydea
Copy link

mydea commented Oct 3, 2018

I do think this behavior has changed after 3.1.

In our app, with ED 3.1, for a hasMany relationship with included IDs and a link, it would load the relationship via the IDs initially. Later, if I'd call e.g. model.hasMany('myRelationship').reload(), it would use the link to re-fetch.

For us, this is a very useful functionality, as we can reduce the number of requests initially, while still being able to later reload the relationship.

As far as I see, this is not really possible (easily) anymore in 3.4. Now, it will always use the link to fetch the relationship - which works, but leads to many more API requests.

A concrete example:

On a page, we load the hasMany relationship for a model, which returns n items.
Now, each of these items in turn can have X subItems, which we also load:

let items = await get(model, 'items');
await RSVP.all(items.mapBy('subItems'));

Previously, this would result in 2 API requests:

  • /api/models/XX/items to fetch the first relationship - here, we include the IDs of the subitems as well as the link
  • /api/subItems?ids=1,2,3,4,5 then it would concatenate all the IDs from the subitems and get them in one big request.

After upgrading to ED 3.4, it results in 1 + n requests, where n is the number of items - as it will fetch the included link, one by one.

Right now, the only way forward I see is either a) living with a huge amount of API requests or b) manually removing the link information in the extractRelationship method of my serializer, which means I loose the ability to reload the relationship.

For me, the old behavior made more sense:

  • if IDs are included and the relationship was never loaded, use it
  • if no IDs are included, always use the link
  • if IDs and a link are included and the relationship was already loaded once, use the link

(Maybe I'm missing something, but this has changed the behavior of our app in subtle ways that were hard to track down and lead to unexpected performance regressions)

@runspired
Copy link
Contributor

@mydea I suspect either you aren't normalizing your data correctly or the relationships in your payloads are less defined than they should be. Here is the behavior you should observe:

1.) No requests Made: (data and links and included)

{
  data: {
    type: 'foo',
    id: '1',
    relationships: {
      bars: {
        data: [{ type: 'bar', id: '1' }, { type: 'bar', id: '2' }],
        links: { related: './foo/1/bars' }
      }
    }
  },
  included: [
    {
      type: 'bar',
      id: '1',
      attributes: {},
      relationships: {
        foo: { data: { type: 'foo', id: '1' } }
      }
    },
    {
      type: 'bar',
      id: '2',
      attributes: {},
      relationships: {
        foo: { data: { type: 'foo', id: '1' } }
      }
    }
  ]
}

2.) A single "links" request made (data and links but not included)

{
  data: {
    type: 'foo',
    id: '1',
    relationships: {
      bars: {
        data: [{ type: 'bar', id: '1' }, { type: 'bar', id: '2' }],
        links: { related: './foo/1/bars' }
      }
    }
  }
}

3.) A single "links" request made (links but not data or included)

{
  data: {
    type: 'foo',
    id: '1',
    relationships: {
      bars: {
        links: { related: './foo/1/bars' }
      }
    }
  }
}

4.) One or more "data" requests made (data but not links or included)

{
  data: {
    type: 'foo',
    id: '1',
    relationships: {
      bars: {
        data: [{ type: 'bar', id: '1' }, { type: 'bar', id: '2' }],
      }
    }
  }
}

@mydea
Copy link

mydea commented Oct 5, 2018

We are in this case using variant 2 (data & links but not included). And this used to use the data in Ember Data 3.1 for us, for the first request. In Ember Data 3.4, the behavior is as you describe, which is

a) a not-so-trivial change in behavior
b) not really better in my view - if IDs are included, why not use them?

Yes, links are conceptually nice, but we specifically added the IDs in this (and some other cases) in our apps because we wanted to avoid triggering literally hundreds of simultaneous API requests at the same time.

@runspired
Copy link
Contributor

@mydea using data instead of links is what would cause you to fire extra requests... links ensures the fewest possible...

@mydea
Copy link

mydea commented Oct 5, 2018

Not when using coalesceFindRequests: true :)

Let's take this example (simplified):

let model = {
  id: 'XX',
  items: [
    { id: 'A', subItems: [1, 2] },
    { id: 'B', subItems: [3] },
    { id: 'C', subItems: [4, 5] }
  ]
}

Where the subItems are hasMany relationships with the given IDs, which are specified with both data and links`, like for example this:

{
  data: {
    type: 'item',
    id: 'A',
    relationships: {
      subItems: {
        data: [{ type: 'subItem', id: '1' }, { type: 'subItem', id: '2' }],
        links: { related: '/api/items/A/subItems' }
      }
    }
  }
}

With data, we get two requests when starting out from model:

  • /api/models/XX/items
  • /api/subItems?ids=1,2,3,4,5

With links, we get 4:

  • /api/models/XX/items
  • /api/items/A/subItems
  • /api/items/B/subItems
  • /api/items/C/subItems

This is a very simplified example - in our case, this often ends up with hundreds of requests.

I hope this helps to clarify what I mean!

@runspired
Copy link
Contributor

So the nice thing about how things have improved in ember-data since the 2.x days is that now you can choose :)

Implement findHasMany and call findMany with the IDs instead of the link. The hope is that the ID only case will do just that by default soon anyway.

@runspired
Copy link
Contributor

I would also suggest that using includes or fastboot shoebox both seem like better solutions here than coalesceFindRequests.

@mydea
Copy link

mydea commented Oct 5, 2018

I actually tried that, but the problem here is that findMany will then not coalesce across multiple requests - it would just result in the following API requests instead:

  • /api/models/XX/items
  • /api/subItems?ids=1,2
  • /api/subItems?ids=3
  • /api/subItems?ids=4,5

Also, including it would be nice, but this is not that easy when working with relationships. In this example, /api/models/XX/items is a link itself to load a hasMany relationship.

We can of course try to somehow include everything up front, but this then leads to one giant API request which blocks rendering of the whole page, which also doesn't really feel nice, and requires us to constantly think about this, whereas previously it would just work pretty nicely by default.

I guess my main question is, if we include IDs, why shouldn't we use it and instead use the link? I don't quite understand why this should be the preferred way from an API perspective. Maybe if coalesceFindRequests is false this makes sense, but if this is true it is by far the more efficient solution. I think it would make sense to either restore the previous behaviour if coalescaFindRequests is true, or at least provide a way to control this ourselves (e.g. a dedicated setting, or a new hook, ...).

Since this default has basically changed silently after ED 3.1, it would be nice to have a concrete recommended solution for this. IMHO, forcing us to use include everywhere is not really a solution - if I need to manually front-load everything in the route upfront, I lose one of the main advantages of Ember Data, which is the automatic relationship resolving.

@runspired
Copy link
Contributor

@mydea if you want to ick up the torch on this RFC and ensure that coalescing could work with it that would be amazing! emberjs/rfcs#356

Here is a twiddle showing how using store.findMany can work with coalescing today: https://ember-twiddle.com/6f1cdaf58fbf3db3f765ed2d6ff4a97c?openFiles=adapters.application.js%2C

It's less nice than I would like because store.findMany is a private API that takes in internalModels at the moment (and this is different on master where we use RecordData). It is likely safer to move this sort of coalescing into the adapter itself.

@runspired
Copy link
Contributor

@mydea and here is an example coalescing within the adapter itself: https://ember-twiddle.com/597ff8ccc4e9a06ff26c1754ba108fb3?openFiles=adapters.application.js%2C

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

No branches or pull requests

3 participants