Merge pull request #8505 from apollographql/avoid-mistaking-reference…

…s-for-entity-objects

Multiple (3) lines of defense against merging `{ __ref }` objects
into normalized `StoreObject` entities.

src/cache/inmemory/__tests__/entityStore.ts

@@ -5,7 +5,7 @@ import { DocumentNode } from 'graphql';
 import { StoreObject } from '../types';
 import { ApolloCache } from '../../core/cache';
 import { Cache } from '../../core/types/Cache';
-import { Reference, makeReference, isReference } from '../../../utilities/graphql/storeUtils';
+import { Reference, makeReference, isReference, StoreValue } from '../../../utilities/graphql/storeUtils';
 import { MissingFieldError } from '../..';
 import { TypedDocumentNode } from '@graphql-typed-document-node/core';
 
@@ -964,6 +964,76 @@ describe('EntityStore', () => {
     expect(cache2.extract()).toEqual({});
   });
 
+  it("cache.gc is not confused by StoreObjects with stray __ref fields", () => {
+    const cache = new InMemoryCache({
+      typePolicies: {
+        Person: {
+          keyFields: ["name"],
+        },
+      },
+    });
+
+    const query = gql`
+      query {
+        parent {
+          name
+          child {
+            name
+          }
+        }
+      }
+    `;
+
+    const data = {
+      parent: {
+        __typename: "Person",
+        name: "Will Smith",
+        child: {
+          __typename: "Person",
+          name: "Jaden Smith",
+        },
+      },
+    };
+
+    cache.writeQuery({ query, data });
+
+    expect(cache.gc()).toEqual([]);
+
+    const willId = cache.identify(data.parent)!;
+    const store = cache["data"];
+    const storeRootData = store["data"];
+    // Hacky way of injecting a stray __ref field into the Will Smith Person
+    // object, clearing store.refs (which was populated by the previous GC).
+    storeRootData[willId]!.__ref = willId;
+    store["refs"] = Object.create(null);
+
+    expect(cache.extract()).toEqual({
+      'Person:{"name":"Jaden Smith"}': {
+        __typename: "Person",
+        name: "Jaden Smith",
+      },
+      'Person:{"name":"Will Smith"}': {
+        __typename: "Person",
+        name: "Will Smith",
+        child: {
+          __ref: 'Person:{"name":"Jaden Smith"}',
+        },
+        // This is the bogus line that makes this Person object look like a
+        // Reference object to the garbage collector.
+        __ref: 'Person:{"name":"Will Smith"}',
+      },
+      ROOT_QUERY: {
+        __typename: "Query",
+        parent: {
+          __ref: 'Person:{"name":"Will Smith"}',
+        },
+      },
+    });
+
+    // Ensure the garbage collector is not confused by the stray __ref.
+    expect(cache.gc()).toEqual([]);
+  });
+
   it("allows evicting specific fields", () => {
     const query: DocumentNode = gql`
       query {
@@ -2523,4 +2593,77 @@ describe('EntityStore', () => {
       "1449373321",
     ]);
   });
+
+  it("Refuses to merge { __ref } objects as StoreObjects", () => {
+    const cache = new InMemoryCache({
+      typePolicies: {
+        Query: {
+          fields: {
+            book: {
+              keyArgs: ["isbn"],
+            },
+          },
+        },
+
+        Book: {
+          keyFields: ["isbn"],
+        },
+      },
+    });
+
+    const store = cache["data"];
+
+    const query = gql`
+      query Book($isbn: string) {
+        book(isbn: $isbn) {
+          title
+        }
+      }
+    `;
+
+    const data = {
+      book: {
+        __typename: "Book",
+        isbn: "1449373321",
+        title: "Designing Data-Intensive Applications",
+      },
+    };
+
+    cache.writeQuery({
+      query,
+      data,
+      variables: {
+        isbn: data.book.isbn,
+      },
+    });
+
+    const bookId = cache.identify(data.book)!;
+
+    store.merge(
+      bookId,
+      makeReference(bookId) as StoreValue as StoreObject,
+    );
+
+    const snapshot = cache.extract();
+    expect(snapshot).toEqual({
+      ROOT_QUERY: {
+        __typename: "Query",
+        'book:{"isbn":"1449373321"}': {
+          __ref: 'Book:{"isbn":"1449373321"}',
+        },
+      },
+      'Book:{"isbn":"1449373321"}': {
+        __typename: "Book",
+        isbn: "1449373321",
+        title: "Designing Data-Intensive Applications",
+      },
+    });
+
+    store.merge(
+      makeReference(bookId) as StoreValue as StoreObject,
+      bookId,
+    );
+
+    expect(cache.extract()).toEqual(snapshot);
+  });
 });

src/cache/inmemory/__tests__/writeToStore.ts

@@ -13,6 +13,8 @@ import {
   storeKeyNameFromField,
   makeReference,
   isReference,
+  Reference,
+  StoreObject,
 } from '../../../utilities/graphql/storeUtils';
 import { addTypenameToDocument } from '../../../utilities/graphql/transform';
 import { cloneDeep } from '../../../utilities/common/cloneDeep';
@@ -2267,6 +2269,164 @@ describe('writing to the store', () => {
     });
   });
 
+  it("should not merge { __ref } as StoreObject when mergeObjects used", () => {
+    const merges: Array<{
+      existing: Reference | undefined;
+      incoming: Reference | StoreObject;
+      merged: Reference;
+    }> = [];
+
+    const cache = new InMemoryCache({
+      typePolicies: {
+        Account: {
+          merge(existing, incoming, { mergeObjects }) {
+            const merged = mergeObjects(existing, incoming);
+            merges.push({ existing, incoming, merged });
+            debugger;
+            return merged;
+          },
+        },
+      },
+    });
+
+    const contactLocationQuery = gql`
+      query {
+        account {
+          contact
+          location
+        }
+      }
+    `;
+
+    const contactOnlyQuery = gql`
+      query {
+        account {
+          contact
+        }
+      }
+    `;
+
+    const locationOnlyQuery = gql`
+      query {
+        account {
+          location
+        }
+      }
+    `;
+
+    cache.writeQuery({
+      query: contactLocationQuery,
+      data: {
+        account: {
+          __typename: "Account",
+          contact: "billing@example.com",
+          location: "Exampleville, Ohio",
+        },
+      },
+    });
+
+    expect(cache.extract()).toEqual({
+      ROOT_QUERY: {
+        __typename: "Query",
+        account: {
+          __typename: "Account",
+          contact: "billing@example.com",
+          location: "Exampleville, Ohio",
+        },
+      },
+    });
+
+    cache.writeQuery({
+      query: contactOnlyQuery,
+      data: {
+        account: {
+          __typename: "Account",
+          id: 12345,
+          contact: "support@example.com",
+        },
+      },
+    });
+
+    expect(cache.extract()).toEqual({
+      "Account:12345": {
+        __typename: "Account",
+        id: 12345,
+        contact: "support@example.com",
+        location: "Exampleville, Ohio",
+      },
+      ROOT_QUERY: {
+        __typename: "Query",
+        account: {
+          __ref: "Account:12345",
+        },
+      },
+    });
+
+    cache.writeQuery({
+      query: locationOnlyQuery,
+      data: {
+        account: {
+          __typename: "Account",
+          location: "Nowhere, New Mexico",
+        },
+      },
+    });
+
+    expect(cache.extract()).toEqual({
+      "Account:12345": {
+        __typename: "Account",
+        id: 12345,
+        contact: "support@example.com",
+        location: "Nowhere, New Mexico",
+      },
+      ROOT_QUERY: {
+        __typename: "Query",
+        account: {
+          __ref: "Account:12345",
+        },
+      },
+    });
+
+    expect(merges).toEqual([
+     {
+        existing: void 0,
+        incoming: {
+          __typename: "Account",
+          contact: "billing@example.com",
+          location: "Exampleville, Ohio",
+        },
+        merged: {
+          __typename: "Account",
+          contact: "billing@example.com",
+          location: "Exampleville, Ohio",
+        },
+      },
+
+      {
+        existing: {
+          __typename: "Account",
+          contact: "billing@example.com",
+          location: "Exampleville, Ohio",
+        },
+        incoming: {
+          __ref: "Account:12345",
+        },
+        merged: {
+          __ref: "Account:12345",
+        },
+      },
+
+      {
+        existing: { __ref: "Account:12345" },
+        incoming: {
+          __typename: "Account",
+          location: "Nowhere, New Mexico",
+        },
+        merged: { __ref: "Account:12345" },
+      }
+    ]);
+  });
+
   it('should not deep-freeze scalar objects', () => {
     const query = gql`
       query {

src/cache/inmemory/entityStore.ts

@@ -102,6 +102,10 @@ export abstract class EntityStore implements NormalizedCache {
   ): void {
     let dataId: string | undefined;
 
+    // Convert unexpected references to ID strings.
+    if (isReference(older)) older = older.__ref;
+    if (isReference(newer)) newer = newer.__ref;
+
     const existing: StoreObject | undefined =
       typeof older === "string"
         ? this.lookup(dataId = older)
@@ -417,18 +421,32 @@ export abstract class EntityStore implements NormalizedCache {
   public findChildRefIds(dataId: string): Record<string, true> {
     if (!hasOwn.call(this.refs, dataId)) {
       const found = this.refs[dataId] = Object.create(null);
-      const workSet = new Set([this.data[dataId]]);
+      const root = this.data[dataId];
+      if (!root) return found;
+
+      const workSet = new Set<Record<string | number, any>>([root]);
       // Within the store, only arrays and objects can contain child entity
       // references, so we can prune the traversal using this predicate:
       workSet.forEach(obj => {
         if (isReference(obj)) {
           found[obj.__ref] = true;
-        } else if (isNonNullObject(obj)) {
-          Object.values(obj!)
+          // In rare cases, a { __ref } Reference object may have other fields.
+          // This often indicates a mismerging of References with StoreObjects,
+          // but garbage collection should not be fooled by a stray __ref
+          // property in a StoreObject (ignoring all the other fields just
+          // because the StoreObject looks like a Reference). To avoid this
+          // premature termination of findChildRefIds recursion, we fall through
+          // to the code below, which will handle any other properties of obj.
+        }
+        if (isNonNullObject(obj)) {
+          Object.keys(obj).forEach(key => {
+            const child = obj[key];
             // No need to add primitive values to the workSet, since they cannot
             // contain reference objects.
-            .filter(isNonNullObject)
-            .forEach(workSet.add, workSet);
+            if (isNonNullObject(child)) {
+              workSet.add(child);
+            }
+          });
         }
       });
     }