Skip to content

Default Handler Implementation #64

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

Merged
merged 9 commits into from
Nov 28, 2018
Merged

Conversation

tzilist
Copy link
Contributor

@tzilist tzilist commented Nov 26, 2018

So this should tackle #10 .

Little bit of info on what is happening here,

The call to router will no longer return Some(RouteResult) and instead always returns a RouteResult. The router will fallback to the default_handler that is set. This allows us to use all middleware that are set in the middleware_base of the root router. In the example, you can try it and see that logging is now working for the fallback route.

I'm not going to lie, this particular PR was probably a bit above my Rust level and feels a bit hacky to me. Let me know what you guys think!

Also, special thank you to @tirr-c for his countless hours answering my non-stop questions ❤️

TODO:
I'd like to write some tests but I will wait until the final implementation to do so. Let's hold off on merging this at least until some tests are written 😄

Copy link
Collaborator

@tirr-c tirr-c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This nicely addresses my suggestion on always-applied middleware at #61 (comment)! However, there are some points that we can get it better, so I left a suggestion.

Some thoughts:

  • You can make params optional if you like. It's totally okay if you don't -- I'll do it then.
  • This is an open question: if the path matched but the endpoint for our method is not there, what middleware should be run? In this case, we also have resource-specific middleware chain and actual wildcard matches.

@tzilist tzilist changed the title initial implementation of default handler completed Default Handler Implementation Nov 26, 2018
@tzilist
Copy link
Contributor Author

tzilist commented Nov 26, 2018

@tirr-c I've gone ahead and made params optional. Would you mind taking a look to make sure it looks ok? Most specifically at the head file. Just want to make sure this logic makes sense - my thought was if we did not find any params here that should be a 400 error. There also may be a better way to write this using method's instead of nested match statements.

@tzilist
Copy link
Contributor Author

tzilist commented Nov 26, 2018

I've gone ahead and split apart the Router into BaseRouter and SubRouter to remove the need to delegate the default_handler around for no reason. I've created a trait called Router. The only downside is that there is some duplication between the at and middleware functions between BaseRouter and SubRouter.

src/head.rs Outdated
Ok(t) => future::ok(Path(t)),
Err(_) => future::err(http::status::StatusCode::BAD_REQUEST.into_response()),
},
None => future::err(http::status::StatusCode::BAD_REQUEST.into_response()),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think trying to extract match information from default handlers is a bug in application code. One should know that there's no wildcard match in default handler. Maybe INTERNAL_SERVER_ERROR is better, or... should panic here? 😝

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha yeah...I am hesitant to panic but I did make it INTERNAL_SERVER_ERROR. I guess I could make it panic? I dunno what to do here 😢

@tzilist tzilist force-pushed the feat-default-handler branch from 09528e0 to 0ff73cf Compare November 28, 2018 11:42
@tzilist tzilist force-pushed the feat-default-handler branch from 0ff73cf to f6cf725 Compare November 28, 2018 11:48
Copy link
Collaborator

@tirr-c tirr-c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're very close!

Copy link
Collaborator

@tirr-c tirr-c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, let's wait for travis!

@tirr-c tirr-c merged commit c704f41 into http-rs:master Nov 28, 2018
@tirr-c tirr-c mentioned this pull request Jan 10, 2019
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