refactoring to remove duplicate code in errors

Signed-off-by: George Pisaltu <gpl@amazon.com>

Author: georgepisaltu

api_server/src/parsed_request.rs

@@ -45,24 +45,25 @@ impl ParsedRequest {
             (Method::Get, "", None) => parse_get_instance_info(),
             (Method::Get, "machine-config", None) => parse_get_machine_config(),
             (Method::Get, "mmds", None) => parse_get_mmds(),
+            (Method::Get, _, Some(_)) => generic_error(Method::Get),
             (Method::Put, "actions", Some(body)) => parse_put_actions(body),
             (Method::Put, "boot-source", Some(body)) => parse_put_boot_source(body),
-            (Method::Put, "drives", maybe_body) => parse_put_drive(maybe_body, path_tokens.get(1)),
+            (Method::Put, "drives", Some(body)) => parse_put_drive(body, path_tokens.get(1)),
             (Method::Put, "logger", Some(body)) => parse_put_logger(body),
-            (Method::Put, "machine-config", maybe_body) => parse_put_machine_config(maybe_body),
+            (Method::Put, "machine-config", Some(body)) => parse_put_machine_config(body),
             (Method::Put, "mmds", Some(body)) => parse_put_mmds(body),
-            (Method::Put, "network-interfaces", maybe_body) => {
-                parse_put_net(maybe_body, path_tokens.get(1))
+            (Method::Put, "network-interfaces", Some(body)) => {
+                parse_put_net(body, path_tokens.get(1))
             }
             (Method::Put, "vsock", Some(body)) => parse_put_vsock(body),
-            (Method::Patch, "drives", maybe_body) => {
-                parse_patch_drive(maybe_body, path_tokens.get(1))
-            }
-            (Method::Patch, "machine-config", maybe_body) => parse_patch_machine_config(maybe_body),
+            (Method::Put, _, None) => generic_error(Method::Put),
+            (Method::Patch, "drives", Some(body)) => parse_patch_drive(body, path_tokens.get(1)),
+            (Method::Patch, "machine-config", Some(body)) => parse_patch_machine_config(body),
             (Method::Patch, "mmds", Some(body)) => parse_patch_mmds(body),
-            (Method::Patch, "network-interfaces", maybe_body) => {
-                parse_patch_net(maybe_body, path_tokens.get(1))
+            (Method::Patch, "network-interfaces", Some(body)) => {
+                parse_patch_net(body, path_tokens.get(1))
             }
+            (Method::Patch, _, None) => generic_error(Method::Patch),
             (method, unknown_uri, _) => {
                 Err(Error::InvalidPathMethod(unknown_uri.to_string(), method))
             }
@@ -128,6 +129,24 @@ fn describe(method: Method, path: &str, body: Option<&Body>) -> String {
     }
 }
 
+/// Generates a `GenericError` for each request method.
+pub fn generic_error(method: Method) -> Result<ParsedRequest, Error> {
+    match method {
+        Method::Get => Err(Error::Generic(
+            StatusCode::BadRequest,
+            "GET request cannot have a body.".to_string(),
+        )),
+        Method::Put => Err(Error::Generic(
+            StatusCode::BadRequest,
+            "Empty PUT request.".to_string(),
+        )),
+        Method::Patch => Err(Error::Generic(
+            StatusCode::BadRequest,
+            "Empty PATCH request.".to_string(),
+        )),
+    }
+}
+
 #[derive(Debug)]
 pub enum Error {
     // A generic error, with a given status code and message to be turned into a fault message.
@@ -235,6 +254,73 @@ mod tests {
         }
     }
 
+    #[test]
+    fn test_invalid_get() {
+        let (mut sender, receiver) = UnixStream::pair().unwrap();
+        let mut connection = HttpConnection::new(receiver);
+        sender
+            .write_all(
+                b"GET /mmds HTTP/1.1\r\n\
+                Content-Type: text/plain\r\n\
+                Content-Length: 4\r\n\r\nbody",
+            )
+            .unwrap();
+        assert!(connection.try_read().is_ok());
+        let req = connection.pop_parsed_request().unwrap();
+        match ParsedRequest::try_from_request(&req) {
+            Err(Error::Generic(StatusCode::BadRequest, err_msg)) => {
+                if err_msg != "GET request cannot have a body." {
+                    panic!("GET request with body.");
+                }
+            }
+            _ => panic!("GET request with body."),
+        };
+    }
+
+    #[test]
+    fn test_invalid_put() {
+        let (mut sender, receiver) = UnixStream::pair().unwrap();
+        let mut connection = HttpConnection::new(receiver);
+        sender
+            .write_all(
+                b"PUT /mmds HTTP/1.1\r\n\
+                Content-Type: application/json\r\n\r\n",
+            )
+            .unwrap();
+        assert!(connection.try_read().is_ok());
+        let req = connection.pop_parsed_request().unwrap();
+        match ParsedRequest::try_from_request(&req) {
+            Err(Error::Generic(StatusCode::BadRequest, err_msg)) => {
+                if err_msg != "Empty PUT request." {
+                    panic!("Empty PUT request.");
+                }
+            }
+            _ => panic!("Empty PUT request."),
+        };
+    }
+
+    #[test]
+    fn test_invalid_patch() {
+        let (mut sender, receiver) = UnixStream::pair().unwrap();
+        let mut connection = HttpConnection::new(receiver);
+        sender
+            .write_all(
+                b"PATCH /mmds HTTP/1.1\r\n\
+                Content-Type: application/json\r\n\r\n",
+            )
+            .unwrap();
+        assert!(connection.try_read().is_ok());
+        let req = connection.pop_parsed_request().unwrap();
+        match ParsedRequest::try_from_request(&req) {
+            Err(Error::Generic(StatusCode::BadRequest, err_msg)) => {
+                if err_msg != "Empty PATCH request." {
+                    panic!("Empty PATCH request.");
+                }
+            }
+            _ => panic!("Empty PATCH request."),
+        };
+    }
+
     #[test]
     fn test_error_into_response() {
         // Generic error.

api_server/src/request/drive.rs

@@ -5,7 +5,7 @@ use serde_json::{Map, Value};
 
 use super::super::VmmAction;
 use logger::{Metric, METRICS};
-use request::{Body, checked_id, Error, ParsedRequest, StatusCode};
+use request::{checked_id, Body, Error, ParsedRequest, StatusCode};
 use vmm::vmm_config::drive::BlockDeviceConfig;
 
 #[derive(Clone)]
@@ -76,10 +76,7 @@ impl PatchDrivePayload {
     }
 }
 
-pub fn parse_put_drive(
-    maybe_body: Option<&Body>,
-    id_from_path: Option<&&str>,
-) -> Result<ParsedRequest, Error> {
+pub fn parse_put_drive(body: &Body, id_from_path: Option<&&str>) -> Result<ParsedRequest, Error> {
     METRICS.put_api_requests.drive_count.inc();
     let id = match id_from_path {
         Some(&id) => checked_id(id)?,
@@ -88,35 +85,25 @@ pub fn parse_put_drive(
         }
     };
 
-    if let Some(body) = maybe_body {
-        let device_cfg = serde_json::from_slice::<BlockDeviceConfig>(body.raw()).map_err(|e| {
-            METRICS.put_api_requests.drive_fails.inc();
-            Error::SerdeJson(e)
-        })?;
+    let device_cfg = serde_json::from_slice::<BlockDeviceConfig>(body.raw()).map_err(|e| {
+        METRICS.put_api_requests.drive_fails.inc();
+        Error::SerdeJson(e)
+    })?;
 
-        if id != device_cfg.drive_id {
-            METRICS.put_api_requests.drive_fails.inc();
-            Err(Error::Generic(
-                StatusCode::BadRequest,
-                "The id from the path does not match the id from the body!".to_string(),
-            ))
-        } else {
-            Ok(ParsedRequest::Sync(VmmAction::InsertBlockDevice(
-                device_cfg,
-            )))
-        }
-    } else {
+    if id != device_cfg.drive_id {
+        METRICS.put_api_requests.drive_fails.inc();
         Err(Error::Generic(
             StatusCode::BadRequest,
-            "Empty PUT request.".to_string(),
+            "The id from the path does not match the id from the body!".to_string(),
         ))
+    } else {
+        Ok(ParsedRequest::Sync(VmmAction::InsertBlockDevice(
+            device_cfg,
+        )))
     }
 }
 
-pub fn parse_patch_drive(
-    maybe_body: Option<&Body>,
-    id_from_path: Option<&&str>,
-) -> Result<ParsedRequest, Error> {
+pub fn parse_patch_drive(body: &Body, id_from_path: Option<&&str>) -> Result<ParsedRequest, Error> {
     METRICS.patch_api_requests.drive_count.inc();
     let id = match id_from_path {
         Some(&id) => checked_id(id)?,
@@ -125,37 +112,30 @@ pub fn parse_patch_drive(
         }
     };
 
-    if let Some(body) = maybe_body {
-        METRICS.patch_api_requests.drive_count.inc();
-        let patch_drive_payload = PatchDrivePayload {
-            fields: serde_json::from_slice(body.raw()).map_err(|e| {
-                METRICS.patch_api_requests.drive_fails.inc();
-                Error::SerdeJson(e)
-            })?,
-        };
-
-        patch_drive_payload.validate()?;
-        let drive_id: String = patch_drive_payload.get_string_field_unchecked("drive_id");
-        let path_on_host: String = patch_drive_payload.get_string_field_unchecked("path_on_host");
-
-        if id != drive_id.as_str() {
+    METRICS.patch_api_requests.drive_count.inc();
+    let patch_drive_payload = PatchDrivePayload {
+        fields: serde_json::from_slice(body.raw()).map_err(|e| {
             METRICS.patch_api_requests.drive_fails.inc();
-            return Err(Error::Generic(
-                StatusCode::BadRequest,
-                String::from("The id from the path does not match the id from the body!"),
-            ));
-        }
+            Error::SerdeJson(e)
+        })?,
+    };
 
-        Ok(ParsedRequest::Sync(VmmAction::UpdateBlockDevicePath(
-            drive_id,
-            path_on_host,
-        )))
-    } else {
-        Err(Error::Generic(
+    patch_drive_payload.validate()?;
+    let drive_id: String = patch_drive_payload.get_string_field_unchecked("drive_id");
+    let path_on_host: String = patch_drive_payload.get_string_field_unchecked("path_on_host");
+
+    if id != drive_id.as_str() {
+        METRICS.patch_api_requests.drive_fails.inc();
+        return Err(Error::Generic(
             StatusCode::BadRequest,
-            "Empty PATCH request.".to_string(),
-        ))
+            String::from("The id from the path does not match the id from the body!"),
+        ));
     }
+
+    Ok(ParsedRequest::Sync(VmmAction::UpdateBlockDevicePath(
+        drive_id,
+        path_on_host,
+    )))
 }
 
 #[cfg(test)]
@@ -164,45 +144,43 @@ mod tests {
 
     #[test]
     fn test_parse_patch_request() {
-        assert!(parse_patch_drive(None, None).is_err());
-        assert!(parse_patch_drive(None, Some(&"id")).is_err());
-        assert!(parse_patch_drive(Some(&Body::new("invalid_payload")), Some(&"id")).is_err());
+        assert!(parse_patch_drive(&Body::new("invalid_payload"), Some(&"id")).is_err());
 
         // PATCH with invalid fields.
         let body = r#"{
                 "drive_id": "bar",
                 "is_read_only": false
               }"#;
-        assert!(parse_patch_drive(Some(&Body::new(body)), Some(&"2")).is_err());
+        assert!(parse_patch_drive(&Body::new(body), Some(&"2")).is_err());
 
         // PATCH with invalid types on fields. Adding a drive_id as number instead of string.
         let body = r#"{
                 "drive_id": 1000,
                 "path_on_host": "dummy"
               }"#;
-        let res = parse_patch_drive(Some(&Body::new(body)), Some(&"1000"));
+        let res = parse_patch_drive(&Body::new(body), Some(&"1000"));
         assert!(res.is_err());
 
         // PATCH with invalid types on fields. Adding a path_on_host as bool instead of string.
         let body = r#"{
                 "drive_id": 1000,
                 "path_on_host": true
               }"#;
-        let res = parse_patch_drive(Some(&Body::new(body)), Some(&"1000"));
+        let res = parse_patch_drive(&Body::new(body), Some(&"1000"));
         assert!(res.is_err());
 
         // PATCH with missing path_on_host field.
         let body = r#"{
                 "drive_id": "dummy_id"
               }"#;
-        let res = parse_patch_drive(Some(&Body::new(body)), Some(&"dummy_id"));
+        let res = parse_patch_drive(&Body::new(body), Some(&"dummy_id"));
         assert!(res.is_err());
 
         // PATCH with missing drive_id field.
         let body = r#"{
                 "path_on_host": true
               }"#;
-        let res = parse_patch_drive(Some(&Body::new(body)), Some(&"1000"));
+        let res = parse_patch_drive(&Body::new(body), Some(&"1000"));
         assert!(res.is_err());
 
         // PATCH that tries to update something else other than path_on_host.
@@ -211,20 +189,20 @@ mod tests {
                 "path_on_host": "dummy",
                 "is_read_only": false
               }"#;
-        let res = parse_patch_drive(Some(&Body::new(body)), Some(&"1234"));
+        let res = parse_patch_drive(&Body::new(body), Some(&"1234"));
         assert!(res.is_err());
 
         // PATCH with payload that is not a json.
         let body = r#"{
                 "fields": "dummy_field"
               }"#;
-        assert!(parse_patch_drive(Some(&Body::new(body)), Some(&"1234")).is_err());
+        assert!(parse_patch_drive(&Body::new(body), Some(&"1234")).is_err());
 
         let body = r#"{
                 "drive_id": "foo",
                 "path_on_host": "dummy"
               }"#;
-        match parse_patch_drive(Some(&Body::new(body)), Some(&"foo")) {
+        match parse_patch_drive(&Body::new(body), Some(&"foo")) {
             Ok(ParsedRequest::Sync(VmmAction::UpdateBlockDevicePath(a, b))) => {
                 assert_eq!(a, "foo".to_string());
                 assert_eq!(b, "dummy".to_string());
@@ -237,21 +215,19 @@ mod tests {
                 "drive_id": "foo",
                 "path_on_host": "dummy"
               }"#;
-        assert!(parse_patch_drive(Some(&Body::new(body)), Some(&"bar")).is_err());
+        assert!(parse_patch_drive(&Body::new(body), Some(&"bar")).is_err());
     }
 
     #[test]
     fn test_parse_put_request() {
-        assert!(parse_put_drive(None, None).is_err());
-        assert!(parse_put_drive(None, Some(&"id")).is_err());
-        assert!(parse_put_drive(Some(&Body::new("invalid_payload")), Some(&"id")).is_err());
+        assert!(parse_put_drive(&Body::new("invalid_payload"), Some(&"id")).is_err());
 
         // PATCH with invalid fields.
         let body = r#"{
                 "drive_id": "bar",
                 "is_read_only": false
               }"#;
-        assert!(parse_put_drive(Some(&Body::new(body)), Some(&"2")).is_err());
+        assert!(parse_put_drive(&Body::new(body), Some(&"2")).is_err());
 
         // PATCH with invalid types on fields. Adding a drive_id as number instead of string.
         let body = r#"{
@@ -273,9 +249,9 @@ mod tests {
                     }
                 }
             }"#;
-        assert!(parse_put_drive(Some(&Body::new(body)), Some(&"1000")).is_ok());
+        assert!(parse_put_drive(&Body::new(body), Some(&"1000")).is_ok());
 
-        assert!(parse_put_drive(Some(&Body::new(body)), Some(&"foo")).is_err());
+        assert!(parse_put_drive(&Body::new(body), Some(&"foo")).is_err());
     }
 
     #[test]