Add fieldPath to missing data field error logs (#4835)

Summary:
For `required` fields and Relay Resolvers which might error, we include the field's path (relative to its parent fragment/query) in the generated artifact.

This allows us to avoid maintaining a field path during read traversal, which would require a large number of push/pops of an array as we traverse through the reader AST.

However, with the addition of `throwFieldOnError` we also report _missing data_ as field errors, but these errors can occur for any field, and including every field's path in the generated artifact is likely too heavy.

This PR attempt to use an efficient approach where we build up the path on the trailing edge of the reader recursion, which allows us to avoid pushing/popping in the common case where there are no errors. The cost in the happy path is just an additional method call and a few null checks as we exit each level of recursion that impacts the path.

If this approach proves effective, we should be able to adopt the same approach for `requried` and resolver errors, simplifying the compiler and making our generated artifacts a bit lighter.

Pull Request resolved: #4835

Reviewed By: tyao1

Differential Revision: D65278745

Pulled By: captbaritone

fbshipit-source-id: 21de9641c7f629353249d6126326fa23f1c83f36

packages/react-relay/__tests__/LiveResolvers-test.js

@@ -975,7 +975,7 @@ describe.each([true, false])(
         expect(relayFieldLogger.mock.calls).toEqual([
           [
             {
-              fieldPath: '',
+              fieldPath: 'username',
               kind: 'missing_expected_data.log',
               owner: 'ResolverThatThrows',
             },

packages/relay-runtime/store/RelayReader.js

@@ -217,7 +217,7 @@ class RelayReader {
     }
   }
 
-  _markDataAsMissing(): void {
+  _markDataAsMissing(fieldName: string): void {
     if (this._isWithinUnmatchedTypeRefinement) {
       return;
     }
@@ -226,18 +226,17 @@ class RelayReader {
     }
 
     // we will add the path later
-    const fieldPath = '';
     const owner = this._fragmentName;
 
     this._errorResponseFields.push(
       this._selector.node.metadata?.throwOnFieldError ?? false
         ? {
             kind: 'missing_expected_data.throw',
             owner,
-            fieldPath,
+            fieldPath: fieldName,
             handled: false,
           }
-        : {kind: 'missing_expected_data.log', owner, fieldPath},
+        : {kind: 'missing_expected_data.log', owner, fieldPath: fieldName},
     );
 
     this._isMissingData = true;
@@ -266,7 +265,7 @@ class RelayReader {
     this._seenRecords.add(dataID);
     if (record == null) {
       if (record === undefined) {
-        this._markDataAsMissing();
+        this._markDataAsMissing('<record>');
       }
       return record;
     }
@@ -489,12 +488,15 @@ class RelayReader {
           this._createFragmentPointer(selection, record, data);
           break;
         case 'AliasedInlineFragmentSpread': {
+          const prevErrors = this._errorResponseFields;
+          this._errorResponseFields = null;
           let fieldValue = this._readInlineFragment(
             selection.fragment,
             record,
             {},
             true,
           );
+          this._prependPreviousErrors(prevErrors, selection.name);
           if (fieldValue === false) {
             fieldValue = null;
           }
@@ -601,9 +603,12 @@ class RelayReader {
     data: SelectorData,
   ): mixed {
     const parentRecordID = RelayModernRecord.getDataID(record);
+    const prevErrors = this._errorResponseFields;
+    this._errorResponseFields = null;
     const result = this._readResolverFieldImpl(field, parentRecordID);
 
     const fieldName = field.alias ?? field.name;
+    this._prependPreviousErrors(prevErrors, fieldName);
     data[fieldName] = result;
     return result;
   }
@@ -982,12 +987,15 @@ class RelayReader {
         RelayModernRecord.getDataID(record),
         prevData,
       );
+      const prevErrors = this._errorResponseFields;
+      this._errorResponseFields = null;
       const edgeValue = this._traverse(
         field.linkedField,
         storeID,
         // $FlowFixMe[incompatible-variance]
         prevData,
       );
+      this._prependPreviousErrors(prevErrors, fieldName);
       this._clientEdgeTraversalPath.pop();
       data[fieldName] = edgeValue;
       return edgeValue;
@@ -1005,7 +1013,7 @@ class RelayReader {
     if (value === null) {
       this._maybeAddErrorResponseFields(record, storageKey);
     } else if (value === undefined) {
-      this._markDataAsMissing();
+      this._markDataAsMissing(fieldName);
     }
     data[fieldName] = value;
     return value;
@@ -1024,7 +1032,7 @@ class RelayReader {
       if (linkedID === null) {
         this._maybeAddErrorResponseFields(record, storageKey);
       } else if (linkedID === undefined) {
-        this._markDataAsMissing();
+        this._markDataAsMissing(fieldName);
       }
       return linkedID;
     }
@@ -1039,12 +1047,73 @@ class RelayReader {
       RelayModernRecord.getDataID(record),
       prevData,
     );
+    const prevErrors = this._errorResponseFields;
+    this._errorResponseFields = null;
     // $FlowFixMe[incompatible-variance]
     const value = this._traverse(field, linkedID, prevData);
+
+    this._prependPreviousErrors(prevErrors, fieldName);
     data[fieldName] = value;
     return value;
   }
 
+  /**
+   * Adds a set of field errors to `this._errorResponseFields`, ensuring the
+   * `fieldPath` property of existing field errors are prefixed with the given
+   * `fieldNameOrIndex`.
+   *
+   * In order to make field errors maximally useful in logs/errors, we want to
+   * include the path to the field that caused the error. A naive approach would
+   * be to maintain a path property on RelayReader which we push/pop field names
+   * to as we traverse into fields/etc. However, this would be expensive to
+   * maintain, and in the common case where there are no field errors, the work
+   * would go unused.
+   *
+   * Instead, we take a lazy approach where as we exit the recurison into a
+   * field/etc we prepend any errors encountered while traversing that field
+   * with the field name. This is somewhat more expensive in the error case, but
+   * ~free in the common case where there are no errors.
+   *
+   * To achieve this, named field readers must do the following to correctly
+   * track error filePaths:
+   *
+   * 1. Stash the value of `this._errorResponseFields` in a local variable
+   * 2. Set `this._errorResponseFields` to `null`
+   * 3. Traverse into the field
+   * 4. Call this method with the stashed errors and the field's name
+   *
+   * Similarly, when creating field errors, we simply initialize the `fieldPath`
+   * as the direct field name.
+   *
+   * Today we only use this apporach for `missing_expected_data` errors, but we
+   * intend to broaden it to handle all field error paths.
+   */
+  _prependPreviousErrors(
+    prevErrors: ?Array<ErrorResponseField>,
+    fieldNameOrIndex: string | number,
+  ): void {
+    if (this._errorResponseFields != null) {
+      for (let i = 0; i < this._errorResponseFields.length; i++) {
+        const event = this._errorResponseFields[i];
+        if (
+          event.owner === this._fragmentName &&
+          (event.kind === 'missing_expected_data.throw' ||
+            event.kind === 'missing_expected_data.log')
+        ) {
+          event.fieldPath = `${fieldNameOrIndex}.${event.fieldPath}`;
+        }
+      }
+      if (prevErrors != null) {
+        for (let i = this._errorResponseFields.length - 1; i >= 0; i--) {
+          prevErrors.push(this._errorResponseFields[i]);
+        }
+        this._errorResponseFields = prevErrors;
+      }
+    } else {
+      this._errorResponseFields = prevErrors;
+    }
+  }
+
   _readActorChange(
     field: ReaderActorChange,
     record: Record,
@@ -1060,7 +1129,7 @@ class RelayReader {
     if (externalRef == null) {
       data[fieldName] = externalRef;
       if (externalRef === undefined) {
-        this._markDataAsMissing();
+        this._markDataAsMissing(fieldName);
       } else if (externalRef === null) {
         this._maybeAddErrorResponseFields(record, storageKey);
       }
@@ -1107,7 +1176,7 @@ class RelayReader {
     if (linkedIDs == null) {
       data[fieldName] = linkedIDs;
       if (linkedIDs === undefined) {
-        this._markDataAsMissing();
+        this._markDataAsMissing(fieldName);
       }
       return linkedIDs;
     }
@@ -1121,11 +1190,13 @@ class RelayReader {
       RelayModernRecord.getDataID(record),
       prevData,
     );
+    const prevErrors = this._errorResponseFields;
+    this._errorResponseFields = null;
     const linkedArray = prevData || [];
     linkedIDs.forEach((linkedID, nextIndex) => {
       if (linkedID == null) {
         if (linkedID === undefined) {
-          this._markDataAsMissing();
+          this._markDataAsMissing(String(nextIndex));
         }
         // $FlowFixMe[cannot-write]
         linkedArray[nextIndex] = linkedID;
@@ -1140,10 +1211,14 @@ class RelayReader {
         RelayModernRecord.getDataID(record),
         prevItem,
       );
+      const prevErrors = this._errorResponseFields;
+      this._errorResponseFields = null;
       // $FlowFixMe[cannot-write]
       // $FlowFixMe[incompatible-variance]
       linkedArray[nextIndex] = this._traverse(field, linkedID, prevItem);
+      this._prependPreviousErrors(prevErrors, nextIndex);
     });
+    this._prependPreviousErrors(prevErrors, fieldName);
     data[fieldName] = linkedArray;
     return linkedArray;
   }
@@ -1166,7 +1241,7 @@ class RelayReader {
       RelayModernRecord.getValue(record, componentKey);
     if (component == null) {
       if (component === undefined) {
-        this._markDataAsMissing();
+        this._markDataAsMissing('<module-import>');
       }
       return;
     }
@@ -1398,7 +1473,7 @@ class RelayReader {
       // fetched the `__is[AbstractType]` flag for this concrete type. In this
       // case we need to report that we are missing data, in case that field is
       // still in flight.
-      this._markDataAsMissing();
+      this._markDataAsMissing('<abstract-type-hint>');
     }
     // $FlowFixMe Casting record value
     return implementsInterface;

packages/relay-runtime/store/RelayStoreTypes.js

@@ -1274,7 +1274,7 @@ export type MissingFieldHandler =
 export type MissingExpectedDataLogEvent = {
   +kind: 'missing_expected_data.log',
   +owner: string,
-  +fieldPath: string,
+  fieldPath: string, // Purposefully mutable to allow lazy construction in RelayReader
 };
 
 /**
@@ -1300,7 +1300,7 @@ export type MissingExpectedDataLogEvent = {
 export type MissingExpectedDataThrowEvent = {
   +kind: 'missing_expected_data.throw',
   +owner: string,
-  +fieldPath: string,
+  fieldPath: string, // Purposefully mutable to allow lazy construction in RelayReader
   +handled: boolean,
 };
 

packages/relay-runtime/store/__tests__/RelayModernStore-Subscriptions-test.js

@@ -417,12 +417,12 @@ function cloneEventWithSets(event: LogEvent) {
           isMissingData: true,
           errorResponseFields: [
             {
-              fieldPath: '',
+              fieldPath: 'profilePicture',
               kind: 'missing_expected_data.log',
               owner: 'RelayModernStoreSubscriptionsTest1Fragment',
             },
             {
-              fieldPath: '',
+              fieldPath: 'emailAddresses',
               kind: 'missing_expected_data.log',
               owner: 'RelayModernStoreSubscriptionsTest1Fragment',
             },
@@ -466,12 +466,12 @@ function cloneEventWithSets(event: LogEvent) {
           },
           errorResponseFields: [
             {
-              fieldPath: '',
+              fieldPath: 'profilePicture',
               kind: 'missing_expected_data.log',
               owner: 'RelayModernStoreSubscriptionsTest1Fragment',
             },
             {
-              fieldPath: '',
+              fieldPath: 'emailAddresses',
               kind: 'missing_expected_data.log',
               owner: 'RelayModernStoreSubscriptionsTest1Fragment',
             },

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

@@ -728,12 +728,12 @@ function cloneEventWithSets(event: LogEvent) {
             {
               owner: 'RelayModernStoreTest5Fragment',
               kind: 'missing_expected_data.log',
-              fieldPath: '',
+              fieldPath: 'profilePicture',
             },
             {
               owner: 'RelayModernStoreTest5Fragment',
               kind: 'missing_expected_data.log',
-              fieldPath: '',
+              fieldPath: 'emailAddresses',
             },
           ],
           missingLiveResolverFields: [],
@@ -782,12 +782,12 @@ function cloneEventWithSets(event: LogEvent) {
             {
               owner: 'RelayModernStoreTest5Fragment',
               kind: 'missing_expected_data.log',
-              fieldPath: '',
+              fieldPath: 'profilePicture',
             },
             {
               owner: 'RelayModernStoreTest5Fragment',
               kind: 'missing_expected_data.log',
-              fieldPath: '',
+              fieldPath: 'emailAddresses',
             },
           ],
           seenRecords: new Set(['842472']),

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

@@ -1041,7 +1041,7 @@ describe('Inline Fragments', () => {
     );
     expect(errorResponseFields).toEqual([
       {
-        fieldPath: '',
+        fieldPath: 'node.aliased_fragment.name',
         kind: 'missing_expected_data.log',
         owner:
           'RelayReaderAliasedFragmentsTestRequiredBubblesOnAbstractTypeQuery',
@@ -1101,7 +1101,7 @@ describe('Inline Fragments', () => {
     );
     expect(errorResponseFields).toEqual([
       {
-        fieldPath: '',
+        fieldPath: 'node.aliased_fragment.<abstract-type-hint>',
         kind: 'missing_expected_data.log',
         owner:
           'RelayReaderAliasedFragmentsTestRequiredBubblesOnAbstractWithMissingTypeInfoQuery',

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

@@ -297,7 +297,7 @@ describe('RelayReader @catch', () => {
 
       expect(errorResponseFields).toEqual([
         {
-          fieldPath: '',
+          fieldPath: 'me.firstName',
           kind: 'missing_expected_data.log',
           owner: 'RelayReaderCatchFieldsTestCatchMissingToNullErrorQuery',
         },