Giraffe icon indicating copy to clipboard operation
Giraffe copied to clipboard

Why is tryBindJson function missing?

Open Dzoukr opened this issue 6 years ago • 4 comments

Please, take this more as a question. Why there is nothing like tryBindJson? In case the the frontend sends bad format of message, bindJson<'a> fails with 500 error and explanation, but I would rather return 400 with message what went wrong. I created my own:

let tryBindJson<'T> (errorF:System.Exception -> HttpHandler) (f : 'T -> HttpHandler) : HttpHandler =
    fun (next : HttpFunc) (ctx : HttpContext) ->
        task {
            try
                let! model = ctx.BindJsonAsync<'T>()
                return! f model next ctx
            with ex -> return! errorF ex next ctx
        }

Maybe there is something wrong with this approach so I better ask - is it on purpose? If not, should I send PR?

Thanks for such great library anyway - love it!

Dzoukr avatar Nov 24 '18 21:11 Dzoukr

There should be a tryBindJson and a tryBindXml, but unfortunately the current libraries which Giraffe uses for JSON and XML serialization don't easily support this use case. The try{...} functions only bind a model if all mandatory fields of a record type have been provided (e.g. only fields of type option may be missing), but currently JSON.NET would still create such an object.

I think this might either require some extra work to provide some settings to the JSON deserialization or perhaps even look at different libraries.

Anyways, you are correct and a PR to help with this would be as always be very welcome.

EDIT: More info here: https://dusted.codes/giraffe-110-more-routing-handlers-better-model-binding-and-brand-new-model-validation-api

dustinmoris avatar Nov 26 '18 21:11 dustinmoris

Thanks for explanation, @dustinmoris. I think I see you point.

I currently use custom serializer to check all non-optional properties are provided, which prevents JSON.NET create such an object (actually throws an exception):

type RequireAllPropertiesContractResolver() =
    inherit CamelCasePropertyNamesContractResolver()
    
    override __.CreateProperty(memb, serialization) =
        let prop = base.CreateProperty(memb, serialization)
        let isRequired = not (prop.PropertyType.IsGenericType && prop.PropertyType.GetGenericTypeDefinition() = typedefof<option<_>>)
        if isRequired then prop.Required <- Required.Always
        prop

Maybe we could something like this as "safe default", but I'm affraid this still doesn't solve the problem. You can still easily create you own JSON serializer and allow to "pass through" invalid (null) values like JSON.NET does on default. I believe this is responsibility of programmer to select correct serializer and setup rules for (de)serialization (e.g. throw exception on missing non-optional property).

So from Giraffe perspective, shouldn't be the tryXYZ function only about trying to call defined serializer and if it fails (exception), than go through error path exception -> HttpHandler? I mean... you setup rules (can be benevolent, I agree) and then you try to bind model having those rules.

I dunno, maybe I am looking from different/completely wrong perspective on it, but feel there could be too much effort on checking things which are up to developer to properly setup/check.

Anyways, thanks for you response! Once I would feel I know how to help, I will send PR for sure.

Dzoukr avatar Nov 26 '18 22:11 Dzoukr

I guess this is a related question - am I correct that bindJson (unlike bindQueryString) won't deserialize non-empty values to an option type?

i.e.

type Message =
    {
        Text : string option
    }
let indexHandler : HttpHandler =
    fun (next : HttpFunc) (ctx : HttpContext) ->
        task {
            // Binds a JSON payload to a Car object
            let! message = ctx.BindJsonAsync<Message>()

            // Sends the object back to the client
            return! Successful.OK "OK" next ctx
        }

and posting:

{"text":"foo"}

will throw:

Newtonsoft.Json.JsonSerializationException: No 'Case' property with union name found. Path '', line 1, position 14.

Reproduction here

nmfisher avatar Apr 13 '19 04:04 nmfisher

For anyone finding this in 2021 or later, I solved this by defining tryBindJson as:

open Microsoft.AspNetCore.Http
open Giraffe
open FSharp.Control.Tasks

module Extensions =
  let tryBindJson<'T> (parsingErrorHandler: string -> HttpHandler) (successHandler: 'T -> HttpHandler): HttpHandler =
    fun (next : HttpFunc) (ctx : HttpContext) ->
      task {
        try
          let! model = ctx.BindJsonAsync<'T>()
          return! successHandler model next ctx
        with ex ->
          return! parsingErrorHandler "Malformed request or missing field in request body" next ctx
      }

It can then be used in your webApp as:

[<CLIMutable>]
type LoginRequest =
  {
    Username: string
    Password: string
  }

let webApp =
  let parsingError message = setStatusCode 400 >=> json message
    choose [
      subRoute "/api"
        (choose [
          POST >=> choose [
            route "/login" >=> tryBindJson<LoginRequest> parsingError User.authenticate
          ]
        ])
      setStatusCode 404 >=> json "Not found"
    ]

where parsingError: HttpHandler is used when parsing fails and User.authenticate: HttpHandler is used when the parser succeeds.

esbenbjerre avatar Feb 11 '21 16:02 esbenbjerre