editoast: remove InternalError usage in Delete and Create Model API #10196

leovalais · 2024-12-24T17:07:58Z

Creates a custom error type wrapping diesel::result::Error in editoast_models
Adapt the API of deletion and creation traits (the rest will come in another PR(s))
Propagate the API change in views (mostly)
- new error variants are created to accommodate some (valid) typing constraints, bringing us closer to our target error management structure
- simplified a few map_diesel_error here and there
The traits will be moved into editoast_models::model later

Tip

Review commit by commit

codecov-commenter · 2024-12-24T17:10:04Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 84.61538% with 16 lines in your changes missing coverage. Please review.

Project coverage is 81.51%. Comparing base (41e05a4) to head (1ed77d0).
Report is 83 commits behind head on dev.

Files with missing lines	Patch %	Lines
editoast/src/models/prelude/delete.rs	0.00%	5 Missing ⚠️
editoast/editoast_models/src/model.rs	85.71%	4 Missing ⚠️
editoast/src/models/prelude/create.rs	0.00%	2 Missing ⚠️
editoast/src/views/temporary_speed_limits.rs	86.66%	2 Missing ⚠️
editoast/src/views/work_schedules.rs	87.50%	2 Missing ⚠️
editoast/src/views/rolling_stock.rs	95.83%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev   #10196      +/-   ##
==========================================
+ Coverage   79.94%   81.51%   +1.56%     
==========================================
  Files        1057     1060       +3     
  Lines      106302   104448    -1854     
  Branches      724      722       -2     
==========================================
+ Hits        84982    85138     +156     
+ Misses      21278    19269    -2009     
+ Partials       42       41       -1

Flag	Coverage Δ
editoast	`73.68% <84.61%> (-0.26%)`	⬇️
front	`89.24% <ø> (+0.04%)`	⬆️
gateway	`2.18% <ø> (ø)`
osrdyne	`3.28% <ø> (ø)`
railjson_generator	`87.50% <ø> (ø)`
tests	`87.05% <ø> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Leo Valais <leo.valais97@gmail.com>

…els::Error Signed-off-by: Leo Valais <leo.valais97@gmail.com>

woshilapin

Nice job. Not approving yet, let's see where the comments goes first.

woshilapin · 2025-01-07T13:49:09Z

editoast/editoast_models/src/model/error.rs

How about defining this directly in src/model.rs? To me, defining the error is also about locality, meaning, where it will be used. I'm not a fan of defining error in its own module for that reason. On top of that:

src/model.rs is empty so it won't clutter much (we can think later if it's start to not be the case)

you will avoid a EditoastModelErrorError in OpenAPI I believe and transform it into EditoastModelError 😃

d5e8f29

Note: this is the only instruction that defines what's being included in the OpenAPI. The module path is only used by derive(EditoastError) to fill in fn get_type.

inventory::submit! { ErrorDefinition::new("editoast:model:ModelError", "Error", "ModelError", 500u16, r#"{}"#) }

OK, so the commit does solve the complicate naming in OpenAPI. But yet, do we need a dedicated Rust module for the error?

6852ad7 fixes the OpenApi

d5e8f29 moves the error to model.rs

I should have linked both, sorry 😅

editoast/src/error.rs

woshilapin · 2025-01-07T13:52:17Z

editoast/src/models/prelude/delete.rs


    /// Just like [Delete::delete] but returns `Err(fail())` if the row didn't exist
-    async fn delete_or_fail<E, F>(&self, conn: &mut DbConnection, fail: F) -> Result<()>
+    async fn delete_or_fail<E, F>(


Any reason why delete_or_fail() is generic over the error E but not delete() above?

The point of delete is to delete the row if it's there (hence the Ok(bool)). The "fail" or delete_or_fail represents the case where we expect the row to be there. In case it's not, it's a caller error that needs to be raised, not a model::Error:

match self.delete(conn).await { Ok(true) => Ok(()), Ok(false) => Err(fail()), // fail(): E Err(e) => Err(E::from(e)), // E: From<model::Error> }

But PostgreSQL errors can still happen, so we need E to "accept" model::Error as well.

woshilapin · 2025-01-07T13:57:05Z

editoast/src/models/timetable.rs

+    async fn delete_static(
+        conn: &mut DbConnection,
+        id: i64,
+    ) -> std::result::Result<bool, editoast_models::model::Error> {


Every place you change Result<something> to std::result::Result<something, editoast_models::model::Error>, you can do this instead Result<something, editoast_models::model::Error> because our crate::error::Result is generic over the error with a default value (so you can just override it).

Haha, well, you changed this only place, but there was a lot of places in your PR where you can do that.

Oh I thought your comment was about this one specifically since the traits are implemented manually for timetables. I'll change the occurrences in the trait functions but for the quoted code in the macros I'd prefer to keep the fully qualified path:

macro hygiene

it lifts ambiguity since "jump to definition" isn't available in snapshots or quoted code

crate::error::Result will disappear after the ViewError migration, so that's one less thing we'll have to do in the future

Wdyt?

editoast/src/models/timetable.rs

editoast/editoast_models/src/model/error.rs

woshilapin · 2025-01-07T14:15:08Z

editoast/src/views/temporary_speed_limits.rs

-    ) {
-        TemporarySpeedLimitError::NameAlreadyUsed {
-            name: name.as_ref().to_string(),
+impl From<model::Error> for TemporarySpeedLimitError {


Will we have to write all From manually from now on? Or is it just a temporary situation until we finish the migration to the new error model (I feel I'm missing a part of the big picture here). If it's the former, then we might to discuss it because I'm not sure we would want that everywhere, what do you think?

Only when we can't write:

#[error(transparent)] #[editoast_error(status = 500)] Database(#[from] model::Error),

because we want the from operation to perform some decisions about which variant to return, like here. When it's just a forwarding, thiserror generates this implementation.

woshilapin · 2025-01-07T14:17:49Z

editoast/src/views/temporary_speed_limits.rs

@@ -158,7 +172,7 @@ async fn create_temporary_speed_limit_group(
        .creation_date(Utc::now().naive_utc())
        .create(conn)
        .await
-        .map_err(|e| map_diesel_error(e, speed_limit_group_name))?;
+        .map_err(|e| TemporarySpeedLimitError::from(e).with_name(speed_limit_group_name))?;


Maybe implementing From is not adapted here. We might need a real new() constructor? What do you think? Or else, we might think about UniqueViolation containing the value of the conflicting key?

Thanks for making me look again the information provided by the error. I believe we can parse the conflicting key and add it to the error.

There may be a way to interface the error with trait Identifiable<K> and trait PreferredIdentifier<K>. I'll look into it.

But before that can we resolve #10196 (comment) (conflicts...)

pub trait DatabaseErrorInformation { /// The primary human-readable error message. Typically one line. fn message(&self) -> &str; /// An optional secondary error message providing more details about the /// problem, if it was provided by the database. Might span multiple lines. fn details(&self) -> Option<&str>; /// An optional suggestion of what to do about the problem, if one was /// provided by the database. fn hint(&self) -> Option<&str>; /// The name of the table the error was associated with, if the error was /// associated with a specific table and the backend supports retrieving /// that information. /// /// Currently this method will return `None` for all backends other than /// PostgreSQL. fn table_name(&self) -> Option<&str>; /// The name of the column the error was associated with, if the error was /// associated with a specific column and the backend supports retrieving /// that information. /// /// Currently this method will return `None` for all backends other than /// PostgreSQL. fn column_name(&self) -> Option<&str>; /// The constraint that was violated if this error is a constraint violation /// and the backend supports retrieving that information. /// /// Currently this method will return `None` for all backends other than /// PostgreSQL. fn constraint_name(&self) -> Option<&str>; /// An optional integer indicating an error cursor position as an index into /// the original statement string. fn statement_position(&self) -> Option<i32>; }

Here's a proposition: 6257f64

I like it better this way, but I wish we didn't have to bother knowing table columns and constraints using models in views. Table definitions are "implementation details" from the views point of view. But that's out of scope of this PR (maybe ModelV3? :p)

woshilapin · 2025-01-07T14:25:31Z

editoast/src/views/rolling_stock.rs

+}
+
+impl RollingStockError {
+    fn with_name(self, name: String) -> Self {


You might be able to avoid this weird function/pattern if model::Error::UniqueViolation is able to retain the key that created the conflict.

#10196 (comment)

woshilapin · 2025-01-07T14:27:37Z

editoast/editoast_models/src/model/error.rs

+use regex::Regex;
+
+#[derive(Debug, thiserror::Error)]
+pub enum Error {


Do you think we would need a CreateError and a DeleteError? From what I understand, they semantically can produce different set of error variants... but diesel seems to have made the choice to only have one enum for them all (even if different functions may only return a subset of it).

#10196 (comment)

I'm not sure that I understand how is this related to #10196 (comment).

It doesn't, sorry 😅

I asked myself the same question. I don't think there will be much difference between each trait's error type. Retrieve may have less error cases, but I don't think that'd be worth dealing with 5+ error types for the Model API alone. I could be proved wrong after having migrated the rest of the traits, but at least for Create and Delete, I believe that holds. Wait and see, or do we split them now and merge them if that's deemed unnecessary later?

Signed-off-by: Leo Valais <leo.valais97@gmail.com>

…ls::Error Signed-off-by: Leo Valais <leo.valais97@gmail.com>

Signed-off-by: Leo Valais <leo.valais97@gmail.com>

github-actions bot added area:front Work on Standard OSRD Interface modules area:editoast Work on Editoast Service labels Dec 24, 2024

leovalais force-pushed the lva/model-errors branch from d790ad7 to f75fab0 Compare December 26, 2024 16:44

leovalais added 5 commits December 26, 2024 17:49

editoast: define Model error type and use it in trait Delete

07503fa

Signed-off-by: Leo Valais <leo.valais97@gmail.com>

editoast: change Model trait DeleteStatic to use editoast_models::Error

d3391df

Signed-off-by: Leo Valais <leo.valais97@gmail.com>

editoast: change DeleteBatch signature to use Model Error type

f5bc015

Signed-off-by: Leo Valais <leo.valais97@gmail.com>

editoast: change Model trait Create to use editoast_models::Error

4591a26

Signed-off-by: Leo Valais <leo.valais97@gmail.com>

editoast: change Model traits CreateBatch,WithKey to use editoast_mod…

f4c7501

…els::Error Signed-off-by: Leo Valais <leo.valais97@gmail.com>

leovalais force-pushed the lva/model-errors branch from f75fab0 to f4c7501 Compare December 26, 2024 16:49

leovalais changed the title ~~lva/model errors~~ editoast: remove InternalError usage in Delete and Create Model API Dec 26, 2024

leovalais marked this pull request as ready for review December 26, 2024 16:54

leovalais requested review from a team as code owners December 26, 2024 16:54

leovalais enabled auto-merge December 26, 2024 16:54

leovalais self-assigned this Jan 6, 2025

leovalais added the kind:enhancement Improvement of existing features label Jan 6, 2025

woshilapin reviewed Jan 7, 2025

View reviewed changes

leovalais added 4 commits January 8, 2025 17:11

fixup! editoast: define Model error type and use it in trait Delete

d5e8f29

Signed-off-by: Leo Valais <leo.valais97@gmail.com>

fixup! editoast: define Model error type and use it in trait Delete

6852ad7

Signed-off-by: Leo Valais <leo.valais97@gmail.com>

fixup! editoast: change Model trait DeleteStatic to use editoast_mode…

2d7acaa

…ls::Error Signed-off-by: Leo Valais <leo.valais97@gmail.com>

fixup! editoast: define Model error type and use it in trait Delete

a15b78c

Signed-off-by: Leo Valais <leo.valais97@gmail.com>

leovalais force-pushed the lva/model-errors branch from 1ed77d0 to a15b78c Compare January 10, 2025 13:04

fixup! editoast: change Model trait Create to use editoast_models::Error

6257f64

Signed-off-by: Leo Valais <leo.valais97@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

editoast: remove InternalError usage in Delete and Create Model API #10196

editoast: remove InternalError usage in Delete and Create Model API #10196

leovalais commented Dec 24, 2024 •

edited

Loading

codecov-commenter commented Dec 24, 2024 •

edited

Loading

woshilapin left a comment

woshilapin Jan 7, 2025

leovalais Jan 8, 2025

woshilapin Jan 10, 2025 •

edited

Loading

leovalais Jan 10, 2025

woshilapin Jan 7, 2025

leovalais Jan 8, 2025

woshilapin Jan 7, 2025

leovalais Jan 8, 2025

woshilapin Jan 10, 2025

leovalais Jan 10, 2025

woshilapin Jan 7, 2025

leovalais Jan 8, 2025

woshilapin Jan 7, 2025

leovalais Jan 8, 2025

leovalais Jan 10, 2025

woshilapin Jan 7, 2025

leovalais Jan 8, 2025

woshilapin Jan 7, 2025

leovalais Jan 8, 2025

woshilapin Jan 10, 2025

leovalais Jan 10, 2025

editoast: remove InternalError usage in Delete and Create Model API #10196

Are you sure you want to change the base?

editoast: remove InternalError usage in Delete and Create Model API #10196

Conversation

leovalais commented Dec 24, 2024 • edited Loading

codecov-commenter commented Dec 24, 2024 • edited Loading

Codecov Report

woshilapin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woshilapin Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leovalais commented Dec 24, 2024 •

edited

Loading

codecov-commenter commented Dec 24, 2024 •

edited

Loading

woshilapin Jan 10, 2025 •

edited

Loading