Unify resolver errors as just another field error (#4802)

Summary: This continues our work to unify how errors are handled on snapshots by unifying resolver errors as part of the larger field error array, rather than treating them as a separate field. Some bugs I noticed as part of this refactor which are not resolved here, but should be in subsequent PRs/diffs: 1. Resolver root fragments with `throwOnFieldError` that encounter a field error should 1. Prevent the rest of the resolver from getting run 2. Should cause the resolver value to become `null` (with a field error) 3. Should not cause the reader of the resolver to throw (even if _that_ fragment has `throwOnFieldError`) 4. Should still get logged to the field logger Pull Request resolved: #4802 Reviewed By: gordyf Differential Revision: D63295872 Pulled By: captbaritone fbshipit-source-id: 5c4a01e755c61332d1f67bbc999da0165a0f7f11
facebook · Sep 27, 2024 · b619919 · b619919
1 parent 2c8089a
commit b619919
Show file tree

Hide file tree

Showing 26 changed files with 85 additions and 143 deletions.
diff --git a/packages/react-relay/__tests__/LiveResolvers-test.js b/packages/react-relay/__tests__/LiveResolvers-test.js
@@ -1011,6 +1011,7 @@ describe.each([true, false])(
  fieldPath: 'node.resolver_that_throws',
  kind: 'relay_resolver.error',
  owner: 'LiveResolversTest8Query',
+ shouldThrow: false,
  },
  ],
  ]);
@@ -1707,12 +1708,13 @@ describe.each([true, false])(
  });
 
  const data = environment.lookup(operation.fragment);
- expect(data.relayResolverErrors).toEqual([
+ expect(data.errorResponseFields).toEqual([
  {
  kind: 'relay_resolver.error',
  owner: 'LiveResolversTest18Query',
  fieldPath: 'live_resolver_throws',
  error: new Error('What?'),
+ shouldThrow: false,
  },
  ]);
  });
@@ -1761,7 +1763,7 @@ describe.each([true, false])(
  });
 
  const snapshot = environment.lookup(operation.fragment);
- expect(snapshot.relayResolverErrors).toEqual([]);
+ expect(snapshot.errorResponseFields).toEqual(null);
  expect(snapshot.data).toEqual({
  hello_world_with_provided_variable: 'Hello, Hello, World!!',
  });
@@ -1786,7 +1788,7 @@ describe.each([true, false])(
  });
 
  const snapshot = environment.lookup(operation.fragment);
- expect(snapshot.relayResolverErrors).toEqual([]);
+ expect(snapshot.errorResponseFields).toEqual(null);
  expect(snapshot.data).toEqual({
  hello_world_with_context: 'Hello Hello Allemaal!!',
  });
@@ -1813,7 +1815,7 @@ describe.each([true, false])(
  });
 
  const snapshot = environment.lookup(operation.fragment);
- expect(snapshot.relayResolverErrors).toEqual([]);
+ expect(snapshot.errorResponseFields).toEqual(null);
  expect(snapshot.data).toEqual({
  hello_world_with_context_object: 'Hello Hello Allemaal!!',
  });

diff --git a/packages/react-relay/__tests__/ReactRelayFragmentContainer-WithFragmentOwnership-test.js b/packages/react-relay/__tests__/ReactRelayFragmentContainer-WithFragmentOwnership-test.js
@@ -209,7 +209,6 @@ describe('ReactRelayFragmentContainer with fragment ownership', () => {
  missingRequiredFields: null,
  errorResponseFields: null,
  missingLiveResolverFields: [],
- relayResolverErrors: [],
  missingClientEdges: null,
  isMissingData: false,
  seenRecords: expect.any(Object),
@@ -254,7 +253,6 @@ describe('ReactRelayFragmentContainer with fragment ownership', () => {
  },
  __fragmentOwner: ownerUser1.request,
  },
- relayResolverErrors: [],
  seenRecords: {},
  isMissingData: false,
  }),
@@ -339,7 +337,6 @@ describe('ReactRelayFragmentContainer with fragment ownership', () => {
  missingRequiredFields: null,
  errorResponseFields: null,
  missingLiveResolverFields: [],
- relayResolverErrors: [],
  missingClientEdges: null,
  isMissingData: false,
  seenRecords: expect.any(Object),
@@ -411,7 +408,6 @@ describe('ReactRelayFragmentContainer with fragment ownership', () => {
  missingRequiredFields: null,
  errorResponseFields: null,
  missingLiveResolverFields: [],
- relayResolverErrors: [],
  missingClientEdges: null,
  isMissingData: false,
  seenRecords: expect.any(Object),

diff --git a/packages/react-relay/__tests__/ReactRelayFragmentContainer-test.js b/packages/react-relay/__tests__/ReactRelayFragmentContainer-test.js
@@ -273,7 +273,6 @@ describe('ReactRelayFragmentContainer', () => {
  missingRequiredFields: null,
  errorResponseFields: null,
  missingLiveResolverFields: [],
- relayResolverErrors: [],
  missingClientEdges: null,
  isMissingData: false,
  seenRecords: expect.any(Object),
@@ -312,7 +311,6 @@ describe('ReactRelayFragmentContainer', () => {
  name: 'Mark', // !== 'Zuck'
  },
  seenRecords: {},
- relayResolverErrors: [],
  }),
  );
 
@@ -377,7 +375,6 @@ describe('ReactRelayFragmentContainer', () => {
  missingRequiredFields: null,
  errorResponseFields: null,
  missingLiveResolverFields: [],
- relayResolverErrors: [],
  missingClientEdges: null,
  seenRecords: expect.any(Object),
  selector: createReaderSelector(
@@ -437,7 +434,6 @@ describe('ReactRelayFragmentContainer', () => {
  missingRequiredFields: null,
  errorResponseFields: null,
  missingLiveResolverFields: [],
- relayResolverErrors: [],
  missingClientEdges: null,
  seenRecords: expect.any(Object),
  selector: createReaderSelector(

diff --git a/packages/react-relay/__tests__/ReactRelayPaginationContainer-test.js b/packages/react-relay/__tests__/ReactRelayPaginationContainer-test.js
@@ -346,7 +346,6 @@ describe('ReactRelayPaginationContainer', () => {
  missingRequiredFields: null,
  errorResponseFields: null,
  missingLiveResolverFields: [],
- relayResolverErrors: [],
  missingClientEdges: null,
  seenRecords: expect.any(Object),
  selector: createReaderSelector(
@@ -389,7 +388,6 @@ describe('ReactRelayPaginationContainer', () => {
  friends: null, // set to null
  },
  seenRecords: {},
- relayResolverErrors: [],
  });
  });
 
@@ -462,7 +460,6 @@ describe('ReactRelayPaginationContainer', () => {
  missingRequiredFields: null,
  errorResponseFields: null,
  missingLiveResolverFields: [],
- relayResolverErrors: [],
  missingClientEdges: null,
  seenRecords: expect.any(Object),
  selector: createReaderSelector(
@@ -533,7 +530,6 @@ describe('ReactRelayPaginationContainer', () => {
  missingRequiredFields: null,
  errorResponseFields: null,
  missingLiveResolverFields: [],
- relayResolverErrors: [],
  missingClientEdges: null,
  seenRecords: expect.any(Object),
  selector: createReaderSelector(
@@ -639,7 +635,6 @@ describe('ReactRelayPaginationContainer', () => {
  missingRequiredFields: null,
  errorResponseFields: null,
  missingLiveResolverFields: [],
- relayResolverErrors: [],
  missingClientEdges: null,
  seenRecords: expect.any(Object),
  selector: createReaderSelector(

diff --git a/packages/react-relay/__tests__/ReactRelayRefetchContainer-test.js b/packages/react-relay/__tests__/ReactRelayRefetchContainer-test.js
@@ -282,7 +282,6 @@ describe('ReactRelayRefetchContainer', () => {
  missingRequiredFields: null,
  errorResponseFields: null,
  missingLiveResolverFields: [],
- relayResolverErrors: [],
  missingClientEdges: null,
  seenRecords: expect.any(Object),
  selector: createReaderSelector(
@@ -319,7 +318,6 @@ describe('ReactRelayRefetchContainer', () => {
  name: 'Mark', // !== 'Zuck'
  },
  seenRecords: {},
- relayResolverErrors: [],
  });
  });
 
@@ -385,7 +383,6 @@ describe('ReactRelayRefetchContainer', () => {
  missingRequiredFields: null,
  errorResponseFields: null,
  missingLiveResolverFields: [],
- relayResolverErrors: [],
  missingClientEdges: null,
  seenRecords: expect.any(Object),
  selector: createReaderSelector(
@@ -446,7 +443,6 @@ describe('ReactRelayRefetchContainer', () => {
  missingRequiredFields: null,
  errorResponseFields: null,
  missingLiveResolverFields: [],
- relayResolverErrors: [],
  missingClientEdges: null,
  seenRecords: expect.any(Object),
  selector: createReaderSelector(
@@ -534,7 +530,6 @@ describe('ReactRelayRefetchContainer', () => {
  missingRequiredFields: null,
  errorResponseFields: null,
  missingLiveResolverFields: [],
- relayResolverErrors: [],
  missingClientEdges: null,
  isMissingData: false,
  seenRecords: expect.any(Object),

diff --git a/packages/react-relay/__tests__/RelayResolverNullableModelClientEdge-test.js b/packages/react-relay/__tests__/RelayResolverNullableModelClientEdge-test.js
@@ -500,12 +500,13 @@ describe.each([true, false])(
  {},
  );
  const snapshot = environment.lookup(operation.fragment);
- expect(snapshot.relayResolverErrors).toEqual([
+ expect(snapshot.errorResponseFields).toEqual([
  {
  error: Error(ERROR_MESSAGE),
  owner: 'RelayResolverNullableModelClientEdgeTest_ErrorModel_Query',
  fieldPath: 'edge_to_model_that_throws.__relay_model_instance',
  kind: 'relay_resolver.error',
+ shouldThrow: false,
  },
  ]);
  const data: $FlowExpectedError = snapshot.data;
@@ -524,20 +525,22 @@ describe.each([true, false])(
  {},
  );
  const snapshot = environment.lookup(operation.fragment);
- expect(snapshot.relayResolverErrors).toEqual([
+ expect(snapshot.errorResponseFields).toEqual([
  {
  error: Error(ERROR_MESSAGE),
  owner:
  'RelayResolverNullableModelClientEdgeTest_PluralErrorModel_Query',
  fieldPath: 'edge_to_plural_models_that_throw.__relay_model_instance',
  kind: 'relay_resolver.error',
+ shouldThrow: false,
  },
  {
  error: Error(ERROR_MESSAGE),
  owner:
  'RelayResolverNullableModelClientEdgeTest_PluralErrorModel_Query',
  fieldPath: 'edge_to_plural_models_that_throw.__relay_model_instance',
  kind: 'relay_resolver.error',
+ shouldThrow: false,
  },
  ]);
  const data: $FlowExpectedError = snapshot.data;
@@ -556,13 +559,14 @@ describe.each([true, false])(
  {},
  );
  const snapshot = environment.lookup(operation.fragment);
- expect(snapshot.relayResolverErrors).toEqual([
+ expect(snapshot.errorResponseFields).toEqual([
  {
  error: Error(ERROR_MESSAGE),
  owner:
  'RelayResolverNullableModelClientEdgeTest_PluralSomeErrorModel_Query',
  fieldPath: 'edge_to_plural_models_some_throw.__relay_model_instance',
  kind: 'relay_resolver.error',
+ shouldThrow: false,
  },
  ]);
  const data: $FlowExpectedError = snapshot.data;

diff --git a/packages/react-relay/relay-hooks/__tests__/FragmentResource-Resolver-test.js b/packages/react-relay/relay-hooks/__tests__/FragmentResource-Resolver-test.js
@@ -100,6 +100,7 @@ describe('FragmentResource RelayResolver behavior', () => {
  fieldPath: 'always_throws',
  kind: 'relay_resolver.error',
  owner: 'FragmentResourceResolverTestFragment1',
+ shouldThrow: false,
  });
  expect(event.error.message).toEqual('I always throw. What did you expect?');
  });

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,7 @@ describe('useFragment_nullability-test.js', () => {
  );
  await TestRenderer.act(() => jest.runAllTimers());
  expect(
- String(renderer.toJSON()).includes('Unexpected resolver exception'),
+ String(renderer.toJSON()).includes('Unexpected response payload'),
  ).toEqual(true);
  });
 
@@ -110,7 +110,7 @@ describe('useFragment_nullability-test.js', () => {
  );
  await TestRenderer.act(() => jest.runAllTimers());
  expect(
- String(renderer.toJSON()).includes('Unexpected resolver exception'),
+ String(renderer.toJSON()).includes('Unexpected response payload'),
  ).toEqual(true);
  });
 
@@ -138,7 +138,7 @@ describe('useFragment_nullability-test.js', () => {
  );
  await TestRenderer.act(() => jest.runAllTimers());
  expect(
- String(renderer.toJSON()).includes('Unexpected resolver exception'),
+ String(renderer.toJSON()).includes('Unexpected response payload'),
  ).toEqual(false);
  });
 });

diff --git a/packages/react-relay/relay-hooks/legacy/FragmentResource.js b/packages/react-relay/relay-hooks/legacy/FragmentResource.js
@@ -562,7 +562,6 @@ class FragmentResourceImpl {
  handlePotentialSnapshotErrors(
  this._environment,
  s.missingRequiredFields,
- s.relayResolverErrors,
  s.errorResponseFields,
  s.selector.node.metadata?.throwOnFieldError ?? false,
  );
@@ -571,7 +570,6 @@ class FragmentResourceImpl {
  handlePotentialSnapshotErrors(
  this._environment,
  snapshot.missingRequiredFields,
- snapshot.relayResolverErrors,
  snapshot.errorResponseFields,
  snapshot.selector.node.metadata?.throwOnFieldError ?? false,
  );
@@ -774,7 +772,6 @@ class FragmentResourceImpl {
  seenRecords: currentSnapshot.seenRecords,
  selector: currentSnapshot.selector,
  missingRequiredFields: currentSnapshot.missingRequiredFields,
- relayResolverErrors: currentSnapshot.relayResolverErrors,
  errorResponseFields: currentSnapshot.errorResponseFields,
  };
  if (updatedData !== renderData) {

diff --git a/packages/react-relay/relay-hooks/readFragmentInternal.js b/packages/react-relay/relay-hooks/readFragmentInternal.js
@@ -86,7 +86,6 @@ function handlePotentialSnapshotErrorsForState(
  handlePotentialSnapshotErrors(
  environment,
  state.snapshot.missingRequiredFields,
- state.snapshot.relayResolverErrors,
  state.snapshot.errorResponseFields,
  state.snapshot.selector.node.metadata?.throwOnFieldError ?? false,
  );
@@ -95,7 +94,6 @@ function handlePotentialSnapshotErrorsForState(
  handlePotentialSnapshotErrors(
  environment,
  snapshot.missingRequiredFields,
- snapshot.relayResolverErrors,
  snapshot.errorResponseFields,
  snapshot.selector.node.metadata?.throwOnFieldError ?? false,
  );

diff --git a/packages/react-relay/relay-hooks/useFragmentInternal_CURRENT.js b/packages/react-relay/relay-hooks/useFragmentInternal_CURRENT.js
@@ -116,7 +116,6 @@ function handlePotentialSnapshotErrorsForState(
  handlePotentialSnapshotErrors(
  environment,
  state.snapshot.missingRequiredFields,
- state.snapshot.relayResolverErrors,
  state.snapshot.errorResponseFields,
  state.snapshot.selector.node.metadata?.throwOnFieldError ?? false,
  );
@@ -125,7 +124,6 @@ function handlePotentialSnapshotErrorsForState(
  handlePotentialSnapshotErrors(
  environment,
  snapshot.missingRequiredFields,
- snapshot.relayResolverErrors,
  snapshot.errorResponseFields,
  snapshot.selector.node.metadata?.throwOnFieldError ?? false,
  );
@@ -165,7 +163,6 @@ function handleMissedUpdates(
  seenRecords: currentSnapshot.seenRecords,
  selector: currentSnapshot.selector,
  missingRequiredFields: currentSnapshot.missingRequiredFields,
- relayResolverErrors: currentSnapshot.relayResolverErrors,
  errorResponseFields: currentSnapshot.errorResponseFields,
  };
  return [
@@ -191,7 +188,6 @@ function handleMissedUpdates(
  seenRecords: currentSnapshot.seenRecords,
  selector: currentSnapshot.selector,
  missingRequiredFields: currentSnapshot.missingRequiredFields,
- relayResolverErrors: currentSnapshot.relayResolverErrors,
  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
@@ -128,7 +128,6 @@ function handlePotentialSnapshotErrorsForState(
  handlePotentialSnapshotErrors(
  environment,
  state.snapshot.missingRequiredFields,
- state.snapshot.relayResolverErrors,
  state.snapshot.errorResponseFields,
  state.snapshot.selector.node.metadata?.throwOnFieldError ?? false,
  );
@@ -137,7 +136,6 @@ function handlePotentialSnapshotErrorsForState(
  handlePotentialSnapshotErrors(
  environment,
  snapshot.missingRequiredFields,
- snapshot.relayResolverErrors,
  snapshot.errorResponseFields,
  snapshot.selector.node.metadata?.throwOnFieldError ?? false,
  );
@@ -177,7 +175,6 @@ function handleMissedUpdates(
  seenRecords: currentSnapshot.seenRecords,
  selector: currentSnapshot.selector,
  missingRequiredFields: currentSnapshot.missingRequiredFields,
- relayResolverErrors: currentSnapshot.relayResolverErrors,
  errorResponseFields: currentSnapshot.errorResponseFields,
  };
  return [
@@ -205,7 +202,6 @@ function handleMissedUpdates(
  seenRecords: currentSnapshot.seenRecords,
  selector: currentSnapshot.selector,
  missingRequiredFields: currentSnapshot.missingRequiredFields,
- relayResolverErrors: currentSnapshot.relayResolverErrors,
  errorResponseFields: currentSnapshot.errorResponseFields,
  };
  if (updatedData !== snapshot.data) {

diff --git a/packages/relay-runtime/query/fetchQuery.js b/packages/relay-runtime/query/fetchQuery.js
@@ -139,7 +139,6 @@ function fetchQuery<TVariables: Variables, TData, TRawResponse>(
  handlePotentialSnapshotErrors(
  environment,
  snapshot.missingRequiredFields,
- snapshot.relayResolverErrors,
  snapshot.errorResponseFields,
  queryNode.fragment.metadata?.throwOnFieldError ?? false,
  );