Attempt to avoid cycled when expanding updated ids to include impacte…

…d resolvers (#4792)

Summary:
We have seen a bug internally which appears to be a case where we encounter a cycle in this code and thus an infinite loop. I have yet to figure out how to reproduce that behavior, but it's clear that there is a bug in this code. We maintain a set of visited IDs but then never check that set as you progress.

For now I'm adding what I believe to be a more correct implementation, however I haven't yet been able to reproduce so I'm not 100% sure this will actually fix the issue. Additionally, it's in a sensitive part of the code, so I'm gating it to allow for a gradual rollout and the ability to quickly disable.

Pull Request resolved: #4792

Test Plan: I've ensured the relevant tests validate both paths. During rollout I'll also create Diffs to run e2e tests with the fix enabled.

Differential Revision: D62508943

Pulled By: captbaritone

fbshipit-source-id: c6100c6f4c118f01bf36f2ce04e47825e0f77e93

packages/relay-runtime/store/__tests__/ClientEdgeToClientObject-test.js

@@ -38,62 +38,66 @@ afterEach(() => {
   RelayFeatureFlags.ENABLE_RELAY_RESOLVERS = false;
 });
 
-test('Can read a deep portion of the schema that is backed by client edges to client objects.', () => {
-  const source = RelayRecordSource.create({
-    'client:root': {
-      __id: 'client:root',
-      __typename: '__Root',
-      me: {__ref: '1'},
-    },
-    '1': {
-      __id: '1',
-      id: '1',
-      __typename: 'User',
-      birthdate: {__ref: '2'},
-    },
-    '2': {
-      __id: '2',
-      id: '2',
-      __typename: 'Date',
-      day: 11,
-      month: 3,
-    },
-  });
-  const FooQuery = graphql`
-    query ClientEdgeToClientObjectTest1Query {
-      me {
-        astrological_sign {
-          __id
-          name
-          house
-          opposite {
-            __id
-            name
-            house
-            opposite {
+describe.each([true, false])(
+  'AVOID_CYCLES_IN_RESOLVER_NOTIFICATION is %p',
+  avoidCycles => {
+    RelayFeatureFlags.AVOID_CYCLES_IN_RESOLVER_NOTIFICATION = avoidCycles;
+    test('Can read a deep portion of the schema that is backed by client edges to client objects.', () => {
+      const source = RelayRecordSource.create({
+        'client:root': {
+          __id: 'client:root',
+          __typename: '__Root',
+          me: {__ref: '1'},
+        },
+        '1': {
+          __id: '1',
+          id: '1',
+          __typename: 'User',
+          birthdate: {__ref: '2'},
+        },
+        '2': {
+          __id: '2',
+          id: '2',
+          __typename: 'Date',
+          day: 11,
+          month: 3,
+        },
+      });
+      const FooQuery = graphql`
+        query ClientEdgeToClientObjectTest1Query {
+          me {
+            astrological_sign {
               __id
               name
+              house
+              opposite {
+                __id
+                name
+                house
+                opposite {
+                  __id
+                  name
+                }
+              }
             }
           }
         }
-      }
-    }
-  `;
+      `;
 
-  const operation = createOperationDescriptor(FooQuery, {});
-  const store = new LiveResolverStore(source, {
-    gcReleaseBufferSize: 0,
-  });
+      const operation = createOperationDescriptor(FooQuery, {});
+      const store = new LiveResolverStore(source, {
+        gcReleaseBufferSize: 0,
+      });
 
-  const environment = new RelayModernEnvironment({
-    network: RelayNetwork.create(jest.fn()),
-    store,
-  });
+      const environment = new RelayModernEnvironment({
+        network: RelayNetwork.create(jest.fn()),
+        store,
+      });
 
-  // $FlowFixMe[unclear-type] - lookup() doesn't have the nice types of reading a fragment through the actual APIs:
-  const {me}: any = environment.lookup(operation.fragment).data;
+      // $FlowFixMe[unclear-type] - lookup() doesn't have the nice types of reading a fragment through the actual APIs:
+      const {me}: any = environment.lookup(operation.fragment).data;
 
-  expect(me).toMatchInlineSnapshot(`
+      expect(me).toMatchInlineSnapshot(`
     Object {
       "astrological_sign": Object {
         "__id": "client:AstrologicalSign:Pisces",
@@ -111,42 +115,42 @@ test('Can read a deep portion of the schema that is backed by client edges to cl
       },
     }
   `);
-});
+    });
 
-test('Can read a plural client edge to list of client defined types', () => {
-  const source = RelayRecordSource.create({
-    'client:root': {
-      __id: 'client:root',
-      __typename: '__Root',
-      me: {__ref: '1'},
-    },
-    '1': {
-      __id: '1',
-      id: '1',
-      __typename: 'User',
-    },
-  });
-  const FooQuery = graphql`
-    query ClientEdgeToClientObjectTest2Query {
-      all_astrological_signs {
-        name
-      }
-    }
-  `;
+    test('Can read a plural client edge to list of client defined types', () => {
+      const source = RelayRecordSource.create({
+        'client:root': {
+          __id: 'client:root',
+          __typename: '__Root',
+          me: {__ref: '1'},
+        },
+        '1': {
+          __id: '1',
+          id: '1',
+          __typename: 'User',
+        },
+      });
+      const FooQuery = graphql`
+        query ClientEdgeToClientObjectTest2Query {
+          all_astrological_signs {
+            name
+          }
+        }
+      `;
 
-  const operation = createOperationDescriptor(FooQuery, {});
-  const store = new LiveResolverStore(source, {
-    gcReleaseBufferSize: 0,
-  });
+      const operation = createOperationDescriptor(FooQuery, {});
+      const store = new LiveResolverStore(source, {
+        gcReleaseBufferSize: 0,
+      });
 
-  const environment = new RelayModernEnvironment({
-    network: RelayNetwork.create(jest.fn()),
-    store,
-  });
+      const environment = new RelayModernEnvironment({
+        network: RelayNetwork.create(jest.fn()),
+        store,
+      });
 
-  const {data} = environment.lookup(operation.fragment);
+      const {data} = environment.lookup(operation.fragment);
 
-  expect(data).toMatchInlineSnapshot(`
+      expect(data).toMatchInlineSnapshot(`
     Object {
       "all_astrological_signs": Array [
         Object {
@@ -188,88 +192,90 @@ test('Can read a plural client edge to list of client defined types', () => {
       ],
     }
   `);
-});
+    });
 
-test('Uses an existing client record if it already exists', () => {
-  const source = RelayRecordSource.create({
-    'client:root': {
-      __id: 'client:root',
-      __typename: '__Root',
-      me: {__ref: '1'},
-    },
-    '1': {
-      __id: '1',
-      id: '1',
-      __typename: 'User',
-      birthdate: {__ref: '2'},
-    },
-    '2': {
-      __id: '2',
-      id: '2',
-      __typename: 'Date',
-      day: 11,
-      month: 3,
-    },
-  });
+    test('Uses an existing client record if it already exists', () => {
+      const source = RelayRecordSource.create({
+        'client:root': {
+          __id: 'client:root',
+          __typename: '__Root',
+          me: {__ref: '1'},
+        },
+        '1': {
+          __id: '1',
+          id: '1',
+          __typename: 'User',
+          birthdate: {__ref: '2'},
+        },
+        '2': {
+          __id: '2',
+          id: '2',
+          __typename: 'Date',
+          day: 11,
+          month: 3,
+        },
+      });
 
-  const FooQuery = graphql`
-    query ClientEdgeToClientObjectTest3Query {
-      me {
-        astrological_sign {
-          __id
-          name
-          notes
+      const FooQuery = graphql`
+        query ClientEdgeToClientObjectTest3Query {
+          me {
+            astrological_sign {
+              __id
+              name
+              notes
+            }
+          }
         }
-      }
-    }
-  `;
+      `;
 
-  const operation = createOperationDescriptor(FooQuery, {});
-  const liveStore = new LiveResolverStore(source, {
-    gcReleaseBufferSize: 0,
-  });
+      const operation = createOperationDescriptor(FooQuery, {});
+      const liveStore = new LiveResolverStore(source, {
+        gcReleaseBufferSize: 0,
+      });
 
-  const environment = new RelayModernEnvironment({
-    network: RelayNetwork.create(jest.fn()),
-    store: liveStore,
-  });
+      const environment = new RelayModernEnvironment({
+        network: RelayNetwork.create(jest.fn()),
+        store: liveStore,
+      });
 
-  const data: ClientEdgeToClientObjectTest3Query$data = (environment.lookup(
-    operation.fragment,
-    // $FlowFixMe[unclear-type] - lookup() doesn't have the nice types of reading a fragment through the actual APIs:
-  ).data: any);
+      const data: ClientEdgeToClientObjectTest3Query$data = (environment.lookup(
+        operation.fragment,
+        // $FlowFixMe[unclear-type] - lookup() doesn't have the nice types of reading a fragment through the actual APIs:
+      ).data: any);
 
-  expect(data).toEqual({
-    me: {
-      astrological_sign: {
-        __id: 'client:AstrologicalSign:Pisces',
-        name: 'Pisces',
-        notes: undefined,
-      },
-    },
-  });
+      expect(data).toEqual({
+        me: {
+          astrological_sign: {
+            __id: 'client:AstrologicalSign:Pisces',
+            name: 'Pisces',
+            notes: undefined,
+          },
+        },
+      });
 
-  commitLocalUpdate(environment, store => {
-    const id = data.me?.astrological_sign?.__id;
-    if (id == null) {
-      throw new Error('Expected to get an id');
-    }
-    const sign = store.get(id);
-    if (sign == null) {
-      throw new Error('Tried to reference a non-existent sign');
-    }
-    sign.setValue('This is a cool note.', 'notes');
-  });
+      commitLocalUpdate(environment, store => {
+        const id = data.me?.astrological_sign?.__id;
+        if (id == null) {
+          throw new Error('Expected to get an id');
+        }
+        const sign = store.get(id);
+        if (sign == null) {
+          throw new Error('Tried to reference a non-existent sign');
+        }
+        sign.setValue('This is a cool note.', 'notes');
+      });
 
-  const {data: newData} = environment.lookup(operation.fragment);
+      const {data: newData} = environment.lookup(operation.fragment);
 
-  expect(newData).toEqual({
-    me: {
-      astrological_sign: {
-        __id: 'client:AstrologicalSign:Pisces',
-        name: 'Pisces',
-        notes: 'This is a cool note.',
-      },
-    },
-  });
-});
+      expect(newData).toEqual({
+        me: {
+          astrological_sign: {
+            __id: 'client:AstrologicalSign:Pisces',
+            name: 'Pisces',
+            notes: 'This is a cool note.',
+          },
+        },
+      });
+    });
+  },
+);