Skip to content

Commit

Permalink
Fix url_for bug
Browse files Browse the repository at this point in the history
linked to actix/actix-web#1763

There was a bug, all links were linking to the same datasets (because
the routes are scoped).

I don't really see a clean way to make actix handle this, so for the
moment I use a custom name for each route based on the scope
(`{scope}/{route_name}`).

With this we can't use the niiiice `#[get("/routes")]` anymore 😭
  • Loading branch information
antoine-de committed Oct 29, 2020
1 parent fd02215 commit 28c8da9
Show file tree
Hide file tree
Showing 12 changed files with 165 additions and 48 deletions.
32 changes: 32 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ slog-scope-futures = "0.1"
serde_json = "1.0"
mockito = "0.15"
transit_model_builder = "0.1.0"
pretty_assertions = "0.6"

[build-dependencies]
prost-build = "0.4"

2 changes: 1 addition & 1 deletion src/routes/api_entry_point.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ async fn entry_point(req: HttpRequest, datasets: web::Data<Datasets>) -> web::Js
})
.collect(),
links: btreemap! {
"documentation" => Link::from_url(&req, "documentation", &[]),
"documentation" => Link::from_url(&req, "documentation"),
"dataset_detail" => Link {
href: raw_url(&req, "/{id}/"),
templated: Some(true),
Expand Down
3 changes: 1 addition & 2 deletions src/routes/general_message.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use crate::siri_lite::{
use crate::transit_realtime;
use crate::utils;
use actix::Addr;
use actix_web::{get, web, Result};
use actix_web::{web, Result};

#[derive(Deserialize, Debug)]
#[serde(rename_all = "PascalCase")]
Expand Down Expand Up @@ -160,7 +160,6 @@ fn general_message(request: Params, rt_data: &RealTimeDataset) -> Result<SiriRes
})
}

#[get("/siri/2.0/general-message.json")]
pub async fn general_message_query(
web::Query(query): web::Query<Params>,
dataset_actor: web::Data<Addr<DatasetActor>>,
Expand Down
4 changes: 1 addition & 3 deletions src/routes/gtfs_rt.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
use crate::actors::{DatasetActor, GetRealtimeDataset};
use crate::transit_realtime;
use actix::Addr;
use actix_web::{error, get, http::ContentEncoding, web, HttpResponse};
use actix_web::{error, http::ContentEncoding, web, HttpResponse};

#[get("/gtfs-rt")]
pub async fn gtfs_rt_protobuf(
dataset_actor: web::Data<Addr<DatasetActor>>,
) -> actix_web::Result<web::HttpResponse> {
Expand All @@ -24,7 +23,6 @@ pub async fn gtfs_rt_protobuf(
})
}

#[get("/gtfs-rt.json")]
pub async fn gtfs_rt_json(
dataset_actor: web::Data<Addr<DatasetActor>>,
) -> actix_web::Result<web::Json<transit_realtime::FeedMessage>> {
Expand Down
13 changes: 11 additions & 2 deletions src/routes/links.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,19 @@ pub struct Link {
}

impl Link {
pub fn from_url(req: &actix_web::HttpRequest, name: &str, params: &[&str]) -> Self {
pub fn from_url(req: &actix_web::HttpRequest, name: &str) -> Self {
Self {
href: req
.url_for(name, params)
.url_for(name, &[""])
.map(|u| u.into_string())
.unwrap_or_else(|_| panic!("route {} has not been registered with a name", name)),
..Default::default()
}
}
pub fn from_scoped_url(req: &actix_web::HttpRequest, name: &str, scope: &str) -> Self {
Self {
href: req
.url_for(&format!("{}/{}", scope, name), &[""])
.map(|u| u.into_string())
.unwrap_or_else(|_| panic!("route {} has not been registered with a name", name)),
..Default::default()
Expand Down
17 changes: 5 additions & 12 deletions src/routes/siri.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
use crate::actors::{DatasetActor, GetDataset};
use crate::routes::{Link, Links};
use actix::Addr;
use actix_web::{get, web, HttpRequest};
use actix_web::{web, HttpRequest};
use maplit::btreemap;

/// Api to tell what is available in siri-lite
#[get("/siri/2.0")]
pub async fn siri_endpoint(
req: HttpRequest,
dataset_actor: web::Data<Addr<DatasetActor>>,
Expand All @@ -14,18 +13,12 @@ pub async fn siri_endpoint(
log::error!("error while querying actor for data: {:?}", e);
actix_web::error::ErrorInternalServerError("impossible to get data".to_string())
})?;
let url_for = |link: &str| Link {
href: req
.url_for(link, &[&dataset.feed_construction_info.dataset_info.id])
.map(|u| u.into_string())
.unwrap_or_else(|_| panic!("impossible to find route {} to make a link", link)),
..Default::default()
};
let dataset_id = &dataset.feed_construction_info.dataset_info.id;
Ok(web::Json(
btreemap! {
"stop-monitoring" => url_for("stop_monitoring_query"),
"stoppoints-discovery" => url_for("stoppoints_discovery_query"),
"general-message" => url_for("general_message_query"),
"stop-monitoring" => Link::from_scoped_url(&req, "stop_monitoring_query", dataset_id),
"stoppoints-discovery" => Link::from_scoped_url(&req, "stoppoints_discovery_query", dataset_id),
"general-message" => Link::from_scoped_url(&req, "general_message_query", dataset_id),
}
.into(),
))
Expand Down
25 changes: 10 additions & 15 deletions src/routes/status.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use crate::actors::{DatasetActor, GetDataset};
use crate::routes::{Link, Links};
use actix::Addr;
use actix_web::{get, web, HttpRequest};
use actix_web::{web, HttpRequest};
use maplit::btreemap;
use openapi_schema::OpenapiSchema;

Expand All @@ -14,7 +14,6 @@ pub struct Status {
pub links: Links,
}

#[get("/")]
pub async fn status_query(
req: HttpRequest,
dataset_actor: web::Data<Addr<DatasetActor>>,
Expand All @@ -23,23 +22,19 @@ pub async fn status_query(
log::error!("error while querying actor for data: {:?}", e);
actix_web::error::ErrorInternalServerError("impossible to get data".to_string())
})?;
let url_for = |link: &str| Link {
href: req
.url_for(link, &[&dataset.feed_construction_info.dataset_info.id])
.map(|u| u.into_string())
.unwrap_or_else(|_| panic!("impossible to find route {} to make a link", link)),
..Default::default()
};

let dataset_id = &dataset.feed_construction_info.dataset_info.id;

Ok(web::Json(Status {
dataset: (&dataset.feed_construction_info.dataset_info).into(),
loaded_at: dataset.loaded_at,
links: btreemap! {
"gtfs-rt" => url_for("gtfs_rt_protobuf"),
"gtfs-rt.json" => url_for("gtfs_rt_json"),
"stop-monitoring" => url_for("stop_monitoring_query"),
"stoppoints-discovery" => url_for("stoppoints_discovery_query"),
"general-message" => url_for("general_message_query"),
"siri-lite" => url_for("siri_endpoint"),
"gtfs-rt" => Link::from_scoped_url(&req, "gtfs_rt_protobuf", &dataset_id),
"gtfs-rt.json" => Link::from_scoped_url(&req, "gtfs_rt_json", &dataset_id),
"stop-monitoring" => Link::from_scoped_url(&req, "stop_monitoring_query", &dataset_id),
"stoppoints-discovery" => Link::from_scoped_url(&req, "stoppoints_discovery_query", &dataset_id),
"general-message" => Link::from_scoped_url(&req, "general_message_query", &dataset_id),
"siri-lite" => Link::from_scoped_url(&req, "siri_endpoint", &dataset_id),
}
.into(),
}))
Expand Down
3 changes: 1 addition & 2 deletions src/routes/stop_monitoring.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use crate::datasets::{Connection, Dataset, RealTimeConnection, RealTimeDataset,
use crate::siri_lite::{self, service_delivery as model, SiriResponse};
use crate::utils;
use actix::Addr;
use actix_web::{error, get, web};
use actix_web::{error, web};
use openapi_schema::OpenapiSchema;
use transit_model::collection::Idx;
use transit_model::objects::StopPoint;
Expand Down Expand Up @@ -227,7 +227,6 @@ fn stop_monitoring(
})
}

#[get("/siri/2.0/stop-monitoring.json")]
pub async fn stop_monitoring_query(
web::Query(query): web::Query<Params>,
dataset_actor: web::Data<Addr<DatasetActor>>,
Expand Down
3 changes: 1 addition & 2 deletions src/routes/stoppoints_discovery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use crate::siri_lite::shared::CommonDelivery;
use crate::siri_lite::stop_points_delivery::{AnnotatedStopPoint, StopPointsDelivery};
use crate::siri_lite::{Siri, SiriResponse};
use actix::Addr;
use actix_web::{get, web};
use actix_web::web;

fn default_limit() -> usize {
20
Expand Down Expand Up @@ -81,7 +81,6 @@ pub fn filter(data: &crate::datasets::Dataset, request: Params) -> SiriResponse
}
}

#[get("/siri/2.0/stoppoints-discovery.json")]
pub async fn stoppoints_discovery_query(
web::Query(query): web::Query<Params>,
dataset_actor: web::Data<Addr<DatasetActor>>,
Expand Down
42 changes: 35 additions & 7 deletions src/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,13 +97,41 @@ fn register_dataset_routes(
cfg.service(
web::scope(&format!("/{id}", id = &d.id))
.data(dataset_actor.clone())
.service(status_query)
.service(gtfs_rt_protobuf)
.service(gtfs_rt_json)
.service(siri_endpoint)
.service(stoppoints_discovery_query)
.service(stop_monitoring_query)
.service(general_message_query),
.service(
web::resource("/")
.name(&format!("{}/status_query", &d.id))
.route(web::get().to(status_query)),
)
.service(
web::resource("/gtfs-rt")
.name(&format!("{}/gtfs_rt_protobuf", &d.id))
.route(web::get().to(gtfs_rt_protobuf)),
)
.service(
web::resource("/gtfs-rt.json")
.name(&format!("{}/gtfs_rt_json", &d.id))
.route(web::get().to(gtfs_rt_json)),
)
.service(
web::resource("/siri/2.0")
.name(&format!("{}/siri_endpoint", &d.id))
.route(web::get().to(siri_endpoint)),
)
.service(
web::resource("/siri/2.0/stoppoints-discovery.json")
.name(&format!("{}/stoppoints_discovery_query", &d.id))
.route(web::get().to(stoppoints_discovery_query)),
)
.service(
web::resource("/siri/2.0/stop-monitoring.json")
.name(&format!("{}/stop_monitoring_query", &d.id))
.route(web::get().to(stop_monitoring_query)),
)
.service(
web::resource("/siri/2.0/general-message.json")
.name(&format!("{}/general_message_query", &d.id))
.route(web::get().to(general_message_query)),
),
);
}
}
Expand Down
67 changes: 66 additions & 1 deletion tests/entry_point_test.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
mod utils;
use pretty_assertions::assert_eq;
use transpo_rt::datasets::DatasetInfo;

#[actix_rt::test]
async fn entry_point_integration_test() {
Expand Down Expand Up @@ -108,7 +110,6 @@ async fn test_siri_entrypoint(srv: &mut actix_web::test::TestServer) {
// test that invalid dataset are filtered
#[actix_rt::test]
async fn invalid_dataset_test() {
use transpo_rt::datasets::DatasetInfo;
let _log_guard = utils::init_log();
let mut srv = utils::make_test_server(vec![
DatasetInfo {
Expand Down Expand Up @@ -142,3 +143,67 @@ async fn invalid_dataset_test() {
Some(&serde_json::json!("valid"))
);
}

#[actix_rt::test]
async fn multiple_datasets_test() {
use transpo_rt::datasets::DatasetInfo;
let _log_guard = utils::init_log();
let first_dataset = DatasetInfo {
id: "first_dataset".into(),
name: "First dataset".into(),
gtfs: "fixtures/gtfs.zip".to_owned(),
gtfs_rt_urls: [mockito::server_url() + "/gtfs_rt_1"].to_vec(),
extras: std::collections::BTreeMap::default(),
};
let second_dataset = DatasetInfo {
id: "second_dataset".into(),
name: "Seond dataset".into(),
gtfs: "fixtures/gtfs.zip".to_owned(),
gtfs_rt_urls: [mockito::server_url() + "/gtfs_rt_1"].to_vec(),
extras: std::collections::BTreeMap::default(),
};
let mut srv =
utils::make_test_server(vec![first_dataset.clone(), second_dataset.clone()]).await;

check_datasets_entrypoints(&mut srv, &first_dataset).await;
check_datasets_entrypoints(&mut srv, &second_dataset).await;
}

async fn check_datasets_entrypoints(srv: &mut actix_web::test::TestServer, dataset: &DatasetInfo) {
let mut resp: serde_json::Value = utils::get_json(srv, &format!("/{}/", dataset.id)).await;

// we change the loaded_at datetime to be able to easily compare the response
*resp.pointer_mut("/loaded_at").unwrap() = "2019-06-20T10:00:00Z".into();
assert_eq!(
resp,
serde_json::json! {
{
"name": &dataset.name,
"id": &dataset.id,
"gtfs": "fixtures/gtfs.zip",
"loaded_at": "2019-06-20T10:00:00Z",
"extras": {},
"_links": {
"general-message": {
"href": &srv.url(&format!("/{}/siri/2.0/general-message.json", &dataset.id))
},
"gtfs-rt": {
"href": &srv.url(&format!("/{}/gtfs-rt", &dataset.id))
},
"gtfs-rt.json": {
"href": &srv.url(&format!("/{}/gtfs-rt.json", &dataset.id))
},
"stop-monitoring": {
"href": &srv.url(&format!("/{}/siri/2.0/stop-monitoring.json", &dataset.id))
},
"siri-lite": {
"href": &srv.url(&format!("/{}/siri/2.0", &dataset.id))
},
"stoppoints-discovery": {
"href": &srv.url(&format!("/{}/siri/2.0/stoppoints-discovery.json", &dataset.id))
}
}
}
}
);
}

0 comments on commit 28c8da9

Please sign in to comment.