Skip to content

* First pass at custom coerce for input objects - #350

Closed
njlr wants to merge 1 commit into
fsprojects:devfrom
njlr:issue-327-input-object-cast
Closed

* First pass at custom coerce for input objects#350
njlr wants to merge 1 commit into
fsprojects:devfrom
njlr:issue-327-input-object-cast

Conversation

@njlr

@njlrnjlr commented Sep 26, 2021

Copy link
Copy Markdown
Contributor

No description provided.

@xperiandrixperiandri left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I like what you have done. This is useful.
Also, it would be more useful to be able to return an error and pass that error to the response with error to GraphQL field mapping.

Comment on lines +173 to +175
|> Array.map (fun field ->
let valueFound = Map.tryFind field.Name map |> Option.toObj
let coercedValue = coerceVariableValue false field.TypeDef vardef valueFound (errMsg + (sprintf "in field '%s': " field.Name))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
|>Array.map (fun field ->
letvalueFound= Map.tryFind field.Name map|> Option.toObj
letcoercedValue= coerceVariableValue false field.TypeDef vardef valueFound (errMsg +(sprintf "in field '%s': " field.Name))
|>Seq.map (fun field ->
letvalueFound=map |>Map.tryFind field.Name |> Option.toObj
letcoercedValue= coerceVariableValue false field.TypeDef vardef valueFound (errMsg +(sprintf "in field '%s': " field.Name))

Maybe it does not worth to materialize it to an array

Comment threadsrc/FSharp.Data.GraphQL.Shared/TypeSystem.fs
@@ -38,31 +38,36 @@ let rec internal compileByType (errMsg: string) (inputDef: InputDef): ExecuteInp
| Scalar scalardef ->
variableOrElse (scalardef.CoerceInput >> Option.toObj)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@jberzy can we switch this to Result<,> and pass an error to the response?

@xperiandri

Copy link
Copy Markdown
Collaborator

There is an issue I have with current scalar implementation

interface ScalarDef withmemberx.Name= x.Name
memberx.Description= x.Description
memberx.CoerceInput input = x.CoerceInput input |> Option.map box
memberx.CoerceValue value =(x.CoerceValue value)|> Option.map box

This piece is very contraintuitive

memberx.CoerceInput input = x.CoerceInput input |> Option.map box
memberx.CoerceValue value =(x.CoerceValue value)|> Option.map box

@jberzy at least XML comments with clear description required.
Can we get rid of CoerceValue at all?

And CoerceInput here may be optional. Because if I preprocess variables with JSON deserializer I don't need any coercion at all.

@njlr

njlr commented Oct 6, 2021

Copy link
Copy Markdown
ContributorAuthor

@xperiandri I have fleshed out this idea further: #351

@njlrnjlr closed this Oct 6, 2021
Sign up for freeto 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.

2 participants

@njlr@xperiandri