Skip to content

Commit

Permalink
[fix] hyperledger-iroha#3233: Remove duplicated error messages
Browse files Browse the repository at this point in the history
Signed-off-by: Daniil Polyakov <[email protected]>
  • Loading branch information
Arjentix committed Jun 2, 2023
1 parent 8476deb commit f407c99
Show file tree
Hide file tree
Showing 60 changed files with 2,387 additions and 1,005 deletions.
20 changes: 20 additions & 0 deletions Cargo.lock

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

2 changes: 2 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ members = [
"tools/parity_scale_decoder",
"version",
"version/derive",
"wasm_codec",
"wasm_codec/derive",
"wasm_builder",
]

Expand Down
35 changes: 24 additions & 11 deletions cli/src/torii/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ pub struct Torii {
pub enum Error {
/// Failed to execute or validate query
#[error("Failed to execute or validate query")]
Query(#[from] iroha_data_model::query::error::QueryExecutionFailure),
Query(#[from] iroha_data_model::ValidationFail),
/// Failed to accept transaction
#[error("Failed to accept transaction: {0}")]
AcceptTransaction(#[from] iroha_data_model::transaction::error::AcceptTransactionFailure),
Expand All @@ -67,16 +67,29 @@ pub enum Error {
}

/// Status code for query error response.
pub(crate) const fn query_status_code(
query_error: &iroha_data_model::query::error::QueryExecutionFailure,
) -> StatusCode {
use iroha_data_model::query::error::QueryExecutionFailure::*;
pub(crate) fn query_status_code(validation_error: &iroha_data_model::ValidationFail) -> StatusCode {
use iroha_data_model::{
isi::error::InstructionExecutionFailure, query::error::QueryExecutionFailure::*,
ValidationFail::*,
};

match query_error {
Evaluate(_) | Conversion(_) => StatusCode::BAD_REQUEST,
Signature(_) | Unauthorized => StatusCode::UNAUTHORIZED,
Permission(_) => StatusCode::FORBIDDEN,
Find(_) => StatusCode::NOT_FOUND,
match validation_error {
NotPermitted(_) => StatusCode::FORBIDDEN,
QueryFailed(query_error)
| InstructionFailed(InstructionExecutionFailure::Query(query_error)) => match query_error {
Evaluate(_) | Conversion(_) => StatusCode::BAD_REQUEST,
Signature(_) | Unauthorized => StatusCode::UNAUTHORIZED,
Find(_) => StatusCode::NOT_FOUND,
},
TooComplex => StatusCode::UNPROCESSABLE_ENTITY,
InternalError(_) => StatusCode::INTERNAL_SERVER_ERROR,
InstructionFailed(error) => {
iroha_logger::error!(
?error,
"Query validation failed with unexpected error. This means a bug inside Runtime Validator",
);
StatusCode::INTERNAL_SERVER_ERROR
}
}
}

Expand All @@ -93,7 +106,7 @@ impl Reply for Error {
}

impl Error {
const fn status_code(&self) -> StatusCode {
fn status_code(&self) -> StatusCode {
use Error::*;
match self {
Query(e) => query_status_code(e),
Expand Down
30 changes: 18 additions & 12 deletions cli/src/torii/routing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ use iroha_data_model::{
query::error::QueryExecutionFailure,
transaction::InBlock,
};
use iroha_logger::prelude::*;
#[cfg(feature = "telemetry")]
use iroha_telemetry::metrics::Status;
use pagination::{paginate, Paginate};
Expand Down Expand Up @@ -66,19 +67,23 @@ impl VerifiedQuery {
pub fn validate(
self,
wsv: &mut WorldStateView,
) -> Result<(ValidQueryRequest, PredicateBox), QueryExecutionFailure> {
let account_has_public_key = wsv.map_account(&self.payload.account_id, |account| {
account.signatories.contains(self.signature.public_key())
})?;
) -> Result<(ValidQueryRequest, PredicateBox), ValidationFail> {
let account_has_public_key = wsv
.map_account(&self.payload.account_id, |account| {
account.signatories.contains(self.signature.public_key())
})
.map_err(QueryExecutionFailure::from)?;
if !account_has_public_key {
return Err(QueryExecutionFailure::Signature(String::from(
"Signature public key doesn't correspond to the account.",
)));
))
.into());
}
wsv.validator_view()
.clone() // Cloning validator is a cheap operation
.validate(wsv, &self.payload.account_id, self.payload.query.clone())
.map_err(|err| QueryExecutionFailure::Permission(err.to_string()))?;
wsv.validator_view().clone().validate(
wsv,
&self.payload.account_id,
self.payload.query.clone(),
)?;
Ok((
ValidQueryRequest::new(self.payload.query),
self.payload.filter,
Expand Down Expand Up @@ -135,12 +140,12 @@ pub(crate) async fn handle_queries(
request: VersionedSignedQuery,
) -> Result<Scale<VersionedPaginatedQueryResult>> {
let VersionedSignedQuery::V1(request) = request;
let request: VerifiedQuery = request.try_into()?;
let request: VerifiedQuery = request.try_into().map_err(ValidationFail::from)?;

let (result, filter) = {
let mut wsv = sumeragi.wsv_clone();
let (valid_request, filter) = request.validate(&mut wsv)?;
let original_result = valid_request.execute(&wsv)?;
let original_result = valid_request.execute(&wsv).map_err(ValidationFail::from)?;
(filter.filter(original_result), filter)
};

Expand All @@ -155,7 +160,8 @@ pub(crate) async fn handle_queries(

let total = total
.try_into()
.map_err(|e: TryFromIntError| QueryExecutionFailure::Conversion(e.to_string()))?;
.map_err(|e: TryFromIntError| QueryExecutionFailure::Conversion(e.to_string()))
.map_err(ValidationFail::from)?;
let result = QueryResult(result);
let paginated_result = PaginatedQueryResult {
result,
Expand Down
55 changes: 25 additions & 30 deletions client/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use iroha_config::{client::Configuration, torii::uri, GetConfiguration, PostConf
use iroha_crypto::{HashOf, KeyPair};
use iroha_data_model::{
block::VersionedCommittedBlock, predicate::PredicateBox, prelude::*,
query::error::QueryExecutionFailure, transaction::TransactionPayload,
transaction::TransactionPayload,
};
use iroha_logger::prelude::*;
use iroha_telemetry::metrics::Status;
Expand Down Expand Up @@ -81,14 +81,15 @@ where
StatusCode::BAD_REQUEST
| StatusCode::UNAUTHORIZED
| StatusCode::FORBIDDEN
| StatusCode::NOT_FOUND => {
let res = QueryExecutionFailure::decode_all(&mut resp.body().as_ref());
| StatusCode::NOT_FOUND
| StatusCode::UNPROCESSABLE_ENTITY => {
let res = ValidationFail::decode_all(&mut resp.body().as_ref());
let err = res.wrap_err(
"Failed to decode error-response from Iroha. \
You are likely using a version of the client library \
that is incompatible with the version of the peer software",
)?;
Err(ClientQueryError::QueryError(err))
Err(ClientQueryError::Validation(err))
}
_ => Err(ResponseReport::with_msg("Unexpected query response", resp).into()),
}
Expand All @@ -103,19 +104,12 @@ where
/// Different errors as a result of query response handling
#[derive(Debug, thiserror::Error)]
pub enum ClientQueryError {
/// Certain Iroha query error
#[error("Query error: {0}")]
QueryError(QueryExecutionFailure),
/// Query validation error
#[error("Query validation error")]
Validation(#[from] ValidationFail),
/// Some other error
#[error("Other error: {0}")]
Other(eyre::Error),
}

impl From<eyre::Error> for ClientQueryError {
#[inline]
fn from(err: eyre::Error) -> Self {
Self::Other(err)
}
#[error("Other error")]
Other(#[from] eyre::Error),
}

impl From<ResponseReport> for ClientQueryError {
Expand Down Expand Up @@ -518,8 +512,8 @@ impl Client {
if let Event::Pipeline(this_event) = event? {
match this_event.status() {
PipelineStatus::Validating => {}
PipelineStatus::Rejected(reason) => {
return Err(reason.clone()).wrap_err("Transaction rejected")
PipelineStatus::Rejected(ref reason) => {
return Err(reason.clone().into());
}
PipelineStatus::Committed => return Ok(hash.transmute()),
}
Expand Down Expand Up @@ -1184,8 +1178,8 @@ pub mod stream_api {
fn drop(&mut self) {
let mut close = || -> eyre::Result<()> {
self.stream.close(None)?;
let mes = self.stream.read_message()?;
if !mes.is_close() {
let msg = self.stream.read_message()?;
if !msg.is_close() {
return Err(eyre!(
"Server hasn't sent `Close` message for websocket handshake"
));
Expand Down Expand Up @@ -1245,8 +1239,8 @@ pub mod stream_api {
pub async fn close(mut self) {
let close = async {
self.stream.close(None).await?;
if let Some(mes) = self.stream.next().await {
if !mes?.is_close() {
if let Some(msg) = self.stream.next().await {
if !msg?.is_close() {
eyre::bail!("Server hasn't sent `Close` message for websocket handshake");
}
}
Expand Down Expand Up @@ -1718,6 +1712,7 @@ mod tests {
#[cfg(test)]
mod query_errors_handling {
use http::Response;
use iroha_data_model::{query::error::QueryExecutionFailure, ValidationFail};

use super::*;

Expand All @@ -1727,24 +1722,24 @@ mod tests {
let responses = vec![
(
StatusCode::UNAUTHORIZED,
QueryExecutionFailure::Signature("whatever".to_owned()),
),
(
StatusCode::FORBIDDEN,
QueryExecutionFailure::Permission("whatever".to_owned()),
ValidationFail::QueryFailed(QueryExecutionFailure::Signature(
"whatever".to_owned(),
)),
),
(StatusCode::UNPROCESSABLE_ENTITY, ValidationFail::TooComplex),
(
StatusCode::NOT_FOUND,
// Here should be `Find`, but actually handler doesn't care
QueryExecutionFailure::Evaluate("whatever".to_owned()),
ValidationFail::QueryFailed(QueryExecutionFailure::Evaluate(
"whatever".to_owned(),
)),
),
];

for (status_code, err) in responses {
let resp = Response::builder().status(status_code).body(err.encode())?;

match sut.handle(resp) {
Err(ClientQueryError::QueryError(actual)) => {
Err(ClientQueryError::Validation(actual)) => {
// PartialEq isn't implemented, so asserting by encoded repr
assert_eq!(actual.encode(), err.encode());
}
Expand Down
3 changes: 2 additions & 1 deletion client/tests/integration/events/data.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,8 @@ fn wasm_execution_should_produce_events() -> Result<()> {
// It's expected that hex values are of even length
write!(
isi_calls,
"(call $exec_isi (i32.const {ptr_offset}) (i32.const {ptr_len}))",
"(call $exec_isi (i32.const {ptr_offset}) (i32.const {ptr_len}))
drop",
ptr_offset = ptr_offset / 2,
ptr_len = ptr_len / 2,
)?;
Expand Down
2 changes: 1 addition & 1 deletion client/tests/integration/offline_peers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use test_network::*;
use tokio::runtime::Runtime;

#[test]
fn genesis_block_is_commited_with_some_offline_peers() -> Result<()> {
fn genesis_block_is_committed_with_some_offline_peers() -> Result<()> {
// Given
let rt = Runtime::test();

Expand Down
8 changes: 6 additions & 2 deletions client/tests/integration/permissions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,9 @@ fn permissions_disallow_asset_transfer() {
//Then
assert!(matches!(
rejection_reason,
&PipelineRejectionReason::Transaction(TransactionRejectionReason::NotPermitted(_))
&PipelineRejectionReason::Transaction(TransactionRejectionReason::Validation(
ValidationFail::NotPermitted(_)
))
));
let alice_assets = get_assets(&mut iroha_client, &alice_id);
assert_eq!(alice_assets, alice_start_assets);
Expand Down Expand Up @@ -146,7 +148,9 @@ fn permissions_disallow_asset_burn() {
//Then
assert!(matches!(
rejection_reason,
&PipelineRejectionReason::Transaction(TransactionRejectionReason::NotPermitted(_))
&PipelineRejectionReason::Transaction(TransactionRejectionReason::Validation(
ValidationFail::NotPermitted(_)
))
));

let alice_assets = get_assets(&mut iroha_client, &alice_id);
Expand Down
8 changes: 6 additions & 2 deletions client/tests/integration/queries/asset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,9 @@ fn find_asset_total_quantity() -> Result<()> {
));
assert!(matches!(
result,
Err(ClientQueryError::QueryError(QueryExecutionFailure::Find(_)))
Err(ClientQueryError::Validation(ValidationFail::QueryFailed(
QueryExecutionFailure::Find(_)
)))
));
}

Expand Down Expand Up @@ -202,7 +204,9 @@ fn find_asset_total_quantity() -> Result<()> {
));
assert!(matches!(
result,
Err(ClientQueryError::QueryError(QueryExecutionFailure::Find(_)))
Err(ClientQueryError::Validation(ValidationFail::QueryFailed(
QueryExecutionFailure::Find(_)
)))
));

Ok(())
Expand Down
4 changes: 2 additions & 2 deletions client/tests/integration/queries/role.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,8 @@ fn find_unregistered_role_by_id() {
// Not found error
assert!(matches!(
found_role,
Err(client::ClientQueryError::QueryError(
QueryExecutionFailure::Find(_)
Err(client::ClientQueryError::Validation(
ValidationFail::QueryFailed(QueryExecutionFailure::Find(_))
))
));
}
Expand Down
4 changes: 3 additions & 1 deletion client/tests/integration/query_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ fn non_existent_account_is_specific_error() {
.expect_err("Should error");

match err {
ClientQueryError::QueryError(QueryExecutionFailure::Find(err)) => match *err {
ClientQueryError::Validation(ValidationFail::QueryFailed(QueryExecutionFailure::Find(
err,
))) => match *err {
FindError::Domain(id) => assert_eq!(id.name.as_ref(), "regalia"),
x => panic!("FindError::Domain expected, got {x:?}"),
},
Expand Down
Loading

0 comments on commit f407c99

Please sign in to comment.