-
Notifications
You must be signed in to change notification settings - Fork 73
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
[RFC] Add support for generic IDictionary<string,obj> query inputs - for #472 #475
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also add some tests to https://github.com/fsprojects/FSharp.Data.GraphQL/tree/dev/tests/FSharp.Data.GraphQL.Tests/Variables%20and%20Inputs ?
@@ -89,10 +89,35 @@ let rec internal compileByType (inputObjectPath: FieldPath) (inputSource : Input | |||
|
|||
| InputObject objDef -> | |||
let objtype = objDef.Type | |||
let ctor = ReflectionHelper.matchConstructor objtype (objDef.Fields |> Array.map (fun x -> x.Name)) | |||
let (constructor : obj[] -> obj), (parameterInfos : Reflection.ParameterInfo[]) = | |||
if typeof<IDictionary<string,obj>>.IsAssignableFrom(objtype) then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also check for IReadOnlyDictionary<string, obj>
and obj
and create ImmutableDictionary<string, obj>
in that case
| _ -> Reflection.ParameterAttributes.None | ||
} | ||
|] | ||
let constructor (args:obj[]) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And format according to the Fantomas settings
let constructor (args:obj[]) = | |
let constructor (args : obj[]) = |
I think we are dealing here with a bigger problem than just these few cosmetic changes. The solution presented here is rather hackish. I'm not familiar enough with the codebase to do proper refactoring required to implement this correctly, so I'm fiddling with the code just enough to keep my own app working and afloat. I think that the real culprit is probably somewhere else, e.g. assuming that input objects are expected to be flat objects where each property is a normal F# type rather than accounting for the case where someone might specify deeply nested input objects. And providing workarounds at the time of object construction is probably not the right place to fix the issue. The last commit (b29ff4b) is a hack around the fact that deeply nested InputsObjects lack I guess that this is beyond me and that someone who is a bit more familiar with the code would be able to provide a much cleaner solution to this. Btw, type Range = { Min: int; Max: int }
with interface IDictionary<string,obj> with
<<IDictionary implementation..>> because in such case, rather than constructing the Range, it would try to construct and populate the dictionary. |
b29ff4b
to
85f95c2
Compare
@xperiandri I assume it is that |
... so what I did here was that I just mocked what ExecuteInput would have done if it had been defined (here I was assuming it's generic objects all the way down, which in my case it is). |
85f95c2
to
33e608c
Compare
@@ -163,14 +188,27 @@ let rec internal compileByType (inputObjectPath: FieldPath) (inputSource : Input | |||
| ValueSome field -> | |||
match Map.tryFind field.Name props with | |||
| None -> Ok <| wrapOptionalNone param.ParameterType field.TypeDef.Type | |||
| Some input when isNull (box field.ExecuteInput) -> | |||
// hack around the case where field.ExecuteInput is null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe take this hack as a separate function and not a nested function inside many loops...
33e608c
to
bed4cc6
Compare
d90f0e9
to
434b4be
Compare
Could you reset your branch pointer one commit back to remove unnecessary commit and add a sample of directive usage which showcases the functionality implemented in this PR? |
This will check if InputObject's type is castable to
IDictionary<string, obj>
and will populate that type.It will work with both
Define.InputObject<Dictionary<string,obj>>
as well asDefine.InputObject<Dynamic.ExpandoObject>
Idealy we'd reverse the order and first check if we can get a type constructor with matching parameters for a custom type and only then try if the type is castable to
IDictionary<string,obj>
(in case user had defined interface IDictionary on their own custom type).