Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Subscription events trigger queries to be repeated unexpectedly when subscription returns same type of object but with subset of fields #8125

Open
insidewhy opened this issue May 4, 2021 · 8 comments

Comments

@insidewhy
Copy link

insidewhy commented May 4, 2021

We have a query and related subscription that look like this:

const QUERY = qgl`
  query CatQuery {
    cat {
      __typename
      _id
      qualities {
        __typename
        _id
        cuddliness
      }
    }
  }
`

const SUBSCRIPTION = qgl`
  subscription CatSub {
    catSub {
      __typename
      _id
      qualities {
        __typename
        _id
      }
    }
  }
`

i.e. the only difference between the fields requested from the query and subscription is that the subscription lacks qualities.cuddliness. Both of these return objects with __typename of Cat.

Now we have two hooks happening in the same component:

useQuery(QUERY);
useSubscription(SUBSCRIPTION);

Now, every time the server sends a new update to the subscription, the query is sent again, which is completely redundant. 10 updates to the subscription = 10 queries sent to the server. With apollo client 2 this worked as we expected, subscription events would not cause new queries to be sent to the server.

If we remove cuddliness from QUERY (so that the query is no longer requesting a superset of the fields in the subscription) then this behavior goes away and we only ever get one query sent.

This is causing a lot of unnecessary queries to be sent from our client to our server since upgrading to apollo 3.

@insidewhy insidewhy changed the title subscription events trigger query to be repeated unexpectedly. Subscription events trigger queries on same type, for superset of same fields, to be repeated unexpectedly. May 4, 2021
@insidewhy insidewhy changed the title Subscription events trigger queries on same type, for superset of same fields, to be repeated unexpectedly. Subscription events trigger queries to be repeated unexpectedly when subscription returns same type of object with with subset of fields May 4, 2021
@insidewhy insidewhy changed the title Subscription events trigger queries to be repeated unexpectedly when subscription returns same type of object with with subset of fields Subscription events trigger queries to be repeated unexpectedly when subscription returns same type of object but with subset of fields May 4, 2021
@benjamn
Copy link
Member

benjamn commented May 4, 2021

@insidewhy Thanks for reporting issues as you attempt to update to Apollo Client 3, instead of suffering in silence!

In this case, you've hit on a common pitfall with list fields that are populated by multiple operations (queries/subscriptions/mutations), which can happen when one of the operations adds objects to the list that are missing some of the properties expected by other consumers of the list, leading to incomplete cache reads for those other consumers, leading to refetching in many cases (depending on fetchPolicy).

I think the ultimate solution to this problem will involve some form of query transformation behind the scenes to ensure a given selection set includes all fields that might be needed by other consumers of that object. On a superficial level, this goes against the GraphQL principle of only fetching what you need, but I believe it would solve deeper problems, like this one.

In the meantime, you have a couple of workaround options:

  1. Use a shared ...QualityFragment for the Cat.qualities field in both the CatQuery and the CatSub, so that cuddliness is included in both places (or neither place).
  2. Use a field policy to define a default value for Quality.cuddliness:
new InMemoryCache({
  typePolicies: {
    Quality: {
      fields: {
        cuddliness(existing = "not cuddly") {
          return existing;
        },
      },
    },
  },
})

This will ensure the cuddliness field never appears missing when reading from the cache, so its absence won't trigger a refetch with fetchPolicy: "cache-first".

@insidewhy
Copy link
Author

@benjamn I see, in several of our cases we have another query that only needs to fetch the _id since we already have the objects with those _id fields in another place. To refetch them from the server entails a performance hit for us (since we have optimisations on the server side to skip the DB when only the id is requested and the id is already known). So the shared fragment thing is possible, but will be a needless performance hit. The defaults idea won't work for us either.

In our case, objects aren't added to the list most of the time. When we have no objects added to the list, and the second request is for a subset of fields, re-querying shouldn't be necessary? Could this optimisation be added to the code? It's something we need so bad, that it's something we could contribute to apollo.

@cglacet
Copy link

cglacet commented May 11, 2021

Wouldn't it make sense for a request to not try to get fields it never required? I think we are experiencing a similar issue, we have a first request that get a list of people without much details (eg. picture and name), then, we trigger a request to get the detail of a single person (many fields, including picture and name).

This second request seems to trigger a refetch of the first (I feel its similar to what is experienced with cats here, no?).

In our case the list is never mutated at any time while these refetch happens.

@insidewhy
Copy link
Author

@cglacet Yep that's exactly it, even when the subscription doesn't add an element to the list, or even change the list in any way, the query still refetches.

If the subscription added a new element to the list with less fields than the query requires, it's understandable. When the subscription doesn't change the list in any way, the re-query is completely unnecessary.

@cglacet
Copy link

cglacet commented May 11, 2021

So the logic behind the current implementation is to refetch a query when it seems like a field is missing?

In other words, if the cache looks like this:

{
    "ROOT_QUERY": {
        "findCats": {
           " __ref": "Cat:1",
            "__ref": "Cat:2"
        },
        "findCat({'id': 1})": {
             "__ref": "Cat:1"
        }
    },
    "Cat:1": {
         "name": "name1", 
         "color": "color1"
    },
    "Cat:2": {
         "name": "name2"
    }
}

The findCats query will be refetched because the cache is missing the second cat's color? Even if findCats doesn't request the color field (while findCat does)? Sorry if I'm asking the same question twice, I just want to make sure we talk about the same thing (because I don't use any subscription).

@insidewhy
Copy link
Author

insidewhy commented May 11, 2021

@cglacet My situation is like this... after the query QUERY in my first example, the cache might have:

{
  "Cat:1": {
    "__typename": "Cat",
    "_id": "Mr Sweetkins",
    "qualities": [{
      "__typename": "Quality",
      "_id": "loveablity",
      "cuddliness": "so loveable"
    }]
  }

Then after a subscription update, this cache entry might not get updated at all. since the subscription supplies this:

{
  "Cat:1": {
    "__typename": "Cat",
    "_id": "Mr Sweetkins",
    "qualities": [{
      "__typename": "Quality",
      "_id": "loveability"
    }]
  }
}

So after this update, the cache is identical. Yet still the query is refetched. I guess it's because the subscription only requests _id of qualities and not also cuddliness, yet that shouldn't matter if the update does not lead to a change in the cache.

@cglacet
Copy link

cglacet commented May 11, 2021

Ah ok, I see, its not the exact same case, but maybe the issue is the same. Let's wait and see 👍

@nevir
Copy link
Contributor

nevir commented May 21, 2021

I think the ultimate solution to this problem will involve some form of query transformation behind the scenes to ensure a given selection set includes all fields that might be needed by other consumers of that object. On a superficial level, this goes against the GraphQL principle of only fetching what you need, but I believe it would solve deeper problems, like this one.

Heh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants