-
Notifications
You must be signed in to change notification settings - Fork 204
Simplify AuthorizeRequest
and AsyncAuthorizeRequest
traits
#192
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
Conversation
@hamza1311 what do you think? |
API looks good. Better than the simple swapping of Option with Result that I suggested |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks a lot cleaner to me!
type ResponseBody; | ||
|
||
/// The Future type returned by `authorize` | ||
type Future: Future<Output = Result<Request<Self::RequestBody>, Response<Self::ResponseBody>>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really neat!
|
||
/// The Future type returned by `authorize` | ||
type Future: Future<Output = Option<Self::Output>>; | ||
/// Set this to `B` if you need to change the request body type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to have this as part of the docs 👍
be used instead ([#170]) (BREAKING) | ||
- **fs**: Changed response body type of `ServeDir` and `ServeFile` to | ||
`ServeFileSystemResponseBody` and `ServeFileSystemResponseFuture` ([#187]) (BREAKING) | ||
- **auth**: Change `AuthorizeRequest` and `AsyncAuthorizeRequest` traits to be simpler ([#???]) (BREAKING) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah forgot to mention this, we can put in the proper pr number here now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah good catch!
Fixes #190
This changes the
AuthorizeRequest
andAsyncAuthorizeRequest
traits to be quite a bit simpler I think.Sync
Before: https://github.com/tower-rs/tower-http/blob/master/tower-http/src/auth/async_require_authorization.rs#L234-L269
After
authorize
now returnsResult<(), Response>
Ok(())
means the request was accepted and will be passed on the inner serviceErr(response)
means the request couldn't be authorized and the response will be returned without calling the inner service.Async
Before: https://github.com/tower-rs/tower-http/blob/master/tower-http/src/auth/async_require_authorization.rs#L234-L269
After
authorize
now receives an owned request. This allows you to move the request into the future, do some async stuff which yields some data (such as user id from a database), and then save that in a request extension.&mut Request
because it would require GATs.Result<Request, Response>
so the request can be passed on the inner service.B
tohttp_body::Full
.AsyncPredicate
from tower.