prevent merging into supertype field policies (#12750)

Co-authored-by: Jerel Miller <[email protected]>
Co-authored-by: jerelmiller <[email protected]>

Author: phryneas

.changeset/great-suns-cover.md

@@ -0,0 +1,5 @@
+---
+"@apollo/client": patch
+---
+
+Prevent field policies from overwriting/merging into supertype field policies.

.size-limits.json

@@ -1,4 +1,4 @@
 {
-  "dist/apollo-client.min.cjs": 42661,
-  "import { ApolloClient, InMemoryCache, HttpLink } from \"@apollo/client\" (production)": 34714
+  "dist/apollo-client.min.cjs": 42669,
+  "import { ApolloClient, InMemoryCache, HttpLink } from \"@apollo/client\" (production)": 34723
 }

src/cache/inmemory/__tests__/policies.ts

@@ -661,6 +661,81 @@ describe("type policies", function () {
     ).toBe('DeathAdder:{"tagId":"LethalAbacus666"}');
   });
 
+  it("specific field policies will not override supertype policies", () => {
+    const cache = new InMemoryCache({
+      typePolicies: {
+        Named: {
+          fields: Object.freeze({
+            name: Object.freeze({
+              read(existing: any) {
+                return existing + " (Named)";
+              },
+            }),
+          }),
+        },
+        Person: {
+          fields: {
+            name: {
+              read(existing) {
+                return existing + " (Person)";
+              },
+            },
+          },
+        },
+      },
+      possibleTypes: {
+        Named: ["Person", "Pet"],
+      },
+    });
+    const query = gql`
+      {
+        me {
+          __typename
+          id
+          name
+          pets {
+            __typename
+            id
+            name
+          }
+        }
+      }
+    `;
+    cache.writeQuery({
+      query,
+      data: {
+        me: {
+          __typename: "Person",
+          id: 23,
+          name: "John Doe",
+          pets: [
+            {
+              __typename: "Pet",
+              id: 1,
+              name: "Fido",
+            },
+            {
+              __typename: "Pet",
+              id: 2,
+              name: "Whiskers",
+            },
+          ],
+        },
+      },
+    });
+    expect(cache.readQuery({ query })).toEqual({
+      me: {
+        __typename: "Person",
+        id: 23,
+        name: "John Doe (Person)",
+        pets: [
+          { __typename: "Pet", id: 1, name: "Fido (Named)" },
+          { __typename: "Pet", id: 2, name: "Whiskers (Named)" },
+        ],
+      },
+    });
+  });
+
   it("typePolicies can be inherited from supertypes with fuzzy possibleTypes", () => {
     const cache = new InMemoryCache({
       possibleTypes: {

src/cache/inmemory/policies.ts

@@ -281,17 +281,20 @@ export type PossibleTypesMap = {
   [supertype: string]: string[];
 };
 
+type InternalFieldPolicy = {
+  typename: string;
+  keyFn?: KeyArgsFunction;
+  read?: FieldReadFunction<any>;
+  merge?: FieldMergeFunction<any>;
+};
+
 export class Policies {
   private typePolicies: {
     [__typename: string]: {
       keyFn?: KeyFieldsFunction;
       merge?: FieldMergeFunction<any>;
       fields: {
-        [fieldName: string]: {
-          keyFn?: KeyArgsFunction;
-          read?: FieldReadFunction<any>;
-          merge?: FieldMergeFunction<any>;
-        };
+        [fieldName: string]: InternalFieldPolicy;
       };
     };
   } = Object.create(null);
@@ -440,7 +443,11 @@ export class Policies {
     });
   }
 
-  private updateTypePolicy(typename: string, incoming: TypePolicy) {
+  private updateTypePolicy(
+    typename: string,
+    incoming: TypePolicy,
+    existingFieldPolicies: Record<string, InternalFieldPolicy>
+  ) {
     const existing = this.getTypePolicy(typename);
     const { keyFields, fields } = incoming;
 
@@ -476,7 +483,17 @@ export class Policies {
 
     if (fields) {
       Object.keys(fields).forEach((fieldName) => {
-        const existing = this.getFieldPolicy(typename, fieldName, true)!;
+        let existing = existingFieldPolicies[fieldName] as
+          | InternalFieldPolicy
+          | undefined;
+        // Field policy inheritance is atomic/shallow: you can't inherit a
+        // field policy and then override just its read function, since read
+        // and merge functions often need to cooperate, so changing only one
+        // of them would be a recipe for inconsistency.
+        // So here we avoid merging an inherited field policy with an updated one.
+        if (!existing || existing?.typename !== typename) {
+          existing = existingFieldPolicies[fieldName] = { typename };
+        }
         const incoming = fields[fieldName];
 
         if (typeof incoming === "function") {
@@ -623,7 +640,11 @@ export class Policies {
       // Merge the pending policies into this.typePolicies, in the order they
       // were originally passed to addTypePolicy.
       inbox.splice(0).forEach((policy) => {
-        this.updateTypePolicy(typename, policy);
+        this.updateTypePolicy(
+          typename,
+          policy,
+          this.typePolicies[typename].fields
+        );
       });
     }
 
@@ -632,21 +653,10 @@ export class Policies {
 
   private getFieldPolicy(
     typename: string | undefined,
-    fieldName: string,
-    createIfMissing: boolean
-  ):
-    | {
-        keyFn?: KeyArgsFunction;
-        read?: FieldReadFunction<any>;
-        merge?: FieldMergeFunction<any>;
-      }
-    | undefined {
+    fieldName: string
+  ): InternalFieldPolicy | undefined {
     if (typename) {
-      const fieldPolicies = this.getTypePolicy(typename).fields;
-      return (
-        fieldPolicies[fieldName] ||
-        (createIfMissing && (fieldPolicies[fieldName] = Object.create(null)))
-      );
+      return this.getTypePolicy(typename).fields[fieldName];
     }
   }
 
@@ -760,13 +770,13 @@ export class Policies {
   }
 
   public hasKeyArgs(typename: string | undefined, fieldName: string) {
-    const policy = this.getFieldPolicy(typename, fieldName, false);
+    const policy = this.getFieldPolicy(typename, fieldName);
     return !!(policy && policy.keyFn);
   }
 
   public getStoreFieldName(fieldSpec: FieldSpecifier): string {
     const { typename, fieldName } = fieldSpec;
-    const policy = this.getFieldPolicy(typename, fieldName, false);
+    const policy = this.getFieldPolicy(typename, fieldName);
     let storeFieldName: Exclude<ReturnType<KeyArgsFunction>, KeySpecifier>;
 
     let keyFn = policy && policy.keyFn;
@@ -835,7 +845,7 @@ export class Policies {
       objectOrReference,
       storeFieldName
     );
-    const policy = this.getFieldPolicy(options.typename, fieldName, false);
+    const policy = this.getFieldPolicy(options.typename, fieldName);
     const read = policy && policy.read;
 
     if (read) {
@@ -866,7 +876,7 @@ export class Policies {
     typename: string | undefined,
     fieldName: string
   ): FieldReadFunction | undefined {
-    const policy = this.getFieldPolicy(typename, fieldName, false);
+    const policy = this.getFieldPolicy(typename, fieldName);
     return policy && policy.read;
   }
 
@@ -878,7 +888,7 @@ export class Policies {
     let policy:
       | Policies["typePolicies"][string]
       | Policies["typePolicies"][string]["fields"][string]
-      | undefined = this.getFieldPolicy(parentTypename, fieldName, false);
+      | undefined = this.getFieldPolicy(parentTypename, fieldName);
     let merge = policy && policy.merge;
     if (!merge && childTypename) {
       policy = this.getTypePolicy(childTypename);