editoast: add an endpoint to list infra objects #11032
Conversation
Signed-off-by: Younes Khoudli <[email protected]>
github-actions
bot
added
area:front
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## dev #11032 +/- ##
==========================================
- Coverage 80.69% 80.67% -0.03%
==========================================
Files 1098 1098
Lines 111888 111972 +84
Branches 745 745
==========================================
+ Hits 90290 90334 +44
- Misses 21555 21595 +40
Partials 43 43
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| tag = "infra", | ||
| params(InfraIdParam, ObjectTypeParam), | ||
| responses( | ||
| (status = 200, description = "The list of objects", body = ListObjectsResponse), |
There was a problem hiding this comment.
| (status = 200, description = "The list of objects", body = ListObjectsResponse), | |
| (status = 200, description = "The list of objects", body = inline(ListObjectsResponse)), |
or
| (status = 200, description = "The list of objects", body = ListObjectsResponse), | |
| (status = 200, description = "The list of objects", body = Vec<String>), |
directly
There was a problem hiding this comment.
Generally, it's a "good idea" to return json objects as a top level response... (To allow adding stuff later without making it a breaking change) But i could go either way, wdyt
There was a problem hiding this comment.
I don't mind an object at all, but in this case since the schema is only used at this one place better inline it
| Path(infra_id_param): Path<InfraIdParam>, | ||
| Path(object_type_param): Path<ObjectTypeParam>, |
There was a problem hiding this comment.
| Path(infra_id_param): Path<InfraIdParam>, | |
| Path(object_type_param): Path<ObjectTypeParam>, | |
| Path(InfraIdParam { infra_id }): Path<InfraIdParam>, | |
| Path(ObjectTypeParam { object_type }): Path<ObjectTypeParam>, |
| params(InfraIdParam, ObjectTypeParam), | ||
| responses( | ||
| (status = 200, description = "The list of objects", body = ListObjectsResponse), | ||
| (status = 404, description = "Infra ID invalid") |
There was a problem hiding this comment.
| (status = 404, description = "Infra ID invalid") |
for consistency with the rest of the routes (that'll come with ViewErrors)
|
|
||
| Ok(Json(ListObjectsResponse { ids: objects })) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
| "/objects/{object_type}" => get_objects, | ||
| "/objects/{object_type}/list" => list_objects, |
There was a problem hiding this comment.
It's not clear to me why we need both a get_objects and a list_objects. Couldn't we put the functionality directly in /objects/{object_type} (if no ID is provided in the body, it defaults to returning all of them?). In any case, it'd be nice to add a description in the PR to explain why this PR.
There was a problem hiding this comment.
When we import the work schedules in OSRD, we look for their associated tracks, using GAIA. Sometimes, the track ids returned are in GAIA but not in the OSRD infrastructure. We don't want to keep these extra track ids.
To check this, we need an endpoint that returns track ids from the OSRD infrastructure. (Alternatively, the other solution would be to add another time-consuming track geometry verification step in the work schedule pipeline.)
Your suggestion would also be appropriate for this need
There was a problem hiding this comment.
(The problem with the actual endpoint is that it crashes when at least one track id in the given payload is not in the OSRD infrastructure)
There was a problem hiding this comment.
Re why not return everything with an empty payload... That's mainly because that felt like a potential mistake waiting to happen, and that you probably do not want to download all tracksections just to get a list of ids
There was a problem hiding this comment.
Yes we only need the list of ids, i don't know if it would be possible to return this as default behavior of the existing endpoint ?
No description provided.