Guid pattern in endpoint router matches invalid values, throws System.FormatException about giraffe HOT 6 CLOSED

Hey, I did explore this problem today and would like to add some comments.

First, I noticed that this problem is happening with Giraffe.EndpointRouting. I did test with Giraffe router, and it simply ignores the request, going to the response status 404:

Request reached the end of the middleware pipeline without being handled by application code. 
Request path: GET http://localhost:5000/try-a-guid/00000000-0000-0000-0000-0000000000123, 
Response status code: 404

I was using this code (Giraffe router):

open System
open Microsoft.AspNetCore.Builder
open Microsoft.AspNetCore.Hosting
open Microsoft.Extensions.Hosting
open Microsoft.Extensions.Logging
open Microsoft.Extensions.DependencyInjection
open Giraffe

let webApp =
    GET >=> routef "/try-a-guid/%O" (fun (guid: Guid) -> text $"Success: {guid}" )

type Startup() =
    member __.ConfigureServices (services : IServiceCollection) =
        // Register default Giraffe dependencies
        services.AddGiraffe() |> ignore

    member __.Configure (app : IApplicationBuilder)
                        (env : IHostEnvironment)
                        (loggerFactory : ILoggerFactory) =
        // Add Giraffe to the ASP.NET Core pipeline
        app.UseGiraffe webApp

[<EntryPoint>]
let main _ =
    Host.CreateDefaultBuilder()
        .ConfigureWebHostDefaults(
            fun webHostBuilder ->
                webHostBuilder
                    .UseStartup<Startup>()
                    |> ignore)
        .Build()
        .Run()
    0

And I confirm that this problem is still happening in the most recent version of Giraffe.

At first, I thought it was not a good idea to add some hidden feature that maps incorrect regex parsing to a generic error response. It could be a pain to debug (imagine having an API with 1_000_000s of requests, and out of nowhere some start returning an unexpected status code + response which you don't find in your codebase). If you're using this %O pattern, it's your responsibility to grant that the input will be GUID-compliant. Otherwise, if you want to avoid this problem, I would recommend using %s and then, inside the endpoint handler, you try to convert to GUID.

Then, after digging deeper into the problem, I think it's better to handle it properly, at least trying to replicate what we have with Giraffe's router. It must demand simply a tweak in the GUID regex pattern.

I'll take a look at it this week, and probably add some new tests.

from giraffe.

Giraffe version 6.4.1-alpha-3 has the fix. I tested locally, and it's not matching the wrong Guid. Please let me know if there's something else regarding this issue @jcmrva.

from giraffe.

Looks good to me. Thanks!

from giraffe.

What's the behavior you're after? Why do you consider the above a bug?

from giraffe.

We'd rather it return 4xx, and IMO in general, user input should not result in unhandled exceptions.

from giraffe.

Feel free to add your review to this PR #594

from giraffe.