Under-fetching fields of new objects in mutations silently breaks mounted Queries

[stevehollaar]

Intended outcome:
When a mounted query can not resolve its data from the cache, as in the case following an under-fetching mutation, (linked example app below), I would expect it to re-fetch.

Actual outcome:
The query does not re-fetch, but instead renders with data: {}. In a real application, the component will likely attempt to access properties of undefined, causing a crash.

How to reproduce the issue:
Error template app here: https://github.com/stevehollaar/react-apollo-error-template/tree/mutate-bug

There are 2 mutation buttons, each which create a new Pet object and assign it to a random person.

• The first mutation response includes all fields on the mutated relationship, adding a new Pet object to the cache, and successfully re-rendering the query in App.
• The second mutation button omits the species field. This still adds a new Pet object to the cache, but the App query is not satisfied, and renders with data: {}
  https://github.com/stevehollaar/react-apollo-error-template/blob/mutate-bug/src/MutateButton.js

Version

• apollo-client@2.2.8

[pleunv]

I've had similar observations when upgrading from 2.0 to 2.1.

When 2 queries that fetch the same node but with different fields run in sequence, the second query initially renders a partial node but with missing fields, instead of presenting the previous behavior of rendering node: undefined while in transit.

For example, when you have the following 2 queries:

query Profile($id: GUID!) {
  profile(id: $id) {
    id
    firstName
    lastName
  }
}

query ProfileWithFields($id: GUID!) {
  profile(id: $id) {
    id
    firstName
    lastName
    fields {
      name
      value
    }
  }
}

The second query will initially be rendered with

profile: {
  id: 123,
  firstName: 'John',
  lastName: 'Doe', 
  fields: undefined
}

whereas before it would initially have rendered with profile: undefined while waiting for the network response.

In our case this is a little bit problematic since we're not checking loading states but rather whether profile !== undefined, and are expecting Profile.fields to be an array instead of undefined as soon as the data gets passed down. If this is a conscious design change this would require our mappers to either look at loading states instead, or our components to be able to deal with partial data, which, as you can suspect, is a bit of a if (x !== undefined) nightmare.

I haven't had time yet to dig into or set up a repro for this and reverted back to 2.0 for now but I have a suspicion it might be related to the subject of this issue.

[pleunv]

As an aside, I think this is caused by react-apollo rather than apollo-client. Reverting react-apollo from 2.1.3 to 2.0.4 fixes this behavior. Perhaps something you can verify as well @stevehollaar?

[cagdastulek]

I have similar problems since upgrade. So far I try to add guards to my code, which is ugly as you can guess, so I would appreciate if this issue gets higher priority.

[whootor]

We encountered the same issue. if (x !== undefined) checks are really annoying. Especially when the whole object breaks in mounted queries, which depending on the logic then just don't show any data anymore. Just triggering a refetch seems like it would then produce unnecessary roundtrips to the server and the other queries are updated later, depending on latency, even though all data would be available. Underfetched optimisicResponse breaks immediately. 😒

[whootor]

Hey, as our issue with optimisticResponses might be very much related, I am posting this here. We reproduced our issue https://github.com/whootor/react-apollo-error-template/tree/optimist.

Example 1

1. An initial component with an underfetched query is mounted, fetched and rendered
2. A second component with a fully fetched query is mounted delayed
3. Shortly after the fully fetched query is mounted, an underfetched mutation is triggered
4. The mutation triggers an optimistic response which is stored in the cache (the initial component shows the optimistic response)
5. The fully fetched query response comes in and updates the second component with loading=false and no data
6. The mutation response comes in and updates both components, the second one with the actually merged data of the full query and the underfetched mutation response.

At 5. we expect one of two behaviours:

1. the second component to not be updated at all, only when the final merged result comes in (stays in loading state)
2. the second component is updated with a merged result of the optimistic response and the full query result

Another issue that arises in Example 3 is that the mutation response is overwritten by the fully fetched query response with old data. As the mutation was triggered after the full query was, it should have precedence before the full query and be merged on top of the full query.

We believe to have traced down the issue to the merging in the in-memory-cache, but didn't get any further so far.

[pleunv]

As an aside, I think this is caused by react-apollo rather than apollo-client. Reverting react-apollo from 2.1.3 to 2.0.4 fixes this behavior.

Also ran into this behavior today after upgrading apollo-client from 2.2.8 to 2.3.1... not sure anymore now which package is causing this.

[felixk42]

Can report I am having the same issue as well, possibly related to apollographql/react-apollo#1314 ?

[idris]

Seems related: #2920 (comment)

[stefanmaric]

To workaround this bug unitl it's fixed, I've used this ugly hack and has worked so far:

  shouldComponentUpdate ({ data }) {
    if (data && !data.loading && !data.error && !data.dataYouWereExpecting) {
      data.refetch()
      return false
    }

    return true
  }

Simply refetch the query if it became broken and prevent buggy renders and flashes while it refetches.

[steelbrain]

@stefanmaric I have a fix for this. Will open a PR soon

Edit: Fix is up in apollographql/react-apollo#2003

[jesper-bylund]

I just spend about two days trying to nail this issue down. Extremely infuriating. Great to hear that a fix is on the way!

[cagdastulek]

(Also posted on apollographql/react-apollo#2003)

No action on this issue so far. 😞
Here is another test to show the issue. Basically, what happens is that if query A is already in the client cache and later same query A and a new query B run at the same time, where query B is a superset of query A, the expected behavior is that query B to not return anything from cache and wait for network response as client cache store does not have all the data to fulfill query B's request. However, the current bug results in FooView which was wrapped by query B to get data with missing fields.

import { InMemoryCache } from "apollo-cache-inmemory";
import { ApolloClient } from "apollo-client";
import { ApolloLink } from "apollo-link";
import { SchemaLink } from "apollo-link-schema";
import Promise from "bluebird";
import { assert } from "chai";
import { mount } from "enzyme";
import gql from "graphql-tag";
import { makeExecutableSchema } from "graphql-tools";
import React from "react";
import { ApolloProvider, Query } from "react-apollo";

describe("Cache-And-Network", () => {
  function createTestSchema() {
    const typeDefs = `
  type Query {
    foo: Foo!
  }
  
  type Foo {
    id: ID!
    field1: String
    field2: String
  }
`;

    const resolvers = {
      Query: {
        async foo() {
          return {
            id: "id5",
            field1: "value1",
            field2: "value2"
          };
        }
      }
    };

    return makeExecutableSchema({ typeDefs, resolvers });
  }

  const schema = createTestSchema();
  const link = ApolloLink.from([new SchemaLink({ schema })]);
  const apolloCache = new InMemoryCache();

  const client = new ApolloClient({
    cache: apolloCache,
    link
  });

  function FooView({ id, field1, field2, bar = {} } = {}) {
    return <div>{[id, field1, field2, bar].join(" ")}</div>;
  }

  class Parent extends React.Component {
    state = { show: "A" };

    render() {
      const { show } = this.state;
      if (show === "A") {
        return (
          <React.Fragment>
            <a onClick={() => this.setState({ show: "B" })} />
            <Query
              query={gql`
                query A {
                  foo {
                    id
                    field1
                  }
                }
              `}
            >
              {({ loading, data }) =>
                loading ? null : <FooView {...data.foo} />
              }
            </Query>
          </React.Fragment>
        );
      } else if (show === "B") {
        return (
          <React.Fragment>
            <Query
              query={gql`
                query A {
                  foo {
                    id
                    field1
                  }
                }
              `}
            >
              {() => {
                return null;
              }}
            </Query>
            <Query
              fetchPolicy="cache-and-network"
              query={gql`
                query B {
                  foo {
                    id
                    field1
                    field2
                  }
                }
              `}
            >
              {({ data }) => {
                return !data.foo ? null : <FooView {...data.foo} />;
              }}
            </Query>
          </React.Fragment>
        );
      }
    }
  }

  it("should not return data.foo unless all fields are resolved from backend", async () => {
    const app = mount(
      <ApolloProvider client={client}>
        <Parent />
      </ApolloProvider>
    );
    await waitUntil(() => {
      app.update();
      return app.find(FooView).exists();
    });

    assert.deepEqual(app.find(FooView).props(), {
      id: "id5",
      field1: "value1",
      __typename: "Foo"
    });

    app.find("a").simulate("click");

    app.update();

    await waitUntil(() => {
      app.update();
      return app.find(FooView).exists();
    });

    assert.deepEqual(app.find(FooView).props(), {
      id: "id5",
      field1: "value1",
      field2: "value2",
      __typename: "Foo"
    });
  });
});

async function waitUntil(checkFn, timeout = 2000, errorMessage) {
  const start = new Date();
  while (!checkFn()) {
    await Promise.delay(10);
    if (new Date() - start > timeout) {
      throw new Error(errorMessage || "Timeout");
    }
  }
}

[talkhot]

Wanne add my experience as-well in the hope the issues might be picket up or get a higher prio.

I'm building a relative large application and constantly hit problem with the cache.
I think the architecture of graphql encourages you to use fragments and only query data you need. With the current bug e.g. if I query part of the viewer it breaks the cache because on top level I use a withUser HOC. They only proper work around I found is to make massive queries who fetch all data (and reuse these with the mutation), but still this hits problems and. Would be great to have this fixed, or continue the discussion on how to fix this properly.

[pleunv]

The first reports of this issue date from almost a year ago, there's a repro (#3267), there's an outstanding PR, but other than that there's very little to no info and no acknowledgement that this is being looked at, or that additional help is needed. I fully understand that this is an open-source project and I absolutely massively appreciate the efforts of the people involved, but the lack of communication on this (in my opinion, very severe) issue is slowly getting a bit frustrating. It feels like efforts are being focused elsewhere, which is fine of course, but I guess it would be nice to know if that's the case. Apollo-client and react-apollo are a core component of my (and many other people's) apps but I'm getting a little bit uncomfortable using it again for any new ones.

[hwillson]

@pleunv We know this is frustrating, and it's something we're looking into resolving in Apollo Client 3.0. The changes to fix this will likely be breaking, which is why it's going to be slotted into the next major release. In the meantime, we recommend people use something like AC's mutate update function to merge results together, and update the store accordingly, to prevent the breakage. We currently can't do this automatically as we don't know which data is stale / valid after a mutation. All of this ties into cache invalidation, which will be a banner feature of 3.0.

In the meantime, I'm very tempted to merge apollographql/react-apollo#2003 to help with this. It's not a perfect solution, but it does work. Would merging that PR help in your case?

[pleunv]

@hwillson I appreciate the response. In my case however (and others, as well as the example above) this behavior is not necessarily triggered by the use of mutate which I think your recommendation is implying, it's caused by multiple queries requesting the same objects but with differing fields. The outcome is the same (fields missing from the response, loading = false, no new query going out) so it's undoubtedly linked to the same caching (in)validation problem but I don't think there's an easy workaround for this one.

I'll give the PR a shot (@steelbrain, you don't have this published to npm by any chance?) but looking at the code the fix seems to be directly in Query.tsx, which I assume leaves the HoCs still in a broken state, unless their implementation has been changed in 2.0 to make use of the <Query /> approach as well.

Due to the caching issue we're unable to upgrade to 2.0, which means that we're still using the HoC approach. Besides that, refactoring to <Query /> will be a long and painful process :). If a potential fix only fixes the Query scenario then we're probably better off holding out for 3.0, unless we can apply it to HoCs as well. Definitely willing to help, would just need some pointers as to where to look.

[hwillson]

The HOC's are now using Query under the hood, so hopefully it helps in both cases. Please test if you can and let me know what you think. Give me about an hour and I should be able to get a React Apollo alpha published to npm that has apollographql/react-apollo#2003 in it. I've tested it against the main repro in this thread and it fixes the issue, so if it doesn't for your case and you're able to put together a small runnable repro that shows what's happening, that would be awesome. I'll post back when the React Apollo alpha is ready. Thanks!

[steelbrain]

re @pleunv: I've published v2.1.11 version of this package with the patch applied to @steelbrain/react-apollo, We're using it in our app in production

[hwillson]

@steelbrain And everything has been working well?

@pleunv Can you try with @steelbrain/react-apollo?

[steelbrain]

Yes, everything has been working well in regards to this bug. But we have been working with a separate similar bug where partial is false, and so is loading but the data is still an empty object.

Usually happens when a large number of queries is invoked in a short amount of time (think logout of app and all mounted react components have their redux state changed and do new gql requests and get unauthorized gql errors).

The way we have "fixed" it in our app for now is with the assumption that in the GraphQL syntax, it's invalid to have a query with no fields, so in any case, regardless of partial, we check if Object.keys(data) is empty and we refetch.

[pleunv]

Squeezed in some preliminary testing with @steelbrain's version and I'm not able to reproduce the underfetching issue! Will keep using it for the next few days because I think some more testing will be needed, and let you guys know if we do run into any problems. I'm really glad this is moving forward! I know you guys are juggling a lot of different projects and keeping a business running at the same time, and I really do appreciate all the time you put into this.

Out of curiousity (mainly since you already mention 3.0): is there any public roadmap available, or is this still being discussed behind closed doors? :)

[hwillson]

@pleunv We're still hashing out the roadmap; it will be ready very soon, and will be referenced from apollographql/apollo-feature-requests#33.