Editoast error management

content/docs/reference/design-docs/editoast-errors/_index.en.md

@@ -0,0 +1,320 @@
+---
+title: "Editoast error management"
+linkTitle: "Editoast error management"
+weight: 80
+---
+
+# Goals
+
+- Have a clear separation between logically distinct errors.
+- Dispose of a way to actually match on errors when they occur deeper in the stack
+- Tie the errors to the endpoint they originate from in the OpenAPI.
+- Separate the error definition and emission of their serailization.
+- Statuate on how we want to forward Core's errors.
+- End the `InternalError` hegemony ⚔️
+
+# Constraints
+
+- Keep the same error format.
+- The error must live until the error is handled, conversion to our standard error format only happen in the response serialization.
+- Errors must implement `std::error::Error`.
+- Errors must be composable.
+- Errors variants must be shareable (e.g.: no multiple `InfraNotFound` variants).
+- Each endpoint must provide all its error cases in the OpenAPI. (How the frontend will consume them is another problem that we'll have to deal with.)
+- The `context` field of the error is populated in the views.
+- Errors can be handled in a generic matter (for situations where it makes some sense to do so).
+  - I.e.: some form of downcasting is available.
+
+# New system
+
+- Rely on `thiserror` everywhere.
+- Keep the `trait EditoastError` but only implement it for errors defined in `views`.
+- Rework `derive(EditoastError)` to interface it with `derive(thiserror::Error)`.
+- The `context` is empty by default but can be provided by the `impl EditoastError`. The macro is also able to take context providers.
+- `EditoastError`'s `#[source]`, `#[from]`, `source` and `backtrace` fields are never serialized, unless explicitely provided. This shouldn't be the case as it exposes editoast internals at the API level.
+- All errors must `impl Debug`.
+- `derive(EditoastError)` generates the `impl utoipa::ToSchema for T`.
+- Errors **do not** require to `impl Serialize`.
+- `impl Serialize for T where T: EditoastError` however.
+- Error cases that will be used repeatedly are defined as a `struct` but still `derive(thiserror::Error)`.
+- The `error_type` of each variant is generated by the macro at the format `ErrorTypeName::VariantName`, but can be provided explicitely if conflicts arise.
+
+## Nominal case
+
+```rust
+// in mod views;
+
+fn get_resource(key: Key) -> Result<Resource, GetError> {
+    todo!()
+}
+
+#[derive(Debug, thiserror::Error)]
+pub enum GetError {
+    #[error("ID not found")]
+    IdNotFound { id: u64 },
+    #[error("name not found")]
+    NameNotFound { name: String }
+}
+
+fn process_resource(resource: Resource) -> Result<Computation, ProcessingError> {
+    todo!()
+}
+
+#[derive(Debug, thiserror::Error)]
+pub enum ProcessingError {
+    #[error("Resource is invalid")]
+    InvalidResource { resource: Resource },
+    #[error("Resource is too old")]
+    OutdatedResource { resource: Resource }
+}
+
+#[utoipa::path(
+    ...,
+    responses(
+        (status = 200, body = Computation),
+        (status = 400, body = EndpointError)
+    )
+)]
+async fn endpoint(Path(key): Path<Key>) -> Result<Json<Computation>, EndpointError> {
+    let resource = get_resource(key)?;
+    let value = process_resource(resource)?;
+    Ok(Json(value))
+}
+
+#[derive(Debug, thiserror::Error, EditoastError)]
+pub enum EndpointError {
+    #[error("Resource not found")]
+    #[editoast_error(user)] // <=> status = 400
+    ResourceNotFound(#[from] GetError),
+
+    #[editoast_error(internal)] // <=> status = 500, default, overrides the message to "Internal error - please contact your administrator"
+    ProcessingFailed(#[from] ProcessingError)
+}
+```
+
+## Share errors between crates
+
+Since we require no other constraint that `impl Error` for composition, it's easy to nest errors using `thiserror`.
+
+````rust
+// in editoast_models
+
+#[derive(Debug, thiserror::Error)]
+#[error("postgres error: {0}")]
+struct DbError(#[from] diesel::Error);
+
+// in editoast_redis (if we actually had that crate 👀)
+
+#[derive(Debug, thiserror::Error)]
+#[error("redis error: {0}")]
+struct RedisError(#[from] redis::Error);
+
+// in editoast_views, where diesel isn't available
+
+#[derive(Debug, thiserror::Error)]
+#[error("invalid resource form: {0}")]
+struct FormError(ResourceForm);
+
+#[derive(Debug, thiserror::Error, EditoastError)]
+#[editoast_error(name = "CreateResourceError")] // schema name & error_type
+enum CreateError {
+    #[error("will be overriden, but still useful for development")]
+    #[editoast_error(internal, name = "Database")]
+    Db(#[from] DbError),
+
+    #[error(transparent)] // shown to the user
+    #[editoast_error(user)]
+    InvalidForm(#[from] FormError)
+}
+
+async fn create(...) -> Result<Json<Resource>, CreateError> {
+    todo!()
+}
+
+#[derive(Debug, thiserror::Error, EditoastError)]
+enum UpdateError {
+    #[editoast_error(internal)]
+    Db(#[from] DbError),
+
+    #[editoast_error(internal)]
+    Redis(#[from] RedisError),
+
+    #[error(transparent)]
+    #[editoast_error(user)]
+    InvalidForm(#[from] FormError)
+}
+
+async fn update(...) -> Result<(), UpdateError> {
+    todo!()
+}
+````
+
+## Ease composability
+
+### Nesting `EditoastError`s
+
+We composed errors by nesting them thanks to `thiserror`. However, to compose and reuse `EditoastErrors`, we need a special flag so that when we attempt to serialize the error, we return the serialization of the source error directly.
+
+```rust
+#[derive(Debug, thiserror::Error, EditoastError)]
+#[error("no such infra: {id}")]
+#[editoast_error(context, status = 404)]
+struct InfraNotFound { id: u64 }
+
+#[derive(Debug, thiserror::Error, EditoastError)]
+#[error("unauthorized")]
+#[editoast_error(status = 401)]
+struct Unauthorized;
+
+#[derive(Debug, thiserror::Error, EditoastError)]
+enum EndpointError {
+    NotFound(#[from] #[editoast_error] InfraNotFound),
+
+    Unauthorized(#[from] #[editoast_error] Unauthorized)
+
+    #[error("oh no")]
+    #[editoast_error(user)]
+    Error1,
+}
+```
+
+### Anonymous enums
+
+Since we now have to really manage errors happening in every function **as precisely as possible**, there will likely be a lot of error enums going around. This may be a hassle and wrongfully encourage returning `Option` as an error. To circumvent that, an easy (albeit opinionated) QoL feature would be to use anonmous enums.
+
+````rust
+fn get_resource(id: u64) -> Result<Resource, Enum3<DbError, RedisError, MissingResourceError>> {
+    todo!()
+}
+
+fn process_resource(resource: Resource) -> Result<Computation, Enum2<DbError, ProcessingError> {
+    todo!()
+}
+
+#[utoipa::path(
+    ...,
+    responses(
+        (status = 200, body = Computation),
+        (status = 400, body = EndpointError)
+    )
+)]
+async fn endpoint(Path(key): Path<Key>) -> Result<Json<Computation>, EndpointError> {
+    let resource = get_resource(key)?;
+    let value = process_resource(resource)?;
+    Ok(Json(value))
+}
+
+#[derive(Debug, thiserror::Error, EditoastError)]
+pub enum EndpointError {
+    Db(#[from] DbError),
+
+    Redis(#[from] RedisError),
+
+    #[error(transparent)]
+    #[editoast_error(user)]
+    Missing(#[from] MissingResourceError)
+
+    #[error(transparent)]
+    #[editoast_error(user)]
+    Processing(#[from] ProcessingError)
+}
+````
+
+The implementation of the `EnumX` type would be rather easy to generate for many tuple sizes. The crate [anon_enum](https://docs.rs/anon_enum/1.1.0/anon_enum/) exists but if we choose to use this pattern, it's probably better to have our own type for greater flexibility and avoid another dependency.
+
+## Full `derive(EditoastError)` spec
+
+The macro supports enums, named structs and tuple structs (fixme correct naming).
+
+````rust
+#[derive(thiserror::Error)] // not an EditoastError
+#[error("wrong string: {0}")]
+struct WrongString(String);
+
+#[derive(EditoastError, thiserror::Error)]
+#[error("wrong int: {value}")]
+#[editoast_error(user, name = "InvalidInt")]
+struct WrongInt { value: i64 };
+
+#[derive(EditoastError, thiserror::Error)]
+#[error("my error type")]
+#[editoast_error(context)]
+enum MyError {
+    // error_type = "MyError::InvalidString"
+    // context = { expected_format: string }
+    // #[editoast_error(internal)] by default
+    InvalidString { #[from] source: WrongString, expected_format: String },
+
+    // error_type = "MyError::InvalidInt"
+    // context = {} (xyz is skipped as we forward the editoast error)
+    WrongInt { #[from] #[editoast_error] source: WrongInt, xyz: String }
+
+    // error_type = "MyError::Bad"
+    // context = { "0": string, "1": number }
+    #[error("user did a bad with {0} and {1}")]
+    #[editoast_error(user, name = "Bad")]
+    Oops(String, i64)
+}
+````
+
+### Providing `context`
+
+Only `4xx` errors require context since these are the errors the frontend may use to provide the user some form of recovery. Context is computed just before the error is serialized in `axum`'s error handler.
+
+`derive(EditoastError)` provides a few ways to set it.
+
+````rust
+#[derive(Debug, thiserror::Error, EditoastError)]
+enum Error {
+    #[editoast_error(user, context)]
+    No { because: String }, // { }
+
+    #[editoast_error(user, context)]
+    Nein { reasons: Vec<String> }, // { "reasons": [string] }
+
+    // explicitely builds the dictionnary (*)
+    #[editoast_error(user, context(
+        reason = "format!(\"because {reason}\")",
+        recovery = "format!(\"do this instead {recovery}\")"
+    ))]
+    QueNo { reason: String, recovery: String },
+}
+
+// with a provider function
+#[derive(Debug, thiserror::Error, EditoastError)]
+#[editoast_error(context_with = context_provider)]
+enum Error {
+    Nao(String),
+    Nee(String, u64)
+}
+
+fn context_provider(error: Error) -> HashMap<String, serde_json::Value> {
+    todo!()
+}
+````
+
+`(*)`: only if the mapping can be collected easily using `darling`.
+
+### Incident reports
+
+For `internal` errors that won't contain meaningful information for the end user, we substitute the error by:
+
+```json
+{
+  "error_type": "InternalError",
+  "message": "<a meaningful message>",
+  "status": 500,
+  "context": {},
+  "incident": "<uuid>"
+}
+```
+
+In order to be able to find and investigate the error later on, we associate to each `5xx` error a unique `incident` identifier. At first we'll just log the incident with:
+
+- the error message
+- the error `Debug` representation
+- the backtrace(s), if any
+
+The log entry will be persisted in datadog/jaeger so it's probably good enough at first.
+
+It's useful in development to have the real error shown in the interface instead of just "Internal error". We can set an environment variable `OSRD_DEV=1` to avoid replacing the error in the `axum` handler.