Two queries loop forever when requesting different parts of same object

[nordvall]

Intended outcome:
I have a CRUD-like application where the user may create, read update or delete objects of different types. Let's say we are talking about "countries" and "cities". I can then execute queries like this:

countries { id name }

or this:

cities { id name }

However, different users have different permissions to work with these objects, and the server owns the logic about this. To indicate to the React app whether Create, Edit or Delete buttons should be visible for the current user and a particular object type, there is a "permissions" object available in the GraphQL schema.

Then we can make queries like this

permissions { countries cities }

...and get back lists of enum values (CREATE, READ, UPDATE, DELETE). An example response for the query above is:

permissions { countries: ['READ'], cities: ['CREATE', 'READ', 'UPDATE'] }

If a specific React component is handling Countries, it may then make the following query:

countries { id name } permissions { countries }

...and another component handling Cities may execute this query:

cities { id name } permissions { cities }

Actual outcome:

This worked fine until 3.0.0-beta.46. But now, at least when two queries like those mentioned above are executed simultaneously, there is in an infinite loop of queries against the graphql server.

The problem seems to be around the permissions object. My conslusion is that the cache is having trouble when different parts of the object are coming in via different queries, for an object that has no identity.

The issue goes away if I do any of the following:

• Remove one of the queries
• Remove the permissions part of at least one of the queries
• Expand the permissions part so all queries request the complete permissions object
• downgrade to a version less than 3.0.0-beta.46

How to reproduce the issue:
I've uploaded a repro project here: https://github.com/nordvall/apollo-query-loop

In the real app I can see the queries running off in the Networing tab in Chrome. In the repro project I've included some console logging, so you can see the looping in the Console tab instead.

The real server is running GraphQL.NET, so I've tried to replicate the schema in the Apollo resolver object model.

Versions
@apollo/client: 3.0.0-beta.53
graphql: 14.6.0
react ^16.12.0

[c10h22]

@nordvall I am getting same issue with v3.0.0-beta.50.

We are prefetching (for caching) some data with two independent queries.
Infinite loops occurs on first rendering because first query is faster than the second when loading of first query is finished, the second relaunch and when the second loading finish, the first one relaunch.

So to resume the situation here below the state logs:

component:adminSettings loading states +242ms {firstQuery: false, secondQuery: false}
component:adminSettings loading states +228ms {firstQuery: false, secondQuery: true}
component:adminSettings loading states +278ms {firstQuery: false, secondQuery: false}
component:adminSettings loading states +199ms {firstQuery: true, secondQuery: false}
component:adminSettings loading states +387ms {firstQuery: false, secondQuery: false}
component:adminSettings loading states +204ms {firstQuery: false, secondQuery: true}
component:adminSettings loading states +236ms {firstQuery: false, secondQuery: false}
component:adminSettings loading states +204ms {firstQuery: true, secondQuery: false}
component:adminSettings loading states +243ms {firstQuery: false, secondQuery: false}

[c10h22]

@nordvall I think this is duplicate #6358, as per comment there we are using useLazyQuery

[benjamn]

@nordvall Can you tell me more about this Permissions type?

I can see that the desired behavior is for the fields of this Permissions object to be merged over time, so you don't lose data fetched by different queries. Is that because the Permissions object is conceptually a global singleton object, so it's always safe to merge new fields into it? Or should the merging of Permissions object fields apply only to objects requested via the root query { permissions } field?

To achieve the first behavior (global singleton), you can give Permissions objects a constant identity with keyFields: []:

const client = new ApolloClient({
  cache: new InMemoryCache({
    typePolicies: {
      Permissions: {
        // Interpretation: Permissions objects are normalized, but they're all
        // the same logical object, because their identity does not depend on
        // any of their fields (other than __typename).
        keyFields: [],
      },
    },
  }),
  link
});

To achieve the second behavior (merge only query { permissions } objects when they collide):

const client = new ApolloClient({
  cache: new InMemoryCache({
    typePolicies: {
      Query: {
        fields: {
          permissions: {
            // Interpretation: normally unidentified objects overwrite each
            // other when they collide, but we can opt into merging their fields
            // if we know that's safe:
            merge(existing, incoming, { mergeObjects }) {
              return mergeObjects(existing, incoming);
            },
          },
        },
      },
    },
  }),
  link
});

Either one of these approaches will stop the mutual clobbering of data that's triggering the refetching. Obviously it would be ideal to detect and warn about situations like this, but I want to be sure the possible solutions that we would recommend in those warning messages actually meet your needs.

[benjamn]

Speaking of warnings, I would also like to draw your attention to PR #6372, which would have given the following warning in your case:

Cache data may be lost when replacing the permissions field of a Query object.

To address this problem (which is not a bug in Apollo Client), either ensure all
objects of type Permissions have IDs, or define a custom merge function for the
Query.permissions field, so InMemoryCache can safely merge these objects:

  existing: {"__typename":"Permissions","countries":["READ","UPDATE"]}
  incoming: {"__typename":"Permissions","cities":["READ","UPDATE","DELETE"]}

For more information about these options, please refer to the documentation:

  * Ensuring entity objects have IDs: https://go.apollo.dev/c/generating-unique-identifiers
  * Defining custom merge functions: https://go.apollo.dev/c/merging-non-normalized-objects

Unless you immediately guessed the nature of the problem, I imagine this warning would have been pretty helpful?

[nordvall]

Thank you for your detailed explanations!

From the options that you describe I would say that the "global singleton" pattern applies to my scenario. (I don't really understand the second option and how it's different conceptually, but nevermind).

I have been reading the about both keyfields and merge functions in the documentation, but to be honest:

• I did not understand that an empty array of keyFields would be a correct configuration (and even less that it would apply to my scenario)
• I did not understand how to address this object with a merge function. I understood that "permissions" needed to be considered as "fields" in the type policy terminology, but I thought my Permissions field sat at the root of the schema so there was no type to apply the policy to (but of course there is the Query type - stupid mistake by me).

I will go for the keyFields solution. Partly because the "global singleton" sounds like a correct desciption of this use case, and partly because it feels like a less "mysterious" configuration than a merge function (I'm caring for other developers trying to understand my configuration in the future).

[nordvall]

The warning in the PR would at least convince me that I was facing a configuration problem and not a bug (and point lazy people to the documentation). But it would not lead me to the keyFields solution =)

Also I understand that an incorrect cache configuration could result in data falling out of the cache, but I don't think it should result in this infinite query looping that we see now. My problem seems to be solved, but I hope for other developers that the situation can be handled more gracefully in future betas.

Edit: By "gracefully" I mean showing a warning like in the PR and also not looping the queries.

[thomassuckow]

I'm trying to figure out what is unsafe about merging unidentified types. They are stored in place in the cache and keyed off any variables that may have been involved in their querying. This also was the behavior of the 2.x InMemoryCache.

[thomassuckow]

This is problematic for us because we have a dynamic schema with UI plugins that can craft queries. Those UI plugins can know about parts of the schema that the base UI didn't know about. So that makes typePolicies an unattractive solution because we would have to keep the base UI in sync with every possible plugin.

That leaves normalizing everything. Are you claiming it would be proper to generate ids for every type?

Singletons are easy just set it to some constant, even possibly the type name

{
  permissons {
    id # Could be "permissions"
    countries
    cities
  }
}

But complex queries with variables get hairy

{
  complex(foo:$foo, bar:$bar){
    id  # Would have to be something like "complex:{foo:\"somthing\", bar:\"somethingelse\"}" and heaven forbid it was nested
    countries
    cities
  }
}

[benjamn]

@thomassuckow The solution to that specific problem is to specify a custom merge function for the Query.complex field, which likely calls options.mergeObjects(existing, incoming).

In general, if you want unidentified objects to be merged together, you have to give the cache permission to do so for each field that you care about. Merging is certainly safe sometimes, but not always, and the cache can't tell the difference without some help from you.

[thomassuckow]

With lazy loaded UI plugins and a dynamic graphql schema it makes knowing all the fields to list in the policies list basically unknowable. It is probably easier for us to impose an ID required policy for all types despite the pain in doing so. Though I think I won't recommend graphql for our future projects, while GraphiQl is really helpful, we have fought the server and client side over and over again and that isn't Apollo's fault it's graphql's.

[thomassuckow]

I abandoned the pull request. I am working on updating all our types to include id fields and hacking the server (Wrap parse as includeIds(parse(query))). Saves me from having to hand edit 1200 graphql queries. Though if someone wants to brag I would be curious what someone's script would look like to locate, parse and modify gql blocks to add a field.

function includeIdsSelectionSet(set, skip=false) {
 if(!set) return set;
 const hasId = skip || set.selections.some(selection => selection.name.value === "id")
 const selections = set.selections.map(selection => {
 const selectionSet = includeIdsSelectionSet(selection.selectionSet);
 return {...selection, selectionSet};
 }).concat(hasId ? [] : [{ kind: "Field", name: { kind: "Name", value: "id" } }]);
 return {...set, selections};
}
/**
 * The apollo graphql cache can result in infinite loops reloading data if data is queried with different fields and no id. So we enforce having an id.
 */
function includeIds(ast) {
 const definitions = ast.definitions.map(operation =>
 operation.operation === "query" ? {...operation, selectionSet: includeIdsSelectionSet(operation.selectionSet, true)} : operation
 );
 return {...ast, definitions};
}

Anyone tempted to use this, its a total hack. It might break Christmas or eat your firstborn.

[benjamn]

Judging by the provided reproduction (thanks!), @apollo/client@3.0.0-rc.5 should fix the endless network request loop, thanks to #6448.

[nordvall]

I removed the "keyFields: []" type policy that you showed me and upgraded to rc.6 in my "real" application (not the repro). I then saw the new, detailed error message in the console, but no request looping.
When I put the type policy mentioned above back, both the error message and the request looping were gone.
Thank you!

[dmt0]

To achieve the first behavior (global singleton), you can give Permissions objects a constant identity with keyFields: []:

const client = new ApolloClient({
  cache: new InMemoryCache({
    typePolicies: {
      Permissions: {
        // Interpretation: Permissions objects are normalized, but they're all
        // the same logical object, because their identity does not depend on
        // any of their fields (other than __typename).
        keyFields: [],
      },
    },
  }),
  link
});

@benjamn Can this become part of the official docs? All I'm seeing here is this:

keyFields?: KeySpecifier | KeyFieldsFunction | false;
type KeySpecifier = (string | KeySpecifier)[];

Would never guess that [] is a legal value there.