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

DS.Snapshot.belongsTo() does not abide by the documented API #7040

Closed
Herriau opened this issue Feb 11, 2020 · 6 comments · Fixed by #7533
Closed

DS.Snapshot.belongsTo() does not abide by the documented API #7040

Herriau opened this issue Feb 11, 2020 · 6 comments · Fixed by #7533
Labels
🏷️ bug This PR primarily fixes a reported issue Needs Bug Verification

Comments

@Herriau
Copy link

Herriau commented Feb 11, 2020

Description

When an optional belongsTo relationship is loaded independently from the model it's defined on, e.g. when invoking await someRecord.belongsTo('someRel').load(), and the response is { "data": null } then DS.Snapshot.belongsTo() for that relationship returns undefined instead of null.

Say I have a first record loaded through store.findRecord('some-model', 1, { include: 'some-rel' }), intially hydrated from the following JSONAPI document:

GET /some-model/1?include=some-rel

{
  data: {
    "type": "some-model",
    "id": 1,
    "attributes": { ... },
    "relationships": {
      "some-rel": {
        "data": null,
        "links": {
          "related": "/some-model/1/some-rel"
        }
      }
    }
  }
}

And another one loaded through store.findRecord('some-model', 2) (note the lack of include), intially hydrated from the following JSONAPI document:

GET /some-model/2

{
  data: {
    "type": "some-model",
    "id": 2,
    "attributes": { ... },
    "relationships": {
      "some-rel": {
        "links": {
          "related": "/some-model/2/some-rel"
        }
      }
    }
  }
}

The someRel relationship is already loaded on ID=1 (by way of ?include), however it's not initially loaded on ID=2, but let's say I load that one relationship separately with await record.belongsTo('someRel').load(), and let's say the request / response that resulted from that call looks like:

GET /some-model/2/some-rel

{
  "data": null
}

Suppose now I have a custom serializer for some-model:

export default DS.JSONAPISerializer.extend({
  serialize(snapshot, options) {
    console.log(snapshot.belongsTo('some-rel'));
    return this._super(...arguments);
  },
});

Then we can observe the following:

someModel1.serialize(); // logs `null`
someModel2.serialize(); // logs `undefined`, expected `null`.

The API doc for DS.Snapshot.belongsTo() states:

returns (Snapshot|String|null|undefined)
A snapshot or ID of a known relationship or null if the relationship is known but unset. undefined will be returned if the contents of the relationship is unknown.

So I would have expected both calls to return null, since both fall under the "known but unset" case.

Versions

└─ [email protected]
└─ [email protected]
├─ @ember-data/[email protected]
├─ @ember-data/[email protected]
├─ @ember-data/[email protected]
├─ @ember-data/[email protected]
├─ @ember-data/[email protected]
├─ @ember-data/[email protected]
├─ @ember-data/[email protected]
├─ @ember-data/[email protected]
├─ @ember-data/[email protected]
├─ @types/[email protected]
├─ [email protected]
└─ [email protected]
@Herriau
Copy link
Author

Herriau commented Feb 11, 2020

This originally came up when investigating a public way of dealing with this other issue: #5629 after #7039 turned out to be a dead-end.

@snewcomer
Copy link
Contributor

The problem here seems to be that we do not handle meta in top level objects. The top-level object coming from record.belongsTo('someRel).load(). I think this was an addition after the spec was initially developed so that is maybe why it is missing.

Any members MAY be specified within meta objects.

I can look into this at some point.

@runspired
Copy link
Contributor

The bug here is because hasAnyRelationshipData is false when it should be true. This indicates that for null payloads fetched via findBelongsTo we do not appropriately update the state flags on the relationship post-fetch.

@runspired
Copy link
Contributor

I think the fix is likely something in syncRelationshipDataFromLink, when we construct the wrapper for the payload we don't seem to account for null.

@runspired
Copy link
Contributor

runspired commented Apr 13, 2021

@snewcomer We do actually handle top level links and meta for this case in theory, I suspect meta not working may be related to not handling null. We should fix both of these issues.

@runspired
Copy link
Contributor

This may be at least partially fixed on master, @Herriau could you PR a test?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bug This PR primarily fixes a reported issue Needs Bug Verification
Projects
None yet
3 participants