DELETE requests to deletef and forwardf routing to a controller fail with 404 not found

MarneeDear commented 5 years ago

Possibly related to issue #54

Using the latest from Saturn master branch as of 12/20/2018 at 10:00 MDT.

Using Controller.Sample I added a deletef route to topRouter and added a new deleteRoute router. The route pattern is /delete/%i.

This is the URL I put in PostMan to make the DELETE request: http://localhost:8085/delete/1111

You can see the source code for this in my forked repo here: https://github.com/MarneeDear/Saturn/blob/delete-routes-404/sample/Controller.Sample/Controller.Sample.fs#L105

Delete requests to this route fail with 404 when the route sends directly to a controller

   deletef "/delete/%i" (fun (_ : int) -> userController)

Requests directly to a function are OK.

let apiDeleteExample2 id = sprintf "Echo: %i" id |> text

//inside topRouter
    deletef "/delete/%s" apiDeleteExample2

I'm using dotnet on Windows 10. When I run Controller.Sample I do dotnet watch run.

UPDATE

I think this happens when the id passed to the controller is not a string.

This doesnt work (`id` is type `int`)


//In userController
    delete (fun ctx id -> (sprintf "Delete handler no version failed - %i" id) |> Controller.text ctx)

//In topRouter
    deletef "/delete/%i" (fun (_:int) -> userController)

This works (`id` is type `string`)

//In userController
    delete (fun ctx id -> (sprintf "Delete handler no version failed - %s" id) |> Controller.text ctx)

//In topRouter
   deletef "/delete/%s" (fun (_:string) -> userController)

MarneeDear commented 5 years ago

forwardf instead of getf seems to work, but I am having problems with pointing it to another router. I'm going to write a new issue.

MarneeDear commented 5 years ago

Made the comment on the wrong issue.

MarneeDear commented 5 years ago

I suspect I will need to use forwardf. I am trying that out.

MarneeDear commented 5 years ago

DELETE with a forwardf to a controller are 404.

These both 404.

    deletef "/delete/%i" (fun (_:int) -> userController)
    forwardf "/delete/%i" (fun (_ : int) -> userController)

See full source code here. https://github.com/MarneeDear/Saturn/blob/delete-routes-404/sample/Controller.Sample/Controller.Sample.fs#L112

NinoFloris commented 5 years ago

I just looked at what the code is doing, and its not wrong. However it's not obvious how it all works either. To understand you need to know a few important points to grasp why all your examples do what they do.

Saturn uses Giraffe subrouting under the hood. For router {} only forward and forwardf do this and also mention it — somewhat obfuscated — in their method details. Forwards calls to different scope. Modifies the HttpRequest.Path to allow subrouting.

When you use forward/forwardf, they consume their matched route segment so to say, allowing the next handler to start matching at the subsequent segment. This is how subController works as well.
If you use any consuming 'formatting' route handler (so forwardf and subController), always use the passed-in argument. This is the only time you'll get that route argument. So every example that does something like: deletef "/delete/%i" (fun (_:int) -> userController) will discard the %i and put the route pointer past that segment. So by the time the userController matches its action routes they all start their matches AFTER /delete/%i.

So that means in this case when you would call it with delete/11/1 you would get 1 as the id in your controller actions. For delete/11 it correctly 404s if you don't have an index/post/deleteall action.

Now you also see, when you don't use a consuming route handler as delete/deletef and related friends are, every subsequent handler will start their match at the same point in path again.

In your samples you write out id as %s in your sprintf handler. Through F#s type inference that will set the controller's 'Key type as string. The result is that the string route basically matches anything of path, returning to you delete/11 as the id.

This last part feels like a bug though, and is something I need to think about, it seems incorrect that a 'Key = string controller would just always match the full remaining path, intuitively I would expect it would return delete as the id instead, stopping at the first unencoded '/'. What do you think @Krzysztof-Cieslak?

Some functional examples for what you want and to get a feel for the mechanisms: This is the canonical idiomatic way to do what you want in Saturn:

let projectsController = controller {
    index (fun ctx -> "List of projects" |> Controller.text ctx)
    ...
    delete (fun ctx id -> sprintf "My id %s" id |> Controller.text ctx)
}

let r = router {
    forward "/projects" projectsController
}

If you need a route that just does delete, you should use your Requests directly to a function are OK. example. But if you really need controller functionality you can do the following:

let deleteController = controller {
    delete (fun ctx id -> sprintf "My id %s" id |> Controller.text ctx)
}

let r = router {
    // forward will consume /delete so /11 is what is passed to deleteController to be parsed as id
    // DELETE and other HttpMethod filters can be found in Giraffe.Core
    forward "/delete" (DELETE >=> deleteController)
}

This is a weird one, but good for grokking how it all works. deleteAll matches on "/" while delete matches on "/%'Key". So now you basically made your controller a function that accepts an id, (like subControllers) and then it uses the deleteAll route to match on the remaining route part if its a DELETE request.

let deleteController id = controller {
    deleteAll (fun ctx -> sprintf "My id %s" id |> Controller.text ctx)
}

let r = router {
    deletef "/delete/%s" deleteController
}

Hope this helps!

NinoFloris commented 5 years ago

Fix for weird matching behavior of %s controllers is pushed, see https://github.com/SaturnFramework/Saturn/pull/168

Krzysztof-Cieslak commented 5 years ago

Hey, @NinoFloris thanks for the detailed response!

@MarneeDear, I'll close this issue - as Nino mentioned above, most of the behaviour is expected, and the %s bug was fixed by #168 and will be available in next Saturn release. Please let us know if we can help more with the problems - we probably need to clarify some documentation around (sub)routing.

SaturnFramework / Saturn