From 356327c97536c43582378dee5945ccb309168f8a Mon Sep 17 00:00:00 2001 From: Evan Yeung Date: Tue, 22 Oct 2024 10:10:02 -0700 Subject: [PATCH] Fix bug in Connections with errors Summary: This diff fixes a bug in the connection handler where errors that occur on a field are not copied into the "connection" record for that field. Connections create virtual fields on a record that contain the results for the set of edges fetched with a set of args. These are merged into the record by the connection handler. However, the errors are only ever set on the base field in the normalizer. By copying the fields over to the connection field, the `throwOnFieldError` directive now works correctly on connections. This diff also adds some public APIs to the RecordProxy and mutator to deal with errors, which have not been handled in Relay previously. Reviewed By: captbaritone Differential Revision: D64505652 fbshipit-source-id: 288b33bf047d5baf7dacd0961ba6b5692766be0e --- .../handlers/connection/ConnectionHandler.js | 7 +- .../__tests__/ConnectionHandler-test.js | 107 +++++++++++++++++- .../mutations/RelayRecordProxy.js | 33 +++++- .../mutations/RelayRecordSourceMutator.js | 27 +++++ .../relay-runtime/store/RelayModernRecord.js | 2 +- .../relay-runtime/store/RelayStoreTypes.js | 8 +- 6 files changed, 177 insertions(+), 7 deletions(-) diff --git a/packages/relay-runtime/handlers/connection/ConnectionHandler.js b/packages/relay-runtime/handlers/connection/ConnectionHandler.js index e47d6ac9b0d0e..b5c5128272de7 100644 --- a/packages/relay-runtime/handlers/connection/ConnectionHandler.js +++ b/packages/relay-runtime/handlers/connection/ConnectionHandler.js @@ -67,7 +67,12 @@ function update(store: RecordSourceProxy, payload: HandleFieldPayload): void { const serverPageInfo = serverConnection && serverConnection.getLinkedRecord(PAGE_INFO); if (!serverConnection) { - record.setValue(null, payload.handleKey); + record.setValue( + null, + payload.handleKey, + undefined, + record.getErrors(payload.fieldKey), + ); return; } // In rare cases the handleKey field may be unset even though the client diff --git a/packages/relay-runtime/handlers/connection/__tests__/ConnectionHandler-test.js b/packages/relay-runtime/handlers/connection/__tests__/ConnectionHandler-test.js index 2ea05b1aeb2db..bb7399b48b27e 100644 --- a/packages/relay-runtime/handlers/connection/__tests__/ConnectionHandler-test.js +++ b/packages/relay-runtime/handlers/connection/__tests__/ConnectionHandler-test.js @@ -45,7 +45,7 @@ describe('ConnectionHandler', () => { let proxy; let sinkSource; - function normalize(payload, variables, options) { + function normalize(payload, variables, options, errors) { RelayResponseNormalizer.normalize( baseSource, createNormalizationSelector( @@ -57,6 +57,7 @@ describe('ConnectionHandler', () => { options ?? { getDataID: defaultGetDataID, }, + errors, ); } @@ -132,6 +133,110 @@ describe('ConnectionHandler', () => { }); }); + describe('field errors', () => { + it('propagates errors to virtual connection field from server connection field', () => { + normalize( + { + node: { + id: '4', + __typename: 'User', + friends: null, + }, + }, + { + after: null, + before: null, + count: 10, + orderby: ['first name'], + id: '4', + }, + undefined, + [{message: 'Oops!', path: ['node', 'friends']}], + ); + const args = {first: 10, orderby: ['first name']}; + const handleKey = + getRelayHandleKey('connection', 'ConnectionQuery_friends', 'friends') + + '(orderby:["first name"])'; + const payload = { + args, + dataID: '4', + fieldKey: getStableStorageKey('friends', args), + handleKey, + }; + ConnectionHandler.update(proxy, payload); + expect(sinkSource.toJSON()['4'].__errors).toEqual({ + '__ConnectionQuery_friends_connection(orderby:["first name"])': [ + {message: 'Oops!'}, + ], + }); + }); + + it('leaves errors when a valid value is added', () => { + normalize( + { + node: { + id: '4', + __typename: 'User', + friends: null, + }, + }, + { + after: null, + before: null, + count: 10, + orderby: ['first name'], + id: '4', + }, + undefined, + [{message: 'Oops!', path: ['node', 'friends']}], + ); + + const args = {first: 10, orderby: ['first name']}; + const handleKey = + getRelayHandleKey('connection', 'ConnectionQuery_friends', 'friends') + + '(orderby:["first name"])'; + const payload = { + args, + dataID: '4', + fieldKey: getStableStorageKey('friends', args), + handleKey, + }; + ConnectionHandler.update(proxy, payload); + // Re-check that an error is set (same as basic error test) + expect(sinkSource.toJSON()['4'].__errors).toEqual({ + '__ConnectionQuery_friends_connection(orderby:["first name"])': [ + {message: 'Oops!'}, + ], + }); + + // Check that the error is not cleared even if new data arrives + normalize( + { + node: { + id: '4', + __typename: 'User', + friends: [], + }, + }, + { + after: null, + before: null, + count: 10, + orderby: ['first name'], + id: '4', + }, + undefined, + undefined, + ); + ConnectionHandler.update(proxy, payload); + expect(sinkSource.toJSON()['4'].__errors).toEqual({ + '__ConnectionQuery_friends_connection(orderby:["first name"])': [ + {message: 'Oops!'}, + ], + }); + }); + }); + describe('insertEdgeAfter()', () => { let connection; let connectionID; diff --git a/packages/relay-runtime/mutations/RelayRecordProxy.js b/packages/relay-runtime/mutations/RelayRecordProxy.js index 10afb8f873c22..ed475d830044d 100644 --- a/packages/relay-runtime/mutations/RelayRecordProxy.js +++ b/packages/relay-runtime/mutations/RelayRecordProxy.js @@ -11,6 +11,7 @@ 'use strict'; +import type {TRelayFieldError} from '../store/RelayErrorTrie'; import type {RecordProxy} from '../store/RelayStoreTypes'; import type {Arguments} from '../store/RelayStoreUtils'; import type {DataID} from '../util/RelayRuntimeTypes'; @@ -65,22 +66,48 @@ class RelayRecordProxy implements RecordProxy { return this._mutator.getValue(this._dataID, storageKey); } - setValue(value: mixed, name: string, args?: ?Arguments): RecordProxy { + setValue( + value: mixed, + name: string, + args?: ?Arguments, + errors?: ?$ReadOnlyArray, + ): RecordProxy { invariant( isValidLeafValue(value), 'RelayRecordProxy#setValue(): Expected a scalar or array of scalars, ' + 'got `%s`.', JSON.stringify(value), ); - return this.setValue__UNSAFE(value, name, args); + + return this.setValue__UNSAFE(value, name, args, errors); + } + + getErrors( + name: string, + args?: ?Arguments, + ): ?$ReadOnlyArray { + const storageKey = getStableStorageKey(name, args); + return this._mutator.getErrors(this._dataID, storageKey); } // This is used in the typesafe updaters. // We already validated that the value has the correct type // so it should be safe to store complex structures as scalar values (custom scalars) - setValue__UNSAFE(value: mixed, name: string, args?: ?Arguments): RecordProxy { + setValue__UNSAFE( + value: mixed, + name: string, + args?: ?Arguments, + errors?: ?$ReadOnlyArray, + ): RecordProxy { const storageKey = getStableStorageKey(name, args); this._mutator.setValue(this._dataID, storageKey, value); + if (errors != null) { + if (errors.length === 0) { + this._mutator.setErrors(this._dataID, storageKey); + } else { + this._mutator.setErrors(this._dataID, storageKey, errors); + } + } return this; } diff --git a/packages/relay-runtime/mutations/RelayRecordSourceMutator.js b/packages/relay-runtime/mutations/RelayRecordSourceMutator.js index e0933af0f203e..42d2e97e88299 100644 --- a/packages/relay-runtime/mutations/RelayRecordSourceMutator.js +++ b/packages/relay-runtime/mutations/RelayRecordSourceMutator.js @@ -11,6 +11,7 @@ 'use strict'; +import type {TRelayFieldError} from '../store/RelayErrorTrie'; import type {RecordState} from '../store/RelayRecordState'; import type { MutableRecordSource, @@ -179,6 +180,32 @@ class RelayRecordSourceMutator { RelayModernRecord.setValue(sinkRecord, storageKey, value); } + getErrors( + dataID: DataID, + storageKey: string, + ): ?$ReadOnlyArray { + for (let ii = 0; ii < this.__sources.length; ii++) { + const record = this.__sources[ii].get(dataID); + if (record) { + const value = RelayModernRecord.getErrors(record, storageKey); + if (value !== undefined) { + return value; + } + } else if (record === null) { + return null; + } + } + } + + setErrors( + dataID: DataID, + storageKey: string, + errors?: $ReadOnlyArray, + ): void { + const sinkRecord = this._getSinkRecord(dataID); + RelayModernRecord.setErrors(sinkRecord, storageKey, errors); + } + getLinkedRecordID(dataID: DataID, storageKey: string): ?DataID { for (let ii = 0; ii < this.__sources.length; ii++) { const record = this.__sources[ii].get(dataID); diff --git a/packages/relay-runtime/store/RelayModernRecord.js b/packages/relay-runtime/store/RelayModernRecord.js index 90e2817c4d71d..ebc5f406e729d 100644 --- a/packages/relay-runtime/store/RelayModernRecord.js +++ b/packages/relay-runtime/store/RelayModernRecord.js @@ -35,7 +35,7 @@ const areEqual = require('areEqual'); const invariant = require('invariant'); const warning = require('warning'); -type StorageKey = Exclude; +export type StorageKey = Exclude; type RelayFieldErrors = {[StorageKey]: $ReadOnlyArray}; diff --git a/packages/relay-runtime/store/RelayStoreTypes.js b/packages/relay-runtime/store/RelayStoreTypes.js index 1ddb1567e61bb..800d1e8c01a39 100644 --- a/packages/relay-runtime/store/RelayStoreTypes.js +++ b/packages/relay-runtime/store/RelayStoreTypes.js @@ -467,6 +467,7 @@ export interface RecordProxy { ): RecordProxy; getType(): string; getValue(name: string, args?: ?Variables): mixed; + getErrors(name: string, args?: ?Variables): ?$ReadOnlyArray; setLinkedRecord( record: RecordProxy, name: string, @@ -477,7 +478,12 @@ export interface RecordProxy { name: string, args?: ?Variables, ): RecordProxy; - setValue(value: mixed, name: string, args?: ?Variables): RecordProxy; + setValue( + value: mixed, + name: string, + args?: ?Variables, + errors?: ?$ReadOnlyArray, + ): RecordProxy; invalidateRecord(): void; }