Giraffe: Make JSON serializer configurable

[alfonsogarciacaro]

Reference https://github.com/SAFE-Stack/SAFE-BookStore/pull/255#issuecomment-352194907

Some users may have particular needs in JSON serialization. For example, when interacting with Fable apps you need to pass the Fable.JsonConverter. This means you cannot use the default http handlers in Giraffe and they need to be customized. Two alternatives have been proposed to fix but they both have inconveniences:

• Write a custom negotiate function, but users need to remember to use it every time (like Successful.ok (myCustomNegotiate promos) next ctx) and the compiler won't help them when they don't.
• Create a Giraffe.Fable package, but it looks too big an effort to create a full package to override all Giraffe handlers only in order to pass a custom converter to JSON serialization settings.

Instead of those :point_up: would you be OK with making the defaultJsonSerializerSettings configurable? And if so, what would be best way to do it (besides making that field mutable)?

[dustinmoris]

Hmm... I agree that it is not ideal at the moment and the more I read your issue and think about similar issues from the past I agree that we could have done much better with the current implementation.

The first thought which comes to my mind would be to use ASP.NET Core's DI model to solve this issue in a more elegant way.

For example, we could change the current json http handler to the following:

let json (dataObj : obj) : HttpHandler =
    fun (next : HttpFunc) (ctx : HttpContext) ->
        let settings = 
            match ctx.GetService<JsonSerializerSettings>() with
            | null -> defaultJsonSerializerSettings
            | x    -> x
        (setHttpHeader "Content-Type" "application/json"
        >=> setBodyAsString (serializeJson settings dataObj)) next ctx

Then in your ASP.NET Core startup you can optionally decide to configure custom JsonSerializerSettings:

let configureServices (services : IServiceCollection) =
    services.AddSingleton<JsonSerializerSettings>(
        JsonSerializerSettings(ContractResolver = CamelCasePropertyNamesContractResolver())) |> ignore

With this we could probably also get rid of customJson and all overloads in the HttpContext extension methods.

I think this would mean that for those who want to use the default settings they will not have to do anything extra, for those who need customization will be able to configure it once in startup where other config can be found as well and probably it's also the correct ASP.NET Core idiomatic way of doing it.

Interested to hear more opinions on this...

[alfonsogarciacaro]

Thanks for your reply @dustinmoris. I fully agree, given that Giraffe integrates with ASP.NET Core DI model this makes total sense. My only doubt is if this should be only for the JsonSerializeSettings (that would be enough for Fable's case) or the full JSON serialization. Is someone out there not using Newtonsoft.Json?

[dustinmoris]

Yeah if we could configure the entire JSON serialization this way that would be perfect. What would be the common interface between Newtonsoft and other JSON serializers? This would probably require us to implement our own interface which then can be used by apps to override the default Newtonsoft implementation. It's a valid point and worth considering before making any breaking changes!

[dustinmoris]

Some pseudo code to demonstrate how we could entirely swap Newtonsoft if someone would want to use a different serializer in the Giraffe http handlers:

type IGiraffeJsonSerializer =
    abstract member Serialize : obj -> string
    abstract member Deserialize<'T> : string -> 'T

type NewtonsoftGiraffeJsonSerializer() =
    let settings = JsonSerializerSettings(ContractResolver = CamelCasePropertyNamesContractResolver())
    interface IGiraffeJsonSerializer with
        member __.Serialize (dataObj : obj)       = JsonConvert.SerializeObject(dataObj, settings)
        member __.Deserialize<'T> (json : string) = JsonConvert.DeserializeObject<'T>(json, settings)

let configureServices (services : IServiceCollection) =
    services.AddTransient<IGiraffeJsonSerializer, MyCustomSerializer>()

[dustinmoris]

Sorry, one last thought - I could implement both options together. For example having an abstract type IGiraffeJsonSerializer which can be used to completely swap out Newtonsoft if someone wants to use a different JSON serializer everywhere in Giraffe and on top of that I can implement the NewtonsoftGiraffeJsonSerializer as seen above, which then can try to load the JsonSerializerSettings from the services. With that I could register the Newtonsoft serializer by default in the Giraffe middleware and then users can either swap out the entire serializer or just the settings by whichever type they register in their application.

[gerardtoconnor]

Although DI is a solution I think we should look at it as a last resort as it is frankly bad programming as we are making compile-time decisions at run-time, using a boxing dictionary & lookup everytime we want to do a json operation. All this seems to be just to avoid having to create our own JsonConverter mutable state but we are just indirectly doing it in a less efficient manner using DI here.

I know it's easy to criticize and alternatives should be offered so perhaps this is a chance to look at default server builder function that can check/inject any mutable states like the JsonConverters
```f#
let GiraffeServer( handler:HttpHandler ,
hostBuilder:IWebHostBuilder -> IWebHostBuilder,
appBuilder : IApplicationBuilder -> unit ,
serviceBuilder : IServiceCollection -> unit
jsonConverter : JsonConverter ) =

let errorHandler (ex : Exception) (logger : ILogger) =
    logger.LogError(EventId(0), ex, "An unhandled exception has occurred while executing the request.")
    (clearResponse >=> setStatusCode 500 >=> text ex.Message) 

HttpHandlers.JsonConverter <- jsonConverter // set converter

let whb : IWebHostBuilder = 
    WebHostBuilder()
        .UseKestrel()    
        .UseContentRoot(Directory.GetCurrentDirectory())
        .UseWebRoot(Directory.GetCurrentDirectory() + @"\wwwroot\")
        .Configure(fun app ->
            app.UseGiraffe handler
            app.UseGiraffeErrorHandler errorHandler |> ignore                                             
            appBuilder app)
        .ConfigureServices(Action<IServiceCollection> serviceBuilder)
        .ConfigureLogging(Action<ILoggerFactory> configureLogging)
        |> hostBuilder            

whb.Build()
    .Run()

// overload with implicit default jsonconverter
let GiraffeServer( handler, hostBuilder , appBuilder, serviceBuilder ) = GiraffeServer( handler, hostBuilder , appBuilder, serviceBuilder , defaultJsonConverter )

All defaults would be set for user so they wouldnt need to worry about kestrel, error handler, content root etc.

And usage would be:
```f#
let main argv =                                

    GiraffeServer(
        WebApi.webApi,  // HttpHandler
        fun host ->     // IWebHostBuilder
            host.UseUrls("http://*:5000")
        ,
        fun app ->      // IAppBuilder
            app.UseStaticFiles() |> ignore
            app.UseWebpackDevMiddleware(WebpackDevMiddlewareOptions(HotModuleReplacement = true))
        ,    
        fun services -> // IServiceCollection
            services.AddNodeServices()
            services.AddResponseCaching() |> ignore
    )

jsonConverter is now an overload of server build function and we have full explicit control of mutability.

[gerardtoconnor]

In the above case, we could tweak all configurable settings to DUs such that
```f#
type ServerConfig =
| WebHost of IWebHostBuilder -> IWebHostBuilder
| AppBuilder of IApplicationBuilder -> unit
| JsonConv of JsonConverter
..

let GiraffeServer( handler:HttpHandler , configs : ServerConfig list ) = ...
```
And at startup server builder can ensure all states set and configured from ServerConfig list, setting defaults where overrides not provided.

Just thinking of multiple ways we can fix as I dont think Json Converters will be the only thing people will end up wanting to override.

[dustinmoris]

Hey, that's an interesting approach! Just got a few questions to clarify:

Although DI is a solution I think we should look at it as a last resort as it is frankly bad programming as we are making compile-time decisions at run-time, using a boxing dictionary & lookup everytime we want to do a json operation.

Not sure if we really make compile-time decisions at run-time? DI frameworks don't have to box/unbox since they use generics right? I would expect a simple dictionary lookup of a singleton to be as fast as it can be. I am not an expert with how things work low level, but I would have thought that a lookup of a dictionary item would be the same as the lookup of a mutable variable, where both are simple pointers to where the actual object is?

All this seems to be just to avoid having to create our own JsonConverter mutable state

Agree, I thought I would have preferred a dictionary lookup over a mutable variable. In this case I thought a mutable variable would be a worse practise, as it is too much for what we really need. A user wouldn't want to change the serializer during run time, they would want to configure it once at startup and then never have to touch it again, therefore I thought a dictionary would be ideal, whereas a mutable variable would be "too much".

This is a genuine question, but does the GiraffeServer add any level of protection over the mutable JsonSerializerSettings object? With the above example, why could a user not just simply do HttpHandlers.JsonConverter <- jsonConverter somewhere in the app?

>

type ServerConfig =
| WebHost of IWebHostBuilder -> IWebHostBuilder
| AppBuilder of  IApplicationBuilder -> unit
| JsonConv of JsonConverter

With additional wrappers/layers like this I think we need to be careful that we don't end up in a scenario where we will always play catch up with new things which users would want to do or whenever things slightly change in ASP.NET Core. Also this would make Giraffe significantly different from how other ASP.NET Core applications get configured which means a lot of current ASP.NET Core documentation which is out there would not directly apply to Giraffe which would be a bit of a shame, but not a big deal if we think it pays off otherwise.

[vasily-kirichenko]

I like the straightforward DI approach (injecting IGiraffeJsonSerializer). It’s idiomatic, simple, transparent, and it will work as asp.net core is evolving.

[forki]

but I would have thought that a lookup of a dictionary item would be the
same as the lookup of a mutable variable,

Dictionary lookup is pretty fast in .NET, but at least couple of orders of
magnetic slower than just using a variable. And regarding boxing. No
generics don't help, because you need to put everything into the dictionary
as object. That is the common base type. So every extracting needs to
unbox. That's slowest part. But tbf this is only one boxing per request and
injected type. That's not the end of the world.

Am 17.12.2017 08:44 schrieb "Vasily Kirichenko" [e-mail]:

I like the straightforward DI approach (injecting IGiraffeJsonSerializer).
It’s idiomatic, simple, transparent, and it will work as asp.net core is
evolving.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/giraffe-fsharp/Giraffe/issues/178#issuecomment-352237986,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADgNB3mP0vgbBySNjLVBu5jtyLdagigks5tBMZ6gaJpZM4REY_x
.

[dustinmoris]

My understanding was that because of Generics we didn't have to store things as objects anymore, that was a big improvement in .NET 2.0. Just googled it to refresh my mind and came across this on SO.

[forki]

Yes that's the idea of generics. To have something like List of T. And that
works without boxing. But think about the DI stuff. It needs to be
Dictionary of (string * all the types that are injectable). And that's
making it obj. And boxing comes back.

Am 17.12.2017 09:18 schrieb "Dustin Moris Gorski" <[email protected]

:

My understanding was that because of Generics we didn't have to store
things as objects anymore, that was a big improvement in .NET 2.0. Just
googled it to refresh my mind and came across this on SO
https://stackoverflow.com/questions/4403055/boxing-and-unboxing-with-generics
.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/giraffe-fsharp/Giraffe/issues/178#issuecomment-352239579,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADgNOqO-UNrwbwJrvsxti_4WK9zhDq5ks5tBM5PgaJpZM4REY_x
.

[forki]

Btw if you use a common interface IInjectable or something then you don't need boxing and you can just use up and downcasts. But then again everything needs to implement that interface.

[gerardtoconnor]

Not sure if we really make compile-time decisions at run-time? DI frameworks don't have to box/unbox since they use generics right? I would expect a simple dictionary lookup of a singleton to be as fast as it can be. I am not an expert with how things work low level, but I would have thought that a lookup of a dictionary item would be the same as the lookup of a mutable variable, where both are simple pointers to where the actual object is?

I'm am not an expert on low-level, but am familiar with JIT , IL and how F# is compiled and executed and represented/run as asm. Basically, as Steffen has described, DI is just a Dictionary of

Dictionary<Type,obj>

And when you add a dependency you are down-casting (cheap) your interfaced type into an obj, and when you look it up later, it is pulling out the obj and up-casting (expensive) it back to your interface. Normally generics avoid casting as type is generic through entire runtime branch in JIT but DI needs a persisted common structure to store all kinds of Interfaces so it hacks everything to obj.

If we know the type and instance, why not invoke directly rather then packing it into a casting data structure and having to resolve it every time its used at runtime? its adding bloat/abstraction for no good reason, it's why I hate DI/IoC, as well as over abstraction.

I agree and appreciate that in my example above I have not thought out a proper restricted access pattern for JsonConverter state, will have a think, but I would also ask if changing the JsonConverter later on is such an issue, even if makes little sense? On our server build we can ensure its set to something and it's only intentional setting to null later that will cause an issue (and often f# will give error trying to set an obj to null anyway).

Just giving feedback on performance & architecture considerations for introducing what will be a heavily used pattern that is less optimal for no great reason.

I think frameworks/platforms need to be performance and efficiency aware so user is confident that the abstraction is minimal and they can butcher performance & abstraction as they see fit.

I appreciate what you mean on DU config bits and having to keep up with Asp.net core but I was only brainstorming given api is now getting mature and changing less and less as of 2.0, and adding new features/services through IWebHostBuilder , IApplicationBuilder & IServicesCollection should capture all/most updates to asp.net core. One of the reasons I have also suggested this default server builder, in what ever form we agree is that many issues have been opened on questions on how to configure startup for simple standard things that should be default or more easily setup.

[gerardtoconnor]

And one small thing on configuring a generic interface, not to be stickler further but most of the usage of json serialisation should be between streams and types, not string and types as the string is an unneeded interim data structure that is high on allocation, potentially hitting LOH, and slower and less efficient as a result. We currently use Stream json binding to great effect.

[forki]

https://github.com/giraffe-fsharp/Giraffe/pull/179 is another way to solve it

[dustinmoris]

Re performance of DI frameworks:

As I said before I am not a low level expert, but unless someone can really prove that a DI framework has a real perf impact I highly doubt that is the case. DI frameworks ar defacto the standard in any OO application and many of such web apps which I have written were extremely high performing and I never hit a perf issue due to DI. I believe that even a plain Kestrel app would use DI and we know it's extremely fast.

When it comes to boxing and unboxing, I thought boxing means if I have to take a value from the stack and put it on the heap because I want to store a bunch of ints and strings in a single array and therefore need to box an int into an object. The only reason why boxing/unboxing is considered expensive is not the act of up/down casting but because storing more stuff on the heap adds up more work for the GC afterwards, otherwise we wouldn't care as far as I know. However, when we have an array of 'T, where 'T is a reference type (most cases when used DI) then the objects are already stored on the heap, so the cast to object doesn't add any more overheard if I am correct.

But even saying that, most DI frameworks optimise for all these things. For instance the most common scenario is not Dictionary<Type,obj> but rather Dictionary<Type, Type> where the first type is an abstract type and the second the concrete type. There is no casting even involved yet and the object gets created transient by default, so I believe the real perf impact is extremely low and really negligible.

I think this SO answer says it nicely:

You should avoid worrying about the performance implications of specific language features unless you have specific evidence (measurements) that they are actually causing a problem.
Your primary concerns should be the correctness of the code and it's maintainability.

[dustinmoris]

Ok, there were many things being mentioned here and also being discussed in #179 and I am keen on agreeing on a general strategy for configuration before this issue or #179 can be completed.

There are a few competing ideas and while I agree that there's no silver bullet for all I would like to avoid at all events to have multiple different ways of configuring different parts of Giraffe and creating a mess for users.

Currently I see the following options:

Using ASP.NET Core's DI

As discussed in this thread DI would be a good way of setting and retrieving configuration.

• Pros
  
  
  
  • Natural fit in ASP.NET Core
  
  
  • Familiarity
  
  
  • Users have full control of scope (transient, singleton, etc.)
  
  
  • Users can create more elaborate functions such as registering a factory method which will initialise a setting (e.g. JsonConverter) based on runtime rules (blue/green testing, feature flags, etc.)
  
  
  • Users have full control of which DI framework to use. ASP.NET Core allows an user to swap in any DI framework they wish, which means features, performance and other aspects of DI frameworks are flexible
  
  
  • Unit testing friendly
  
  
  • Configuration of services is found in a single place in ASP.NET Core. New users of an application would always know where to look for settings.
  
  
• Cons
  
  
  
  • Not exactly F# idiomatic
  
  
  • No easy discovery of all settings through Intellisense
  
  

Mutable globals

In #179 we've been talking about mutable globals. A user could change a setting by simply setting a global variable anywhere in an application. We've further discussed that if we would go with globals, we would set default values and protect the mutables behind functions, so that it is impossible to ever set null to variable. In such a case I'd also want to place all mutable variables into a single Giraffe.Config object, which would allow a user to browse through all possible settings via Intellisense.

• Pros
  
  
  
  • null would be impossible
  
  
  • Settings can be discovered through Intellisense
  
  
• Cons
  
  
  
  • Setting config values could be scattered across an application
  
  
  • Problems with unit testing. Setting a global variable would affect all unit tests. I have seen many problems with unit test runners which run large amounts of tests in parallel which resulted in intermittent failures, because one test set a global to A, whilst another test was expecting it to be B.
  
  
  • More difficult if users have special requirements. For example if a user would want to pick the JsonConverter based on runtime decisions (blue/green testing) it would be difficult to do this with a global variable which doesn't allow factory methods by default. We could end up having to revisit the config problem again and again and hack it together so that every user which might open an issue one day can accommodate their personal use case.
  
  
  • Users in Giraffe would still have to deal with DI configuration for non-Giraffe features (auth, security, DBs, etc.) and therefore be confronted with two config systems
  
  

Adding settings to HttpHandler

Another option would be to have a Giraffe.Config object similar to what I was thinking for mutable globals, except that we would add it to the signature of an HttpHandler.

Example:

let someHandler : HttpHandler =
    fun ctx config next ->
        // Do stuff

• Pros
  
  
  
  • Idiomatic F#
  
  
  • Unit test friendly
  
  
  • null would be impossible
  
  
  • Settings can be discovered through Intellisense
  
  
  • Settings would be found in one place where the GiraffeMiddleware gets initialised
  
  
• Cons
  
  
  
  • Same difficulty with special requirements if a user wants to do something more exotic which is not easily possible through a simple variable assignment (e.g. factory methods)
  
  
  • Users in Giraffe would still have to deal with DI configuration for non-Giraffe features (auth, security, DBs, etc.) and therefore be confronted with two config systems
  
  

--
To me this is a big change, because whichever way we go we set a precedence of how config should be done in an Giraffe app and I want to make an informed an objective decision based on real pros/cons rather than a rushed impulsive one. With that I tried to illustrate some pros/cons of the current options and see if someone has more to add and it's as always up for discussion.

[forki]

I don't think Kestrel uses DI. Why would it? And tbh I'm not worried about perf in DI.

Problems with unit testing. Setting a global variable would affect all unit tests. I have seen many problems with unit test runners which run large amounts of tests in parallel which resulted in intermittent failures, because one test set a global to A, whilst another test was expecting it to be B.

that's excatly what happens with DI if don't take care of the scope. ;-) No difference at all seriously.

[dustinmoris]

@forki

that's excatly what happens with DI if don't take care of the scope. ;-) No difference at all seriously.

See this example for the difference:

[<Fact>]
let ``Test A`` () =
    let host =
        WebHostBuilder()
            .Configure(Action<IApplicationBuilder> A.configureApp)
            .ConfigureServices(Action<IServiceCollection> A.configureServices)
    use server = new TestServer(host)
    use client = server.CreateClient()

    get client "/" |> shouldEqual "A"

[<Fact>]
let ``Test B`` () =
    let host =
        WebHostBuilder()
            .Configure(Action<IApplicationBuilder> B.configureApp)
            .ConfigureServices(Action<IServiceCollection> B.configureServices)
    use server = new TestServer(host)
    use client = server.CreateClient()

    get client "/" |> shouldEqual "B"

In this example above it is easily possible to have both tests completely being independent and isolated from other tests. In test A we use te DI config from A.configureServices and in test B we use B.onfigureServices. If my default test runner runs both tests in parallel (which is what most people want to get a quicker feedback loop) then I would always have a reliable result.

On the other hand, with globals this wouldn't be possible AFAIK:

[<Fact>]
let ``Test A`` () =
    Giraffe.Config.SomeSetting = "A"
    let host =
        WebHostBuilder()
            .Configure(Action<IApplicationBuilder> A.configureApp)
    use server = new TestServer(host)
    use client = server.CreateClient()

    get client "/" |> shouldEqual "A"

[<Fact>]
let ``Test B`` () =
    Giraffe.Config.SomeSetting = "B"
    let host =
        WebHostBuilder()
            .Configure(Action<IApplicationBuilder> B.configureApp)    use server = new TestServer(host)
    use client = server.CreateClient()

    get client "/" |> shouldEqual "B"

[forki]

ah now I see what you mean. ;-) of course you would not want to make it that global ;-)

[<Fact>]
let ``Test A`` () =
    let setting = "A"
    let host =
        WebHostBuilder()
            .Configure(Action<IApplicationBuilder> A.configureApp)
    use server = new TestServer(host,setting)
    use client = server.CreateClient()

    get client "/" |> shouldEqual "A"

[<Fact>]
let ``Test B`` () =
    let setting = "B"
    let host =
        WebHostBuilder()
            .Configure(Action<IApplicationBuilder> B.configureApp)
    use server = new TestServer(host,setting)
    use client = server.CreateClient()

    get client "/" |> shouldEqual "B"

[forki]

[<Fact>]
let ``Test DefaultSettings`` () =
    let host =
        WebHostBuilder()
            .Configure(Action<IApplicationBuilder> B.configureApp)
    use server = new TestServer(host)
    use client = server.CreateClient()

    get client "/" |> shouldEqual "C"

[dustinmoris]

Ok, but how does the ASP.NET TestServer all of a sudden know about setting? If we have for example let mutable SomeSetting = DefaultSetting then it is global in that entire test assembly and not just for the specific unit test.

I think your example is more similar to what I described by extending the HttpHandler to expose a config variable, which we could pass in when initialising the Giraffe middleware.

Example:

let config =
    {
        JsonConverter = SomeConverter
        // more stuff
    }

let configureApp (app : IApplicationBuilder) =
    app.UseGiraffe webApp config

[forki]

you need to control the scope, that is definetly true. and you don't want to set it global global. I hope I can find time later today to refactor my PR to reperesent that a bit better.

[dustinmoris]

@forki cool, thanks and looking forward to seeing it

[gerardtoconnor]

For instance the most common scenario is not Dictionary but rather Dictionary

This transient DI calls concrete type constructor EVERY time, so on every single request we would be constructing & allocating an eg. JsonConstructor State obj which would certainly destroy performance, acts as a cached result to prevent this. no one said boxing, we said casting, and down-casting is relatively expensive in JIT (obj -> 'T) vs up-casting (as validations required), but not near as bad as boxing.

I think this SO answer says it nicely

SO is an open forum where many people with opinions chime in pretending to be fact, 70% of answers on SO are non-sense, just lucky that the correct answers tend to be up voted so still useful, the person your quoting is just providing an opinion, Fact is, it effects performance, question is, is it material for your use case as most devs don't need performance and it will be an immaterial component of their application.

Ignoring performance is a slippery slope, we should always aim for best performance while keeping public facing api as simple & elegant as possible, that's the compromise worth making.

I appreciate your concerns on a global config state for testing etc, on that basis, for injecting custom functionality, I would be more inclined to revert back to the method we are already using eg. for JsonConverter, allow user to create their custom myJson handler by just adding a customised converter to creator function. That way everything is quite clear and no unneeded middleman (DI).

Given you are unconvinced on DI vs direct invocation, I will try put together a benchmark test in next few days to show difference, will be small as expected but these things add up on a high performance web server like asp.net core where we handle 100k req/sec

In principle "config / settings" should not be used at runtime, settings are called at startup/lazily to provide config/settings for construction of a state obj, like if we look at settings in a appsettings.json etc. Configured concrete instances of functions/classes are compile-time known entities so once again, for likes of JsonConvert, I prefer user creating their own custom handler and we provide a "handler creator" function.

Just offering performance considerations, if you really feel DI is way to go for "config" then ok, I just think it is wasteful abstraction. Also, I would rather not see another config variable introduced into HttpHandler as two is painful enough and unlike next we do have alt options available.

[dustinmoris]

@gerardtoconnor Thanks for your input. I think being able to see the material perf impact of DI vs. globals would be interesting indeed. We are not creating a new JsonConverter a million times per second in a loop, one request would create exactly one object IF the route ends up returning a JSON response. A typical app creates a thousand of objects along the way, so creating +-1 really shouldn't make a difference.

Here is what I struggle to understand, MS is obviously extremely keen on getting Kestrel and MVC extremely fast. They have mentioned multiple times that they don't just want to create a fast web server, but they want to compete with the fastest servers from all other languages, being somewhere at the top and in many scenarios they are already at the top. Even though perf is a big consideration they still opted to introduce and build the entire stack on top of DI (it comes with plain ASP.NET Core, not even MVC), so clearly DI can't have a real material impact based on that thinking? I mean ASP.NET Core is more low level than Giraffe, so if DI is performant enough a level lower than us, why should it be too slow for us then?

[forki]

Kestrel <> ASP.NET Core <> MVC

But that said: I think if you put it into the DI then it's at least consistent.
I experimented with the 3rd approach (new cfg param in the handlers) it actually felt really nice. But yes it's a big change with impact everywhere.

[dustinmoris]

@forki Yes, I am only trying to get clarity on the actual implications of every option. Personally I think DI has a lot of pros, which is not surprising when I think it is widely used and considered a good standard. Right after DI I think I'd prefer the third param option and last the globals one. I'm not worried about breaking changes if the benefit outweighs the one time pain which some users will have of adapting their app to the new format.

[gerardtoconnor]

I get what you are saying, thing is 1000 design decisions that add only 0.5% performance slowdown, in total, lead to a system 500% slower so its maintaining best perf throughout while maintaining the elegant public api.

If you look at all the perf tests asp.net core do, plain text, json, etc, none use DI, they all use barebone server components so they know DI comes with compromise but where it is used, for things like DB/service calls, it does start to become immaterial, for a simple action of writing json it is material, if users have advanced cached writes on server, with no DB etc bottlenecks, starts to become material.

On passing config with handler, (in our common use case) how does that help user parse json using their preferred JsonConverter ... if anything it makes process more complicated but I may have mis-understood?

[forki]

Considered a good standard.

Yes from some part of the development community. Not so much from another
part ;-)
But since you already made the decision to set giraffe on top of asp.net
core and not kestrel this decision is basically already settled.
I'm not in favor of that, but that does not matter. All things together I
think the argument for consistency wins.

Am 19.12.2017 12:58 schrieb "Dustin Moris Gorski" <[email protected]

:

@forki https://github.com/forki Yes, I am only trying to get clarity on
the actual implications of every option. Personally I think DI has a lot of
pros, which is not surprising when I think it is widely used and considered
a good standard. Right after DI I think I'd prefer the third param option
and last the globals one. I'm not worried about breaking changes if the
benefit outweighs the one time pain which some users will have of adapting
their app to the new format.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/giraffe-fsharp/Giraffe/issues/178#issuecomment-352729243,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADgNIgrc_EdfUm6UQGjWuj1vqpqC3JNks5tB6TbgaJpZM4REY_x
.

[dustinmoris]

@gerardtoconnor

On passing config with handler, (in our common use case) how does that help user parse json using their preferred JsonConverter ... if anything it makes process more complicated but I may have mis-understood?

let json (dataObj : obj) : HttpHandler =
    fun ctx config next ->
        let settings = config.Serialization.Json.Settings
        (setHttpHeader "Content-Type" "application/json"
        >=> setBodyAsString (serializeJson settings dataObj)) ctx config next

[gerardtoconnor]

Seems painful to break just to allow json to have this option.

Out of genuine curiosity, why are we no longer happy with:
f# let json = customJson MyJsonSettings //overwrite json def through application

[dustinmoris]

Seems painful to break just to allow json to have this option.

Agree, this is why I'd prefer DI overall if you ask me. Also it isn't just json. Json is today, tomorrow it will be XML, then for @forki it is the default unacceptable handler during negotiate, for someone else it will be the list of supported mime types in negotiate, etc. The more feature rich Giraffe will become the more requirements will grow where people will need to customise central parts of it. I am looking for a future proof way of allowing people to make any sort of changes in the long term without having to constantly break existing functions or hack new things into the framework in order to accommodate these type of customisation requests.

Out of genuine curiosity, why are we no longer happy with let json = customJson MyJsonSettings //overwrite json def through application

This still uses Newtonsoft for serialisation. Today this is good enough for Fable users, but again, it's only a matter of time until the next person will come and say that Newtonsoft is not performant enough and they want to use a faster serializer and then will send PRs for hacking json, negotiate, BindJsonAsync, etc. to accommodate their use case.

I think the customJson was a nice try of me, but I have to agree that it is not flexible enough for the long run and it wasn't enough for Fable already, otherwise this issue wouldn't have been created in the first place.

[forki]

I think we are fine with json. The problem is that negioate is used
everywhere in the request errors and I can't customize it

Am 19.12.2017 13:13 schrieb "Gerard" [e-mail]:

Seems painful to break just to allow json to have this option.

Out of genuine curiosity, why are we no longer happy with:

let json = customJson MyJsonSettings //overwrite json def through application

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/giraffe-fsharp/Giraffe/issues/178#issuecomment-352732437,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADgNET0jTGCW5KTjaq_5LD9URv1Yq4rks5tB6h-gaJpZM4REY_x
.

[alfonsogarciacaro]

This is a very interesting discussion, thanks a lot for your comments! I agree performance is a very important consideration, though when we had to take similar decisions in Fable in most of the cases usability proved to be more important than raw performance (particularly when it's difficult to measure). That's why I was favoring the DI approach for consistence with ASP.NET Core, though I'm starting to see some merits now to the parameter approach.

I'm thinking that in the SAFE bookstore template we are already passing some functions to the handlers for DB access and this is probably a need of many users. So we could think of a way to merge Giraffe and user configuration. This would probably benefit tests as well:

type IGiraffeConfig =
  abstract ToJson: obj -> string

// The signature for routes could be something as follows
// Handlers accept any type implementing the IGiraffeConfig interface
let route<'Config when 'Config :> IGiraffeConfig>
  (path: string)
  (handlers: ('Config -> HttpFunc -> HttpContext -> HttpFuncResult) list) = ...

// In their apps, users create their config object implementing the interface
// Thanks to this they're aware of which parts of Giraffe are customizable
type MyConfig() =
  member __.AskDB() = ...
  interface IGiraffeConfig with   
    member __.ToJson(o) = ... // We can use a Giraffe default here

let myHandler (cfg: MyConfig) (next: HttpFunc) (ctx: HttpContext) =
  // I can access config fields here
  ...

let startApp() =
  let config = MyConfig()
  // let config = Giraffe.defaultConfig // Enough for hello world apps
  router config Giraffe.notFound [
      route "/" myHandler
  ]

[dustinmoris]

Oh good point, if we'd replace a static config type with mutables with an interface in the 3rd parameter option then we'd basically eliminate all downsides from that option, because now a user could implement any desired complexity into the custom type.

Example:

type IGiraffeConfig =
    // using this just for this sample, the actual signature of the json stuff could be different later
    abstract ToJson: obj -> string

type SimplePerformantConfig() =
    let serializer = SomeSerializer()
    interface IGiraffeConfig with
        member __.ToJson (o : obj) = serializer.ToJson o

type ComplexConfig (settingsFromConfigFile) =
    // Both serializers are singletons in this example
    let serializerA = SerializerA()
    let serializerB = SerializerB()

    interface IGiraffeConfig with
        member __.ToJson (o : obj) =
            if settingsFromConfigFile.EnableNewFeature = true then
                // A user could also choose to create transient serializers:
                // let serializerA = new SerializerA()
                serializerA.ToJson o
            else serializerB.ToJson o

EDIT: Because we talk about all these variations I think we can basically call the first IoC option the service locater (that's what it essentially is), the second one is globals and the third is basically IoC through method injection if we are honest.

The only downside of the last option (=method injection) is that we have to bundle all possible configurations into an ever growing interface, because we wouldn't want to constantly add more parameters to the HttpHandler signature. The service locator option would allow us to create smaller more concise interfaces for different parts of Giraffe (negotiation, serialisation, etc.).

[gerardtoconnor]

BenchmarkDotNet=v0.10.10, OS=Windows 10 Redstone 3 [1709, Fall Creators Update] (10.0.16299.125)
Processor=Intel Core i7-4770K CPU 3.50GHz (Haswell), ProcessorCount=8
Frequency=3415984 Hz, Resolution=292.7414 ns, Timer=TSC
.NET Core SDK=2.1.2
  [Host]     : .NET Core 2.0.3 (Framework 4.6.25815.02), 64bit RyuJIT
  DefaultJob : .NET Core 2.0.3 (Framework 4.6.25815.02), 64bit RyuJIT

| Method | Mean | Error | StdDev | Scaled | ScaledSD | Gen 0 | Allocated |
|------------- |-----------:|----------:|----------:|-------:|---------:|-------:|----------:|
| Direct | 0.1960 ns | 0.0121 ns | 0.0107 ns | 1.00 | 0.00 | - | 0 B |
| DepInjection | 67.8828 ns | 1.3381 ns | 1.2516 ns | 347.34 | 19.27 | 0.0056 | 24 B |

Just getting back on benchmarking, as expected direct invocation way faster x347, but using crude benchmarks. The bigger question is if it is material.

doing a basic rough calc, If we take it that giraffe can process +100k req/sec, that is about 10k ns spent on each req, direct invocation adds 0.002% to the execution of a request, while DI adds 0.68% ... it is small in isolation but adds up if not keeping in check.
```f#
type DummyClass() = member val Value = 0 with get , set
[]
type MapCastTest() =
let cls = DummyClass()
let di =
dict [
typeof,cls :> obj
typeof,MapTest :> obj
typeof,RemoveTest :> obj
]

[<Benchmark(Baseline=true)>]
member __.Direct () =
        cls.Value <- cls.Value + 1

[<Benchmark>]
member __.DepInjection () =
        let temp = di.[typeof<DummyClass>] :?> DummyClass
        temp.Value <- temp.Value + 1

```

[alfonsogarciacaro]

Thanks for the benchmarks @gerardtoconnor! This helps a lot to understand the actual difference between using DI or direct invocation :+1:

@dustinmoris I was also thinking that too big an interface can be cumbersome to implement. The only solution I can think of right now is to make only a single method available and then extend default options as FAKE APIs usually do:

type IGiraffeConfigProvider =
    abstract GiraffeConfig: IGiraffeConfig

type SimplePerformantConfig() =
    let config = { Giraffe.defaultConfig with JsonSerializer = mySerializer }
    interface IGiraffeConfigProvider with
        member __.GiraffeConfig = config

It's true that int this case the interface seems to be redundant as you could pass the object directly. But it still allows users to make decisions that fit their project needs: cache config or not (to check runtime parameters), add own methods, etc.

[dustinmoris]

I've started a feature branch where I tested a first version of using DI.

The handler customJson and all overloads in the Giraffe.HttpContextExtensions module which accepted JsonSerializerSettings have been deleted.

Instead a user can register a different IJsonSerializer in the startup code now:

type MyOwnJsonSerializer() =
    interface IJsonSerializer with
        member __.Serialize...
        member __.Deserialize...
        ...

let configureApp (app : IApplicationBuilder) =
    app.UseGiraffeErrorHandler(errorHandler).UseGiraffe webApp

let configureServices (services : IServiceCollection) =
    services.AddGiraffe().AddSingleton<IJsonSerializer, MyOwnJsonSerializer>()

[<EntryPoint>]
let main _ =
    WebHost.CreateDefaultBuilder()
        .Configure(Action<IApplicationBuilder> configureApp)
        .ConfigureServices(configureServices)
        .Build()
        .Run()
    0

If a user wants to keep using Newtonsoft, but with different settings then they can simply override the default settings like:

let configureServices (services : IServiceCollection) =
    let customSettings = new JsonSerializerSettings()
    services.AddGiraffe().AddSingleton<JsonSerializerSettings>(customSettings)

You can start testing this version of Giraffe by either downloading the NuGet package from the build artifacts or by registering the private project feed in your NuGet/Paket settings.

[alfonsogarciacaro]

Great work @dustinmoris! Yes, the more I think about this the more convinced I am DI is the way to go. Apparently the two central concepts of ASP.NET Core are middleware and dependency injection. And given that Giraffe replicates the middleware approach, it would be unintuitive for users if it moves away from DI (specially when you still have to use DI nonetheless to configure the logger and other services). We also talked above about scoping and ASP.NET Core already provides singleton, transient and scoped dependencies, users will likely expect to be able to apply these with Giraffe too.

For example, in the case of the Fable.JsonConverter it's better to use a singleton as it creates a cache to prevent too many reflection calls.

[vasily-kirichenko]

@dustinmoris looks really intuitive, simple and idiomatic. Great job!

[dustinmoris]

Just pushed another change to the feature branch which can be used for evaluation. I added an INegotiationConfig interface which can be used to customise the default negotiate handler as well.

Also someone more clever than me pointed out that we shouldn't register a type in Giraffe which doesn't belong to Giraffe like I initially did with the JsonSerializerSettings. This could cause conflicts or problems with other middleware which might also try to register the same type. In Giraffe we should only register types which are owned by Giraffe, so the preferred way to customize the Json serializer would be:

Keep JSON.NET, but change settings:

let configureServices (services : IServiceCollection) =
    let customSettings = new JsonSerializerSettings()
    services.AddGiraffe().AddSingleton<IJsonSerializer>(NewtonsoftJsonSerializer(customSettings))

Complete different serializer:

let configureServices (services : IServiceCollection) =
    services.AddGiraffe().AddSingleton<IJsonSerializer, MyOwnJsonSerializer>()

[dustinmoris]

This went out today with Giraffe 1.0.0. See the Giraffe Documentation for more information.