Merge pull request #8996 from apollographql/identify-written-results-…

…using-StoreObject-in-processSelectionSet

Identify written results using processed `StoreObject` in `StoreWriter#processSelectionSet`

CHANGELOG.md

@@ -24,6 +24,9 @@
 - Report single `MissingFieldError` instead of a potentially very large `MissingFieldError[]` array for incomplete cache reads, improving performance and memory usage. <br/>
   [@benjamn](https://github.com/benjamn) in [#8734](https://github.com/apollographql/apollo-client/pull/8734)
 
+- When writing results into `InMemoryCache`, each written object is now identified using `policies.identify` _after_ traversing the fields of the object (rather than before), simplifying identification and reducing duplicate work. If you have custom `keyFields` functions, they still receive the raw result object as their first parameter, but the `KeyFieldsContext` parameter now provides `context.storeObject` (the `StoreObject` just processed by `processSelectionSet`) and `context.readField` (a helper function for reading fields from `context.storeObject` and any `Reference`s it might contain, similar to `readField` for `read`, `merge`, and `cache.modify` functions). <br/>
+  [@benjamn](https://github.com/benjamn) in [#8996](https://github.com/apollographql/apollo-client/pull/8996)
+
 - 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)
 

package.json

@@ -55,7 +55,7 @@
     {
       "name": "apollo-client",
       "path": "./dist/apollo-client.min.cjs",
-      "maxSize": "28.25 kB"
+      "maxSize": "28.45 kB"
     }
   ],
   "engines": {

src/cache/inmemory/__tests__/__snapshots__/policies.ts.snap

@@ -65,6 +65,17 @@ exports[`type policies complains about missing key fields 1`] = `
       \\"name\\": \\"James Gleick\\"
     }
   }
+}",
+    ],
+    Array [
+      "Missing field 'year' while writing result {
+  \\"__typename\\": \\"Book\\",
+  \\"isbn\\": \\"1400096235\\",
+  \\"title\\": \\"The Information\\",
+  \\"subtitle\\": \\"A History, a Theory, a Flood\\",
+  \\"author\\": {
+    \\"name\\": \\"James Gleick\\"
+  }
 }",
     ],
   ],
@@ -73,6 +84,10 @@ exports[`type policies complains about missing key fields 1`] = `
       "type": "return",
       "value": undefined,
     },
+    Object {
+      "type": "return",
+      "value": undefined,
+    },
   ],
 }
 `;

src/cache/inmemory/__tests__/key-extractor.ts

@@ -0,0 +1,182 @@
+import { KeySpecifier } from "../policies";
+import { canonicalStringify } from "../object-canon";
+import {
+  getSpecifierPaths,
+  collectSpecifierPaths,
+  extractKeyPath,
+} from "../key-extractor";
+
+describe("keyFields and keyArgs extraction", () => {
+  it("getSpecifierPaths should work for various specifiers", () => {
+    function check(specifier: KeySpecifier, expected: string[][]) {
+      const actualPaths = getSpecifierPaths(specifier);
+      expect(actualPaths).toEqual(expected);
+      // Make sure paths lookup is cached.
+      expect(getSpecifierPaths(specifier)).toBe(actualPaths);
+    }
+
+    check([], []);
+    check(["a"], [["a"]]);
+
+    check(["a", "b", "c"], [
+      ["a"],
+      ["b"],
+      ["c"]
+    ]);
+
+    check(["a", ["b", "c"], "d"], [
+      ["a", "b"],
+      ["a", "c"],
+      ["d"],
+    ]);
+
+    check(["a", "b", ["c"], "d"], [
+      ["a"],
+      ["b", "c"],
+      ["d"],
+    ]);
+
+    check(["a", "b", ["c", ["d", ["e", "f"], "g"]]], [
+      ["a"],
+      ["b", "c", "d", "e"],
+      ["b", "c", "d", "f"],
+      ["b", "c", "g"],
+    ]);
+  });
+
+  it("collectSpecifierPaths should work for various specifiers", () => {
+    const object = {
+      a: 123,
+      b: {
+        d: {
+          f: 567,
+          e: 456,
+        },
+        c: 234,
+        g: 678,
+      },
+      h: 789,
+    };
+
+    function collect(specifier: KeySpecifier) {
+      return collectSpecifierPaths(
+        specifier,
+        path => extractKeyPath(object, path)
+      );
+    }
+
+    function check(
+      specifier: KeySpecifier,
+      expected: Record<string, any>,
+    ) {
+      const actual = collect(specifier);
+      expect(actual).toEqual(expected);
+      // Not only must actual and expected be equal, but their key orderings
+      // must also be the same.
+      expect(JSON.stringify(actual)).toBe(JSON.stringify(expected));
+    }
+
+    check([], {});
+
+    check(["a", "h"], {
+      a: 123,
+      h: 789,
+    });
+
+
+    check(["h", "a", "bogus"], {
+      h: 789,
+      a: 123,
+    });
+
+    check(["b", ["d", ["e"]]], {
+      b: { d: { e: 456 }}
+    });
+
+    check(["b", ["d", ["e"]], "a"], {
+      b: { d: { e: 456 }},
+      a: 123,
+    });
+
+    check(["b", ["g", "d"], "a"], {
+      b: {
+        g: 678,
+        d: {
+          e: 456,
+          f: 567,
+        },
+      },
+      a: 123,
+    });
+
+    check(["b", "a"], {
+      b: {
+        // Notice that the keys of each nested object are sorted, despite being
+        // out of order in the original object.
+        c: 234,
+        d: {
+          e: 456,
+          f: 567,
+        },
+        g: 678,
+      },
+      // This a key comes after the b key, however, because we requested that
+      // ordering with the ["b", "a"] specifier array.
+      a: 123,
+    });
+  });
+
+  it("extractKeyPath can handle arrays", () => {
+    const object = {
+      extra: "read about it elsewhere",
+      array: [
+        { value: 1, a: "should be first" },
+        { value: 2, x: "should come after value" },
+        { z: "should come last", value: 3 },
+      ],
+      more: "another word for extra",
+    };
+
+    expect(
+      extractKeyPath(object, ["array", "value"]),
+    ).toEqual([1, 2, 3]);
+
+    expect(collectSpecifierPaths(
+      ["array", ["value", "a", "x", "z"]],
+      path => {
+        expect(path.length).toBe(2);
+        expect(path[0]).toBe("array");
+        expect(["value", "a", "x", "z"]).toContain(path[1]);
+        return extractKeyPath(object, path);
+      },
+    )).toEqual({
+      array: {
+        value: object.array.map(item => item.value),
+        a: ["should be first", void 0, void 0],
+        x: [void 0, "should come after value", void 0],
+        z: [void 0, void 0, "should come last"],
+      },
+    });
+
+    // This test case is "underspecified" because the specifier array ["array"]
+    // does not name any nested fields to pull from each array element.
+    const underspecified = extractKeyPath(object, ["array"])
+    expect(underspecified).toEqual(object.array);
+    const understringified = JSON.stringify(underspecified);
+    // Although the objects are structurally equal, they do not stringify the
+    // same, since the underspecified keys have been stably sorted.
+    expect(understringified).not.toEqual(JSON.stringify(object.array));
+
+    expect(understringified).toBe(JSON.stringify([
+      // Note that a: object.array[0].a has become the first key, because "a"
+      // precedes "value" alphabetically.
+      { a: object.array[0].a, value: 1 },
+      { value: 2, x: object.array[1].x },
+      { value: 3, z: object.array[2].z },
+    ]));
+
+    // This new ordering also happens to be the canonical/stable ordering,
+    // according to canonicalStringify.
+    expect(understringified).toBe(canonicalStringify(object.array));
+  });
+});

src/cache/inmemory/__tests__/policies.ts

@@ -224,6 +224,99 @@ describe("type policies", function () {
     checkAuthorName(cache);
   });
 
+  it("serializes nested keyFields objects in stable order", function () {
+    const cache = new InMemoryCache({
+      typePolicies: {
+        Book: {
+          // If you explicitly specify the order of author sub-fields, there
+          // will be no ambiguity about how the author object should be
+          // serialized. However, cache IDs should at least be stably
+          // stringified if the child property names are omitted, as below.
+          // keyFields: ["title", "author", ["firstName", "lastName"]],
+          keyFields: ["title", "author"],
+        },
+      },
+    });
+
+    const query = gql`
+      query {
+        book {
+          title
+          writer: author {
+            first: firstName
+            lastName
+          }
+        }
+      }
+    `;
+
+    cache.writeQuery({
+      query,
+      data: {
+        book: {
+          __typename: "Book",
+          writer: {
+            // The order of fields shouldn't matter here, since cache
+            // identification will stringify them in a stable order.
+            first: "Rebecca",
+            lastName: "Schwarzlose",
+            __typename: "Person",
+          },
+          title: "Brainscapes",
+        },
+      },
+    });
+
+    cache.writeQuery({
+      query,
+      data: {
+        book: {
+          __typename: "Book",
+          title: "The Science of Can and Can't",
+          writer: {
+            // The order of fields shouldn't matter here, since cache
+            // identification will stringify them in a stable order.
+            lastName: "Marletto",
+            __typename: "Person",
+            first: "Chiarra",
+          },
+        },
+      },
+    });
+
+    expect(cache.extract(true)).toEqual({
+      // The order of the author object's __typename, firstName, and lastName
+      // fields has been determined by our keyFields configuration and stable
+      // stringification.
+      'Book:{"title":"Brainscapes","author":{"__typename":"Person","firstName":"Rebecca","lastName":"Schwarzlose"}}': {
+        __typename: "Book",
+        title: "Brainscapes",
+        author: {
+          __typename: "Person",
+          firstName: "Rebecca",
+          lastName: "Schwarzlose",
+        },
+      },
+      // Again, __typename, firstName, and then lastName, despite the different
+      // order of keys in the data we wrote.
+      'Book:{"title":"The Science of Can and Can\'t","author":{"__typename":"Person","firstName":"Chiarra","lastName":"Marletto"}}': {
+        __typename: "Book",
+        title: "The Science of Can and Can't",
+        author: {
+          __typename: "Person",
+          firstName: "Chiarra",
+          lastName: "Marletto",
+        },
+      },
+      ROOT_QUERY: {
+        __typename: "Query",
+        book: {
+          __ref: 'Book:{"title":"The Science of Can and Can\'t","author":{"__typename":"Person","firstName":"Chiarra","lastName":"Marletto"}}',
+        },
+      },
+    });
+  });
+
   it("accepts keyFields functions", function () {
     const cache = new InMemoryCache({
       typePolicies: {