Skip to content

Commit 4b07f59

Browse files
lowhunglowhung
committed
Refactor: Simplify QueryError variants, remove unused PartialNotFound and QueryFailed, standardize error handling with internal_error, and update method signatures across modules
1 parent 571d63d commit 4b07f59

File tree

20 files changed

+133
-160
lines changed

20 files changed

+133
-160
lines changed

‎common/src/queries/errors.rs‎

Lines changed: 4 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -8,17 +8,14 @@ pub enum QueryError {
88
NotFound { resource: String },
99

1010
/// An error occurred while processing the query
11-
QueryFailed { message: String },
11+
Internal { message: String },
1212

1313
/// Storage backend is disabled in configuration
1414
StorageDisabled { storage_type: String },
1515

1616
/// Invalid request parameters
1717
InvalidRequest { message: String },
1818

19-
/// One or more resources in a batch query were not found
20-
PartialNotFound { message: String },
21-
2219
/// Query variant is not implemented yet
2320
NotImplemented { query: String },
2421
}
@@ -30,8 +27,8 @@ impl QueryError {
3027
}
3128
}
3229

33-
pub fn query_failed(message: impl Into<String>) -> Self {
34-
Self::QueryFailed {
30+
pub fn internal_error(message: impl Into<String>) -> Self {
31+
Self::Internal {
3532
message: message.into(),
3633
}
3734
}
@@ -48,12 +45,6 @@ impl QueryError {
4845
}
4946
}
5047

51-
pub fn partial_not_found(message: impl Into<String>) -> Self {
52-
Self::PartialNotFound {
53-
message: message.into(),
54-
}
55-
}
56-
5748
pub fn not_implemented(query: impl Into<String>) -> Self {
5849
Self::NotImplemented {
5950
query: query.into(),
@@ -65,12 +56,11 @@ impl fmt::Display for QueryError {
6556
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
6657
match self {
6758
Self::NotFound { resource } => write!(f, "Not found: {}", resource),
68-
Self::QueryFailed { message } => write!(f, "Query failed: {}", message),
59+
Self::Internal { message } => write!(f, "Query failed: {}", message),
6960
Self::StorageDisabled { storage_type } => {
7061
write!(f, "{} storage is not enabled", storage_type)
7162
}
7263
Self::InvalidRequest { message } => write!(f, "Invalid request: {}", message),
73-
Self::PartialNotFound { message } => write!(f, "Partial result: {}", message),
7464
Self::NotImplemented { query } => write!(f, "Query not implemented: {}", query),
7565
}
7666
}

‎common/src/queries/utils.rs‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -21,15 +21,14 @@ where
2121
.message_bus
2222
.request(topic, request_msg)
2323
.await
24-
.map_err(|e| QueryError::query_failed(e.to_string()))?;
24+
.map_err(|e| QueryError::internal_error(e.to_string()))?;
2525

2626
let message = Arc::try_unwrap(raw_msg).unwrap_or_else(|arc| (*arc).clone());
2727

2828
extractor(message)
2929
}
3030

31-
/// Query state and return a REST response directly
32-
/// This is a convenience function for simple handlers that just fetch and serialize data
31+
/// The outer option in the extractor return value is whether the response was handled by F
3332
pub async fn rest_query_state<T, F>(
3433
context: &Arc<Context<Message>>,
3534
topic: &str,
@@ -42,13 +41,13 @@ where
4241
{
4342
let data = query_state(context, topic, request_msg, |response| {
4443
extractor(response).unwrap_or_else(|| {
45-
Err(QueryError::query_failed(format!(
44+
Err(QueryError::internal_error(format!(
4645
"Unexpected response message type from {topic}"
4746
)))
4847
})
4948
})
50-
.await?; // QueryError auto-converts to RESTError via From trait
49+
.await?;
5150

52-
let json = serde_json::to_string_pretty(&data)?; // Uses From<serde_json::Error> for RESTError
51+
let json = serde_json::to_string_pretty(&data)?;
5352
Ok(RESTResponse::with_json(200, &json))
5453
}

‎common/src/rest_error.rs‎

Lines changed: 11 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ use crate::queries::errors::QueryError;
22
use anyhow::Error as AnyhowError;
33
use caryatid_module_rest_server::messages::RESTResponse;
44
use std::fmt;
5-
use std::num::ParseIntError;
65

76
/// Standard error types for the application
87
#[derive(Debug)]
@@ -49,12 +48,12 @@ impl RESTError {
4948
RESTError::BadRequest("Invalid hex string".to_string())
5049
}
5150

52-
/// Resource not found error (flexible message)
51+
/// Resource not found error
5352
pub fn not_found(message: &str) -> Self {
5453
RESTError::NotFound(message.to_string())
5554
}
5655

57-
/// Feature not implemented error (flexible message)
56+
/// Feature not implemented error
5857
pub fn not_implemented(message: &str) -> Self {
5958
RESTError::NotImplemented(message.to_string())
6059
}
@@ -64,21 +63,16 @@ impl RESTError {
6463
RESTError::NotImplemented(format!("{} storage is disabled in config", storage_type))
6564
}
6665

67-
/// Unexpected response error (flexible message)
66+
/// Unexpected response error
6867
pub fn unexpected_response(message: &str) -> Self {
6968
RESTError::InternalServerError(message.to_string())
7069
}
7170

72-
/// Query failed error (flexible message)
71+
/// Query failed error
7372
pub fn query_failed(message: &str) -> Self {
7473
RESTError::InternalServerError(message.to_string())
7574
}
7675

77-
/// Serialization failed error
78-
pub fn serialization_failed(what: &str, error: impl fmt::Display) -> Self {
79-
RESTError::InternalServerError(format!("Failed to serialize {}: {}", what, error))
80-
}
81-
8276
/// Encoding failed error
8377
pub fn encoding_failed(what: &str) -> Self {
8478
RESTError::InternalServerError(format!("Failed to encode {}", what))
@@ -107,12 +101,12 @@ impl From<AnyhowError> for RESTError {
107101
}
108102
}
109103

110-
/// Convert ParseIntError to RESTError (400 Bad Request)
111-
impl From<ParseIntError> for RESTError {
112-
fn from(error: ParseIntError) -> Self {
113-
RESTError::BadRequest(error.to_string())
114-
}
115-
}
104+
// /// Convert ParseIntError to RESTError (400 Bad Request)
105+
// impl From<ParseIntError> for RESTError {
106+
// fn from(error: ParseIntError) -> Self {
107+
// RESTError::BadRequest(error.to_string())
108+
// }
109+
// }
116110

117111
/// Convert hex decode errors to RESTError (400 Bad Request)
118112
impl From<hex::FromHexError> for RESTError {
@@ -147,8 +141,7 @@ impl From<QueryError> for RESTError {
147141
fn from(error: QueryError) -> Self {
148142
match error {
149143
QueryError::NotFound { resource } => RESTError::NotFound(resource),
150-
QueryError::PartialNotFound { message } => RESTError::BadRequest(message),
151-
QueryError::QueryFailed { message } => RESTError::InternalServerError(message),
144+
QueryError::Internal { message } => RESTError::InternalServerError(message),
152145
QueryError::StorageDisabled { storage_type } => {
153146
RESTError::storage_disabled(&storage_type)
154147
}
@@ -158,8 +151,6 @@ impl From<QueryError> for RESTError {
158151
}
159152
}
160153

161-
pub type AppResult<T> = Result<T, RESTError>;
162-
163154
#[cfg(test)]
164155
mod tests {
165156
use super::*;
@@ -192,13 +183,6 @@ mod tests {
192183
assert_eq!(app_error.status_code(), 500);
193184
}
194185

195-
#[test]
196-
fn test_from_parse_int_error() {
197-
let result: Result<u64, _> = "not_a_number".parse();
198-
let app_error: RESTError = result.unwrap_err().into();
199-
assert_eq!(app_error.status_code(), 400);
200-
}
201-
202186
#[test]
203187
fn test_from_hex_error() {
204188
let result = hex::decode("not_hex_gg");

‎modules/accounts_state/src/accounts_state.rs‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -552,7 +552,7 @@ impl AccountsState {
552552
Some(map) => {
553553
AccountsStateQueryResponse::AccountsDrepDelegationsMap(map)
554554
}
555-
None => AccountsStateQueryResponse::Error(QueryError::query_failed(
555+
None => AccountsStateQueryResponse::Error(QueryError::internal_error(
556556
"Error retrieving DRep delegations map",
557557
)),
558558
}
@@ -568,7 +568,7 @@ impl AccountsState {
568568
match state.get_accounts_utxo_values_map(stake_addresses) {
569569
Some(map) => AccountsStateQueryResponse::AccountsUtxoValuesMap(map),
570570
None => AccountsStateQueryResponse::Error(
571-
QueryError::partial_not_found("One or more accounts not found"),
571+
QueryError::not_found("One or more accounts not found"),
572572
),
573573
}
574574
}
@@ -577,7 +577,7 @@ impl AccountsState {
577577
match state.get_accounts_utxo_values_sum(stake_addresses) {
578578
Some(sum) => AccountsStateQueryResponse::AccountsUtxoValuesSum(sum),
579579
None => AccountsStateQueryResponse::Error(
580-
QueryError::partial_not_found("One or more accounts not found"),
580+
QueryError::not_found("One or more accounts not found"),
581581
),
582582
}
583583
}
@@ -586,7 +586,7 @@ impl AccountsState {
586586
match state.get_accounts_balances_map(stake_addresses) {
587587
Some(map) => AccountsStateQueryResponse::AccountsBalancesMap(map),
588588
None => AccountsStateQueryResponse::Error(
589-
QueryError::partial_not_found("One or more accounts not found"),
589+
QueryError::not_found("One or more accounts not found"),
590590
),
591591
}
592592
}
@@ -601,15 +601,15 @@ impl AccountsState {
601601
match state.get_account_balances_sum(stake_addresses) {
602602
Some(sum) => AccountsStateQueryResponse::AccountsBalancesSum(sum),
603603
None => AccountsStateQueryResponse::Error(
604-
QueryError::partial_not_found("One or more accounts not found"),
604+
QueryError::not_found("One or more accounts not found"),
605605
),
606606
}
607607
}
608608

609609
AccountsStateQuery::GetSPDDByEpoch { epoch } => match spdd_store_guard {
610610
Some(spdd_store) => match spdd_store.query_by_epoch(*epoch) {
611611
Ok(result) => AccountsStateQueryResponse::SPDDByEpoch(result),
612-
Err(e) => AccountsStateQueryResponse::Error(QueryError::query_failed(
612+
Err(e) => AccountsStateQueryResponse::Error(QueryError::internal_error(
613613
e.to_string(),
614614
)),
615615
},
@@ -626,7 +626,7 @@ impl AccountsState {
626626
AccountsStateQueryResponse::SPDDByEpochAndPool(result)
627627
}
628628
Err(e) => AccountsStateQueryResponse::Error(
629-
QueryError::query_failed(e.to_string()),
629+
QueryError::internal_error(e.to_string()),
630630
),
631631
}
632632
}

‎modules/address_state/src/address_state.rs‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,6 @@
44
55
use std::sync::Arc;
66

7-
use crate::{
8-
immutable_address_store::ImmutableAddressStore,
9-
state::{AddressStorageConfig, State},
10-
};
11-
use acropolis_common::queries::errors::QueryError;
127
use acropolis_common::{
138
messages::{CardanoMessage, Message, StateQuery, StateQueryResponse},
149
queries::addresses::{
@@ -21,6 +16,11 @@ use caryatid_sdk::{module, Context, Module, Subscription};
2116
use config::Config;
2217
use tokio::sync::{mpsc, Mutex};
2318
use tracing::{error, info};
19+
use acropolis_common::queries::errors::QueryError;
20+
use crate::{
21+
immutable_address_store::ImmutableAddressStore,
22+
state::{AddressStorageConfig, State},
23+
};
2424
mod immutable_address_store;
2525
mod state;
2626
mod volatile_addresses;
@@ -213,7 +213,7 @@ impl AddressState {
213213
Ok(None) => AddressStateQueryResponse::Error(QueryError::not_found(
214214
"Address not found",
215215
)),
216-
Err(e) => AddressStateQueryResponse::Error(QueryError::query_failed(
216+
Err(e) => AddressStateQueryResponse::Error(QueryError::internal_error(
217217
e.to_string(),
218218
)),
219219
}
@@ -224,15 +224,15 @@ impl AddressState {
224224
Ok(None) => AddressStateQueryResponse::Error(QueryError::not_found(
225225
"Address not found",
226226
)),
227-
Err(e) => AddressStateQueryResponse::Error(QueryError::query_failed(
227+
Err(e) => AddressStateQueryResponse::Error(QueryError::internal_error(
228228
e.to_string(),
229229
)),
230230
}
231231
}
232232
AddressStateQuery::GetAddressTotals { address } => {
233233
match state.get_address_totals(address).await {
234234
Ok(totals) => AddressStateQueryResponse::AddressTotals(totals),
235-
Err(e) => AddressStateQueryResponse::Error(QueryError::query_failed(
235+
Err(e) => AddressStateQueryResponse::Error(QueryError::internal_error(
236236
e.to_string(),
237237
)),
238238
}

‎modules/assets_state/src/assets_state.rs‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -298,7 +298,7 @@ impl AssetsState {
298298
let reg = registry.lock().await;
299299
match state.get_assets_list(&reg) {
300300
Ok(list) => AssetsStateQueryResponse::AssetsList(list),
301-
Err(e) => AssetsStateQueryResponse::Error(QueryError::query_failed(
301+
Err(e) => AssetsStateQueryResponse::Error(QueryError::internal_error(
302302
e.to_string(),
303303
)),
304304
}
@@ -316,7 +316,7 @@ impl AssetsState {
316316
)))
317317
}
318318
Err(e) => AssetsStateQueryResponse::Error(
319-
QueryError::query_failed(e.to_string()),
319+
QueryError::internal_error(e.to_string()),
320320
),
321321
},
322322
None => {
@@ -349,7 +349,7 @@ impl AssetsState {
349349
)))
350350
}
351351
Err(e) => AssetsStateQueryResponse::Error(
352-
QueryError::query_failed(e.to_string()),
352+
QueryError::internal_error(e.to_string()),
353353
),
354354
},
355355
None => {
@@ -382,7 +382,7 @@ impl AssetsState {
382382
)))
383383
}
384384
Err(e) => AssetsStateQueryResponse::Error(
385-
QueryError::query_failed(e.to_string()),
385+
QueryError::internal_error(e.to_string()),
386386
),
387387
},
388388
None => {
@@ -413,7 +413,7 @@ impl AssetsState {
413413
)))
414414
}
415415
Err(e) => AssetsStateQueryResponse::Error(
416-
QueryError::query_failed(e.to_string()),
416+
QueryError::internal_error(e.to_string()),
417417
),
418418
},
419419
None => {
@@ -438,7 +438,7 @@ impl AssetsState {
438438
Ok(None) => AssetsStateQueryResponse::Error(QueryError::not_found(
439439
format!("Assets for policy {}", hex::encode(policy)),
440440
)),
441-
Err(e) => AssetsStateQueryResponse::Error(QueryError::query_failed(
441+
Err(e) => AssetsStateQueryResponse::Error(QueryError::internal_error(
442442
e.to_string(),
443443
)),
444444
}

‎modules/chain_store/src/chain_store.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,14 +73,14 @@ impl ChainStore {
7373
};
7474
let Some(state) = query_history.lock().await.current().cloned() else {
7575
return Arc::new(Message::StateQueryResponse(StateQueryResponse::Blocks(
76-
BlocksStateQueryResponse::Error(QueryError::query_failed(
76+
BlocksStateQueryResponse::Error(QueryError::internal_error(
7777
"Uninitialized state",
7878
)),
7979
)));
8080
};
8181
let res =
8282
Self::handle_blocks_query(&query_store, &state, query).unwrap_or_else(|err| {
83-
BlocksStateQueryResponse::Error(QueryError::query_failed(err.to_string()))
83+
BlocksStateQueryResponse::Error(QueryError::internal_error(err.to_string()))
8484
});
8585
Arc::new(Message::StateQueryResponse(StateQueryResponse::Blocks(res)))
8686
}

0 commit comments

Comments
 (0)