Skip to content

Support lists in external reference resolvers #8001

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.

Sign up for GitHub

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 1 commit into
base: main
Choose a base branch
from
Open

Support lists in external reference resolvers #8001

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
44 changes: 44 additions & 0 deletions src/HotChocolate/ApolloFederation/src/ApolloFederation/Resolvers/ArgumentParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
using System.Collections;
using System.Reflection;
using System.Runtime.CompilerServices;
using HotChocolate.Language;
using HotChocolate.Utilities;
Expand Down Expand Up @@ -62,6 +64,45 @@ private static bool TryGetValue<T>(
value = default;
return true;
}
case SyntaxKind.ListValue:
{
if (type is not ListType listType)
{
break;
}

var list = CreateList(listType);
var items = ((ListValueNode)valueNode).Items;
var flatList = !listType.ElementType.IsListType();
var elementType = type.ElementType();
var elementClrType = typeof(T).GetGenericArguments().Single()!;

var innerTryGetValueMethod = typeof(ArgumentParser)
.GetMethod(nameof(TryGetValue), BindingFlags.NonPublic | BindingFlags.Static)!
.MakeGenericMethod(elementClrType);

if (flatList)
{
for (var j = 0; j < items.Count; j++)
{
var parameters = new object?[] { items[j], elementType, path, i + 1, null };
var succeeded = (bool) innerTryGetValueMethod.Invoke(null, parameters)!;
if (!succeeded)
{
throw new NotImplementedException();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% clear on the signature for the function - if we fail to parse any of the list elements, should we short-circuit and just return false? Do we need to make any changes to value?

}
var innerValue = parameters[4]!;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this way of doing things, but I'm not familiar enough with dotnet reflection to know a better way of doing it. From what I can tell:

  1. This method is generic over T, which should be something like List<E>, but we need to recurse by calling TryGetValue<E> on each list element.
  2. This forces us to use the MakeGenericMethod stuff around lines 74-82.
  3. This means we need to call the method using Invoke rather than being able to pass in parameters normally. Invoke takes in an array of object? as its parameters
  4. This gets pretty annoying when dealing with out var parameters - you have to pass in null and then grab out the specific parameter by index here after calling the method.

To me this feels quite fragile, as if the arguments for this method change, this will break. This should be caught by the test added in this PR, but I'm open to other ways of implementing this that'd be more elegant!

list.Add(innerValue);
}
}
else
{
throw new NotImplementedException();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really sure how to handle nested lists in this case. Does it actually matter? I think the logic just works the same way. I'll try adding another test case for this to confirm it works.

}

value = (T)list;
return true;
}
case SyntaxKind.StringValue:
case SyntaxKind.IntValue:
case SyntaxKind.FloatValue:
Expand Down Expand Up @@ -99,6 +140,9 @@ private static bool TryGetValue<T>(
return false;
}

private static IList CreateList(ListType type)
=> (IList)Activator.CreateInstance(type.ToRuntimeType())!;

public static bool Matches(IValueNode valueNode, IReadOnlyList<string[]> required)
{
if (required.Count == 1)
Expand Down
57 changes: 57 additions & 0 deletions src/HotChocolate/ApolloFederation/test/ApolloFederation.Tests/EntitiesResolverTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
using HotChocolate.ApolloFederation.Types;
using HotChocolate.Execution;
using HotChocolate.Language;
using HotChocolate.Resolvers;
using Microsoft.Extensions.DependencyInjection;
using Moq;
using static HotChocolate.ApolloFederation.TestHelper;
Expand Down Expand Up @@ -71,6 +72,36 @@ public async Task TestResolveViaForeignServiceType_MixedTypes()
Assert.Equal("InternalValue", obj.InternalField);
}

[Fact]
public async Task TestResolveViaForeignServiceType_ListType()
{
// arrange
var schema = await new ServiceCollection()
.AddGraphQL()
.AddApolloFederation()
.AddQueryType<Query>()
.BuildSchemaAsync();

var context = CreateResolverContext(schema);

// act
var representations = new List<Representation>
{
new("ListFieldType",
new ObjectValueNode(
new ObjectFieldNode("id", "1"),
new ObjectFieldNode("listField", new ListValueNode(new List<IntValueNode> { new(1), new(2), new(3) })))),
};

// assert
var result =
await EntitiesResolver.ResolveAsync(schema, representations, context);
var obj = Assert.IsType<ListFieldType>(result[0]);
Assert.Equal("1", obj.Id);
Assert.Equal([1, 2, 3], obj.ListField);
Assert.Equal("InternalValue", obj.InternalField);
}

[Fact]
public async Task TestResolveViaEntityResolver()
{
Expand Down Expand Up @@ -249,6 +280,7 @@ public class Query
public TypeWithoutRefResolver TypeWithoutRefResolver { get; set; } = default!;
public MixedFieldTypes MixedFieldTypes { get; set; } = default!;
public FederatedType TypeWithReferenceResolverMany { get; set; } = default!;
public ListFieldType ListFieldType { get; set; } = default!;
}

public class TypeWithoutRefResolver
Expand Down Expand Up @@ -335,6 +367,31 @@ public class FederatedType
}
}

[ExtendServiceType]
public class ListFieldType
{
public ListFieldType(string id, List<double> listField)
{
Id = id;
ListField = listField;
}

[Key]
[External]
public string Id { get; }

[External]
public List<double> ListField { get; }
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested this locally by swapping out various types of lists, but I'm happy to add in a more comprehensive set of tests depending on what you'd prefer eg:

  • Just the one test, but adding in many external list fields with differing inner types
  • Tests for a few key types (eg: nested lists/lists of objects)
  • I could try and find a way to parameterize the test for the list's inner type but I think that'd be overcomplicating things


public string InternalField { get; set; } = "InternalValue";

[ReferenceResolver]
public static ListFieldType GetByExternal(string id, List<double> listField, IResolverContext context)
{
return new(id, listField);
}
}

public class FederatedTypeDataLoader : BatchDataLoader<string, FederatedType>
{
public int TimesCalled { get; private set; }
Expand Down