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

Regression: Ember Data tries to reload an already-loaded belongsTo #7049

Closed
Herriau opened this issue Feb 18, 2020 · 3 comments · Fixed by #7532
Closed

Regression: Ember Data tries to reload an already-loaded belongsTo #7049

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

Comments

@Herriau
Copy link

Herriau commented Feb 18, 2020

This is essentially exactly the same issue that was reported in #4099

Reproduction

Ember Twiddle: Here

If you have a console open while running this twiddle, you will observe the following output:

Requesting http://localhost:3000/authors/1
Requesting http://localhost:3000/posts/1/author
Requesting http://localhost:3000/posts/2/author
Requesting http://localhost:3000/posts/3/author
Requesting http://localhost:3000/posts/4/author

Expected output:

Requesting http://localhost:3000/authors/1

Description

If a hasMany relationship is loaded through includes (e.g. store.findRecord('author', 1, { include: 'posts' })) then the inverse belongsTo (e.g. post.author) is still not considered loaded and consuming it will result in unnecessary additional requests.

Versions

This behaves properly in EmberData 3.4.4, but it does not in EmberData 3.5.2, and all the way through the current release.

@oliverlangan
Copy link

There are some differences in API-side implementations of JSON:API.

Sometimes relationship types/ids are a part of the payload even when they are not loaded (includes). In those cases, it seems as though Ember Data does the right thing: it realizes that it already has the related data loaded, and does not trigger a new request.

Other times, the data is not loaded at all, and rather a link to it is included. In those cases, we see the behavior above.

@snewcomer
Copy link
Contributor

@Herriau 👋 I can start looking at this. If we have already set in stone the belongs-to relationship b/w a post and author by the time we follow the related link, I'm assuming this is solvable.

@runspired
Copy link
Contributor

@Herriau if you PR a test I suspect recent improvements resolved this; however, I'd also note that this failure occurs because of ambiguity in the JSON:API spec that allows for related resource payloads to convey conflicting levels of information.

When we process the belongsTo side in the example above we receive only a links object, which conveys that we don't know the data and still must load it. That payload is processed in isolation from the hasMany side, and so the lack of bi-directional full-linkage here (which the spec allows for) means that there is room for error.

We should have been smart enough internally to still settle into the correct state, but the complexity of the former relationship-layer left a lot of odd outcomes on the table. It is much simpler now and if this is still an issue would be a simple fix. If you PR a test I'll fix it if it fails and merge it to prevent regression if it passes.

sly7-7 added a commit to sly7-7/data that referenced this issue May 21, 2021
runspired pushed a commit to sly7-7/data that referenced this issue May 26, 2021
runspired added a commit that referenced this issue May 26, 2021
… its data (#7049) (#7532)

* add test (closes #7049)

* adding links makes the test fail

* update test and fix issue

* fix test

Co-authored-by: Chris Thoburn <[email protected]>
@runspired runspired added 🏷️ bug This PR primarily fixes a reported issue and removed Bug labels Sep 11, 2023
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
Development

Successfully merging a pull request may close this issue.

4 participants