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.

Sign up for GitHub

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: add an endpoint to list infra objects #11032

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
+127 −0
Open

editoast: add an endpoint to list infra objects #11032

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions editoast/openapi.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -920,6 +920,32 @@ paths:
description: Duplicate object ids provided
'404':
description: Object ID or infra ID invalid
/infra/{infra_id}/objects/{object_type}/list:
get:
tags:
- infra
parameters:
- name: infra_id
in: path
description: An existing infra ID
required: true
schema:
type: integer
format: int64
- name: object_type
in: path
required: true
schema:
$ref: '#/components/schemas/ObjectType'
responses:
'200':
description: The list of objects
content:
application/json:
schema:
$ref: '#/components/schemas/ListObjectsResponse'
'404':
description: Infra ID invalid
/infra/{infra_id}/path_properties:
post:
tags:
Expand Down Expand Up @@ -8036,6 +8062,15 @@ components:
type: array
items:
$ref: '#/components/schemas/RollingStockLivery'
ListObjectsResponse:
type: object
required:
- ids
properties:
ids:
type: array
items:
type: string
LoadingGaugeLimit:
type: object
required:
Expand Down
26 changes: 26 additions & 0 deletions editoast/src/models/infra/object_queryable.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,4 +72,30 @@ impl Infra {
.await?;
Ok(objects)
}

pub async fn list_objects(
&self,
conn: &mut DbConnection,
object_type: ObjectType,
) -> Result<Vec<String>> {
let query = format!(
"SELECT obj_id FROM {} WHERE infra_id = $1",
get_table(&object_type)
);

#[derive(QueryableByName)]
struct ObjectId {
#[diesel(sql_type = Text)]
obj_id: String,
}

let objects = sql_query(query)
.bind::<BigInt, _>(self.id)
.load::<ObjectId>(conn.write().await.deref_mut())
.await?
.into_iter()
.map(|e| e.obj_id)
.collect();
Ok(objects)
}
}
1 change: 1 addition & 0 deletions editoast/src/views/infra/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ crate::routes! {
editoast_common::schemas! {
pathfinding::schemas(),
delimited_area::schemas(),
objects::schemas(),
InfraState,
InfraWithState,
}
Expand Down
46 changes: 46 additions & 0 deletions editoast/src/views/infra/objects.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@ use crate::Retrieve;

crate::routes! {
"/objects/{object_type}" => get_objects,
"/objects/{object_type}/list" => list_objects,
Comment on lines 24 to +25
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

(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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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 ?

}

editoast_common::schemas! {
ListObjectsResponse,
}

#[derive(Debug, Error, EditoastError)]
Expand Down Expand Up @@ -114,6 +119,47 @@ async fn get_objects(
Ok(Json(result))
}

#[derive(serde::Serialize, utoipa::ToSchema)]
struct ListObjectsResponse {
ids: Vec<String>,
}

#[utoipa::path(
get, path = "",
tag = "infra",
params(InfraIdParam, ObjectTypeParam),
responses(
(status = 200, description = "The list of objects", body = ListObjectsResponse),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
(status = 200, description = "The list of objects", body = ListObjectsResponse),
(status = 200, description = "The list of objects", body = inline(ListObjectsResponse)),

or

Suggested change
(status = 200, description = "The list of objects", body = ListObjectsResponse),
(status = 200, description = "The list of objects", body = Vec<String>),

directly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

(status = 404, description = "Infra ID invalid")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
(status = 404, description = "Infra ID invalid")

for consistency with the rest of the routes (that'll come with ViewErrors)

)
)]
async fn list_objects(
Path(infra_id_param): Path<InfraIdParam>,
Path(object_type_param): Path<ObjectTypeParam>,
Comment on lines +137 to +138
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Path(infra_id_param): Path<InfraIdParam>,
Path(object_type_param): Path<ObjectTypeParam>,
Path(InfraIdParam { infra_id }): Path<InfraIdParam>,
Path(ObjectTypeParam { object_type }): Path<ObjectTypeParam>,

State(db_pool): State<DbConnectionPoolV2>,
Extension(auth): AuthenticationExt,
) -> Result<Json<ListObjectsResponse>> {
let authorized = auth
.check_roles([BuiltinRole::InfraRead].into())
.await
.map_err(AuthorizationError::AuthError)?;
if !authorized {
return Err(AuthorizationError::Forbidden.into());
}

let infra_id = infra_id_param.infra_id;
let infra = Infra::retrieve_or_fail(&mut db_pool.get().await?, infra_id, || {
InfraApiError::NotFound { infra_id }
})
.await?;

let objects = infra
.list_objects(&mut db_pool.get().await?, object_type_param.object_type)
.await?;

Ok(Json(ListObjectsResponse { ids: objects }))
}

#[cfg(test)]
mod tests {
Copy link
Contributor

Choose a reason for hiding this comment

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

a test would be nice

use axum::http::StatusCode;
Expand Down
19 changes: 19 additions & 0 deletions front/src/common/api/generatedEditoastApi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,15 @@ const injectedRtkApi = api
}),
providesTags: ['infra'],
}),
getInfraByInfraIdObjectsAndObjectTypeList: build.query<
GetInfraByInfraIdObjectsAndObjectTypeListApiResponse,
GetInfraByInfraIdObjectsAndObjectTypeListApiArg
>({
query: (queryArg) => ({
url: `/infra/${queryArg.infraId}/objects/${queryArg.objectType}/list`,
}),
providesTags: ['infra'],
}),
postInfraByInfraIdPathProperties: build.query<
PostInfraByInfraIdPathPropertiesApiResponse,
PostInfraByInfraIdPathPropertiesApiArg
Expand Down Expand Up @@ -1420,6 +1429,13 @@ export type PostInfraByInfraIdObjectsAndObjectTypeApiArg = {
objectType: ObjectType;
body: string[];
};
export type GetInfraByInfraIdObjectsAndObjectTypeListApiResponse =
/** status 200 The list of objects */ ListObjectsResponse;
export type GetInfraByInfraIdObjectsAndObjectTypeListApiArg = {
/** An existing infra ID */
infraId: number;
objectType: ObjectType;
};
export type PostInfraByInfraIdPathPropertiesApiResponse =
/** status 200 Path properties */ PathProperties;
export type PostInfraByInfraIdPathPropertiesApiArg = {
Expand Down Expand Up @@ -2762,6 +2778,9 @@ export type InfraObjectWithGeometry = {
obj_id: string;
railjson: object;
};
export type ListObjectsResponse = {
ids: string[];
};
export type OperationalPointExtensions = {
identifier?: {
name: string;
Expand Down
Loading