isLoading true after failed response from server

[g13013]

I have two Models
Model1-key --- hasMany -- Model2 (async)

when ember-data loads the records and one of the records request fail, its model isLoading property still always be true

[igorT]

Hi,
could you write a test for this please? It also seems related to #3011

[pangratz]

Is this such a failing test for this scenario?

var get = Ember.get;
var hasMany = DS.hasMany;
var Post, Comment, env;
var run = Ember.run;

module("integration/load - Loading Records", {
  setup: function() {
    Post = DS.Model.extend({
      comments: hasMany({ async: true })
    });

    Comment = DS.Model.extend();

    Post.toString = function() { return "Post"; };
    Comment.toString = function() { return "Comment"; };

    env = setupStore({ post: Post, comment: Comment });
  },

  teardown: function() {
    run(env.container, 'destroy');
  }
});

test("isLoading is false when request failed", function() {
  env.adapter.find = function(store, type, id, snapshot) {
    if (type === Post) {
      return Ember.RSVP.resolve({ id: id, comments: [1, 2] });
    }

    // reject second find call for comment
    if (type === Comment && id === '2') {
      return Ember.RSVP.reject();
    }

    return Ember.RSVP.resolve({ id: id });
  };

  run(function() {
    var post;

    env.store.find('post', 1).then(function(lePost) {
      post = lePost;

      equal(post.get("isLoading"), false, "post is not loading anymore");
      return post.get('comments');
    }).then(null, function() {
      equal(post.get("isLoading"), false, "post is not loading");
      equal(post.get('comments').objectAt(0).get("isLoading"), false, "first comment is no more loading");

      // fails here: isLoading is true but should be false
      equal(post.get('comments').objectAt(1).get("isLoading"), false, "second comment is no more loading");
    });
  });
});

[sly7-7]

@pangratz LGTM, did you plan to fix this ?

[pangratz]

@sly7-7 yup, I give it a try.

[pangratz]

Okay, so after digging into the source I think the problem lies in the rejection handler of the system/store/finders#_find method: to invoke record.notFound() and store.unloadRecord(record), the rejected record is retrieved via var record = store.getById(typeClass, id). This method checks for a loaded record via hasRecordForId which in turn returns false since the record has a isLoaded == false. So store.getById(typeClass, id) eventually returns null and hence the original record stays in the same state, namely rootState.loading, which has the isLoading flag set to true.

If the var record = store.getById(typeClass, id) is changed to a var record = store.recordForId(typeClass, id), then the record is available and the record.notFound() and store.unloadRecord(record); can be invoked and hence a transition into the empty state is made, where isLoading is false.

A (currently failing) test case for this:

 test("When loading a record fails, the isLoading is set to false", function() {
   env.adapter.find = function(store, type, id, snapshot) {
     return Ember.RSVP.reject();
   };

   run(function() {
     var leRecord = env.store.find('post', 1);
     leRecord.then(null, function() {
       // we know that such a record exists in the store, so we can use the private `recordForId`, 
       // as otherwise there is no way to to get the record, since `leRecord` is a rejected promise
       var post = env.store.recordForId('post', 1);

       equal(post.get("isLoading"), false, "post is not loading anymore");
     });
   });
 });

Fixing the above failing test by using store.recordForId() instead if store.getById() would fix the bug for the case where a record is requested via find.

---

The question for rejected records via adapter#findMany() (and optional adapter#findHasMany and adapter#findBelongsTo) is another one, since rejected promises of those methods are not yet handled, as far as I can tell: the rejection handlers are null (http://git.io/vJoLO, http://git.io/vJoLV and http://git.io/vJoL6). So rejected records for those requests suffer from the same difficulty, where the isLoaded stays true.

---

So, I hope my investigation report is understandable 😟. Fixing the case for the find request would be easy since only a change from store.getById to store.recordForId is needed. Fixing the findMany/findHasMany/findBelongsTo needs some further discussion on how to handle this ...

[pangratz]

I think a better solution for fixing the _find method is to reuse the record parameter. I'll create a PR for this ...

[igorT]

I think findMany is gonna be handled by #2891

[pangratz]

I think findMany is gonna be handled by #2891

👍

[g13013]

@igorT sorry, I totally forgot to write a test for it 😱, I was very busy ...