From 2710481ad333f70a8b4c7883b60d169862ea209b Mon Sep 17 00:00:00 2001
From: Ben Newman <ben@apollographql.com>
Date: Thu, 26 Aug 2021 11:52:48 -0400
Subject: [PATCH] Improve `keyFields` error behavior when primary key fields
 are missing (#8679)

* Make `cache.identify` return undefined instead of throwing, to help
  with `cache.identify`-related cases of issue #6673.

* Improve error message when `computeKeyFieldsObject` throws:
  https://github.com/apollographql/apollo-client/issues/6673#issuecomment-902031088
---
 CHANGELOG.md                                |  3 +++
 src/cache/inmemory/__tests__/entityStore.ts | 18 +++++++++++++++---
 src/cache/inmemory/__tests__/policies.ts    |  6 +++++-
 src/cache/inmemory/inMemoryCache.ts         |  9 +++++++--
 src/cache/inmemory/policies.ts              |  4 +++-
 5 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 46160e22899..af9e20a93a9 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -21,6 +21,9 @@
   ```
   [@benjamn](https://github.com/benjamn) in [#8678](https://github.com/apollographql/apollo-client/pull/8678)
 
+- Ensure `cache.identify` never throws when primary key fields are missing, and include the source object in the error message when `keyFields` processing fails. <br/>
+  [@benjamn](https://github.com/benjamn) in [#8679](https://github.com/apollographql/apollo-client/pull/8679)
+
 ### React Refactoring
 
 #### Bug Fixes (due to [@brainkim](https://github.com/brainkim) in [#8596](https://github.com/apollographql/apollo-client/pull/8596)):
diff --git a/src/cache/inmemory/__tests__/entityStore.ts b/src/cache/inmemory/__tests__/entityStore.ts
index a02febc9dfe..53edc086d66 100644
--- a/src/cache/inmemory/__tests__/entityStore.ts
+++ b/src/cache/inmemory/__tests__/entityStore.ts
@@ -1772,9 +1772,21 @@ describe('EntityStore', () => {
       c: 3,
     })).toBe('ABCs:{"b":2,"a":1,"c":3}');
 
-    expect(() => cache.identify(ABCs)).toThrowError(
-      "Missing field 'b' while computing key fields",
-    );
+    { // TODO Extact this to a helper function.
+      const consoleWarnSpy = jest.spyOn(console, "warn");
+      consoleWarnSpy.mockImplementation(() => {});
+      try {
+        expect(cache.identify(ABCs)).toBeUndefined();
+        expect(consoleWarnSpy).toHaveBeenCalledTimes(1);
+        expect(consoleWarnSpy).toHaveBeenCalledWith(
+          new Error(`Missing field 'b' while extracting keyFields from ${
+            JSON.stringify(ABCs)
+          }`),
+        );
+      } finally {
+        consoleWarnSpy.mockRestore();
+      }
+    }
 
     expect(cache.readFragment({
       id: cache.identify({
diff --git a/src/cache/inmemory/__tests__/policies.ts b/src/cache/inmemory/__tests__/policies.ts
index c2a82e025c9..a8bf40b51a1 100644
--- a/src/cache/inmemory/__tests__/policies.ts
+++ b/src/cache/inmemory/__tests__/policies.ts
@@ -346,7 +346,11 @@ describe("type policies", function () {
           book: theInformationBookData,
         },
       });
-    }).toThrowError("Missing field 'year' while computing key fields");
+    }).toThrowError(
+      `Missing field 'year' while extracting keyFields from ${JSON.stringify(
+        theInformationBookData
+      )}`,
+    );
   });
 
   it("does not clobber previous keyFields with undefined", function () {
diff --git a/src/cache/inmemory/inMemoryCache.ts b/src/cache/inmemory/inMemoryCache.ts
index f48a210f071..c021250bb84 100644
--- a/src/cache/inmemory/inMemoryCache.ts
+++ b/src/cache/inmemory/inMemoryCache.ts
@@ -4,6 +4,7 @@ import './fixPolyfills';
 import { DocumentNode } from 'graphql';
 import { OptimisticWrapperFunction, wrap } from 'optimism';
 import { equal } from '@wry/equality';
+import { invariant } from 'ts-invariant';
 
 import { ApolloCache } from '../core/cache';
 import { Cache } from '../core/types/Cache';
@@ -334,8 +335,12 @@ export class InMemoryCache extends ApolloCache<NormalizedCacheObject> {
   // sure that none of the primary key fields have been renamed by aliasing.
   // If you pass a Reference object, its __ref ID string will be returned.
   public identify(object: StoreObject | Reference): string | undefined {
-    return isReference(object) ? object.__ref :
-      this.policies.identify(object)[0];
+    if (isReference(object)) return object.__ref;
+    try {
+      return this.policies.identify(object)[0];
+    } catch (e) {
+      invariant.warn(e);
+    }
   }
 
   public evict(options: Cache.EvictOptions): boolean {
diff --git a/src/cache/inmemory/policies.ts b/src/cache/inmemory/policies.ts
index bdbacfa43ad..d5971168c30 100644
--- a/src/cache/inmemory/policies.ts
+++ b/src/cache/inmemory/policies.ts
@@ -1103,7 +1103,9 @@ function computeKeyFieldsObject(
       const responseKey = aliases && aliases[s] || s;
       invariant(
         hasOwn.call(response, responseKey),
-        `Missing field '${responseKey}' while computing key fields`,
+        `Missing field '${responseKey}' while extracting keyFields from ${
+          JSON.stringify(response)
+        }`,
       );
       keyObj[lastActualKey = s] = response[lastResponseKey = responseKey];
     }