Skip to content
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

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

pkese
Copy link
Contributor

@pkese pkese commented Apr 6, 2024

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

Copy link
Collaborator

@xperiandri xperiandri left a comment

Choose a reason for hiding this comment

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

@@ -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
Copy link
Collaborator

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[]) =
Copy link
Collaborator

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

Suggested change
let constructor (args:obj[]) =
let constructor (args : obj[]) =

@pkese
Copy link
Contributor Author

pkese commented Apr 9, 2024

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 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 #472 (comment))

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
Copy link
Collaborator

Thanks, I cap pick this up.
@pkese tell me, please, what was the case for this code?
b29ff4b

@pkese
Copy link
Contributor Author

pkese commented Apr 22, 2024

@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
Copy link
Contributor Author

pkese commented Apr 22, 2024

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

@@ -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
Copy link
Member

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

@xperiandri xperiandri force-pushed the dev branch 5 times, most recently from d90f0e9 to 434b4be Compare October 11, 2024 16:35
@xperiandri
Copy link
Collaborator

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants