SaturnFramework / Saturn

Opinionated, web development framework for F# which implements the server-side, functional MVC pattern
https://saturnframework.org
MIT License
707 stars 108 forks source link

[Feedback requested] WIP: Add HEAD handling to router and controller #273

Open rmunn opened 3 years ago

rmunn commented 3 years ago

This is an initial implementation of #269, looking for feedback. Things I think should be added before this PR is ready to merge:

1. Using get_head in the same router as either get or head is probably programmer error, as one of those handlers will trump the other one.

The way I wrote it, get_head is checked after get and head. So if you specify both get and get_head, but not head, in a router, then all GET requests will be handled by the get handler, and HEAD requests will be handled by the get_head handler. If you specify both head and get_head, but not get, then HEAD requests will be handled by the head handler while GET requests will be handled by the get_head handler. And if you specify all three, then the get_head handler will never be called.

I don't think the computation expression syntax will allow us to throw up a compile-time error if get_head is present in the same router as either get or head, but a runtime warning might be a good idea, as well as a note in the documentation.

2. exists in controller currently doesn't dictate return type. Should it dictate a return type of bool? Or rather, Task<bool>?

If exists is written in the way I currently have it, where it handles a generic return type called 'ExistsOutput, then the user is responsible for turning "Yes, the item exists" into a 200 OK response, and "No, it doesn't exist" into a 404 NOT FOUND response. (Or there might be other responses desired, such as 403 FORBIDDEN in some cases). If we change the exists operation to expect a function returning Task<bool>, then the user just has to return true (which Saturn would change to 200 OK) or false (which Saturn would change to 404 NOT FOUND). Simpler for the user, but someone wanting to return 403 FORBIDDEN would have to do that in a controller plugin since the exists operation wouldn't do it for him.

I'm leaning towards saying that the exists operation should expect a function returning Task<bool>, and then Saturn will take care of turning that into a 200/404 response. Would welcome feedback on this one.

3. Unit tests are a good idea, and I haven't written them yet. Should test, among other things:

4. Saturn documentation needs to be updated to document the new head and get_head operations in router CEs and exists in controller CEs.

I won't be able to work on documentation for a week or two, I think. If someone else wants to do it before then, I'd welcome the help.

Krzysztof-Cieslak commented 3 years ago

How this will look like on the endpoint routing (ControllerEndpoint.fs and RouterEndpoint.fs)? - since we're going to focus on endpoint routing in the future, it's really important to make sure any ideas are working fine with it.