Skip to content

Commit dababdf

Browse files
Shane32Shane32
authored
Fix argument parsing for merged selection sets (graphql-dotnet#4083)
* Fix 4083 * Only use concurrent dictionary for non-serial execution strategies * Add comments * Add comments
1 parent 0ba2031 commit dababdf

File tree

6 files changed

+172
-11
lines changed

6 files changed

+172
-11
lines changed

‎src/GraphQL.Tests/Bugs/Bug4083.cs

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
using GraphQL.Types;
2+
using Microsoft.Extensions.DependencyInjection;
3+
4+
namespace GraphQL.Tests.Bugs;
5+
6+
public class Bug4083
7+
{
8+
[Fact]
9+
public async Task FragmentsCombineCorrectlyWithVariables()
10+
{
11+
var services = new ServiceCollection();
12+
services.AddGraphQL(b => b
13+
.AddAutoSchema<Query>()
14+
);
15+
var provider = services.BuildServiceProvider();
16+
var schema = provider.GetRequiredService<ISchema>();
17+
schema.Initialize();
18+
var ret = await schema.ExecuteAsync(o =>
19+
{
20+
o.Query = """
21+
query heroQuery($id: Int!) {
22+
...fragment1
23+
...fragment2
24+
}
25+
26+
fragment fragment1 on Query {
27+
hero(id: $id) {
28+
id
29+
}
30+
}
31+
32+
fragment fragment2 on Query {
33+
hero(id: $id) {
34+
name
35+
}
36+
}
37+
""";
38+
o.Variables = """{"id":1}""".ToInputs();
39+
});
40+
ret.ShouldBeCrossPlatJson("""
41+
{
42+
"data": {
43+
"hero": {
44+
"id": 1,
45+
"name": "John Doe"
46+
}
47+
}
48+
}
49+
""");
50+
}
51+
52+
public class Query
53+
{
54+
public static Class1 Hero(int id) => new Class1 { Id = id, Name = "John Doe" };
55+
}
56+
57+
public class Class1
58+
{
59+
public int Id { get; set; }
60+
public string Name { get; set; }
61+
}
62+
}

‎src/GraphQL.Tests/Bugs/Issue4060.cs

Lines changed: 74 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
using System.Collections;
2+
using System.Collections.Concurrent;
23
using System.Collections.Immutable;
34
using GraphQL.Execution;
45
using GraphQL.SystemTextJson;
@@ -58,6 +59,60 @@ public async Task VariableModification_ShouldReflectInSecondField()
5859
""");
5960
}
6061

62+
[Fact]
63+
public async Task SampleCodeWorksWhenMergingSelectionSets()
64+
{
65+
var heroType = new ObjectGraphType() { Name = "Hero" };
66+
heroType.Field<IntGraphType>("id");
67+
heroType.Field<StringGraphType>("name");
68+
var query = new ObjectGraphType() { Name = "Query" };
69+
query.Field("hero", heroType)
70+
.Argument<IntGraphType>("id")
71+
.Resolve(context => new { id = context.GetArgument<int>("id"), name = "John Doe" });
72+
var schema = new Schema { Query = query };
73+
schema.Initialize();
74+
var options = new ExecutionOptions
75+
{
76+
Schema = schema,
77+
Query = """
78+
query heroQuery($id: Int!) {
79+
...fragment1
80+
...fragment2
81+
}
82+
fragment fragment1 on Query {
83+
hero(id: $id) {
84+
id
85+
}
86+
}
87+
fragment fragment2 on Query {
88+
hero(id: $id) {
89+
name
90+
}
91+
}
92+
""",
93+
Variables = new Inputs(new Dictionary<string, object?>
94+
{
95+
{ "id", 123 }
96+
}),
97+
ValidationRules = DocumentValidator.CoreRules.Append(new FieldMappingValidationRule()),
98+
};
99+
options.Listeners.Add(new DynamicVariableExecutionListener());
100+
101+
var result = await new DocumentExecuter().ExecuteAsync(options);
102+
var resultJson = new GraphQLSerializer().Serialize(result);
103+
104+
resultJson.ShouldBeCrossPlatJson("""
105+
{
106+
"data": {
107+
"hero": {
108+
"id": 123,
109+
"name": "John Doe"
110+
}
111+
}
112+
}
113+
""");
114+
}
115+
61116
public class FieldMappingValidationRule : ValidationRuleBase
62117
{
63118
public override ValueTask<INodeVisitor?> GetPostNodeVisitorAsync(ValidationContext context) => new(new MyVisitor());
@@ -97,13 +152,14 @@ public override Task BeforeExecutionAsync(IExecutionContext context)
97152
return Task.CompletedTask;
98153
}
99154

100-
private class FieldArgumentDictionary : IReadOnlyDictionary<GraphQLField, IDictionary<string, ArgumentValue>>
155+
private class FieldArgumentDictionary : IReadOnlyDictionary<GraphQLField, IDictionary<string, ArgumentValue>>, IDictionary<GraphQLField, IDictionary<string, ArgumentValue>>
101156
{
102157
private readonly IExecutionContext _executionContext;
103158
private readonly IDictionary<GraphQLField, FieldType> _fieldMappings;
104159
private GraphQLField? _lastField;
105160
private IDictionary<string, ArgumentValue>? _lastArgs;
106161
private readonly object _lastFieldLock = new();
162+
private ConcurrentDictionary<GraphQLField, IDictionary<string, ArgumentValue>>? _overrideDictionary;
107163

108164
public FieldArgumentDictionary(IExecutionContext executionContext, IDictionary<GraphQLField, FieldType> fieldMappings)
109165
{
@@ -119,18 +175,28 @@ public IDictionary<string, ArgumentValue> this[GraphQLField key]
119175
return value;
120176
throw new ArgumentOutOfRangeException(nameof(key));
121177
}
178+
set => (_overrideDictionary ??= new())[key] = value;
122179
}
123180

124181
public IEnumerable<GraphQLField> Keys => throw new NotImplementedException();
125182
public IEnumerable<IDictionary<string, ArgumentValue>> Values => throw new NotImplementedException();
126183
public int Count => throw new NotImplementedException();
127184

185+
ICollection<GraphQLField> IDictionary<GraphQLField, IDictionary<string, ArgumentValue>>.Keys => throw new NotImplementedException();
186+
187+
ICollection<IDictionary<string, ArgumentValue>> IDictionary<GraphQLField, IDictionary<string, ArgumentValue>>.Values => throw new NotImplementedException();
188+
189+
bool ICollection<KeyValuePair<GraphQLField, IDictionary<string, ArgumentValue>>>.IsReadOnly => throw new NotImplementedException();
190+
128191
public bool ContainsKey(GraphQLField key) => _fieldMappings.ContainsKey(key);
129192

130193
public IEnumerator<KeyValuePair<GraphQLField, IDictionary<string, ArgumentValue>>> GetEnumerator() => throw new NotImplementedException();
131194

132195
public bool TryGetValue(GraphQLField key, out IDictionary<string, ArgumentValue> value)
133196
{
197+
if (_overrideDictionary?.TryGetValue(key, out value!) == true)
198+
return true;
199+
134200
lock (_lastFieldLock)
135201
{
136202
if (key == _lastField)
@@ -163,6 +229,13 @@ public bool TryGetValue(GraphQLField key, out IDictionary<string, ArgumentValue>
163229
}
164230

165231
IEnumerator IEnumerable.GetEnumerator() => throw new NotImplementedException();
232+
void IDictionary<GraphQLField, IDictionary<string, ArgumentValue>>.Add(GraphQLField key, IDictionary<string, ArgumentValue> value) => throw new NotImplementedException();
233+
bool IDictionary<GraphQLField, IDictionary<string, ArgumentValue>>.Remove(GraphQLField key) => throw new NotImplementedException();
234+
void ICollection<KeyValuePair<GraphQLField, IDictionary<string, ArgumentValue>>>.Add(KeyValuePair<GraphQLField, IDictionary<string, ArgumentValue>> item) => throw new NotImplementedException();
235+
void ICollection<KeyValuePair<GraphQLField, IDictionary<string, ArgumentValue>>>.Clear() => throw new NotImplementedException();
236+
bool ICollection<KeyValuePair<GraphQLField, IDictionary<string, ArgumentValue>>>.Contains(KeyValuePair<GraphQLField, IDictionary<string, ArgumentValue>> item) => throw new NotImplementedException();
237+
void ICollection<KeyValuePair<GraphQLField, IDictionary<string, ArgumentValue>>>.CopyTo(KeyValuePair<GraphQLField, IDictionary<string, ArgumentValue>>[] array, int arrayIndex) => throw new NotImplementedException();
238+
bool ICollection<KeyValuePair<GraphQLField, IDictionary<string, ArgumentValue>>>.Remove(KeyValuePair<GraphQLField, IDictionary<string, ArgumentValue>> item) => throw new NotImplementedException();
166239
}
167240
}
168241
}

‎src/GraphQL/Execution/DocumentExecuter.cs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
using System.Collections.Concurrent;
12
using GraphQL.DI;
23
using GraphQL.Execution;
34
using GraphQL.Instrumentation;
@@ -310,6 +311,13 @@ protected virtual ExecutionContext BuildExecutionContext(ExecutionOptions option
310311

311312
context.ExecutionStrategy = SelectExecutionStrategy(context);
312313

314+
if (context.ExecutionStrategy is not SerialExecutionStrategy)
315+
{
316+
// TODO: remove for v9 after ValidationContext has been changed to use concurrent dictionaries -- see issue #4085
317+
context.ArgumentValues = context.ArgumentValues == null ? null : new ConcurrentDictionary<GraphQLField, IDictionary<string, ArgumentValue>>(context.ArgumentValues);
318+
context.DirectiveValues = context.DirectiveValues == null ? null : new ConcurrentDictionary<ASTNode, IDictionary<string, DirectiveInfo>>(context.DirectiveValues);
319+
}
320+
313321
return context;
314322
}
315323

‎src/GraphQL/Execution/ExecutionContext.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -69,10 +69,10 @@ public ExecutionContext(ExecutionContext context)
6969
public Variables Variables { get; set; }
7070

7171
/// <inheritdoc/>
72-
public IReadOnlyDictionary<GraphQLField, IDictionary<string, ArgumentValue>>? ArgumentValues { get; set; }
72+
public IReadOnlyDictionary<GraphQLField, IDictionary<string, ArgumentValue>>? ArgumentValues { get; set; } // TODO: change to IDictionary for v9 - see #4085
7373

7474
/// <inheritdoc/>
75-
public IReadOnlyDictionary<ASTNode, IDictionary<string, DirectiveInfo>>? DirectiveValues { get; set; }
75+
public IReadOnlyDictionary<ASTNode, IDictionary<string, DirectiveInfo>>? DirectiveValues { get; set; } // TODO: change to IDictionary for v9 - see #4085
7676

7777
/// <inheritdoc/>
7878
public ExecutionErrors Errors { get; set; }

‎src/GraphQL/Execution/ExecutionStrategy.cs

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -293,7 +293,7 @@ static bool Contains(ROM[] array, ROM item, int count)
293293
var fieldType = GetFieldDefinition(context.Schema, (specificType as IComplexGraphType)!, field)
294294
?? throw new InvalidOperationException($"Schema is not configured correctly to fetch field '{field.Name}' from type '{specificType.Name}'.");
295295

296-
Add(fields, field, fieldType);
296+
Add(context, fields, field, fieldType);
297297
}
298298
}
299299
else if (selection is GraphQLFragmentSpread spread)
@@ -319,20 +319,38 @@ static bool Contains(ROM[] array, ROM item, int count)
319319
}
320320
}
321321

322-
static void Add(Dictionary<string, (GraphQLField Field, FieldType FieldType)> fields, GraphQLField field, FieldType fieldType)
322+
static void Add(ExecutionContext context, Dictionary<string, (GraphQLField Field, FieldType FieldType)> fields, GraphQLField field, FieldType fieldType)
323323
{
324324
string name = field.Alias != null ? field.Alias.Name.StringValue : fieldType.Name; //ISSUE:allocation in case of alias
325325

326-
fields[name] = fields.TryGetValue(name, out var original)
327-
? (new GraphQLField(original.Field.Name) // Sets a new field selection node with the child field selection nodes merged with another field's child field selection nodes.
326+
if (fields.TryGetValue(name, out var original))
327+
{
328+
var newField = new GraphQLField(original.Field.Name) // Sets a new field selection node with the child field selection nodes merged with another field's child field selection nodes.
328329
{
329330
Alias = original.Field.Alias,
330331
Arguments = original.Field.Arguments,
331332
SelectionSet = Merge(original.Field.SelectionSet, field.SelectionSet),
332333
Directives = original.Field.Directives,
333334
Location = original.Field.Location,
334-
}, original.FieldType)
335-
: (field, fieldType);
335+
};
336+
var argumentValues = context.ArgumentValues;
337+
if (argumentValues != null && argumentValues.TryGetValue(original.Field, out var arguments))
338+
{
339+
// TODO: remove cast for v9 - see #4085
340+
((IDictionary<GraphQLField, IDictionary<string, ArgumentValue>>)argumentValues)[newField] = arguments;
341+
}
342+
var directiveValues = context.DirectiveValues;
343+
if (directiveValues != null && directiveValues.TryGetValue(original.Field, out var directives))
344+
{
345+
// TODO: remove cast for v9 - see #4085
346+
((IDictionary<ASTNode, IDictionary<string, DirectiveInfo>>)directiveValues)[newField] = directives;
347+
}
348+
fields[name] = (newField, original.FieldType);
349+
}
350+
else
351+
{
352+
fields[name] = (field, fieldType);
353+
}
336354
}
337355

338356
// Returns a new selection set node with the contents merged with another selection set node's contents.

‎src/GraphQL/Validation/ValidationContext.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ internal void Reset()
9797
/// unless no field arguments were found, in which case the value will be <see langword="null"/>.
9898
/// Note that fields will not be present in this dictionary if they would only contain arguments with default values.
9999
/// </summary>
100-
public Dictionary<GraphQLField, IDictionary<string, ArgumentValue>>? ArgumentValues { get; set; }
100+
public Dictionary<GraphQLField, IDictionary<string, ArgumentValue>>? ArgumentValues { get; set; } // TODO: use a concurrent dictionary -- see #4085
101101

102102
/// <summary>
103103
/// A dictionary of fields, and for each field, a dictionary of directives defined for the field with their values.
@@ -107,7 +107,7 @@ internal void Reset()
107107
/// unless no field arguments were found, in which case the value will be <see langword="null"/>.
108108
/// Note that fields will not be present in this dictionary if they would only contain arguments with default values.
109109
/// </summary>
110-
public Dictionary<ASTNode, IDictionary<string, DirectiveInfo>>? DirectiveValues { get; set; }
110+
public Dictionary<ASTNode, IDictionary<string, DirectiveInfo>>? DirectiveValues { get; set; } // TODO: use a concurrent dictionary -- see #4085
111111

112112
/// <summary>
113113
/// Adds a validation error to the list of validation errors.

0 commit comments

Comments
 (0)