Skip to content

SelectionExpressionBuilder: Fallback to best-matching constructor if MemberInit fails #8487

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ private void CollectTypes(Context context, ISelection selection, TypeContainer p
return BuildSelectionSetExpression(context, parent.Nodes[0]);
}

private static MemberInitExpression? BuildSelectionSetExpression(
private static Expression? BuildSelectionSetExpression(
Context context,
TypeNode parent)
{
Expand All @@ -183,9 +183,84 @@ private void CollectTypes(Context context, ISelection selection, TypeContainer p
return null;
}

return Expression.MemberInit(
Expression.New(context.ParentType),
assignments.ToImmutable());
var assignmentList = assignments.ToImmutable();

// Quick path: parameterless constructor + MemberInit
var parameterlessCtor = context.ParentType.GetConstructor(Type.EmptyTypes);
if (parameterlessCtor != null)
{
var allWritable = assignmentList.All(a =>
a.Member is PropertyInfo { CanWrite: true, SetMethod.IsPublic: true });
if (allWritable)
{
return Expression.MemberInit(
Expression.New(parameterlessCtor),
assignmentList);
}
}

// Fallback: Use the best matching constructor.
// Argument names must match parameter names (case-insensitive),
// and argument types must be assignable to the parameter types.
var bestMatchingCtor = context.ParentType.GetConstructors()
.Select(c => (Constructor: c, Parameters: c.GetParameters()))
.OrderBy(cp => cp.Parameters.Length)
.FirstOrDefault(cp =>
cp.Parameters.Length >= assignmentList.Length
&& assignmentList.All(a =>
cp.Parameters.Any(p =>
string.Equals(a.Member.Name, p.Name, StringComparison.OrdinalIgnoreCase)
&& a.Expression.Type.IsAssignableTo(p.ParameterType))));

if (bestMatchingCtor.Constructor != null)
{
var ctorParams = bestMatchingCtor.Parameters;
var args = ctorParams.Select(p =>
{
var match = assignmentList.FirstOrDefault(a =>
string.Equals(a.Member.Name, p.Name, StringComparison.OrdinalIgnoreCase)
&& a.Expression.Type.IsAssignableTo(p.ParameterType));

if (match != null)
{
return match.Expression.Type == p.ParameterType
? match.Expression
: Expression.Convert(match.Expression, p.ParameterType);
}

if (p.HasDefaultValue)
{
return Expression.Convert(Expression.Constant(p.DefaultValue), p.ParameterType);
}

if (!p.ParameterType.IsValueType && IsMarkedAsExplicitlyNonNullable(p))
{
/*
* The constructor includes a non-nullable reference type (e.g.,
* public record Foo(string Prop1, string Prop2);),
* but we cannot provide a value for 'Prop2' based on the current selection set,
* as 'Prop2' is not included in it.
*
* To fix this, consider updating the constructor to:
* - Make the parameter nullable: Foo(string Prop1, string? Prop2);
* - Provide a default value: Foo(string Prop1, string Prop2 = "");
* - That may also be a null-forgiving one: Foo(string Prop1, string Prop2 = default!)
*
* Lets hope that this error is descriptive for the user.
*/
throw new InvalidOperationException(
$"Cannot construct '{context.ParentType.Name}': missing required argument '{p.Name}' "
+ "(non-nullable reference type with no default value).");
}

return Expression.Default(p.ParameterType);
}).ToArray();

return Expression.New(bestMatchingCtor.Constructor, args);
}

throw new InvalidOperationException(
$"No writable properties or suitable constructor found for type '{context.ParentType.Name}'.");
}

private void CollectSelection(
Expand Down Expand Up @@ -350,4 +425,9 @@ private readonly record struct Context(
: null;
}
}

static bool IsMarkedAsExplicitlyNonNullable(ParameterInfo p)
{
return new NullabilityInfoContext().Create(p).WriteState is NullabilityState.NotNull;
}
}
121 changes: 121 additions & 0 deletions src/HotChocolate/Data/test/Data.Tests/IntegrationTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using HotChocolate.Data.Sorting;
using HotChocolate.Execution;
using HotChocolate.Types;
using HotChocolate.Types.Pagination;
using Microsoft.Extensions.DependencyInjection;

namespace HotChocolate.Data;
Expand Down Expand Up @@ -960,6 +961,88 @@ public async Task AsSortDefinition_Descending_QueryContext_2()
result.MatchSnapshot();
}

[Fact]
public async Task UsingQueryContext_ShouldNotBreak_Pagination_ForRecordReturnType()
{
// arrange
var executor = await new ServiceCollection()
.AddGraphQL()
.AddErrorFilter(x => new Error {Message = x.Exception!.Message})
.AddFiltering()
.AddSorting()
.AddProjections()
.AddQueryType<RecordQuery>()
.BuildRequestExecutorAsync();

// act
var result = await executor.ExecuteAsync(
"""
query f {
# Selection set matches record-ctor params
u1: nonNullUsers {
edges {
node {
firstName
id
}
}
}

# Selection set is subset of record-ctor params (incompatible, since we can`t provide a valid value)
u2: nonNullUsers {
edges {
node {
firstName
}
}
}

# Selection set matches record-ctor params
u3: nonNullDefaultValuesUsers {
edges {
node {
id
firstName
zipCode
address
}
}
}

# Selection set is subset of record-ctor params (compatible, since we can provide the default value)
u4: nonNullDefaultValuesUsers {
edges {
node {
firstName
}
}
}

# Selection set matches record-ctor params
u5: nullableUsers {
edges {
node {
id
firstName
}
}
}

# Selection set is subset of record-ctor params
u6: nullableUsers {
edges {
node {
firstName
}
}
}
}
""");

// assert
result.MatchSnapshot();
}

[QueryType]
public static class StaticQuery
{
Expand Down Expand Up @@ -1214,4 +1297,42 @@ public IQueryable<Author> GetAuthorsData2(QueryContext<Author> context)
}.AsQueryable()
.With(context, t => t with { Operations = t.Operations.Add(SortBy<Author>.Ascending(t => t.Id)) });
}

#pragma warning disable RCS1102
public class RecordQuery
#pragma warning restore RCS1102
{
[UsePaging]
[UseFiltering]
public Connection<NonNullUser> GetNonNullUsers(QueryContext<NonNullUser> query)
=> Connection.Empty<NonNullUser>();

[UsePaging]
[UseFiltering]
public Connection<NonNullDefaultValuesUser> GetNonNullDefaultValuesUsers(QueryContext<NonNullDefaultValuesUser> query)
=> Connection.Empty<NonNullDefaultValuesUser>();

[UsePaging]
[UseFiltering]
public Connection<NullableUser> GetNullableUsers(QueryContext<NullableUser> query)
=> Connection.Empty<NullableUser>();

public record NonNullUser(
string Id,
string FirstName
);

public record NonNullDefaultValuesUser(
string Id = "",
string FirstName = "",
string ZipCode = null!,
// ReSharper disable once PreferConcreteValueOverDefault
string Address = default!
);

public record NullableUser(
string? Id,
string? FirstName
);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
{
"errors": [
{
"message": "Cannot construct 'NonNullUser': missing required argument 'Id' (non-nullable reference type with no default value).",
"locations": [
{
"line": 13,
"column": 3
}
],
"path": [
"u2"
]
}
],
"data": {
"u1": {
"edges": []
},
"u2": null,
"u3": {
"edges": []
},
"u4": {
"edges": []
},
"u5": {
"edges": []
},
"u6": {
"edges": []
}
}
}
Loading