DocumentNode passed to refetchQueries are compared by references

[charpeni]

Issue Description

We recently noticed that refetchQueries was acting strangely, it was inconsistent and did not work in all locations.

We use a combination of @apollo/client and GraphQL Code Generator with client-preset and we pass the DocumentNode directly to the mutation's refetchQueries function. We noticed that in some locations, it was working fine, but in other locations, using the exact same DocumentNode wasn't working as expected and leading to the following warnings: Unknown query X requested in refetchQueries options.include array. Using Apollo Dev Tools confirmed the expected query was still active.

Investigation Notes

I started investigating to understand why it was working half the time and found some really interesting things. Most of the relevant logic happens in QueryManager class, more specifically, the getObservableQueries method.

The first thing happening is that we declare a new Map (queryNamesAndDocs) which its keys will be containing string (query name) | DocumentNode:

apollo-client/src/core/QueryManager.ts

Line 856 in 77b8931

const queryNamesAndDocs = new Map<string | DocumentNode, boolean>();

Then, we loop over include, which is basically what we passed to refetchQueries, to build the previous Map (queryNamesAndDocs):

apollo-client/src/core/QueryManager.ts

Lines 859 to 869 in 77b8931

	if (Array.isArray(include)) {
	include.forEach((desc) => {
	if (typeof desc === "string") {
	queryNamesAndDocs.set(desc, false);
	} else if (isDocumentNode(desc)) {
	queryNamesAndDocs.set(this.transform(desc), false);
	} else if (isNonNullObject(desc) && desc.query) {
	legacyQueryOptions.add(desc);
	}
	});
	}

My understanding is that we build a Map of queries, and we start with the one we passed to refetchQueries, but we default the boolean (the value, which is whether they are included or not) to false as they weren't checked yet. As of the key, we rely on DocumentTransform to transform it before using the whole DocumentNode as a key.

Then, we loop over this.queries and look for matches between queryName and document in queryNamesAndDocs:

apollo-client/src/core/QueryManager.ts

Lines 871 to 900 in 77b8931

	this.queries.forEach(({ observableQuery: oq, document }, queryId) => {
	if (oq) {
	if (include === "all") {
	queries.set(queryId, oq);
	return;
	}
	const {
	queryName,
	options: { fetchPolicy },
	} = oq;
	if (
	fetchPolicy === "standby" ||
	(include === "active" && !oq.hasObservers())
	) {
	return;
	}
	if (
	include === "active" ||
	(queryName && queryNamesAndDocs.has(queryName)) ||
	(document && queryNamesAndDocs.has(document))
	) {
	queries.set(queryId, oq);
	if (queryName) queryNamesAndDocs.set(queryName, true);
	if (document) queryNamesAndDocs.set(document, true);
	}
	}
	});

I'm particularly interested in the following logic:

apollo-client/src/core/QueryManager.ts

Lines 890 to 899 in 77b8931

	if (
	include === "active" ||
	(queryName && queryNamesAndDocs.has(queryName)) ||
	(document && queryNamesAndDocs.has(document))
	) {
	queries.set(queryId, oq);
	if (queryName) queryNamesAndDocs.set(queryName, true);
	if (document) queryNamesAndDocs.set(document, true);
	}
	}

If we were passing string values to refetchQueries, then we would be relying on queryName, but as we're passing DocumentNode, it means we rely on document instead. We previously saw that we created an entry in the Map with the DocumentNode transformed as the key. So, that's where I added breakpoints to understand what's going on.

I double-checked to make sure that the query I passed to refetchQueries was still active, and it was the case, then I checked that specific condition:

apollo-client/src/core/QueryManager.ts

Line 893 in 77b8931

(document && queryNamesAndDocs.has(document))

It turns out that the inconsistency comes from that exact condition. When everything works, then queryNamesAndDocs.has(document) returns true, but in some cases, it returns false. I looked at both objects, and they were identical, but it wasn't enough for the condition. We need to have the exact same DocumentNode reference, otherwise, the object key won't be found in queryNamesAndDocs:

 // Returns `false`, as they aren't the same reference
queryNamesAndDocs.entries().next().value[0] === document;

// Returns `true`, they don't have the same reference, but they have the same values
JSON.stringify(queryNamesAndDocs.entries().next().value[0]) === JSON.stringify(document);

We can easily reproduce this by making sure we're passing a new instance of a DocumentNode:

await mutation({ variables: {}, refetchQueries: [JSON.parse(JSON.stringify(SomeQueryDocumentNode))] });

Will be producing: Unknown query X requested in refetchQueries options.include array.

See the reproduction in 🔗 CodeSandbox.

I haven't been able to figure out why they won't have the same reference. I made sure we were only using one single instance of Apollo Client. I tagged both DocumentNode with a special key, I took a heap snapshot and was able to confirm that in multiple locations, they were the same object reference but something in between is changing that.

I found this issue: #5419, where multiple people described a similar issue to what we experienced—not directly related to the top-level message, but down there, where everything is getting mixed, people mentioned similar issues and especially that it works fine locally, but not in production. I can think of multiple contributing factors to this issue, but they all come to the fact that we use the DocumentNode as a key and then compare it by reference.

I wonder if one major contributing factor could be how bundle splitting is configured and how we can end up with the same DocumentNode contained in multiple bundles and therefore, it's getting complicated to guarantee that for a given query, we'll always reuse the same instance of a DocumentNode. The mentioned issue above also suggested to either passing queries as a string or by using the legacy object: { query, variables }, but I would like to avoid the last option at all cost because it forces us to pass the exact same variables that were used when we should just be refetching the latest variables used.

Workaround

There's a workaround available to make it work all the time, it's not convenient and I would like this to make it bake in in Apollo Client, but in the meantime, using the DocumentNode and extracting the query name can be achieved via getOperationName(DocumentNode), so you don't have to use magic strings in your code to refer to queries:

import { getOperationName } from '@apollo/client/utilities';
// ...

await mutation({ variables: {}, refetchQueries: [getOperationName(SomeQueryDocumentNode)] });

Potential Solutions

This led me to brainstorm on the following solutions...

Note

I would be happy to send a pull request or at least, start one, but I wanted to confirm which direction to take first.

Retrieve the operation name from the DocumentNode and use it as a key (same as queryName)

Do we even need to store the whole DocumentNode object as a key? We could reduce complexity by removing it and reusing the workaround suggested above, but internally, within refetchQueries's logic.

E.g., we could still accept both string | DocumentNode, but refactor the following logic to extract the query name from the DocumentNode via getOperationName:

if (Array.isArray(include)) {
  include.forEach((desc) => {
    if (typeof desc === "string") {
      queryNamesAndDocs.set(desc, false);
    } else if (isDocumentNode(desc)) {
-     queryNamesAndDocs.set(this.transform(desc), false);
+     queryNamesAndDocs.set(getOperationName(this.transform(desc)), false);
    } else if (isNonNullObject(desc) && desc.query) {
      legacyQueryOptions.add(desc);
    }
  });
}

if (
  include === "active" ||
  (queryName && queryNamesAndDocs.has(queryName)) ||
- (document && queryNamesAndDocs.has(document))
+ (document && queryNamesAndDocs.has(getOperationName(document)))
) {
  queries.set(queryId, oq);
  if (queryName) queryNamesAndDocs.set(queryName, true);
- if (document) queryNamesAndDocs.set(document, true);
+ if (document) queryNamesAndDocs.set(getOperationName(document), true);
}

Compare by value, not by reference

Warning

I don't believe this is particularly a good idea, but I had to share about it because the whole issue is the comparison point and we could be stuck here in case we need to keep DocumentNode as keys.

Another option could be to store the object as a string, where we could use JSON.stringify before storing the value:

if (Array.isArray(include)) {
  include.forEach((desc) => {
    if (typeof desc === "string") {
      queryNamesAndDocs.set(desc, false);
    } else if (isDocumentNode(desc)) {
-     queryNamesAndDocs.set(this.transform(desc), false);
+     queryNamesAndDocs.set(JSON.stringify(this.transform(desc)), false);
    } else if (isNonNullObject(desc) && desc.query) {
      legacyQueryOptions.add(desc);
    }
  });
}

So we can either compare it as a string or convert it back to an object via JSON.parse, but I would be concerned about performance issues when calling JSON.stringify and JSON.parse back and forth, and also because queries length are unpredictable, they could be quite small or quite big.

Link to Reproduction

https://codesandbox.io/p/devbox/apollo-client-refetchqueries-issue-f626v9?workspaceId=a02ec0ab-7d00-4b4d-b865-a63aba3fe944

Reproduction Steps

No response

@apollo/client version

3.11.4

[jerelmiller]

Hey @charpeni 👋

Thanks for that thorough write-up and deep dive into the code! It is certainly interesting that you're seeing intermittent behavior here. The only thing I could possibly think of on the client end that might play a factor is document transforms. We have a default transform that we use to add __typename throughout the document:

apollo-client/src/core/QueryManager.ts

Lines 152 to 156 in 77b8931

	const defaultDocumentTransform = new DocumentTransform(
	(document) => this.cache.transformDocument(document),
	// Allow the apollo cache to manage its own transform caches
	{ cache: false }
	);

In those cases where you're getting a mismatch (i.e. the "Unknown query" warning), I'd be curious if the the stored document is the transformed one or your original document. You can use the graphql.js print function to output the string representation which should make it easier to compare the document you give refetchQueries and the documents that are traversed in the internal logic.

import { print } from 'graphql';

print(document) // => "query MyQuery { ... }"

You said though that using JSON.parse/stringify tends to return true here, so I would suspect this isn't quite it. Very strange indeed!

Would you be willing to check the above case to see if the default document transform plays a role here or not?

On your solutions:

Retrieve the operation name from the DocumentNode

While I like this idea, this solution unfortunately won't handle anonymous queries like the raw DocumentNode can. If you had more than 1 anonymous query in your application, you'd quickly run into issues where its going to fetch whichever anonymous query happens to be stored first.

That said, I like your idea about comparing strings rather than relying on referential equality with raw DocumentNodes. What if in the case of a document node, we use the GraphQL string as the key in this map? Apollo Client has an internal print function that adds caching on top of the native graphql.js print function so you would only pay the performance penalty the first time the document is printed. This would allow anonymous queries to continue to function as they do today.

Is this something you'd want to try out?

[charpeni]

Oh, thanks for mentioning print function, I'll have a try next week!

Now that you mentioned the document transform could cause it, I made sure we don't have other document transforms that could be conflicting. The only other document transform we have is configured within the GraphQL Code Generator, where we automatically query for the id field if it exists, but it's done via the codegen and shouldn't be impacting Apollo Client.

But I found something interesting that I just remembered I should mention here. It seems like the DocumentTransform instance in QueryManager could be setup to cache document transforms, meaning that for a given DocumentNode, it should produce the same output reference. I don't have the setup in front of me, but if I remember correctly, the caching layer was set to false. If that's truly the case, I wonder if it could be the document transform returning new references for the same DocumentNode.

Ah, the internal print function is really interesting, especially since it's built on AutoCleanedWeakCache. Good catch about anonymous queries! I'll give it a shot next week to see where it lands.

[jerelmiller]

It seems like the DocumentTransform instance in QueryManager could be setup to cache document transforms, meaning that for a given DocumentNode, it should produce the same output reference

It's a bit difficult to see, but that this.cache.transformDocument(document) within the default document transform in QueryManager relies on the cache.transformDocument function to cache the result of its work. If you trace enough of the code, you'll see that the document transform used in InMemoryCache uses a cached document transform:

apollo-client/src/cache/inmemory/inMemoryCache.ts

Line 49 in 77b8931

private addTypenameTransform = new DocumentTransform(addTypenameToDocument);

The only reason we disable the document transform cache in the QueryManager's defaultDocumentTransform is because we have no idea how a third party cache abstraction handles transformations and may do so in an unsafe way. Ideally its idempotent, but we can't guarantee that, hence why we move the responsibility of caching the cache's document transform to the cache itself. Hopefully that helps provide some background 🙂

So as long as you're using InMemoryCache, you should still have caching on that document transform.

Hope the experimentation with print works well! Looking forward to hearing back how that works 🎉.

[github-actions]

Do you have any feedback for the maintainers? Please tell us by taking a one-minute survey. Your responses will help us understand Apollo Client usage and allow us to serve you better.