could use customisation of error presentation

[jclulow]

One imagines many dropshot servers will be straight REST APIs, where clients are just libraries or commands and uniform JSON error messages make sense. Some, however, may also serve other artefacts; e.g., a web server that had an API component, but also an interactive HTML component that a browser may access with some pages and images and such.

Presently dropshot will produce a stock uniform JSON error response for, say, endpoints that do not map to a handler. If the client sends an Accept header that suggests they understand or prefer a HTML response, it'd be good to be able to choose somehow to render a reasonable 404 page or event a 302 redirect to the right place for browsers.

[ahl]

Great point. @davepacheco and I were discussing either a ignore = true option to the endpoint annotation or having two annotations: one for APIs and one for artifacts. I could imagine we might want some different options for each. Do you have an opinion?

[davepacheco]

See also #40. I kind of like the idea of separate ApiDescription objects on the same server.

[david-crespo]

This is needed for oxidecomputer/omicron#3333, which would do the mentioned return HTML if Accept=HTML thing in Nexus. Current 404 response generation code is here:

dropshot/dropshot/src/error.rs

Lines 217 to 267 in c719b41

	/// Generates an `HttpError` for a 404 "Not Found" error with a custom
	/// internal message `internal_message`. The external message will be "Not
	/// Found" (i.e., the standard label for status code 404).
	pub fn for_not_found(
	error_code: Option<String>,
	internal_message: String,
	) -> Self {
	let status_code = http::StatusCode::NOT_FOUND;
	let external_message =
	status_code.canonical_reason().unwrap().to_string();
	HttpError {
	status_code,
	error_code,
	internal_message,
	external_message,
	}
	}
	/// Generates an HTTP response for the given `HttpError`, using `request_id`
	/// for the response's request id.
	pub fn into_response(
	self,
	request_id: &str,
	) -> hyper::Response<hyper::Body> {
	// TODO-hardening: consider handling the operational errors that the
	// Serde serialization fails or the response construction fails. In
	// those cases, we should probably try to report this as a serious
	// problem (e.g., to the log) and send back a 500-level response. (Of
	// course, that could fail in the same way, but it's less likely because
	// there's only one possible set of input and we can test it. We'll
	// probably have to use unwrap() there and make sure we've tested that
	// code at least once!)
	hyper::Response::builder()
	.status(self.status_code)
	.header(
	http::header::CONTENT_TYPE,
	super::http_util::CONTENT_TYPE_JSON,
	)
	.header(super::http_util::HEADER_REQUEST_ID, request_id)
	.body(
	serde_json::to_string_pretty(&HttpErrorResponseBody {
	request_id: request_id.to_string(),
	message: self.external_message,
	error_code: self.error_code,
	})
	.unwrap()
	.into(),
	)
	.unwrap()
	}
	}

[david-crespo]

To my JavaScript-pilled brain, this looks like an optional Dropshot config param that's a function with a signature like: (HttpError) -> hyper::Response<hyper::Body>. Obviously you can't put that in TOML. Then the above handler would use it if present, otherwise fall back to the default implementation. Maybe the function would also return an Option in order to be able to signal to Dropshot that it should fall back to the default implementation.

[davepacheco]

I discussed a few approaches offline with @jclulow and @ahl:

1. Similar to what @david-crespo suggested above, allow registering a function (or trait impl) whose semantics are: any time request handling resulted in an HttpError that Dropshot is going to serialize out, you get a chance to return your own response instead.
2. Relax the definition of API endpoint functions so that you don't have to return HttpError as the error type but can instead return your own error type that impls JsonSchema and Serialize (just like the way the success type works today).
3. Allow consumers to register a function that gets invoked whenever Dropshot encounters an error that didn't come from the route handler (e.g., a 404 or 405 during routing) and allows them to produce their own response. This function would accept some new enum type describing all the Dropshot-internal errors, similar to what's mentioned in error codes for dropshot-provided errors need work #41.

The choices are basically either (1) or (2)+(3). Neither (2) nor (3) alone is enough to deal with this.

(1) sounds simple but has a pretty big downside, which is that you lose a lot of information about the underlying problem. Say you're in the middle of a request handler and you have a very specific problem with a lot of information about it. With (2), you can return your own Error type and have all that information available when constructing the response -- and you could even include it in the OpenAPI spec if you wanted. This could be really useful for validation errors where the client wants to know more about what went wrong (e.g., which field name was wrong). With (1), you lose all that information.

@ahl, @jclulow, and I all prefer (2) + (3). There are no immediate plans to do it, but if someone wants to address this, that looks like the way to go.

(2)+(3) also completely solves #41. Consumers would be completely in control of the returned error responses so they could use the same codes, their own codes, or whatever.

Edit: this approach would also solve #801, too.

[hawkw]

I think I might take a crack at implementing the proposed solutions (2) and (3) described in #39 (comment), since this has come up a few times recently and it would be nice to have.

One thing I don't see discussed in @davepacheco's comment is how HTTP status codes for errors would be determined. The approach that comes to my mind immediately is to also require that error types implement a trait that returns an http::StatusCode in addition to Serialize + JsonSchema. The existing HttpError could implement such a trait, so this should be backwards compatible with existing code, I think.

[hawkw]

This is starting to look like it will also involve a reasonably big change to introduce more structured error types to dropshot's internals, in support of point (3) in the above comment from Dave:

Allow consumers to register a function that gets invoked whenever Dropshot encounters an error that didn't come from the route handler (e.g., a 404 or 405 during routing) and allows them to produce their own response. This function would accept some new enum type describing all the Dropshot-internal errors, similar to what's mentioned in #41.

I was hoping to be able to make those changes separately from allowing handlers to return errors other than HttpError, but I think it'll end up having to be one PR, since dropshot-internal errors may also be returned from handler functions when the handler calls into Dropshot APIs, and it seems like it would be nice to allow the user-defined return type error to be constructed from the dropshot type. The actual plumbing to register an error handler function for stuff that occurs outside of an endpoint could be added later, though.

[hawkw]

One design question that's come up while working on this: for the purposes of OpenAPI schema generation, do we think it's reasonable to assume/require that all Errs returned by handler functions will have 4xx/5xx status codes? This would make it easier to generate an OpenAPI document where the generated client can disambiguate between Ok and Err responses in the case where the response type doesn't have a known status code. For example, if we could make that assumption, a handler like

#[endpoint(...)]
async fn foo(...) -> Result<Foo, FooError> {
    // ...
}

(where Foo implements JsonSchema + Serialize but is not one of dropshot's known-status code wrapper types) could generate a schema like:

      responses:
        "4xx":
          content:
            application/json:
              schema:
                $ref: "#/components/schemas/FooError"
        "5xx":
          content:
            application/json:
              schema:
                $ref: "#/components/schemas/FooError"
        default:
          content:
            application/json:
              schema:
                $ref: "#/components/schemas/Foo"

This allows generated client code to determine whether to expect the Foo or FooError schemas based on the status.

Otherwise, if we want to allow Err return values to generate any status code, we can't easily do that; instead, we need to produce something more like:

      responses:
        default:
          content:
            application/json:
              schema:
                oneOf:
                 - $ref: "#/components/schemas/Foo"
                 - $ref: "#/components/schemas/FooError"

in which case, the client will just have to try to parse the response as both. Or, we could generate a Result<Foo, FooError> schema, and use a discriminator to indicate to the client whether it should try to parse the response as the Ok schema or the Err schema, but that means that the endpoint no longer just returns the JSON representation of the Ok type in the success case, instead wrapping it in a Result, which seems kinda unfortunate.

Unfortunately, there isn't really a nice way to enforce that error types that implement a trait that returns http::StatusCode will only return a 4xx/5xx, besides asserting that the return type is that and introducing a sad potential runtime panic. Alternatively, we could introduce our own ErrorStatusCode type which only represents 4xx/5xx statuses, and instead require that errors return that. But that introduces a lot of unfortunate code duplication and makes the API a little awkward for users, because they can no longer just return the http::StatusCode type in implementations of the error trait, despite it being used everywhere else in dropshot...

[epompeii]

Alternatively, we could introduce our own ErrorStatusCode type which only represents 4xx/5xx statuses, and instead require that errors return that. But that introduces a lot of unfortunate code duplication and makes the API a little awkward for users, because they can no longer just return the http::StatusCode type in implementations of the error trait, despite it being used everywhere else in dropshot...

Could we create our own type, ErrorStatusCode and require impl Into<ErrorStatusCode>. Then go ahead and implement the From for http:StatusCode. If for some reason we get a non-4xx/5xx just map the status code to something like status code 520, instead of a runtime panic.

[ahl]

One design question that's come up while working on this: for the purposes of OpenAPI schema generation, do we think it's reasonable to assume/require that all Errs returned by handler functions will have 4xx/5xx status codes?

Yes. While--yes--this is reflective of opinionation rather than constraint, Dropshot is intended to be opinionated! Saying that Err equates to some "error" response code seems fine!

Or, we could generate a Result<Foo, FooError> schema, and use a discriminator to indicate to the client whether it should try to parse the response as the Ok schema or the Err schema, but that means that the endpoint no longer just returns the JSON representation of the Ok type in the success case, instead wrapping it in a Result, which seems kinda unfortunate.

1. I don't think the discriminator is particularly necessary here (indeed, I don't think it's generally necessary, and we ignore it in progenitor)
2. This approach would seem to leak quite a bit of Rust-iness into the OpenAPI doc.

Unfortunately, there isn't really a nice way to enforce that error types that implement a trait that returns http::StatusCode will only return a 4xx/5xx, besides asserting that the return type is that and introducing a sad potential runtime panic. Alternatively, we could introduce our own ErrorStatusCode type which only represents 4xx/5xx statuses, and instead require that errors return that.

I think there may be some merit to that. This approach may--for example--be a relevant approach to address #693.

[hawkw]

@davepacheco suggested that, as a middle ground, we could also have dropshot return a 500 if a user-defined error type returns a status that isn't an error. That's less violent than panicking, but still allows users to return http::StatusCodes if they promise they'll be good about it.