Use field error events on snapshot (#4798)

Summary: Previously we had two different object shapes which were nearly the same. One for stashing on the snapshot, and another for logging. By unifying the two shapes we simplify things a bit and avoid the transformation. As part of this simplification, I've also removed the special case handling for snapshots which contain only missing data errors. This means we will end up with duplicate events if the snapshot is missing multiple fields. This is noisier, but perhaps more correct because it captures the fact that multiple fields were missing. Once/when/if we add runtime pathName to RelayReader, these multiple events will be even more helpful. Pull Request resolved: #4798 Reviewed By: gordyf Differential Revision: D63032950 Pulled By: captbaritone fbshipit-source-id: 30fc941134e641eb31f1440f2d9e066f23f07621
facebook · Sep 19, 2024 · 55795a1 · 55795a1
1 parent 790d468
commit 55795a1
Show file tree

Hide file tree

Showing 10 changed files with 149 additions and 177 deletions.
diff --git a/packages/react-relay/__tests__/RelayResolverNullableModelClientEdge-test.js b/packages/react-relay/__tests__/RelayResolverNullableModelClientEdge-test.js
@@ -503,9 +503,9 @@ describe.each([true, false])(
       expect(snapshot.relayResolverErrors).toEqual([
         {
           error: Error(ERROR_MESSAGE),
-          kind: 'relay_resolver.error',
           owner: 'RelayResolverNullableModelClientEdgeTest_ErrorModel_Query',
           fieldPath: 'edge_to_model_that_throws.__relay_model_instance',
+          kind: 'relay_resolver.error',
         },
       ]);
       const data: $FlowExpectedError = snapshot.data;
@@ -527,17 +527,17 @@ describe.each([true, false])(
       expect(snapshot.relayResolverErrors).toEqual([
         {
           error: Error(ERROR_MESSAGE),
-          kind: 'relay_resolver.error',
           owner:
             'RelayResolverNullableModelClientEdgeTest_PluralErrorModel_Query',
           fieldPath: 'edge_to_plural_models_that_throw.__relay_model_instance',
+          kind: 'relay_resolver.error',
         },
         {
           error: Error(ERROR_MESSAGE),
-          kind: 'relay_resolver.error',
           owner:
             'RelayResolverNullableModelClientEdgeTest_PluralErrorModel_Query',
           fieldPath: 'edge_to_plural_models_that_throw.__relay_model_instance',
+          kind: 'relay_resolver.error',
         },
       ]);
       const data: $FlowExpectedError = snapshot.data;
@@ -559,10 +559,10 @@ describe.each([true, false])(
       expect(snapshot.relayResolverErrors).toEqual([
         {
           error: Error(ERROR_MESSAGE),
-          kind: 'relay_resolver.error',
           owner:
             'RelayResolverNullableModelClientEdgeTest_PluralSomeErrorModel_Query',
           fieldPath: 'edge_to_plural_models_some_throw.__relay_model_instance',
+          kind: 'relay_resolver.error',
         },
       ]);
       const data: $FlowExpectedError = snapshot.data;

diff --git a/packages/relay-runtime/store/RelayReader.js b/packages/relay-runtime/store/RelayReader.js
@@ -215,39 +215,30 @@ class RelayReader {
     }
     for (const error of errors) {
       this._errorResponseFields.push({
+        kind: 'relay_field_payload.error',
         owner,
-        path: (error.path ?? []).join('.'),
-        type: 'PAYLOAD_ERROR',
+        fieldPath: (error.path ?? []).join('.'),
         error,
+        shouldThrow: this._selector.node.metadata?.throwOnFieldError ?? false,
       });
     }
   }
 
   _markDataAsMissing(): void {
-    // We want to mark multiple missing_data - but not duplicate ones.
-    const alreadyInErrors = Boolean(
-      this._errorResponseFields?.some(
-        err => err.owner === this._fragmentName && err.type === 'MISSING_DATA',
-      ),
-    );
-
-    if (!alreadyInErrors) {
-      if (this._errorResponseFields == null) {
-        this._errorResponseFields = [];
-      }
-      this._errorResponseFields.push({
-        owner: this._fragmentName,
-        // we will add the path later
-        path: '',
-        type: 'MISSING_DATA',
-        error: {
-          message:
-            'Relay: Missing data for one or more fields in ' +
-            this._fragmentName,
-        },
-      });
+    if (this._errorResponseFields == null) {
+      this._errorResponseFields = [];
     }
 
+    // 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}
+        : {kind: 'missing_expected_data.log', owner, fieldPath},
+    );
+
     this._isMissingData = true;
 
     if (this._clientEdgeTraversalPath.length) {
@@ -346,7 +337,24 @@ class RelayReader {
       "Couldn't determine field name for this field. It might be a ReaderClientExtension - which is not yet supported.",
     );
 
-    let errors = this._errorResponseFields?.map(error => error.error);
+    let errors = this._errorResponseFields?.map(error => {
+      switch (error.kind) {
+        case 'relay_field_payload.error':
+          return error.error;
+        case 'missing_expected_data.throw':
+        case 'missing_expected_data.log':
+          return {
+            message: `Relay: Missing data for one or more fields in ${error.owner}`,
+          };
+        default:
+          (error.kind: empty);
+          invariant(
+            false,
+            'Unexpected error errorResponseField kind: %s',
+            error.kind,
+          );
+      }
+    });
 
     if (this._resolverErrors.length > 0) {
       if (errors == null) {

diff --git a/packages/relay-runtime/store/RelayStoreTypes.js b/packages/relay-runtime/store/RelayStoreTypes.js
@@ -32,7 +32,6 @@ import type {
   NormalizationSelectableNode,
 } from '../util/NormalizationNode';
 import type {
-  CatchFieldTo,
   ReaderClientEdgeToServerObject,
   ReaderFragment,
   ReaderLinkedField,
@@ -122,19 +121,16 @@ type FieldLocation = {
   owner: string,
 };
 
-type ErrorFieldLocation = {
-  ...FieldLocation,
-  error: TRelayFieldError,
-  type: FieldErrorType,
-  to?: CatchFieldTo,
-};
-
 export type MissingRequiredFields = $ReadOnly<
   | {action: 'THROW', field: FieldLocation}
   | {action: 'LOG', fields: Array<FieldLocation>},
 >;
 
-export type ErrorResponseFields = Array<ErrorFieldLocation>;
+export type ErrorResponseFields = Array<
+  | RelayFieldPayloadErrorEvent
+  | MissingExpectedDataLogEvent
+  | MissingExpectedDataThrowEvent,
+>;
 
 export type ClientEdgeTraversalInfo = {
   +readerClientEdge: ReaderClientEdgeToServerObject,
@@ -1361,8 +1357,7 @@ export type RelayFieldPayloadErrorEvent = {
   +owner: string,
   +fieldPath: string,
   +error: TRelayFieldError,
-  // TODO: Should we add `shouldThrow` as a flag here, or perhaps have two
-  // different event types?
+  +shouldThrow: boolean,
 };
 
 /**

diff --git a/packages/relay-runtime/store/__tests__/RelayModernStore-Subscriptions-test.js b/packages/relay-runtime/store/__tests__/RelayModernStore-Subscriptions-test.js
@@ -419,13 +419,14 @@ function cloneEventWithSets(event: LogEvent) {
           isMissingData: true,
           errorResponseFields: [
             {
-              error: {
-                message:
-                  'Relay: Missing data for one or more fields in RelayModernStoreSubscriptionsTest1Fragment',
-              },
+              fieldPath: '',
+              kind: 'missing_expected_data.log',
+              owner: 'RelayModernStoreSubscriptionsTest1Fragment',
+            },
+            {
+              fieldPath: '',
+              kind: 'missing_expected_data.log',
               owner: 'RelayModernStoreSubscriptionsTest1Fragment',
-              type: 'MISSING_DATA',
-              path: '',
             },
           ],
           seenRecords: new Set(Object.keys(nextSource.toJSON())),
@@ -468,13 +469,14 @@ function cloneEventWithSets(event: LogEvent) {
           missingRequiredFields: null,
           errorResponseFields: [
             {
-              error: {
-                message:
-                  'Relay: Missing data for one or more fields in RelayModernStoreSubscriptionsTest1Fragment',
-              },
+              fieldPath: '',
+              kind: 'missing_expected_data.log',
+              owner: 'RelayModernStoreSubscriptionsTest1Fragment',
+            },
+            {
+              fieldPath: '',
+              kind: 'missing_expected_data.log',
               owner: 'RelayModernStoreSubscriptionsTest1Fragment',
-              type: 'MISSING_DATA',
-              path: '',
             },
           ],
           missingLiveResolverFields: [],

diff --git a/packages/relay-runtime/store/__tests__/RelayModernStore-test.js b/packages/relay-runtime/store/__tests__/RelayModernStore-test.js
@@ -728,6 +728,18 @@ function cloneEventWithSets(event: LogEvent) {
             name: 'Joe',
             profilePicture: undefined,
           },
+          errorResponseFields: [
+            {
+              owner: 'RelayModernStoreTest5Fragment',
+              kind: 'missing_expected_data.log',
+              fieldPath: '',
+            },
+            {
+              owner: 'RelayModernStoreTest5Fragment',
+              kind: 'missing_expected_data.log',
+              fieldPath: '',
+            },
+          ],
           missingRequiredFields: null,
           missingLiveResolverFields: [],
           missingClientEdges: null,
@@ -774,13 +786,14 @@ function cloneEventWithSets(event: LogEvent) {
           isMissingData: true,
           errorResponseFields: [
             {
-              error: {
-                message:
-                  'Relay: Missing data for one or more fields in RelayModernStoreTest5Fragment',
-              },
               owner: 'RelayModernStoreTest5Fragment',
-              type: 'MISSING_DATA',
-              path: '',
+              kind: 'missing_expected_data.log',
+              fieldPath: '',
+            },
+            {
+              owner: 'RelayModernStoreTest5Fragment',
+              kind: 'missing_expected_data.log',
+              fieldPath: '',
             },
           ],
           seenRecords: new Set(['842472']),

diff --git a/packages/relay-runtime/store/__tests__/RelayReader-CatchFields-test.js b/packages/relay-runtime/store/__tests__/RelayReader-CatchFields-test.js
@@ -140,13 +140,14 @@ describe('RelayReader @catch', () => {
 
       expect(errorResponseFields).toEqual([
         {
-          path: 'me.lastName',
+          fieldPath: 'me.lastName',
           error: {
             message: 'There was an error!',
             path: ['me', 'lastName'],
           },
           owner: 'RelayReaderCatchFieldsTestSiblingErrorQuery',
-          type: 'PAYLOAD_ERROR',
+          kind: 'relay_field_payload.error',
+          shouldThrow: false,
         },
       ]);
     });

diff --git a/packages/relay-runtime/store/__tests__/RelayReader-RelayErrorHandling-test.js b/packages/relay-runtime/store/__tests__/RelayReader-RelayErrorHandling-test.js
@@ -54,12 +54,13 @@ describe('RelayReader error fields', () => {
     expect(errorResponseFields).toEqual([
       {
         owner: 'RelayReaderRelayErrorHandlingTest1Query',
-        path: 'me.lastName',
+        fieldPath: 'me.lastName',
         error: {
           message: 'There was an error!',
           path: ['me', 'lastName'],
         },
-        type: 'PAYLOAD_ERROR',
+        kind: 'relay_field_payload.error',
+        shouldThrow: false,
       },
     ]);
   });
@@ -104,21 +105,18 @@ describe('RelayReader error fields', () => {
     expect(errorResponseFields).toEqual([
       {
         owner: 'RelayReaderRelayErrorHandlingTest4Query',
-        path: 'me.lastName',
-        type: 'PAYLOAD_ERROR',
+        fieldPath: 'me.lastName',
+        kind: 'relay_field_payload.error',
         error: {
           message: 'There was an error!',
           path: ['me', 'lastName'],
         },
+        shouldThrow: true,
       },
       {
         owner: 'RelayReaderRelayErrorHandlingTest4Query',
-        path: '',
-        type: 'MISSING_DATA',
-        error: {
-          message:
-            'Relay: Missing data for one or more fields in RelayReaderRelayErrorHandlingTest4Query',
-        },
+        fieldPath: '',
+        kind: 'missing_expected_data.throw',
       },
     ]);
   });

diff --git a/packages/relay-runtime/store/__tests__/RelayReader-Resolver-test.js b/packages/relay-runtime/store/__tests__/RelayReader-Resolver-test.js
@@ -208,10 +208,11 @@ describe.each([true, false])(
           const {errorResponseFields} = store.lookup(operation.fragment);
           expect(errorResponseFields).toEqual([
             {
+              fieldPath: 'me.lastName',
+              kind: 'relay_field_payload.error',
               error: {message: 'There was an error!', path: ['me', 'lastName']},
               owner: 'RelayReaderResolverTestFieldErrorQuery',
-              type: 'PAYLOAD_ERROR',
-              path: 'me.lastName',
+              shouldThrow: false,
             },
           ]);
         });