Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Is there a cleaner way to compose with routef? #223

Closed
Lanayx opened this issue Feb 10, 2018 · 16 comments
Closed

Is there a cleaner way to compose with routef? #223

Lanayx opened this issue Feb 10, 2018 · 16 comments
Labels
ready for release Issue has been already resolved in the development branch
Milestone

Comments

@Lanayx
Copy link

Lanayx commented Feb 10, 2018

In the sample file there are lines

let showUserHandler id =
    mustBeAdmin >=>
    text (sprintf "User ID: %i" id)

https://github.com/giraffe-fsharp/Giraffe/blob/master/samples/SampleApp/SampleApp/Program.fs#L71-L73

So mustBeAdmin filter moves from all routes configuration to particular handler. Is there a better way to have both routef and mustBeAdmin composed together like route routes?

@Lanayx Lanayx changed the title Is there a more clean way to compose with routef? Is there a cleaner way to compose with routef? Feb 10, 2018
@dustinmoris
Copy link
Member

Hi,

Yes, you could also do this:

let webApp =
    mustBeAdmin >=> choose [
        routef "/....." handler1
        routef "/....." handler2
    ]

@Lanayx
Copy link
Author

Lanayx commented Feb 10, 2018

In my app I want to have some routes secured and some not, just like in the sample app, so I want to apply mustBeAdmin on per-route basis. And this works fine with route, but not fine with routef

@dustinmoris
Copy link
Member

dustinmoris commented Feb 10, 2018

Ok I understand... can you give me a more concrete example? The routef handler has obviously a dynamic element where some part of the route is whatever a user has typed into a URL. You could use subRoute or routeStartsWith in combination with an auth handler before all routef handlers:

let webApp =
    subRoute "/api"
        (mustBeAdmin >=> choose [
            routef "/..." handler1
            routef "/..." handler2
        ])

See https://github.com/giraffe-fsharp/Giraffe/blob/master/DOCUMENTATION.md#subroute for more info.

If this is not what you need, could you please provide me a simple example of what you would want to achieve?

Thank you

@dustinmoris
Copy link
Member

dustinmoris commented Feb 10, 2018

This is another example:

let webApp : HttpHandler =
    choose [
        // Not secured
        route "/"    >=> text "Hello"
        route "/foo" >=> text "Bar"

        // Secured without subRoute
        mustBeLoggedIn >=>
            choose [
                route "/user/"    >=> text "Hello"
                route "/user/foo" >=> text "Bar"
            ]

        // Secured with subRoute
        subRoute "/secure" (
            mustBeLoggedIn >=>
                choose [
                    routef "/%s"    handler1
                    routef "/%s/%i" handler2
                ]
        )
    ]

Essentially you can chain and compose http handlers however you wish!

@Lanayx
Copy link
Author

Lanayx commented Feb 10, 2018

So, here is the sample, I want this urls:
/en/articles/top
/en/article/123
to be public and this url
/en/dashboard
to be private
I was planning to have such rules:

routef "/%s/articles/top" getTopArticles
routef "/%s/articles/%s" getArticle
routef "/%s/dashboard" >=> authorize >=> getDashboard

Is there a way to implement this?

@Lanayx
Copy link
Author

Lanayx commented Feb 10, 2018

I see, looks like you mean that I need to exchange authorize and routef? And all private rules should go under the public ones, right?

@Lanayx
Copy link
Author

Lanayx commented Feb 10, 2018

I think that there is one issue there. If user enteres the wrong url, it will go through the authorization step first, which includes database interaction, while it could be just rejected by the wrong url. I think urls check should go before the user verification.

@dustinmoris
Copy link
Member

Right.. because you have a dynamic %s at the very beginning of your route I think you are better by having the authorize inside the handler like shown in the first example which you referred to.

To be fair this is the first use case where I see how a subRouteF could make sense... let me think about it and perhaps I'll try to implement something which can make this more elegant for you, but for now go with it like shown in the sample app.

@Lanayx
Copy link
Author

Lanayx commented Feb 10, 2018

Ok, thank you for the response

@dustinmoris
Copy link
Member

I have added a subRoutef http handler which will be available with the next release.

See subRoutef

@dustinmoris dustinmoris added the ready for release Issue has been already resolved in the development branch label Feb 11, 2018
@Lanayx
Copy link
Author

Lanayx commented Feb 11, 2018

Thank you! But looks like there will still be no way to silently pass the lang argument further in the compose pipe. I'm just comparing it with asp.net mvc were you just put [Authorize] attribute regardless of the route, but the workaround in your example looks sufficient, at least for now I don't think the surrounding function can become too complex.

@dustinmoris
Copy link
Member

dustinmoris commented Feb 11, 2018

Given your routes...

routef "/%s/articles/top" getTopArticles
routef "/%s/articles/%s" getArticle
routef "/%s/dashboard" >=> authorize >=> getDashboard

You can do this now:

let webApp : HttpHandler =
    subRoutef "/%s" (fun lang ->
        choose [
            subRoute "/articles" (
                choose [
                    route  "/top" >=> getTopArticles lang
                    routef "/%s" (getArticle lang)
                ])
            route "/dashboard" >=> authorize >=> getDashboard lang
        ]
    )

or:

let webApp : HttpHandler =
    subRoutef "/%s" (
        fun lang ->
            choose [
                route  "/articles/top" >=> getTopArticles lang
                routef "/articles/%s" (getArticle lang)
                route  "/dashboard" >=> authorize >=> getDashboard lang
            ]
    )

or:

let webApp : HttpHandler =
    choose [
        routef "/%s/articles/top" getTopArticles
        routef "/%s/articles/%s"  getArticle
        subRoutef "/%s" (
            fun lang ->
                route "/dashboard"
                >=> authorize
                >=> getDashboard lang
        )
    ]

The subRoutef handler allows you now to parse the first dynamic parameter at an earlier stage, so that following routes like /dashboard can be specified with a simple route where you can chain the authorize in the webApp instead of having it embedded inside your getDashboard handler .

I thought this was ultimately what you wanted right?

@dustinmoris
Copy link
Member

dustinmoris commented Feb 11, 2018

But, if you don't want to pre-parse the first parameter, there is another option which will work as well. For actually a very different reason (trailing slash handling) I have just finished implementing a routex handler: https://github.com/giraffe-fsharp/Giraffe/blob/develop/DOCUMENTATION.md#routex

With that you could check the route is correct before doing authorize and then use the full routef path for silent parameter passing:

let webApp : HttpHandler =
    choose [
        routef "/%s/articles/top" getTopArticles
        routef "/%s/articles/%s"  getArticle
        routex "/[a-z]{2}/dashboard" >=> authorize >=> routef "/%s/dashboard" getDashboard
    ]

What do you think?

@Lanayx
Copy link
Author

Lanayx commented Feb 11, 2018

You are right, I wanted to be less verbose and don't specify lang parameter in the route configuration code. For example I could also have private route /en/my-articles/123 with route /%s/my-articles/%s, so this will introduce more nested functions. The solution with routex looks more attractive to me, however I'm a bit worried about performance if regex is used there =)

@dustinmoris
Copy link
Member

Fair enough :). The regex is compiled so should be considerably faster than normal, but I understand your concerns. Unfortunately this is the best I can offer for now.

@Lanayx
Copy link
Author

Lanayx commented Feb 11, 2018

I see, thank you for such quick cooperation anyway!

@dustinmoris dustinmoris added this to the 1.1.0 milestone Feb 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for release Issue has been already resolved in the development branch
Projects
None yet
Development

No branches or pull requests

2 participants