BlairAllegroTech / js-data-jsonapi

JsonApi Adapter for js-data
MIT License
15 stars 5 forks source link

Cached loadRelations result incorrectly reused for different objects. #33

Open rgant opened 8 years ago

rgant commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

Need to add some tests for this change

rgant commented 8 years ago

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 commented 8 years ago

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.