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

Returning 406 when mustAccept fails #598

Open
johnnyggalt opened this issue Jun 4, 2024 · 10 comments
Open

Returning 406 when mustAccept fails #598

johnnyggalt opened this issue Jun 4, 2024 · 10 comments
Labels
question General question

Comments

@johnnyggalt
Copy link

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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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 🙈

@64J0
Copy link
Member

64J0 commented Jun 13, 2024

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
Copy link
Member

64J0 commented Jun 19, 2024

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
Copy link
Member

64J0 commented Jun 19, 2024

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 64J0 added the question General question label Jun 19, 2024
@johnnyggalt
Copy link
Author

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.

@64J0
Copy link
Member

64J0 commented Aug 3, 2024

Maybe negotiate could be used.

@64J0
Copy link
Member

64J0 commented Aug 3, 2024

Also, this PR must be related #502

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question General question
Projects
None yet
Development

No branches or pull requests

2 participants