.get for async hasMany relationships return error forever if request to fetch related data fails

[mike-lang]

Hi,

I've found that if you perform a .get against an async hasMany relationship on a model and the underlying API request fails, subsequent calls to .get the relationship continue to hand out the error from the original request.

I've created a tiny demo app to reproduce the behavior here, in summary though what it does is it invokes the getter for the relationship within the model hook of its index route. The mock back-end included with the demo app is written to emit an error on the endpoint that serves the data for the relationship. The failure comes back and triggers the error action handler for the route, which invokes and endpoint on the mock back-end to turn off the error emitting behavior and then the transition is retried. The model hook runs again, and the getter for the async relationship is invoked again, but it continues to return the error that was emitted the first time the getter was invoked. The underlying call to the back-end is never retried. The code for the demo app confirms that the back-end has in fact stopped emitting errors by counting how many times the catch handler has been invoked, and manually calling the back-end endpoint on the second catch. A successful response confirms that the back-end has stopped emitting errors.

I believe this issue is due to 2 aspects of the Ember Data code.

First, [the computed property generated on the model for the async relationship] doesn't have any logic to detect errors and preventing the typical value caching behavior of computed properties in general on error responses. I believe this could be resolved by simply adding a catch handler here that calls propertyDidChange on errors to clear errors so they aren't handed out as related values anymore once we've learned the retrieval fails:

Perhaps change this in relationships/has-many.js to:

  return Ember.computed({
    get(key) {
      return this._internalModel._relationships.get(key).getRecords()
        .catch((err) => {
          this.propertyDidChange(key);
          throw err;
        });
    },
    ...

Secondly, the relationship state code for findLink() retains the promise that succeeds/fails depending upon the API call and continues to hand it out on subsequent calls regardless of whether the fetchLink() call succeeds or fails. This makes the getRecords() call that backs the computed property continue to return the first failure on subsequent calls. I believe this could be resolved by simply zeroing out .linkPromise if the call to fetchLink() fails.

Perhaps change this in relationships/state/relationship.js to:

  findLink() {
    heimdall.increment(findLink);
    if (this.linkPromise) {
      return this.linkPromise;
    } else {
      let promise = this.fetchLink();
      this.linkPromise = promise;
      return promise.then((result) => result).catch((err) => {
        // console.log("fetchLink failed! nulling out this.linkPromise so we don't just hand out that failure over and over again");
        this.linkPromise = null;
        throw err;
      });
    }
  }

I could try to figure out how to write a failing test for this behavior and put together a pull request, but I first wanted to see if I was perhaps misunderstanding the intended design/behavior, and if Ember Data was interested in incorporating this sort of change. It seemed like we'd want to handle failure scenarios for async relationships this way though in Ember data rather than making application code remember to put catch handlers on every async relationship access and manually dump their models somehow. Did I perhaps miss something about how failure of relationship lookups are supposed to work? Please let me know what you think! Should I continue going down this path?

[karlbecker]

Hello! I'm a teammate of Mike's, and I hope we could get some help on this.

Thanks!

[stefanpenner]

It's in my radar. Slowly been fixing up quirks in this area.

sorry about the delay

[karlbecker]

Hi @stefanpenner ,

Any update on this? We ended up tackling this in a particular way, but would love to have any insight you or someone else could provide.

Thanks!

[karlbecker]

Hey @bmac , This is me being a squeaky wheel 😄

Think you could help us figure out whether making this happen would be wise, and give us a pointer to write a test for this?

Thanks!

[mike-lang]

Hi all @stefanpenner @bmac,

Could you provide some direction on how to move this forward? We were able to work around our issue at the time by using the relationshipRef interface, but we've now run across a scenario where our workaround appears to not work, so I'm looking at this issue again.

[runspired]

I'd look at the recent work on links, it may be easier to cleanup now. @mike-lang

[karlbecker]

Hi @runspired ,

Just a heads up that @mike-lang ran into this same issue today in a different piece of our code, and it's causing more workarounds.

We are on Ember 2.12 - do you think the recent work you mentioned would have been in Ember 2.12, or a newer version than that? Wondering which version we'd need to move up to in order to possibly eliminate this workaround.

Thanks!

[runspired]

@karlbecker I would target upgrading to 3.4 which is an LTS candidate. The relationship layer has been cleaned up a lot there and it would be easy to resolve this bug if it is still present. Either way we should add a regression test.

[karlbecker]

Thanks @runspired ! It'll be some effort to upgrade, so would be great to know which is the lowest version we might have to get to, but will let you know once we do get around to upgrading.

[sebastianhelbig]

This bug is still present. Any news here or a known workaround?

[runspired]

I suspect this is fixed, but a test PR to demonstrate one way or the other whether it is fixed would expedite a fix if needed / let us close this if not.

[karlbecker]

Hi @runspired , Does this demo Mike posted long ago work to demonstrate? https://github.com/thirdiron/ember-data-failed-async-relationship-lookup-repro-demo

It's on an older version of Ember, but feel free to upgrade it to a newer version, or I can try.

Thanks for the followup!

[runspired]

@karlbecker I'm not sure but if I can find someone to PR a test using what's in that repo that would be ideal, I far prefer a test PR here because otherwise if/when fixed we have regression protection.

[karlbecker]

Thanks @runspired - if you think you can find someone to make a PR for a test here, that would indeed be fantastic. Please use that repo as a reference point.

If you're unsuccessful in finding someone after awhile, I suppose I could always try, although I've never made a PR for Ember Data before.