Forcing returnPartialData in fragmentMatcher is causing inconsistent behaviour

[msimulcik]

1. I have observable query that is being refetched due to variables changes. It contains union type which is causing that returnPartialData is forced to true in fragmentMatcher (here)
2. Later in readStoreResolver (here), throwing of error is skipped even if some data is missing
3. As a consequence, getCurrentQueryResult (here) ends up returning partial: false which is obviously inconsistent.
4. Therefore also loading state calculation is wrong in currentResult (here)
5. Finally in react, instead of displaying loader, it fails to render results due to missing data. This is why I started to investigate.

[tmeasday]

Thanks for the detailed report @msimulcik. May I ask why you don't have __typename available in this case? That seems like an easy workaround/solution to the problem.

This does seem like an issue however.

@stubailo I kind of feel like this is a bit of a hack because it changes the behavior of readQueryFromStore in ways other than what it's trying to do (allow this one fragment to return impartial data).

I would suggest the solution is possibly to allow graphql-anywhere fragment matchers to affect the "partiality" of their subtree without affecting the entire query. What do you think?

[msimulcik]

@tmeasday, sorry, I linked wrong line in fragmentMatcher. It's actually this line that is causing problem in my case.

[tmeasday]

@msimulcik - got it, so adding __typename won't help.

[helfer]

@tmeasday I think we'll have to refactor where and how we set returnPartialData. Right now it's used in too many places, which makes it hard to follow what's going on, ultimately leading to bugs like this one.

[tmeasday]

@helfer well if we want to just focus on this particular problem, we'd need some way to take the result of a query and be able to tell if it's "partial" or not. I'm not sure if it's possible to do that in a way that wouldn't always return true in this case*

• A query with a fragment on an interface/union

[helfer]

I don't understand the fragmentMatcher well enough to know if setting returnPartialData to true was just a quick temporary solution, or if it is actually required.

In general it's worth pointing out that when you have a fragment it's not currently possible for the client to know whether the absence of matching data in the cache for that fragment is expected or not.

Take this query for example:

{
  user(id: "1"){
    name
    ... on Moderator {
      experience
    }
  }
}

If the cache contains the name field but not the experience field, is it because we're missing data, or because the fragment wouldn't actually match on the server? I know we have __typename, but that doesn't help us in the case of interfaces, which is why we're going to have to add at least partial knowledge of the schema on the client.

[tmeasday]

Note that there's probably similar bugs here: https://github.com/apollostack/apollo-client/blob/currentResult-optimisiticResponse/src/core/QueryManager.ts#L939 as a result of this code

[msimulcik]

What is the status of this issue? Is anybody working on it? It's actually preventing me from updating react-apollo to a newer version. Maybe I can help with it, but will need a little guidance.

[helfer]

@msimulcik Nobody is working on it right now. I think for a permanent fix we'll need a more solid way to deal with fragments, which we'll be able to do in January most likely.

In the meantime, I think we can fix the bug that makes apollo-client wrongly indicate that full data was returned when it is in fact partial. In order to do that I think we need to change graphql-anywhere so partial data can be turned on just when matching fragments but not otherwise (or for an entire subtree as @tmeasday suggested).

The best way to help out would be to make a PR with a unit test that reproduces this issue.

[tmeasday]

I think the simplest unit test to add would be one that highlights that the API contract

readQueryFromStore({ ..., returnPartialData: false }) throws an exception if incomplete data is is in the store

is actually incorrect in the case of union fragments.

(I say "API contract" because that assumption is made at various places in the code base. We should either enforce it or not make those assumptions).