editoast: refactor core error #11041

hamz2a · 2025-03-06T09:40:00Z

codecov-commenter · 2025-03-06T09:54:08Z

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

Codecov Report

Attention: Patch coverage is 35.89744% with 25 lines in your changes missing coverage. Please review.

Project coverage is 80.62%. Comparing base (164c697) to head (1263fcd).
Report is 13 commits behind head on dev.

Files with missing lines	Patch %	Lines
editoast/src/views/core_error_response.rs	11.11%	8 Missing ⚠️
editoast/src/views/train_schedule.rs	0.00%	6 Missing ⚠️
editoast/src/core/mod.rs	58.33%	5 Missing ⚠️
editoast/src/views/path/pathfinding.rs	0.00%	3 Missing ⚠️
editoast/src/views/timetable/stdcm.rs	66.66%	3 Missing ⚠️

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

Additional details and impacted files

@@            Coverage Diff             @@
##              dev   #11041      +/-   ##
==========================================
- Coverage   80.63%   80.62%   -0.02%     
==========================================
  Files        1099     1102       +3     
  Lines      112187   112362     +175     
  Branches      745      747       +2     
==========================================
+ Hits        90460    90589     +129     
- Misses      21684    21730      +46     
  Partials       43       43

Flag	Coverage Δ
editoast	`72.16% <35.89%> (-0.03%)`	⬇️
front	`89.92% <ø> (-0.03%)`	⬇️
gateway	`2.18% <ø> (ø)`
osrdyne	`2.53% <ø> (ø)`
railjson_generator	`87.58% <ø> (ø)`
tests	`87.90% <ø> (ø)`

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.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

woshilapin

Thank you. Some comment, but mostly unrelated to the work of this PR, could be addressed in another PR.

woshilapin · 2025-03-06T13:44:38Z

editoast/src/core/mod.rs

+    MqClientError(#[from] MqClientError),
+
+    #[error(transparent)]
+    StandardCoreError(#[from] StandardCoreError),


"Standard" feels vague, especially since there is also a variant "Generic". Not sure what each does.

Unless I'm wrong I think we could remove the other GenericCoreError and only keep the StandardCoreError one (maybe simply Core would suffice as a name?)

For GenericCoreError, we serialize the error without a specific format raw_error (example), while for StandardCoreError, we use it to parse the error and get a specific format (example).
Both are used.

I understand the difference but since they have the same meaning I believe we could replace GenericCoreError (it was created at a time when Core errors weren't homogeneous if I recall correctly)

editoast/src/views/timetable/stdcm.rs

leovalais

Thanks for resuming the work on the split in crates!

editoast/src/core/mocking.rs

leovalais · 2025-03-06T14:15:37Z

editoast/src/core/mod.rs

+    MqClientError(#[from] MqClientError),
+
+    #[error(transparent)]
+    StandardCoreError(#[from] StandardCoreError),


Unless I'm wrong I think we could remove the other GenericCoreError and only keep the StandardCoreError one (maybe simply Core would suffice as a name?)

editoast/src/views/core_error_response.rs

editoast/src/core/mod.rs

Signed-off-by: hamz2a <[email protected]>

leovalais

thanks for the changes!

leovalais · 2025-03-07T16:41:36Z

editoast/src/core/mod.rs

+        if let Error::StandardCoreError(standard_core_error) = result {
+            assert_eq!(standard_core_error, expected_error);
+        } else {
+            panic!("Unexpected error type: {:?}", result);
+        }


Nit: using assert_eq with the Error variant would be more idiomatic than if-else panicking imo

leovalais · 2025-03-07T16:43:47Z

editoast/src/error.rs

@@ -290,6 +292,65 @@ impl EditoastError for editoast_models::model::Error {
    }
 }

+impl EditoastError for core::Error {


The inventory::submit! is missing for translation keys, a few examples in this file or in macro expansion of EeitoastError

leovalais · 2025-03-07T16:44:36Z

editoast/src/error.rs

+            core::Error::UnparsableErrorOutput => Default::default(),
+            core::Error::ConnectionClosedBeforeMessageCompleted => Default::default(),
+            core::Error::ConnectionResetByPeer => Default::default(),
+            core::Error::BrokenPipe => Default::default(),
+            core::Error::MqClientError(_) => Default::default(),
+            core::Error::StandardCoreError(_) => Default::default(),


Suggested change

core::Error::UnparsableErrorOutput => Default::default(),

core::Error::ConnectionClosedBeforeMessageCompleted => Default::default(),

core::Error::ConnectionResetByPeer => Default::default(),

core::Error::BrokenPipe => Default::default(),

core::Error::MqClientError(_) => Default::default(),

core::Error::StandardCoreError(_) => Default::default(),

_ => Default::default(),

leovalais · 2025-03-07T16:47:19Z

editoast/src/views/core_error_response.rs

+pub struct CoreErrorResponse {
+    #[serde(with = "StatusCodeRemoteDef", default = "default_status_code")]
+    #[schema(value_type = u16, minimum = 100, maximum = 599)]
+    pub status: StatusCode,
+    #[serde(rename = "type")]
+    pub error_type: String,
+    pub context: HashMap<String, Value>,
+    pub message: String,
+}


pub(in crate::views) when necessary

leovalais · 2025-03-07T16:48:11Z

editoast/src/views/mod.rs

@@ -1,4 +1,5 @@
 mod authz;
+pub mod core_error_response;


Suggested change

pub mod core_error_response;

mod core_error_response;

use core_error_response::CoreErrorResponse;

avoids repetition in import paths

leovalais · 2025-03-07T16:48:17Z

editoast/src/views/core_error_response.rs

+    pub message: String,
+}
+
+impl From<Error> for CoreErrorResponse {


Suggested change

impl From<Error> for CoreErrorResponse {

impl From<core::Error> for CoreErrorResponse {

hamz2a requested review from a team as code owners March 6, 2025 09:40

github-actions bot added area:front Work on Standard OSRD Interface modules area:editoast Work on Editoast Service labels Mar 6, 2025

hamz2a force-pushed the hai/editoast-refactor-core-error branch from 69b2431 to 83d7f73 Compare March 6, 2025 09:52

hamz2a force-pushed the hai/editoast-refactor-core-error branch 2 times, most recently from a869efc to 17959f4 Compare March 6, 2025 10:38

woshilapin approved these changes Mar 6, 2025

View reviewed changes

hamz2a force-pushed the hai/editoast-refactor-core-error branch from 17959f4 to d32fff5 Compare March 6, 2025 14:04

leovalais requested changes Mar 6, 2025

View reviewed changes

hamz2a force-pushed the hai/editoast-refactor-core-error branch 2 times, most recently from ab5c613 to 2dfb381 Compare March 6, 2025 15:21

editoast: refactor core error

1263fcd

Signed-off-by: hamz2a <[email protected]>

hamz2a force-pushed the hai/editoast-refactor-core-error branch from 2dfb381 to 1263fcd Compare March 7, 2025 10:55

hamz2a requested review from leovalais and woshilapin March 7, 2025 14:18

leovalais reviewed Mar 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

editoast: refactor core error #11041

editoast: refactor core error #11041

hamz2a commented Mar 6, 2025 •

edited

Loading

codecov-commenter commented Mar 6, 2025 •

edited

Loading

woshilapin left a comment

woshilapin Mar 6, 2025

leovalais Mar 6, 2025

hamz2a Mar 7, 2025 •

edited

Loading

leovalais Mar 7, 2025

leovalais left a comment

leovalais Mar 6, 2025

leovalais left a comment

leovalais Mar 7, 2025

leovalais Mar 7, 2025

leovalais Mar 7, 2025

leovalais Mar 7, 2025

leovalais Mar 7, 2025

leovalais Mar 7, 2025

	pub mod core_error_response;
	mod core_error_response;
	use core_error_response::CoreErrorResponse;

	impl From<Error> for CoreErrorResponse {
	impl From<core::Error> for CoreErrorResponse {

editoast: refactor core error #11041

Are you sure you want to change the base?

editoast: refactor core error #11041

Conversation

hamz2a commented Mar 6, 2025 • edited Loading

codecov-commenter commented Mar 6, 2025 • 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

hamz2a Mar 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leovalais left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leovalais left a 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

hamz2a commented Mar 6, 2025 •

edited

Loading

codecov-commenter commented Mar 6, 2025 •

edited

Loading

hamz2a Mar 7, 2025 •

edited

Loading