Integrate missing required fields into errorResponseFields (#4807)

Summary: This represents the last major step in consolidating how field errors are modeled on Snapshot. Here we move missing required fields into the existing `errorResponseFields` (which should probably get renamed at some point) and are now directly stored in their event shape, rather than needing an intermediate shape. This comes with a few small changes: 1. The decision of if we should throw or not is now guided by the actual event rather than just the presence of `throwOnFieldError` on the fragment being read. This is important because we may inherit events from resolvers that we read. 2. We now throw individual errors based on the first field error we encounter rather than throwing a composite error with lots of metadata attached. In practice I think this is probably better. Many errors are likely uncommon, and having an informative error message is likely to result in a more actionable error report (error reporting infra won't easily pick up the metadata). Advanced users are free to use the FieldLogger to build more advanced errors if they wish 3. We dropped the feature where cascading missing required field errors were not logged. We now log all required field errors (even if some are the result of previous errors) but only the first error will throw. Pull Request resolved: #4807 Reviewed By: itamark Differential Revision: D63563076 Pulled By: captbaritone fbshipit-source-id: bf5b400b0ff143378a8b04f3bb1d74b6f40e0ab3
facebook · Sep 30, 2024 · 535f35c · 535f35c
1 parent 6e1a5c6
commit 535f35c
Show file tree

Hide file tree

Showing 32 changed files with 467 additions and 623 deletions.
diff --git a/packages/react-relay/__tests__/ReactRelayFragmentContainer-WithFragmentOwnership-test.js b/packages/react-relay/__tests__/ReactRelayFragmentContainer-WithFragmentOwnership-test.js
@@ -206,7 +206,6 @@ describe('ReactRelayFragmentContainer with fragment ownership', () => {
         },
         __fragmentOwner: ownerUser1.request,
       },
-      missingRequiredFields: null,
       errorResponseFields: null,
       missingLiveResolverFields: [],
       missingClientEdges: null,
@@ -334,7 +333,6 @@ describe('ReactRelayFragmentContainer with fragment ownership', () => {
         },
         __fragmentOwner: ownerUser2.request,
       },
-      missingRequiredFields: null,
       errorResponseFields: null,
       missingLiveResolverFields: [],
       missingClientEdges: null,
@@ -405,7 +403,6 @@ describe('ReactRelayFragmentContainer with fragment ownership', () => {
         },
         __fragmentOwner: ownerUser1WithCondVar.request,
       },
-      missingRequiredFields: null,
       errorResponseFields: null,
       missingLiveResolverFields: [],
       missingClientEdges: null,

diff --git a/packages/react-relay/__tests__/ReactRelayFragmentContainer-test.js b/packages/react-relay/__tests__/ReactRelayFragmentContainer-test.js
@@ -270,7 +270,6 @@ describe('ReactRelayFragmentContainer', () => {
         id: '4',
         name: 'Zuck',
       },
-      missingRequiredFields: null,
       errorResponseFields: null,
       missingLiveResolverFields: [],
       missingClientEdges: null,
@@ -372,7 +371,6 @@ describe('ReactRelayFragmentContainer', () => {
         name: 'Joe',
       },
       isMissingData: false,
-      missingRequiredFields: null,
       errorResponseFields: null,
       missingLiveResolverFields: [],
       missingClientEdges: null,
@@ -431,7 +429,6 @@ describe('ReactRelayFragmentContainer', () => {
         // Name is excluded since value of cond is now false
       },
       isMissingData: false,
-      missingRequiredFields: null,
       errorResponseFields: null,
       missingLiveResolverFields: [],
       missingClientEdges: null,

diff --git a/packages/react-relay/__tests__/ReactRelayPaginationContainer-test.js b/packages/react-relay/__tests__/ReactRelayPaginationContainer-test.js
@@ -343,7 +343,6 @@ describe('ReactRelayPaginationContainer', () => {
     expect(environment.subscribe.mock.calls[0][0]).toEqual({
       data: expect.any(Object),
       isMissingData: false,
-      missingRequiredFields: null,
       errorResponseFields: null,
       missingLiveResolverFields: [],
       missingClientEdges: null,
@@ -457,7 +456,6 @@ describe('ReactRelayPaginationContainer', () => {
     expect(environment.subscribe.mock.calls[0][0]).toEqual({
       data: expect.any(Object),
       isMissingData: false,
-      missingRequiredFields: null,
       errorResponseFields: null,
       missingLiveResolverFields: [],
       missingClientEdges: null,
@@ -527,7 +525,6 @@ describe('ReactRelayPaginationContainer', () => {
     expect(environment.subscribe.mock.calls[0][0]).toEqual({
       data: expect.any(Object),
       isMissingData: false,
-      missingRequiredFields: null,
       errorResponseFields: null,
       missingLiveResolverFields: [],
       missingClientEdges: null,
@@ -632,7 +629,6 @@ describe('ReactRelayPaginationContainer', () => {
     expect(environment.subscribe.mock.calls[0][0]).toEqual({
       data: expect.any(Object),
       isMissingData: false,
-      missingRequiredFields: null,
       errorResponseFields: null,
       missingLiveResolverFields: [],
       missingClientEdges: null,

diff --git a/packages/react-relay/__tests__/ReactRelayRefetchContainer-test.js b/packages/react-relay/__tests__/ReactRelayRefetchContainer-test.js
@@ -279,7 +279,6 @@ describe('ReactRelayRefetchContainer', () => {
         name: 'Zuck',
       },
       isMissingData: false,
-      missingRequiredFields: null,
       errorResponseFields: null,
       missingLiveResolverFields: [],
       missingClientEdges: null,
@@ -380,7 +379,6 @@ describe('ReactRelayRefetchContainer', () => {
         name: 'Joe',
       },
       isMissingData: false,
-      missingRequiredFields: null,
       errorResponseFields: null,
       missingLiveResolverFields: [],
       missingClientEdges: null,
@@ -440,7 +438,6 @@ describe('ReactRelayRefetchContainer', () => {
         // Name is excluded since value of cond is now false
       },
       isMissingData: false,
-      missingRequiredFields: null,
       errorResponseFields: null,
       missingLiveResolverFields: [],
       missingClientEdges: null,
@@ -527,7 +524,6 @@ describe('ReactRelayRefetchContainer', () => {
         id: '4',
         // Name is excluded since value of cond is now false
       },
-      missingRequiredFields: null,
       errorResponseFields: null,
       missingLiveResolverFields: [],
       missingClientEdges: null,

diff --git a/packages/react-relay/relay-hooks/__tests__/useFragment_nullability-test.js b/packages/react-relay/relay-hooks/__tests__/useFragment_nullability-test.js
@@ -82,7 +82,9 @@ describe('useFragment_nullability-test.js', () => {
     );
     await TestRenderer.act(() => jest.runAllTimers());
     expect(
-      String(renderer.toJSON()).includes('Unexpected response payload'),
+      String(renderer.toJSON()).includes(
+        "Resolver error at path 'field_that_throws' in 'useFragmentNullabilityTest1Query'.",
+      ),
     ).toEqual(true);
   });
 
@@ -110,7 +112,9 @@ describe('useFragment_nullability-test.js', () => {
     );
     await TestRenderer.act(() => jest.runAllTimers());
     expect(
-      String(renderer.toJSON()).includes('Unexpected response payload'),
+      String(renderer.toJSON()).includes(
+        "Resolver error at path 'field_that_throws' in 'useFragmentNullabilityTestFragmentWithFieldThatThrows'.",
+      ),
     ).toEqual(true);
   });
 

diff --git a/packages/react-relay/relay-hooks/legacy/FragmentResource.js b/packages/react-relay/relay-hooks/legacy/FragmentResource.js
@@ -559,19 +559,12 @@ class FragmentResourceImpl {
   _throwOrLogErrorsInSnapshot(snapshot: SingularOrPluralSnapshot) {
     if (Array.isArray(snapshot)) {
       snapshot.forEach(s => {
-        handlePotentialSnapshotErrors(
-          this._environment,
-          s.missingRequiredFields,
-          s.errorResponseFields,
-          s.selector.node.metadata?.throwOnFieldError ?? false,
-        );
+        handlePotentialSnapshotErrors(this._environment, s.errorResponseFields);
       });
     } else {
       handlePotentialSnapshotErrors(
         this._environment,
-        snapshot.missingRequiredFields,
         snapshot.errorResponseFields,
-        snapshot.selector.node.metadata?.throwOnFieldError ?? false,
       );
     }
   }
@@ -771,7 +764,6 @@ class FragmentResourceImpl {
       missingLiveResolverFields: currentSnapshot.missingLiveResolverFields,
       seenRecords: currentSnapshot.seenRecords,
       selector: currentSnapshot.selector,
-      missingRequiredFields: currentSnapshot.missingRequiredFields,
       errorResponseFields: currentSnapshot.errorResponseFields,
     };
     if (updatedData !== renderData) {

diff --git a/packages/react-relay/relay-hooks/readFragmentInternal.js b/packages/react-relay/relay-hooks/readFragmentInternal.js
@@ -85,18 +85,11 @@ function handlePotentialSnapshotErrorsForState(
   if (state.kind === 'singular') {
     handlePotentialSnapshotErrors(
       environment,
-      state.snapshot.missingRequiredFields,
       state.snapshot.errorResponseFields,
-      state.snapshot.selector.node.metadata?.throwOnFieldError ?? false,
     );
   } else if (state.kind === 'plural') {
     for (const snapshot of state.snapshots) {
-      handlePotentialSnapshotErrors(
-        environment,
-        snapshot.missingRequiredFields,
-        snapshot.errorResponseFields,
-        snapshot.selector.node.metadata?.throwOnFieldError ?? false,
-      );
+      handlePotentialSnapshotErrors(environment, snapshot.errorResponseFields);
     }
   }
 }

diff --git a/packages/react-relay/relay-hooks/useFragmentInternal_CURRENT.js b/packages/react-relay/relay-hooks/useFragmentInternal_CURRENT.js
@@ -115,18 +115,11 @@ function handlePotentialSnapshotErrorsForState(
   if (state.kind === 'singular') {
     handlePotentialSnapshotErrors(
       environment,
-      state.snapshot.missingRequiredFields,
       state.snapshot.errorResponseFields,
-      state.snapshot.selector.node.metadata?.throwOnFieldError ?? false,
     );
   } else if (state.kind === 'plural') {
     for (const snapshot of state.snapshots) {
-      handlePotentialSnapshotErrors(
-        environment,
-        snapshot.missingRequiredFields,
-        snapshot.errorResponseFields,
-        snapshot.selector.node.metadata?.throwOnFieldError ?? false,
-      );
+      handlePotentialSnapshotErrors(environment, snapshot.errorResponseFields);
     }
   }
 }
@@ -162,7 +155,6 @@ function handleMissedUpdates(
       missingLiveResolverFields: currentSnapshot.missingLiveResolverFields,
       seenRecords: currentSnapshot.seenRecords,
       selector: currentSnapshot.selector,
-      missingRequiredFields: currentSnapshot.missingRequiredFields,
       errorResponseFields: currentSnapshot.errorResponseFields,
     };
     return [
@@ -187,7 +179,6 @@ function handleMissedUpdates(
         missingLiveResolverFields: currentSnapshot.missingLiveResolverFields,
         seenRecords: currentSnapshot.seenRecords,
         selector: currentSnapshot.selector,
-        missingRequiredFields: currentSnapshot.missingRequiredFields,
         errorResponseFields: currentSnapshot.errorResponseFields,
       };
       if (updatedData !== snapshot.data) {

diff --git a/packages/react-relay/relay-hooks/useFragmentInternal_EXPERIMENTAL.js b/packages/react-relay/relay-hooks/useFragmentInternal_EXPERIMENTAL.js
@@ -127,18 +127,11 @@ function handlePotentialSnapshotErrorsForState(
   if (state.kind === 'singular') {
     handlePotentialSnapshotErrors(
       environment,
-      state.snapshot.missingRequiredFields,
       state.snapshot.errorResponseFields,
-      state.snapshot.selector.node.metadata?.throwOnFieldError ?? false,
     );
   } else if (state.kind === 'plural') {
     for (const snapshot of state.snapshots) {
-      handlePotentialSnapshotErrors(
-        environment,
-        snapshot.missingRequiredFields,
-        snapshot.errorResponseFields,
-        snapshot.selector.node.metadata?.throwOnFieldError ?? false,
-      );
+      handlePotentialSnapshotErrors(environment, snapshot.errorResponseFields);
     }
   }
 }
@@ -174,7 +167,6 @@ function handleMissedUpdates(
       missingLiveResolverFields: currentSnapshot.missingLiveResolverFields,
       seenRecords: currentSnapshot.seenRecords,
       selector: currentSnapshot.selector,
-      missingRequiredFields: currentSnapshot.missingRequiredFields,
       errorResponseFields: currentSnapshot.errorResponseFields,
     };
     return [
@@ -201,7 +193,6 @@ function handleMissedUpdates(
         missingLiveResolverFields: currentSnapshot.missingLiveResolverFields,
         seenRecords: currentSnapshot.seenRecords,
         selector: currentSnapshot.selector,
-        missingRequiredFields: currentSnapshot.missingRequiredFields,
         errorResponseFields: currentSnapshot.errorResponseFields,
       };
       if (updatedData !== snapshot.data) {

diff --git a/packages/relay-runtime/index.js b/packages/relay-runtime/index.js
@@ -134,7 +134,6 @@ export type {
   LogEvent,
   LogFunction,
   MissingFieldHandler,
-  MissingRequiredFields,
   ModuleImportPointer,
   MutableRecordSource,
   MutationParameters,

diff --git a/packages/relay-runtime/query/fetchQuery.js b/packages/relay-runtime/query/fetchQuery.js
@@ -136,12 +136,7 @@ function fetchQuery<TVariables: Variables, TData, TRawResponse>(
   const fetchPolicy = options?.fetchPolicy ?? 'network-only';
 
   function readData(snapshot: Snapshot): TData {
-    handlePotentialSnapshotErrors(
-      environment,
-      snapshot.missingRequiredFields,
-      snapshot.errorResponseFields,
-      queryNode.fragment.metadata?.throwOnFieldError ?? false,
-    );
+    handlePotentialSnapshotErrors(environment, snapshot.errorResponseFields);
     /* $FlowFixMe[incompatible-return] we assume readData returns the right
      * data just having written it from network or checked availability. */
     return snapshot.data;

diff --git a/packages/relay-runtime/store/RelayErrorTrie.js b/packages/relay-runtime/store/RelayErrorTrie.js
@@ -16,16 +16,6 @@ import type {PayloadError} from '../network/RelayNetworkTypes';
 // $FlowFixMe[recursive-definition]
 const SELF: Self = Symbol('$SELF');
 
-class RelayFieldError extends Error {
-  constructor(message: string, errors: Array<TRelayFieldErrorForDisplay> = []) {
-    super(message);
-    this.name = 'RelayFieldError';
-    this.message = message;
-    this.errors = errors;
-  }
-  errors: Array<TRelayFieldErrorForDisplay>;
-}
-
 export opaque type Self = typeof SELF;
 
 export type TRelayFieldErrorForDisplay = $ReadOnly<{
@@ -180,11 +170,9 @@ module.exports = ({
   buildErrorTrie,
   getNestedErrorTrieByKey,
   getErrorsByKey,
-  RelayFieldError,
 }: {
   SELF: typeof SELF,
   buildErrorTrie: typeof buildErrorTrie,
   getNestedErrorTrieByKey: typeof getNestedErrorTrieByKey,
   getErrorsByKey: typeof getErrorsByKey,
-  RelayFieldError: Class<RelayFieldError>,
 });
diff --git a/packages/relay-runtime/store/RelayModernFragmentSpecResolver.js b/packages/relay-runtime/store/RelayModernFragmentSpecResolver.js
@@ -19,7 +19,6 @@ import type {
   FragmentSpecResolver,
   FragmentSpecResults,
   IEnvironment,
-  MissingRequiredFields,
   PluralReaderSelector,
   RelayContext,
   SelectorData,
@@ -227,7 +226,6 @@ class SelectorResolver {
   _data: ?SelectorData;
   _environment: IEnvironment;
   _isMissingData: boolean;
-  _missingRequiredFields: ?MissingRequiredFields;
   _errorResponseFields: ?ErrorResponseFields;
   _rootIsQueryRenderer: boolean;
   _selector: SingularReaderSelector;
@@ -244,7 +242,6 @@ class SelectorResolver {
     this._callback = callback;
     this._data = snapshot.data;
     this._isMissingData = snapshot.isMissingData;
-    this._missingRequiredFields = snapshot.missingRequiredFields;
     this._errorResponseFields = snapshot.errorResponseFields;
     this._environment = environment;
     this._rootIsQueryRenderer = rootIsQueryRenderer;
@@ -326,12 +323,7 @@ class SelectorResolver {
         }
       }
     }
-    handlePotentialSnapshotErrors(
-      this._environment,
-      this._missingRequiredFields,
-      this._errorResponseFields,
-      this._selector.node.metadata?.throwOnFieldError ?? false,
-    );
+    handlePotentialSnapshotErrors(this._environment, this._errorResponseFields);
     return this._data;
   }
 
@@ -346,7 +338,6 @@ class SelectorResolver {
     const snapshot = this._environment.lookup(selector);
     this._data = recycleNodesInto(this._data, snapshot.data);
     this._isMissingData = snapshot.isMissingData;
-    this._missingRequiredFields = snapshot.missingRequiredFields;
     this._errorResponseFields = snapshot.errorResponseFields;
     this._selector = selector;
     this._subscription = this._environment.subscribe(snapshot, this._onChange);
@@ -383,7 +374,6 @@ class SelectorResolver {
   _onChange = (snapshot: Snapshot): void => {
     this._data = snapshot.data;
     this._isMissingData = snapshot.isMissingData;
-    this._missingRequiredFields = snapshot.missingRequiredFields;
     this._errorResponseFields = snapshot.errorResponseFields;
     this._callback();
   };