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

Introduce FromRequest::on_entry() #480

Closed
dpc opened this issue Nov 20, 2017 · 6 comments
Closed

Introduce FromRequest::on_entry() #480

dpc opened this issue Nov 20, 2017 · 6 comments
Labels
request Request for new functionality

Comments

@dpc
Copy link

dpc commented Nov 20, 2017

Feature Requests

Why you believe this feature is necessary.

Right now FromRequest::from_request logic can only operate before the route is taken, typically to decide if the route should be taken or not. It would make request guards more powerful if they were able to do some stuff after the route was actually taken.

Convincing use-case

I have a rate-limiting type, and I'd like the rate-limiting logic to check rete limits after the actual route was matched and taken, but I don't want to have to call it everywhere manually.

Potentially this could be used to fix #466 by clearing the Flash only when route was taken.

Why this feature can't or shouldn't exist outside of Rocket.

Rocket controls what gets called and when.

Notes

This could be implemented by adding new method to FromRequest with no-op default implementation and generated code calling it on all request guards before calling the handler function.

Maybe on_exit would be useful too.

@SergioBenitez
Copy link
Member

Can you detail further why it's important that you know which route was taken in your example of rate-limiting? In on_request, you have the complete request information, including the path. I'm wondering: why isn't this sufficient?

I believe you can also implement what you're suggesting as a request guard. Then you'd simple need to add the type to the route's signature to get the functionality you're looking. This way, you also have the flexibility to order guards the way you'd like.

@SergioBenitez SergioBenitez added the request Request for new functionality label Nov 22, 2017
@dpc
Copy link
Author

dpc commented Nov 22, 2017

@SergioBenitez : Yes. So I think I want on_request-like hook for request guards. Right now in from_request I don't know if the route will be actually taken, so I have to manually call something at the top of the function:

#[(get(...)]
fn some_api_call(rl : RateLimiter) -> Result<Template> {
    rl.limit()?;

}

Instead, I'd like RateLimiter to do rl.limit() in a on_enter hook, so I don't have to remember about calling it.

@SergioBenitez
Copy link
Member

SergioBenitez commented Nov 22, 2017

If you have a route that looks like this:

#[(get(...)]
fn something(a: A, b: B, c: C, rl: RateLimiter) -> T {
   ...
}

Then if the from_request() method of RateLimiter gets called, the route something will be called as long as RateLimiter's guard doesn't fail. Request guards are the final mechanisms by which routing can fail. If the last request guard (the right-most one) gets called, routing will succeed as long as that request guard doesn't fail or forward the request.

@dpc
Copy link
Author

dpc commented Nov 22, 2017

@SergioBenitez I don't think users should have to figure out the right order of elements, especially that nothing can enforce it, and mistakes would lead to weird bugs. That's why I think from_request could be split into "check" and "do" part. "check"s should be fast and idempotent, while "do" can have side effects.

There are basically two classes of request guards: failable matches (that can prevent taking the route at all), and infallible utilities, and they are both used the same way, but the user has to keep their order straight.

Hmmm... Maybe the macro could take care of the ordering internally. Like one of the could be marked / distinct and macro would try them in the right order. Is that possible?

@dpc
Copy link
Author

dpc commented Nov 22, 2017

Hmmm... But then in my RateLimiter I'm returning a special error case, that is being handled in Response implementation of `Result, and this displays a nice "too fast, try in a minute" page. I wish that would still be possible.

@SergioBenitez
Copy link
Member

I believe the general notion being asked for here will eventually be answered by #749. Closing in favor of that issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
request Request for new functionality
Projects
None yet
Development

No branches or pull requests

2 participants