Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Editoast error management #237

Open
wants to merge 27 commits into
base: master
Choose a base branch
from
Open
Changes from 1 commit
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
07f8a25
docs: add editoast error management proposal
leovalais Sep 18, 2024
e7a15f4
fix typos
leovalais Nov 8, 2024
c9bb82d
some adjustments
leovalais Nov 8, 2024
e670075
proposition: rename EditoastError to ViewError
leovalais Nov 8, 2024
c2fbf6a
add reasons for the new system
leovalais Nov 26, 2024
717e31d
add a section Rejected Ideas
leovalais Nov 26, 2024
ec3e8d2
add a section about Core errors
leovalais Nov 26, 2024
f735332
add implementation plan
leovalais Nov 26, 2024
1813559
workshop
leovalais Nov 26, 2024
cb0bf8b
Update content/docs/reference/design-docs/editoast-errors/_index.en.md
leovalais Nov 27, 2024
da75bbd
Update content/docs/reference/design-docs/editoast-errors/_index.en.md
leovalais Nov 27, 2024
46336d8
Update content/docs/reference/design-docs/editoast-errors/_index.en.md
leovalais Nov 27, 2024
ab758a5
fix inconsistency
leovalais Dec 2, 2024
db47135
Update _index.en.md
leovalais Jan 6, 2025
0581f19
workshop with @flomonster conclusions
leovalais Jan 7, 2025
3c86cc6
no
leovalais Jan 7, 2025
489c7be
more info about context
leovalais Jan 7, 2025
c28a470
remove CoreError.additional_metadata — bad idea
leovalais Jan 7, 2025
0e29b95
Update content/docs/reference/design-docs/editoast-errors/_index.en.md
leovalais Jan 7, 2025
558e345
Update content/docs/reference/design-docs/editoast-errors/_index.en.md
leovalais Jan 7, 2025
5e5a2f8
r/redis/valkey
leovalais Jan 7, 2025
de611a9
Update content/docs/reference/design-docs/editoast-errors/_index.en.md
leovalais Jan 7, 2025
81ffc41
:)
leovalais Jan 7, 2025
adea77c
hopefully better wording
leovalais Jan 8, 2025
3e611c6
IntoResponse
leovalais Jan 10, 2025
f7bb491
\#
leovalais Jan 10, 2025
accb041
a new derive macro 😏
leovalais Jan 10, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions content/docs/reference/design-docs/editoast-errors/_index.en.md
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,54 @@ Cons:
* The implementors of an `AsCoreRequest` in the client also has to list all the expected errors (one might argue that it's part of the job...)
* We need to make sure that we forward **all** information returned by Core otherwise it might degrade the debuggability when using the frontend.

### Implementation plan

We'll need a progressive migration as this implies too much change to fit in a single PR. `EditoastError` and `ViewError` will have to cohabit for some time.

#### Setup

1. Create the `trait views::ViewError`
2. Implement an `axum::IntoResponse` for `ViewError` to generate a standard OSRD error response payload
3. Add a post-processing step to the OpenAPI generation to ensure the consistency of error status codes. More details below.
4. Create a derive macro `ViewError` that interfaces with `thiserror::Error` API and generates:
* `impl ViewError`
* `impl utoipa::ToSchema` that returns the schema of the generated OSRD error
5. Adapt the frontend error keys collection script to also look for errors in the OpenAPI routes response section.

We decide the status code of each `ViewError` at it's definition as it's a required data to generate the response (the status code is part of the payload). Duplicating it in the `utoipa::path` annotation is a bad idea, and extracting it from the generate `utoipa::Route` is tricky. Therefore the error has to stay the source of truth about its status code.

```rust
#[utoipa::path(
...,
responses(
(status = 900, body = Computation),
(status = 901, body = EndpointError)
)
)]
```

The trick proposed here is to set each error to a non-existent status code and hamonize everything while post-processing the OpenAPI. The information will be readily available from the error schema as a constant.
leovalais marked this conversation as resolved.
Show resolved Hide resolved

#### Migration

The easier way to proceed here would be, in my (Léo) opinion, to start by converting simple errors that occur deep in the stack (such as Postgres errors, Redis errors, Core errors, etc.). This way, we can rely on the Rust compiler to guide us through the process and ensure we don't forget any error. We'll need some kind of adapters to incorporate these errors into `EditoastError`s. We may find a generic way to do that, but that's more an implementation detail, especially since that would be temporary.
leovalais marked this conversation as resolved.
Show resolved Hide resolved
woshilapin marked this conversation as resolved.
Show resolved Hide resolved

A good starting place would be `editoast_search`[^1] because its internal errors do not implmeent `EditoastError` already. Valkey errors may also be a decent candidate.
leovalais marked this conversation as resolved.
Show resolved Hide resolved

One large change that will have to be atomic will be the adaptation of `Model`'s errors.

[^1]: Provided we start this migration before the rewrite of the search engine.

#### Wrapping up things

Eventually, when all errors are converted and views errors are attached to their endpoint(s) in the OpenAPI, we'll have to:

1. Remove `trait EditoastError`, `derive(EditoastError)` and `struct InternalError` (at least its former version as the name may be reused in a different scope)
2. Remove the error collection mechanism
3. Remove the scanning of `components/schemas` by the frontend script collecting error keys for translation
4. If not already done, try to ensure the sync between Core and editoast errors is maintained in the CI
5. (Out of scope) Discuss with the frontend the level of visibility about internal errors we want to give the user
woshilapin marked this conversation as resolved.
Show resolved Hide resolved
leovalais marked this conversation as resolved.
Show resolved Hide resolved

### Rejected ideas

#### Anonymous enums
Expand Down
Loading