Skip to content

Commit 7301f80

Browse files
tobias-tenglertobias-tengler
committed
Handle case when data is null
1 parent 823dd2e commit 7301f80

11 files changed

+113
-89
lines changed

‎src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Execution/Clients/ErrorTrie.cs

Lines changed: 8 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -4,32 +4,25 @@ public sealed class ErrorTrie : Dictionary<object, ErrorTrie>
44
{
55
public IError? Error { get; set; }
66

7-
public (object[] Path, IError Error)? FindPathToFirstError()
7+
public IError? FindFirstError()
88
{
99
if (Error is not null)
1010
{
11-
return ([], Error);
11+
return Error;
1212
}
1313

14-
var stack = new Stack<(ErrorTrie Node, List<object> Path)>();
14+
var stack = new Stack<ErrorTrie>(Values);
1515

16-
foreach (var kvp in this)
16+
while (stack.TryPop(out var errorTrie))
1717
{
18-
stack.Push((kvp.Value, [kvp.Key]));
19-
}
20-
21-
while (stack.Count > 0)
22-
{
23-
var (node, path) = stack.Pop();
24-
25-
if (node.Error is not null)
18+
if (errorTrie.Error is not null)
2619
{
27-
return ([..path], node.Error);
20+
return errorTrie.Error;
2821
}
2922

30-
foreach (var kvp in node)
23+
foreach (var child in errorTrie.Values)
3124
{
32-
stack.Push((kvp.Value, [..path, kvp.Key]));
25+
stack.Push(child);
3326
}
3427
}
3528

‎src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Execution/Nodes/OperationExecutionNode.cs

Lines changed: 31 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ public sealed class OperationExecutionNode : ExecutionNode
1616
{
1717
private readonly OperationRequirement[] _requirements;
1818
private readonly string[] _variables;
19+
private readonly ImmutableArray<string> _responseNames;
1920
private ExecutionNode[] _dependencies = [];
2021
private ExecutionNode[] _dependents = [];
2122
private int _dependencyCount;
@@ -57,14 +58,14 @@ public OperationExecutionNode(
5758
_variables = variables.ToArray();
5859

5960
// TODO: This might include requirements that we wouldn't want to create a front-end facing error for
61+
// TODO: How to deal with selections on a specific type? Creating errors for all possible response names is wrong
6062
_responseNames = GetResponseNamesFromPath(operation, source);
6163
}
6264

63-
// TODO: Move
64-
private readonly ImmutableArray<string> _responseNames;
65-
6665
public override int Id { get; }
6766

67+
private ReadOnlySpan<string> ResponseNames => _responseNames.AsSpan();
68+
6869
/// <summary>
6970
/// Gets the unique identifier of the operation.
7071
/// </summary>
@@ -153,7 +154,7 @@ private async Task<ExecutionNodeResult> ExecuteInternalAsync(
153154
}
154155
catch (Exception exception)
155156
{
156-
AddErrors(context, exception, variables, _responseNames);
157+
AddErrors(context, exception, variables, ResponseNames);
157158

158159
return new ExecutionNodeResult(
159160
Id,
@@ -174,7 +175,7 @@ private async Task<ExecutionNodeResult> ExecuteInternalAsync(
174175
buffer[index++] = result;
175176
}
176177

177-
context.AddPartialResults(Source, buffer.AsSpan(0, index));
178+
context.AddPartialResults(Source, buffer.AsSpan(0, index), ResponseNames);
178179
}
179180
catch (Exception exception)
180181
{
@@ -186,7 +187,7 @@ private async Task<ExecutionNodeResult> ExecuteInternalAsync(
186187
result?.Dispose();
187188
}
188189

189-
AddErrors(context, exception, variables, _responseNames);
190+
AddErrors(context, exception, variables, ResponseNames);
190191

191192
return new ExecutionNodeResult(
192193
Id,
@@ -208,6 +209,7 @@ private async Task<ExecutionNodeResult> ExecuteInternalAsync(
208209
Stopwatch.GetElapsedTime(start));
209210
}
210211

212+
// TODO: Error handling
211213
internal async Task<SubscriptionResult> SubscribeAsync(
212214
OperationPlanContext context,
213215
CancellationToken cancellationToken = default)
@@ -385,34 +387,34 @@ private static void AddErrors(
385387
OperationPlanContext context,
386388
Exception exception,
387389
ImmutableArray<VariableValues> variables,
388-
ImmutableArray<string> responseNames)
390+
ReadOnlySpan<string> responseNames)
389391
{
390-
var error = ErrorBuilder.FromException(exception).Build();
391-
392-
if (variables.Length == 0)
393-
{
394-
context.AddErrors(error, responseNames.AsSpan(), Path.Root);
395-
}
396-
else
397-
{
398-
var bufferLength = Math.Max(variables.Length, 1);
399-
var buffer = ArrayPool<Path>.Shared.Rent(bufferLength);
392+
var error = ErrorBuilder.FromException(exception).Build();
400393

401-
try
402-
{
403-
for (var i = 0; i < variables.Length; i++)
404-
{
405-
buffer[i] = variables[i].Path;
406-
}
394+
if (variables.Length == 0)
395+
{
396+
context.AddErrors(error, responseNames, Path.Root);
397+
}
398+
else
399+
{
400+
var pathBufferLength = variables.Length;
401+
var pathBuffer = ArrayPool<Path>.Shared.Rent(pathBufferLength);
407402

408-
context.AddErrors(error, responseNames.AsSpan(), buffer.AsSpan(0, bufferLength));
409-
}
410-
finally
403+
try
404+
{
405+
for (var i = 0; i < variables.Length; i++)
411406
{
412-
buffer.AsSpan(0, bufferLength).Clear();
413-
ArrayPool<Path>.Shared.Return(buffer);
407+
pathBuffer[i] = variables[i].Path;
414408
}
409+
410+
context.AddErrors(error, responseNames, pathBuffer.AsSpan(0, pathBufferLength));
411+
}
412+
finally
413+
{
414+
pathBuffer.AsSpan(0, pathBufferLength).Clear();
415+
ArrayPool<Path>.Shared.Return(pathBuffer);
415416
}
417+
}
416418
}
417419

418420
private sealed class SubscriptionEnumerable : IAsyncEnumerable<EventMessageResult>
@@ -500,7 +502,7 @@ public async ValueTask<bool> MoveNextAsync()
500502
if (hasResult)
501503
{
502504
_resultBuffer[0] = _resultEnumerator.Current;
503-
_context.AddPartialResults(_node.Source, _resultBuffer);
505+
_context.AddPartialResults(_node.Source, _resultBuffer, _node.ResponseNames);
504506
}
505507
}
506508
catch (Exception ex)

‎src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Execution/OperationPlanContext.cs

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,8 +99,11 @@ internal ImmutableArray<VariableValues> CreateVariableValueSets(
9999
}
100100
}
101101

102-
internal void AddPartialResults(SelectionPath sourcePath, ReadOnlySpan<SourceSchemaResult> results)
103-
=> _resultStore.AddPartialResults(sourcePath, results);
102+
internal void AddPartialResults(
103+
SelectionPath sourcePath,
104+
ReadOnlySpan<SourceSchemaResult> results,
105+
ReadOnlySpan<string> responseNames)
106+
=> _resultStore.AddPartialResults(sourcePath, results, responseNames);
104107

105108
internal void AddPartialResults(ObjectResult result, ReadOnlySpan<Selection> selections)
106109
=> _resultStore.AddPartialResults(result, selections);

‎src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Execution/Results/FetchResultStore.cs

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,8 @@ public void Reset(ResultPoolSession resultPoolSession)
6868

6969
public bool AddPartialResults(
7070
SelectionPath sourcePath,
71-
ReadOnlySpan<SourceSchemaResult> results)
71+
ReadOnlySpan<SourceSchemaResult> results,
72+
ReadOnlySpan<string> responseNames)
7273
{
7374
ObjectDisposedException.ThrowIf(_disposed, this);
7475
ArgumentNullException.ThrowIfNull(sourcePath);
@@ -110,7 +111,7 @@ public bool AddPartialResults(
110111
errorTrie = ref Unsafe.Add(ref errorTrie, 1)!;
111112
}
112113

113-
return SaveSafe(results, dataElementsSpan, errorTriesSpan);
114+
return SaveSafe(results, dataElementsSpan, errorTriesSpan, responseNames);
114115
}
115116
finally
116117
{
@@ -167,14 +168,14 @@ public void AddErrors(IError error, ReadOnlySpan<string> responseNames, params R
167168
return;
168169
}
169170

170-
var result = path.IsRoot ? _root : GetStartObjectResult(path);
171+
var objectResult = path.IsRoot ? _root : GetStartObjectResult(path);
171172

172-
if (result.IsInvalidated)
173+
if (objectResult.IsInvalidated)
173174
{
174175
continue;
175176
}
176177

177-
_valueCompletion.BuildResult(result, responseNames, error, path);
178+
_valueCompletion.BuildResult(objectResult, responseNames, error, path);
178179

179180
path = ref Unsafe.Add(ref path, 1)!;
180181
}
@@ -188,7 +189,8 @@ public void AddErrors(IError error, ReadOnlySpan<string> responseNames, params R
188189
private bool SaveSafe(
189190
ReadOnlySpan<SourceSchemaResult> results,
190191
ReadOnlySpan<JsonElement> dataElements,
191-
ReadOnlySpan<ErrorTrie?> errorTries)
192+
ReadOnlySpan<ErrorTrie?> errorTries,
193+
ReadOnlySpan<string> responseNames)
192194
{
193195
_lock.EnterWriteLock();
194196

@@ -214,10 +216,7 @@ private bool SaveSafe(
214216
continue;
215217
}
216218

217-
if (!_valueCompletion.BuildResult(selectionSet, data, errorTrie, objectResult))
218-
{
219-
return false;
220-
}
219+
_valueCompletion.BuildResult(selectionSet, data, errorTrie, responseNames, objectResult);
221220

222221
result = ref Unsafe.Add(ref result, 1)!;
223222
data = ref Unsafe.Add(ref data, 1);

‎src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Execution/Results/ValueCompletion.cs

Lines changed: 7 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -36,37 +36,23 @@ public ValueCompletion(
3636
_errors = errors;
3737
}
3838

39-
public bool BuildResult(
39+
public void BuildResult(
4040
SelectionSet selectionSet,
4141
JsonElement data,
4242
ErrorTrie? errorTrie,
43+
ReadOnlySpan<string> responseNames,
4344
ObjectResult objectResult)
4445
{
4546
if (data is not { ValueKind: JsonValueKind.Object })
4647
{
47-
// If we encounter a null, we check if there's an error on this field
48-
// or somewhere below. If there is, we add the error, since it likely
49-
// propagated and erased the current field result.
50-
if (errorTrie?.FindPathToFirstError() is { } pathToFirstError)
51-
{
52-
var path = Path.FromList(pathToFirstError.Path);
53-
54-
var errorWithPath = ErrorBuilder.FromError(pathToFirstError.Error)
55-
.SetPath(objectResult.Path.Append(path))
56-
57-
// We should add the location here, but not sure if it's worth it to iterate the
58-
// selections for it.
48+
var error = errorTrie?.FindFirstError() ??
49+
ErrorBuilder.New()
50+
.SetMessage("Unexpected execution error") // TODO: Better message
5951
.Build();
6052

61-
_errors.Add(errorWithPath);
53+
BuildResult(objectResult, responseNames, error, objectResult.Path);
6254

63-
if (_errorHandling is ErrorHandling.Propagate)
64-
{
65-
PropagateNullValues(objectResult);
66-
}
67-
}
68-
69-
return false;
55+
return;
7056
}
7157

7258
foreach (var selection in selectionSet.Selections)
@@ -92,8 +78,6 @@ public bool BuildResult(
9278
}
9379
}
9480
}
95-
96-
return true;
9781
}
9882

9983
public void BuildResult(

‎src/HotChocolate/Fusion-vnext/test/Fusion.AspNetCore.Tests/__snapshots__/SubgraphErrorTests.Error_On_Lookup_Field.snap

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,8 @@
33
{
44
"message": "Could not resolve Product",
55
"path": [
6-
"topProduct"
6+
"topProduct",
7+
"name"
78
]
89
}
910
],

‎src/HotChocolate/Fusion-vnext/test/Fusion.AspNetCore.Tests/__snapshots__/SubgraphErrorTests.Error_On_Lookup_Field_In_List.snap

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,43 @@
11
{
2+
"data": {
3+
"topProducts": [
4+
{
5+
"price": 13.99,
6+
"name": null
7+
},
8+
{
9+
"price": 13.99,
10+
"name": null
11+
},
12+
{
13+
"price": 13.99,
14+
"name": null
15+
}
16+
]
17+
},
218
"errors": [
319
{
420
"message": "Could not resolve Product",
521
"path": [
622
"topProducts",
7-
0
23+
0,
24+
"name"
25+
]
26+
},
27+
{
28+
"message": "Could not resolve Product",
29+
"path": [
30+
"topProducts",
31+
1,
32+
"name"
33+
]
34+
},
35+
{
36+
"message": "Could not resolve Product",
37+
"path": [
38+
"topProducts",
39+
2,
40+
"name"
841
]
942
}
1043
],

‎src/HotChocolate/Fusion-vnext/test/Fusion.AspNetCore.Tests/__snapshots__/SubgraphErrorTests.Error_On_Lookup_Field_In_List_Leaf_NonNull.snap

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,8 @@
44
"message": "Could not resolve Product",
55
"path": [
66
"topProducts",
7-
0
7+
0,
8+
"name"
89
]
910
}
1011
],

‎src/HotChocolate/Fusion-vnext/test/Fusion.AspNetCore.Tests/__snapshots__/SubgraphErrorTests.No_Data_And_Error_With_Path_For_Lookup.snap

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,8 @@
33
{
44
"message": "Could not resolve Product",
55
"path": [
6-
"topProduct"
6+
"topProduct",
7+
"name"
78
]
89
}
910
],

‎src/HotChocolate/Fusion-vnext/test/Fusion.AspNetCore.Tests/__snapshots__/SubgraphErrorTests.No_Data_And_Error_Without_Path_For_Lookup.snap

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,14 @@
11
{
2-
"data": {
3-
"topProduct": {
4-
"price": 13.99,
5-
"name": null
6-
}
7-
},
82
"errors": [
93
{
104
"message": "A global error"
5+
},
6+
{
7+
"message": "Unexpected execution error",
8+
"path": [
9+
"topProduct",
10+
"name"
11+
]
1112
}
1213
],
1314
"extensions": {

0 commit comments

Comments
 (0)