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

Sideposting with lid leaves relationship in invalid state #7127

Closed
andreyfel opened this issue Apr 15, 2020 · 6 comments
Closed

Sideposting with lid leaves relationship in invalid state #7127

andreyfel opened this issue Apr 15, 2020 · 6 comments

Comments

@andreyfel
Copy link
Contributor

#7126 adds a test illustrating this issue.
First assertion fails with error:

assert.deepEqual(cake.hasMany('ingredients').ids(), ['2']);
     expected: [ '2', [length]: 1 ]
       actual: [ '2', '2', [length]: 2 ]

Second assertion fails with error:

assert.equal(cake.ingredients.objectAt(0).name, 'Cheese');
Promise rejected during "lid works for sideposting scenario": Assertion Failed: You looked up the 'ingredients' relationship on a 'cake' with id 1 but some of the associated records were not loaded. Either make sure they are all loaded together with the parent record, or specify that the relationship is async ('hasMany({ async: true })')

     Error: Assertion Failed: You looked up the 'ingredients' relationship on a 'cake' with id 1 but some of the associated records were not loaded. Either make sure they are all loaded together with the parent record, or specify that the relationship is async ('hasMany({ async: true })')
         at Object.assert (http://localhost:7357/assets/vendor.js:36137:15)
         at InternalModel.getHasMany (http://localhost:7357/assets/vendor.js:87228:48)
         at Cake.get (http://localhost:7357/assets/vendor.js:73221:36)
         at http://localhost:7357/assets/vendor.js:15502:32
         at untrack (http://localhost:7357/assets/vendor.js:55813:7)
         at ComputedProperty.get (http://localhost:7357/assets/vendor.js:15501:34)
         at Cake.CPGETTER_FUNCTION [as ingredients] (http://localhost:7357/assets/vendor.js:14490:25)
         at Object.<anonymous> (http://localhost:7357/assets/tests.js:10215:25)

@runspired
Copy link
Contributor

runspired commented Apr 15, 2020

Each position in which a ResourceIdentifierObject occurs in the payload will need to have the lid. From the perspective of JSON:API this is consistent with the original proposal

When a server receives a request document that contains resources with local
IDs, the server must include the matching lid member and value in every
representation of that resource or resource identifier in the response document.

In the interest of completeness, the local:id proposal which supersedes this does not have this requirement, which appears to be a shortcoming (I fail to see how local:id solves side posting, although with JSON:API moving towards operations probably it was seen as an unnecessary use case, I'm curious on @dgeb's thoughts there)

@runspired
Copy link
Contributor

Local:ID Proposal https://github.com/json-api/json-api/pull/1436/files
Original lid proposal json-api/json-api#1244

@runspired
Copy link
Contributor

There are ergonomics here that we want to improve in the long term, but currently due to the primary (and only) method of updating the cache being store.push which expects complete documents, the linkages throughout a document need to be complete throughout.

@andreyfel
Copy link
Contributor Author

When I've tried to make a minimal test case to reproduce the issue I faced in my app I first tried to use store.push() and I didn't see the issue with relationships. Only when I mocked Adapter and Serializer and called save, the issue appeared.

I tried to add lid to the relationship as you suggested and it didn't help. Please see the updated PR.

@andreyfel
Copy link
Contributor Author

I've also noticed that if I remove id from relationship and leave only the lid, the first assertion passes, but the second one is still failing.

@andreyfel
Copy link
Contributor Author

Closed by #7126

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

2 participants