fsprojects / FSharp.Data.GraphQL

FSharp implementation of Facebook GraphQL query language.
http://fsprojects.github.io/FSharp.Data.GraphQL/
MIT License
395 stars 72 forks source link

[RFC] Add support for generic IDictionary<string,obj> query inputs - for #472 #475

Open pkese opened 2 months ago

pkese commented 2 months ago

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 as Define.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).

pkese commented 2 months ago

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 (https://github.com/fsprojects/FSharp.Data.GraphQL/pull/475/commits/b29ff4b0dda7661aeb711eb919fb8658a80f8077) is a hack around the fact that deeply nested InputsObjects lack ExecuteInput member ... again hinting at the fact that the data structures that we built when analyzing input objects specs and its types do not match the stuff that we're trying to parse at runtime.

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,
this will also break the case if someone would provide a type that implements a IDictionary interface (e.g. for https://github.com/fsprojects/FSharp.Data.GraphQL/issues/472#issuecomment-2041054066)

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. This is fixable, but at the same time the fix would introduce more changes that are probably not going into the right direction. Current hack works for my personal use, because in my case, all InputObjects are dynamically generated, so I'm not hitting this issue, but for someone else, this may not lead to expected results.

xperiandri commented 2 months ago

Thanks, I cap pick this up. @pkese tell me, please, what was the case for this code? https://github.com/fsprojects/FSharp.Data.GraphQL/commit/b29ff4b0dda7661aeb711eb919fb8658a80f8077

pkese commented 2 months ago

@xperiandri
The thing was that when defining generic (untyped) objects as inputs, the library will only parse and populate the fist level of inputs.
If input object contains nested objects, these ended up unpopulated - i.e. ctx.Args would have just the top layer of data populated, but not any of the nested inputs (nested inputs in ctx.Args would end up being empty obj() without any properties).

I assume it is that ExecuteInput is not created recursively for subfields, when analyzing Define.Object<_>.

pkese commented 2 months ago

... 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).