RelayReader: dont consider data on non-matching types "missing"

Summary:
== Motivation ==

The goal of this stack is to more accurately model GraphQL semantics so that client-side execution matches server-side execution. Specifically, the server has full knowledge of the schema (which types implement which interfaces) and uses this to evaluate or skip type-conditional selections (inline fragments and fragment spreads). Relay doesn't currently have full knowledge of the schema and uses simple rules to decide which type-conditional selections to evaluate, which can result in incorrectly evaluating selections that (per schema/spec) should not be evaluated. There are two cases:

1. Reading fragment spreads where a concrete fragment is spread within an abstract type. Currently Relay cannot distinguish whether the fragment is on a concrete type or not, and always tries to read the fragment. Technically, the fragment shouldn't be evaluated (read) at all.
2. Reading inline fragments.

== This Diff ==

This diff is a follow-up to D21281486 and addresses item 1 above. The previous diff changed fragments to have a `concreteType: ?string` field that tells us whether the fragment has a concrete type or not. In this diff, RelayReader is updated to use that information to ensure that `isMissingData` is considered false whenever we try to read a concrete fragment for an object w a non-matching type. As an example, if the object is a `Page` but the fragment is `on User`, we will always set `isMissingData: false` since the types don't match. This makes sense if you think of `isMissingData` as an abbreviation of `isMissing(Expected)Data`: in these cases no data is actually expected.

Reviewed By: jstejada

Differential Revision: D21306925

fbshipit-source-id: 4074880487ac94e87b4f586570b83e084837e92c

packages/relay-experimental/__tests__/useRefetchableFragmentNode-test.js

@@ -1305,7 +1305,7 @@ describe('useRefetchableFragmentNode', () => {
 
       // $FlowFixMe
       const warningCalls = warning.mock.calls.filter(call => call[0] === false);
-      expect(warningCalls.length).toEqual(4); // the other warnings are from FragmentResource.js
+      expect(warningCalls.length).toEqual(2); // the other warnings are from FragmentResource.js
       expect(
         warningCalls[1][1].includes(
           'Relay: Call to `refetch` returned data with a different __typename:',

packages/relay-runtime/store/RelayReader.js

@@ -37,6 +37,7 @@ const {
   getArgumentValues,
   getStorageKey,
   getModuleComponentKey,
+  ROOT_ID,
 } = require('./RelayStoreUtils');
 
 import type {
@@ -89,6 +90,38 @@ class RelayReader {
   read(): Snapshot {
     const {node, dataID} = this._selector;
     const data = this._traverse(node, dataID, null);
+
+    // Handle an edge-case in missing data-detection. Fragments
+    // with a concrete type can be spread anywhere that type *might*
+    // appear (ie, on parents that return an abstract type whose
+    // possible types include the concrete type). In this case, Relay
+    // allows trying to read the fragment data even if the actual type
+    // didn't match, and returns whatever data happened to be present.
+    // However, in this case it is entirely expected that fields may
+    // be missing, since the concrete types don't match.
+    // In this case, reset isMissingData back to false.
+    // Quickly skip this check in the common case that no data was
+    // missing or fragments on abstract types.
+    if (this._isMissingData && node.concreteType != null) {
+      const record = this._recordSource.get(dataID);
+      if (record != null) {
+        const recordType = RelayModernRecord.getType(record);
+        if (recordType !== node.concreteType && dataID !== ROOT_ID) {
+          // The record exists and its (concrete) type differs
+          // from the fragment's concrete type: data is
+          // expected to be missing, so don't flag it as such
+          // since doing so could incorrectly trigger suspense.
+          // NOTE `isMissingData` is really more "is missing
+          // *expected* data", and the data isn't expected here.
+          // Also note that the store uses a hard-code __typename
+          // for the root object, while fragments on the Query
+          // type will use whatever the schema names the Query type.
+          // Assume fragments read on the root object have the right
+          // type and trust isMissingData.
+          this._isMissingData = false;
+        }
+      }
+    }
     return {
       data,
       isMissingData: this._isMissingData,

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

@@ -1445,6 +1445,119 @@ describe('RelayReader', () => {
         expect(data.id).toBe('1');
         expect(isMissingData).toBe(false);
       });
+
+      it('should not consider data missing if the fragment type does not match the data', () => {
+        const {ActorQuery, UserProfile} = generateAndCompile(`
+          query ActorQuery {
+            viewer {
+              actor {
+                ...UserProfile
+              }
+            }
+          }
+          fragment UserProfile on User {
+            name
+          }
+        `);
+        source = new RelayRecordSource({
+          'client:root': {
+            __id: 'client:root',
+            __typename: '__Root',
+            viewer: {__ref: 'client:root:viewer'},
+          },
+          'client:root:viewer': {
+            __id: 'client:root:viewer',
+            __typename: 'Viewer',
+            actor: {__ref: '1'},
+          },
+          '1': {
+            __id: '1',
+            __typename: 'Page',
+            // NOTE: no 'name' value, server would not return one since
+            // name is only selected if viewer.actor is a User, and it's
+            // a Page
+          },
+        });
+        const owner = createOperationDescriptor(ActorQuery, {});
+        const {data, isMissingData} = read(
+          source,
+          createReaderSelector(UserProfile, '1', {}, owner.request),
+        );
+        expect(data).toEqual({
+          name: undefined,
+        });
+        expect(isMissingData).toBe(false);
+      });
+
+      it('should consider data missing if the fragment type is abstract', () => {
+        const {ActorQuery, ActorProfile} = generateAndCompile(`
+          query ActorQuery {
+            viewer {
+              actor {
+                ...ActorProfile
+              }
+            }
+          }
+          fragment ActorProfile on Actor {
+            name
+          }
+        `);
+        source = new RelayRecordSource({
+          'client:root': {
+            __id: 'client:root',
+            __typename: '__Root',
+            viewer: {__ref: 'client:root:viewer'},
+          },
+          'client:root:viewer': {
+            __id: 'client:root:viewer',
+            __typename: 'Viewer',
+            actor: {__ref: '1'},
+          },
+          '1': {
+            __id: '1',
+            __typename: 'Page',
+            // NOTE: no 'name' value
+          },
+        });
+        const owner = createOperationDescriptor(ActorQuery, {});
+        const {data, isMissingData} = read(
+          source,
+          createReaderSelector(ActorProfile, '1', {}, owner.request),
+        );
+        expect(data).toEqual({
+          name: undefined,
+        });
+        expect(isMissingData).toBe(true);
+      });
+
+      it('should consider data missing if the fragment is concrete but on the root', () => {
+        const {Query, RootFragment} = generateAndCompile(`
+          query Query {
+            ...RootFragment
+          }
+          fragment RootFragment on Query {
+            me {
+              name
+            }
+          }
+        `);
+        source = new RelayRecordSource({
+          'client:root': {
+            __id: 'client:root',
+            __typename: '__Root',
+            // No 'me' value
+          },
+        });
+        const owner = createOperationDescriptor(Query, {});
+        const {data, isMissingData} = read(
+          source,
+          createReaderSelector(RootFragment, 'client:root', {}, owner.request),
+        );
+        expect(data).toEqual({
+          me: undefined,
+        });
+        expect(isMissingData).toBe(true);
+      });
     });
 
     describe('@stream_connection', () => {