Error messages are not returned

[KoltesDigital]

Hi,

Given the following app:

async fn handle(req: Request<()>) -> tide::Result {
	Err(Error::from_str(
		StatusCode::BadRequest,
		"Body validation failed because of ...",
	))
}

app.at("/").post(handle);

POST requests will fail, but the error message won't be sent. I expect that they should, because they can contain valuable information, e.g. related to validation failures. I've seen no mechanism to switch that. Note that I'm developing an API, so responses can be brief, of course for a public-facing 404 error page it makes sense to have an actual Response with some HTML template.

Perhaps such messages shouldn't be Err but Ok values, but then, why do Error::from_str take these two arguments where only the former is taken into account?

FWIW I've done several servers in NodeJS with express, and I find this library to have sensible default behaviors, which is to display the error message, adding details (stacktrace) only in development environment, and all of that can be overridden by error middlewares. I haven't seen these features in Tide, what do you think about them?

[mdtusz]

This seems to be a relatively common confusion - because the Err is returned, the error is treated as a server-error (i.e. 500 level) and is not returned to the client to prevent leaking sensitive information. This has bitten me and led to writing a custom middleware handler that re-writes errors of specific types to be client errors wrapped in an Ok (e.g. data validation errors), but I think it may be worthwhile to modify the way tide deals with errors in general to be a bit more "expected".

I believe @Fishrock123 has a logging middleware that deals with this as well (for json response errors) so it would be nice to re-evaluate error handling and maybe make that a part of the tide core.

[KoltesDigital]

@mdtusz this is half true. To be more accurate: given the above code, the status code is indeed 400. But the body is empty. So I'm confused, why the first argument "leaks through" but not the second?

Express used to support this code:

res.send(400, "Validation failed ...");

and Err::from_str has the same signature, hence my expectation.

On the other side, and I didn't think about it before I wrote it on my previous message, Response is designed to handle errors as well:

Ok(Response::builder(StatusCode::BadRequest).body("Validation failed ...").into())

which is FWIW close to the actual express equivalent:

res.status(400).send("Validation failed ...");

If Errs are meant for server-side errors, maybe the real issue here is that any status code is accepted, whereas only 5xx should be? I don't see a way to force that at compile-time because all status codes are inside the same enum, but at least a panic would have hinted me that my error responses are actually Ok values.

Another fallacy is that it's easier to early return Errs thanks to ?. Say the validation block should now return a Result<(), ValidationError> which must be handled like match validate() { Err(err) => Ok(...) ... which is somewhat disturbing. Or to factorize even more, the validation block could return an Option<Response>, where the handling of validated requests occurs in the or_else, which is also disturbing.

[KoltesDigital]

I've finally found the error handling example. However I'm not fan of dynamic casts. I prefer to send upfront the proper response, rather than relying on post-process transform.

Another issue I stumbled across is that, while handlers are added through objects implementing Endpoint, it's impossible to overwrite its implementation for Fn. This means that the usual to write handlers with lambdas has to return a Result with an http::Error as error type.

I imagine two solutions:

1. default feature e.g. "fn-endpoint" to enable or disable this implementation, so that one could have its own Result from handlers.
2. tide should not expect a Result<Response, http::Error> but a Result<Response, E = http::Error> where E: Into<Response> so that the current behavior is kept by default, but one could define their own error type with response creation.

Both could be done, but for solving my issue, I prefer having the latter only. It's such a tiny change and I think it's not a breaking change.

What do you think about that?

[mdtusz]

Agreed it feels a bit strange matching on an error and converting it back into an Ok result.

@jbr @Fishrock123 thoughts on the comments above? I know there's a few issues (and maybe PR's) open around error handling so may be good to consolidate them and re-evaluate options on a way to address some concerns.

The second suggestion seems reasonable to me, but I could imagine this causing issues for some existing middlewares that are out there. It would definitely be very useful to be able to return custom domain-specific error types that can be converted into a proper http error response though (without having to write a specific case in a middleware handler for each).

[jbr]

I think this is a @yoshuawuyts question. My personal opinion is that handlers should necessarily return Response, but that's not at all tide's design. Unless Yosh says otherwise, my guess is that tide's design will remain exactly as it is currently until the language changes