Skip to content

Commit dcd4a25

Browse files
lukemurrayclaude
andcommitted
Fix the same shape without a parent service field, bump to 6.2.3
The replacer fix alone left `srv.Get(new Key(ctx.A, ctx.B))` broken on a plain service field with nothing above it - it turned "An item with the same key has already been added" into "Argument types do not match", because the extracted expression is still the whole construction. Walk through New/MemberInit in ExpressionExtractor.ProcessPotentialLeaf so each member read credits itself, which is what the class doc-comment already describes. Keep the ExpressionReplacer change: the extractor still emits a repeat for a conditional argument, which hits the identical crash. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent efdcc84 commit dcd4a25

6 files changed

Lines changed: 88 additions & 13 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,13 @@
1+
# 6.2.3
2+
3+
## Fixes
4+
5+
- Fixed `An item with the same key has already been added` when a service field's resolver builds an object from more than one context member as an argument to the service call - `(ctx, srv) => srv.Get(new Key(ctx.A, ctx.B))`. `ExpressionExtractor` credited both member reads to the enclosing construction rather than to themselves, so it emitted the same `Expression` instance twice under the one name and `ExpressionReplacer` added each of them to a dictionary keyed by node identity. A construction is now walked through rather than treated as a leaf, so each read is extracted on its own. `ExpressionReplacer` also tolerates a repeated expression, which the extractor still emits for other shapes (a conditional argument, where the branch reads are both credited to the conditional).
6+
17
# 6.2.2
28

39
## Fixes
410

5-
- Fixed `An item with the same key has already been added` when a service field's resolver builds an object from more than one context member as an argument to the service call (`(ctx, srv) => srv.Get(new Key(ctx.A, ctx.B))`) and that field is selected below another service field. `ExpressionExtractor` credits both member reads to the enclosing construction rather than to themselves, so it emits the same `Expression` instance twice under the one name - which its own contract allows - and `ExpressionReplacer` was adding each of them to a dictionary keyed by node identity.
611
- Fixed a field whose dotnet type is a nullable value type (`Instant?`, `DateTime?`, `int?`) resolving to `default(T)` instead of `null`, for a query that also selects an async field. Results are rebuilt after any async field is awaited, and each member's new type was taken from the resolved value's runtime type.
712

813
# 6.2.1

‎src/EntityGraphQL.AspNet/EntityGraphQL.AspNet.csproj‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
<AssemblyName>EntityGraphQL.AspNet</AssemblyName>
55
<PackageId>EntityGraphQL.AspNet</PackageId>
66
<LangVersion>14</LangVersion>
7-
<PackageVersion>6.2.2</PackageVersion>
7+
<PackageVersion>6.2.3</PackageVersion>
88
<Description>Contains ASP.NET extensions and middleware for EntityGraphQL</Description>
99
<Authors>Luke Murray</Authors>
1010
<PackageProjectUrl>https://github.com/lukemurray/EntityGraphQL</PackageProjectUrl>

‎src/EntityGraphQL/Compiler/Util/ExpressionExtractor.cs‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,13 @@ protected override Expression VisitBinary(BinaryExpression node)
8787

8888
private void ProcessPotentialLeaf(Expression node)
8989
{
90-
var shouldAddNotAdd = node.NodeType == ExpressionType.MemberAccess && ((MemberExpression)node).Expression?.Type.IsNullableType() == true;
90+
// A construction is not a leaf - it is a wrapper around the reads that build it. Visiting it with itself on
91+
// the stack credits every one of those reads to the construction, so `srv.Get(new Key(ctx.A, ctx.B))` extracts
92+
// that one node twice instead of ctx.A and ctx.B. Visit it without pushing so each read credits itself.
93+
var shouldAddNotAdd =
94+
(node.NodeType == ExpressionType.MemberAccess && ((MemberExpression)node).Expression?.Type.IsNullableType() == true)
95+
|| node.NodeType == ExpressionType.New
96+
|| node.NodeType == ExpressionType.MemberInit;
9197
if (shouldAddNotAdd)
9298
Visit(node);
9399
else

‎src/EntityGraphQL/EntityGraphQL.csproj‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
<LangVersion>14.0</LangVersion>
55
<AssemblyName>EntityGraphQL</AssemblyName>
66
<PackageId>EntityGraphQL</PackageId>
7-
<PackageVersion>6.2.2</PackageVersion>
7+
<PackageVersion>6.2.3</PackageVersion>
88
<Description>A GraphQL library for .NET. Compiles queries into .NET Expressions (LinqProvider) for runtime execution against object graphs. E.g. against an ORM data model (EntityFramework or others) or just an in-memory object.</Description>
99
<Authors>Luke Murray</Authors>
1010
<PackageProjectUrl>https://github.com/lukemurray/EntityGraphQL</PackageProjectUrl>

‎src/tests/EntityGraphQL.Tests/QueryTests/ServiceFieldTests.cs‎

Lines changed: 50 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1377,10 +1377,56 @@ public void TestCollectionWithServiceUsingNewInExpression()
13771377
}
13781378

13791379
/// <summary>
1380-
/// A resolver that builds an object from more than one context member, as an argument to the service call,
1381-
/// has both member reads credited to the enclosing new/init expression - so ExpressionExtractor records that
1382-
/// single node instance twice. Below another service field the bulk path is unavailable and ExpressionReplacer
1383-
/// is built from those expressions, which used to throw "An item with the same key has already been added".
1380+
/// A resolver that builds an object from more than one context member, as an argument to the service call.
1381+
/// ExpressionExtractor used to credit both member reads to the enclosing new/init expression, recording that
1382+
/// single node instance twice - which threw "An item with the same key has already been added" when
1383+
/// ExpressionReplacer was built from those expressions, and left the second pass unable to rebuild the call
1384+
/// once the replacer tolerated the repeat ("Argument types do not match"). The construction is now walked
1385+
/// through, so each member read is extracted on its own.
1386+
/// </summary>
1387+
[Fact]
1388+
public void TestServiceFieldWithNewInServiceCallArg()
1389+
{
1390+
var schema = SchemaBuilder.FromObject<TestDataContext>();
1391+
schema.AddType<ProjectConfig>("ProjectConfig").AddAllFields();
1392+
schema.Type<User>().AddField("config", "Config for the user").Resolve<ConfigService>((user, srv) => srv.Get(new Project { Id = user.Id, Name = user.Field2 }));
1393+
1394+
var gql = new QueryRequest { Query = "{ users { id config { type } } }" };
1395+
1396+
var context = new TestDataContext().FillWithTestData();
1397+
var sp = new ServiceCollection().AddSingleton(new ConfigService()).BuildServiceProvider();
1398+
1399+
var res = schema.ExecuteRequestWithContext(gql, context, sp, null);
1400+
Assert.Null(res.Errors);
1401+
dynamic users = res.Data!["users"]!;
1402+
Assert.Equal("Something", users[0].config.type);
1403+
}
1404+
1405+
/// <summary>
1406+
/// Same shape, but the argument is a conditional. There is no VisitConditional override in the extractor so the
1407+
/// ifTrue/ifFalse reads are still both credited to the conditional - the repeat ExpressionReplacer has to tolerate.
1408+
/// </summary>
1409+
[Fact]
1410+
public void TestServiceFieldWithConditionalInServiceCallArg()
1411+
{
1412+
var schema = SchemaBuilder.FromObject<TestDataContext>();
1413+
schema.AddType<ProjectConfig>("ProjectConfig").AddAllFields();
1414+
schema.Type<User>().AddField("config", "Config for the user").Resolve<ConfigService>((user, srv) => srv.Get(user.Id > 1 ? user.Id : user.Field2.Length));
1415+
1416+
var gql = new QueryRequest { Query = "{ users { id config { type } } }" };
1417+
1418+
var context = new TestDataContext().FillWithTestData();
1419+
var sp = new ServiceCollection().AddSingleton(new ConfigService()).BuildServiceProvider();
1420+
1421+
var res = schema.ExecuteRequestWithContext(gql, context, sp, null);
1422+
Assert.Null(res.Errors);
1423+
dynamic users = res.Data!["users"]!;
1424+
Assert.Equal("Something", users[0].config.type);
1425+
}
1426+
1427+
/// <summary>
1428+
/// As above but selected below another service field, where the bulk path is unavailable and the per-item
1429+
/// resolver runs instead.
13841430
/// </summary>
13851431
[Fact]
13861432
public void TestServiceFieldWithNewInServiceCallArgUnderServiceField()

‎src/tests/EntityGraphQL.Tests/Util/ExpressionExtractorTests.cs‎

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -53,16 +53,34 @@ public void ExtractLongMemberExpressionSameNameInMethod()
5353
[Fact]
5454
public void ExtractObjectBuiltFromMultipleMembersInMethodArg()
5555
{
56-
// Building an object from 2 context members as an argument to the service call - both member reads are
57-
// credited to the enclosing expression, so the same node instance is extracted twice under the one name.
58-
// ExpressionReplacer relies on tolerating that repeat - see TestServiceFieldWithNewInServiceCallArgUnderServiceField
56+
// Building an object from 2 context members as an argument to the service call - the construction is not a
57+
// leaf, so each member read is extracted on its own rather than both being credited to the construction.
5958
Expression<Func<User, ConfigService, ProjectConfig>> expression = (user, srv) => srv.Get(new Project { Id = user.Id, Name = user.Field2 });
6059
var extractor = new ExpressionExtractor();
6160
var extracted = extractor.Extract(expression.Body, expression.Parameters[0], false);
6261
Assert.NotNull(extracted);
63-
Assert.Single(extracted);
62+
Assert.Equal(2, extracted.Count);
63+
var init = (MemberInitExpression)((MethodCallExpression)expression.Body).Arguments[0];
64+
Assert.Equal("egql__user_Id", extracted.First().Key);
65+
Assert.Equal(((MemberAssignment)init.Bindings[0]).Expression, extracted.First().Value.Single());
66+
Assert.Equal("egql__user_Field2", extracted.ElementAt(1).Key);
67+
Assert.Equal(((MemberAssignment)init.Bindings[1]).Expression, extracted.ElementAt(1).Value.Single());
68+
}
69+
70+
[Fact]
71+
public void ExtractConditionalInMethodArg()
72+
{
73+
// No VisitConditional override - the ifTrue/ifFalse reads are both credited to the conditional, so that one
74+
// node is extracted twice under the one name. ExpressionReplacer tolerates that repeat by design.
75+
// (the test is a binary, which VisitBinary walks into, so user.Id is also extracted on its own)
76+
Expression<Func<User, ConfigService, ProjectConfig>> expression = (user, srv) => srv.Get(user.Id > 1 ? user.Id : user.Field2.Length);
77+
var extractor = new ExpressionExtractor();
78+
var extracted = extractor.Extract(expression.Body, expression.Parameters[0], false);
79+
Assert.NotNull(extracted);
80+
Assert.Equal(2, extracted.Count);
81+
Assert.Equal("egql__user_Id", extracted.First().Key);
6482
var arg = ((MethodCallExpression)expression.Body).Arguments[0];
65-
Assert.Equal([arg, arg], extracted.First().Value);
83+
Assert.Equal([arg, arg], extracted.ElementAt(1).Value);
6684
}
6785

6886
[Fact]

0 commit comments

Comments
 (0)