Avoid letting defaultDataIdFromObject normalize objects with nullis…

…h ids (#9862) * Reject id:null and id:undefined in defaultDataIdFromObject. * Passing regression tests for issue #9814.
apollographql · Sep 21, 2022 · b091220 · b091220
1 parent 1812a63
commit b091220
Show file tree

Hide file tree

Showing 2 changed files with 192 additions and 4 deletions.
diff --git a/src/cache/inmemory/__tests__/writeToStore.ts b/src/cache/inmemory/__tests__/writeToStore.ts
@@ -673,6 +673,186 @@ describe('writing to the store', () => {
     });
   });
 
+  it('refuses to normalize objects with nullish id fields', () => {
+    const query: TypedDocumentNode<{
+      objects: Array<{
+        __typename: string;
+        id?: any;
+        text?: string;
+      }>;
+    }> = gql`
+      query {
+        objects {
+          id
+          text
+        }
+      }
+    `;
+
+    const cache = new InMemoryCache({
+      // No keyFields type policy or dataIdFromObject, so we're using/testing
+      // the default implementation, defaultDataIdFromObject.
+    });
+
+    cache.writeQuery({
+      query,
+      data: {
+        objects: [
+          { __typename: "Object", text: "a", id: 123 },
+          { __typename: "Object", text: "b", id: null },
+          { __typename: "Object", text: "c", id: void 0 },
+          { __typename: "Object", text: "d", id: 0 },
+          { __typename: "Object", text: "e", id: "" },
+          { __typename: "Object", text: "f", id: false },
+          { __typename: "Object", text: "g" },
+        ]
+      },
+    });
+
+    expect(cache.extract()).toEqual({
+     "Object:123": {
+       __typename: "Object",
+       id: 123,
+       text: "a",
+     },
+     "Object:0": {
+      __typename: "Object",
+      id: 0,
+      text: "d",
+    },
+    "Object:": {
+      __typename: "Object",
+      id: "",
+      text: "e",
+    },
+    "Object:false": {
+      __typename: "Object",
+      id: false,
+      text: "f",
+    },
+     "ROOT_QUERY": {
+       __typename: "Query",
+       objects: [
+         { __ref: "Object:123" },
+         {
+           __typename: "Object",
+           id: null,
+           text: "b",
+         },
+         { __typename: "Object", text: "c" },
+         { __ref: "Object:0" },
+         { __ref: "Object:" },
+         { __ref: "Object:false" },
+         { __typename: "Object", text: "g" },
+       ],
+     },
+    });
+
+    expect(cache.readQuery({
+      query: gql`query { objects { text }}`,
+    })).toEqual({
+      objects: [
+        { __typename: "Object", text: "a" },
+        { __typename: "Object", text: "b" },
+        { __typename: "Object", text: "c" },
+        { __typename: "Object", text: "d" },
+        { __typename: "Object", text: "e" },
+        { __typename: "Object", text: "f" },
+        { __typename: "Object", text: "g" },
+      ],
+    });
+  });
+
+  it('refuses to normalize objects with nullish id fields', () => {
+    const query: TypedDocumentNode<{
+      objects: Array<{
+        __typename: string;
+        _id?: any;
+        text?: string;
+      }>;
+    }> = gql`
+      query {
+        objects {
+          _id
+          text
+        }
+      }
+    `;
+
+    const cache = new InMemoryCache({
+      // No keyFields type policy or dataIdFromObject, so we're using/testing
+      // the default implementation, defaultDataIdFromObject.
+    });
+
+    cache.writeQuery({
+      query,
+      data: {
+        objects: [
+          { __typename: "Object", text: "a", _id: 123 },
+          { __typename: "Object", text: "b", _id: null },
+          { __typename: "Object", text: "c", _id: void 0 },
+          { __typename: "Object", text: "d", _id: 0 },
+          { __typename: "Object", text: "e", _id: "" },
+          { __typename: "Object", text: "f", _id: false },
+          { __typename: "Object", text: "g" },
+        ]
+      },
+    });
+
+    expect(cache.extract()).toEqual({
+     "Object:123": {
+       __typename: "Object",
+       _id: 123,
+       text: "a",
+     },
+     "Object:0": {
+      __typename: "Object",
+      _id: 0,
+      text: "d",
+    },
+    "Object:": {
+      __typename: "Object",
+      _id: "",
+      text: "e",
+    },
+    "Object:false": {
+      __typename: "Object",
+      _id: false,
+      text: "f",
+    },
+     "ROOT_QUERY": {
+       __typename: "Query",
+       objects: [
+         { __ref: "Object:123" },
+         {
+           __typename: "Object",
+           _id: null,
+           text: "b",
+         },
+         { __typename: "Object", text: "c" },
+         { __ref: "Object:0" },
+         { __ref: "Object:" },
+         { __ref: "Object:false" },
+         { __typename: "Object", text: "g" },
+       ],
+     },
+    });
+
+    expect(cache.readQuery({
+      query: gql`query { objects { text }}`,
+    })).toEqual({
+      objects: [
+        { __typename: "Object", text: "a" },
+        { __typename: "Object", text: "b" },
+        { __typename: "Object", text: "c" },
+        { __typename: "Object", text: "d" },
+        { __typename: "Object", text: "e" },
+        { __typename: "Object", text: "f" },
+        { __typename: "Object", text: "g" },
+      ],
+    });
+  });
+
   it('properly normalizes an object occurring in different graphql paths twice', () => {
     const query = gql`
       {

diff --git a/src/cache/inmemory/helpers.ts b/src/cache/inmemory/helpers.ts
@@ -24,20 +24,28 @@ export const {
   hasOwnProperty: hasOwn,
 } = Object.prototype;
 
+export function isNullish(value: any): value is null | undefined {
+  return value === null || value === void 0;
+}
+
 export function defaultDataIdFromObject(
   { __typename, id, _id }: Readonly<StoreObject>,
   context?: KeyFieldsContext,
 ): string | undefined {
   if (typeof __typename === "string") {
     if (context) {
       context.keyObject =
-         id !== void 0 ? {  id } :
-        _id !== void 0 ? { _id } :
+        !isNullish(id) ? { id } :
+        !isNullish(_id) ? { _id } :
         void 0;
     }
+
     // If there is no object.id, fall back to object._id.
-    if (id === void 0) id = _id;
-    if (id !== void 0) {
+    if (isNullish(id) && !isNullish(_id)) {
+      id = _id;
+    }
+
+    if (!isNullish(id)) {
       return `${__typename}:${(
         typeof id === "number" ||
         typeof id === "string"