-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Typed catchers #2814
base: master
Are you sure you want to change the base?
Typed catchers #2814
Conversation
- Catchers now carry `TypeId` and type name for collision detection - Transient updated to 0.3, with new derive macro - Added `Transient` or `Static` implementations for error types - CI should now pass
See catch attribute docs for the new syntax.
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.
Thank you so, so much for this. As you know, this is a feature that's been
requested for quite some time now, so it's exciting to see some real progress
here.
While reviewing this PR, I found myself consistently asking:
- Does this overcome the fundamental technical hurdles that have plagued this
getting this feature into Rocket? - Does this meet the usability requirements we expect?
- Is this feature-complete enough to avoid missing expectation due to missing
features?
I believe the answer to the first question is yes, which is truly exciting!
I temper this statement only because I also believe we're missing some key
usability-impacting features, and there's a concern that the approach won't
extend to these features, but at a first glance it seems probable.
Feature-Completeness
This PR current allows errors from input guards (FromParam
, FromSegments
,
FromForm
, and FromData
) to be type-erased and later downcast into a concrete
type in an error catcher. The critical missing piece is allowing the same for
responders. In particular, we want the following to compile and work as one
might expect:
#[derive(Debug, Transient /* Error */)]
struct MyType<'r>(&'r str);
#[get("/<foo>")]
fn hello(foo: &str) -> Result<&'static str, MyType<'_>> {
Err(MyType(foo))
}
#[catch(default)]
fn error(foo: &MyType) -> &str {
foo.0
}
This means that Responder
needs to change to accommodate an error type.
type Outcome<'o> = Outcome<Response<'o>, E, Status>;
pub trait Responder<'r, 'o: 'r> {
type Error: Debug + 'o;
fn respond_to(self, request: &'r Request<'_>) -> Outcome<'o, Self::Error>;
}
Where:
Outcome::Success(Response)
is akin to today'sOk(Response)
Outcome::Forward(Status)
is akin to today'sErr(Status)
and can be read
as "forwarding to the catcher forStatus
"Outcome::Error(Self::Error)
is new. For many types,Self:Error
will be
set toInfallible
, implying that the responder only succeeds or forwards.
One thing to consider is that we probably want to associate a status with the
error, and we also want to allow the error to respond directly if nothing
catches it.
Here we have two obvious approaches:
-
Require
Responder::Error: Responder
, apply the static dispatch
specialization trick to check forT: Transient
as this PR does. Check that
we don't run into an infinite error-loop (i.e, a responder that returns
itself as an error which then errors with itself, and so on). -
Create a new trait,
Error
, and apply the static dispatch specialization
trick to check forT: Error
. The trait might look something like:trait Error<'r>: Transient + .. { fn respond_to(&self, request: &'r Request<'_>) -> Result<Response<'r>, Status> { Err(self.status()) } /// Maybe? Ignore for now. fn status(&self) -> Status { Status::InternalServerError } /// Log on catcher failure: expected type Foo, have error Bar. fn type_name(&self) -> &'static str { .. } fn upcast(self) -> Box<dyn Transient<'r>> { .. } fn source(&self) -> Option<Box<dyn Error<'r>>> { .. } }
It's not obvious to me at this point which approach might be better, or if
there's another approach that's missing here. The main difference is that the
former requires the error type to implement Responder
, which may not be what
we want, and furthermore that the Responder
implementation not vary between
being used as a regular response and as an error. As such, I lean towards the
latter approach.
Usability
My main concerns on the usability front are two-fold:
- the extensions to the
#[catch]
macro. In particular, I'd like to find a
way to remove the need for additional attribute arguments ofstatus
and
perhaps evenerror
. - the difficulty in implementing custom downcastable errors
#[catch]
In other attributes, like #[route]
, we need attribute parameters because
either they provide information about what to parse (path, segments, and query
params) or they operate in a fundamentally different manner at the type-system
level: (data and request). There's also a security and category-theoretical
argument to be made about creating different class types for parseable
parameters.
But with status
and error
, none of these argument need necessarily apply:
all of the information derives from references or copy-able types without and
particular security considerations. So the only reason* (there's one reason we
might want an error
argument, actually) for the arguments to exist is for our
sake in implementing codegen.
More concretely, the #[catch]
attribute separates types into three categories:
T: FromRequest
T: Transient
viaerror =
Status
viastatus =
I argue that in reality, these should all be one:
-
T: FromError
trait FromError<'r> { fn from_error(request: &'r Request<'_>, error: Option<ErasedError>); }
Then we can recover all of the obvious implementations:
impl<'r> FromError<'r> for Status {
fn from_error(request: &'r Request<'_>, error: Option<ErasedError>) {
error.as_ref().map_or(Status::InternalServerError, |e| e.status())
}
}
Implementing Custom Errors
Consider the following complete program:
use rocket::*;
use rocket::request::{self, Request, FromRequest};
use rocket::outcome::Outcome;
use rocket::http::Status;
#[derive(Debug, transient::Transient)]
struct MyError<'r>(&'r str);
#[derive(Debug)]
struct MyGuard;
#[rocket::async_trait]
impl<'r> FromRequest<'r> for MyGuard {
type Error = MyError<'r>;
async fn from_request(req: &'r Request<'_>) -> request::Outcome<Self, Self::Error> {
Outcome::Error((Status::InternalServerError, MyError(req.uri().path().as_str())))
}
}
#[get("/")]
fn index(guard: MyGuard) -> &'static str {
"Hello, world!"
}
#[catch(default, error = "<foo>")]
fn error<'a>(foo: &MyError<'a>) -> &'a str {
foo.0
}
#[launch]
fn rocket() -> _ {
rocket::build()
.mount("/", routes![index])
.register("/", catchers![error])
}
There are a few issues here:
- I needed to depend on
transient
directly or else the derive wouldn't work
since it referencestransient::
. This shouldn't need to be the case. - The error catcher fails at runtime because, for whatever reason, the error
typeMyError
is not meeting the specialization requirements. I would
expect that if my type doesn't meet the requirements to be an error, then
I wouldn't be allowed to use it as typed error at all, and my program would
fail to compile. - Covariance needs to be declared: can we figure it out automatically, i.e,
via by callingrustc
, or for references syntactically (&T)?
This also begs the question: what exactly is required to be implemented of error
types? Transient
does not appear to be sufficient. This further supports the
idea of a Rocket-specific Error
trait that has all of the requirements
built-in. And if Error
is a Rocket trait, then we can re-export the derive
to allow users to do the following, completely relegating transient
to an
implementation detail.
#[derive(rocket::Error)]
struct MyError<'r> { .. }
As an aside, the following gives an error message that we should look to improve:
#[derive(Debug, Responder, Transient)]
struct MyType<'r>(&'r str);
#[catch(default, error = "<foo>")]
fn error<'a>(uri: &Origin, foo: &'a MyType) -> &'a str {
foo.0
}
core/lib/src/catcher/catcher.rs
Outdated
fn rank(base: Path<'_>) -> isize { | ||
-(base.segments().filter(|s| !s.is_empty()).count() as isize) | ||
-(base.segments().filter(|s| !s.is_empty()).count() as isize) * 2 | ||
} |
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 feels brittle. What we're doing here is partitioning catchers into typed and untyped variants. If that's the case, then we should do so directly. May I suggest:
fn rank(base: Path<'_>, typed: bool) -> isize {
let length = base.segments().filter(|s| !s.is_empty()).count();
let order = (length * 2) + (typed as usize);
-(order as isize)
}
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 logic has been fully removed. The preference for typed catchers is now implemented in lifecycle.rs, since it also needs to check the error's .source()
method, it now always checks for a matching type before a catcher with any type.
@@ -187,6 +191,7 @@ impl Catcher { | |||
name: None, | |||
base: uri::Origin::root().clone(), | |||
handler: Box::new(handler), | |||
error_type: None, | |||
rank: rank(uri::Origin::root().path()), |
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.
Then this becomes rank(uri, false)
.
@@ -141,7 +141,9 @@ impl Catcher { | |||
/// assert!(!a.collides_with(&b)); | |||
/// ``` | |||
pub fn collides_with(&self, other: &Self) -> bool { | |||
self.code == other.code && self.base().segments().eq(other.base().segments()) | |||
self.code == other.code && | |||
types_collide(self, other) && |
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 doesn't feel right. It doesn't look like matching was changed, so it's almost certainly incorrect for collision checking to be changed. When we look at the rules from #2790 (comment), and in particular:
...we see that this breaks the rule since two catchers can match but not collide, i.e., when two types have the same base and code but differently typed errors, we get a match for both no collision which leads to a routing ambiguity.
- Add Error trait - Use Option<Box<dyn Error>> - Update Responder to return a result - Update all core implementations
- Update Responder derive to match new trait - Update lifecycle to deal with error types correctly - Update catcher rank to ignore type - now handled by lifecycle
Still has several major missing features, but is back to passing CI.
- The previous `on_request` was unsafe, now uses a separate `filter_request` method to ensure `error` only contains immutable borrows of `Request`. - Added full safety comments and arguements to `erased`. - Added extra logic to upgrade, to prevent upgrading if an error has been thrown.
@SergioBenitez I've made some significant progress, and I've found a couple ways to improve the API. I wanted to put them in writing, both so you can comment on them sooner rather than later, and for future reference. The 'final' APII've opted to remove The final form of the // `AsAny` is a sealed/blanket impl for a `fn as_any(&self) -> &dyn Any`
pub trait TypedError<'r>: AsAny<Inv<'r>> + Send + Sync + 'r {
/// Generates a default response for this type (or forwards to a default catcher)
fn respond_to(&self, request: &'r Request<'_>) -> Result<Response<'r>, Status> {
Err(self.status())
}
/// A descriptive name of this error type. Defaults to the type name.
fn name(&self) -> &'static str { std::any::type_name::<Self>() }
/// The error that caused this error. Defaults to None.
///
/// # Warning
/// A typed catcher will not attempt to follow the source of an error
/// more than (TODO: exact number) 5 times.
fn source(&'r self) -> Option<&'r (dyn TypedError<'r> + 'r)> { None }
/// Status code
fn status(&self) -> Status { Status::InternalServerError }
} It should be fairly obvious what these methods get used for. One change, is that Remaining decisionsIs With this in mind, if it isn't too much of a burden, we might be able to eliminate the specialization trick, since each of these error types would be required to implement The main alternative would be to require some kind of I'm also considering removing Potential issuesThe We likely need to provide error wrappers for the contrib db crates, since there really isn't a good way to implement |
Addresses #749, adds support for typed catchers via the
transient
crate.When a
FromRequest
,FromParam
,FromQuery
, orFromData
implementation returns anError
orForward
, if the returned error implementsAny<Co<'r>>
, it is boxed and travels with theOutcome
.A catcher can have a third parameter, now
(&E, Status, &Request)
, whereE
is some type that implementsAny<Co<'r>>
. If the error contained in theOutcome
is of the same type, it is downcast tothe specific type, and passed to the catcher.
A working example can be seen in the
error-handling
example - simply visit/hello/name/a
to see aParseIntError
returned by theFromParam
ofi8
.TODO
FromError
trait, and aerror = "<error>"
parameter for the upcast error.Option<TypeId>
, and use it to prevent false collisions.TypeId
TypedError
traitResponder
should return anOutcome<..., Self::Error, ...>
Any<Co<'r>>
should beAny<Inv<'r>>
. Transient actually already protects us from castingAny<Inv<'r>>
to a differentAny<Inv<'l>>
, by telling the compiler thatInv<'a>
is invariant wrt'a
.Transient
, to simply using the wrong error type in the catcher. I'm not sure what the right way to present this is.Transient
, since many types don't).type_name
isn't a stable name - nevermind, it is, since the test is compiled with the same version).TypedError
derive macro.Transient
implementation at the same time.rocket::form::Errors
(and it's parts) should implement Transient. (Now non-issue)FromError
traitTypedError
)TypedError
.TypedError
, and responder/fairing impls need to be updated.