js-data-jsonapi icon indicating copy to clipboard operation
js-data-jsonapi copied to clipboard

Cached loadRelations result incorrectly reused for different objects.

Open rgant opened this issue 9 years ago • 5 comments
trafficstars

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 avatar Sep 09 '16 19:09 rgant

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 avatar Sep 09 '16 20:09 rgant

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(//...

rgant avatar Sep 09 '16 20:09 rgant

Need to add some tests for this change

BlairAllegroTech avatar Sep 13 '16 08:09 BlairAllegroTech

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.

rgant avatar Sep 13 '16 15:09 rgant

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 avatar Sep 13 '16 21:09 BlairAllegroTech