Allow any error type in handler result

[djc]

r? @valkum

[codecov]

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (f08ac8c) 57.38% compared to head (4d56552) 57.43%.

❗ Current head 4d56552 differs from pull request most recent head 4bf5dbd. Consider uploading reports for the commit 4bf5dbd to get more accurate results

Files	Patch %	Lines
mendes/src/application.rs	42.10%	11 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #89      +/-   ##
==========================================
+ Coverage   57.38%   57.43%   +0.05%     
==========================================
  Files           7        7              
  Lines        1368     1365       -3     
==========================================
- Hits          785      784       -1     
+ Misses        583      581       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[valkum]

See my PR against this branch for tests and usage. This is a good step, but I think we should do more to improve ergonomics.

[djc]

See the second commit. That would get us in trouble with Epoxide, though, which has this impl:

impl FromContext<'_, App> for Session {
    fn from_context(
        app: &'_ Arc<App>,
        req: &'_ Parts,
        _: &mut mendes::application::PathState,
        _: &mut Option<<App as Application>::RequestBody>,
    ) -> Result<Session, Error> {
        app.cookie::<Self>(&req.headers)
            .ok_or_else(|| Error::forbidden("not logged in"))
    }
}

I guess we could add a mendes::Error variant for Cookie(cookies::Error)?

[valkum]

I think this would limit the usefulness of the FromContext trait, as you can't provide your own Error variant anymore.
What if we would change the expanded version of handler?

Currently it expands to

pub(super) async fn handler(
        cx: &mut mendes::application::Context<App>,
    ) -> Result<Response<String>, HandlerError> {
        match &cx.req.method {
            &mendes::http::Method::GET => {}
            _ => return Err(mendes::Error::MethodNotAllowed.into()),
        }
        let _0 = <&App as mendes::FromContext<
            App,
        >>::from_context(&cx.app, &cx.req, &mut cx.path, &mut cx.body)?;
        call(_0).await
    }

What if we would change the signature of handler() to
-> Result<Response<String>, Response<String>> or similar?
We would keep the signature of call() so that the implemented function can limit/use its local Error type, but the generated handler basically can emit two kinds of errors by turning them directly into a response (A::Error and the local error emitted by call).

On the other side, keeping it this way, forcing the user to implement From<A::Error>, would be more explicit. Especially for the use case we currently face. API endpoints and browser/redirect endpoints.

[djc]

On the other side, keeping it this way, forcing the user to implement From<A::Error>, would be more explicit. Especially for the use case we currently face. API endpoints and browser/redirect endpoints.

All other things being equal, this feels preferable to me?

[valkum]

All other things being equal, this feels preferable to me?

I agree. Do you want me to update the PR test adds the tests so that PR can be merged into this PR or do you want to merge this first and merge the test PR into main afterwards?

[djc]

I agree. Do you want me to update the PR test adds the tests so that PR can be merged into this PR or do you want to merge this first and merge the test PR into main afterwards?

I don't really care -- the state on main will be the same either way after both get merged, right? If you agree that this PR does the right thing per your tests, then let's merge this and merge your PR right after.

[valkum]

I think there is a misunderstanding. I think with

On the other side, keeping it this way, forcing the user to implement From<A::Error>, would be more explicit. Especially for the use case we currently face. API endpoints and browser/redirect endpoints.

I was referencing the state without the Restrict FromContext Result to mendes::Error commit.
Having to explicitly handle mendes::Error in your impl IntoResponse for your local error type is a good thing, I believe. You would have full control over the serialization of that any errors caused by FromContext.
In the RedirectError use case, we have the option to either return mendes::Error as a redirect or as a static error page.

Edit: And you keep the freedom to provide your own Error variants for custom FromContext implementations.

[djc]

Alright, that makes sense to me. I'll see what happens if I plug this into bolt before we merge it.

[djc]

Seems to just work in bolt. Merging!