Add failing test of updating nested inline array fields #3419
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
added
the
Generated by 🚫 dangerJS |
dferber90
commented
May 7, 2018
| return undefined; | ||
| }; | ||
|
|
||
| writeQueryToStore({ |
There was a problem hiding this comment.
On the first write, all goes according to plan.
| }, | ||
| }); | ||
|
|
||
| writeQueryToStore({ |
There was a problem hiding this comment.
On the second write, we remove the first animal. However things go downward when trying to write this data to the store, as the store does not get updated correctly.
| }, | ||
| }, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Here is where the test fails.
The received value is
{
"$Shelter__some-id.animals.0.species": undefined,
"$Shelter__some-id.animals.1.species": {
"name": "dog"
},
"ROOT_QUERY": {
"shelter": {
"generated": false,
"id": "Shelter__some-id",
"type": "id",
"typename": "Shelter"
}
},
"Shelter__some-id": {
"animals": [{
"generated": true,
"id": "Shelter__some-id.animals.0",
"type": "id",
"typename": "Animal"
}],
"id": "some-id"
},
"Shelter__some-id.animals.0": {
"name": "Dieter",
"species": {
"generated": true,
"id": "$Shelter__some-id.animals.0.species",
"type": "id",
"typename": "Dog"
}
},
"Shelter__some-id.animals.1": {
"name": "Dieter",
"species": {
"generated": true,
"id": "$Shelter__some-id.animals.1.species",
"type": "id",
"typename": "Dog"
}
}
}The actual difference being:
{
- "$Shelter__some-id.animals.0.species": undefined,
- "$Shelter__some-id.animals.1.species": {
+ "$Shelter__some-id.animals.0.species": {
"name": "dog"
},
"ROOT_QUERY": {
|
I was able to dumb it down some more. The following fails as well: Show failing test caseit("should updated inlined array fields", () => {
const store = defaultNormalizedCacheFactory();
const query = gql`
query {
animals {
species {
name
}
}
}
`;
const dataIdFromObject = (object: any) => {
if (object.__typename && object.id) {
return object.__typename + "__" + object.id;
}
return undefined;
};
writeQueryToStore({
query,
dataIdFromObject,
result: {
animals: [
{
__typename: "Animal",
name: "Carol",
species: {
__typename: "Cat",
name: "cat"
}
},
{
__typename: "Animal",
name: "Dieter",
species: {
__typename: "Dog",
name: "dog"
}
}
]
},
store
});
expect(store.toObject()).toEqual({
"$ROOT_QUERY.animals.0.species": { name: "cat" },
"$ROOT_QUERY.animals.1.species": { name: "dog" },
ROOT_QUERY: {
animals: [
{
generated: true,
id: "ROOT_QUERY.animals.0",
type: "id",
typename: "Animal"
},
{
generated: true,
id: "ROOT_QUERY.animals.1",
type: "id",
typename: "Animal"
}
]
},
"ROOT_QUERY.animals.0": {
species: {
generated: true,
id: "$ROOT_QUERY.animals.0.species",
type: "id",
typename: "Cat"
}
},
"ROOT_QUERY.animals.1": {
species: {
generated: true,
id: "$ROOT_QUERY.animals.1.species",
type: "id",
typename: "Dog"
}
}
});
writeQueryToStore({
query,
dataIdFromObject,
result: {
animals: [
// First element deleted -> nested "type" field has GraphQL Type change
{
__typename: "Animal",
name: "Dieter",
species: {
__typename: "Dog",
name: "dog"
}
}
]
},
store
});
// This shows the actual output. Notice the "not".
// The correct output should be something else.
//
// Especially the species of the first result is lost, as it is `undefined`
expect(store.toObject()).not.toEqual({
"$ROOT_QUERY.animals.0.species": undefined,
"$ROOT_QUERY.animals.1.species": { name: "dog" },
ROOT_QUERY: {
animals: [
{
generated: true,
id: "ROOT_QUERY.animals.0",
type: "id",
typename: "Animal"
}
]
},
"ROOT_QUERY.animals.0": {
species: {
generated: true,
id: "$ROOT_QUERY.animals.0.species",
type: "id",
typename: "Dog"
}
},
"ROOT_QUERY.animals.1": {
species: {
generated: true,
id: "$ROOT_QUERY.animals.1.species",
type: "id",
typename: "Dog"
}
}
});
}); |
|
I digged some more and the culprit seems to be here: Using the second failing test example from this comment, The store looks like this before the deletion {
"$ROOT_QUERY.animals.0.species": {
"name": "dog"
},
"$ROOT_QUERY.animals.1.species": {
"name": "dog"
},
"ROOT_QUERY": {
animals: [
{ generated: true, id: "ROOT_QUERY.animals.0", type: "id", typename: "Animal" },
{ generated: true, id: "ROOT_QUERY.animals.1", type: "id", typename: "Animal" }
]
},
"ROOT_QUERY.animals.0": {
species: { generated: true, type: "id", id: "$ROOT_QUERY.animals.0.species", typename: "Cat" },
},
"ROOT_QUERY.animals.1": {
species: { generated: true, type: "id", id: "$ROOT_QUERY.animals.1.species", typename: "Dog" },
}
}So When commenting out The issue seems to be that we're deleting after writing (or deleting the wrong thing). |
|
Based on these findings, I've come up with an even simpler test case Simpler case it("should updated nested fields of mixed types within arrays", () => {
const store = defaultNormalizedCacheFactory();
const query = gql`
query {
animals {
species {
name
}
}
}
`;
writeQueryToStore({
query,
result: {
animals: [
{
__typename: "Animal",
species: {
__typename: "Cat",
name: "cat"
}
}
]
},
store
});
expect(store.toObject()).toEqual({
"$ROOT_QUERY.animals.0.species": { name: "cat" },
ROOT_QUERY: {
animals: [
{
generated: true,
id: "ROOT_QUERY.animals.0",
type: "id",
typename: "Animal"
}
]
},
"ROOT_QUERY.animals.0": {
species: {
generated: true,
id: "$ROOT_QUERY.animals.0.species",
type: "id",
typename: "Cat"
}
}
});
writeQueryToStore({
query,
result: {
animals: [
{
__typename: "Animal",
species: {
__typename: "Dog",
name: "dog"
}
}
]
},
store
});
expect(store.toObject()).not.toEqual({
// ⚠️ This should not be undefined
"$ROOT_QUERY.animals.0.species": undefined,
ROOT_QUERY: {
animals: [
{
generated: true,
id: "ROOT_QUERY.animals.0",
type: "id",
typename: "Animal"
}
]
},
"ROOT_QUERY.animals.0": {
species: {
generated: true,
id: "$ROOT_QUERY.animals.0.species",
type: "id",
typename: "Dog"
}
}
});
}); |
|
I think I actually figured out how to solve this one. if (escapedId.generated) {
generatedKey = escapedId.id;
// we should only merge if it's an object of the same type
// otherwise, we should delete the generated object
if (typenameChanged) {
- store.delete(generatedKey);
+ // remove the old generated value in case the old value was
+ // inlined and the new value is not, which is indicated by
+ // the old id being generated and the new id not having been generated
+ if (!generated) {
+ store.delete(generatedKey);
+ }
} else {
shouldMerge = true;
}
}It seems that the cache attempted to do cleanup of a generated object in case the reference to it gets replaced with one where we're actually provided a non-generated id. However, this also ran when a generated object's type changed, thus removing the new generated object by accident. We also need to adapt the const expStoreWithAuthor = defaultNormalizedCacheFactory({
+ "$ROOT_QUERY.author": undefined,
Author__129: {And that's all. I've found another case where the cache stays polluted with unreferenced objects which happens when removing an item form an array which contained an inlined reference. This happens even after this fix. It seems unrelated to this PR though, maybe it's even intentional. Reproduction for case where unreferenced elements stay in cache it("should remove unreferenced items from the cache", () => {
const store = defaultNormalizedCacheFactory();
const query = gql`
query {
animals {
species {
name
}
}
}
`;
const dataIdFromObject = (object: any) => {
if (object.__typename && object.id) {
return object.__typename + "__" + object.id;
}
return undefined;
};
writeQueryToStore({
query,
dataIdFromObject,
result: {
animals: [
{
__typename: "Animal",
name: "Carol",
species: {
__typename: "Cat",
name: "cat"
}
},
{
__typename: "Animal",
name: "Dieter",
species: {
__typename: "Dog",
name: "dog"
}
}
]
},
store
});
expect(store.toObject()).toEqual({
"$ROOT_QUERY.animals.0.species": { name: "cat" },
"$ROOT_QUERY.animals.1.species": { name: "dog" },
ROOT_QUERY: {
animals: [
{
generated: true,
id: "ROOT_QUERY.animals.0",
type: "id",
typename: "Animal"
},
{
generated: true,
id: "ROOT_QUERY.animals.1",
type: "id",
typename: "Animal"
}
]
},
"ROOT_QUERY.animals.0": {
species: {
generated: true,
id: "$ROOT_QUERY.animals.0.species",
type: "id",
typename: "Cat"
}
},
"ROOT_QUERY.animals.1": {
species: {
generated: true,
id: "$ROOT_QUERY.animals.1.species",
type: "id",
typename: "Dog"
}
}
});
writeQueryToStore({
query,
dataIdFromObject,
result: {
animals: [
// First element deleted -> nested "type" field has GraphQL Type change
{
__typename: "Animal",
name: "Dieter",
species: {
__typename: "Dog",
name: "dog"
}
}
]
},
store
});
expect(store.toObject()).not.toEqual({
"$ROOT_QUERY.animals.0.species": { name: "dog" },
"$ROOT_QUERY.animals.1.species": { name: "dog" },
ROOT_QUERY: {
animals: [
{
generated: true,
id: "ROOT_QUERY.animals.0",
type: "id",
typename: "Animal"
}
]
},
"ROOT_QUERY.animals.0": {
species: {
generated: true,
id: "$ROOT_QUERY.animals.0.species",
type: "id",
typename: "Dog"
}
},
"ROOT_QUERY.animals.1": {
species: {
generated: true,
id: "$ROOT_QUERY.animals.1.species",
type: "id",
typename: "Dog"
}
}
});
}); |
|
I opened another PR with a clean slate and without all the noise: #3422 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is an issue report in the form of a failing test case. Hope that is okay. If preferred, I can open a regular issue instead.
Our app crashes when Apollo has to update the store's data. Specifically, when it has to update an inlined, nested selection of a changing type in an array.
In this case
shelter.animalsis an array containing a fieldspecieswith a varying type, namely eitherCatorDog.The app crashes only when the update replaces an existing entry at an index where the previous entry had a different type than the new entry. The app crashes because Apollo writes the wrong data to the cache and subsequent reads seem to fail because of that.
So, when
[{ species: Cat}, { species:Dog }]gets replaced by[{ species:Dog }]. Then the type ofspeciesat index 0 changes. This leads to Apollo writing the wrong data to the cache.In the other cases (
[{ species: Cat}, { species: Dog }]gets replaced by[{ species: Cat }]) the data seems to get corrupted, but without causing a crash.Intended outcome:
I would expect the data to get normalised correctly. I am not sure which data Apollo should write exactly, but I know that the currently written data is wrong.
Maybe something like:
Actual outcome:
🛑 Apollo writes to the cache, but it loses the data on the type nested in the array:
"$Shelter__some-id.animals.0.species"should not beundefined. It should be{ "name": "dog" }."$Shelter__some-id.animals.1.species"should probably not exist. Not sure about this.How to reproduce the issue:
The contained failing test shows how to reproduce the issue in code.
For an actual user flow, either of these might lead to this situation:
apollo-cache-inmemoryA simpler reproduction:
writeQuerychanges the dataapollo-cache-inmemoryVersion