Infinite Loop (Interlocking) when two useQuery calls to the same nested object w/ different input

[forresty]

Issue Description

See https://codesandbox.io/s/friendly-roentgen-4wk6q2

update: see https://codesandbox.io/s/festive-carlos-m7v5jd for infinite loop without input

specifically, when two or more components try to fetch the same nested object but with different inputs, the Apollo client refuses to auto-merge the cache of the nested object, but instead overwrites nestedObject repeatedly, causing an infinite loop

query QueryWithInputInNestedObject($input: Int!) {
  rootObject {
    id
    nestedObject {
      fieldWithInput(input: $input)
    }
  }
}

const Interlock = ({ input }) => {
  const { loading, data } = useQuery(QUERY_WITH_INPUT_IN_NESTED_OBJECT, {
    variables: { input }
  });

  return <p>{loading ? "loading" : JSON.stringify(data)}</p>;
};

const InterlockExample = () => {
  return (
    <>
      <h3>
        If at least two components requesting the same object including a nested
        object, with one field in the nested object having a different input.
        The cache is not auto-merged for the nested object. Creates an interlock
        overwriting the cache every time.
      </h3>
      <Interlock input={0} />
      <Interlock input={1} />
    </>
  );
};

The cache key fieldWithInput keep alternating based on the variables of two useQuery calls

---

To be fair, when the infinite loop happens, there is the following warning in the console:

Cache data may be lost when replacing the nestedObject field of a RootObject object.

However, it's totally unintuitive that two seemingly harmless useQuery calls can cause an infinite loop.

Link to Reproduction

https://codesandbox.io/s/friendly-roentgen-4wk6q2

update: see https://codesandbox.io/s/festive-carlos-m7v5jd for infinite loop without input

Reproduction Steps

follow the link and see the interlocking infinite loop in action

[phryneas]

You have cut the warning message short here - the full text of it is

Cache data may be lost when replacing the nestedObject field of a RootObject object.

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

  existing: {"__typename":"NestedObject","fieldWithInput({\"input\":1})":"7j7hf"}
  incoming: {"__typename":"NestedObject","fieldWithInput({\"input\":0})":"rd3yu"}

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

When working with Apollo, you are working with a global cache - and that only really works if you give it data that can be identified uniquely.
In this case, you request two objects that, without your help, will end up being identified the same way and overwriting each other.

I get that this might be unintuitive at first, but it's a consequence of using a global cache instead of a cache that works on a per-hook-call-level.
We hope to show up these pitfalls early and give as much guidance as we can with that warning message.

If you have any ideas on how to improve the warning, that would be very welcome :)

[phryneas]

Adding more explanation here:

The problem here is at it’s core this conflict:

  existing: {"__typename":"NestedObject","fieldWithInput({\"input\":1})":"7j7hf"}
  incoming: {"__typename":"NestedObject","fieldWithInput({\"input\":0})":"rd3yu"}

As this object has no unique identifier for nestedObject, Apollo Client cannot make a choice if nestedObject is the same instance of NestedObject with extra properties, or a completely new NestedObject instance that would have a completely different value for fieldWithInput({ "input": 1 }) and merging those would be dangerous.

The input here is not really necessary to trigger this behavior.

Imagine this alternative scenario:

query {
    post {
      id
      comments {
        author
      }
    }
  }

and another query

query {
    post {
      id
      comments {
        text
      }
    }
  }

Now, fire those two queries off at two different times. You will get responses along the line

{ post: { id: 1, comments: [ { author: "Tim" }, { author: "Lilly" } ] } }

and

{ post: { id: 1, comments: [ { text: "I approve of this" }, { text: "I don't agree" } ] } }

Now, if AC were to blindly merge those object, it would end up with

{ post: { id: 1, comments: [ { author: "Tim", text: "I approve of this" }, { author: "Lilly", text: "I don't agree" } ] } }

But do we know this is the case?
In the meantime, Tim could have deleted their comment, and Sandy could have written a new one. The real data could look like

{ post: { id: 1, comments: [ { author: "Lilly", text: "I approve of this" }, { author: "Sandy", text: "I don't agree" } ] } }

and if we were blindly auto-merging, we would get completely wrong data.

That’s why Apollo Client is not automatically merging things, but if it cannot identify cache objects, instead replaces them - even if it means that data might be lost (which in this case causes it to be queried again over and over).

And in that case, you get that warning, telling you that you need some form of unique identifier for AC to make sure it can safely merge.

[forresty]

hi @phryneas, thanks for the quick response!

I get your point, if the user fully understands that useQuery by default relies on the caching mechanism to achieve the broadcasting effect, this would be more like a feature and less like a bug. However, I think most modern-day developers won't have the time or patience to read through the document, and to them, this definitely looks more like a bug.

Another argument is, that by toggling fetchPolicy (change to no-cache) and we will have a completely different behavior, it would be even more confusing.

And for our case, our special use case means that we don't have a stable ID field of the nestedObject that would make sense; and in order for our API clients to avoid an infinite loop, mandating them to request that unnecessary ID field is not very elegant. Unless there's a way to always send the ID field to the client without them requesting it?

Also, unfortunately, our company uses a shared Apollo client setting that makes configuring a custom merge function difficult.

If you have any ideas on how to improve the warning, that would be very welcome :)

I guess at least we need to update the warning to mention the possibility of infinite loops ;-)

[phryneas]

Also, unfortunately, our company uses a shared Apollo client setting that makes configuring a custom merge function difficult.

At least this one I can help you with easily :) You can use addTypePolicy to add additional type policies, after creating your Apollo Client - so that would even work with a shared base configuration.

Unfortunately, we have been lacking docs on that feature, but new docs are right now being added over in #10967

[phryneas]

mandating them to request that unnecessary ID field is not very elegant

This is something we should probably highlight more in the docs: that id field is by far not unneccessary - it is one of the core features of Apollo Client, and you should add it always, and everywhere, if the entity has one.
We cannot automatically add that to your queries, as Apollo Client doesn't know your schema, and if we would add it to an entity where it doesn't exist in the schema, you would end up with an error.
So the next best thing you could do would be to set up a schema-aware lint rule that warns if you are not adding the id field where it would be possible.

[forresty]

Now, if AC were to blindly merge those object, it would end up with

{ post: { id: 1, comments: [ { author: "Tim", text: "I approve of this" }, { author: "Lilly", text: "I don't agree" } ] } }

This is a very good example, thanks! Now it makes sense why the Apollo client is not auto-merging objects without an identifier.

[forresty]

At least this one I can help you with easily :) You can use addTypePolicy to add additional type policies, after creating your Apollo Client - so that would even work with a shared base configuration.

So the next best thing you could do would be to set up a schema-aware lint rule that warns if you are not adding the id field where it would be possible.

Thanks! I will see if I can push through with these options. I will refer to this issue when discussing it with the relevant internal teams.

We cannot automatically add that to your queries, as Apollo Client doesn't know your schema, and if we would add it to an entity where it doesn't exist in the schema, you would end up with an error.

I haven't gone through the spec yet, but is there a way to mark a field so the client has to request it? If not, is there a plan to add such functionality?

As for the warning message, do you mind if I send a simple pull request to add info regarding possible infinite loops, and maybe point to this issue?

[phryneas]

I haven't gone through the spec yet, but is there a way to mark a field so the client has to request it? If not, is there a plan to add such functionality?

This circles back to the core problem: Apollo Client doesn't know your Schema. Your Schema could be Megabytes big, and you don't want to ship that to your users (or, in some cases might not want to expose it at all). So marking a field in the schema so the client has to request it would not work - because the client would never see that "mark". Likewise, the server cannot just send data that has not been requested by the client - the client would just discard it, as per spec it wouldn't be allowed to use it (and the server would already violate the spec by returning it).

As for the warning message, do you mind if I send a simple pull request to add info regarding possible infinite loops, and maybe point to this issue?

Sure, go ahead! We can go back and forth on the exact wording in the PR :)

[adamesque]

We've run into similar issues when working on large codebases with many teams contributing features and queries on a single page, and echo the recommendation for a schema-aware linter to catch these missing ids.

For objects that either don't have an easily identifiable id field, a non standardized key field name, or that require a custom merge policy, we've wondered if a small set of schema directives could be used to annotate schema and drive a type policy generator for common cases (simple merge, simple array merge, etc) similar how possibleTypes can be generated.

Also, ideally (in a perfect world), Apollo Client would do some cycle counting to detect cache bash loops and short-circuit them by returning an error for some (or all) guilty queries.

[forresty]

Hi, I've updated the codesandbox (new link https://codesandbox.io/s/festive-carlos-m7v5jd, I did not claim the old one so now my own modifications become a new fork) to show that infinite loop can still present when no input is given or not in nested objects, to reflect that the true cause is AC won't auto-merge objects without identifiers.

query QueryRandomBeerName {
  randomBeer {
    # if we request ID, the cache will be auto merged and no infinite loop
    # id
    name
  }
}

query QueryRandomBeerOrigin {
  randomBeer {
    # if we request ID, the cache will be auto merged and no infinite loop
    # id
    origin
  }
}

I will file a PR to update the warning message later ;-)

[forresty]

This circles back to the core problem: Apollo Client doesn't know your Schema. Your Schema could be Megabytes big, and you don't want to ship that to your users (or, in some cases might not want to expose it at all). So marking a field in the schema so the client has to request it would not work - because the client would never see that "mark". Likewise, the server cannot just send data that has not been requested by the client - the client would just discard it, as per spec it wouldn't be allowed to use it (and the server would already violate the spec by returning it).

Again, great explanation. Thanks!

[comp615]

We encountered the issue @forresty highlighted above with Apollo Client recently although it seems like it'd been latent for a while. Currently, it seems like the workaround (or mandatory behavior perhaps) as noted is to request id (or rather the keyFields?) on every query even if it's not needed.

I tried reading various docs and questions, but don't totally understand what's happening there and want to understand better. In our case, we don't seem to get a warning message in the client. It would seem impossible to merge a cache with no primary key, so it feels like the incoming result should just be discarded. Indeed, inspecting the cache after such a query I don't see it updated...but then why do the query hooks trigger each other if they aren't actually updating the cache?