Giraffe icon indicating copy to clipboard operation
Giraffe copied to clipboard

Returning 406 when mustAccept fails

Open johnnyggalt opened this issue 1 year ago • 10 comments

Hi,

Maybe I'm missing something obvious, but I can't find an example of getting mustAccept to behave according to best practices when the Accept header does not match any of the specified content types, which is to return 406. The implementation skips the pipeline in that case, which in my case will result in a 404 because that's the reasonable default.

How can I use mustAccept in a way that allows me to return a 406 if it fails?

johnnyggalt avatar Jun 04 '24 04:06 johnnyggalt

OK, I worked it out - sorry. Something like this:

let handler: HttpHandler  =
    choose [
        mustAccept [ "application/json" ] >=>
            choose [
                subRoute "/v1" v1Handler
            ]

        RequestErrors.notAcceptable (text "Not accepted")
    ]

johnnyggalt avatar Jun 04 '24 04:06 johnnyggalt

Actually, wait, no. That means that if the resource is not found I'll get a 406 rather than 404 😢 I remain confused and would really appreciate a pointer in the right direction 🙏

johnnyggalt avatar Jun 04 '24 05:06 johnnyggalt

I landed on this helper function, which I'm not overly happy about, but don't know if there's a better way to achieve my goals:

let mustAcceptJson (inner): HttpHandler =
    let mimeType = "application/json"
    choose [
        mustAccept [ mimeType ] >=> inner
        RequestErrors.notAcceptable (text $"Client does not accept MIME type: {mimeType}")
    ]

I use it like this:

route "/user_info" >=> mustAcceptJson (authenticated >=> userInfoHandler)

Any suggestions for refinement are welcome.

johnnyggalt avatar Jun 04 '24 05:06 johnnyggalt

OK, after further testing I had to revisit this and I give up trying to use mustAccept - I just cannot see how to compose it in the manner required. Instead, I wrote my own helper:

let private jsonMimeType = MediaTypeHeaderValue.Parse("application/json")

let acceptsJson: HttpHandler =
    fun (next : HttpFunc) (ctx : HttpContext) ->
        ctx.Request.GetTypedHeaders().Accept
        |> Seq.exists jsonMimeType.IsSubsetOf
        |> function
            | true -> next ctx
            | false -> RequestErrors.notAcceptable (text $"Client does not accept MIME type: {jsonMimeType}") next ctx

This can be plumbed through with the fish operator in an intuitive manner:

GET >=> acceptsJson >=> authenticated >=> routef "/something/%O" getSomethingHandler

Here's hoping I don't find any more gotchas 🙈

johnnyggalt avatar Jun 12 '24 01:06 johnnyggalt

Hi @johnnyggalt, thanks for opening this issue and for posting the advances you're making, it could definitely be useful for other people too!

I'll try to take a look at this problem during this week.

64J0 avatar Jun 13 '24 23:06 64J0

Actually, wait, no. That means that if the resource is not found I'll get a 406 rather than 404 😢 I remain confused and would really appreciate a pointer in the right direction 🙏

So you want:

  1. A middleware that checks whether the request has the necessary mime type header (for example, application/json), and returns the 406 status code if it does not have (short-circuit to the error);
  2. And keep returning the 404 status code if the route does not exist.

Is that it?

64J0 avatar Jun 19 '24 23:06 64J0

OK, after further testing I had to revisit this and I give up trying to use mustAccept - I just cannot see how to compose it in the manner required. Instead, I wrote my own helper:

let private jsonMimeType = MediaTypeHeaderValue.Parse("application/json")

let acceptsJson: HttpHandler =
    fun (next : HttpFunc) (ctx : HttpContext) ->
        ctx.Request.GetTypedHeaders().Accept
        |> Seq.exists jsonMimeType.IsSubsetOf
        |> function
            | true -> next ctx
            | false -> RequestErrors.notAcceptable (text $"Client does not accept MIME type: {jsonMimeType}") next ctx

This can be plumbed through with the fish operator in an intuitive manner:

GET >=> acceptsJson >=> authenticated >=> routef "/something/%O" getSomethingHandler

Here's hoping I don't find any more gotchas 🙈

I think this is the best approach. For instance, you're doing something similar to what mustAccept does: https://github.com/giraffe-fsharp/Giraffe/blob/master/src/Giraffe/Core.fs#L222-L240.

64J0 avatar Jun 19 '24 23:06 64J0

you're doing something similar to what mustAccept does

Right, but that's why I thought I should be able to use mustAccept in the first place, but it does not compose in the manner required. I'm curious whether people are using mustAccept because to me it seems useless because you end up stuck if you want a "standard" experience of 406 for incorrect Accept headers whilst retaining other things like 404 for undefined routes.

PS. I'm perfectly happy with the implementation of acceptsJson I came up with, so this is now more a curiosity about mustAccept than anything else.

johnnyggalt avatar Jun 21 '24 05:06 johnnyggalt

Maybe negotiate could be used.

64J0 avatar Aug 03 '24 22:08 64J0

Also, this PR must be related https://github.com/giraffe-fsharp/Giraffe/pull/502

64J0 avatar Aug 03 '24 22:08 64J0