8000 Add fieldPath to missing data field error logs (#4835) · facebook/relay@3eb627d · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Commit 3eb627d

Browse files
captbaritonefacebook-github-bot
captbaritone
authored and
facebook-github-bot
committed
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
1 parent 3064e18 commit 3eb627d

18 files changed

+1471
-34
lines changed

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -975,7 +975,7 @@ describe.each([true, false])(
975975
expect(relayFieldLogger.mock.calls).toEqual([
976976
[
977977
{
978-
fieldPath: '',
978+
fieldPath: 'username',
979979
kind: 'missing_expected_data.log',
980980
owner: 'ResolverThatThrows',
981981
},

‎packages/relay-runtime/store/RelayReader.js

+87-12
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,7 @@ class RelayReader {
217217
}
218218
}
219219

220-
_markDataAsMissing(): void {
220+
_markDataAsMissing(fieldName: string): void {
221221
if (this._isWithinUnmatchedTypeRefinement) {
222222
return;
223223
}
@@ -226,18 +226,17 @@ class RelayReader {
226226
}
227227

228228
// we will add the path later
229-
const fieldPath = '';
230229
const owner = this._fragmentName;
231230

232231
this._errorResponseFields.push(
233232
this._selector.node.metadata?.throwOnFieldError ?? false
234233
? {
235234
kind: 'missing_expected_data.throw',
236235
owner,
237-
fieldPath,
236+
fieldPath: fieldName,
238237
handled: false,
239238
}
240-
: {kind: 'missing_expected_data.log', owner, fieldPath},
239+
: {kind: 'missing_expected_data.log', owner, fieldPath: fieldName},
2 F438 41240
);
242241

243242
this._isMissingData = true;
@@ -266,7 +265,7 @@ class RelayReader {
266265
this._seenRecords.add(dataID);
267266
if (record == null) {
268267
if (record === undefined) {
269-
this._markDataAsMissing();
268+
this._markDataAsMissing('<record>');
270269
}
271270
return record;
272271
}
@@ -489,12 +488,15 @@ class RelayReader {
489488
this._createFragmentPointer(selection, record, data);
490489
break;
491490
case 'AliasedInlineFragmentSpread': {
491+
const prevErrors = this._errorResponseFields;
492+
this._errorResponseFields = null;
492493
let fieldValue = this._readInlineFragment(
493494
selection.fragment,
494495
record,
495496
{},
496497
true,
497498
);
499+
this._prependPreviousErrors(prevErrors, selection.name);
498500
if (fieldValue === false) {
499501
fieldValue = null;
500502
}
@@ -601,9 +603,12 @@ class RelayReader {
601603
data: SelectorData,
602604
): mixed {
603605
const parentRecordID = RelayModernRecord.getDataID(record);
606+
const prevErrors = this._errorResponseFields;
607+
this._errorResponseFields = null;
604608
const result = this._readResolverFieldImpl(field, parentRecordID);
605609

606610
const fieldName = field.alias ?? field.name;
611+
this._prependPreviousErrors(prevErrors, fieldName);
607612
data[fieldName] = result;
608613
return result;
609614
}
@@ -982,12 +987,15 @@ class RelayReader {
982987
RelayModernRecord.getDataID(record),
983988
prevData,
984989
);
990+
const prevErrors = this._errorResponseFields;
991+
this._errorResponseFields = null;
985992
const edgeValue = this._traverse(
986993
field.linkedField,
987994
storeID,
988995
// $FlowFixMe[incompatible-variance]
989996
prevData,
990997
);
998+
this._prependPreviousErrors(prevErrors, fieldName);
991999
this._clientEdgeTraversalPath.pop();
9921000
data[fieldName] = edgeValue;
9931001
return edgeValue;
@@ -1005,7 +1013,7 @@ class RelayReader {
10051013
if (value === null) {
10061014
this._maybeAddErrorResponseFields(record, storageKey);
10071015
} else if (value === undefined) {
1008-
this._markDataAsMissing();
1016+
this._markDataAsMissing(fieldName);
10091017
}
10101018
data[fieldName] = value;
10111019
return value;
@@ -1024,7 +1032,7 @@ class RelayReader {
10241032
if (linkedID === null) {
10251033
this._maybeAddErrorResponseFields(record, storageKey);
10261034
} else if (linkedID === undefined) {
1027-
this._markDataAsMissing();
1035+
this._markDataAsMissing(fieldName);
10281036
}
10291037
return linkedID;
10301038
}
@@ -1039,12 +1047,73 @@ class RelayReader {
10391047
RelayModernRecord.getDataID(record),
10401048
prevData,
10411049
);
1050+
const prevErrors = this._errorResponseFields;
1051+
this._errorResponseFields = null;
10421052
// $FlowFixMe[incompatible-variance]
10431053
const value = this._traverse(field, linkedID, prevData);
1054+
1055+
this._prependPreviousErrors(prevErrors, fieldName);
10441056
data[fieldName] = value;
10451057
return value;
10461058
}
10471059

1060+
/**
1061+
* Adds a set of field errors to `this._errorResponseFields`, ensuring the
1062+
* `fieldPath` property of existing field errors are prefixed with the given
1063+
* `fieldNameOrIndex`.
1064+
*
1065+
* In order to make field errors maximally useful in logs/errors, we want to
1066+
* include the path to the field that caused the error. A naive approach would
1067+
* be to maintain a path property on RelayReader which we push/pop field names
1068+
* to as we traverse into fields/etc. However, this would be expensive to
1069+
* maintain, and in the common case where there are no field errors, the work
1070+
* would go unused.
1071+
*
1072+
* Instead, we take a lazy approach where as we exit the recurison into a
1073+
* field/etc we prepend any errors encountered while traversing that field
1074+
* with the field name. This is somewhat more expensive in the error case, but
1075+
* ~free in the common case where there are no errors.
1076+
*
1077+
* To achieve this, named field readers must do the following to correctly
1078+
* track error filePaths:
1079+
*
1080+
* 1. Stash the value of `this._errorResponseFields` in a local variable
1081+
* 2. Set `this._errorResponseFields` to `null`
1082+
* 3. Traverse into the field
1083+
* 4. Call this method with the stashed errors and the field's name
1084+
*
1085+
* Similarly, when creating field errors, we simply initialize the `fieldPath`
1086+
* as the direct field name.
1087+
*
1088+
* Today we only use this apporach for `missing_expected_data` errors, but we
1089+
* intend to broaden it to handle all field error paths.
1090+
*/
1091+
_prependPreviousErrors(
1092+
prevErrors: ?Array<ErrorResponseField>,
1093+
fieldNameOrIndex: string | number,
1094+
): void {
1095+
if (this._errorResponseFields != null) {
1096+
for (let i = 0; i < this._errorResponseFields.length; i++) {
1097+
const event = this._errorResponseFields[i];
1098+
if (
1099+
event.owner === this._fragmentName &&
1100+
(event.kind === 'missing_expected_data.throw' ||
1101+
event.kind === 'missing_expected_data.log')
1102+
) {
1103+
event.fieldPath = `${fieldNameOrIndex}.${event.fieldPath}`;
1104+
}
1105+
}
1106+
if (prevErrors != null) {
1107+
for (let i = this._errorResponseFields.length - 1; i >= 0; i--) {
1108+
prevErrors.push(this._errorResponseFields[i]);
1109+
}
1110+
this._errorResponseFields = prevErrors;
1111+
}
1112+
} else {
1113+
this._errorResponseFields = prevErrors;
1114+
}
1115+
}
1116+
10481117
_readActorChange(
10491118
field: ReaderActorChange,
10501119
record: Record,
@@ -1060,7 +1129,7 @@ class RelayReader {
10601129
if (externalRef == null) {
10611130
data[fieldName] = externalRef;
10621131
if (externalRef === undefined) {
1063-
this._markDataAsMissing();
1132+
this._markDataAsMissing(fieldName);
10641133
} else if (externalRef === null) {
10651134
this._maybeAddErrorResponseFields(record, storageKey);
10661135
}
@@ -1107,7 +1176,7 @@ class RelayReader {
11071176
if (linkedIDs == null) {
11081177
data[fieldName] = linkedIDs;
11091178
if (linkedIDs === undefined) {
1110-
this._markDataAsMissing();
1179+
this._markDataAsMissing(fieldName);
11111180
}
11121181
return linkedIDs;
11131182
}
@@ -1121,11 +1190,13 @@ class RelayReader {
11211190
RelayModernRecord.getDataID(record),
11221191
prevData,
11231192
);
1193+
const prevErrors = this._errorResponseFields;
1194+
this._errorResponseFields = null;
11241195
const linkedArray = prevData || [];
11251196
linkedIDs.forEach((linkedID, nextIndex) => {
11261197
if (linkedID == null) {
11271198
if (linkedID === undefined) {
1128-
this._markDataAsMissing();
1199+
this._markDataAsMissing(String(nextIndex));
11291200
}
11301201
// $FlowFixMe[cannot-write]
11311202
linkedArray[nextIndex] = linkedID;
@@ -1140,10 +1211,14 @@ class RelayReader {
11401211
RelayModernRecord.getDataID(record),
11411212
prevItem,
11421213
);
1214+
const prevErrors = this._errorResponseFields;
1215+
this._errorResponseFields = null;
11431216
// $FlowFixMe[cannot-write]
11441217
// $FlowFixMe[incompatible-variance]
11451218
linkedArray[nextIndex] = this._traverse(field, linkedID, prevItem);
1219+
this._prependPreviousErrors(prevErrors, nextIndex);
11461220
});
1221+
this._prependPreviousErrors(prevErrors, fieldName);
11471222
data[fieldName] = linkedArray;
11481223
return linkedArray;
11491224
}
@@ -1166,7 +1241,7 @@ class RelayReader {
11661241
RelayModernRecord.getValue(record, componentKey);
11671242
if (component == null) {
11681243
if (component === undefined) {
1169-
this._markDataAsMissing();
1244+
this._markDataAsMissing('<module-import>');
11701245
}
11711246
return;
11721247
}
@@ -1398,7 +1473,7 @@ class RelayReader {
13981473
// fetched the `__is[AbstractType]` flag for this concrete type. In this
13991474
// case we need to report that we are missing data, in case that field is
14001475
// still in flight.
1401-
this._markDataAsMissing();
1476+
this._markDataAsMissing('<abstract-type-hint>');
14021477
}
14031478
// $FlowFixMe Casting record value
14041479
return implementsInterface;

‎packages/relay-runtime/store/RelayStoreTypes.js

+2-2
Original file line numberDiff line numberDiff line change
@@ -1274,7 +1274,7 @@ export type MissingFieldHandler =
12741274
export type MissingExpectedDataLogEvent = {
12751275
+kind: 'missing_expected_data.log',
12761276
+owner: string,
1277-
+fieldPath: string,
1277+
fieldPath: string, // Purposefully mutable to allow lazy construction in RelayReader
12781278
};
12791279

12801280
/**
@@ -1300,7 +1300,7 @@ export type MissingExpectedDataLogEvent = {
13001300
export type MissingExpectedDataThrowEvent = {
13011301
+kind: 'missing_expected_data.throw',
13021302
+owner: string,
1303-
+fieldPath: string,
1303+
fieldPath: string, // Purposefully mutable to allow lazy construction in RelayReader
13041304
+handled: boolean,
13051305
};
13061306

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

+4-4
Original file line numberDiff line numberDiff line change
@@ -417,12 +417,12 @@ function cloneEventWithSets(event: LogEvent) {
417417
isMissingData: true,
418418
errorResponseFields: [
419419
{
420-
fieldPath: '',
420+
fieldPath: 'profilePicture',
421421
kind: 'missing_expected_data.log',
422422
owner: 'RelayModernStoreSubscriptionsTest1Fragment',
423423
},
424424
{
425-
fieldPath: '',
425+
fieldPath: 'emailAddresses',
426426
kind: 'missing_expected_data.log',
427427
owner: 'RelayModernStoreSubscriptionsTest1Fragment',
428428
},
@@ -466,12 +466,12 @@ function cloneEventWithSets(event: LogEvent) {
466466
},
467467
errorResponseFields: [
468468
{
469-
fieldPath: '',
469+
fieldPath: 'profilePicture',
470470
kind: 'missing_expected_data.log',
471471
owner: 'RelayModernStoreSubscriptionsTest1Fragment',
472472
},
473473
{
474-
fieldPath: '',
474+
fieldPath: 'emailAddresses',
475475
kind: 'missing_expected_data.log',
476476
owner: 'RelayModernStoreSubscriptionsTest1Fragment',
477477
},

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

+4-4
Original file line numberDiff line numberDiff line change
@@ -728,12 +728,12 @@ function cloneEventWithSets(event: LogEvent) {
728728
{
729729
owner: 'RelayModernStoreTest5Fragment',
730730
kind: 'missing_expected_data.log',
731-
fieldPath: '',
731+
fieldPath: 'profilePicture',
732732
},
733733
{
734734
owner: 'RelayModernStoreTest5Fragment',
735735
kind: 'missing_expected_data.log',
736-
fieldPath: '',
736+
fieldPath: 'emailAddresses',
737737
},
738738
],
739739
missingLiveResolverFields: [],
@@ -782,12 +782,12 @@ function cloneEventWithSets(event: LogEvent) {
782782
{
783783
owner: 'RelayModernStoreTest5Fragment',
784784
kind: 'missing_expected_data.log',
785-
fieldPath: '',
785+
fieldPath: 'profilePicture',
786786
},
787787
{
788788
owner: 'RelayModernStoreTest5Fragment',
789789
kind: 'missing_expected_data.log',
790-
fieldPath: '',
790+
fieldPath: 'emailAddresses',
791791
},
792792
],
793793
seenRecords: new Set(['842472']),

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

+2-2
Original file line numberDiff line numberDiff line change
@@ -1041,7 +1041,7 @@ describe('Inline Fragments', () => {
10411041
);
10421042
expect(errorResponseFields).toEqual([
10431043
{
1044-
fieldPath: '',
1044+
fieldPath: 'node.aliased_fragment.name',
10451045
kind: 'missing_expected_data.log',
10461046
owner:
10471047
'RelayReaderAliasedFragmentsTestRequiredBubblesOnAbstractTypeQuery',
@@ -1101,7 +1101,7 @@ describe('Inline Fragments', () => {
11011101
);
11021102
expect(errorResponseFields).toEqual([
11031103
{
1104-
fieldPath: '',
1104+
fieldPath: 'node.aliased_fragment.<abstract-type-hint>',
11051105
kind: 'missing_expected_data.log',
11061106
owner:
11071107
'RelayReaderAliasedFragmentsTestRequiredBubblesOnAbstractWithMissingTypeInfoQuery',

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -297,7 +297,7 @@ describe('RelayReader @catch', () => {
297297

298298
expect(errorResponseFields).toEqual([
299299
{
300-
fieldPath: '',
300+
fieldPath: 'me.firstName',
301301
kind: 'missing_expected_data.log',
302302
owner: 'RelayReaderCatchFieldsTestCatchMissingToNullErrorQuery',
303303
},

0 commit comments

Comments
 (0)
0