Typed controller output

[Krzysztof-Cieslak]

This extends controller CE to support generic output type from the controller actions. If action returns Task<HttpContext option> nothing changes - it's non breaking change for existing code.

If action will return something else (Task<'a>) then we pipe 'a through new Controller.response helper.

Controller.response helper is transforming any object to correct response that client expects. It checks Accept header (and if that's not available it checks Content-Type header) to find correct supported serialization format.

We also support automatically returning Giraffe.GiraffeViewEngine.XmlNode which is Giraffe's server side view type.

[Krzysztof-Cieslak]

Sample controller:

type Response = {
    a: string
    b: string
}

type DifferentResponse = {
    c: int
    d: DateTime
}

let typedController = controller {
    index (fun _ -> task {
        return {a = "hello"; b = "world"}
    })

    add (fun _ -> task {
        return {c = 123; d = DateTime.Now}
    })
}

[Krzysztof-Cieslak]

Comments and review welcomed.

CC: @forki @rmunn @NinoFloris @forki @baronfel @ChrSteinert @jeremyabbott @theimowski

[MangelMaxime]

Is the JSON serializer called by this technique customizable ? I don't know any of Saturn or Giraffe :)

[Krzysztof-Cieslak]

The JSON serializer used by this is JSON serialized used by Giraffe which is Json.Net but can be customized... I think.

[baronfel]

There are builder extensions for it for json and xml

[jeremyabbott]

@Krzysztof-Cieslak trying to grok the update. Makes sense so far. 👍

[jeremyabbott]

It would be nice if we could make something like the following possible:

let initialController =

        let ubHttpFunc = unbox<HttpFuncResult> // aliased since it gets called a lot
        let addGetAction action actionValue path =
          match action with
          // factor out common flow for things that don't take an input on the query string
          | Add | Edit -> (route path >=> (fun _ ctx -> actionValue(ctx) |> ubHttpFunc))

          | _ -> failwith "%O is not a \"GET\" action"

        choose [
          yield GET >=> choose [
            if state.Add.IsSome then
              siteMap.AddPath "/add" "GET"
              match typeof<'AddOutput> with
              // usage for addGetAction
              | k when k = typeof<HttpContext option> -> yield addGetAction Add state.Add.Value "/add"
              | _ -> yield addPlugs Add (route "/add" >=> (fun _ ctx -> state.Add.Value(ctx) |> response<_> ctx))

It seems to fall apart on the actions that take a query string parameter though. :/

[NinoFloris]

Done! @Krzysztof-Cieslak you can pull it in from https://github.com/NinoFloris/Saturn/tree/typed_controller_output/src/Saturn 🙃

Also one thing that's critical to add is special casing Result<'T, 'TError> so Saturn by default unwraps the union and negotiates the response type on the matching case. Otherwise every error returning handler needs to return obj or keep what they have. I think it should be the exception that you want that union serialized vs deconstructed, can also create a custom Result type to prevent some of that a bit.

[jeremyabbott]

@ninofloris that looks GREAT. 👏🏼

[isaacabraham]

The JSON serializer used by this is JSON serialized used by Giraffe which is Json.Net but can be customized... I think.

@Krzysztof-Cieslak yep, we do that at the start of the SAFE template to use the Fable.JSON converter instead of the basic one for Fable interop.

[Krzysztof-Cieslak]

OK, I've remade logic of response helper to include checking quality. Review welcomed - I really should start writing some tests for this stuff ;-)

This, however has one small side effect - most browsers (https://developer.mozilla.org/en-US/docs/Web/HTTP/Content_negotiation/List_of_default_Accept_values) by default accepts application/xml (with high quality) and not application/json - JSON is handled by */* case but it usually has lower quality. This means that by default we try to serialize objects to XML... and with for example F# records this is failing:

<string>
  Controller.Sample.Response cannot be serialized because it does not have a parameterless constructor.</string>

[NinoFloris]

@Krzysztof-Cieslak you should look to https://github.com/aspnet/Mvc/blob/4f1f97b5d524b344c34a25a7031691626d50ec68/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/DefaultOutputFormatterSelector.cs#L90 for inspiration. They have a config switch to ignore the default accept header browsers send, that way we can still use negotiation to by default return json. I wouldn't like my apis to return xml when people use the browser for api discovery.

[rmunn]

I feel like JSON is one place where we should depart from a strict interpretation of the RFC. Most APIs there days are designed to serve up JSON, not XML, and I think that's what people are expecting. So I think it makes the most sense here to default to JSON APIs, and only serve up XML APIs if the library user has specifically requested XML handling for that controller. This is actually allowed for in the RFC:

A user agent cannot rely on proactive negotiation preferences being consistently honored, since the origin server might not implement proactive negotiation for the requested resource or might decide that sending a response that doesn't conform to the user agent's preferences is better than sending a 406 (Not Acceptable) response.

So in the case of APIs specifically, since almost everyone will be expecting a JSON response from those API endpoints and XML would be surprising, I think that it's better to ignore the quality values in this one situation, and send JSON if any part of the Accepts header allows it. Which, sigh, is the opposite of what I said regarding text/plain and text/html earlier... but while consistency is important, not breaking people's expectations is more important.

So while I feel it's a good idea to respect the Accepts header (including quality) for HTML/plaintext decisions in the case of string returns, defaulting to HTML if both are equally acceptable... I think the decision when an F# record is returned should be the original approach: if JSON is at all acceptable then return JSON, else if XML is at all acceptable then return XML, else fail.

[NinoFloris]

@rmunn that's exactly what mvc does, if they see the standard browser accept header they by default don't honor it

[rmunn]

@NinoFloris I hadn't yet seen your comment when I started typing mine, but we're clearly both in agreement, and it's nice to see some supporting evidence (i.e., what Mvc does) as well.

[Krzysztof-Cieslak]

OK, let's go with it:

• If the output type is string then we use quality to decide whether it is plain or html
• If the output type is GiraffeViewEngine.XmlNode AND client accepts text/html then render it.
• If output type is other check if client accept JSON, if so return JSON, otherwise check XML, if XML is also not supported fail with exception.

Everyone OK with that?

EDIT: (Also, we're still in pre 1.0 phase, so we can change it after playing with it a bit)

[Krzysztof-Cieslak]

Also, is there any other standard behavior like that we should support?

[Krzysztof-Cieslak]

Should we do things like unwrapping Option, Choice, and Result ?

[ChrSteinert]

I would highly favored that!

---

From: Krzysztof Cieślak notifications@github.com Sent: Friday, July 6, 2018 9:01:45 PM To: SaturnFramework/Saturn Cc: Christian Steinert; Mention Subject: Re: [SaturnFramework/Saturn] Typed controller output (#86)

Also, should we do things like unwrapping Option, Choice, and Result ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/SaturnFramework/Saturn/pull/86#issuecomment-403120405, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AJaTxfkTv4UItKDzSCroPj9Skz5ojaFmks5uD7QZgaJpZM4VET7U.

[NinoFloris]

Yes! I'm not sure if there are cases where we wouldn't want that, but at least Result would make me very happy

Also about content negotiation, I really think you should special case the existence of */* to go for a default formatter, just like mvc. Otherwise a json api opened in the browser reads suddenly as XML

[Krzysztof-Cieslak]

Also about content negotiation, I really think you should special case the existence of / to go for a default formatter, just like mvc.

Since we don't use quality, and JSON check is first (before XML check) than it will always match if */* is present in Accept header. As application/json is subset of */*

[Krzysztof-Cieslak]

OK, let's merge this one and discuss unwrapping in separate issue as I'm not 100% convinced about it yet.

Thanks a lot everyone for feedback, and reviews! ❤️