Support lists in external reference resolvers #8001
Conversation
|
|
| var succeeded = (bool) innerTryGetValueMethod.Invoke(null, parameters)!; | ||
| if (!succeeded) | ||
| { | ||
| throw new NotImplementedException(); |
There was a problem hiding this comment.
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?
| { | ||
| throw new NotImplementedException(); | ||
| } | ||
| var innerValue = parameters[4]!; |
There was a problem hiding this comment.
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:
- This method is generic over
T, which should be something likeList<E>, but we need to recurse by callingTryGetValue<E>on each list element. - This forces us to use the
MakeGenericMethodstuff around lines 74-82. - This means we need to call the method using
Invokerather than being able to pass in parameters normally. Invoke takes in an array ofobject?as its parameters - This gets pretty annoying when dealing with
out varparameters - 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!
| } | ||
| else | ||
| { | ||
| throw new NotImplementedException(); |
There was a problem hiding this comment.
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.
| public string Id { get; } | ||
|
|
||
| [External] | ||
| public List<double> ListField { get; } |
There was a problem hiding this comment.
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
Add support for the Apollo
[External]attribute/directive on list types.@externalsupport, and an implementation for the ArgumentParser that lets it parse lists.Closes #7991