error codes for dropshot-provided errors need work

[davepacheco]

As it says in error.rs, we envision that errors from an API typically have a string code (separate from the HTTP status code):

dropshot/dropshot/src/error.rs

Lines 52 to 69 in 308be87

	* `HttpError` represents an error generated as part of handling an API
	* request. When these bubble up to the top of the request handling stack
	* (which is most of the time that they're generated), these are turned into an
	* HTTP response, which includes:
	*
	* * a status code, which is likely either 400-level (indicating a client
	* error, like bad input) or 500-level (indicating a server error).
	* * a structured (JSON) body, which includes:
	* * a string error code, which identifies the underlying error condition
	* so that clients can potentially make programmatic decisions based on
	* the error type
	* * a string error message, which is the human-readable summary of the
	* issue, intended to make sense for API users (i.e., not API server
	* developers)
	* * optionally: additional metadata describing the issue. For a
	* validation error, this could include information about which
	* parameter was invalid and why. This should conform to a schema
	* associated with the error code.

I've generally assumed that the namespace of these codes needs to be under the control of the Dropshot consumer, since they will likely be defining most of the codes for their own application-specific conditions. However, Dropshot currently generates a bunch of different errors of its own (e.g., when a request arrives for a path that has no handler associated with it). What code should it use?

I'm thinking out loud through a bunch of options here:

1. Have Dropshot define a set of codes for itself and let consumers use whatever codes they want, too. This kind of sucks. It's hard to expand Dropshot's set later since we might stomp on a code that consumers are already using. We could use a prefix to carve out part of the namespace, but this is part of the consumer's public API -- that's ugly. Plus, it's hard for Dropshot to make judgments about what error cases a consumer's client wants to distinguish.
2. Let the consumer define the entire namespace and allow consumers to define the codes that Dropshot will use for its own errors. Maybe the caller provides a function that maps HTTP status codes to Strings (or something that we can turn into a String). Or maybe Dropshot defines an enum for the conditions it needs to generate codes for and consumers provide a function that maps from that. We could even provide a default implementation that would hopefully be suitable for most consumers. This gives us most of the benefits of (1) but without the expansion problem -- if we expand it, if the consumer uses a match, they'll get a compile error that forces them to decide how to map the new condition.
3. Have Dropshot own the entire namespace: create a set of codes like ObjectNotFound, BadArgs, etc. and require that consumers use these. This might actually be nice because many consumers will wind up using a lot of the same codes, but it seems like a non-starter that consumers can't extend this.

The behavior today is that the code is optional and Dropshot generally provides None. That was basically a hack to be able to make forward progress -- it's pretty crappy for consumers and their users.

See also #39.

[raycar5]

I have implemented Rust APIs like these that are running in production so I'll give my grain of salt.

I think something along the lines of (2) is a good solution, Dropshot provides 2 error types, one DropshotError that it generates internally when an error happens inside of Dropshot code and one HTTPError that represents any HTTP response with a non 2xx status code. Dropshot should provide a default mapping between these 2 error types but a user should be able to change it, probably on a route by route basis (The same error can mean different things depending on context).

Which leads me to another suggestion about errors that I think is pretty valuable, I think users shouldn't be encouraged to implement Into<HTTPError> traits for their MyAppError types. What kind of status code and error message you want to send is usually more related to where an error happens in the handler rather than what the error is. A much better pattern is to have a trait implemented for the Result<T,MyAppError> type that allows you to explicitly map a Result<T,MyAppError> to Result<T,HTTPError>.

For example:

use std::fs;
use std::io;
// Simplification, missing headers etc.
struct HTTPResponse {
    code: u16, // Simplification, use enum.
    body: Vec<u8>, // Simplification, use serializable object. 
}
// Simplification, you might want to assert that the status code is non 2xx.
type HTTPError = HTTPResponse;

#[derive(Debug)]
enum MyAppError {
    IO(io::Error),
    OtherError { cause: String, code: u16 },
}

type MyAppResult<T> = Result<T, MyAppError>;
impl From<io::Error> for MyAppError {
    fn from(error: io::Error) -> Self {
        Self::IO(error)
    }
}

// This trait needs to be defined by the app itself because Rust doesn't allow implementing
// traits if you don't own the trait or the type.
// However it could be provided by Dropshot using a macro.
trait IntoHttpResultEXT<T> {
    fn with_code(self, code: u16) -> Result<T, HTTPError>;
    fn not_found(self) -> Result<T, HTTPError>;
    // fn internal_error(), etc...
    // You can also have functions that add some additional context about the error.
}

impl<T> IntoHttpResultEXT<T> for MyAppResult<T> {
    fn with_code(self, code: u16) -> Result<T, HTTPError> {
        // At this step you might want to log the error since here is the last point
        // where it containts the most context.
        self.map_err(|err| HTTPError {
            code,
            // Simplification, normally you would have a mapping depending on the kind of MyAppError.
            body: format!("{:?}", err).into_bytes(),
        })
    }
    fn not_found(self) -> Result<T, HTTPError> {
        self.with_code(404)
    }
}

fn handler(path: &str) -> Result<HTTPResponse, HTTPError> {
    // You can imagine a scenario where a different io::Error maps to an internal error
    // instead of not found depending on the call site.
    let file = fs::read(path).map_err(Into::into).not_found()?;
    Ok(HTTPResponse {
        code: 200,
        body: file,
    })
}

[davepacheco]

Thanks for that -- that's helpful. We're largely working on other areas right now but this will be helpful when we get back to it.