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, front: forbid zero-length paths in pathfinding endpoint #9931

Merged
merged 2 commits into from
Jan 22, 2025

Conversation

Sh099078
Copy link
Contributor

@Sh099078 Sh099078 commented Dec 3, 2024

Fixes #9444

  • Editoast: immediately return a 422 Unprocessable Content error at deserialization when the endpoint pathfinding/blocks is called with the same origin and destination.
  • Editoast: pathfinding/blocks: fail in post-processing with a new PathfindingInputError::ZeroLengthPath variant when core returns zero-length paths.
  • Front: add a new type of stdcm error type when trying to make an STDCM request with the same origin and destination, update the stdcm inputs validation method.

@github-actions github-actions bot added area:front Work on Standard OSRD Interface modules area:editoast Work on Editoast Service labels Dec 3, 2024
@Sh099078 Sh099078 force-pushed the lf/bugfix-pathfinding branch from 7c7ecdd to 9b5ea0c Compare December 3, 2024 14:17
@Sh099078 Sh099078 self-assigned this Dec 3, 2024
@codecov-commenter
Copy link

codecov-commenter commented Dec 3, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.80%. Comparing base (8d71c73) to head (2a2142a).
Report is 4 commits behind head on dev.

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

Additional details and impacted files
@@           Coverage Diff           @@
##              dev    #9931   +/-   ##
=======================================
  Coverage   81.80%   81.80%           
=======================================
  Files        1073     1073           
  Lines      106584   106601   +17     
  Branches      731      731           
=======================================
+ Hits        87192    87207   +15     
- Misses      19353    19355    +2     
  Partials       39       39           
Flag Coverage Δ
editoast 74.29% <100.00%> (-0.01%) ⬇️
front 89.34% <100.00%> (+<0.01%) ⬆️
gateway 2.18% <ø> (ø)
osrdyne 3.28% <ø> (ø)
railjson_generator 87.50% <ø> (ø)
tests 87.05% <ø> (ø)

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.

@Sh099078 Sh099078 force-pushed the lf/bugfix-pathfinding branch 6 times, most recently from 5564da9 to 4cb44f7 Compare December 5, 2024 23:25
@Sh099078 Sh099078 marked this pull request as ready for review December 6, 2024 00:01
@Sh099078 Sh099078 requested review from a team as code owners December 6, 2024 00:01
Copy link
Contributor

@woshilapin woshilapin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix. Thank you.

editoast/src/views/path/pathfinding.rs Outdated Show resolved Hide resolved
front/public/locales/en/stdcm.json Outdated Show resolved Hide resolved
@Sh099078 Sh099078 requested a review from emersion December 6, 2024 10:05
Copy link
Member

@emersion emersion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from this, looks good!

@SharglutDev
Copy link
Contributor

Note

This will conflict with #9924

@flomonster
Copy link
Contributor

Should we authorize pathfinding that has a total length higher than 0m but between two steps has a length of 0m?

@Sh099078 Sh099078 force-pushed the lf/bugfix-pathfinding branch 2 times, most recently from 858887c to 94aee2d Compare January 14, 2025 10:57
@Sh099078 Sh099078 force-pushed the lf/bugfix-pathfinding branch from 838a3ea to df926d1 Compare January 14, 2025 15:15
@Sh099078 Sh099078 force-pushed the lf/bugfix-pathfinding branch from 01850f1 to 30a4ecb Compare January 20, 2025 14:17
Copy link
Contributor

@SharglutDev SharglutDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the changes, one more thing :)

front/src/applications/stdcm/hooks/useStaticPathfinding.ts Outdated Show resolved Hide resolved
@Sh099078 Sh099078 force-pushed the lf/bugfix-pathfinding branch from 30a4ecb to 5c13dfe Compare January 22, 2025 10:52
@Sh099078 Sh099078 requested a review from SharglutDev January 22, 2025 14:31
Copy link
Contributor

@SharglutDev SharglutDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm and tested, great job !

@Sh099078 Sh099078 force-pushed the lf/bugfix-pathfinding branch 2 times, most recently from 2e294ea to 97db6c4 Compare January 22, 2025 18:02
- Editoast: treat core pathfinding responses with a length of zero as
  errors.
- Frontend, stdcm: add error notifications when the user enters the same
  origin and destination.

Signed-off-by: Loup Federico <[email protected]>
Pathfinding is prevented when some validity checks don't pass, as for
example when the user selects the same origin and destination. The error
warning was only updated when pathfinging occurs, causing the user
warnings not to be displayed when we could detect before launching the
pathfinding that its request would be invalid.

Update the warnings independently on whether the pathfinding actually
happened so that invalid checks preventing the pathfinding also generate
their user warnings.

Signed-off-by: Loup Federico <[email protected]>
@Sh099078 Sh099078 force-pushed the lf/bugfix-pathfinding branch from 97db6c4 to 2a2142a Compare January 22, 2025 18:04
@Sh099078 Sh099078 enabled auto-merge January 22, 2025 18:05
@Sh099078 Sh099078 added this pull request to the merge queue Jan 22, 2025
Merged via the queue into dev with commit c014f83 Jan 22, 2025
27 checks passed
@Sh099078 Sh099078 deleted the lf/bugfix-pathfinding branch January 22, 2025 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:editoast Work on Editoast Service area:front Work on Standard OSRD Interface modules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

0-length paths aren't explicitely forbidden but break most endpoints
6 participants