-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Two queries loop forever when requesting different parts of same object #6370
Comments
@nordvall I am getting same issue with v3.0.0-beta.50. We are prefetching (for caching) some data with two independent queries. So to resume the situation here below the state logs:
|
@nordvall Can you tell me more about this I can see that the desired behavior is for the fields of this To achieve the first behavior (global singleton), you can give 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 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. |
Speaking of warnings, I would also like to draw your attention to PR #6372, which would have given the following warning in your case:
Unless you immediately guessed the nature of the problem, I imagine this warning would have been pretty helpful? |
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 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). |
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. |
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. |
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 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
}
} |
Solves infinite loops when more than one query with different fields on an unidentified object Fixes apollographql#6370
@thomassuckow The solution to that specific problem is to specify a custom 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. |
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. |
I abandoned the pull request. I am working on updating all our types to include id fields and hacking the server (Wrap 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. |
Ever since the big refactoring in #6221 made the client more aggressive about triggering network requests in response to incomplete cache data (when using a cache-first FetchPolicy), folks testing the betas/RCs have reported that feuding queries can end up triggering an endless loop of unhelpful network requests. This change does not solve the more general problem of queries competing with each other to update the same data in incompatible ways (see #6372 for mitigation strategies), but I believe this commit will effectively put a stop to most cases of endless network requests. See my lengthy implementation comment for more details, since this is a non-obvious solution to a very tricky problem. Fixes #6307. Fixes #6370. Fixes #6434. Fixes #6444.
Judging by the provided reproduction (thanks!), |
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. |
@benjamn Can this become part of the official docs? All I'm seeing here is this:
Would never guess that |
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:
permissions
part of at least one of the queriesHow 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
The text was updated successfully, but these errors were encountered: