Refactor BNA API (#127)

Refactors the BNA API to match the Zalando best practices as well as our OpenAPI specification. - Fixes ACCEPT_CONTENT headers in `APIError` - Adds convenience functions associated to `APIErrors` - Expands the BNARequestExt Trait - Fixes the CityPost wrapper - Fixes the logic of the POST /cities endpoint and adds some extra validations - Signed-off-by: Rémy Greinhofer <[email protected]> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
PeopleForBikes · Aug 8, 2024 · 1ea73b4 · 1ea73b4
1 parent 843bf93
commit 1ea73b4
Show file tree

Hide file tree

Showing 11 changed files with 291 additions and 147 deletions.
diff --git a/effortless/src/error.rs b/effortless/src/error.rs
@@ -1,4 +1,4 @@
-use lambda_http::{http::StatusCode, Body, Response};
+use lambda_http::{http::header, http::StatusCode, Body, Response};
 use serde::{Deserialize, Serialize};
 use serde_json::json;
 use serde_with::skip_serializing_none;
@@ -133,6 +133,26 @@ impl APIErrors {
             errors: errors.to_vec(),
         }
     }
+
+    /// Creates an empty `APIErrors`.
+    pub fn empty() -> Self {
+        Self { errors: vec![] }
+    }
+
+    /// Adds an `APIError`.
+    pub fn add(mut self, value: APIError) {
+        self.errors.push(value);
+    }
+
+    /// Extends with an existing `APIErrors`.
+    pub fn extend(&mut self, value: APIErrors) {
+        self.errors.extend(value.errors);
+    }
+
+    /// Returns True if there is no error.
+    pub fn is_empty(&self) -> bool {
+        self.errors.is_empty()
+    }
 }
 
 impl From<APIError> for APIErrors {
@@ -156,6 +176,7 @@ impl From<APIErrors> for Response<Body> {
         };
         Response::builder()
             .status(status)
+            .header(header::CONTENT_TYPE, "application/json")
             .body(json!(value).to_string().into())
             .unwrap()
     }

diff --git a/effortless/src/fragment.rs b/effortless/src/fragment.rs
@@ -150,6 +150,9 @@ pub trait BnaRequestExt {
     /// If there is no request ID or the event is not coming from an ApiGatewayV2, the
     /// function returns None.
     fn apigw_request_id(&self) -> Option<String>;
+
+    /// Returns true if there are path parameters available.
+    fn has_path_parameters(&self) -> bool;
 }
 
 impl<B> BnaRequestExt for http::Request<B> {
@@ -183,6 +186,10 @@ impl<B> BnaRequestExt for http::Request<B> {
             _ => None,
         }
     }
+
+    fn has_path_parameters(&self) -> bool {
+        !self.path_parameters().is_empty()
+    }
 }
 
 #[cfg(test)]

diff --git a/entity/src/wrappers/city.rs b/entity/src/wrappers/city.rs
@@ -8,6 +8,7 @@ pub struct CityPost {
     pub latitude: Option<f64>,
     pub longitude: Option<f64>,
     pub name: String,
+    pub region: Option<String>,
     pub state: String,
     pub state_abbrev: Option<String>,
     pub speed_limit: Option<i32>,
@@ -21,7 +22,7 @@ impl IntoActiveModel<city::ActiveModel> for CityPost {
             latitude: ActiveValue::Set(self.latitude),
             longitude: ActiveValue::Set(self.longitude),
             name: ActiveValue::Set(self.name),
-            region: ActiveValue::NotSet,
+            region: ActiveValue::set(self.region),
             state: ActiveValue::Set(self.state),
             state_abbrev: ActiveValue::Set(self.state_abbrev),
             speed_limit: ActiveValue::Set(self.speed_limit),

diff --git a/lambdas/src/cities/get-cities-bnas.rs b/lambdas/src/cities/get-cities-bnas.rs
@@ -1,54 +1,47 @@
 use dotenv::dotenv;
-use effortless::api::{entry_not_found, missing_parameter, parse_path_parameter};
+use effortless::api::entry_not_found;
 use entity::{city, summary};
 use lambda_http::{run, service_fn, Body, Error, Request, Response};
-use lambdas::{build_paginated_response, database_connect, pagination_parameters};
+use lambdas::{
+    build_paginated_response,
+    cities::{extract_path_parameters, PathParameters},
+    database_connect, pagination_parameters,
+};
 use sea_orm::{EntityTrait, PaginatorTrait};
 use serde_json::json;
 use tracing::info;
 
 async fn function_handler(event: Request) -> Result<Response<Body>, Error> {
     dotenv().ok();
 
-    // Set the database connection.
-    let db = database_connect(Some("DATABASE_URL_SECRET_ID")).await?;
+    // Extract the path parameters.
+    let params: PathParameters = match extract_path_parameters(&event) {
+        Ok(p) => p,
+        Err(e) => return Ok(e.into()),
+    };
 
     // Retrieve pagination parameters if any.
     let (page_size, page) = match pagination_parameters(&event) {
         Ok((page_size, page)) => (page_size, page),
         Err(e) => return Ok(e),
     };
 
-    let country = match parse_path_parameter::<String>(&event, "country") {
-        Ok(value) => value,
-        Err(e) => return Ok(e.into()),
-    };
-    dbg!(&country);
-    let region = match parse_path_parameter::<String>(&event, "region") {
-        Ok(value) => value,
-        Err(e) => return Ok(e.into()),
-    };
-    let name = match parse_path_parameter::<String>(&event, "name") {
-        Ok(value) => value,
-        Err(e) => return Ok(e.into()),
-    };
+    // Set the database connection.
+    let db = database_connect(Some("DATABASE_URL_SECRET_ID")).await?;
 
-    if country.is_some() && region.is_some() && name.is_some() {
-        let select = city::Entity::find_by_id((country.unwrap(), region.unwrap(), name.unwrap()))
-            .find_also_related(summary::Entity);
-        let model = select
-            .clone()
-            .paginate(&db, page_size)
-            .fetch_page(page - 1)
-            .await?;
-        if model.is_empty() {
-            return Ok(entry_not_found(&event).into());
-        }
-        let total_items = select.count(&db).await?;
-        build_paginated_response(json!(model), total_items, page, page_size, &event)
-    } else {
-        Ok(missing_parameter(&event, "country or region or name").into())
+    // Retrieve the city and associated BNA summary(ies).
+    let select = city::Entity::find_by_id((params.country, params.region, params.name))
+        .find_also_related(summary::Entity);
+    let model = select
+        .clone()
+        .paginate(&db, page_size)
+        .fetch_page(page - 1)
+        .await?;
+    if model.is_empty() {
+        return Ok(entry_not_found(&event).into());
     }
+    let total_items = select.count(&db).await?;
+    build_paginated_response(json!(model), total_items, page, page_size, &event)
 }
 
 #[tokio::main]

diff --git a/lambdas/src/cities/get-cities-census.rs b/lambdas/src/cities/get-cities-census.rs
@@ -1,8 +1,12 @@
 use dotenv::dotenv;
-use effortless::api::{entry_not_found, missing_parameter, parse_path_parameter};
+use effortless::api::entry_not_found;
 use entity::{census, city};
 use lambda_http::{run, service_fn, Body, Error, Request, Response};
-use lambdas::{build_paginated_response, database_connect, pagination_parameters};
+use lambdas::{
+    build_paginated_response,
+    cities::{extract_path_parameters, PathParameters},
+    database_connect, pagination_parameters,
+};
 use sea_orm::{EntityTrait, PaginatorTrait};
 use serde_json::json;
 use tracing::info;
@@ -26,44 +30,34 @@ async fn main() -> Result<(), Error> {
 async fn function_handler(event: Request) -> Result<Response<Body>, Error> {
     dotenv().ok();
 
-    // Set the database connection.
-    let db = database_connect(Some("DATABASE_URL_SECRET_ID")).await?;
+    // Extract the path parameters.
+    let params: PathParameters = match extract_path_parameters(&event) {
+        Ok(p) => p,
+        Err(e) => return Ok(e.into()),
+    };
 
     // Retrieve pagination parameters if any.
     let (page_size, page) = match pagination_parameters(&event) {
         Ok((page_size, page)) => (page_size, page),
         Err(e) => return Ok(e),
     };
 
-    let country = match parse_path_parameter::<String>(&event, "country") {
-        Ok(value) => value,
-        Err(e) => return Ok(e.into()),
-    };
-    let region = match parse_path_parameter::<String>(&event, "region") {
-        Ok(value) => value,
-        Err(e) => return Ok(e.into()),
-    };
-    let name = match parse_path_parameter::<String>(&event, "name") {
-        Ok(value) => value,
-        Err(e) => return Ok(e.into()),
-    };
+    // Set the database connection.
+    let db = database_connect(Some("DATABASE_URL_SECRET_ID")).await?;
 
-    if country.is_some() && region.is_some() && name.is_some() {
-        let select = city::Entity::find_by_id((country.unwrap(), region.unwrap(), name.unwrap()))
-            .find_also_related(census::Entity);
-        let model = select
-            .clone()
-            .paginate(&db, page_size)
-            .fetch_page(page - 1)
-            .await?;
-        if model.is_empty() {
-            return Ok(entry_not_found(&event).into());
-        }
-        let total_items = select.count(&db).await?;
-        build_paginated_response(json!(model), total_items, page, page_size, &event)
-    } else {
-        Ok(missing_parameter(&event, "country or region or name").into())
+    // Retrieve the city and associated census(es).
+    let select = city::Entity::find_by_id((params.country, params.region, params.name))
+        .find_also_related(census::Entity);
+    let model = select
+        .clone()
+        .paginate(&db, page_size)
+        .fetch_page(page - 1)
+        .await?;
+    if model.is_empty() {
+        return Ok(entry_not_found(&event).into());
     }
+    let total_items = select.count(&db).await?;
+    build_paginated_response(json!(model), total_items, page, page_size, &event)
 }
 
 // #[cfg(test)]

diff --git a/lambdas/src/cities/get-cities.rs b/lambdas/src/cities/get-cities.rs
@@ -1,8 +1,12 @@
 use dotenv::dotenv;
-use effortless::api::{entry_not_found, parse_path_parameter};
+use effortless::{api::entry_not_found, fragment::BnaRequestExt};
 use entity::city;
 use lambda_http::{run, service_fn, Body, Error, IntoResponse, Request, Response};
-use lambdas::{build_paginated_response, database_connect, pagination_parameters};
+use lambdas::{
+    build_paginated_response,
+    cities::{extract_path_parameters, PathParameters},
+    database_connect, pagination_parameters_2,
+};
 use sea_orm::{EntityTrait, PaginatorTrait};
 use serde_json::json;
 use tracing::info;
@@ -13,43 +17,43 @@ async fn function_handler(event: Request) -> Result<Response<Body>, Error> {
     // Set the database connection.
     let db = database_connect(Some("DATABASE_URL_SECRET_ID")).await?;
 
-    // Retrieve pagination parameters if any.
-    let (page_size, page) = match pagination_parameters(&event) {
-        Ok((page_size, page)) => (page_size, page),
-        Err(e) => return Ok(e),
-    };
-
-    let country = match parse_path_parameter::<String>(&event, "country") {
-        Ok(value) => value,
-        Err(e) => return Ok(e.into()),
-    };
-    let region = match parse_path_parameter::<String>(&event, "region") {
-        Ok(value) => value,
-        Err(e) => return Ok(e.into()),
-    };
-    let name = match parse_path_parameter::<String>(&event, "name") {
-        Ok(value) => value,
-        Err(e) => return Ok(e.into()),
-    };
+    // With params.
+    if event.has_path_parameters() {
+        let params: PathParameters = match extract_path_parameters(&event) {
+            Ok(p) => p,
+            Err(e) => return Ok(e.into()),
+        };
 
-    if country.is_some() && region.is_some() && name.is_some() {
-        let select = city::Entity::find_by_id((country.unwrap(), region.unwrap(), name.unwrap()));
+        let select = city::Entity::find_by_id((params.country, params.region, params.name));
         let model = select.one(&db).await?;
         let res: Response<Body> = match model {
             Some(model) => json!(model).into_response().await,
             None => entry_not_found(&event).into(),
         };
-        Ok(res)
-    } else {
-        let select = city::Entity::find();
-        let body = select
-            .clone()
-            .paginate(&db, page_size)
-            .fetch_page(page - 1)
-            .await?;
-        let total_items = select.count(&db).await?;
-        build_paginated_response(json!(body), total_items, page, page_size, &event)
+        return Ok(res);
     }
+
+    // Retrieve pagination parameters if any.
+    let pagination = match pagination_parameters_2(&event) {
+        Ok(p) => p,
+        Err(e) => return Ok(e),
+    };
+
+    // Without params.
+    let select = city::Entity::find();
+    let body = select
+        .clone()
+        .paginate(&db, pagination.page_size)
+        .fetch_page(pagination.page - 1)
+        .await?;
+    let total_items = select.count(&db).await?;
+    build_paginated_response(
+        json!(body),
+        total_items,
+        pagination.page,
+        pagination.page_size,
+        &event,
+    )
 }
 
 #[tokio::main]
@@ -70,25 +74,30 @@ async fn main() -> Result<(), Error> {
 
 #[cfg(test)]
 mod tests {
-    // use super::*;
-    // use lambda_http::{http, RequestExt};
-    // use std::collections::HashMap;
+    use super::*;
+    use lambda_http::{http, RequestExt};
+    use std::collections::HashMap;
 
-    // #[tokio::test]
-    // async fn test_handler_opportunity() {
-    //     let event = http::Request::builder()
-    //         .header(http::header::CONTENT_TYPE, "application/json")
-    //         .body(Body::Empty)
-    //         .expect("failed to build request")
-    //         .with_path_parameters(HashMap::from([
-    //             ("country".to_string(), "United%20States".to_string()),
-    //             ("region".to_string(), "Texas".to_string()),
-    //             ("name".to_string(), "Austin".to_string()),
-    //         ]))
-    //         .with_request_context(lambda_http::request::RequestContext::ApiGatewayV2(
-    //             lambda_http::aws_lambda_events::apigw::ApiGatewayV2httpRequestContext::default(),
-    //         ));
-    //     let r = function_handler(event).await.unwrap();
-    //     dbg!(r);
-    // }
+    #[test]
+    fn test_extract_path_parameters() {
+        let country: String = String::from("United States");
+        let region: String = String::from("Texas");
+        let name: String = String::from("Austin");
+        let event = http::Request::builder()
+            .header(http::header::CONTENT_TYPE, "application/json")
+            .body(Body::Empty)
+            .expect("failed to build request")
+            .with_path_parameters(HashMap::from([
+                ("country".to_string(), country.clone()),
+                ("region".to_string(), region.clone()),
+                ("name".to_string(), name.clone()),
+            ]))
+            .with_request_context(lambda_http::request::RequestContext::ApiGatewayV2(
+                lambda_http::aws_lambda_events::apigw::ApiGatewayV2httpRequestContext::default(),
+            ));
+        let r = extract_path_parameters(&event).unwrap();
+        assert_eq!(r.country, country);
+        assert_eq!(r.region, region);
+        assert_eq!(r.name, name);
+    }
 }