Refetch should not trigger a field merge

[vigie]

Intended outcome:

From the docs for refetch:

Update the variables of this observable query, and fetch the new results

It seems wrong to me that when the new results come back it triggers the field's merge function. As we are resetting the variables the previous value for the query should be considered invalid and thrown out implicitly.

If there is not agreement on the above proposition, at the very least I would like to have a way to know that I'm in a merge function as the result of a refetch, so I can throw out the previous result explicitly.

Actual outcome:

The field's merge function is triggered and new results are merged with the old, creating an erroneous cache entry that results from a combination of old variable values and new.

How to reproduce the issue:

This is default behavior, trigger a refetch on a query whose result has a custom field merge policy. The problem is most easily identified in the context of list type fields and pagination.

Versions

  System:
    OS: macOS 10.15.7
  Binaries:
    Node: 12.18.2 - ~/.nvm/versions/node/v12.18.2/bin/node
    npm: 6.14.5 - ~/.nvm/versions/node/v12.18.2/bin/npm
  Browsers:
    Chrome: 87.0.4280.88
    Firefox: 79.0
    Safari: 14.0.2
  npmPackages:
    @apollo/client: 3.0.0-rc.4 => 3.0.0-rc.4
    apollo-angular: ^2.0.0-beta.2 => 2.0.0-beta.2

[adsee42]

Any update on this?

[JeffJankowski]

Also encountering this issue. I am using the new type policies and merge for pagination, but I also need to be able to fetch fresh results from the network and replace the old data. I agree with @vigie that a refetch should evict the old cache entry, or at the very least notify the merge function so we can handle it explicitly.

[vigie]

Btw for anyone hitting this issue I found the following acceptable workaround: use fetchMore instead of refetch. When using fetchMore you can pass an argument that only persists for that one call and is not written and persisted to the query like it would be with refetch. I pass dropCache: true and interpret it as necessary in the merge function.

Perhaps the maintainers want to argue that this is in fact the correct solution..? It does however call into question what the legitimate use cases for refetch are...

[JeffJankowski]

Btw for anyone hitting this issue I found the following acceptable workaround: use fetchMore instead of refetch. When using fetchMore you can pass an argument that only persists for that one call and is not written and persisted to the query like it would be with refetch. I pass dropCache: true and interpret it as necessary in the merge function.

Perhaps the maintainers want to argue that this is in fact the correct solution..? It does however call into question what the legitimate use cases for refetch are...

For those of us using TypeScript and query variable types generated from apollo client:codegen, this is a pretty painful hack. 😕

[adsee42]

My workaround is to use fetchMore instead of refetch, and check offset in merge function.
If offset === 0, return incoming, else, do custom merge.

No extra parameter is needed😋

[gaurabsarkar]

I had similar issue. Refetch would not call merge in TypePolicy and would not update cache. I have an infinite loading list and this is how I solved it.

Hope this helps!

[Daavidaviid]

I have the same issue when useQuery is called multiple times and there is a merge policy, like when I display a page that has already been displayed previously in a React Native app.
The first time it works great, I can fetch more, I can refetch the page, no issue, but when go back and go to the same page again, it breaks.

Still looking for a clean way to solve this

[jtoce]

Now that I’ve looked under the hood I can tell you why. When you use fetchMore with the new field policies, it writes straight to the cache.

apollo-client/src/core/ObservableQuery.ts

Lines 316 to 325 in 4fd6824

	// If we're using a field policy instead of updateQuery, the only
	// thing we need to do is write the new data to the cache using
	// combinedOptions.variables (instead of this.variables, which is
	// what this.updateQuery uses, because it works by abusing the
	// original field value, keyed by the original variables).
	this.queryManager.cache.writeQuery({
	query: combinedOptions.query,
	variables: combinedOptions.variables,
	data,
	});

However... When you call refetch it instead performs the fetch (usually defaulting back to fetchPolicy: 'network-only') and then it does a comparison of your previous lastWrite to see if anything has changed before writing to the cache.

apollo-client/src/core/QueryInfo.ts

Lines 327 to 364 in 4fd6824

	// If result is the same as the last result we received from
	// the network (and the variables match too), avoid writing
	// result into the cache again. The wisdom of skipping this
	// cache write is far from obvious, since any cache write
	// could be the one that puts the cache back into a desired
	// state, fixing corruption or missing data. However, if we
	// always write every network result into the cache, we enable
	// feuds between queries competing to update the same data in
	// incompatible ways, which can lead to an endless cycle of
	// cache broadcasts and useless network requests. As with any
	// feud, eventually one side must step back from the brink,
	// letting the other side(s) have the last word(s). There may
	// be other points where we could break this cycle, such as
	// silencing the broadcast for cache.writeQuery (not a good
	// idea, since it just delays the feud a bit) or somehow
	// avoiding the network request that just happened (also bad,
	// because the server could return useful new data). All
	// options considered, skipping this cache write seems to be
	// the least damaging place to break the cycle, because it
	// reflects the intuition that we recently wrote this exact
	// result into the cache, so the cache *should* already/still
	// contain this data. If some other query has clobbered that
	// data in the meantime, that's too bad, but there will be no
	// winners if every query blindly reverts to its own version
	// of the data. This approach also gives the network a chance
	// to return new data, which will be written into the cache as
	// usual, notifying only those queries that are directly
	// affected by the cache updates, as usual. In the future, an
	// even more sophisticated cache could perhaps prevent or
	// mitigate the clobbering somehow, but that would make this
	// particular cache write even less important, and thus
	// skipping it would be even safer than it is today.
	if (this.diff && this.diff.complete) {
	// Reuse data from the last good (complete) diff that we
	// received, when possible.
	result.data = this.diff.result;
	return;
	}

Here's the problem- lastWrite is only updated on the initial fetch, but diff is updated by fetchMore. So for example- if you use pagination (i.e. page, size) and nothing has changed on your first page of results, then the logic of shouldWrite is false since both your variables and data are the same. So it returns you the entire cache diff instead of your new network results. The obvious problem of comparing the initial fetch (i.e. page one) and then deciding to return all pages in the cache, is that something might have changed on later pages.

[benjamn]

@jtoce Thanks for digging into that! I think this should be fixable, given your analysis.

[benjamn]

Here's my current thinking on the original issue, as reported by @vigie and echoed by @JeffJankowski:

If you define a merge function for a field, InMemoryCache must call that function any time new data is written for that field, even the very first time (when the existing data is undefined), because merge functions give you the ability to customize the internal representation of field data within the cache. If merge was called most of the time but not all the time, it would be very difficult to know what type of data to expect when reading from the cache.

However, I believe we can preserve this guarantee (that merge is always called) while achieving the behavior @vigie is proposing, by providing a way to call merge functions with undefined existing data, so they behave exactly as they would if there was no existing data, and thus do not attempt to combine incoming data with existing data (but still potentially transform the incoming data, as appropriate).

My current plan is to add an overwrite?: boolean option to the cache.writeQuery and cache.writeFragment option types. It will be false by default, but passing overwrite: true will prevent any merge functions involved in the write from seeing any existing data, thereby overwriting those field values. For fields that do not have a merge function, new field data always replaces existing data, so you can think of overwrite: true as a way to achieve that default behavior even when you have a merge function.

Of course, since you don't manually call cache.writeQuery when you're using refetch, we will need to arrange for overwrite: true to be passed behind the scenes, when refetch does its cache write. I agree this should be the default behavior for refetch, but we may need to make it optional (opt-in) to avoid breaking changes.

I welcome your thoughts/questions about this idea. It is straightforward to implement, but needs more unit tests. I'll kick off a PR tomorrow, with the goal of releasing it in Apollo Client 3.4 (and in the next beta release, more immediately).

The issue @jtoce identified in #7491 (comment) may well be a contributing factor (preventing writes in some cases, thereby not triggering any merge functions), but that should be easy to fix by deleting queryInfo.lastWrite before refetching. I'll put that change in the PR too.