[Discussion] Removing the contextual remoting handler

[Zaid-Ajaj]

TL;DR

Remove the ability to create contextual remoting protocols to prevent people from writing untestable/hard to unit test code.

Description

We have added the ability to access and read http context data to make Fable.Remoting more flexible, the way we do it is by defining generic protocols, an implementation of the protocol gives a concrete HttpContext as the type parameter, this is explained in Accessing Request Context. You end up writing protocols of this shape:

type IMusicStore<'Context> = {
    /// return favorite albums of current logged in user
    favoriteAlbums : 'Context -> Async<Album list>
}

However, for a while now I am thinking that this should be removed and keeping the good old non-generic interfaces because it hurts the testability of the concrete implementations: there is no simple way of mocking HttpContext (in Suave, I think you must build it from scratch for testing) and I think people using remoting shouldn't be bothered with HttpContext at all if they are testing their implementations and should only be testing their functions

Alternative

The alternative already "implemented" and explained in Logging - Section: Without extending protocol definition

That is by contructing the remoting WebPart from the context functions that give access to the context from outside the implementation.

The following example is provided in the docs (which itself is quite rare I think) is only testable at the HTTP level, so you have to actually send and requests and get responses to assert results (which in my opinion, shouldn't be the case)

type IMusicStore<'Context> = {
    /// return favorite albums of current logged in user
    favoriteAlbums : 'Context -> Async<Album list>
}

let musicStore : IMusicStore<HttpContext> = {
    // return favorite albums of current logged-in user
    favoriteAlbums = fun ctx -> async {
        // get the authorization header, if any
        let authHeader = 
            ctx.request.headers
            |> List.tryFind (fun (key, _) -> key = "authorization")
            |> Option.map snd 

        match Security.extractUser authHeader with
        | None -> return [] // no albums 
        | Some user -> 
            let! favoriteAlbums = Database.favoriteAlbums user
            return favoriteAlbums
    }
}

// WebPart
let webApp = remoting musicStore {()}

Becomes

type SecurityToken = SecurityToken of string 

type IMusicStore = {
    /// return favorite albums of current logged in user
    favoriteAlbums : Async<Album list>
}

Then the implementation becomes the following

// give me a database and a security token and get a music store 
// this is easily testable: only mock database operations and provide a security token
let createMusicStore (db: IDatabase) 
                     (security: ISecurityOps)
                     (securityToken: Option<SecurityToken>) : IMusicStore =  
    let musicStore : IMusicStore = {
        // return favorite albums of current logged-in user
        favoriteAlbums = async {
            match security.extractUser securityToken with
            | None -> return [] // no albums 
            | Some user -> return! db.favoriteAlbums user
        }
    }

    musicStore

// a web app needs a database 
// IDatabase -> ISecurityOps -> WebPart
let createWebApp (db: IDatabase) (security: ISecurityOps) = context <| fun ctx ->
    // extract token from the context
    let securityToken = 
       ctx.request.headers
        |> List.tryFind (fun (key, _) -> key = "authorization")
        |> Option.map (snd >>  SecurityToken)
    // create the store
    let musicStore = createMusicStore db security securityToken 
    // expose as web part
    remoting musicStore {()}

// actual webApp, the composition root of the app
let webApp : WebPart = 
   let db = Database.createFromProductionConfig()
   let security = Security.defaultImplementation() 
   createWebApp db security  

Your opinions please!

What do you guys think? did I miss anything or do you think there are examples where the contextual remoting handler is better than using the context function from suave or giraffe? @Nhowka @irium

[Nhowka]

Never used the feature, so I won't miss it. I was planning to clean the OO/abstract class from the server, so that change gives a good opportunity for doing it.

[Zaid-Ajaj]

@Nhowka I don't use that feature either, always used the alternative approach (for example when I needed to access the context to get the contextual logger when using Suave.SerilogExtensions) but I was wondering how the others are using it and whether it is a good approach (to me, as explained above, that is not the case because of testability)

I was planning to clean the OO/abstract class from the server

That's great, I was thinking the same, also wanted to simplify the api and use/add good old chaining instead of/along with the CE api (but we can discuss this in another issue maybe)

[irium]

I use Context very often. With ASP.NET Core it's very easy to do integration testing via specail testable TestServer and HttpClient (see my commit 5bd07b5e1be6b9e5184fc76be4466965141e69f3 done special for it) - it works blazingly fast :)

(Very IMHO): Skipping web server part from test doesn't make big sense because it's not integration test anymore, but still using HTTP pipeline's abstractions: forms, auth, headers etc. So I prefere to test units at application level: app and domain services, and to test integration at full web server level using mentioned above TestServer.

Maybe difference here is from the fact, that Suave and ASP.NET Core are using very different programming model.

[Zaid-Ajaj]

I use Context very often.

Are you using it with the contextual handler or are you using the alternative approach described above?

Maybe difference here is from the fact, that Suave and ASP.NET Core are using very different programming model.

That is correct, in Suave, there isn't a TestServer and it requires some extra work that involves working with Http stuff. but the reason for this discussion is just to prevent people from writing code that is not unit-testable. Integration testing is always possible with/without these handlers

Note here that I am planning to just remove the contextual handler but not remove access to HttpContext. You can use and access HttpContext, see the new doc section: Accessing Request Context that now contains an example of writing a unit-testable implementation that has both per-request and static dependencies

Also your feedback on whether it is a helpful example or not would be highly appreciated :smile:

[irium]

Are you using it with the contextual handler or are you using the alternative approach described above?

I'm using contextual handler since it's simpler to use in some aspects. But I'm thinking about to try another approach, although for now I don't see much benefits (using ASPNET.Core, keep in mind my previous comments).

As for documentation - it's excellent. Very clear :)

[Zaid-Ajaj]

@irium I have added Giraffe/Asp.net core to the docs, although I haven't tested it, I believe that migrating to a non-contextual version should go smooth, would that be the case in your project or do you expect some difficulties?

Say if you had this:

type IMusicStore<'ctx> = { 
    bestAlbums : 'ctx -> Async<Album list> 
}

let createWebApp() = 
  let musicStore : IMusicStore<HttpContext> = {
    bestAlbums = fun ctx -> async { 
       // do stuff with the ctx : HttpContext 
       return [ ] 
    } 
  }
  remoting musicStore { () }

it can be easily converted to:

type IMusicStore = { 
    bestAlbums : unit -> Async<Album list> 
}

let createWebApp() = context <| fun ctx -> 
  let musicStore = { 
    bestAlbums = fun () -> async { 
        // do stuff with the ctx : HttpContext
        return [ ] 
    } 
  } 
  remoting musicStore { () } 

where context in Giraffe/Saturn is not built-in so you have to define it yourself:

let context (f: HttpContext -> HttpHandler) : HttpHandler =
    fun (next: HttpFunc) (ctx : HttpContext) -> 
        task {
            let handler = f ctx
            return! handler next ctx 
        }

P.S. I still need to test this to make sure it works as expected

[irium]

@Zaid-Ajaj My setup is more complicated than your simple example :)

let createWebApp (db: IDatabase) (security: ISecurityOps) = context <| fun ctx ->
    // extract token from the context
    let securityToken = 
       ctx.request.headers
        |> List.tryFind (fun (key, _) -> key = "authorization")
        |> Option.map (snd >>  SecurityToken)
    // create the store
    let musicStore = createMusicStore db security securityToken 
    // expose as web part
    remoting musicStore {()}

Let's imagine that I have more than one "remotings" - together with musicStore there are about ten "web parts". And not every one needs securityToken and db. So there will be overhead of creating securityToken, parsing http headers etc that will not be used further in the chain.

So I think we need another level of indirection - some services holder object:

    type SiteServices = {
        db: unit->IDatabase
        security: unit->ISecurityOps
        securityToken: unit->SecurityToken option
    }

...

let createWebApp (db: IDatabase) (security: ISecurityOps) = context <| fun ctx ->
    // create services object
    let services = {
        db = fun () -> db
        security = fun () -> security
        securityToken = fun () ->
            // extract token from the context
            ctx.request.headers
            |> List.tryFind (fun (key, _) -> key = "authorization")
            |> Option.map (snd >> SecurityToken)
    }

    // create the store
    let musicStore = createMusicStore services
    // create other part
    let admin = createAdmin services

    // compose parts
    choose [
        GET >=> route "/" >=> (app |> htmlView)
        subRoute Endpoint (Images.handler ImagesPath)
        subRoute Endpoint musicStore
        subRoute Admin.Endpoint admin
        setStatusCode 404 >=> text "Not Found" ]

So each web part essentially turns to func SiteServices->HttpHandler. (While now it is HttpContext->HttpHandler). I went further by making each action handler to Reader<HttpContext,'a> (using great FSharpPlus library and custom CE handler). Thus I'm abstracting away access to services to smth. like this:

        let getStreams() : ApiHandler<Async<StreamId list>> =
            handler {
                let! eventStore = asks getService<IEventStore>
                return eventStore.Stream()
                       |> Seq.map Persisted.getStreamId
                       |> Seq.distinct
                       |> Seq.sort
                       |> Seq.toList
            }

where

type ApiHandler<'a> = Reader<HttpContext,'a>

where Reader<'a,'b> is essentially a func 'a->'b.

And we can abstract it further: Reader monad is functional way to do Dependency Injection. So of creating each time custom SiteServices type we can use something like this:

type IServices =
    abstract member Resolve<'T> : unit -> 'T
    abstract member TryResolve<'T> : unit -> 'T option

let resolve<'T> =
    Reader (fun (services:IServices) -> services.Resolve<'T>())

let tryResolve<'T> =
    Reader (fun (services:IServices) -> services.TryResolve<'T>())
...

// Usage:
    let handle query =
        monad {
            let! storage = resolve<IImageStorageService>
            let! search = tryResolve<ImageSearcher>
            ...
        }

Uugh, long post :) I hope you got my thoughts.

[irium]

Addendum: my dependencies has complex inter-connection graph, some of them are created by factories, that depends on another deps. So it's easier to manage this complex things via standard ASPNET.Core depency injection (via Autofac that do all that plumbing automatically).

Maybe it just because I've used to do OO for a long time before :)) But indeed Autofac is a great library that do almost every thing.

[irium]

Also, another performance hit is that all remotings will be created every request.

[Zaid-Ajaj]

@irium

Uugh, long post :) I hope you got my thoughts.

Long post you say? I wanted MORE! :smile: this is very interesting

Ok, let me break this down to make sure I understand you

Let's imagine that I have more than one "remotings" - together with musicStore there are about ten "web parts". And not every one needs securityToken and db. So there will be overhead of creating securityToken, parsing http headers etc that will not be used further in the chain.

If each remoting handler requires different dependencies, then each factory (i.e. createMusicStore, createAdmin etc.) would require different dependencies:

let createMusicStore (db: IMusicDb) (security: ISecurityOps) = (* *) 
let createAdmin (security: ISecurityOps) = (*  *) 

Please correct me on the following: I think you already know that this is possible but as you said, you want to "abstract away access to services" So you don't want to have to define what every remoting requires explicitly but you want a dependency injection mechanism that allows you to resolve<'Service> whenever you need a 'Service and the mechanism would know how to get you that 'Service for you which is what AutoFac is doing at the moment.

I am totally in favor of integrating an opt-in dependency injection mechanism with Fable.Remoting, I think it is an important missing piece. let me assume some requirements

• The mechanism is an integration layer for Asp.net core dependency injection, so we won't be building our own IoC container to let people use what is already there (AutoFac, Ninject etc.)
• Creating the remoting handler happens once (even though the overhead is very minimum)

Because of my limited knowledge on the subject, your input would be highly appreciated! I will have to do some research on how asp.net core does IoC but since you already implemented a custom solution for this, what do you suggest for an ideal built-in usage of the remoting api? I already like your handler CE and something like would be nice.

My first thought is to make a remoting handler that takes a ApiHandler<'ServerImpl> instead of just 'ServerImpl like it does now so it would be used like this (looks like your Services -> IMusicStore but without a record):

let createMusicStore : ApiHandler<IMusicStore> =
   handler {
       let! musicDb = getService<IMusicDb>()
       return {  popularAlbums = musicDb.mostPopularAlbums() }  
   } 

Moreover, can this handler be unit tested?

Lastly, how does removing the contextual handler affect your implementation for this?

@Nhowka What do you think of this? Let me know :smile:

[Nhowka]

We are doing some sort of primitive injection here: https://github.com/Zaid-Ajaj/Fable.Remoting/blob/master/Fable.Remoting.Server/ServerSide.fs#L183-L189

Here is how it knows when to inject: https://github.com/Zaid-Ajaj/Fable.Remoting/blob/master/Fable.Remoting.Server/ServerSide.fs#L183-L189

We could allow any number of generic arguments and have a dictionary of implementations that could be inject. As they are independent of the request it could be more deterministic and testable but wouldn't solve the context problem.

[Zaid-Ajaj]

Thanks to @irium I have more or less managed to implement a prototype that utilizes the standard DI mechanism of asp.net core which I am going to experiment on with the asp.net core middleware. I won't be able to work on the this until next week but here is a taste of how it looks like:

type IMusicStore = { bestAlbums : Async<Album list> }

// IMusicStore that uses DI 
let musicStore = reader {
    let! musicDb = DI.Resolve<IMusicDb>()
    let! securityOps = DI.Resolve<ISecurityOps>() 
    let! context = DI.Resolve<HttpContext>() 
    return { bestAlbums = async { return musicDb.getAwesomeAlbums() } }
}

// plain IMusicStore value without DI 
let simpleMusicStore = { bestAlbums = (*  *) }

let remoteMusicStore = 
    Remoting.createApi()
    |> Remoting.withRouteBuilder (sprintf "/api/%s/%s")
    |> Remoting.fromReader musicStore 
//  |> Remoting.fromValue simpleMusicStore 
    |> Remoting.build 

// wire up with IAppBuilder
app.UseRemoting(remoteMusicStore)

@Nhowka I am afraid I will have to rewrite middleware code to simplify the whole thing because I won't be using custom handlers and this only applies to Asp.net core

[Zaid-Ajaj]

I guess the builder for Suave can become a simple function of HttpContext -> 'Implementation instead of a reader (technically the same) because Suave doens't have a DI mechanism as far as I know, so it can be:

let musicStore (context: HttpContext) : IMusicStore=  {
    (* do stuff with context *)
    bestAlbums = (* *)
}

// plain IMusicStore value without DI 
let simpleMusicStore = { bestAlbums = (*  *) }

let remoteMusicStore = 
    Remoting.createApi()
    |> Remoting.withRouteBuilder (sprintf "/api/%s/%s")
    |> Remoting.fromContext musicStore 
//  |> Remoting.fromValue simpleMusicStore 
    |> Remoting.buildWebPart

[Nhowka]

Couldn't we just have it take an extra Type -> obj and for the asp.net case create an wrapper to the existing DI while enabling Suave to use the same code?

[Zaid-Ajaj]

@Nhowka Can you please elaborate, what do mean by Type -> obj?

create an wrapper to the existing DI

The DI.Resolve<'T>() in the sample above is already the wrapper (that I still need to test)

[Nhowka]

@Zaid-Ajaj Is the DI.Resolve<'T> using typeof<'T> internally at some point? That would be effectively the same as the Type -> obj that could return the implementation to inject on the function. This way we could make a Map<Guid,obj> to register the implementations on Suave and then having fun (t:Type) -> map |> Map.tryFind t.Guid |> Option.defaultValue null as the dependency injector filling the Type -> obj spot.

[Zaid-Ajaj]

I understand now, it would be possible to create our own IoC container for Suave but it wouldn't be as simple as Map<Guid, obj>, IoC containers need to be able to create instances of different classes and control their lifetime, so if possible we would build a wrapper around an existing implementation like AutoFac or Ninject but again I am not sure whether we should do that just to be able to share code between essentially two implementations (Suave vs. Aspnet core and friends)

[Nhowka]

That could be just an example implementation, but any Type -> obj could be fine. I don't even know if most IoC can be fit on that signature, but having it optional to be used inside that point where we are already injecting the context could work.

[Zaid-Ajaj]

Using Type -> obj would unify the API but the only thing you can now resolve in Suave is the HttpContext, so having a generic function for that is overkill but we will see how things can turn out later if we start working on DI integration for Suave

[Nhowka]

Did a test branch with a function for injection but CI seems broken. Integration tests passed locally.

[Zaid-Ajaj]

@Nhowka I don't know why it didn't work but I restarted the build and it seems to run fine now :)

[Zaid-Ajaj]

Fixed in #58, reader added