Rework middleware to receive Response directly, add optional errors to Response #570

Fishrock123 · 2020-06-05T05:41:01Z

Rework middleware so that:

next.run().await always returns a Response.
middleware return of tide::Result is always transformed into a Response with error set.

Adds the following things to Response:

pub fn error(&self) -> Option<&Error>
pub fn downcast_error<E>(&self) -> Option<&E>
pub fn take_error(&mut self) -> Option<Error>
impl From<Error> for Response
impl From<crate::Result> for Response

An example of this being used for an error handling middleware is included at examples/error_handling.rs, and Jacob has transformed someone else's in #452 (comment).

Dependencies:

~~[x] Merge Response: add Error storage, retrieval, conversion http-types#174 (ABI breaking)~~
~~[ ] http-types release~~
~~[ ] http-client release~~
~~[ ] Surf release~~
Response: remove #[must_use] for set_status() #612

Note: Please let me rebase before merging

jbr · 2020-06-06T00:14:05Z

@Fishrock123 i added a commit at Fishrock123#1 that gets this pr compiling

examples/error_handling.rs

Fishrock123 · 2020-06-09T20:29:42Z

Here is what this could enable in the future once the Try trait stabilizes: Fishrock123#2

yoshuawuyts

Overall this is looking really good! -- left a few nits; but this is def in the right direction. Thanks so much @Fishrock123!

src/response.rs

src/route.rs

src/router.rs

.github/workflows/ci.yaml

src/middleware.rs

src/endpoint.rs

src/route.rs

prabirshrestha · 2020-06-21T19:45:28Z

examples/error_handling.rs

+    tide::log::start();
+    let mut app = tide::new();
+
+    app.middleware(After(|mut res: Response| async move {


should After take both request and response? For example I might want to read a request headers and modify the response as appropriate. One example is taking the client request header from request and setting it to response header.

Unfortunately that's not possible currently with After. If you need access to the request and the response, you'll need to write a full middleware. It would be misleading for After to provide a request, because it would be a clone, and potentially the "real" request could have changed after the clone was made, but that would be obfuscated by the After.

Fishrock123 · 2020-06-21T20:49:12Z

~~ci blocked due to #612~~

This allows errors to be propogated through the Tide middleware stack while still keeping an existing Response intact. Effectively allowing headers, body, etc to be passed along with an Error; while also opening up options for nicer error handling via `res.downcast_error<>` checking. Also contains APIs for turning an Error into a Response via Into, and taking the error from / setting an error on an existing Response. This is a breaking change in that `next.run().await` within middleware no longer returns a tide::Result but rather always returns a tide::Response. Thanks to Jacob Rothstein for helping me get this to compile! PR-URL: http-rs#570

Fishrock123 · 2020-06-22T19:02:18Z

I believe this is just waiting on downstream releases now. I've rebased the commits nicely.

examples/error_handling.rs

src/response.rs

src/server.rs

This allows errors to be propogated through the Tide middleware stack while still keeping an existing Response intact. Effectively allowing headers, body, etc to be passed along with an Error; while also opening up options for nicer error handling via `res.downcast_error<>` checking. Also contains APIs for turning an Error into a Response via Into, and taking the error from / setting an error on an existing Response. This is a breaking change in that `next.run().await` within middleware no longer returns a tide::Result but rather always returns a tide::Response. Thanks to Jacob Rothstein for helping me get this to compile! PR-URL: http-rs#570

yoshuawuyts

One last comment, but otherwise this is looking 💯

src/response.rs

Refs: http-rs#570 (comment)

Fishrock123 · 2020-06-25T19:51:00Z

Addressed that, removed From<Result> for Response.

tirr-c

This approach seems better for middleware. Thanks!

yoshuawuyts

Patch LGTM!

http-rs/http-types#191 however was recently reverted, and this patch needs to be updated to bring the error types into Tide itself. That should allow us to move off the git deps, and allow us to merge. Excited for this!

This allows errors to be propagated through the Tide middleware stack while still keeping an existing Response intact. Effectively allowing headers, body, etc to be passed along with an Error; while also opening up options for nicer error handling via `res.downcast_error<>` checking. Also contains APIs for turning an Error into a Response via Into, and taking the error from / setting an error on an existing Response. This is a breaking change in that `next.run().await` within middleware no longer returns a tide::Result but rather always returns a tide::Response. Thanks to Jacob Rothstein for helping me get this to compile! PR-URL: http-rs#570

Refs: http-rs#570 (comment)

This allows errors to be propagated through the Tide middleware stack while still keeping an existing Response intact. Effectively allowing headers, body, etc to be passed along with an Error; while also opening up options for nicer error handling via `res.downcast_error<>` checking. Also contains APIs for turning an Error into a Response via Into, and taking the error from / setting an error on an existing Response. This is a breaking change in that `next.run().await` within middleware no longer returns a tide::Result but rather always returns a tide::Response. Thanks to Jacob Rothstein for helping me get this to compile! PR-URL: http-rs#570

Makes use of the newly introduced error propgation from `Response: add optional error storage` to show a nice and flexible error handler middleware approach. Related to http-rs#549

Makes use of the newly updated error propgation from `Response: add optional error storage` for a more informative error case in the log middleware.

This is probably never what a Tide user wants. The developer of a server can use the log middleware, otherwise this presents a risk as it may point to potentially exploitable flaws. Additionally, Tide can't completely aguarentee this will be in an expected format.

Refs: http-rs#570 (comment)

Fishrock123 · 2020-07-07T22:27:35Z

Rebased and pulled the reverted logic from http-types (http-rs/http-types#174) up into here. I believe this is ready to go.

This was referenced Jun 7, 2020

Ergonomics issue with enabling ? in endpoint #452

Open

Error handler middleware with error storage jbr/tide#1

Closed

Fishrock123 force-pushed the new-http-types-errors branch from 5d9b539 to a1cb10e Compare June 8, 2020 23:15

Fishrock123 commented Jun 9, 2020

View reviewed changes

examples/error_handling.rs Outdated Show resolved Hide resolved

Fishrock123 changed the title ~~wip on response-optional-error~~ Rework middleware to receive Response directly, add optional errors to Response Jun 9, 2020

This was referenced Jun 10, 2020

Error handler middleware #549

Closed

Proposal: Return Into<Response> from endpoints #596

Closed

Fishrock123 mentioned this pull request Jun 15, 2020

Rework middleware error handling http-rs/surf#189

Open

jbr added blocked An issue that's blocked on upstream changes design Open design question enhancement New feature or request labels Jun 18, 2020

yoshuawuyts approved these changes Jun 21, 2020

View reviewed changes

prabirshrestha reviewed Jun 21, 2020

View reviewed changes

Fishrock123 force-pushed the new-http-types-errors branch from a1cb10e to f2d1608 Compare June 21, 2020 20:44

Fishrock123 force-pushed the new-http-types-errors branch 3 times, most recently from 7c9e426 to 9fd5722 Compare June 22, 2020 04:33

Fishrock123 force-pushed the new-http-types-errors branch from a031ad3 to cd9568a Compare June 22, 2020 19:01

Fishrock123 marked this pull request as ready for review June 22, 2020 19:02