%s matches components of a path

[alisterw]

I'm not sure if this is a bug or expected behaviour, but if I have this route:

routef "/foo/%s/%s" (fun (s1, s2) -> text (sprintf "%s,%s" s1 s2))

Then when I request /foo/a/b/c/d the first string contains a/b/c and the second contains d.

This is actually the behaviour I want, but it's very surprising to me. Can I rely on it, or is it unintended and liable to be fixed in future, thereby breaking me?

[alisterw]

As another experiment, I added this route:

routef "/bar/%c/%s" (fun (c1, s2) -> text (sprintf "%c,%s" c1 s2))

then requested /bar//xyzzy, which gave me / and xyzzy.

This also seems surprising.

[johlrich]

I also found this counter-intuitive and noticed routeBind works differently and matches the routes I originally expected routef to match. The following will not match /foo/a/b/c/d:

routeBind<TestRoute> "/foo/{s1}/{s2}" (fun route -> text (sprintf "%s,%s" route.s1 route.s2))

[<CLIMutable>]
type TestRoute = {
  s1: string
  s2: string
}

After taking a closer look, routef would match like routeBind if the %s regex were to map to "([^/]+)"

This failing test would pass with that change (without affecting any other test case):

[<Fact>]
let ``routef: GET "/foo/bar/baz/qux" returns 404 "Not found"`` () =
    let ctx = Substitute.For<HttpContext>()
    let app =
        GET >=> choose [
            routef "/foo/%s/%s" (fun (s1, s2) -> text (sprintf "%s,%s" s1 s2))
            setStatusCode 404 >=> text "Not found" ]

    ctx.Request.Method.ReturnsForAnyArgs "GET" |> ignore
    ctx.Request.Path.ReturnsForAnyArgs (PathString("/foo/bar/baz/qux")) |> ignore
    ctx.Response.Body <- new MemoryStream()
    let expected = "Not found"

    task {
        let! result = app next ctx

        match result with
        | None     -> assertFailf "Result was expected to be %s" expected
        | Some ctx ->
            let body = getBody ctx
            Assert.Equal(expected, body)
            Assert.Equal(404, ctx.Response.StatusCode)
    }

It feels like a bug, but maybe changing routef is not a good idea since folks may depend on the current behavior even though it's not covered by a test. If the current behavior is expected, perhaps some documentation and an additional route* function is a more appropriate way to offer both options?

[alisterw]

Agreed on the regex. I've also been toying with an implementation of catch-all routing for routeBind, so that you can have things like:

routeBind "/{*Foo}/{Bar}"

where Foo is a string, and then /a/b/c/d will route with Foo = "a/b/c" and Bar = d.

But it adds a fair bit of complexity so I might not bother submitting it as a PR.

[dustinmoris]

@johlrich I will have a close look at your change and test today. I would prefer to fix the library to follow expected conventions rather than adding much documentation to explain unexpected behaviour. This would be a big breaking change indeed, but at this point I think the responsible thing to do is to mention this in big capital letters at the top of the release notes and increase the major version. I'll have a dig at this later today!

[Lanayx]

@dustinmoris Hi, I'm running into this issue as well, can you share when we should expect the 4.0 release to be out?

[maxdeviant]

Just ran into this as well (we're using routeCif, but I'm assuming the same fix applies).

[OnurGumus]

@dustinmoris, what if we want the old behavior?