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

Cached loadRelations result incorrectly reused for different objects. #33

Open
rgant opened this issue Sep 9, 2016 · 5 comments
Open
Milestone

Comments

@rgant
Copy link
Contributor

rgant commented Sep 9, 2016

Basically if you use LoadRelations in a loop for the same relationship the first call is used for every future call.

var commentsList = [];
DS.findAll('reports').then(function (rpts) {
    rpts.forEach(function (r) {
        if (r.status === 'Feedback Provided') {
            // This is a hasMany relation, so findAll is used.
            DS.loadRelations('reports', r, ['comments']).then(function (rpt) {
                commentsList.concat(rpt.comments);
            });
        }
    });
});

Trying to trace through now to figure out why loadRelations treats the request for /v3/reports/123/comments as the same as /v3/reports/456/comments.

@rgant
Copy link
Contributor Author

rgant commented Sep 9, 2016

Think this is the issue:

DS#findAll queries are cached and keyed in the data store by JSON.stringify(params). So, by default, DS#findAll first checks to see if the query has made been before, and if so (by default) will immediately resolve with the items that were returned the first time. JSData behaves in two different ways here depending on the boolean value of options.useFilter, as explained next:
http://www.js-data.io/docs/dsfindall

Plus using '?' as the id for the hasOne relations find.

@rgant
Copy link
Contributor Author

rgant commented Sep 9, 2016

I quick fix for the hasOne relation is to replace the 'id' with the relationship ID.

var relationId = options.jsonApi.jsonApiPath;
return childResourceDef.find(relationId, options).then(//...

@BlairAllegroTech
Copy link
Owner

Need to add some tests for this change

@rgant
Copy link
Contributor Author

rgant commented Sep 13, 2016

You are quite right. I apologize for not including tests. I can probably work on some next week.

My thoughts would be to setup two primary objects with 1 hasOne and 1 hasMany relationships. We would then want to load object 1's hasOne relationship followed by object 2's and confirm that they are different results. Same with the hasMany relationship.

Not sure if we need to cover belongsTo.

And it would also be good to confirm that the cacheing is actually functioning correctly. However I'm not sure how to do that off hand.

@BlairAllegroTech
Copy link
Owner

Yes i agree with above. I would comment out the changes that fix this issue. Create the test and check that it fails. Then un-comment the changes and ensure the test passes.

Checking caching is working correctly
This could be done by making a request to loadRelations and then making a second request and check that js-data does not invoke the adapter again to load the relationship. I assume that using bypassCache should by-pass this behavior.

@BlairAllegroTech BlairAllegroTech modified the milestone: V2 Sep 13, 2016
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