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: no nesting checks between projects, studies and scenarios #5630

Closed
Math-R opened this issue Nov 8, 2023 · 3 comments · Fixed by #5646
Closed

editoast: no nesting checks between projects, studies and scenarios #5630

Math-R opened this issue Nov 8, 2023 · 3 comments · Fixed by #5646
Assignees
Labels
area:editoast Work on Editoast Service difficulty:good first issue Good for newcomers kind:bug Something isn't working severity:major Major severity bug

Comments

@Math-R
Copy link
Contributor

Math-R commented Nov 8, 2023

What happened?

With the new router implementation in the front part of the application, the user know can access projects, studies, and scenarios using some parameters in the url.

Projects :
operationnal-studies/projects/{projectId}

Studies :
operationnal-studies/projects/{projectId}/studies/{studyId}

Scenario :
operationnal-studies/projects/{projectId}/studies/{studyId}/scenarios/{scenarioId}

All ids are unique in the DB.

But if the user try to access scenario : 4, he can do it from all the studies :

operationnal-studies/projects/1/studies/1/scenarios/4 => work
operationnal-studies/projects/2180/studies/1/scenarios/4 => work
operationnal-studies/projects/1/studies/850/scenarios/4 => work

Actually the API asks the ids for project, study , and scenario, but just to check if there are some existing ones but not if they're linked together.

What did you expect to happen?

Back must throw a 404 error when the id nesting is not consistent

How can we reproduce it (as minimally and precisely as possible)?

Create a 2 projects and 2 studies

Try to access the study hold by project 1 using the id of project 2

You will retrieve the study even if the project id is not the right one

What operating system, browser and environment are you using?

  • Browser: Firefox v109
  • OS: Nixos v22.11
  • Env: Local

OSRD version (top right corner Account button > Informations)

5c88cf6

@Math-R Math-R added kind:bug Something isn't working area:editoast Work on Editoast Service severity:major Major severity bug labels Nov 8, 2023
@Math-R Math-R changed the title editoast: no dependencies between projects, studies and scenarios editoast: no nesting checks between projects, studies and scenarios Nov 8, 2023
@flomonster flomonster added the difficulty:good first issue Good for newcomers label Nov 8, 2023
@leovalais
Copy link
Contributor

Good catch, my take on this would be to get rid of the nesting which brings no benefit since you already have a scenario ID. The nesting would be useful if scenarios that belong to different projects could share the same identification key (such as their name by example) that we could use to identify them. But that's not our case.

@Math-R does the url nesting or the inclusion check bring any benefit to the front?

@osrd-project/editoast-maintainers Wdyt? We could still check the inclusion using for instance optional URL parameters, like editoast:/scenario/1?project=1&study=42

@Tristramg
Copy link
Contributor

From a rest point of view, I’d say /scenario/{id} seems to be enough and we don’t need that nesting (and maybe the same for /studies ?)
I’m not sure about the need for a coherence check and I find it a bit weird, but maybe there are some reasons I don’t know to double check that the scenario really belongs to a given study. As the issue is flagged major, I’m probably missing a point here

@flomonster
Copy link
Contributor

We wanted to keep a nesting endpoint cause we'd like to replace IDs with slugs in the near future. I don't yet know how this will be done but the unicity property may no longer be valid.

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 difficulty:good first issue Good for newcomers kind:bug Something isn't working severity:major Major severity bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants