Context.overwrite truthy so existing array always voided and so undefined when arg in merge method of apollo cache

[atomless]

Hi All.

This feels like a bug in apollo client or in the inMemoryCache but could of course be something I'm doing wrong. Basically, using the chrome dev tools extension for apollo I can see that for the uuid of the outgoing/incoming request there are already multiple cached results but by the time code execution reaches the merge method invoked by Apollo Cache, the "existing" arg is always undefined.

Intended outcome:
I'm using Apollo Cache and want to write custom code to conditionally merge incoming results with existing cached results.

Actual outcome:
The existing results arg in the merge method invoked by Apollo Cache is always undefined, even though I can see matching entries in the cache using the apollo extension in chrome dev tools. Also if I break in the code and step back up the execution stack I can see that existing did have entries but was voided as "context.overwite" is truthy without me setting it so.

How to reproduce the issue:
When I set a breakpoint in the merge function and then step back in the execution stack I can see that "existing" DOES have entries but it is voided when execution reaches the apollo client file policies.ts on lines 862 to 664, due to the context.overwrite boolean being truthy:

    // If cache.writeQuery or cache.writeFragment was called with
    // options.overwrite set to true, we still call merge functions, but
    // the existing data is always undefined, so the merge function will
    // not attempt to combine the incoming data with the existing data.
    if (context.overwrite) {
      existing = void 0;
    }

After execution of the lines above, when console logging "existing" in the merge function below, it's value is ALWAYS undefined. I don't set the overwrite option anywhere and it's default is supposed to be false. What am I missing, or is this a bug?
Here's my basic config and the merge method in question.

new ApolloClient({ cache: new InMemoryCache({
    typePolicies: {
        DataResult: {
            keyFields: ["UUID", "isDelta"] 
        },
        Query: {
            fields: {
                getData: {
                    keyArgs: ["request", ["UUID"]],
                    merge(existing, incoming, { readField, args, toReference }){
                        console.log(existing);
                    }
                }
            }
        }
    }
}), ApolloLink.from([
        new RetryLink({
            delay: {
                initial: 300,
                max: 30000,
                jitter: false
            },
            attempts: {
                max: retry_max,
                retryIf
            }
        }),
        new BatchHttpLink({
            uri,
            batchInterval: batch_interval,
            headers: headers
        })
    ])
});

Versions
"@apollo/client": "3.5.10",
"graphql": "16.0.1",
"react": "17.0.2",
System:
OS: macOS 10.15.7
Binaries:
Node: 16.13.0 - ~/.nvm/versions/node/v16.13.0/bin/node
npm: 8.1.0 - ~/.nvm/versions/node/v16.13.0/bin/npm
Browser:
Chrome: Version 99.0.4844.74 (Official Build) (x86_64)

[benjamn]

I suspect context.overwrite is true because a refetch his happening, and PR #7966 (v3.4.0) made overwriting the default behavior for refetched data. The rest of this comment leans into this assumption, so please feel free to ignore what I've written below if you have good evidence this problem is happening for all fetches (not just fetches triggered by the refetch function).

The goal of this overwriting after refetch is to replace existing data with refetched data without combining them together, since the refetched data is typically more fresh/complete/recent than the existing data. However, it's still important to call merge functions for any fields that are being replaced, since merge functions are supposed to be able to dictate the internal format of the cached field data, so calling them only sometimes is not a good idea (see #7491 (comment)). To make that work, we call merge functions with undefined existing data after refetches, by passing overwrite: true by default to cache.writeQuery after the refetch.

With that explanation of the default behavior out of the way, some (hopefully) good news: this behavior is configurable! You can pass an option called refetchWritePolicy in your useQuery or client.watchQuery options, and it can be either "overwrite" (which ignores existing data) or "merge" (which allows existing data to be combined with incoming data, as it sounds like you'd prefer). An obscure option for sure, but one we added just for situations like this (if my hypothesis holds).

If you want to make the pre-v3.4 behavior ("merge") default again, you can use defaultOptions.watchQuery.refetchWritePolicy:

new ApolloClient({
  defaultOptions: {
    watchQuery: {
      // We recommend "overwrite" instead of "merge", but here's how
      // you can restore the "merge" behavior by default for all queries.
      refetchWritePolicy: "merge",
    },
  },
})

Again, I'm not 100% sure about my initial diagnosis, so please give refetchWritePolicy a try when you have time, and let us know how much of the problem that solves?

[atomless]

@benjamn you absolute diamond!
The addition of refetchWritePolicy as shown in snippet below appears to have fixed it for me.
I started cursing myself for missing that in the docs but it doesn't appear to have made it into the docs yet??

const query_options = useMemo(() => ({
        onCompleted: onResultCallback, fetchPolicy: fetch_policy, refetchWritePolicy: "merge", notifyOnNetworkStatusChange: true, returnPartialData: false, errorPolicy: "all"
    }), [fetch_policy, onResultCallback]);

const [sendRequest, { error }] = useLazyQuery(gql`${query}`, { ...query_options, variables });
// ...
// later on in execution we call sendRequest with updated request variables like so...
sendRequest({ variables: variables_ref.current });

The reason merge is important for our use case is that there are 3 types of response expected from any request with a matching UUID, the first is usually a null response that indicates the data is not yet available so polling continues, the second is a full result and the third is a delta update. We separate these out using isDelta as a keyField (see config above) as it's this result property that indicates the 3 possible result types, null, false, and true respectively. I want to add code to our merge function to only merge in non-null results. Which I should now be able to do.

Thank you so much!

[atomless]

@benjamn so as you can see in the snippet posted in my previous reply, we're not retrieving the refetch method from the lazy query hook, we use the send method, is that equivalent? Should overwrite still be true by default in this scenario?

[benjamn]

Oh, thanks for reminding me: we currently have a bug that the sendRequest function from useLazyQuery is incorrectly implemented as a refetch here, which disregards the fetchPolicy (always network-only) and also uses overwrite: true to write the results.

That bug is a top priority for me before we publish the first release candidate of v3.6, so I've added this issue to the Release 3.6 milestone to make sure I comment here once that's fixed in prerelease.

[benjamn]

@atomless The problem with the useLazyQuery execution function always triggering a refetch should have been fixed in @apollo/client@3.6.0-beta.11 (thanks to #9564), if you want to give that a try sometime soon! We're aiming for a v3.6 release candidate by the end of this week, though of course we'll take feedback any time.

Specifically, after updating to the latest v3.6 beta version, you should no longer see overwrite: true behavior when calling the useLazyQuery exec function (only when you deliberately call refetch).

[benjamn]

We've merged release-3.6 into main and released PR #9564 in @apollo/client@3.6.0 just now. That version is still tagged as next rather than latest on npm, but we will promote it to latest after some final testing. Thanks for opening this issue and for your patience waiting for the solution to land!