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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rmunn
Copy link
Contributor

@rmunn rmunn commented Sep 3, 2020

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:

  • get_head handles both GET and HEAD requests, and HEAD requests end up with bodiless responses thanks to what ASP.NET does for you. (That last part might be more of an acceptance test)
  • get_head in same router as head: head handles HEAD requests while get_head handles GET requests
  • get_head in same router as get: get handles GET requests while get_head handles HEAD requests
  • exists in controller

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.

Router adds "head", "headf", "get_head", "get_headf" operations, and
controller adds "exists" operation.
@rmunn rmunn changed the title Add HEAD handling to router and controller [Feedback requested] WIP: Add HEAD handling to router and controller Sep 4, 2020
@rmunn rmunn marked this pull request as draft September 4, 2020 07:55
@Krzysztof-Cieslak
Copy link
Member

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.

@Krzysztof-Cieslak Krzysztof-Cieslak marked this pull request as ready for review November 8, 2020 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants