Merge pull request #343 from input-output-hk/lowhung/313-query-error-type

Refactor: Introduce standardized `RESTError` and `QueryError` types

Author: lowhung

.github/PULL_REQUEST_TEMPLATE.md

@@ -1,21 +1,25 @@
 # Pull Request Title
+
 <!--
 Use a short, descriptive title:
 Example: `node: fix crash when config is absent`
 -->
 
 ## Description
+
 <!--
 Describe what this PR does and why. Keep it concise but include enough context
 for reviewers who are not familiar with the area.
 -->
 
 ## Related Issue(s)
+
 <!--
 Link any related issues, e.g. `Fixes #123` or `Relates to #456`.
 -->
 
 ## How was this tested?
+
 <!--
 Describe the tests that you ran to verify your changes. Include instructions
 so reviewers can reproduce. Examples:
@@ -25,18 +29,21 @@ so reviewers can reproduce. Examples:
 -->
 
 ## Checklist
+
 - [ ] My code builds and passes local tests
 - [ ] I added/updated tests for my changes, where applicable
 - [ ] I updated documentation (if applicable)
 - [ ] CI is green for this PR
 
 ## Impact / Side effects
+
 <!--
 Describe any potential side effects, e.g. performance, compatibility, or security concerns.
 If the PR introduces a breaking change, explain migration steps for users.
 -->
 
 ## Reviewer notes / Areas to focus
+
 <!--
 If you want specific feedback, list files/functions to review or aspects you are unsure about.
 -->

common/src/queries/errors.rs

@@ -56,3 +56,11 @@ impl QueryError {
         }
     }
 }
+
+impl From<anyhow::Error> for QueryError {
+    fn from(err: anyhow::Error) -> Self {
+        Self::Internal {
+            message: err.to_string(),
+        }
+    }
+}

common/src/queries/utils.rs

@@ -1,61 +1,45 @@
-use anyhow::Result;
 use caryatid_sdk::Context;
 use serde::Serialize;
 use std::sync::Arc;
 
 use crate::messages::{Message, RESTResponse};
+use crate::queries::errors::QueryError;
+use crate::rest_error::RESTError;
 
 pub async fn query_state<T, F>(
     context: &Arc<Context<Message>>,
     topic: &str,
     request_msg: Arc<Message>,
     extractor: F,
-) -> Result<T, anyhow::Error>
+) -> Result<T, QueryError>
 where
-    F: FnOnce(Message) -> Result<T, anyhow::Error>,
+    F: FnOnce(Message) -> Result<T, QueryError>,
 {
-    // build message to query
     let raw_msg = context.message_bus.request(topic, request_msg).await?;
-
     let message = Arc::try_unwrap(raw_msg).unwrap_or_else(|arc| (*arc).clone());
 
     extractor(message)
 }
 
-/// The outer option in the extractor return value is whether the response was handled by F
 pub async fn rest_query_state<T, F>(
     context: &Arc<Context<Message>>,
     topic: &str,
     request_msg: Arc<Message>,
     extractor: F,
-) -> Result<RESTResponse>
+) -> Result<RESTResponse, RESTError>
 where
-    F: FnOnce(Message) -> Option<Result<Option<T>, anyhow::Error>>,
+    F: FnOnce(Message) -> Option<Result<T, QueryError>>,
     T: Serialize,
 {
-    let result = query_state(context, topic, request_msg, |response| {
-        match extractor(response) {
-            Some(response) => response,
-            None => Err(anyhow::anyhow!(
+    let data = query_state(context, topic, request_msg, |response| {
+        extractor(response).ok_or_else(|| {
+            QueryError::internal_error(format!(
                 "Unexpected response message type while calling {topic}"
-            )),
-        }
+            ))
+        })?
     })
-    .await;
-    match result {
-        Ok(result) => match result {
-            Some(result) => match serde_json::to_string(&result) {
-                Ok(json) => Ok(RESTResponse::with_json(200, &json)),
-                Err(e) => Ok(RESTResponse::with_text(
-                    500,
-                    &format!("Internal server error while calling {topic}: {e}"),
-                )),
-            },
-            None => Ok(RESTResponse::with_text(404, "Not found")),
-        },
-        Err(e) => Ok(RESTResponse::with_text(
-            500,
-            &format!("Internal server error while calling {topic}: {e}"),
-        )),
-    }
+    .await?;
+
+    let json = serde_json::to_string_pretty(&data)?;
+    Ok(RESTResponse::with_json(200, &json))
 }

common/src/rest_error.rs

@@ -42,12 +42,12 @@ impl RESTError {
 
     /// Parameter missing error
     pub fn param_missing(param_name: &str) -> Self {
-        RESTError::BadRequest(format!("{} parameter is missing", param_name))
+        RESTError::BadRequest(format!("Missing {} parameter", param_name))
     }
 
     /// Invalid parameter error
     pub fn invalid_param(param_name: &str, reason: &str) -> Self {
-        RESTError::BadRequest(format!("Invalid {}: {}", param_name, reason))
+        RESTError::BadRequest(format!("Invalid {} parameter: {}", param_name, reason))
     }
 
     /// Invalid hex string error

common/src/rest_helper.rs

@@ -1,6 +1,7 @@
 //! Helper functions for REST handlers
 
 use crate::messages::{Message, RESTResponse};
+use crate::rest_error::RESTError;
 use anyhow::{anyhow, Result};
 use caryatid_sdk::Context;
 use futures::future::Future;
@@ -17,20 +18,15 @@ pub fn handle_rest<F, Fut>(
 ) -> JoinHandle<()>
 where
     F: Fn() -> Fut + Send + Sync + Clone + 'static,
-    Fut: Future<Output = Result<RESTResponse>> + Send + 'static,
+    Fut: Future<Output = Result<RESTResponse, RESTError>> + Send + 'static,
 {
     context.handle(topic, move |message: Arc<Message>| {
         let handler = handler.clone();
         async move {
             let response = match message.as_ref() {
                 Message::RESTRequest(request) => {
                     info!("REST received {} {}", request.method, request.path);
-                    match handler().await {
-                        Ok(response) => response,
-                        Err(error) => {
-                            RESTResponse::with_text(500, &format!("{error:?}").to_string())
-                        }
-                    }
+                    handler().await.unwrap_or_else(|error| error.into())
                 }
                 _ => {
                     error!("Unexpected message type {:?}", message);
@@ -51,7 +47,7 @@ pub fn handle_rest_with_path_parameter<F, Fut>(
 ) -> JoinHandle<()>
 where
     F: Fn(&[&str]) -> Fut + Send + Sync + Clone + 'static,
-    Fut: Future<Output = Result<RESTResponse>> + Send + 'static,
+    Fut: Future<Output = Result<RESTResponse, RESTError>> + Send + 'static,
 {
     let topic_owned = topic.to_string();
     context.handle(topic, move |message: Arc<Message>| {
@@ -65,12 +61,7 @@ where
                         extract_params_from_topic_and_path(&topic_owned, &request.path_elements);
                     let params_slice: Vec<&str> = params_vec.iter().map(|s| s.as_str()).collect();
 
-                    match handler(&params_slice).await {
-                        Ok(response) => response,
-                        Err(error) => {
-                            RESTResponse::with_text(500, &format!("{error:?}").to_string())
-                        }
-                    }
+                    handler(&params_slice).await.unwrap_or_else(|error| error.into())
                 }
                 _ => {
                     error!("Unexpected message type {:?}", message);
@@ -83,26 +74,23 @@ where
     })
 }
 
-// Handle a REST request with query parameters
+/// Handle a REST request with query parameters
 pub fn handle_rest_with_query_parameters<F, Fut>(
     context: Arc<Context<Message>>,
     topic: &str,
     handler: F,
 ) -> JoinHandle<()>
 where
     F: Fn(HashMap<String, String>) -> Fut + Send + Sync + Clone + 'static,
-    Fut: Future<Output = Result<RESTResponse>> + Send + 'static,
+    Fut: Future<Output = Result<RESTResponse, RESTError>> + Send + 'static,
 {
     context.handle(topic, move |message: Arc<Message>| {
         let handler = handler.clone();
         async move {
             let response = match message.as_ref() {
                 Message::RESTRequest(request) => {
                     let params = request.query_parameters.clone();
-                    match handler(params).await {
-                        Ok(response) => response,
-                        Err(error) => RESTResponse::with_text(500, &format!("{error:?}")),
-                    }
+                    handler(params).await.unwrap_or_else(|error| error.into())
                 }
                 _ => RESTResponse::with_text(500, "Unexpected message in REST request"),
             };
@@ -112,15 +100,15 @@ where
     })
 }
 
-// Handle a REST request with path and query parameters
+/// Handle a REST request with path and query parameters
 pub fn handle_rest_with_path_and_query_parameters<F, Fut>(
     context: Arc<Context<Message>>,
     topic: &str,
     handler: F,
 ) -> JoinHandle<()>
 where
     F: Fn(&[&str], HashMap<String, String>) -> Fut + Send + Sync + Clone + 'static,
-    Fut: Future<Output = Result<RESTResponse>> + Send + 'static,
+    Fut: Future<Output = Result<RESTResponse, RESTError>> + Send + 'static,
 {
     let topic_owned = topic.to_string();
     context.handle(topic, move |message: Arc<Message>| {
@@ -133,10 +121,7 @@ where
                         extract_params_from_topic_and_path(&topic_owned, &request.path_elements);
                     let params_slice: Vec<&str> = params_vec.iter().map(|s| s.as_str()).collect();
                     let query_params = request.query_parameters.clone();
-                    match handler(&params_slice, query_params).await {
-                        Ok(response) => response,
-                        Err(error) => RESTResponse::with_text(500, &format!("{error:?}")),
-                    }
+                    handler(&params_slice, query_params).await.unwrap_or_else(|error| error.into())
                 }
                 _ => RESTResponse::with_text(500, "Unexpected message in REST request"),
             };

common/src/snapshot/NOTES.md

@@ -1,7 +1,10 @@
 # Bootstrapping from a Snapshot file
+
 We can boot an Acropolis node either from geneis and replay all of the blocks up to
 some point, or we can boot from a snapshot file. This module provides the components
-needed to boot from a snapshot file. See [snapshot_bootsrapper](../../../modules/snapshot_bootstrapper/src/snapshot_bootstrapper.rs) for the process that references and runs with these helpers.
+needed to boot from a snapshot file.
+See [snapshot_bootsrapper](../../../modules/snapshot_bootstrapper/src/snapshot_bootstrapper.rs) for the process that
+references and runs with these helpers.
 
 Booting from a snapshot takes minutes instead of the hours it takes to boot from
 genesis. It also allows booting from a given epoch which allows one to create tests
@@ -10,22 +13,27 @@ eras and will typically boot from Conway around epoch 305, 306, and 307. It take
 three epochs to have enough context to correctly calculate the rewards.
 
 The required data for boostrapping are:
+
 - snapshot files (each has an associated epoch number and point)
 - nonces
 - headers
 
 ## Snapshot Files
+
 The snapshots come from the Amaru project. In their words,
 "the snapshots we generated are different [from a Mithril snapshot]: they're
-the actual ledger state; i.e. the in-memory state that is constructed by iterating over each block up to a specific point. So, it's all the UTxOs, the set of pending governance actions, the account balance, etc.
+the actual ledger state; i.e. the in-memory state that is constructed by iterating over each block up to a specific
+point. So, it's all the UTxOs, the set of pending governance actions, the account balance, etc.
 If you get this from a trusted source, you don't need to do any replay, you can just start up and load this from disk.
-The format of these is completely non-standard; we just forked the haskell node and spit out whatever we needed to in CBOR."
+The format of these is completely non-standard; we just forked the haskell node and spit out whatever we needed to in
+CBOR."
 
 Snapshot files are referenced by their epoch number in the config.json file below.
 
 See [Amaru snapshot format](../../../docs/amaru-snapshot-structure.md)
 
 ## Configuration files
+
 There is a path for each network bootstrap configuration file. Network Should
 be one of 'mainnet', 'preprod', 'preview' or 'testnet_<magic>' where
 `magic` is a 32-bits unsigned value denoting a particular testnet.
@@ -43,7 +51,8 @@ a network name of `preview`, the expected layout for configuration files would b
 * `data/preview/nonces.json`: a list of `InitialNonces` values,
 * `data/preview/headers.json`: a list of `Point`s.
 
-These files are loaded by [snapshot_bootsrapper](../../../modules/snapshot_bootstrapper/src/snapshot_bootstrapper.rs) during bootup.
+These files are loaded by [snapshot_bootsrapper](../../../modules/snapshot_bootstrapper/src/snapshot_bootstrapper.rs)
+during bootup.
 
 ## Bootstrapping sequence
 
@@ -75,7 +84,8 @@ headers. Snapshot parsing is done while streaming the data to keep the memory
 footprint lower. As elements of the file are parsed, callbacks provide the data
 to the boostrapper which publishes the data on the message bus.
 
-There are TODO markers in [snapshot_bootsrapper](../../../modules/snapshot_bootstrapper/src/snapshot_bootstrapper.rs) that show where to add the
+There are TODO markers in [snapshot_bootsrapper](../../../modules/snapshot_bootstrapper/src/snapshot_bootstrapper.rs)
+that show where to add the
 publishing of the parsed snapshot data.
 
 

modules/drdd_state/src/rest.rs

@@ -1,6 +1,6 @@
 use crate::state::State;
+use acropolis_common::rest_error::RESTError;
 use acropolis_common::{extract_strict_query_params, messages::RESTResponse, DRepCredential};
-use anyhow::Result;
 use serde::Serialize;
 use std::{collections::HashMap, sync::Arc};
 use tokio::sync::Mutex;
@@ -17,16 +17,10 @@ struct DRDDResponse {
 pub async fn handle_drdd(
     state: Option<Arc<Mutex<State>>>,
     params: HashMap<String, String>,
-) -> Result<RESTResponse> {
-    let locked = match state.as_ref() {
-        Some(state) => state.lock().await,
-        None => {
-            return Ok(RESTResponse::with_text(
-                503,
-                "DRDD storage is disabled by configuration",
-            ));
-        }
-    };
+) -> Result<RESTResponse, RESTError> {
+    let state_arc = state.as_ref().ok_or_else(|| RESTError::storage_disabled("DRDD"))?;
+
+    let locked = state_arc.lock().await;
 
     extract_strict_query_params!(params, {
         "epoch" => epoch: Option<u64>,
@@ -36,10 +30,7 @@ pub async fn handle_drdd(
         Some(epoch) => match locked.get_epoch(epoch) {
             Some(drdd) => Some(drdd),
             None => {
-                return Ok(RESTResponse::with_text(
-                    404,
-                    &format!("DRDD not found for epoch {}", epoch),
-                ));
+                return Err(RESTError::not_found(&format!("DRDD in epoch {}", epoch)));
             }
         },
         None => locked.get_latest(),
@@ -65,26 +56,16 @@ pub async fn handle_drdd(
             no_confidence: drdd.no_confidence,
         };
 
-        match serde_json::to_string(&response) {
-            Ok(body) => Ok(RESTResponse::with_json(200, &body)),
-            Err(e) => Ok(RESTResponse::with_text(
-                500,
-                &format!("Internal server error retrieving DRep delegation distribution: {e}"),
-            )),
-        }
+        let body = serde_json::to_string(&response)?;
+        Ok(RESTResponse::with_json(200, &body))
     } else {
         let response = DRDDResponse {
             dreps: HashMap::new(),
             abstain: 0,
             no_confidence: 0,
         };
 
-        match serde_json::to_string(&response) {
-            Ok(body) => Ok(RESTResponse::with_json(200, &body)),
-            Err(_) => Ok(RESTResponse::with_text(
-                500,
-                "Internal server error serializing empty DRDD response",
-            )),
-        }
+        let body = serde_json::to_string(&response)?;
+        Ok(RESTResponse::with_json(200, &body))
     }
 }