Remove type check that prevents polymorphic types in API responses

[ryandoherty]

This is a PR for the request (#7357) to remove the assertion that prevents different types from being returned in API responses. Tests pass but I think there might need to be some tests added that assert Ember Data does 'the right thing' with varying types returned. (I have no idea how to do that :) )

Details:

tldr; This block:

data/packages/store/addon/-private/identifiers/cache.ts

Line 518 in ff4f911

if (type && type !== identifier.type) {

which verifies that the returned type matches the requested type is a problem for APIs that have polymorphic types. When we fetch data from the parent type's API endpoint, ember-data throws an exception because the api endpoint returns multiple types. But we do want all the subtypes of the parent type.

Example:

Given the JSON api:

/api/animals
/api/dogs
/api/cats

In our backend, 'dogs' and 'cats' are a subclass of 'animal', and queries to the /api/animals endpoint returns instances of cats and dogs. (This is via flask-restless & SQLAlchemy using the JSONAPI spec). Our Adapter is DS.JSONAPIAdapter

/api/animals returns:

{
  "data": [
    {
      "attributes": {
        "animal_type": "cat", <-- type column that SQLAlchemy uses to track the type of object to instantiate from the db row
        "created_at": "2020-10-19T13:29:07",
        "description": "my cat",
        "last_modified": "2020-10-21T15:04:08",
        "name": "foo"
      },
      "id": "1"
      },
      "type": "cat"
    },
    {
      "attributes": {
        "animal_type": "dog",
        "created_at": "2020-10-19T13:29:07",
        "description": "my dog",
        "last_modified": "2020-10-21T15:04:08",
        "name": "bar"
      },
      "id": "2"
      },
      "type": "dog"
    },
],
  "jsonapi": {
    "version": "1.0"
  },
  "links": {
    "first": "http://localhost:5000/api/animals?page[size]=10&page[number]=1",
    "last": "http://localhost:5000/api/animals?page[size]=10&page[number]=1",
    "next": null,
    "prev": null,
    "self": "/api/animals"
  },
  "meta": {
    "total": 2
  }
}

We have setup our ember models to have a parent type of 'Animal', 'Dog' and 'Cat' extend it. When we query the store for all animals like so: this.store.fetchAll('animals'), the reference error above is thrown because ember data expects only the type 'animal', but received 'dog' and 'cat'.

What we'd like ember to do is handle this and return an array (or individual object) of the types received. There are many use cases where we want to query and display all animals in the database, but we have to query each subtype separately. We've done some hacks to our API and ember app to support this (forcing all types for /api/animals to be 'animal' and jumping through some hoops after loaded to recreate the actual type on the frontend)

There are also situations when we know the id of the animal, but not type and would like to use this.store.findRecord('animal', id) and get back an instance of whatever subtype it is.

I hope this is clear. From the comment in ember-data's code where the exception is thrown, it appears there is awareness that it would help to remove the check. Thank you!

[snewcomer]

Thanks @ryandoherty! Do you have a test addition we can verify this results in added behaviour?

[ryandoherty]

I do not, I can give it a whirl. First time working on anything Ember, happy to take pointers to where/how I should do this. Thanks!

[runspired]

I'd note that we do have tests for single-table polymorphism support in place already. We might be able to improve our default implementation but the intent with identifiers is that you'll configure the cache in your own app to handle edge cases around one record having multiple potential identities.

[ryandoherty]

Update on this: I am writing tests, my test case is failing because the way the store updates it passes the type that corresponds to the url and not the type that is returned to some functions to update from the api response. Will have an update in a few days.

[ryandoherty]

So I spent more than a few hours trying to get tests working only to learn that doing this will require a substantial rewrite of the store.

The primary reason is the problem of how the store returns data from a findAll (and I'm assuming other calls). It loads up the RecordArray of the type of model it thinks it's fetching and returns it after the store has been populate with the response. But with this change, we could get back many types of objects, meaning we need to somehow merge all the RecordArrays together (I don't think this is possible b/c RecordArrays contain their type) or return a set of them.

I get the feeling that this change would require a substantial rewrite and new API to handle the case of "I asked the store to find items of typeA and it returned typeB and typeC that are subclasses of typeA."

I'm pretty new to Ember so unless I'm missing something I think this is as far as I can take this. I'd love to hear if I've understood all this correctly though, it was an interesting dive into Ember.

This is the code I'm looking at

data/packages/store/addon/-private/system/store/finders.js

Line 340 in 5c661db

return promise.then(

Thanks!

[snewcomer]

@ryandoherty Great job digging in! What version of e-d are you running? We do have some relevant tests. It looks like we do have a test for "changing type".

Do you happen to have a minimal reproduction (new ember app that reproduces this problem) we can use to make the test case? Or perhaps more insight on how these tests don't cover your scenario?

https://github.com/emberjs/data/blob/master/packages/-ember-data/tests/integration/identifiers/polymorphic-scenarios-test.ts

[ryandoherty]

@snewcomer I just pushed up my test case I created. Any pointers is appreciated, but if I'm correct then this request may be a big rewrite beyond a simple PR.

[runspired]

@ryandoherty in your app/tests you want query for this not findAll, query will return whatever records were listed in the primary data field of the JSON:API formatted response.

[runspired]

this should be singular

[runspired]

this should be singular too, though this may not matter for the test

[snewcomer]

@ryandoherty I'm interested in getting this across the line. Do you want to pick it back up or should I take the baton? Happy to do whatever.

[ryandoherty]

Thanks Scott! You should pick it up, it's a bit more than I can handle at the moment. Thanks! -Ryan

…

On Sun, Feb 6, 2022 at 6:50 PM Scott Newcomer ***@***.***> wrote: @ryandoherty <https://github.com/ryandoherty> I'm interested in getting this across the line. Do you want to pick it back up or should I take the baton? Happy to do whatever. — Reply to this email directly, view it on GitHub <#7421 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAALEPA5BX6UYM545OSIKOTUZ4XOZANCNFSM4WQ6FMQQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[runspired]

Closing in favor of #7955 for relationships and further RFC work for anything else. I'd note query likely "just works" and for peekAll / findAll a simple array concat would also "just work".