From 371866324664388b5d9caee91327acc59a5909d8 Mon Sep 17 00:00:00 2001 From: Akvinikym Date: Wed, 24 Oct 2018 12:16:41 +0300 Subject: [PATCH 01/17] Introduced error codes to SQL Signed-off-by: Akvinikym --- irohad/ametsuchi/command_executor.hpp | 6 +- .../impl/postgres_command_executor.cpp | 530 ++++-------------- .../impl/postgres_command_executor.hpp | 3 +- .../ametsuchi/postgres_executor_test.cpp | 10 + 4 files changed, 137 insertions(+), 412 deletions(-) diff --git a/irohad/ametsuchi/command_executor.hpp b/irohad/ametsuchi/command_executor.hpp index ec31a98359..17d5ea0f0e 100644 --- a/irohad/ametsuchi/command_executor.hpp +++ b/irohad/ametsuchi/command_executor.hpp @@ -27,7 +27,7 @@ namespace shared_model { class SetQuorum; class SubtractAssetQuantity; class TransferAsset; - } // namespace interface + } // namespace interface } // namespace shared_model namespace iroha { @@ -38,8 +38,10 @@ namespace iroha { * Contains command name, as well as an error message */ struct CommandError { + using ErrorCodeType = uint32_t; + std::string command_name; - std::string error_message; + ErrorCodeType error_code; std::string toString() const; }; diff --git a/irohad/ametsuchi/impl/postgres_command_executor.cpp b/irohad/ametsuchi/impl/postgres_command_executor.cpp index 1a7225a50c..bfd9f86c00 100644 --- a/irohad/ametsuchi/impl/postgres_command_executor.cpp +++ b/irohad/ametsuchi/impl/postgres_command_executor.cpp @@ -76,10 +76,10 @@ namespace { } iroha::expected::Error makeCommandError( - const std::string &error_message, - const std::string &command_name) noexcept { + const std::string &command_name, + const iroha::ametsuchi::CommandError::ErrorCodeType code) noexcept { return iroha::expected::makeError( - iroha::ametsuchi::CommandError{command_name, error_message}); + iroha::ametsuchi::CommandError{command_name, code}); } /** @@ -89,25 +89,54 @@ namespace { * @param sql - connection on which to execute statement * @param cmd - sql query to be executed * @param command_name - which command executes a query - * @param error_generator functions which must generate error message - * Functions are passed instead of string to avoid overhead of string - * construction in successful case. * @return CommandResult with command name and error message */ iroha::ametsuchi::CommandResult executeQuery( soci::session &sql, const std::string &cmd, - const std::string &command_name, - std::vector> &error_generator) noexcept { + const std::string &command_name) noexcept { uint32_t result; try { sql << cmd, soci::into(result); if (result != 0) { - return makeCommandError(error_generator[result - 1](), command_name); + return makeCommandError(command_name, result); } return {}; } catch (std::exception &e) { - return makeCommandError(e.what(), command_name); + // try to parse the SQL error to get an actual one + std::string exc_body = e.what(); + + if (exc_body.find("DETAIL: Key (account_id)=") != std::string::npos + and exc_body.find("is not present in table") != std::string::npos) { + return makeCommandError(command_name, 2); + } + + if (exc_body.find("DETAIL: Key (role_id)=") != std::string::npos + and exc_body.find("is not present in table") != std::string::npos) { + return makeCommandError(command_name, 6); + } + + if (exc_body.find("DETAIL: Key (domain_id)=") != std::string::npos + and exc_body.find("is not present in table") != std::string::npos) { + return makeCommandError(command_name, 7); + } + + if (exc_body.find("DETAIL: Key (asset_id)=") != std::string::npos + and exc_body.find("already exists") != std::string::npos) { + return makeCommandError(command_name, 9); + } + + if (exc_body.find("DETAIL: Key (domain_id)=") != std::string::npos + and exc_body.find("already exists") != std::string::npos) { + return makeCommandError(command_name, 10); + } + + if (exc_body.find("DETAIL: Key (role_id)=") != std::string::npos + and exc_body.find("already exists") != std::string::npos) { + return makeCommandError(command_name, 11); + } + + return makeCommandError(command_name, 1); } } @@ -260,7 +289,7 @@ namespace iroha { WHEN NOT EXISTS (SELECT value FROM new_value WHERE value < 2::decimal ^ (256 - $3) LIMIT 1) THEN 4 - ELSE 5 + ELSE 1 END AS result;)"; const std::string PostgresCommandExecutor::addPeerBase = R"( @@ -276,7 +305,7 @@ namespace iroha { ) SELECT CASE WHEN EXISTS (SELECT * FROM inserted) THEN 0 %s - ELSE 2 END AS result)"; + ELSE 1 END AS result)"; const std::string PostgresCommandExecutor::addSignatoryBase = R"( PREPARE %s (text, text, text) AS @@ -301,8 +330,8 @@ namespace iroha { SELECT CASE WHEN EXISTS (SELECT * FROM insert_account_signatory) THEN 0 %s - WHEN EXISTS (SELECT * FROM insert_signatory) THEN 2 - ELSE 3 + WHEN EXISTS (SELECT * FROM insert_signatory) THEN 3 + ELSE 1 END AS RESULT;)"; const std::string PostgresCommandExecutor::appendRoleBase = R"( @@ -316,7 +345,7 @@ namespace iroha { SELECT CASE WHEN EXISTS (SELECT * FROM inserted) THEN 0 %s - ELSE 4 + ELSE 1 END AS result)"; const std::string PostgresCommandExecutor::createAccountBase = R"( @@ -366,15 +395,15 @@ namespace iroha { WHEN EXISTS (SELECT * FROM insert_account_role) THEN 0 %s WHEN NOT EXISTS (SELECT * FROM account - WHERE account_id = $2) THEN 2 + WHERE account_id = $2) THEN 3 WHEN NOT EXISTS (SELECT * FROM account_has_signatory WHERE account_id = $2 - AND public_key = $4) THEN 3 + AND public_key = $4) THEN 4 WHEN NOT EXISTS (SELECT * FROM account_has_roles WHERE account_id = account_id AND role_id = ( SELECT default_role FROM get_domain_default_role) - ) THEN 4 - ELSE 5 + ) THEN 8 + ELSE 1 END AS result)"; const std::string PostgresCommandExecutor::createAssetBase = R"( @@ -390,7 +419,7 @@ namespace iroha { ) SELECT CASE WHEN EXISTS (SELECT * FROM inserted) THEN 0 %s - ELSE 2 END AS result)"; + ELSE 1 END AS result)"; const std::string PostgresCommandExecutor::createDomainBase = R"( PREPARE %s (text, text, text) AS @@ -424,8 +453,8 @@ namespace iroha { SELECT CASE WHEN EXISTS (SELECT * FROM insert_role_permissions) THEN 0 %s - WHEN EXISTS (SELECT * FROM role WHERE role_id = $2) THEN 1 - ELSE 4 + WHEN EXISTS (SELECT * FROM role WHERE role_id = $2) THEN 5 + ELSE 1 END AS result)"; const std::string PostgresCommandExecutor::detachRoleBase = R"( @@ -440,8 +469,14 @@ namespace iroha { RETURNING (1) ) SELECT CASE WHEN EXISTS (SELECT * FROM deleted) THEN 0 + WHEN NOT EXISTS (SELECT * FROM account + WHERE account_id = $2) THEN 2 + WHEN NOT EXISTS (SELECT * FROM role + WHERE role_id = $3) THEN 6 + WHEN NOT EXISTS (SELECT * FROM account_has_roles + WHERE account_id=$2 AND role_id=$3) THEN 3 %s - ELSE 2 END AS result)"; + ELSE 1 END AS result)"; const std::string PostgresCommandExecutor::grantPermissionBase = R"( PREPARE %s (text, text, bit, bit) AS @@ -457,7 +492,7 @@ namespace iroha { ) SELECT CASE WHEN EXISTS (SELECT * FROM inserted) THEN 0 %s - ELSE 2 END AS result)"; + ELSE 1 END AS result)"; const std::string PostgresCommandExecutor::removeSignatoryBase = R"( PREPARE %s (text, text, text) AS @@ -484,7 +519,7 @@ namespace iroha { WHERE public_key = $3) THEN 0 WHEN EXISTS (SELECT 1 FROM peer WHERE public_key = $3) THEN 0 - ELSE 2 + ELSE 1 END %s ELSE 1 @@ -505,7 +540,7 @@ namespace iroha { ) SELECT CASE WHEN EXISTS (SELECT * FROM inserted) THEN 0 %s - ELSE 2 END AS result)"; + ELSE 1 END AS result)"; const std::string PostgresCommandExecutor::setAccountDetailBase = R"( PREPARE %s (text, text, text[], text[], text, text) AS @@ -520,7 +555,7 @@ namespace iroha { ) SELECT CASE WHEN EXISTS (SELECT * FROM inserted) THEN 0 %s - ELSE 2 END AS result)"; + ELSE 1 END AS result)"; const std::string PostgresCommandExecutor::setQuorumBase = R"( PREPARE %s (text, text, int) AS @@ -535,7 +570,7 @@ namespace iroha { ) SELECT CASE WHEN EXISTS (SELECT * FROM updated) THEN 0 %s - ELSE 4 + ELSE 1 END AS result)"; const std::string PostgresCommandExecutor::subtractAssetQuantityBase = R"( @@ -578,7 +613,7 @@ namespace iroha { WHEN NOT EXISTS (SELECT * FROM has_asset LIMIT 1) THEN 3 WHEN NOT EXISTS (SELECT value FROM new_value WHERE value >= 0 LIMIT 1) THEN 4 - ELSE 5 + ELSE 1 END AS result)"; const std::string PostgresCommandExecutor::transferAssetBase = R"( @@ -654,19 +689,19 @@ namespace iroha { SELECT CASE WHEN EXISTS (SELECT * FROM insert_dest LIMIT 1) THEN 0 %s - WHEN NOT EXISTS (SELECT * FROM has_dest_account LIMIT 1) THEN 2 - WHEN NOT EXISTS (SELECT * FROM has_src_account LIMIT 1) THEN 3 + WHEN NOT EXISTS (SELECT * FROM has_dest_account LIMIT 1) THEN 3 + WHEN NOT EXISTS (SELECT * FROM has_src_account LIMIT 1) THEN 2 WHEN NOT EXISTS (SELECT * FROM has_asset LIMIT 1) THEN 4 WHEN NOT EXISTS (SELECT value FROM new_src_value - WHERE value >= 0 LIMIT 1) THEN 5 + WHERE value >= 0 LIMIT 1) THEN 8 WHEN NOT EXISTS (SELECT value FROM new_dest_value WHERE value < 2::decimal ^ (256 - $5) - LIMIT 1) THEN 6 - ELSE 7 + LIMIT 1) THEN 11 + ELSE 1 END AS result)"; std::string CommandError::toString() const { - return (boost::format("%s: %s") % command_name % error_message).str(); + return (boost::format("%s: %d") % command_name % error_code).str(); } PostgresCommandExecutor::PostgresCommandExecutor(soci::session &sql) @@ -696,19 +731,7 @@ namespace iroha { cmd = (cmd % account_id % asset_id % precision % amount); - std::vector> message_gen = { - [&] { - return missRolePerm( - creator_account_id_, - shared_model::interface::permissions::Role::kAddAssetQty); - }, - [] { return std::string("Account does not exist"); }, - [] { - return std::string("Asset with given precision does not exist"); - }, - [] { return std::string("Summation overflows uint256"); }, - }; - return executeQuery(sql_, cmd.str(), "AddAssetQuantity", message_gen); + return executeQuery(sql_, cmd.str(), "AddAssetQuantity"); } CommandResult PostgresCommandExecutor::operator()( @@ -721,20 +744,7 @@ namespace iroha { cmd = (cmd % creator_account_id_ % peer.pubkey().hex() % peer.address()); - std::vector> message_gen = { - [&] { - return missRolePerm( - creator_account_id_, - shared_model::interface::permissions::Role::kAddPeer); - }, - [&] { - return (boost::format("failed to insert peer, public key: '%s', " - "address: '%s'") - % peer.pubkey().hex() % peer.address()) - .str(); - }, - }; - return executeQuery(sql_, cmd.str(), "AddPeer", message_gen); + return executeQuery(sql_, cmd.str(), "AddPeer"); } CommandResult PostgresCommandExecutor::operator()( @@ -747,30 +757,7 @@ namespace iroha { cmd = (cmd % creator_account_id_ % account_id % pubkey); - std::vector> message_gen = { - [&] { - return missRoleOrGrantablePerm( - creator_account_id_, - account_id, - shared_model::interface::permissions::Role::kAddSignatory, - shared_model::interface::permissions::Grantable:: - kAddMySignatory); - }, - [&] { - return (boost::format( - "failed to insert account signatory, account id: " - "'%s', signatory hex string: '%s") - % account_id % pubkey) - .str(); - }, - [&] { - return (boost::format("failed to insert signatory, " - "signatory hex string: '%s'") - % pubkey) - .str(); - }, - }; - return executeQuery(sql_, cmd.str(), "AddSignatory", message_gen); + return executeQuery(sql_, cmd.str(), "AddSignatory"); } CommandResult PostgresCommandExecutor::operator()( @@ -781,35 +768,7 @@ namespace iroha { appendCommandName("appendRole", cmd, do_validation_); - cmd = (cmd % creator_account_id_ % account_id % role_name); - std::vector> message_gen = { - [&] { - return (boost::format("is valid command validation failed: no " - "roles in account %s") - % creator_account_id_) - .str(); - }, - [&] { - return (boost::format( - "is valid command validation failed: account %s" - " does not have some of the permissions in a role %s") - % creator_account_id_ % command.roleName()) - .str(); - }, - [&] { - return missRolePerm( - creator_account_id_, - shared_model::interface::permissions::Role::kAppendRole); - }, - [&] { - return (boost::format( - "failed to insert account role, account: '%s', " - "role name: '%s'") - % account_id % role_name) - .str(); - }, - }; - return executeQuery(sql_, cmd.str(), "AppendRole", message_gen); + return executeQuery(sql_, cmd.str(), "AppendRole"); } CommandResult PostgresCommandExecutor::operator()( @@ -826,38 +785,7 @@ namespace iroha { cmd = (cmd % creator_account_id_ % account_id % domain_id % pubkey); - std::vector> message_gen = { - [&] { - return missRolePerm( - creator_account_id_, - shared_model::interface::permissions::Role::kCreateAccount); - }, - [&] { - return (boost::format("failed to insert account, " - "account id: '%s', " - "domain id: '%s', " - "quorum: '1', " - "json_data: {}") - % account_id % domain_id) - .str(); - }, - [&] { - return (boost::format("failed to insert account signatory, " - "account id: " - "'%s', signatory hex string: '%s") - % account_id % pubkey) - .str(); - }, - [&] { - return (boost::format( - "failed to insert account role, account: '%s' " - "with default domain role name for domain: " - "'%s'") - % account_id % domain_id) - .str(); - }, - }; - return executeQuery(sql_, cmd.str(), "CreateAccount", message_gen); + return executeQuery(sql_, cmd.str(), "CreateAccount"); } CommandResult PostgresCommandExecutor::operator()( @@ -871,19 +799,7 @@ namespace iroha { cmd = (cmd % creator_account_id_ % asset_id % domain_id % precision); - std::vector> message_gen = { - [&] { - return missRolePerm( - creator_account_id_, - shared_model::interface::permissions::Role::kCreateDomain); - }, - [&] { - return (boost::format("failed to insert asset, asset id: '%s', " - "domain id: '%s', precision: %d") - % asset_id % domain_id % precision) - .str(); - }}; - return executeQuery(sql_, cmd.str(), "CreateAsset", message_gen); + return executeQuery(sql_, cmd.str(), "CreateAsset"); } CommandResult PostgresCommandExecutor::operator()( @@ -895,19 +811,8 @@ namespace iroha { appendCommandName("createDomain", cmd, do_validation_); cmd = (cmd % creator_account_id_ % domain_id % default_role); - std::vector> message_gen = { - [&] { - return missRolePerm( - creator_account_id_, - shared_model::interface::permissions::Role::kCreateDomain); - }, - [&] { - return (boost::format("failed to insert domain, domain id: '%s', " - "default role: '%s'") - % domain_id % default_role) - .str(); - }}; - return executeQuery(sql_, cmd.str(), "CreateDomain", message_gen); + + return executeQuery(sql_, cmd.str(), "CreateDomain"); } CommandResult PostgresCommandExecutor::operator()( @@ -920,36 +825,8 @@ namespace iroha { appendCommandName("createRole", cmd, do_validation_); cmd = (cmd % creator_account_id_ % role_id % perm_str); - std::vector> message_gen = { - [&] { - // TODO(@l4l) 26/06/18 need to be simplified at IR-1479 - const auto &str = - shared_model::proto::permissions::toString(permissions); - const auto perm_debug_str = - std::accumulate(str.begin(), str.end(), std::string()); - return (boost::format("failed to insert role permissions, role " - "id: '%s', permissions: [%s]") - % role_id % perm_debug_str) - .str(); - }, - [&] { - return (boost::format( - "is valid command validation failed: account %s" - " does not have some of the permissions from a role %s") - % creator_account_id_ % command.roleName()) - .str(); - }, - [&] { - return missRolePerm( - creator_account_id_, - shared_model::interface::permissions::Role::kCreateRole); - }, - [&] { - return (boost::format("failed to insert role: '%s'") % role_id) - .str(); - }, - }; - return executeQuery(sql_, cmd.str(), "CreateRole", message_gen); + + return executeQuery(sql_, cmd.str(), "CreateRole"); } CommandResult PostgresCommandExecutor::operator()( @@ -961,20 +838,8 @@ namespace iroha { appendCommandName("detachRole", cmd, do_validation_); cmd = (cmd % creator_account_id_ % account_id % role_name); - std::vector> message_gen = { - [&] { - return missRolePerm( - creator_account_id_, - shared_model::interface::permissions::Role::kDetachRole); - }, - [&] { - return (boost::format( - "failed to delete account role, account id: '%s', " - "role name: '%s'") - % account_id % role_name) - .str(); - }}; - return executeQuery(sql_, cmd.str(), "DetachRole", message_gen); + + return executeQuery(sql_, cmd.str(), "DetachRole"); } CommandResult PostgresCommandExecutor::operator()( @@ -994,26 +859,8 @@ namespace iroha { cmd = (cmd % creator_account_id_ % permittee_account_id % perm_str % perm); - std::vector> message_gen = { - [&] { - return missGrantablePerm(creator_account_id_, - permittee_account_id, - command.permissionName()); - }, - [&] { - return (boost::format( - "failed to insert account grantable permission, " - "permittee account id: '%s', " - "account id: '%s', " - "permission: '%s'") - % permittee_account_id - % creator_account_id_ - // TODO(@l4l) 26/06/18 need to be simplified at IR-1479 - % shared_model::proto::permissions::toString(permission)) - .str(); - }}; - - return executeQuery(sql_, cmd.str(), "GrantPermission", message_gen); + + return executeQuery(sql_, cmd.str(), "GrantPermission"); } CommandResult PostgresCommandExecutor::operator()( @@ -1025,48 +872,8 @@ namespace iroha { appendCommandName("removeSignatory", cmd, do_validation_); cmd = (cmd % creator_account_id_ % account_id % pubkey); - std::vector> message_gen = { - [&] { - return (boost::format( - "failed to delete account signatory, account id: " - "'%s', signatory hex string: '%s'") - % account_id % pubkey) - .str(); - }, - [&] { - return (boost::format("failed to delete signatory, " - "signatory hex string: '%s'") - % pubkey) - .str(); - }, - [&] { - return (boost::format( - "command validation failed: no account %s found") - % command.accountId()) - .str(); - }, - [&] { - return (boost::format( - "command validation failed: no signatories in " - "account %s found") - % command.accountId()) - .str(); - }, - [&] { - return "command validation failed: size of rest " - "signatories " - "becomes less than the quorum"; - }, - [&] { - return missRoleOrGrantablePerm( - creator_account_id_, - command.accountId(), - shared_model::interface::permissions::Role::kRemoveSignatory, - shared_model::interface::permissions::Grantable:: - kRemoveMySignatory); - }, - }; - return executeQuery(sql_, cmd.str(), "RemoveSignatory", message_gen); + + return executeQuery(sql_, cmd.str(), "RemoveSignatory"); } CommandResult PostgresCommandExecutor::operator()( @@ -1090,25 +897,7 @@ namespace iroha { cmd = (cmd % creator_account_id_ % permittee_account_id % perms % without_perm_str); - std::vector> message_gen = { - [&] { - return missGrantablePerm(creator_account_id_, - command.accountId(), - command.permissionName()); - }, - [&] { - return (boost::format( - "failed to delete account grantable permission, " - "permittee account id: '%s', " - "account id: '%s', " - "permission id: '%s'") - % permittee_account_id - % account_id - // TODO(@l4l) 26/06/18 need to be simplified at IR-1479 - % shared_model::proto::permissions::toString(permission)) - .str(); - }}; - return executeQuery(sql_, cmd.str(), "RevokePermission", message_gen); + return executeQuery(sql_, cmd.str(), "RevokePermission"); } CommandResult PostgresCommandExecutor::operator()( @@ -1132,23 +921,8 @@ namespace iroha { cmd = (cmd % creator_account_id_ % account_id % json % filled_json % val % empty_json); - std::vector> message_gen = { - [&] { - return missRoleOrGrantablePerm( - creator_account_id_, - command.accountId(), - shared_model::interface::permissions::Role::kSetDetail, - shared_model::interface::permissions::Grantable:: - kSetMyAccountDetail); - }, - [&] { - return (boost::format( - "failed to set account key-value, account id: '%s', " - "creator account id: '%s',\n key: '%s', value: '%s'") - % account_id % creator_account_id_ % key % value) - .str(); - }}; - return executeQuery(sql_, cmd.str(), "SetAccountDetail", message_gen); + + return executeQuery(sql_, cmd.str(), "SetAccountDetail"); } CommandResult PostgresCommandExecutor::operator()( @@ -1160,38 +934,8 @@ namespace iroha { appendCommandName("setQuorum", cmd, do_validation_); cmd = (cmd % creator_account_id_ % account_id % quorum); - std::vector> message_gen = { - [&] { - return (boost::format("is valid command validation failed: no " - "signatories of an " - "account %s found") - % account_id) - .str(); - }, - [&] { - return (boost::format( - "is valid command validation failed: account's %s" - " new quorum size is " - "out of bounds; " - "value is %s") - % account_id % std::to_string(quorum)) - .str(); - }, - [&] { - return missRoleOrGrantablePerm( - creator_account_id_, - account_id, - shared_model::interface::permissions::Role::kSetQuorum, - shared_model::interface::permissions::Grantable::kSetMyQuorum); - }, - [&] { - return (boost::format("failed to update account, account id: '%s', " - "quorum: '%s'") - % account_id % quorum) - .str(); - }, - }; - return executeQuery(sql_, cmd.str(), "SetQuorum", message_gen); + + return executeQuery(sql_, cmd.str(), "SetQuorum"); } CommandResult PostgresCommandExecutor::operator()( @@ -1205,18 +949,7 @@ namespace iroha { cmd = (cmd % creator_account_id_ % asset_id % precision % amount); - std::vector> message_gen = { - [&] { - return missRolePerm( - creator_account_id_, - shared_model::interface::permissions::Role::kSubtractAssetQty); - }, - [&] { return "Account does not exist with given precision"; }, - [&] { return "Asset with given precision does not exist"; }, - [&] { return "Subtracts overdrafts account asset"; }, - }; - return executeQuery( - sql_, cmd.str(), "SubtractAssetQuantity", message_gen); + return executeQuery(sql_, cmd.str(), "SubtractAssetQuantity"); } CommandResult PostgresCommandExecutor::operator()( @@ -1233,32 +966,8 @@ namespace iroha { cmd = (cmd % creator_account_id_ % src_account_id % dest_account_id % asset_id % precision % amount); - std::vector> message_gen = { - [&] { - return (boost::format( - "has permission command validation failed: account %s" - " does not have permission %s" - " for account or does not have %s" - " for his own account or destination account %s" - " does not have %s") - % creator_account_id_ - % shared_model::proto::permissions::toString( - shared_model::interface::permissions::Grantable:: - kTransferMyAssets) - % shared_model::proto::permissions::toString( - shared_model::interface::permissions::Role::kTransfer) - % command.destAccountId() - % shared_model::proto::permissions::toString( - shared_model::interface::permissions::Role::kReceive)) - .str(); - }, - [&] { return "Destination account does not exist"; }, - [&] { return "Source account does not exist"; }, - [&] { return "Asset with given precision does not exist"; }, - [&] { return "Transfer overdrafts source account asset"; }, - [&] { return "Transfer overflows destanation account asset"; }, - }; - return executeQuery(sql_, cmd.str(), "TransferAsset", message_gen); + + return executeQuery(sql_, cmd.str(), "TransferAsset"); } void PostgresCommandExecutor::prepareStatements(soci::session &sql) { @@ -1273,7 +982,7 @@ namespace iroha { "$1")) .str(), "AND (SELECT * from has_perm)", - "WHEN NOT (SELECT * from has_perm) THEN 1"}}); + "WHEN NOT (SELECT * from has_perm) THEN 5"}}); statements.push_back( {"addPeer", @@ -1283,7 +992,7 @@ namespace iroha { shared_model::interface::permissions::Role::kAddPeer, "$1")) .str(), "WHERE (SELECT * FROM has_perm)", - "WHEN NOT (SELECT * from has_perm) THEN 1"}}); + "WHEN NOT (SELECT * from has_perm) THEN 5"}}); statements.push_back( {"addSignatory", @@ -1299,7 +1008,7 @@ namespace iroha { .str(), " WHERE (SELECT * FROM has_perm)", " AND (SELECT * FROM has_perm)", - "WHEN NOT (SELECT * from has_perm) THEN 1"}}); + "WHEN NOT (SELECT * from has_perm) THEN 5"}}); const auto bits = shared_model::interface::RolePermissionSet::size(); const auto grantable_bits = @@ -1335,9 +1044,9 @@ namespace iroha { (SELECT * FROM account_has_role_permissions) AND (SELECT * FROM has_perm))", R"( - WHEN NOT EXISTS (SELECT * FROM account_roles) THEN 1 - WHEN NOT (SELECT * FROM account_has_role_permissions) THEN 2 - WHEN NOT (SELECT * FROM has_perm) THEN 3)"}}); + WHEN NOT EXISTS (SELECT * FROM account_roles) THEN 5 + WHEN NOT (SELECT * FROM account_has_role_permissions) THEN 5 + WHEN NOT (SELECT * FROM has_perm) THEN 5)"}}); statements.push_back( {"createAccount", @@ -1349,7 +1058,7 @@ namespace iroha { "$1")) .str(), R"(AND (SELECT * FROM has_perm))", - R"(WHEN NOT (SELECT * FROM has_perm) THEN 1)"}}); + R"(WHEN NOT (SELECT * FROM has_perm) THEN 5)"}}); statements.push_back( {"createAsset", @@ -1361,7 +1070,7 @@ namespace iroha { "$1")) .str(), R"(WHERE (SELECT * FROM has_perm))", - R"(WHEN NOT (SELECT * FROM has_perm) THEN 1)"}}); + R"(WHEN NOT (SELECT * FROM has_perm) THEN 5)"}}); statements.push_back( {"createDomain", @@ -1373,7 +1082,7 @@ namespace iroha { "$1")) .str(), R"(WHERE (SELECT * FROM has_perm))", - R"(WHEN NOT (SELECT * FROM has_perm) THEN 1)"}}); + R"(WHEN NOT (SELECT * FROM has_perm) THEN 5)"}}); statements.push_back( {"createRole", @@ -1394,8 +1103,8 @@ namespace iroha { R"(WHERE (SELECT * FROM account_has_role_permissions) AND (SELECT * FROM has_perm))", R"(WHEN NOT (SELECT * FROM - account_has_role_permissions) THEN 2 - WHEN NOT (SELECT * FROM has_perm) THEN 3)"}}); + account_has_role_permissions) THEN 5 + WHEN NOT (SELECT * FROM has_perm) THEN 5)"}}); statements.push_back( {"detachRole", @@ -1407,7 +1116,7 @@ namespace iroha { "$1")) .str(), R"(AND (SELECT * FROM has_perm))", - R"(WHEN NOT (SELECT * FROM has_perm) THEN 1)"}}); + R"(WHEN NOT (SELECT * FROM has_perm) THEN 5)"}}); statements.push_back({"grantPermission", grantPermissionBase, @@ -1419,7 +1128,7 @@ namespace iroha { % bits) .str(), R"( WHERE (SELECT * FROM has_perm))", - R"(WHEN NOT (SELECT * FROM has_perm) THEN 1)"}}); + R"(WHEN NOT (SELECT * FROM has_perm) THEN 5)"}}); statements.push_back( {"removeSignatory", @@ -1433,6 +1142,10 @@ namespace iroha { SELECT public_key FROM account_has_signatory WHERE account_id = $2 ), + get_signatory AS ( + SELECT * FROM get_signatories + WHERE public_key = $3 + ), check_account_signatories AS ( SELECT quorum FROM get_account WHERE quorum < (SELECT COUNT(*) FROM get_signatories) @@ -1452,10 +1165,11 @@ namespace iroha { AND EXISTS (SELECT * FROM check_account_signatories) )", R"( - WHEN NOT EXISTS (SELECT * FROM get_account) THEN 3 - WHEN NOT (SELECT * FROM has_perm) THEN 6 + WHEN NOT EXISTS (SELECT * FROM get_account) THEN 2 + WHEN NOT (SELECT * FROM has_perm) THEN 5 WHEN NOT EXISTS (SELECT * FROM get_signatories) THEN 4 - WHEN NOT EXISTS (SELECT * FROM check_account_signatories) THEN 5 + WHEN NOT EXISTS (SELECT * FROM get_signatory) THEN 3 + WHEN NOT EXISTS (SELECT * FROM check_account_signatories) THEN 8 )"}}); statements.push_back({"revokePermission", @@ -1468,7 +1182,7 @@ namespace iroha { % grantable_bits) .str(), R"( AND (SELECT * FROM has_perm))", - R"( WHEN NOT (SELECT * FROM has_perm) THEN 1 )"}}); + R"( WHEN NOT (SELECT * FROM has_perm) THEN 5 )"}}); statements.push_back( {"setAccountDetail", @@ -1492,7 +1206,7 @@ namespace iroha { "$2")) .str(), R"( AND (SELECT * FROM has_perm))", - R"( WHEN NOT (SELECT * FROM has_perm) THEN 1 )"}}); + R"( WHEN NOT (SELECT * FROM has_perm) THEN 5 )"}}); statements.push_back( {"setQuorum", @@ -1520,9 +1234,9 @@ namespace iroha { AND EXISTS (SELECT * FROM check_account_signatories) AND (SELECT * FROM has_perm))", R"( - WHEN NOT (SELECT * FROM has_perm) THEN 3 - WHEN NOT EXISTS (SELECT * FROM get_signatories) THEN 1 - WHEN NOT EXISTS (SELECT * FROM check_account_signatories) THEN 2 + WHEN NOT (SELECT * FROM has_perm) THEN 5 + WHEN NOT EXISTS (SELECT * FROM get_signatories) THEN 3 + WHEN NOT EXISTS (SELECT * FROM check_account_signatories) THEN 4 )"}}); statements.push_back( @@ -1535,7 +1249,7 @@ namespace iroha { "$1")) .str(), R"( AND (SELECT * FROM has_perm))", - R"( WHEN NOT (SELECT * FROM has_perm) THEN 1 )"}}); + R"( WHEN NOT (SELECT * FROM has_perm) THEN 5 )"}}); statements.push_back( {"transferAsset", @@ -1570,7 +1284,7 @@ namespace iroha { .str(), R"( AND (SELECT * FROM has_perm))", R"( AND (SELECT * FROM has_perm))", - R"( WHEN NOT (SELECT * FROM has_perm) THEN 1 )"}}); + R"( WHEN NOT (SELECT * FROM has_perm) THEN 5 )"}}); for (const auto &st : statements) { prepareStatement(sql, st); diff --git a/irohad/ametsuchi/impl/postgres_command_executor.hpp b/irohad/ametsuchi/impl/postgres_command_executor.hpp index 7b369f7a50..e22007c029 100644 --- a/irohad/ametsuchi/impl/postgres_command_executor.hpp +++ b/irohad/ametsuchi/impl/postgres_command_executor.hpp @@ -71,8 +71,7 @@ namespace iroha { CommandResult operator()( const shared_model::interface::TransferAsset &command) override; - static void - prepareStatements(soci::session &sql); + static void prepareStatements(soci::session &sql); private: soci::session &sql_; diff --git a/test/module/irohad/ametsuchi/postgres_executor_test.cpp b/test/module/irohad/ametsuchi/postgres_executor_test.cpp index 310a4871a2..7d68cee727 100644 --- a/test/module/irohad/ametsuchi/postgres_executor_test.cpp +++ b/test/module/irohad/ametsuchi/postgres_executor_test.cpp @@ -357,6 +357,16 @@ namespace iroha { != signatories->end()); } + TEST_F(AddSignatory, InvalidAddSignatoryTestWheNoAccount) { + auto perm = + shared_model::interface::permissions::Grantable::kAddMySignatory; + auto foo = *err(execute(buildCommand(TestTransactionBuilder().grantPermission( + account->accountId(), perm)), + true, + "id2@domain")); + ASSERT_TRUE(true); + } + /** * @given command * @when trying to add signatory without permissions From 62277c078d0f00e7a55db64bbd79819f33caf7be Mon Sep 17 00:00:00 2001 From: Akvinikym Date: Wed, 24 Oct 2018 12:55:02 +0300 Subject: [PATCH 02/17] Builds Signed-off-by: Akvinikym --- irohad/ametsuchi/command_executor.hpp | 1 + .../impl/postgres_command_executor.cpp | 49 ++++--------------- 2 files changed, 11 insertions(+), 39 deletions(-) diff --git a/irohad/ametsuchi/command_executor.hpp b/irohad/ametsuchi/command_executor.hpp index 17d5ea0f0e..9c6b1d9b5d 100644 --- a/irohad/ametsuchi/command_executor.hpp +++ b/irohad/ametsuchi/command_executor.hpp @@ -42,6 +42,7 @@ namespace iroha { std::string command_name; ErrorCodeType error_code; + std::string error_extra; std::string toString() const; }; diff --git a/irohad/ametsuchi/impl/postgres_command_executor.cpp b/irohad/ametsuchi/impl/postgres_command_executor.cpp index bfd9f86c00..aa43ba9112 100644 --- a/irohad/ametsuchi/impl/postgres_command_executor.cpp +++ b/irohad/ametsuchi/impl/postgres_command_executor.cpp @@ -77,9 +77,10 @@ namespace { iroha::expected::Error makeCommandError( const std::string &command_name, - const iroha::ametsuchi::CommandError::ErrorCodeType code) noexcept { + const iroha::ametsuchi::CommandError::ErrorCodeType code, + const std::string &error_extra = "") noexcept { return iroha::expected::makeError( - iroha::ametsuchi::CommandError{command_name, code}); + iroha::ametsuchi::CommandError{command_name, code, error_extra}); } /** @@ -136,7 +137,8 @@ namespace { return makeCommandError(command_name, 11); } - return makeCommandError(command_name, 1); + // parsing is not successful, return the general error + return makeCommandError(command_name, 1, e.what()); } } @@ -198,41 +200,6 @@ namespace { .str(); } - std::string missRolePerm( - shared_model::interface::types::AccountIdType account, - shared_model::interface::permissions::Role perm) { - return (boost::format("command validation failed: account %s" - " does not have permission %s (role)") - % account % shared_model::proto::permissions::toString(perm)) - .str(); - } - - std::string missGrantablePerm( - shared_model::interface::types::AccountIdType account, - shared_model::interface::types::AccountIdType permittee, - shared_model::interface::permissions::Grantable perm) { - return (boost::format( - "command validation failed: account %s" - " does not have permission %s (grantable) for account %s") - % account % shared_model::proto::permissions::toString(perm) - % permittee) - .str(); - } - - std::string missRoleOrGrantablePerm( - shared_model::interface::types::AccountIdType account, - shared_model::interface::types::AccountIdType permittee, - shared_model::interface::permissions::Role role_perm, - shared_model::interface::permissions::Grantable grantable_perm) { - return (boost::format("command validation failed: account %s" - " does not have permission %s (role)" - " and permission %s (grantable) for account %s") - % account % shared_model::proto::permissions::toString(role_perm) - % shared_model::proto::permissions::toString(grantable_perm) - % permittee) - .str(); - } - template void appendCommandName(const std::string &name, Format &cmd, @@ -701,7 +668,9 @@ namespace iroha { END AS result)"; std::string CommandError::toString() const { - return (boost::format("%s: %d") % command_name % error_code).str(); + return (boost::format("%s: %d with extra info '%s'") % command_name + % error_code % error_extra) + .str(); } PostgresCommandExecutor::PostgresCommandExecutor(soci::session &sql) @@ -766,6 +735,8 @@ namespace iroha { auto &role_name = command.roleName(); auto cmd = boost::format("EXECUTE %1% ('%2%', '%3%', '%4%')"); + cmd = (cmd % creator_account_id_ % account_id % role_name); + appendCommandName("appendRole", cmd, do_validation_); return executeQuery(sql_, cmd.str(), "AppendRole"); From 9cff49eb91aa397705392c4d1ad19d14d92f805e Mon Sep 17 00:00:00 2001 From: Akvinikym Date: Thu, 25 Oct 2018 11:37:03 +0300 Subject: [PATCH 03/17] 4/16 commands Signed-off-by: Akvinikym --- .../impl/postgres_command_executor.cpp | 24 +- .../ametsuchi/postgres_executor_test.cpp | 225 ++++++++++++------ 2 files changed, 161 insertions(+), 88 deletions(-) diff --git a/irohad/ametsuchi/impl/postgres_command_executor.cpp b/irohad/ametsuchi/impl/postgres_command_executor.cpp index aa43ba9112..73fc0071ce 100644 --- a/irohad/ametsuchi/impl/postgres_command_executor.cpp +++ b/irohad/ametsuchi/impl/postgres_command_executor.cpp @@ -107,36 +107,41 @@ namespace { // try to parse the SQL error to get an actual one std::string exc_body = e.what(); - if (exc_body.find("DETAIL: Key (account_id)=") != std::string::npos + if (exc_body.find("Key (account_id)=") != std::string::npos and exc_body.find("is not present in table") != std::string::npos) { return makeCommandError(command_name, 2); } - if (exc_body.find("DETAIL: Key (role_id)=") != std::string::npos + if (exc_body.find("Key (role_id)=") != std::string::npos and exc_body.find("is not present in table") != std::string::npos) { return makeCommandError(command_name, 6); } - if (exc_body.find("DETAIL: Key (domain_id)=") != std::string::npos + if (exc_body.find("Key (domain_id)=") != std::string::npos and exc_body.find("is not present in table") != std::string::npos) { return makeCommandError(command_name, 7); } - if (exc_body.find("DETAIL: Key (asset_id)=") != std::string::npos + if (exc_body.find("Key (asset_id)=") != std::string::npos and exc_body.find("already exists") != std::string::npos) { return makeCommandError(command_name, 9); } - if (exc_body.find("DETAIL: Key (domain_id)=") != std::string::npos + if (exc_body.find("Key (domain_id)=") != std::string::npos and exc_body.find("already exists") != std::string::npos) { return makeCommandError(command_name, 10); } - if (exc_body.find("DETAIL: Key (role_id)=") != std::string::npos + if (exc_body.find("Key (role_id)=") != std::string::npos and exc_body.find("already exists") != std::string::npos) { return makeCommandError(command_name, 11); } + if (exc_body.find("Key (account_id, public_key)") != std::string::npos + and exc_body.find("already exists") != std::string::npos) { + return makeCommandError(command_name, 12); + } + // parsing is not successful, return the general error return makeCommandError(command_name, 1, e.what()); } @@ -297,13 +302,13 @@ namespace iroha { SELECT CASE WHEN EXISTS (SELECT * FROM insert_account_signatory) THEN 0 %s - WHEN EXISTS (SELECT * FROM insert_signatory) THEN 3 ELSE 1 END AS RESULT;)"; const std::string PostgresCommandExecutor::appendRoleBase = R"( PREPARE %s (text, text, text) AS WITH %s + role_exists AS (SELECT * FROM role WHERE role_id = $3), inserted AS ( INSERT INTO account_has_roles(account_id, role_id) ( @@ -311,6 +316,7 @@ namespace iroha { ) SELECT CASE WHEN EXISTS (SELECT * FROM inserted) THEN 0 + WHEN NOT EXISTS (SELECT * FROM role_exists) THEN 6 %s ELSE 1 END AS result)"; @@ -735,10 +741,10 @@ namespace iroha { auto &role_name = command.roleName(); auto cmd = boost::format("EXECUTE %1% ('%2%', '%3%', '%4%')"); - cmd = (cmd % creator_account_id_ % account_id % role_name); - appendCommandName("appendRole", cmd, do_validation_); + cmd = (cmd % creator_account_id_ % account_id % role_name); + return executeQuery(sql_, cmd.str(), "AppendRole"); } diff --git a/test/module/irohad/ametsuchi/postgres_executor_test.cpp b/test/module/irohad/ametsuchi/postgres_executor_test.cpp index 7d68cee727..dcd2eaafc5 100644 --- a/test/module/irohad/ametsuchi/postgres_executor_test.cpp +++ b/test/module/irohad/ametsuchi/postgres_executor_test.cpp @@ -98,6 +98,18 @@ namespace iroha { true))); } + /** + * Check that command result contains specific error code + * @param result to be checked + * @param expected_code to be in the result + */ + void checkErrorCode(const CommandResult &result, + CommandError::ErrorCodeType expected_code) { + auto error = err(result); + ASSERT_TRUE(error); + ASSERT_EQ(error->error.error_code, expected_code); + } + const std::string role = "role"; const std::string another_role = "role2"; shared_model::interface::RolePermissionSet role_permissions; @@ -177,25 +189,19 @@ namespace iroha { } /** - * @given command - * @when trying to add account asset without permission - * @then account asset not added + * @given command + * @when trying to add account asset with non-existing account + * @then account asset fails to added */ - TEST_F(AddAccountAssetTest, InvalidAddAccountAssetTestNoPerms) { + TEST_F(AddAccountAssetTest, AddAccountAssetTestInvalidAccount) { addAsset(); - ASSERT_TRUE( - val(execute(buildCommand(TestTransactionBuilder() - .addAssetQuantity(asset_id, "1.0") - .creatorAccountId(account->accountId())), - true))); - auto account_asset = - query->getAccountAsset(account->accountId(), asset_id); - ASSERT_TRUE(account_asset); - ASSERT_EQ("1.0", account_asset.get()->balance().toStringRepr()); - ASSERT_TRUE(err( + auto cmd_result = execute(buildCommand(TestTransactionBuilder() .addAssetQuantity(asset_id, "1.0") - .creatorAccountId(account->accountId()))))); + .creatorAccountId("some@domain")), + true, + "some@domain"); + checkErrorCode(cmd_result, 2); } /** @@ -204,26 +210,12 @@ namespace iroha { * @then account asset fails to be added */ TEST_F(AddAccountAssetTest, AddAccountAssetTestInvalidAsset) { - ASSERT_TRUE( - err(execute(buildCommand(TestTransactionBuilder() - .addAssetQuantity(asset_id, "1.0") - .creatorAccountId(account->accountId())), - true))); - } - - /** - * @given command - * @when trying to add account asset with non-existing account - * @then account asset fails to added - */ - TEST_F(AddAccountAssetTest, AddAccountAssetTestInvalidAccount) { - addAsset(); - ASSERT_TRUE( - err(execute(buildCommand(TestTransactionBuilder() - .addAssetQuantity(asset_id, "1.0") - .creatorAccountId("some@domain")), - true, - "some@domain"))); + auto cmd_result = + execute(buildCommand(TestTransactionBuilder() + .addAssetQuantity(asset_id, "1.0") + .creatorAccountId(account->accountId())), + true); + checkErrorCode(cmd_result, 3); } /** @@ -242,11 +234,36 @@ namespace iroha { .addAssetQuantity(asset_id, uint256_halfmax) .creatorAccountId(account->accountId())), true))); - ASSERT_TRUE(err( + auto cmd_result = execute(buildCommand(TestTransactionBuilder() .addAssetQuantity(asset_id, uint256_halfmax) .creatorAccountId(account->accountId())), - true))); + true); + checkErrorCode(cmd_result, 4); + } + + /** + * @given command + * @when trying to add account asset without permission + * @then account asset not added + */ + TEST_F(AddAccountAssetTest, AddAccountAssetTestNoPerms) { + addAsset(); + ASSERT_TRUE( + val(execute(buildCommand(TestTransactionBuilder() + .addAssetQuantity(asset_id, "1.0") + .creatorAccountId(account->accountId())), + true))); + auto account_asset = + query->getAccountAsset(account->accountId(), asset_id); + ASSERT_TRUE(account_asset); + ASSERT_EQ("1.0", account_asset.get()->balance().toStringRepr()); + + auto cmd_result = + execute(buildCommand(TestTransactionBuilder() + .addAssetQuantity(asset_id, "1.0") + .creatorAccountId(account->accountId()))); + checkErrorCode(cmd_result, 5); } class AddPeer : public CommandExecutorTest { @@ -287,8 +304,9 @@ namespace iroha { * @then peer is not added */ TEST_F(AddPeer, InvalidAddPeerTestWhenNoPerms) { - ASSERT_TRUE(err(execute(buildCommand( - TestTransactionBuilder().addPeer(peer->address(), peer->pubkey()))))); + auto cmd_result = execute(buildCommand( + TestTransactionBuilder().addPeer(peer->address(), peer->pubkey()))); + checkErrorCode(cmd_result, 5); } class AddSignatory : public CommandExecutorTest { @@ -357,14 +375,20 @@ namespace iroha { != signatories->end()); } - TEST_F(AddSignatory, InvalidAddSignatoryTestWheNoAccount) { + /** + * @given command + * @when trying to add signatory to non-existing account + * @then signatory is not added + */ + TEST_F(AddSignatory, InvalidAddSignatoryTestNoAccount) { auto perm = shared_model::interface::permissions::Grantable::kAddMySignatory; - auto foo = *err(execute(buildCommand(TestTransactionBuilder().grantPermission( - account->accountId(), perm)), - true, - "id2@domain")); - ASSERT_TRUE(true); + auto cmd_result = + execute(buildCommand(TestTransactionBuilder().grantPermission( + account->accountId(), perm)), + true, + "id2@domain"); + checkErrorCode(cmd_result, 2); } /** @@ -373,15 +397,35 @@ namespace iroha { * @then signatory is not added */ TEST_F(AddSignatory, InvalidAddSignatoryTestWhenNoPerms) { - ASSERT_TRUE( - err(execute(buildCommand(TestTransactionBuilder().addSignatory( - account->accountId(), *pubkey))))); + auto cmd_result = + execute(buildCommand(TestTransactionBuilder().addSignatory( + account->accountId(), *pubkey))); + checkErrorCode(cmd_result, 5); + auto signatories = query->getSignatories(account->accountId()); ASSERT_TRUE(signatories); ASSERT_TRUE(std::find(signatories->begin(), signatories->end(), *pubkey) == signatories->end()); } + /** + * @given command + * @when successfully adding signatory to the account @and trying to add the + * same signatory again + * @then signatory is not added + */ + TEST_F(AddSignatory, InvalidAddSignatoryExistingPubKey) { + addAllPerms(); + ASSERT_TRUE( + val(execute(buildCommand(TestTransactionBuilder().addSignatory( + account->accountId(), *pubkey))))); + + auto cmd_result = + execute(buildCommand(TestTransactionBuilder().addSignatory( + account->accountId(), *pubkey))); + checkErrorCode(cmd_result, 12); + } + class AppendRole : public CommandExecutorTest { public: void SetUp() override { @@ -402,19 +446,30 @@ namespace iroha { shared_model::interface::RolePermissionSet role_permissions2; }; - /** - * @given command - * @when trying to append role with perms that creator does not have - * @then role is not appended - */ - TEST_F(AppendRole, AppendRoleTestInvalidWhenAccountDoesNotHavePerms) { - role_permissions2.set( - shared_model::interface::permissions::Role::kRemoveMySignatory); + TEST_F(AppendRole, ValidAppendRoleTest) { + addAllPerms(); ASSERT_TRUE(val(execute(buildCommand(TestTransactionBuilder().createRole( - another_role, role_permissions2)), + another_role, role_permissions)), true))); - ASSERT_TRUE(err(execute(buildCommand(TestTransactionBuilder().appendRole( + ASSERT_TRUE(val(execute(buildCommand(TestTransactionBuilder().appendRole( + account->accountId(), another_role))))); + auto roles = query->getAccountRoles(account->accountId()); + ASSERT_TRUE(roles); + ASSERT_TRUE(std::find(roles->begin(), roles->end(), another_role) + != roles->end()); + } + + TEST_F(AppendRole, ValidAppendRoleTestWhenEmptyPerms) { + addAllPerms(); + ASSERT_TRUE(val(execute( + buildCommand(TestTransactionBuilder().createRole(another_role, {})), + true))); + ASSERT_TRUE(val(execute(buildCommand(TestTransactionBuilder().appendRole( account->accountId(), another_role))))); + auto roles = query->getAccountRoles(account->accountId()); + ASSERT_TRUE(roles); + ASSERT_TRUE(std::find(roles->begin(), roles->end(), another_role) + != roles->end()); } /** @@ -438,42 +493,54 @@ namespace iroha { != roles->end()); } + TEST_F(AppendRole, AppendRoleTestInvalidNoAccount) { + addAllPerms(); + ASSERT_TRUE(val(execute( + buildCommand(TestTransactionBuilder().createRole(another_role, {})), + true))); + auto cmd_result = execute(buildCommand( + TestTransactionBuilder().appendRole("doge@noaccount", another_role))); + checkErrorCode(cmd_result, 2); + } + TEST_F(AppendRole, InvalidAppendRoleTestWhenNoPerms) { ASSERT_TRUE(val(execute(buildCommand(TestTransactionBuilder().createRole( another_role, role_permissions)), true))); - ASSERT_TRUE(err(execute(buildCommand(TestTransactionBuilder().appendRole( - account->accountId(), another_role))))); + auto cmd_result = + execute(buildCommand(TestTransactionBuilder().appendRole( + account->accountId(), another_role))); + checkErrorCode(cmd_result, 5); + auto roles = query->getAccountRoles(account->accountId()); ASSERT_TRUE(roles); ASSERT_TRUE(std::find(roles->begin(), roles->end(), another_role) == roles->end()); } - TEST_F(AppendRole, ValidAppendRoleTest) { - addAllPerms(); + /** + * @given command + * @when trying to append role with perms that creator does not have + * @then role is not appended + */ + TEST_F(AppendRole, AppendRoleTestInvalidWhenAccountDoesNotHavePerms) { + role_permissions2.set( + shared_model::interface::permissions::Role::kRemoveMySignatory); ASSERT_TRUE(val(execute(buildCommand(TestTransactionBuilder().createRole( - another_role, role_permissions)), + another_role, role_permissions2)), true))); - ASSERT_TRUE(val(execute(buildCommand(TestTransactionBuilder().appendRole( - account->accountId(), another_role))))); - auto roles = query->getAccountRoles(account->accountId()); - ASSERT_TRUE(roles); - ASSERT_TRUE(std::find(roles->begin(), roles->end(), another_role) - != roles->end()); + auto cmd_result = + execute(buildCommand(TestTransactionBuilder().appendRole( + account->accountId(), another_role))); + checkErrorCode(cmd_result, 5); } - TEST_F(AppendRole, ValidAppendRoleTestWhenEmptyPerms) { + TEST_F(AppendRole, AppendRoleTestInvalidNoSuchRole) { addAllPerms(); - ASSERT_TRUE(val(execute( - buildCommand(TestTransactionBuilder().createRole(another_role, {})), - true))); - ASSERT_TRUE(val(execute(buildCommand(TestTransactionBuilder().appendRole( - account->accountId(), another_role))))); - auto roles = query->getAccountRoles(account->accountId()); - ASSERT_TRUE(roles); - ASSERT_TRUE(std::find(roles->begin(), roles->end(), another_role) - != roles->end()); + auto cmd_result = + execute(buildCommand(TestTransactionBuilder().appendRole( + account->accountId(), another_role))); + checkErrorCode(cmd_result, 6); } class CreateAccount : public CommandExecutorTest { From 0374fae466264e0fff25788a71c69e40cc4b623d Mon Sep 17 00:00:00 2001 From: Akvinikym Date: Fri, 26 Oct 2018 12:01:54 +0300 Subject: [PATCH 04/17] 13/16 done Signed-off-by: Akvinikym --- .../impl/postgres_command_executor.cpp | 33 +- .../ametsuchi/postgres_executor_test.cpp | 349 ++++++++++++++---- 2 files changed, 288 insertions(+), 94 deletions(-) diff --git a/irohad/ametsuchi/impl/postgres_command_executor.cpp b/irohad/ametsuchi/impl/postgres_command_executor.cpp index 73fc0071ce..735ff7e7f7 100644 --- a/irohad/ametsuchi/impl/postgres_command_executor.cpp +++ b/irohad/ametsuchi/impl/postgres_command_executor.cpp @@ -107,7 +107,8 @@ namespace { // try to parse the SQL error to get an actual one std::string exc_body = e.what(); - if (exc_body.find("Key (account_id)=") != std::string::npos + if ((exc_body.find("Key (account_id)=") != std::string::npos + or exc_body.find("Key (permittee_account_id)") != std::string::npos) and exc_body.find("is not present in table") != std::string::npos) { return makeCommandError(command_name, 2); } @@ -142,6 +143,16 @@ namespace { return makeCommandError(command_name, 12); } + if (exc_body.find("Key (account_id)") != std::string::npos + and exc_body.find("already exists") != std::string::npos) { + return makeCommandError(command_name, 13); + } + + if (exc_body.find("Key (default_role)") != std::string::npos + and exc_body.find("is not present in table") != std::string::npos) { + return makeCommandError(command_name, 14); + } + // parsing is not successful, return the general error return makeCommandError(command_name, 1, e.what()); } @@ -323,8 +334,9 @@ namespace iroha { const std::string PostgresCommandExecutor::createAccountBase = R"( PREPARE %s (text, text, text, text) AS - WITH get_domain_default_role AS (SELECT default_role FROM domain - WHERE domain_id = $3), + WITH has_domain AS (SELECT * FROM domain WHERE domain_id = $3), + get_domain_default_role AS (SELECT default_role FROM domain + WHERE domain_id = $3), %s insert_signatory AS ( @@ -367,15 +379,7 @@ namespace iroha { SELECT CASE WHEN EXISTS (SELECT * FROM insert_account_role) THEN 0 %s - WHEN NOT EXISTS (SELECT * FROM account - WHERE account_id = $2) THEN 3 - WHEN NOT EXISTS (SELECT * FROM account_has_signatory - WHERE account_id = $2 - AND public_key = $4) THEN 4 - WHEN NOT EXISTS (SELECT * FROM account_has_roles - WHERE account_id = account_id AND role_id = ( - SELECT default_role FROM get_domain_default_role) - ) THEN 8 + WHEN NOT EXISTS (SELECT * FROM has_domain) THEN 7 ELSE 1 END AS result)"; @@ -407,7 +411,7 @@ namespace iroha { ) SELECT CASE WHEN EXISTS (SELECT * FROM inserted) THEN 0 %s - ELSE 2 END AS result)"; + ELSE 1 END AS result)"; const std::string PostgresCommandExecutor::createRoleBase = R"( PREPARE %s (text, text, bit) AS @@ -528,6 +532,8 @@ namespace iroha { ) SELECT CASE WHEN EXISTS (SELECT * FROM inserted) THEN 0 %s + WHEN NOT EXISTS + (SELECT * FROM account WHERE account_id=$2) THEN 2 ELSE 1 END AS result)"; const std::string PostgresCommandExecutor::setQuorumBase = R"( @@ -1144,7 +1150,6 @@ namespace iroha { R"( WHEN NOT EXISTS (SELECT * FROM get_account) THEN 2 WHEN NOT (SELECT * FROM has_perm) THEN 5 - WHEN NOT EXISTS (SELECT * FROM get_signatories) THEN 4 WHEN NOT EXISTS (SELECT * FROM get_signatory) THEN 3 WHEN NOT EXISTS (SELECT * FROM check_account_signatories) THEN 8 )"}}); diff --git a/test/module/irohad/ametsuchi/postgres_executor_test.cpp b/test/module/irohad/ametsuchi/postgres_executor_test.cpp index dcd2eaafc5..45528d350d 100644 --- a/test/module/irohad/ametsuchi/postgres_executor_test.cpp +++ b/test/module/irohad/ametsuchi/postgres_executor_test.cpp @@ -576,17 +576,6 @@ namespace iroha { std::unique_ptr account2; }; - /** - * @given command and no target domain in ledger - * @when trying to create account - * @then account is not created - */ - TEST_F(CreateAccount, InvalidCreateAccountNoDomainTest) { - addAllPerms(); - ASSERT_TRUE(err(execute(buildCommand( - TestTransactionBuilder().createAccount("id2", "domain2", *pubkey))))); - } - /** * @given command * @when trying to create account @@ -608,13 +597,34 @@ namespace iroha { * @then account is created */ TEST_F(CreateAccount, InvalidCreateAccountWithoutPermsTest) { - ASSERT_TRUE( - err(execute(buildCommand(TestTransactionBuilder().createAccount( - "id2", domain->domainId(), *pubkey))))); + auto cmd_result = + execute(buildCommand(TestTransactionBuilder().createAccount( + "id2", domain->domainId(), *pubkey))); + checkErrorCode(cmd_result, 5); auto acc = query->getAccount(account2->accountId()); ASSERT_FALSE(acc); } + /** + * @given command and no target domain in ledger + * @when trying to create account + * @then account is not created + */ + TEST_F(CreateAccount, InvalidCreateAccountNoDomainTest) { + addAllPerms(); + auto cmd_result = execute(buildCommand( + TestTransactionBuilder().createAccount("doge", "domain6", *pubkey))); + checkErrorCode(cmd_result, 7); + } + + TEST_F(CreateAccount, InvalidCreateAccountNameExists) { + addAllPerms(); + auto cmd_result = + execute(buildCommand(TestTransactionBuilder().createAccount( + "id", domain->domainId(), *pubkey))); + checkErrorCode(cmd_result, 13); + } + class CreateAsset : public CommandExecutorTest { public: void SetUp() override { @@ -625,16 +635,6 @@ namespace iroha { "coin#" + domain->domainId(); }; - /** - * @given command and no target domain in ledger - * @when trying to create asset - * @then asset is not created - */ - TEST_F(CreateAsset, InvalidCreateAssetNoDomainTest) { - ASSERT_TRUE(err(execute(buildCommand(TestTransactionBuilder().createAsset( - asset_name, domain->domainId(), 1))))); - } - /** * @given command * @when trying to create asset @@ -671,7 +671,7 @@ namespace iroha { * @when trying to create asset without permission * @then asset is not created */ - TEST_F(CreateAsset, InvalidCreateAssetWithDomainTest) { + TEST_F(CreateAsset, InvalidCreateAssetWithNoPermission) { ASSERT_TRUE(val(execute(buildCommand(TestTransactionBuilder().createRole( role, role_permissions)), true))); @@ -688,12 +688,68 @@ namespace iroha { val(execute(buildCommand(TestTransactionBuilder().createAccount( "id", domain->domainId(), *pubkey)), true))); - ASSERT_TRUE(err(execute(buildCommand(TestTransactionBuilder().createAsset( - "coin", domain->domainId(), 1))))); + auto cmd_result = execute(buildCommand( + TestTransactionBuilder().createAsset("coin", domain->domainId(), 1))); + checkErrorCode(cmd_result, 5); auto ass = query->getAsset(asset->assetId()); ASSERT_FALSE(ass); } + /** + * @given command and no target domain in ledger + * @when trying to create asset + * @then asset is not created + */ + TEST_F(CreateAsset, InvalidCreateAssetNoDomain) { + role_permissions.set( + shared_model::interface::permissions::Role::kCreateAsset); + ASSERT_TRUE(val(execute(buildCommand(TestTransactionBuilder().createRole( + role, role_permissions)), + true))); + ASSERT_TRUE( + val(execute(buildCommand(TestTransactionBuilder().createDomain( + domain->domainId(), role)), + true))); + auto asset = clone(TestAccountAssetBuilder() + .domainId(domain->domainId()) + .assetId(asset_id) + .precision(1) + .build()); + ASSERT_TRUE( + val(execute(buildCommand(TestTransactionBuilder().createAccount( + "id", domain->domainId(), *pubkey)), + true))); + auto cmd_result = execute(buildCommand( + TestTransactionBuilder().createAsset(asset_name, "no_domain", 1))); + checkErrorCode(cmd_result, 7); + } + + TEST_F(CreateAsset, InvalidCreateAssetNameNotUnique) { + role_permissions.set( + shared_model::interface::permissions::Role::kCreateAsset); + ASSERT_TRUE(val(execute(buildCommand(TestTransactionBuilder().createRole( + role, role_permissions)), + true))); + ASSERT_TRUE( + val(execute(buildCommand(TestTransactionBuilder().createDomain( + domain->domainId(), role)), + true))); + auto asset = clone(TestAccountAssetBuilder() + .domainId(domain->domainId()) + .assetId(asset_id) + .precision(1) + .build()); + ASSERT_TRUE( + val(execute(buildCommand(TestTransactionBuilder().createAccount( + "id", domain->domainId(), *pubkey)), + true))); + ASSERT_TRUE(val(execute(buildCommand(TestTransactionBuilder().createAsset( + "coin", domain->domainId(), 1))))); + auto cmd_result = execute(buildCommand( + TestTransactionBuilder().createAsset("coin", domain->domainId(), 1))); + checkErrorCode(cmd_result, 9); + } + class CreateDomain : public CommandExecutorTest { public: void SetUp() override { @@ -717,18 +773,6 @@ namespace iroha { std::unique_ptr domain2; }; - /** - * @given command when there is no role - * @when trying to create domain - * @then domain is not created - */ - TEST_F(CreateDomain, InvalidCreateDomainWhenNoRoleTest) { - addAllPerms(); - ASSERT_TRUE( - err(execute(buildCommand(TestTransactionBuilder().createDomain( - domain2->domainId(), another_role))))); - } - /** * @given command * @when trying to create domain @@ -749,12 +793,35 @@ namespace iroha { * @then domain is not created */ TEST_F(CreateDomain, InvalidCreateDomainTestWhenNoPerms) { - ASSERT_TRUE(err(execute(buildCommand( - TestTransactionBuilder().createDomain(domain2->domainId(), role))))); + auto cmd_result = execute(buildCommand( + TestTransactionBuilder().createDomain(domain2->domainId(), role))); + checkErrorCode(cmd_result, 5); auto dom = query->getDomain(domain2->domainId()); ASSERT_FALSE(dom); } + TEST_F(CreateDomain, InvalidCreateDomainWhenNameNotUnique) { + addAllPerms(); + ASSERT_TRUE(val(execute(buildCommand( + TestTransactionBuilder().createDomain(domain2->domainId(), role))))); + auto cmd_result = execute(buildCommand( + TestTransactionBuilder().createDomain(domain2->domainId(), role))); + checkErrorCode(cmd_result, 10); + } + + /** + * @given command when there is no role + * @when trying to create domain + * @then domain is not created + */ + TEST_F(CreateDomain, InvalidCreateDomainWhenNoRoleTest) { + addAllPerms(); + auto cmd_result = + execute(buildCommand(TestTransactionBuilder().createDomain( + domain2->domainId(), another_role))); + checkErrorCode(cmd_result, 14); + } + class CreateRole : public CommandExecutorTest { public: void SetUp() override { @@ -797,13 +864,24 @@ namespace iroha { TEST_F(CreateRole, CreateRoleTestInvalidWhenHasNoPerms) { role_permissions2.set( shared_model::interface::permissions::Role::kRemoveMySignatory); - ASSERT_TRUE(err(execute(buildCommand(TestTransactionBuilder().createRole( - another_role, role_permissions2))))); + auto cmd_result = + execute(buildCommand(TestTransactionBuilder().createRole( + another_role, role_permissions2))); + checkErrorCode(cmd_result, 5); auto rl = query->getRolePermissions(another_role); ASSERT_TRUE(rl); ASSERT_TRUE(rl->none()); } + TEST_F(CreateRole, CreateRoleTestInvalidNameNotUnique) { + addAllPerms(); + ASSERT_TRUE(val(execute(buildCommand(TestTransactionBuilder().createRole( + another_role, role_permissions))))); + auto cmd_result = execute(buildCommand( + TestTransactionBuilder().createRole(another_role, role_permissions))); + checkErrorCode(cmd_result, 11); + } + class DetachRole : public CommandExecutorTest { public: void SetUp() override { @@ -846,20 +924,62 @@ namespace iroha { == roles->end()); } + /** + * @given command + * @when trying to detach role from non-existing account + * @then correspondent error code is returned + */ + TEST_F(DetachRole, InvalidDetachRoleTestNoAccount) { + addAllPerms(); + auto cmd_result = execute(buildCommand( + TestTransactionBuilder().detachRole("doge@noaccount", another_role))); + checkErrorCode(cmd_result, 2); + } + + /** + * @given command + * @when trying to detach role, which the account does not have + * @then correspondent error code is returned + */ + TEST_F(DetachRole, InvalidDetachRoleTestNoRoleInAccountRoles) { + addAllPerms(); + ASSERT_TRUE(val(execute(buildCommand(TestTransactionBuilder().detachRole( + account->accountId(), another_role))))); + auto cmd_result = + execute(buildCommand(TestTransactionBuilder().detachRole( + account->accountId(), another_role))); + checkErrorCode(cmd_result, 3); + } + /** * @given command * @when trying to detach role without permission * @then role is detached */ TEST_F(DetachRole, InvalidDetachRoleTestWhenNoPerms) { - ASSERT_TRUE(err(execute(buildCommand(TestTransactionBuilder().detachRole( - account->accountId(), another_role))))); + auto cmd_result = + execute(buildCommand(TestTransactionBuilder().detachRole( + account->accountId(), another_role))); + checkErrorCode(cmd_result, 5); auto roles = query->getAccountRoles(account->accountId()); ASSERT_TRUE(roles); ASSERT_TRUE(std::find(roles->begin(), roles->end(), another_role) != roles->end()); } + /** + * @given command + * @when trying to detach a non-existing role + * @then correspondent error code is returned + */ + TEST_F(DetachRole, InvalidDetachRoleTestNoRole) { + addAllPerms(); + auto cmd_result = + execute(buildCommand(TestTransactionBuilder().detachRole( + account->accountId(), "not_existing_role"))); + checkErrorCode(cmd_result, 6); + } + class GrantPermission : public CommandExecutorTest { public: void SetUp() override { @@ -900,6 +1020,21 @@ namespace iroha { ASSERT_TRUE(has_perm); } + /** + * @given command + * @when trying to grant permission to non-existent account + * @then corresponding error code is returned + */ + TEST_F(GrantPermission, InvalidGrantPermissionTestNoAccount) { + addAllPerms(); + auto perm = shared_model::interface::permissions::Grantable::kSetMyQuorum; + auto cmd_result = + execute(buildCommand(TestTransactionBuilder() + .grantPermission("doge@noaccount", perm) + .creatorAccountId(account->accountId()))); + checkErrorCode(cmd_result, 2); + } + /** * @given command * @when trying to grant permission without permission @@ -907,10 +1042,12 @@ namespace iroha { */ TEST_F(GrantPermission, InvalidGrantPermissionTestWhenNoPerm) { auto perm = shared_model::interface::permissions::Grantable::kSetMyQuorum; - ASSERT_TRUE(err( + auto cmd_result = + execute(buildCommand(TestTransactionBuilder() .grantPermission(account->accountId(), perm) - .creatorAccountId(account->accountId()))))); + .creatorAccountId(account->accountId()))); + checkErrorCode(cmd_result, 5); auto has_perm = query->hasAccountGrantablePermission( account->accountId(), account->accountId(), perm); ASSERT_FALSE(has_perm); @@ -922,6 +1059,9 @@ namespace iroha { CommandExecutorTest::SetUp(); pubkey = std::make_unique( std::string('1', 32)); + another_pubkey = + std::make_unique( + std::string('7', 32)); ASSERT_TRUE( val(execute(buildCommand(TestTransactionBuilder().createRole( role, role_permissions)), @@ -936,6 +1076,8 @@ namespace iroha { true))); } std::unique_ptr pubkey; + std::unique_ptr + another_pubkey; }; /** @@ -996,6 +1138,51 @@ namespace iroha { == signatories->end()); } + /** + * @given command + * @when trying to remove signatory from a non existing account + * @then corresponding error code is returned + */ + TEST_F(RemoveSignatory, RemoveSignatoryTestNonExistingAccount) { + addAllPerms(); + shared_model::interface::types::PubkeyType pk(std::string('5', 32)); + ASSERT_TRUE( + val(execute(buildCommand(TestTransactionBuilder().addSignatory( + account->accountId(), pk)), + true))); + + auto cmd_result = execute(buildCommand( + TestTransactionBuilder().removeSignatory("hello", *pubkey))); + checkErrorCode(cmd_result, 2); + } + + /** + * @given command + * @when trying to remove signatory, which is not attached to this account + * @then corresponding error code is returned + */ + TEST_F(RemoveSignatory, RemoveSignatoryTestNoSuchSignatory) { + addAllPerms(); + shared_model::interface::types::PubkeyType pk(std::string('5', 32)); + ASSERT_TRUE( + val(execute(buildCommand(TestTransactionBuilder().addSignatory( + account->accountId(), pk)), + true))); + ASSERT_TRUE( + val(execute(buildCommand(TestTransactionBuilder().addSignatory( + account->accountId(), *another_pubkey)), + true))); + ASSERT_TRUE( + val(execute(buildCommand(TestTransactionBuilder().removeSignatory( + account->accountId(), *another_pubkey)), + true))); + + auto cmd_result = + execute(buildCommand(TestTransactionBuilder().removeSignatory( + account->accountId(), *another_pubkey))); + checkErrorCode(cmd_result, 3); + } + /** * @given command * @when trying to remove signatory without permission @@ -1007,9 +1194,11 @@ namespace iroha { val(execute(buildCommand(TestTransactionBuilder().addSignatory( account->accountId(), pk)), true))); - ASSERT_TRUE( - err(execute(buildCommand(TestTransactionBuilder().removeSignatory( - account->accountId(), *pubkey))))); + auto cmd_result = + execute(buildCommand(TestTransactionBuilder().removeSignatory( + account->accountId(), *pubkey))); + checkErrorCode(cmd_result, 5); + auto signatories = query->getSignatories(account->accountId()); ASSERT_TRUE(signatories); ASSERT_TRUE(std::find(signatories->begin(), signatories->end(), *pubkey) @@ -1033,26 +1222,9 @@ namespace iroha { ASSERT_TRUE( val(execute(buildCommand(TestTransactionBuilder().removeSignatory( account->accountId(), *pubkey))))); - ASSERT_TRUE( - err(execute(buildCommand(TestTransactionBuilder().removeSignatory( - account->accountId(), pk))))); - } - - /** - * @given command - * @when trying to remove signatory from a non existing account - * @then signatory is not removed - */ - TEST_F(RemoveSignatory, RemoveSignatoryTestNonExistingAccount) { - addAllPerms(); - shared_model::interface::types::PubkeyType pk(std::string('5', 32)); - ASSERT_TRUE( - val(execute(buildCommand(TestTransactionBuilder().addSignatory( - account->accountId(), pk)), - true))); - - ASSERT_TRUE(err(execute(buildCommand( - TestTransactionBuilder().removeSignatory("hello", *pubkey))))); + auto cmd_result = execute(buildCommand( + TestTransactionBuilder().removeSignatory(account->accountId(), pk))); + checkErrorCode(cmd_result, 8); } class RevokePermission : public CommandExecutorTest { @@ -1123,10 +1295,11 @@ namespace iroha { TEST_F(RevokePermission, InvalidRevokePermissionTestWithNoPermission) { auto perm = shared_model::interface::permissions::Grantable::kRemoveMySignatory; - ASSERT_TRUE(err( + auto cmd_result = execute(buildCommand(TestTransactionBuilder() .revokePermission(account->accountId(), perm) - .creatorAccountId(account->accountId()))))); + .creatorAccountId(account->accountId()))); + checkErrorCode(cmd_result, 5); } class SetAccountDetail : public CommandExecutorTest { @@ -1218,15 +1391,31 @@ namespace iroha { /** * @given command - * @when trying to set kv - * @then kv is set + * @when trying to set kv to non-existing account + * @then corresponding error code is returned */ - TEST_F(SetAccountDetail, InvalidSetAccountDetailTest) { - ASSERT_TRUE( - err(execute(buildCommand(TestTransactionBuilder().setAccountDetail( - account2->accountId(), "key", "value")), - false, - account->accountId()))); + TEST_F(SetAccountDetail, InvalidSetAccountDetailTestNoAccount) { + addAllPerms(); + auto cmd_result = + execute(buildCommand(TestTransactionBuilder().setAccountDetail( + "doge@noaccount", "key", "value")), + false, + account->accountId()); + checkErrorCode(cmd_result, 2); + } + + /** + * @given command + * @when trying to set kv while having no permissions + * @then corresponding error code is returned + */ + TEST_F(SetAccountDetail, InvalidSetAccountDetailTestNoPerms) { + auto cmd_result = + execute(buildCommand(TestTransactionBuilder().setAccountDetail( + account2->accountId(), "key", "value")), + false, + account->accountId()); + checkErrorCode(cmd_result, 5); auto kv = query->getAccountDetail(account2->accountId()); ASSERT_TRUE(kv); ASSERT_EQ(kv.get(), "{}"); From efaeb6c948c8d9849482b72a94186c056249838c Mon Sep 17 00:00:00 2001 From: Akvinikym Date: Fri, 26 Oct 2018 15:36:48 +0300 Subject: [PATCH 05/17] Tests are finished Signed-off-by: Akvinikym --- .../impl/postgres_command_executor.cpp | 1 - .../ametsuchi/postgres_executor_test.cpp | 220 ++++++++++-------- 2 files changed, 129 insertions(+), 92 deletions(-) diff --git a/irohad/ametsuchi/impl/postgres_command_executor.cpp b/irohad/ametsuchi/impl/postgres_command_executor.cpp index 735ff7e7f7..ce3015579b 100644 --- a/irohad/ametsuchi/impl/postgres_command_executor.cpp +++ b/irohad/ametsuchi/impl/postgres_command_executor.cpp @@ -588,7 +588,6 @@ namespace iroha { SELECT CASE WHEN EXISTS (SELECT * FROM inserted LIMIT 1) THEN 0 %s - WHEN NOT EXISTS (SELECT * FROM has_account LIMIT 1) THEN 2 WHEN NOT EXISTS (SELECT * FROM has_asset LIMIT 1) THEN 3 WHEN NOT EXISTS (SELECT value FROM new_value WHERE value >= 0 LIMIT 1) THEN 4 diff --git a/test/module/irohad/ametsuchi/postgres_executor_test.cpp b/test/module/irohad/ametsuchi/postgres_executor_test.cpp index 45528d350d..61a6cfa6a3 100644 --- a/test/module/irohad/ametsuchi/postgres_executor_test.cpp +++ b/test/module/irohad/ametsuchi/postgres_executor_test.cpp @@ -1440,26 +1440,6 @@ namespace iroha { } }; - /** - * @given command - * @when trying to set quorum more than amount of signatories - * @then quorum is not set - */ - TEST_F(SetQuorum, SetQuorumTestInvalidSignatories) { - addAllPerms(); - ASSERT_TRUE( - val(execute(buildCommand(TestTransactionBuilder().setAccountQuorum( - account->accountId(), 3))))); - shared_model::interface::types::PubkeyType pk(std::string('5', 32)); - ASSERT_TRUE( - val(execute(buildCommand(TestTransactionBuilder().addSignatory( - account->accountId(), pk)), - true))); - ASSERT_TRUE( - err(execute(buildCommand(TestTransactionBuilder().setAccountQuorum( - account->accountId(), 1))))); - } - /** * @given command * @when trying to set quorum @@ -1495,13 +1475,33 @@ namespace iroha { /** * @given command - * @when trying to set quorum without perms + * @when trying to set quorum more than amount of signatories * @then quorum is not set */ - TEST_F(SetQuorum, InvalidSetQuorumTestWithNoPerms) { + TEST_F(SetQuorum, SetQuorumTestInvalidSignatoriesAmount) { + addAllPerms(); ASSERT_TRUE( - err(execute(buildCommand(TestTransactionBuilder().setAccountQuorum( + val(execute(buildCommand(TestTransactionBuilder().setAccountQuorum( account->accountId(), 3))))); + shared_model::interface::types::PubkeyType pk(std::string('5', 32)); + ASSERT_TRUE( + val(execute(buildCommand(TestTransactionBuilder().addSignatory( + account->accountId(), pk)), + true))); + auto cmd_result = execute(buildCommand( + TestTransactionBuilder().setAccountQuorum(account->accountId(), 1))); + checkErrorCode(cmd_result, 4); + } + + /** + * @given command + * @when trying to set quorum without perms + * @then quorum is not set + */ + TEST_F(SetQuorum, InvalidSetQuorumTestWithNoPerms) { + auto cmd_result = execute(buildCommand( + TestTransactionBuilder().setAccountQuorum(account->accountId(), 3))); + checkErrorCode(cmd_result, 5); } class SubtractAccountAssetTest : public CommandExecutorTest { @@ -1576,32 +1576,6 @@ namespace iroha { ASSERT_EQ("1.0", account_asset.get()->balance().toStringRepr()); } - /** - * @given command - * @when trying to subtract account asset - * @then account asset is successfully subtracted - */ - TEST_F(SubtractAccountAssetTest, - InvalidSubtractAccountAssetTestWhenNoPerms) { - addAsset(); - ASSERT_TRUE( - val(execute(buildCommand(TestTransactionBuilder() - .addAssetQuantity(asset_id, "1.0") - .creatorAccountId(account->accountId())), - true))); - auto account_asset = - query->getAccountAsset(account->accountId(), asset_id); - ASSERT_TRUE(account_asset); - ASSERT_EQ("1.0", account_asset.get()->balance().toStringRepr()); - ASSERT_TRUE(err( - execute(buildCommand(TestTransactionBuilder() - .subtractAssetQuantity(asset_id, "1.0") - .creatorAccountId(account->accountId()))))); - account_asset = query->getAccountAsset(account->accountId(), asset_id); - ASSERT_TRUE(account_asset); - ASSERT_EQ("1.0", account_asset.get()->balance().toStringRepr()); - } - /** * @given command * @when trying to subtract account asset with non-existing asset @@ -1609,57 +1583,72 @@ namespace iroha { */ TEST_F(SubtractAccountAssetTest, SubtractAccountAssetTestInvalidAsset) { addAllPerms(); - ASSERT_TRUE(err( + auto cmd_result = execute(buildCommand(TestTransactionBuilder() .subtractAssetQuantity(asset_id, "1.0") - .creatorAccountId(account->accountId()))))); + .creatorAccountId(account->accountId()))); + checkErrorCode(cmd_result, 3); } /** * @given command - * @when trying to add account subtract with non-existing account - * @then account asset fails to subtracted + * @when trying to add account asset with wrong precision + * @then account asset fails to added */ - TEST_F(SubtractAccountAssetTest, SubtractAccountAssetTestInvalidAccount) { + TEST_F(SubtractAccountAssetTest, SubtractAccountAssetTestInvalidPrecision) { addAllPerms(); addAsset(); - ASSERT_TRUE( - err(execute(buildCommand(TestTransactionBuilder() - .subtractAssetQuantity(asset_id, "1.0") - .creatorAccountId("some@domain"))))); + auto cmd_result = + execute(buildCommand(TestTransactionBuilder() + .subtractAssetQuantity(asset_id, "1.0000") + .creatorAccountId(account->accountId()))); + checkErrorCode(cmd_result, 3); } /** * @given command - * @when trying to add account asset with wrong precision - * @then account asset fails to added + * @when trying to subtract more account asset than account has + * @then account asset fails to be subtracted */ - TEST_F(SubtractAccountAssetTest, SubtractAccountAssetTestInvalidPrecision) { + TEST_F(SubtractAccountAssetTest, SubtractAccountAssetTestNotEnoughAsset) { addAllPerms(); addAsset(); - ASSERT_TRUE(err( + ASSERT_TRUE( + val(execute(buildCommand(TestTransactionBuilder() + .addAssetQuantity(asset_id, "1.0") + .creatorAccountId(account->accountId())), + true))); + auto cmd_result = execute(buildCommand(TestTransactionBuilder() - .subtractAssetQuantity(asset_id, "1.0000") - .creatorAccountId(account->accountId()))))); + .subtractAssetQuantity(asset_id, "2.0") + .creatorAccountId(account->accountId()))); + checkErrorCode(cmd_result, 4); } /** * @given command - * @when trying to add account asset that overflows - * @then account asset fails to added + * @when trying to subtract account asset without permissions + * @then corresponding error code is returned */ - TEST_F(SubtractAccountAssetTest, SubtractAccountAssetTestUint256Overflow) { - addAllPerms(); + TEST_F(SubtractAccountAssetTest, + InvalidSubtractAccountAssetTestWhenNoPerms) { addAsset(); ASSERT_TRUE( val(execute(buildCommand(TestTransactionBuilder() .addAssetQuantity(asset_id, "1.0") .creatorAccountId(account->accountId())), true))); + auto account_asset = + query->getAccountAsset(account->accountId(), asset_id); + ASSERT_TRUE(account_asset); + ASSERT_EQ("1.0", account_asset.get()->balance().toStringRepr()); ASSERT_TRUE(err( execute(buildCommand(TestTransactionBuilder() - .subtractAssetQuantity(asset_id, "2.0") + .subtractAssetQuantity(asset_id, "1.0") .creatorAccountId(account->accountId()))))); + account_asset = query->getAccountAsset(account->accountId(), asset_id); + ASSERT_TRUE(account_asset); + ASSERT_EQ("1.0", account_asset.get()->balance().toStringRepr()); } class TransferAccountAssetTest : public CommandExecutorTest { @@ -1797,6 +1786,32 @@ namespace iroha { ASSERT_EQ("1.0", account_asset.get()->balance().toStringRepr()); } + /** + * @given command + * @when trying to transfer asset back and forth with non-existing account + * @then account asset fails to be transfered + */ + TEST_F(TransferAccountAssetTest, TransferAccountAssetTestInvalidAccount) { + addAllPerms(); + addAllPerms(account2->accountId(), "all2"); + addAsset(); + ASSERT_TRUE( + val(execute(buildCommand(TestTransactionBuilder() + .addAssetQuantity(asset_id, "1.0") + .creatorAccountId(account->accountId())), + true))); + auto cmd_result = execute( + buildCommand(TestTransactionBuilder().transferAsset( + "some@domain", account2->accountId(), asset_id, "desc", "1.0")), + true); + checkErrorCode(cmd_result, 2); + cmd_result = execute( + buildCommand(TestTransactionBuilder().transferAsset( + account->accountId(), "some@domain", asset_id, "desc", "1.0")), + true); + checkErrorCode(cmd_result, 3); + } + /** * @given command * @when trying to transfer account asset with non-existing asset @@ -1805,42 +1820,34 @@ namespace iroha { TEST_F(TransferAccountAssetTest, TransferAccountAssetTestInvalidAsset) { addAllPerms(); addAllPerms(account2->accountId(), "all2"); - ASSERT_TRUE(err(execute(buildCommand( + auto cmd_result = execute(buildCommand( TestTransactionBuilder().transferAsset(account->accountId(), account2->accountId(), asset_id, "desc", - "1.0"))))); + "1.0"))); + checkErrorCode(cmd_result, 4); } /** * @given command - * @when trying to transfer account asset with non-existing account - * @then account asset fails to transfered + * @when trying to transfer account asset with no permissions + * @then account asset fails to be transfered */ - TEST_F(TransferAccountAssetTest, TransferAccountAssetTestInvalidAccount) { - addAllPerms(); - addAllPerms(account2->accountId(), "all2"); - addAsset(); - ASSERT_TRUE( - val(execute(buildCommand(TestTransactionBuilder() - .addAssetQuantity(asset_id, "1.0") - .creatorAccountId(account->accountId())), - true))); - ASSERT_TRUE( - err(execute(buildCommand(TestTransactionBuilder().transferAsset( - account->accountId(), "some@domain", asset_id, "desc", "1.0"))))); - ASSERT_TRUE(err(execute(buildCommand( - TestTransactionBuilder().transferAsset("some@domain", + TEST_F(TransferAccountAssetTest, + TransferAccountAssetTestInvalidNoPermission) { + auto cmd_result = execute(buildCommand( + TestTransactionBuilder().transferAsset(account->accountId(), account2->accountId(), asset_id, "desc", - "1.0"))))); + "1.0"))); + checkErrorCode(cmd_result, 5); } /** * @given command - * @when trying to transfer account asset that overflows + * @when trying to transfer account asset, but has insufficient amount of it * @then account asset fails to transfered */ TEST_F(TransferAccountAssetTest, TransferAccountAssetOwerdraftTest) { @@ -1852,12 +1859,43 @@ namespace iroha { .addAssetQuantity(asset_id, "1.0") .creatorAccountId(account->accountId())), true))); - ASSERT_TRUE(err(execute(buildCommand( + auto cmd_result = execute(buildCommand( TestTransactionBuilder().transferAsset(account->accountId(), account2->accountId(), asset_id, "desc", - "2.0"))))); + "2.0"))); + checkErrorCode(cmd_result, 8); + } + + TEST_F(TransferAccountAssetTest, + TransferAccountAssetInvalidOverflowDestination) { + addAllPerms(); + addAllPerms(account2->accountId(), "all2"); + addAsset(); + std::string uint256_halfmax = + "57896044618658097711785492504343953926634992332820282019728792003956" + "5648" + "19966.0"; + ASSERT_TRUE(val( + execute(buildCommand(TestTransactionBuilder() + .addAssetQuantity(asset_id, uint256_halfmax) + .creatorAccountId(account->accountId())), + true))); + ASSERT_TRUE( + val(execute(buildCommand(TestTransactionBuilder().addAssetQuantity( + asset_id, uint256_halfmax)), + false, + account2->accountId()))); + auto cmd_result = + execute(buildCommand(TestTransactionBuilder().transferAsset( + account->accountId(), + account2->accountId(), + asset_id, + "desc", + uint256_halfmax)), + true); + checkErrorCode(cmd_result, 11); } } // namespace ametsuchi } // namespace iroha From 65d011052614c3f0bcb3a64f03583c9a32dd6787 Mon Sep 17 00:00:00 2001 From: Akvinikym Date: Mon, 29 Oct 2018 11:24:35 +0300 Subject: [PATCH 06/17] Review issues Signed-off-by: Akvinikym --- .../impl/postgres_command_executor.cpp | 137 ++++++----- .../ametsuchi/postgres_executor_test.cpp | 231 +++++++++++------- 2 files changed, 218 insertions(+), 150 deletions(-) diff --git a/irohad/ametsuchi/impl/postgres_command_executor.cpp b/irohad/ametsuchi/impl/postgres_command_executor.cpp index ce3015579b..9526804605 100644 --- a/irohad/ametsuchi/impl/postgres_command_executor.cpp +++ b/irohad/ametsuchi/impl/postgres_command_executor.cpp @@ -76,11 +76,82 @@ namespace { } iroha::expected::Error makeCommandError( - const std::string &command_name, + std::string command_name, const iroha::ametsuchi::CommandError::ErrorCodeType code, const std::string &error_extra = "") noexcept { - return iroha::expected::makeError( - iroha::ametsuchi::CommandError{command_name, code, error_extra}); + return iroha::expected::makeError(iroha::ametsuchi::CommandError{ + std::move(command_name), code, error_extra}); + } + + /** + * Parse the SQL error to turn it into an error code + * @param command_name of the failed command + * @param error - string error, which SQL gave out + * @return command_error structure + */ + iroha::ametsuchi::CommandResult parseSqlError( + std::string command_name, const std::string &error) noexcept { + if ((error.find("Key (account_id)=") != std::string::npos + or error.find("Key (permittee_account_id)") != std::string::npos) + and error.find("is not present in table") != std::string::npos) { + // account, which is specified in query, is not found + return makeCommandError(std::move(command_name), 2); + } + + if (error.find("Key (role_id)=") != std::string::npos + and error.find("is not present in table") != std::string::npos) { + // role, which is specified in query, is not found + return makeCommandError(std::move(command_name), 6); + } + + if (error.find("Key (domain_id)=") != std::string::npos + and error.find("is not present in table") != std::string::npos) { + // domain, which is specified in query, is not found + return makeCommandError(std::move(command_name), 7); + } + + if (error.find("Key (asset_id)=") != std::string::npos + and error.find("already exists") != std::string::npos) { + // asset, which is specified in query, already exists and thus cannot be + // created + return makeCommandError(std::move(command_name), 9); + } + + if (error.find("Key (domain_id)=") != std::string::npos + and error.find("already exists") != std::string::npos) { + // domain, which is specified in query, already exists and thus cannot be + // created + return makeCommandError(std::move(command_name), 10); + } + + if (error.find("Key (role_id)=") != std::string::npos + and error.find("already exists") != std::string::npos) { + // role, which is specified in query, already exists and thus cannot be + // created + return makeCommandError(std::move(command_name), 11); + } + + if (error.find("Key (account_id, public_key)") != std::string::npos + and error.find("already exists") != std::string::npos) { + // account already has such signatory attached + return makeCommandError(std::move(command_name), 12); + } + + if (error.find("Key (account_id)") != std::string::npos + and error.find("already exists") != std::string::npos) { + // account, which is specified in query, already exists and thus cannot be + // created + return makeCommandError(std::move(command_name), 13); + } + + if (error.find("Key (default_role)") != std::string::npos + and error.find("is not present in table") != std::string::npos) { + // default role for the domain is not found + return makeCommandError(std::move(command_name), 14); + } + + // parsing is not successful, return the general error + return makeCommandError(std::move(command_name), 1, error); } /** @@ -95,66 +166,16 @@ namespace { iroha::ametsuchi::CommandResult executeQuery( soci::session &sql, const std::string &cmd, - const std::string &command_name) noexcept { + std::string command_name) noexcept { uint32_t result; try { sql << cmd, soci::into(result); if (result != 0) { - return makeCommandError(command_name, result); + return makeCommandError(std::move(command_name), result); } return {}; - } catch (std::exception &e) { - // try to parse the SQL error to get an actual one - std::string exc_body = e.what(); - - if ((exc_body.find("Key (account_id)=") != std::string::npos - or exc_body.find("Key (permittee_account_id)") != std::string::npos) - and exc_body.find("is not present in table") != std::string::npos) { - return makeCommandError(command_name, 2); - } - - if (exc_body.find("Key (role_id)=") != std::string::npos - and exc_body.find("is not present in table") != std::string::npos) { - return makeCommandError(command_name, 6); - } - - if (exc_body.find("Key (domain_id)=") != std::string::npos - and exc_body.find("is not present in table") != std::string::npos) { - return makeCommandError(command_name, 7); - } - - if (exc_body.find("Key (asset_id)=") != std::string::npos - and exc_body.find("already exists") != std::string::npos) { - return makeCommandError(command_name, 9); - } - - if (exc_body.find("Key (domain_id)=") != std::string::npos - and exc_body.find("already exists") != std::string::npos) { - return makeCommandError(command_name, 10); - } - - if (exc_body.find("Key (role_id)=") != std::string::npos - and exc_body.find("already exists") != std::string::npos) { - return makeCommandError(command_name, 11); - } - - if (exc_body.find("Key (account_id, public_key)") != std::string::npos - and exc_body.find("already exists") != std::string::npos) { - return makeCommandError(command_name, 12); - } - - if (exc_body.find("Key (account_id)") != std::string::npos - and exc_body.find("already exists") != std::string::npos) { - return makeCommandError(command_name, 13); - } - - if (exc_body.find("Key (default_role)") != std::string::npos - and exc_body.find("is not present in table") != std::string::npos) { - return makeCommandError(command_name, 14); - } - - // parsing is not successful, return the general error - return makeCommandError(command_name, 1, e.what()); + } catch (const std::exception &e) { + return parseSqlError(std::move(command_name), e.what()); } } @@ -674,7 +695,7 @@ namespace iroha { WHERE value >= 0 LIMIT 1) THEN 8 WHEN NOT EXISTS (SELECT value FROM new_dest_value WHERE value < 2::decimal ^ (256 - $5) - LIMIT 1) THEN 11 + LIMIT 1) THEN 15 ELSE 1 END AS result)"; diff --git a/test/module/irohad/ametsuchi/postgres_executor_test.cpp b/test/module/irohad/ametsuchi/postgres_executor_test.cpp index 61a6cfa6a3..f9c92a15fb 100644 --- a/test/module/irohad/ametsuchi/postgres_executor_test.cpp +++ b/test/module/irohad/ametsuchi/postgres_executor_test.cpp @@ -168,7 +168,7 @@ namespace iroha { * @when trying to add account asset * @then account asset is successfully added */ - TEST_F(AddAccountAssetTest, ValidAddAccountAssetTest) { + TEST_F(AddAccountAssetTest, Valid) { addAsset(); addAllPerms(); ASSERT_TRUE(val( @@ -191,9 +191,9 @@ namespace iroha { /** * @given command * @when trying to add account asset with non-existing account - * @then account asset fails to added + * @then account asset fails to be added */ - TEST_F(AddAccountAssetTest, AddAccountAssetTestInvalidAccount) { + TEST_F(AddAccountAssetTest, InvalidAccount) { addAsset(); auto cmd_result = execute(buildCommand(TestTransactionBuilder() @@ -209,7 +209,7 @@ namespace iroha { * @when trying to add account asset with non-existing asset * @then account asset fails to be added */ - TEST_F(AddAccountAssetTest, AddAccountAssetTestInvalidAsset) { + TEST_F(AddAccountAssetTest, InvalidAsset) { auto cmd_result = execute(buildCommand(TestTransactionBuilder() .addAssetQuantity(asset_id, "1.0") @@ -223,7 +223,7 @@ namespace iroha { * @when trying to add account asset that overflows * @then account asset fails to added */ - TEST_F(AddAccountAssetTest, AddAccountAssetTestUint256Overflow) { + TEST_F(AddAccountAssetTest, Uint256Overflow) { std::string uint256_halfmax = "57896044618658097711785492504343953926634992332820282019728792003956" "5648" @@ -247,7 +247,7 @@ namespace iroha { * @when trying to add account asset without permission * @then account asset not added */ - TEST_F(AddAccountAssetTest, AddAccountAssetTestNoPerms) { + TEST_F(AddAccountAssetTest, NoPerms) { addAsset(); ASSERT_TRUE( val(execute(buildCommand(TestTransactionBuilder() @@ -292,7 +292,7 @@ namespace iroha { * @when trying to add peer * @then peer is successfully added */ - TEST_F(AddPeer, ValidAddPeerTest) { + TEST_F(AddPeer, Valid) { addAllPerms(); ASSERT_TRUE(val(execute(buildCommand( TestTransactionBuilder().addPeer(peer->address(), peer->pubkey()))))); @@ -303,7 +303,7 @@ namespace iroha { * @when trying to add peer without perms * @then peer is not added */ - TEST_F(AddPeer, InvalidAddPeerTestWhenNoPerms) { + TEST_F(AddPeer, NoPerms) { auto cmd_result = execute(buildCommand( TestTransactionBuilder().addPeer(peer->address(), peer->pubkey()))); checkErrorCode(cmd_result, 5); @@ -336,7 +336,7 @@ namespace iroha { * @when trying to add signatory with role permission * @then signatory is successfully added */ - TEST_F(AddSignatory, ValidAddSignatoryTestRolePerms) { + TEST_F(AddSignatory, Valid) { addAllPerms(); ASSERT_TRUE( val(execute(buildCommand(TestTransactionBuilder().addSignatory( @@ -352,7 +352,7 @@ namespace iroha { * @when trying to add signatory with grantable permission * @then signatory is successfully added */ - TEST_F(AddSignatory, ValidAddSignatoryTestGrantablePerms) { + TEST_F(AddSignatory, ValidGrantablePerms) { ASSERT_TRUE( val(execute(buildCommand(TestTransactionBuilder().createAccount( "id2", @@ -380,7 +380,7 @@ namespace iroha { * @when trying to add signatory to non-existing account * @then signatory is not added */ - TEST_F(AddSignatory, InvalidAddSignatoryTestNoAccount) { + TEST_F(AddSignatory, NoAccount) { auto perm = shared_model::interface::permissions::Grantable::kAddMySignatory; auto cmd_result = @@ -396,7 +396,7 @@ namespace iroha { * @when trying to add signatory without permissions * @then signatory is not added */ - TEST_F(AddSignatory, InvalidAddSignatoryTestWhenNoPerms) { + TEST_F(AddSignatory, NoPerms) { auto cmd_result = execute(buildCommand(TestTransactionBuilder().addSignatory( account->accountId(), *pubkey))); @@ -414,7 +414,7 @@ namespace iroha { * same signatory again * @then signatory is not added */ - TEST_F(AddSignatory, InvalidAddSignatoryExistingPubKey) { + TEST_F(AddSignatory, ExistingPubKey) { addAllPerms(); ASSERT_TRUE( val(execute(buildCommand(TestTransactionBuilder().addSignatory( @@ -446,7 +446,12 @@ namespace iroha { shared_model::interface::RolePermissionSet role_permissions2; }; - TEST_F(AppendRole, ValidAppendRoleTest) { + /** + * @given command + * @when trying to append role + * @then role is appended + */ + TEST_F(AppendRole, Valid) { addAllPerms(); ASSERT_TRUE(val(execute(buildCommand(TestTransactionBuilder().createRole( another_role, role_permissions)), @@ -459,7 +464,12 @@ namespace iroha { != roles->end()); } - TEST_F(AppendRole, ValidAppendRoleTestWhenEmptyPerms) { + /** + * @given command + * @when trying append role, which does not have any permissions + * @then role is appended + */ + TEST_F(AppendRole, ValidEmptyPerms) { addAllPerms(); ASSERT_TRUE(val(execute( buildCommand(TestTransactionBuilder().createRole(another_role, {})), @@ -473,12 +483,12 @@ namespace iroha { } /** - * @given command - * @when trying to append role with perms that creator does not have - * but in genesis block + * @given command + * @when trying to append role with perms that creator does not have but in + * genesis block * @then role is appended */ - TEST_F(AppendRole, AppendRoleTestValidWhenAccountDoesNotHavePermsGenesis) { + TEST_F(AppendRole, AccountDoesNotHavePermsGenesis) { role_permissions2.set( shared_model::interface::permissions::Role::kRemoveMySignatory); ASSERT_TRUE(val(execute(buildCommand(TestTransactionBuilder().createRole( @@ -493,7 +503,12 @@ namespace iroha { != roles->end()); } - TEST_F(AppendRole, AppendRoleTestInvalidNoAccount) { + /** + * @given command + * @when trying to append role to non-existing account + * @then role is not appended + */ + TEST_F(AppendRole, NoAccount) { addAllPerms(); ASSERT_TRUE(val(execute( buildCommand(TestTransactionBuilder().createRole(another_role, {})), @@ -503,7 +518,12 @@ namespace iroha { checkErrorCode(cmd_result, 2); } - TEST_F(AppendRole, InvalidAppendRoleTestWhenNoPerms) { + /** + * @given command + * @when trying to append role having no permission to do so + * @then role is not appended + */ + TEST_F(AppendRole, NoPerms) { ASSERT_TRUE(val(execute(buildCommand(TestTransactionBuilder().createRole( another_role, role_permissions)), true))); @@ -523,7 +543,7 @@ namespace iroha { * @when trying to append role with perms that creator does not have * @then role is not appended */ - TEST_F(AppendRole, AppendRoleTestInvalidWhenAccountDoesNotHavePerms) { + TEST_F(AppendRole, NoRolePermsInAccount) { role_permissions2.set( shared_model::interface::permissions::Role::kRemoveMySignatory); ASSERT_TRUE(val(execute(buildCommand(TestTransactionBuilder().createRole( @@ -535,7 +555,12 @@ namespace iroha { checkErrorCode(cmd_result, 5); } - TEST_F(AppendRole, AppendRoleTestInvalidNoSuchRole) { + /** + * @given command + * @when trying to append non-existing role + * @then role is not appended + */ + TEST_F(AppendRole, NoRole) { addAllPerms(); auto cmd_result = execute(buildCommand(TestTransactionBuilder().appendRole( @@ -581,7 +606,7 @@ namespace iroha { * @when trying to create account * @then account is created */ - TEST_F(CreateAccount, ValidCreateAccountWithDomainTest) { + TEST_F(CreateAccount, Valid) { addAllPerms(); ASSERT_TRUE( val(execute(buildCommand(TestTransactionBuilder().createAccount( @@ -593,10 +618,10 @@ namespace iroha { /** * @given command - * @when trying to create account - * @then account is created + * @when trying to create account without permission to do so + * @then account is not created */ - TEST_F(CreateAccount, InvalidCreateAccountWithoutPermsTest) { + TEST_F(CreateAccount, NoPerms) { auto cmd_result = execute(buildCommand(TestTransactionBuilder().createAccount( "id2", domain->domainId(), *pubkey))); @@ -606,18 +631,23 @@ namespace iroha { } /** - * @given command and no target domain in ledger + * @given command @and no target domain in ledger * @when trying to create account * @then account is not created */ - TEST_F(CreateAccount, InvalidCreateAccountNoDomainTest) { + TEST_F(CreateAccount, NoDomain) { addAllPerms(); auto cmd_result = execute(buildCommand( TestTransactionBuilder().createAccount("doge", "domain6", *pubkey))); checkErrorCode(cmd_result, 7); } - TEST_F(CreateAccount, InvalidCreateAccountNameExists) { + /** + * @given command + * @when trying to create with an occupied name + * @then account is not created + */ + TEST_F(CreateAccount, NameExists) { addAllPerms(); auto cmd_result = execute(buildCommand(TestTransactionBuilder().createAccount( @@ -640,7 +670,7 @@ namespace iroha { * @when trying to create asset * @then asset is created */ - TEST_F(CreateAsset, ValidCreateAssetWithDomainTest) { + TEST_F(CreateAsset, Valid) { role_permissions.set( shared_model::interface::permissions::Role::kCreateAsset); ASSERT_TRUE(val(execute(buildCommand(TestTransactionBuilder().createRole( @@ -671,7 +701,7 @@ namespace iroha { * @when trying to create asset without permission * @then asset is not created */ - TEST_F(CreateAsset, InvalidCreateAssetWithNoPermission) { + TEST_F(CreateAsset, NoPerms) { ASSERT_TRUE(val(execute(buildCommand(TestTransactionBuilder().createRole( role, role_permissions)), true))); @@ -700,7 +730,7 @@ namespace iroha { * @when trying to create asset * @then asset is not created */ - TEST_F(CreateAsset, InvalidCreateAssetNoDomain) { + TEST_F(CreateAsset, NoDomain) { role_permissions.set( shared_model::interface::permissions::Role::kCreateAsset); ASSERT_TRUE(val(execute(buildCommand(TestTransactionBuilder().createRole( @@ -724,7 +754,12 @@ namespace iroha { checkErrorCode(cmd_result, 7); } - TEST_F(CreateAsset, InvalidCreateAssetNameNotUnique) { + /** + * @given command + * @when trying to create asset with an occupied name + * @then asset is not created + */ + TEST_F(CreateAsset, NameNotUnique) { role_permissions.set( shared_model::interface::permissions::Role::kCreateAsset); ASSERT_TRUE(val(execute(buildCommand(TestTransactionBuilder().createRole( @@ -778,7 +813,7 @@ namespace iroha { * @when trying to create domain * @then domain is created */ - TEST_F(CreateDomain, ValidCreateDomainTest) { + TEST_F(CreateDomain, Valid) { addAllPerms(); ASSERT_TRUE(val(execute(buildCommand( TestTransactionBuilder().createDomain(domain2->domainId(), role))))); @@ -792,7 +827,7 @@ namespace iroha { * @when trying to create domain * @then domain is not created */ - TEST_F(CreateDomain, InvalidCreateDomainTestWhenNoPerms) { + TEST_F(CreateDomain, NoPerms) { auto cmd_result = execute(buildCommand( TestTransactionBuilder().createDomain(domain2->domainId(), role))); checkErrorCode(cmd_result, 5); @@ -800,7 +835,12 @@ namespace iroha { ASSERT_FALSE(dom); } - TEST_F(CreateDomain, InvalidCreateDomainWhenNameNotUnique) { + /** + * @given command + * @when trying to create domain with an occupied name + * @then domain is not created + */ + TEST_F(CreateDomain, NameNotUnique) { addAllPerms(); ASSERT_TRUE(val(execute(buildCommand( TestTransactionBuilder().createDomain(domain2->domainId(), role))))); @@ -810,11 +850,11 @@ namespace iroha { } /** - * @given command when there is no role + * @given command when there is no default role * @when trying to create domain * @then domain is not created */ - TEST_F(CreateDomain, InvalidCreateDomainWhenNoRoleTest) { + TEST_F(CreateDomain, NoDefaultRole) { addAllPerms(); auto cmd_result = execute(buildCommand(TestTransactionBuilder().createDomain( @@ -847,7 +887,7 @@ namespace iroha { * @when trying to create role * @then role is created */ - TEST_F(CreateRole, ValidCreateRoleTest) { + TEST_F(CreateRole, Valid) { addAllPerms(); ASSERT_TRUE(val(execute(buildCommand(TestTransactionBuilder().createRole( another_role, role_permissions))))); @@ -861,7 +901,7 @@ namespace iroha { * @when trying to create role when creator doesn't have all permissions * @then role is not created */ - TEST_F(CreateRole, CreateRoleTestInvalidWhenHasNoPerms) { + TEST_F(CreateRole, NoPerms) { role_permissions2.set( shared_model::interface::permissions::Role::kRemoveMySignatory); auto cmd_result = @@ -873,7 +913,12 @@ namespace iroha { ASSERT_TRUE(rl->none()); } - TEST_F(CreateRole, CreateRoleTestInvalidNameNotUnique) { + /** + * @given command + * @when trying to create role with an occupied name + * @then role is not created + */ + TEST_F(CreateRole, NameNotUnique) { addAllPerms(); ASSERT_TRUE(val(execute(buildCommand(TestTransactionBuilder().createRole( another_role, role_permissions))))); @@ -914,7 +959,7 @@ namespace iroha { * @when trying to detach role * @then role is detached */ - TEST_F(DetachRole, ValidDetachRoleTest) { + TEST_F(DetachRole, Valid) { addAllPerms(); ASSERT_TRUE(val(execute(buildCommand(TestTransactionBuilder().detachRole( account->accountId(), another_role))))); @@ -929,7 +974,7 @@ namespace iroha { * @when trying to detach role from non-existing account * @then correspondent error code is returned */ - TEST_F(DetachRole, InvalidDetachRoleTestNoAccount) { + TEST_F(DetachRole, NoAccount) { addAllPerms(); auto cmd_result = execute(buildCommand( TestTransactionBuilder().detachRole("doge@noaccount", another_role))); @@ -941,7 +986,7 @@ namespace iroha { * @when trying to detach role, which the account does not have * @then correspondent error code is returned */ - TEST_F(DetachRole, InvalidDetachRoleTestNoRoleInAccountRoles) { + TEST_F(DetachRole, NoSuchRoleInAccount) { addAllPerms(); ASSERT_TRUE(val(execute(buildCommand(TestTransactionBuilder().detachRole( account->accountId(), another_role))))); @@ -956,7 +1001,7 @@ namespace iroha { * @when trying to detach role without permission * @then role is detached */ - TEST_F(DetachRole, InvalidDetachRoleTestWhenNoPerms) { + TEST_F(DetachRole, NoPerms) { auto cmd_result = execute(buildCommand(TestTransactionBuilder().detachRole( account->accountId(), another_role))); @@ -972,7 +1017,7 @@ namespace iroha { * @when trying to detach a non-existing role * @then correspondent error code is returned */ - TEST_F(DetachRole, InvalidDetachRoleTestNoRole) { + TEST_F(DetachRole, NoRole) { addAllPerms(); auto cmd_result = execute(buildCommand(TestTransactionBuilder().detachRole( @@ -1008,7 +1053,7 @@ namespace iroha { * @when trying to grant permission * @then permission is granted */ - TEST_F(GrantPermission, ValidGrantPermissionTest) { + TEST_F(GrantPermission, Valid) { addAllPerms(); auto perm = shared_model::interface::permissions::Grantable::kSetMyQuorum; ASSERT_TRUE(val( @@ -1025,7 +1070,7 @@ namespace iroha { * @when trying to grant permission to non-existent account * @then corresponding error code is returned */ - TEST_F(GrantPermission, InvalidGrantPermissionTestNoAccount) { + TEST_F(GrantPermission, NoAccount) { addAllPerms(); auto perm = shared_model::interface::permissions::Grantable::kSetMyQuorum; auto cmd_result = @@ -1040,7 +1085,7 @@ namespace iroha { * @when trying to grant permission without permission * @then permission is not granted */ - TEST_F(GrantPermission, InvalidGrantPermissionTestWhenNoPerm) { + TEST_F(GrantPermission, NoPerms) { auto perm = shared_model::interface::permissions::Grantable::kSetMyQuorum; auto cmd_result = @@ -1085,7 +1130,7 @@ namespace iroha { * @when trying to remove signatory * @then signatory is successfully removed */ - TEST_F(RemoveSignatory, ValidRemoveSignatoryTest) { + TEST_F(RemoveSignatory, Valid) { addAllPerms(); shared_model::interface::types::PubkeyType pk(std::string('5', 32)); ASSERT_TRUE( @@ -1108,7 +1153,7 @@ namespace iroha { * @when trying to remove signatory * @then signatory is successfully removed */ - TEST_F(RemoveSignatory, ValidRemoveSignatoryTestWhenHasGrantablePerm) { + TEST_F(RemoveSignatory, ValidGrantablePerm) { ASSERT_TRUE( val(execute(buildCommand(TestTransactionBuilder().createAccount( "id2", domain->domainId(), *pubkey)), @@ -1143,7 +1188,7 @@ namespace iroha { * @when trying to remove signatory from a non existing account * @then corresponding error code is returned */ - TEST_F(RemoveSignatory, RemoveSignatoryTestNonExistingAccount) { + TEST_F(RemoveSignatory, NoAccount) { addAllPerms(); shared_model::interface::types::PubkeyType pk(std::string('5', 32)); ASSERT_TRUE( @@ -1161,7 +1206,7 @@ namespace iroha { * @when trying to remove signatory, which is not attached to this account * @then corresponding error code is returned */ - TEST_F(RemoveSignatory, RemoveSignatoryTestNoSuchSignatory) { + TEST_F(RemoveSignatory, NoSuchSignatory) { addAllPerms(); shared_model::interface::types::PubkeyType pk(std::string('5', 32)); ASSERT_TRUE( @@ -1188,7 +1233,7 @@ namespace iroha { * @when trying to remove signatory without permission * @then signatory is not removed */ - TEST_F(RemoveSignatory, InvalidRemoveSignatoryTestWhenNoPerms) { + TEST_F(RemoveSignatory, NoPerms) { shared_model::interface::types::PubkeyType pk(std::string('5', 32)); ASSERT_TRUE( val(execute(buildCommand(TestTransactionBuilder().addSignatory( @@ -1209,10 +1254,11 @@ namespace iroha { /** * @given command - * @when trying to remove signatory from account so it has less than quorum + * @when trying to remove signatory from an account, after which it will + * have signatories less, than its quorum * @then signatory is not removed */ - TEST_F(RemoveSignatory, RemoveSignatoryTestInvalidWhenQuorum) { + TEST_F(RemoveSignatory, SignatoriesLessThanQuorum) { addAllPerms(); shared_model::interface::types::PubkeyType pk(std::string('5', 32)); ASSERT_TRUE( @@ -1257,7 +1303,7 @@ namespace iroha { * @when trying to revoke permission * @then permission is revoked */ - TEST_F(RevokePermission, ValidRevokePermissionTest) { + TEST_F(RevokePermission, Valid) { auto perm = shared_model::interface::permissions::Grantable::kRemoveMySignatory; ASSERT_TRUE(query->hasAccountGrantablePermission( @@ -1292,7 +1338,7 @@ namespace iroha { * @when trying to revoke permission without permission * @then permission is revoked */ - TEST_F(RevokePermission, InvalidRevokePermissionTestWithNoPermission) { + TEST_F(RevokePermission, NoPerms) { auto perm = shared_model::interface::permissions::Grantable::kRemoveMySignatory; auto cmd_result = @@ -1340,7 +1386,7 @@ namespace iroha { * @when trying to set kv * @then kv is set */ - TEST_F(SetAccountDetail, ValidSetAccountDetailTest) { + TEST_F(SetAccountDetail, Valid) { ASSERT_TRUE( val(execute(buildCommand(TestTransactionBuilder().setAccountDetail( account->accountId(), "key", "value"))))); @@ -1354,7 +1400,7 @@ namespace iroha { * @when trying to set kv when has grantable permission * @then kv is set */ - TEST_F(SetAccountDetail, ValidSetAccountDetailTestWhenGrantablePerm) { + TEST_F(SetAccountDetail, ValidGrantablePerm) { auto perm = shared_model::interface::permissions::Grantable::kSetMyAccountDetail; ASSERT_TRUE( @@ -1377,7 +1423,7 @@ namespace iroha { * @when trying to set kv when has role permission * @then kv is set */ - TEST_F(SetAccountDetail, ValidSetAccountDetailTestWhenPerm) { + TEST_F(SetAccountDetail, ValidRolePerm) { addAllPerms(); ASSERT_TRUE( val(execute(buildCommand(TestTransactionBuilder().setAccountDetail( @@ -1394,7 +1440,7 @@ namespace iroha { * @when trying to set kv to non-existing account * @then corresponding error code is returned */ - TEST_F(SetAccountDetail, InvalidSetAccountDetailTestNoAccount) { + TEST_F(SetAccountDetail, NoAccount) { addAllPerms(); auto cmd_result = execute(buildCommand(TestTransactionBuilder().setAccountDetail( @@ -1409,7 +1455,7 @@ namespace iroha { * @when trying to set kv while having no permissions * @then corresponding error code is returned */ - TEST_F(SetAccountDetail, InvalidSetAccountDetailTestNoPerms) { + TEST_F(SetAccountDetail, NoPerms) { auto cmd_result = execute(buildCommand(TestTransactionBuilder().setAccountDetail( account2->accountId(), "key", "value")), @@ -1445,7 +1491,7 @@ namespace iroha { * @when trying to set quorum * @then quorum is set */ - TEST_F(SetQuorum, ValidSetQuorumTestWithRolePerms) { + TEST_F(SetQuorum, Valid) { addAllPerms(); ASSERT_TRUE( val(execute(buildCommand(TestTransactionBuilder().setAccountQuorum( @@ -1457,7 +1503,7 @@ namespace iroha { * @when trying to set quorum * @then quorum is set */ - TEST_F(SetQuorum, ValidSetQuorumTestWithGrantablePerms) { + TEST_F(SetQuorum, ValidGrantablePerms) { ASSERT_TRUE( val(execute(buildCommand(TestTransactionBuilder().createAccount( "id2", domain->domainId(), *pubkey)), @@ -1478,7 +1524,7 @@ namespace iroha { * @when trying to set quorum more than amount of signatories * @then quorum is not set */ - TEST_F(SetQuorum, SetQuorumTestInvalidSignatoriesAmount) { + TEST_F(SetQuorum, LessSignatoriesThanNewQuorum) { addAllPerms(); ASSERT_TRUE( val(execute(buildCommand(TestTransactionBuilder().setAccountQuorum( @@ -1498,7 +1544,7 @@ namespace iroha { * @when trying to set quorum without perms * @then quorum is not set */ - TEST_F(SetQuorum, InvalidSetQuorumTestWithNoPerms) { + TEST_F(SetQuorum, NoPerms) { auto cmd_result = execute(buildCommand( TestTransactionBuilder().setAccountQuorum(account->accountId(), 3))); checkErrorCode(cmd_result, 5); @@ -1547,7 +1593,7 @@ namespace iroha { * @when trying to subtract account asset * @then account asset is successfully subtracted */ - TEST_F(SubtractAccountAssetTest, ValidSubtractAccountAssetTest) { + TEST_F(SubtractAccountAssetTest, Valid) { addAllPerms(); addAsset(); ASSERT_TRUE( @@ -1581,7 +1627,7 @@ namespace iroha { * @when trying to subtract account asset with non-existing asset * @then account asset fails to be subtracted */ - TEST_F(SubtractAccountAssetTest, SubtractAccountAssetTestInvalidAsset) { + TEST_F(SubtractAccountAssetTest, NoAsset) { addAllPerms(); auto cmd_result = execute(buildCommand(TestTransactionBuilder() @@ -1593,9 +1639,9 @@ namespace iroha { /** * @given command * @when trying to add account asset with wrong precision - * @then account asset fails to added + * @then account asset fails to be added */ - TEST_F(SubtractAccountAssetTest, SubtractAccountAssetTestInvalidPrecision) { + TEST_F(SubtractAccountAssetTest, InvalidPrecision) { addAllPerms(); addAsset(); auto cmd_result = @@ -1610,7 +1656,7 @@ namespace iroha { * @when trying to subtract more account asset than account has * @then account asset fails to be subtracted */ - TEST_F(SubtractAccountAssetTest, SubtractAccountAssetTestNotEnoughAsset) { + TEST_F(SubtractAccountAssetTest, NotEnoughAsset) { addAllPerms(); addAsset(); ASSERT_TRUE( @@ -1630,8 +1676,7 @@ namespace iroha { * @when trying to subtract account asset without permissions * @then corresponding error code is returned */ - TEST_F(SubtractAccountAssetTest, - InvalidSubtractAccountAssetTestWhenNoPerms) { + TEST_F(SubtractAccountAssetTest, NoPerms) { addAsset(); ASSERT_TRUE( val(execute(buildCommand(TestTransactionBuilder() @@ -1705,10 +1750,9 @@ namespace iroha { /** * @given command * @when trying to add transfer asset - * @then account asset is successfully transfered + * @then account asset is successfully transferred */ - TEST_F(TransferAccountAssetTest, - ValidTransferAccountAssetTestWhenRolePerms) { + TEST_F(TransferAccountAssetTest, Valid) { addAllPerms(); addAllPerms(account2->accountId(), "all2"); addAsset(); @@ -1746,10 +1790,9 @@ namespace iroha { /** * @given command * @when trying to add transfer asset - * @then account asset is successfully transfered + * @then account asset is successfully transferred */ - TEST_F(TransferAccountAssetTest, - ValidTransferAccountAssetTestWhenGrantablePerms) { + TEST_F(TransferAccountAssetTest, ValidGrantablePerms) { addAllPerms(account2->accountId(), "all2"); addAsset(); auto perm = @@ -1789,9 +1832,9 @@ namespace iroha { /** * @given command * @when trying to transfer asset back and forth with non-existing account - * @then account asset fails to be transfered + * @then account asset fails to be transferred */ - TEST_F(TransferAccountAssetTest, TransferAccountAssetTestInvalidAccount) { + TEST_F(TransferAccountAssetTest, NoAccount) { addAllPerms(); addAllPerms(account2->accountId(), "all2"); addAsset(); @@ -1815,9 +1858,9 @@ namespace iroha { /** * @given command * @when trying to transfer account asset with non-existing asset - * @then account asset fails to be transfered + * @then account asset fails to be transferred */ - TEST_F(TransferAccountAssetTest, TransferAccountAssetTestInvalidAsset) { + TEST_F(TransferAccountAssetTest, NoAsset) { addAllPerms(); addAllPerms(account2->accountId(), "all2"); auto cmd_result = execute(buildCommand( @@ -1832,10 +1875,9 @@ namespace iroha { /** * @given command * @when trying to transfer account asset with no permissions - * @then account asset fails to be transfered + * @then account asset fails to be transferred */ - TEST_F(TransferAccountAssetTest, - TransferAccountAssetTestInvalidNoPermission) { + TEST_F(TransferAccountAssetTest, NoPerms) { auto cmd_result = execute(buildCommand( TestTransactionBuilder().transferAsset(account->accountId(), account2->accountId(), @@ -1848,9 +1890,9 @@ namespace iroha { /** * @given command * @when trying to transfer account asset, but has insufficient amount of it - * @then account asset fails to transfered + * @then account asset fails to be transferred */ - TEST_F(TransferAccountAssetTest, TransferAccountAssetOwerdraftTest) { + TEST_F(TransferAccountAssetTest, Owerdraft) { addAllPerms(); addAllPerms(account2->accountId(), "all2"); addAsset(); @@ -1868,8 +1910,13 @@ namespace iroha { checkErrorCode(cmd_result, 8); } - TEST_F(TransferAccountAssetTest, - TransferAccountAssetInvalidOverflowDestination) { + /** + * @given command + * @when trying to transfer account asset, but final value overflows the + * destination's asset value + * @then account asset fails to be transferred + */ + TEST_F(TransferAccountAssetTest, OverflowDestination) { addAllPerms(); addAllPerms(account2->accountId(), "all2"); addAsset(); From 0179a203443f800cdeb49c0255981bc31fde5b75 Mon Sep 17 00:00:00 2001 From: Akvinikym Date: Mon, 29 Oct 2018 12:32:39 +0300 Subject: [PATCH 07/17] Introduced error codes to SFV Signed-off-by: Akvinikym --- irohad/ametsuchi/impl/temporary_wsv_impl.cpp | 27 ++++++++++--------- .../impl/stateful_validator_impl.cpp | 2 +- .../validation/stateful_validator_common.hpp | 4 +-- 3 files changed, 18 insertions(+), 15 deletions(-) diff --git a/irohad/ametsuchi/impl/temporary_wsv_impl.cpp b/irohad/ametsuchi/impl/temporary_wsv_impl.cpp index 48ebc2e436..bd0d1764d8 100644 --- a/irohad/ametsuchi/impl/temporary_wsv_impl.cpp +++ b/irohad/ametsuchi/impl/temporary_wsv_impl.cpp @@ -54,22 +54,25 @@ namespace iroha { soci::use(boost::size(keys_range), "signatures_count"), soci::use(transaction.creatorAccountId(), "account_id"); } catch (const std::exception &e) { - log_->error(e.what()); - return expected::makeError(validation::CommandError{ - "signatures validation", - (boost::format("database error: %s") % e.what()).str(), - false}); + log_->error( + "Transaction %s failed signatures validation with db error: %s", + transaction.toString(), + e.what()); + // TODO [IR-1816] Akvinikym 29.10.18: substitute error code magic number + // with named constant + return expected::makeError( + validation::CommandError{"signatures validation", 1, false}); } if (signatories_valid and *signatories_valid) { return {}; } else { - return expected::makeError(validation::CommandError{ - "signatures validation", - "possible reasons: no account, number of signatures is less than " - "account quorum, signatures are not a subset of account " - "signatories", - false}); + log_->error("Transaction %s failed signatures validation", + transaction.toString()); + // TODO [IR-1816] Akvinikym 29.10.18: substitute error code magic number + // with named constant + return expected::makeError( + validation::CommandError{"signatures validation", 2, false}); } } @@ -101,7 +104,7 @@ namespace iroha { .match([](expected::Value &) { return true; }, [i, &cmd_error](expected::Error &error) { cmd_error = {error.error.command_name, - error.error.toString(), + error.error.error_code, true, i}; return false; diff --git a/irohad/validation/impl/stateful_validator_impl.cpp b/irohad/validation/impl/stateful_validator_impl.cpp index c707bea940..c5def674e2 100644 --- a/irohad/validation/impl/stateful_validator_impl.cpp +++ b/irohad/validation/impl/stateful_validator_impl.cpp @@ -24,7 +24,7 @@ namespace iroha { * @param temporary_wsv to apply commands on * @param transactions_errors_log to write errors to * @param tx to be checked - * @return empty result, if check is succesfull, command error otherwise + * @return empty result, if check is successful, command error otherwise */ static bool checkTransactions( ametsuchi::TemporaryWsv &temporary_wsv, diff --git a/irohad/validation/stateful_validator_common.hpp b/irohad/validation/stateful_validator_common.hpp index e03b44ec2e..70acb9a14d 100644 --- a/irohad/validation/stateful_validator_common.hpp +++ b/irohad/validation/stateful_validator_common.hpp @@ -26,8 +26,8 @@ namespace iroha { /// Name of the failed command std::string name; - /// Error, with which the command failed - std::string error; + /// Error code, with which the command failed + uint32_t error_code; /// Shows, if transaction has passed initial validation bool tx_passed_initial_validation; From 6a535196053e9c819e83f1e532be5f86230e4873 Mon Sep 17 00:00:00 2001 From: Akvinikym Date: Mon, 29 Oct 2018 13:16:09 +0300 Subject: [PATCH 08/17] Is now built Signed-off-by: Akvinikym --- .../torii/processor/impl/transaction_processor_impl.cpp | 8 ++++---- test/module/iroha-cli/client_test.cpp | 4 ++-- test/module/irohad/simulator/simulator_test.cpp | 4 ++-- .../irohad/torii/processor/transaction_processor_test.cpp | 3 +-- test/module/irohad/torii/torii_service_test.cpp | 5 ++--- test/module/irohad/validation/stateful_validator_test.cpp | 7 +++++-- 6 files changed, 16 insertions(+), 15 deletions(-) diff --git a/irohad/torii/processor/impl/transaction_processor_impl.cpp b/irohad/torii/processor/impl/transaction_processor_impl.cpp index 9ba2b6138f..091cde2e09 100644 --- a/irohad/torii/processor/impl/transaction_processor_impl.cpp +++ b/irohad/torii/processor/impl/transaction_processor_impl.cpp @@ -25,16 +25,16 @@ namespace iroha { if (not tx_error.first.tx_passed_initial_validation) { return (boost::format("Stateful validation error: transaction %s " "did not pass initial verification: " - "checking '%s', error message '%s'") + "checking '%s', error code '%d'") % tx_error.second.hex() % tx_error.first.name - % tx_error.first.error) + % tx_error.first.error_code) .str(); } return (boost::format("Stateful validation error in transaction %s: " "command '%s' with index '%d' did not pass " - "verification with error '%s'") + "verification with error code '%d'") % tx_error.second.hex() % tx_error.first.name - % tx_error.first.index % tx_error.first.error) + % tx_error.first.index % tx_error.first.error_code) .str(); } } // namespace diff --git a/test/module/iroha-cli/client_test.cpp b/test/module/iroha-cli/client_test.cpp index 4f1f68a679..6ce7b3f51b 100644 --- a/test/module/iroha-cli/client_test.cpp +++ b/test/module/iroha-cli/client_test.cpp @@ -299,12 +299,12 @@ TEST_F(ClientServerTest, SendTxWhenStatefulInvalid) { std::make_pair(verified_proposal, iroha::validation::TransactionsErrors{std::make_pair( iroha::validation::CommandError{ - "CommandName", "CommandError", true, 2}, + "CommandName", 2, true, 2}, tx.hash())}))); auto stringified_error = "Stateful validation error in transaction " + tx.hash().hex() + ": command 'CommandName' with " "index '2' did not pass verification with " - "error 'CommandError'"; + "error code '2'"; auto getAnswer = [&]() { return client.getTxStatus(shared_model::crypto::toBinaryString(tx.hash())) diff --git a/test/module/irohad/simulator/simulator_test.cpp b/test/module/irohad/simulator/simulator_test.cpp index be15b16a8e..f4cad05311 100644 --- a/test/module/irohad/simulator/simulator_test.cpp +++ b/test/module/irohad/simulator/simulator_test.cpp @@ -286,9 +286,9 @@ TEST_F(SimulatorTest, RightNumberOfFailedTxs) { .transactions(std::vector{tx}) .build()); auto tx_errors = iroha::validation::TransactionsErrors{ - std::make_pair(validation::CommandError{"SomeCommand", "SomeError", true}, + std::make_pair(validation::CommandError{"SomeCommand", 1, true}, shared_model::crypto::Hash(std::string(32, '0'))), - std::make_pair(validation::CommandError{"SomeCommand", "SomeError", true}, + std::make_pair(validation::CommandError{"SomeCommand", 1, true}, shared_model::crypto::Hash(std::string(32, '0')))}; shared_model::proto::Block block = makeBlock(proposal->height() - 1); diff --git a/test/module/irohad/torii/processor/transaction_processor_test.cpp b/test/module/irohad/torii/processor/transaction_processor_test.cpp index 735ac1dac0..3e30443b61 100644 --- a/test/module/irohad/torii/processor/transaction_processor_test.cpp +++ b/test/module/irohad/torii/processor/transaction_processor_test.cpp @@ -382,8 +382,7 @@ TEST_F(TransactionProcessorTest, TransactionProcessorInvalidTxsTest) { auto txs_errors = iroha::validation::TransactionsErrors{}; for (size_t i = 0; i < invalid_txs.size(); ++i) { txs_errors.push_back(std::make_pair( - iroha::validation::CommandError{ - "SomeCommandName", "SomeCommandError", true, i}, + iroha::validation::CommandError{"SomeCommandName", 1, true, i}, invalid_txs[i].hash())); } verified_prop_notifier.get_subscriber().on_next( diff --git a/test/module/irohad/torii/torii_service_test.cpp b/test/module/irohad/torii/torii_service_test.cpp index 5185596bb9..a6fe95c88f 100644 --- a/test/module/irohad/torii/torii_service_test.cpp +++ b/test/module/irohad/torii/torii_service_test.cpp @@ -328,13 +328,12 @@ TEST_F(ToriiServiceTest, StatusWhenBlocking) { .build()); auto errors = iroha::validation::TransactionsErrors{std::make_pair( iroha::validation::CommandError{ - "FailedCommand", "stateful validation failed", true, 2}, + "FailedCommand", 2, true, 2}, failed_tx_hash)}; auto stringified_error = "Stateful validation error in transaction " + failed_tx_hash.hex() + ": " "command 'FailedCommand' with index '2' " - "did not pass verification with error 'stateful " - "validation failed'"; + "did not pass verification with error code '2'"; verified_prop_notifier_.get_subscriber().on_next( std::make_shared( std::make_pair(verified_proposal, errors))); diff --git a/test/module/irohad/validation/stateful_validator_test.cpp b/test/module/irohad/validation/stateful_validator_test.cpp index c4b438cad7..96811b079c 100644 --- a/test/module/irohad/validation/stateful_validator_test.cpp +++ b/test/module/irohad/validation/stateful_validator_test.cpp @@ -197,13 +197,14 @@ TEST_F(Validator, SomeTxsFail) { .build(); EXPECT_CALL(*temp_wsv_mock, apply(Eq(ByRef(invalid_tx)))) - .WillOnce(Return(iroha::expected::Error({}))); + .WillOnce(Return(iroha::expected::makeError(CommandError{"", 2, false}))); EXPECT_CALL(*temp_wsv_mock, apply(Eq(ByRef(valid_tx)))) .WillRepeatedly(Return(iroha::expected::Value({}))); auto verified_proposal_and_errors = sfv->validate(proposal, *temp_wsv_mock); ASSERT_EQ(verified_proposal_and_errors.first->transactions().size(), 2); ASSERT_EQ(verified_proposal_and_errors.second.size(), 1); + ASSERT_EQ(verified_proposal_and_errors.second[0].first.error_code, 2); } /** @@ -267,7 +268,8 @@ TEST_F(Validator, Batches) { EXPECT_CALL(*temp_wsv_mock, apply(Eq(ByRef(txs[2])))) .WillOnce(Return(iroha::expected::Value({}))); EXPECT_CALL(*temp_wsv_mock, apply(Eq(ByRef(txs[3])))) - .WillOnce(Return(iroha::expected::Error({}))); + .WillOnce( + Return(iroha::expected::makeError(CommandError({"", 2, false})))); EXPECT_CALL(*temp_wsv_mock, apply(Eq(ByRef(txs[5])))) .WillOnce(Return(iroha::expected::Value({}))); EXPECT_CALL(*temp_wsv_mock, apply(Eq(ByRef(txs[6])))) @@ -276,4 +278,5 @@ TEST_F(Validator, Batches) { auto verified_proposal_and_errors = sfv->validate(proposal, *temp_wsv_mock); ASSERT_EQ(verified_proposal_and_errors.first->transactions().size(), 5); ASSERT_EQ(verified_proposal_and_errors.second.size(), 1); + ASSERT_EQ(verified_proposal_and_errors.second[0].first.error_code, 2); } From d4eb53bdbf92303dc94c53b00f0435a26d445178 Mon Sep 17 00:00:00 2001 From: Akvinikym Date: Wed, 31 Oct 2018 11:59:14 +0300 Subject: [PATCH 09/17] Added a separated function for parsing SQL errors and making an error Signed-off-by: Akvinikym --- .../impl/postgres_command_executor.cpp | 123 +++++++++--------- .../ametsuchi/postgres_executor_test.cpp | 109 ++++++++-------- 2 files changed, 115 insertions(+), 117 deletions(-) diff --git a/irohad/ametsuchi/impl/postgres_command_executor.cpp b/irohad/ametsuchi/impl/postgres_command_executor.cpp index 9526804605..f5d83e33ed 100644 --- a/irohad/ametsuchi/impl/postgres_command_executor.cpp +++ b/irohad/ametsuchi/impl/postgres_command_executor.cpp @@ -76,7 +76,7 @@ namespace { } iroha::expected::Error makeCommandError( - std::string command_name, + std::string &&command_name, const iroha::ametsuchi::CommandError::ErrorCodeType code, const std::string &error_extra = "") noexcept { return iroha::expected::makeError(iroha::ametsuchi::CommandError{ @@ -84,70 +84,69 @@ namespace { } /** - * Parse the SQL error to turn it into an error code - * @param command_name of the failed command - * @param error - string error, which SQL gave out - * @return command_error structure + * Match the given substring against given SQL error and form a command error, + * if match was successful + * @param command_name - name of the failed command + * @param error to be matched against + * @param error_code to be inserted into a command error + * @param key - first element to be in an error + * @param to_be_presented - second element to be in an error + * @return optional with command error */ - iroha::ametsuchi::CommandResult parseSqlError( - std::string command_name, const std::string &error) noexcept { - if ((error.find("Key (account_id)=") != std::string::npos - or error.find("Key (permittee_account_id)") != std::string::npos) - and error.find("is not present in table") != std::string::npos) { - // account, which is specified in query, is not found - return makeCommandError(std::move(command_name), 2); - } - - if (error.find("Key (role_id)=") != std::string::npos - and error.find("is not present in table") != std::string::npos) { - // role, which is specified in query, is not found - return makeCommandError(std::move(command_name), 6); - } - - if (error.find("Key (domain_id)=") != std::string::npos - and error.find("is not present in table") != std::string::npos) { - // domain, which is specified in query, is not found - return makeCommandError(std::move(command_name), 7); - } - - if (error.find("Key (asset_id)=") != std::string::npos - and error.find("already exists") != std::string::npos) { - // asset, which is specified in query, already exists and thus cannot be - // created - return makeCommandError(std::move(command_name), 9); - } - - if (error.find("Key (domain_id)=") != std::string::npos - and error.find("already exists") != std::string::npos) { - // domain, which is specified in query, already exists and thus cannot be - // created - return makeCommandError(std::move(command_name), 10); - } - - if (error.find("Key (role_id)=") != std::string::npos - and error.find("already exists") != std::string::npos) { - // role, which is specified in query, already exists and thus cannot be - // created - return makeCommandError(std::move(command_name), 11); - } - - if (error.find("Key (account_id, public_key)") != std::string::npos - and error.find("already exists") != std::string::npos) { - // account already has such signatory attached - return makeCommandError(std::move(command_name), 12); + boost::optional parseSqlError( + std::string command_name, + const std::string &error, + iroha::ametsuchi::CommandError::ErrorCodeType error_code, + const std::string &key, + const std::string &to_be_presented) { + auto errors_matched = error.find(key) != std::string::npos + and error.find(to_be_presented) != std::string::npos; + if (errors_matched) { + return boost::make_optional( + makeCommandError(std::move(command_name), error_code)); } + return {}; + } - if (error.find("Key (account_id)") != std::string::npos - and error.find("already exists") != std::string::npos) { - // account, which is specified in query, already exists and thus cannot be - // created - return makeCommandError(std::move(command_name), 13); - } + /// mapping between pairs of SQL error substrings and related error codes + std::vector> + sql_to_error_code = { + std::make_tuple(2, "Key (account_id)=", "is not present in table"), + std::make_tuple( + 2, "Key (permittee_account_id)", "is not present in table"), + std::make_tuple(6, "Key (role_id)=", "is not present in table"), + std::make_tuple(7, "Key (domain_id)=", "is not present in table"), + std::make_tuple(9, "Key (asset_id)=", "already exists"), + std::make_tuple(10, "Key (domain_id)=", "already exists"), + std::make_tuple(11, "Key (role_id)=", "already exists"), + std::make_tuple( + 12, "Key (account_id, public_key)=", "already exists"), + std::make_tuple(13, "Key (account_id)=", "already exists"), + std::make_tuple( + 14, "Key (default_role)=", "is not present in table")}; - if (error.find("Key (default_role)") != std::string::npos - and error.find("is not present in table") != std::string::npos) { - // default role for the domain is not found - return makeCommandError(std::move(command_name), 14); + /** + * Get an error code from the text SQL error + * @param command_name - name of the failed command + * @param error - string error, which SQL gave out + * @return command_error structure + */ + iroha::ametsuchi::CommandResult getCommandError( + std::string &&command_name, const std::string &error) noexcept { + boost::optional result_opt; + + iroha::ametsuchi::CommandError::ErrorCodeType err_code; + std::string key, to_be_presented; + + for (auto error_tuple : sql_to_error_code) { + std::tie(err_code, key, to_be_presented) = error_tuple; + result_opt = + parseSqlError(command_name, error, err_code, key, to_be_presented); + if (result_opt) { + return *result_opt; + } } // parsing is not successful, return the general error @@ -175,7 +174,7 @@ namespace { } return {}; } catch (const std::exception &e) { - return parseSqlError(std::move(command_name), e.what()); + return getCommandError(std::move(command_name), e.what()); } } diff --git a/test/module/irohad/ametsuchi/postgres_executor_test.cpp b/test/module/irohad/ametsuchi/postgres_executor_test.cpp index f9c92a15fb..59ae9d187a 100644 --- a/test/module/irohad/ametsuchi/postgres_executor_test.cpp +++ b/test/module/irohad/ametsuchi/postgres_executor_test.cpp @@ -100,15 +100,12 @@ namespace iroha { /** * Check that command result contains specific error code - * @param result to be checked + * @param cmd_result to be checked * @param expected_code to be in the result */ - void checkErrorCode(const CommandResult &result, - CommandError::ErrorCodeType expected_code) { - auto error = err(result); - ASSERT_TRUE(error); - ASSERT_EQ(error->error.error_code, expected_code); - } +#define CHECK_ERROR_CODE(cmd_result, expected_code) \ + ASSERT_TRUE(err(cmd_result)); \ + ASSERT_EQ(err(cmd_result)->error.error_code, expected_code); const std::string role = "role"; const std::string another_role = "role2"; @@ -201,7 +198,9 @@ namespace iroha { .creatorAccountId("some@domain")), true, "some@domain"); - checkErrorCode(cmd_result, 2); + // TODO [IR-1816] Akvinikym 29.10.18: replace magic numbers with named + // constants + CHECK_ERROR_CODE(cmd_result, 2); } /** @@ -215,7 +214,7 @@ namespace iroha { .addAssetQuantity(asset_id, "1.0") .creatorAccountId(account->accountId())), true); - checkErrorCode(cmd_result, 3); + CHECK_ERROR_CODE(cmd_result, 3); } /** @@ -239,7 +238,7 @@ namespace iroha { .addAssetQuantity(asset_id, uint256_halfmax) .creatorAccountId(account->accountId())), true); - checkErrorCode(cmd_result, 4); + CHECK_ERROR_CODE(cmd_result, 4); } /** @@ -263,7 +262,7 @@ namespace iroha { execute(buildCommand(TestTransactionBuilder() .addAssetQuantity(asset_id, "1.0") .creatorAccountId(account->accountId()))); - checkErrorCode(cmd_result, 5); + CHECK_ERROR_CODE(cmd_result, 5); } class AddPeer : public CommandExecutorTest { @@ -306,7 +305,7 @@ namespace iroha { TEST_F(AddPeer, NoPerms) { auto cmd_result = execute(buildCommand( TestTransactionBuilder().addPeer(peer->address(), peer->pubkey()))); - checkErrorCode(cmd_result, 5); + CHECK_ERROR_CODE(cmd_result, 5); } class AddSignatory : public CommandExecutorTest { @@ -388,7 +387,7 @@ namespace iroha { account->accountId(), perm)), true, "id2@domain"); - checkErrorCode(cmd_result, 2); + CHECK_ERROR_CODE(cmd_result, 2); } /** @@ -400,7 +399,7 @@ namespace iroha { auto cmd_result = execute(buildCommand(TestTransactionBuilder().addSignatory( account->accountId(), *pubkey))); - checkErrorCode(cmd_result, 5); + CHECK_ERROR_CODE(cmd_result, 5); auto signatories = query->getSignatories(account->accountId()); ASSERT_TRUE(signatories); @@ -423,7 +422,7 @@ namespace iroha { auto cmd_result = execute(buildCommand(TestTransactionBuilder().addSignatory( account->accountId(), *pubkey))); - checkErrorCode(cmd_result, 12); + CHECK_ERROR_CODE(cmd_result, 12); } class AppendRole : public CommandExecutorTest { @@ -515,7 +514,7 @@ namespace iroha { true))); auto cmd_result = execute(buildCommand( TestTransactionBuilder().appendRole("doge@noaccount", another_role))); - checkErrorCode(cmd_result, 2); + CHECK_ERROR_CODE(cmd_result, 2); } /** @@ -530,7 +529,7 @@ namespace iroha { auto cmd_result = execute(buildCommand(TestTransactionBuilder().appendRole( account->accountId(), another_role))); - checkErrorCode(cmd_result, 5); + CHECK_ERROR_CODE(cmd_result, 5); auto roles = query->getAccountRoles(account->accountId()); ASSERT_TRUE(roles); @@ -552,7 +551,7 @@ namespace iroha { auto cmd_result = execute(buildCommand(TestTransactionBuilder().appendRole( account->accountId(), another_role))); - checkErrorCode(cmd_result, 5); + CHECK_ERROR_CODE(cmd_result, 5); } /** @@ -565,7 +564,7 @@ namespace iroha { auto cmd_result = execute(buildCommand(TestTransactionBuilder().appendRole( account->accountId(), another_role))); - checkErrorCode(cmd_result, 6); + CHECK_ERROR_CODE(cmd_result, 6); } class CreateAccount : public CommandExecutorTest { @@ -625,7 +624,7 @@ namespace iroha { auto cmd_result = execute(buildCommand(TestTransactionBuilder().createAccount( "id2", domain->domainId(), *pubkey))); - checkErrorCode(cmd_result, 5); + CHECK_ERROR_CODE(cmd_result, 5); auto acc = query->getAccount(account2->accountId()); ASSERT_FALSE(acc); } @@ -639,7 +638,7 @@ namespace iroha { addAllPerms(); auto cmd_result = execute(buildCommand( TestTransactionBuilder().createAccount("doge", "domain6", *pubkey))); - checkErrorCode(cmd_result, 7); + CHECK_ERROR_CODE(cmd_result, 7); } /** @@ -652,7 +651,7 @@ namespace iroha { auto cmd_result = execute(buildCommand(TestTransactionBuilder().createAccount( "id", domain->domainId(), *pubkey))); - checkErrorCode(cmd_result, 13); + CHECK_ERROR_CODE(cmd_result, 13); } class CreateAsset : public CommandExecutorTest { @@ -720,7 +719,7 @@ namespace iroha { true))); auto cmd_result = execute(buildCommand( TestTransactionBuilder().createAsset("coin", domain->domainId(), 1))); - checkErrorCode(cmd_result, 5); + CHECK_ERROR_CODE(cmd_result, 5); auto ass = query->getAsset(asset->assetId()); ASSERT_FALSE(ass); } @@ -751,7 +750,7 @@ namespace iroha { true))); auto cmd_result = execute(buildCommand( TestTransactionBuilder().createAsset(asset_name, "no_domain", 1))); - checkErrorCode(cmd_result, 7); + CHECK_ERROR_CODE(cmd_result, 7); } /** @@ -782,7 +781,7 @@ namespace iroha { "coin", domain->domainId(), 1))))); auto cmd_result = execute(buildCommand( TestTransactionBuilder().createAsset("coin", domain->domainId(), 1))); - checkErrorCode(cmd_result, 9); + CHECK_ERROR_CODE(cmd_result, 9); } class CreateDomain : public CommandExecutorTest { @@ -830,7 +829,7 @@ namespace iroha { TEST_F(CreateDomain, NoPerms) { auto cmd_result = execute(buildCommand( TestTransactionBuilder().createDomain(domain2->domainId(), role))); - checkErrorCode(cmd_result, 5); + CHECK_ERROR_CODE(cmd_result, 5); auto dom = query->getDomain(domain2->domainId()); ASSERT_FALSE(dom); } @@ -846,7 +845,7 @@ namespace iroha { TestTransactionBuilder().createDomain(domain2->domainId(), role))))); auto cmd_result = execute(buildCommand( TestTransactionBuilder().createDomain(domain2->domainId(), role))); - checkErrorCode(cmd_result, 10); + CHECK_ERROR_CODE(cmd_result, 10); } /** @@ -859,7 +858,7 @@ namespace iroha { auto cmd_result = execute(buildCommand(TestTransactionBuilder().createDomain( domain2->domainId(), another_role))); - checkErrorCode(cmd_result, 14); + CHECK_ERROR_CODE(cmd_result, 14); } class CreateRole : public CommandExecutorTest { @@ -907,7 +906,7 @@ namespace iroha { auto cmd_result = execute(buildCommand(TestTransactionBuilder().createRole( another_role, role_permissions2))); - checkErrorCode(cmd_result, 5); + CHECK_ERROR_CODE(cmd_result, 5); auto rl = query->getRolePermissions(another_role); ASSERT_TRUE(rl); ASSERT_TRUE(rl->none()); @@ -924,7 +923,7 @@ namespace iroha { another_role, role_permissions))))); auto cmd_result = execute(buildCommand( TestTransactionBuilder().createRole(another_role, role_permissions))); - checkErrorCode(cmd_result, 11); + CHECK_ERROR_CODE(cmd_result, 11); } class DetachRole : public CommandExecutorTest { @@ -978,7 +977,7 @@ namespace iroha { addAllPerms(); auto cmd_result = execute(buildCommand( TestTransactionBuilder().detachRole("doge@noaccount", another_role))); - checkErrorCode(cmd_result, 2); + CHECK_ERROR_CODE(cmd_result, 2); } /** @@ -993,7 +992,7 @@ namespace iroha { auto cmd_result = execute(buildCommand(TestTransactionBuilder().detachRole( account->accountId(), another_role))); - checkErrorCode(cmd_result, 3); + CHECK_ERROR_CODE(cmd_result, 3); } /** @@ -1005,7 +1004,7 @@ namespace iroha { auto cmd_result = execute(buildCommand(TestTransactionBuilder().detachRole( account->accountId(), another_role))); - checkErrorCode(cmd_result, 5); + CHECK_ERROR_CODE(cmd_result, 5); auto roles = query->getAccountRoles(account->accountId()); ASSERT_TRUE(roles); ASSERT_TRUE(std::find(roles->begin(), roles->end(), another_role) @@ -1022,7 +1021,7 @@ namespace iroha { auto cmd_result = execute(buildCommand(TestTransactionBuilder().detachRole( account->accountId(), "not_existing_role"))); - checkErrorCode(cmd_result, 6); + CHECK_ERROR_CODE(cmd_result, 6); } class GrantPermission : public CommandExecutorTest { @@ -1077,7 +1076,7 @@ namespace iroha { execute(buildCommand(TestTransactionBuilder() .grantPermission("doge@noaccount", perm) .creatorAccountId(account->accountId()))); - checkErrorCode(cmd_result, 2); + CHECK_ERROR_CODE(cmd_result, 2); } /** @@ -1092,7 +1091,7 @@ namespace iroha { execute(buildCommand(TestTransactionBuilder() .grantPermission(account->accountId(), perm) .creatorAccountId(account->accountId()))); - checkErrorCode(cmd_result, 5); + CHECK_ERROR_CODE(cmd_result, 5); auto has_perm = query->hasAccountGrantablePermission( account->accountId(), account->accountId(), perm); ASSERT_FALSE(has_perm); @@ -1198,7 +1197,7 @@ namespace iroha { auto cmd_result = execute(buildCommand( TestTransactionBuilder().removeSignatory("hello", *pubkey))); - checkErrorCode(cmd_result, 2); + CHECK_ERROR_CODE(cmd_result, 2); } /** @@ -1225,7 +1224,7 @@ namespace iroha { auto cmd_result = execute(buildCommand(TestTransactionBuilder().removeSignatory( account->accountId(), *another_pubkey))); - checkErrorCode(cmd_result, 3); + CHECK_ERROR_CODE(cmd_result, 3); } /** @@ -1242,7 +1241,7 @@ namespace iroha { auto cmd_result = execute(buildCommand(TestTransactionBuilder().removeSignatory( account->accountId(), *pubkey))); - checkErrorCode(cmd_result, 5); + CHECK_ERROR_CODE(cmd_result, 5); auto signatories = query->getSignatories(account->accountId()); ASSERT_TRUE(signatories); @@ -1270,7 +1269,7 @@ namespace iroha { account->accountId(), *pubkey))))); auto cmd_result = execute(buildCommand( TestTransactionBuilder().removeSignatory(account->accountId(), pk))); - checkErrorCode(cmd_result, 8); + CHECK_ERROR_CODE(cmd_result, 8); } class RevokePermission : public CommandExecutorTest { @@ -1345,7 +1344,7 @@ namespace iroha { execute(buildCommand(TestTransactionBuilder() .revokePermission(account->accountId(), perm) .creatorAccountId(account->accountId()))); - checkErrorCode(cmd_result, 5); + CHECK_ERROR_CODE(cmd_result, 5); } class SetAccountDetail : public CommandExecutorTest { @@ -1447,7 +1446,7 @@ namespace iroha { "doge@noaccount", "key", "value")), false, account->accountId()); - checkErrorCode(cmd_result, 2); + CHECK_ERROR_CODE(cmd_result, 2); } /** @@ -1461,7 +1460,7 @@ namespace iroha { account2->accountId(), "key", "value")), false, account->accountId()); - checkErrorCode(cmd_result, 5); + CHECK_ERROR_CODE(cmd_result, 5); auto kv = query->getAccountDetail(account2->accountId()); ASSERT_TRUE(kv); ASSERT_EQ(kv.get(), "{}"); @@ -1536,7 +1535,7 @@ namespace iroha { true))); auto cmd_result = execute(buildCommand( TestTransactionBuilder().setAccountQuorum(account->accountId(), 1))); - checkErrorCode(cmd_result, 4); + CHECK_ERROR_CODE(cmd_result, 4); } /** @@ -1547,7 +1546,7 @@ namespace iroha { TEST_F(SetQuorum, NoPerms) { auto cmd_result = execute(buildCommand( TestTransactionBuilder().setAccountQuorum(account->accountId(), 3))); - checkErrorCode(cmd_result, 5); + CHECK_ERROR_CODE(cmd_result, 5); } class SubtractAccountAssetTest : public CommandExecutorTest { @@ -1633,7 +1632,7 @@ namespace iroha { execute(buildCommand(TestTransactionBuilder() .subtractAssetQuantity(asset_id, "1.0") .creatorAccountId(account->accountId()))); - checkErrorCode(cmd_result, 3); + CHECK_ERROR_CODE(cmd_result, 3); } /** @@ -1648,7 +1647,7 @@ namespace iroha { execute(buildCommand(TestTransactionBuilder() .subtractAssetQuantity(asset_id, "1.0000") .creatorAccountId(account->accountId()))); - checkErrorCode(cmd_result, 3); + CHECK_ERROR_CODE(cmd_result, 3); } /** @@ -1668,7 +1667,7 @@ namespace iroha { execute(buildCommand(TestTransactionBuilder() .subtractAssetQuantity(asset_id, "2.0") .creatorAccountId(account->accountId()))); - checkErrorCode(cmd_result, 4); + CHECK_ERROR_CODE(cmd_result, 4); } /** @@ -1847,12 +1846,12 @@ namespace iroha { buildCommand(TestTransactionBuilder().transferAsset( "some@domain", account2->accountId(), asset_id, "desc", "1.0")), true); - checkErrorCode(cmd_result, 2); + CHECK_ERROR_CODE(cmd_result, 2); cmd_result = execute( buildCommand(TestTransactionBuilder().transferAsset( account->accountId(), "some@domain", asset_id, "desc", "1.0")), true); - checkErrorCode(cmd_result, 3); + CHECK_ERROR_CODE(cmd_result, 3); } /** @@ -1869,7 +1868,7 @@ namespace iroha { asset_id, "desc", "1.0"))); - checkErrorCode(cmd_result, 4); + CHECK_ERROR_CODE(cmd_result, 4); } /** @@ -1884,7 +1883,7 @@ namespace iroha { asset_id, "desc", "1.0"))); - checkErrorCode(cmd_result, 5); + CHECK_ERROR_CODE(cmd_result, 5); } /** @@ -1892,7 +1891,7 @@ namespace iroha { * @when trying to transfer account asset, but has insufficient amount of it * @then account asset fails to be transferred */ - TEST_F(TransferAccountAssetTest, Owerdraft) { + TEST_F(TransferAccountAssetTest, Overdraft) { addAllPerms(); addAllPerms(account2->accountId(), "all2"); addAsset(); @@ -1907,7 +1906,7 @@ namespace iroha { asset_id, "desc", "2.0"))); - checkErrorCode(cmd_result, 8); + CHECK_ERROR_CODE(cmd_result, 8); } /** @@ -1942,7 +1941,7 @@ namespace iroha { "desc", uint256_halfmax)), true); - checkErrorCode(cmd_result, 11); + CHECK_ERROR_CODE(cmd_result, 11); } } // namespace ametsuchi } // namespace iroha From 9734efbab2c635a930ac4cd078604ba5c2ee9246 Mon Sep 17 00:00:00 2001 From: Akvinikym Date: Wed, 31 Oct 2018 14:31:19 +0300 Subject: [PATCH 10/17] Review issues are addressed Signed-off-by: Akvinikym --- .../impl/postgres_command_executor.cpp | 12 +- .../ametsuchi/postgres_executor_test.cpp | 120 +++++++++--------- 2 files changed, 66 insertions(+), 66 deletions(-) diff --git a/irohad/ametsuchi/impl/postgres_command_executor.cpp b/irohad/ametsuchi/impl/postgres_command_executor.cpp index f5d83e33ed..16d7691db1 100644 --- a/irohad/ametsuchi/impl/postgres_command_executor.cpp +++ b/irohad/ametsuchi/impl/postgres_command_executor.cpp @@ -99,6 +99,8 @@ namespace { iroha::ametsuchi::CommandError::ErrorCodeType error_code, const std::string &key, const std::string &to_be_presented) { + // TODO [IR-1830] Akvinikym 31.10.18: make benchmarks to compare exception + // parsing vs nested queries auto errors_matched = error.find(key) != std::string::npos and error.find(to_be_presented) != std::string::npos; if (errors_matched) { @@ -249,6 +251,8 @@ namespace { namespace iroha { namespace ametsuchi { + // TODO [IR-1830] Akvinikym 31.10.18: make benchmarks to compare exception + // parsing vs nested queries const std::string PostgresCommandExecutor::addAssetQuantityBase = R"( PREPARE %s (text, text, int, text) AS WITH has_account AS (SELECT account_id FROM account @@ -354,9 +358,8 @@ namespace iroha { const std::string PostgresCommandExecutor::createAccountBase = R"( PREPARE %s (text, text, text, text) AS - WITH has_domain AS (SELECT * FROM domain WHERE domain_id = $3), - get_domain_default_role AS (SELECT default_role FROM domain - WHERE domain_id = $3), + WITH get_domain_default_role AS (SELECT default_role FROM domain + WHERE domain_id = $3), %s insert_signatory AS ( @@ -399,7 +402,7 @@ namespace iroha { SELECT CASE WHEN EXISTS (SELECT * FROM insert_account_role) THEN 0 %s - WHEN NOT EXISTS (SELECT * FROM has_domain) THEN 7 + WHEN NOT EXISTS (SELECT * FROM get_domain_default_role) THEN 7 ELSE 1 END AS result)"; @@ -881,7 +884,6 @@ namespace iroha { CommandResult PostgresCommandExecutor::operator()( const shared_model::interface::RevokePermission &command) { auto &permittee_account_id = command.accountId(); - auto &account_id = creator_account_id_; auto permission = command.permissionName(); const auto without_perm_str = shared_model::interface::GrantablePermissionSet() diff --git a/test/module/irohad/ametsuchi/postgres_executor_test.cpp b/test/module/irohad/ametsuchi/postgres_executor_test.cpp index 59ae9d187a..18b318bd21 100644 --- a/test/module/irohad/ametsuchi/postgres_executor_test.cpp +++ b/test/module/irohad/ametsuchi/postgres_executor_test.cpp @@ -21,6 +21,7 @@ namespace iroha { using namespace framework::expected; class CommandExecutorTest : public AmetsuchiTest { + // TODO [IR-1831] Akvinikym 31.10.18: rework the CommandExecutorTest public: CommandExecutorTest() { domain = clone( @@ -120,6 +121,11 @@ namespace iroha { std::unique_ptr query; std::unique_ptr executor; + + std::string uint256_halfmax = + "57896044618658097711785492504343953926634992332820282019728792003956" + "5648" + "19966.0"; // 2**255 - 2tra }; class AddAccountAssetTest : public CommandExecutorTest { @@ -161,7 +167,7 @@ namespace iroha { }; /** - * @given command + * @given command * @when trying to add account asset * @then account asset is successfully added */ @@ -204,7 +210,7 @@ namespace iroha { } /** - * @given command + * @given command * @when trying to add account asset with non-existing asset * @then account asset fails to be added */ @@ -218,15 +224,11 @@ namespace iroha { } /** - * @given command + * @given command * @when trying to add account asset that overflows * @then account asset fails to added */ TEST_F(AddAccountAssetTest, Uint256Overflow) { - std::string uint256_halfmax = - "57896044618658097711785492504343953926634992332820282019728792003956" - "5648" - "19966.0"; // 2**255 - 2tra addAsset(); ASSERT_TRUE(val( execute(buildCommand(TestTransactionBuilder() @@ -242,7 +244,7 @@ namespace iroha { } /** - * @given command + * @given command * @when trying to add account asset without permission * @then account asset not added */ @@ -287,7 +289,7 @@ namespace iroha { }; /** - * @given command + * @given command * @when trying to add peer * @then peer is successfully added */ @@ -298,7 +300,7 @@ namespace iroha { } /** - * @given command + * @given command * @when trying to add peer without perms * @then peer is not added */ @@ -331,7 +333,7 @@ namespace iroha { }; /** - * @given command + * @given command * @when trying to add signatory with role permission * @then signatory is successfully added */ @@ -347,7 +349,7 @@ namespace iroha { } /** - * @given command + * @given command * @when trying to add signatory with grantable permission * @then signatory is successfully added */ @@ -391,7 +393,7 @@ namespace iroha { } /** - * @given command + * @given command * @when trying to add signatory without permissions * @then signatory is not added */ @@ -538,7 +540,7 @@ namespace iroha { } /** - * @given command + * @given command * @when trying to append role with perms that creator does not have * @then role is not appended */ @@ -601,7 +603,7 @@ namespace iroha { }; /** - * @given command + * @given command * @when trying to create account * @then account is created */ @@ -616,7 +618,7 @@ namespace iroha { } /** - * @given command + * @given command * @when trying to create account without permission to do so * @then account is not created */ @@ -630,7 +632,7 @@ namespace iroha { } /** - * @given command @and no target domain in ledger + * @given command @and no target domain in ledger * @when trying to create account * @then account is not created */ @@ -665,7 +667,7 @@ namespace iroha { }; /** - * @given command + * @given command * @when trying to create asset * @then asset is created */ @@ -696,7 +698,7 @@ namespace iroha { } /** - * @given command + * @given command * @when trying to create asset without permission * @then asset is not created */ @@ -725,7 +727,7 @@ namespace iroha { } /** - * @given command and no target domain in ledger + * @given command and no target domain in ledger * @when trying to create asset * @then asset is not created */ @@ -808,7 +810,7 @@ namespace iroha { }; /** - * @given command + * @given command * @when trying to create domain * @then domain is created */ @@ -822,7 +824,7 @@ namespace iroha { } /** - * @given command when there is no perms + * @given command when there is no perms * @when trying to create domain * @then domain is not created */ @@ -849,7 +851,7 @@ namespace iroha { } /** - * @given command when there is no default role + * @given command when there is no default role * @when trying to create domain * @then domain is not created */ @@ -882,7 +884,7 @@ namespace iroha { }; /** - * @given command + * @given command * @when trying to create role * @then role is created */ @@ -896,7 +898,7 @@ namespace iroha { } /** - * @given command + * @given command * @when trying to create role when creator doesn't have all permissions * @then role is not created */ @@ -954,7 +956,7 @@ namespace iroha { }; /** - * @given command + * @given command * @when trying to detach role * @then role is detached */ @@ -996,7 +998,7 @@ namespace iroha { } /** - * @given command + * @given command * @when trying to detach role without permission * @then role is detached */ @@ -1048,7 +1050,7 @@ namespace iroha { }; /** - * @given command + * @given command * @when trying to grant permission * @then permission is granted */ @@ -1080,7 +1082,7 @@ namespace iroha { } /** - * @given command + * @given command * @when trying to grant permission without permission * @then permission is not granted */ @@ -1125,7 +1127,7 @@ namespace iroha { }; /** - * @given command + * @given command * @when trying to remove signatory * @then signatory is successfully removed */ @@ -1148,7 +1150,7 @@ namespace iroha { } /** - * @given command + * @given command * @when trying to remove signatory * @then signatory is successfully removed */ @@ -1183,7 +1185,7 @@ namespace iroha { } /** - * @given command + * @given command * @when trying to remove signatory from a non existing account * @then corresponding error code is returned */ @@ -1228,7 +1230,7 @@ namespace iroha { } /** - * @given command + * @given command * @when trying to remove signatory without permission * @then signatory is not removed */ @@ -1252,7 +1254,7 @@ namespace iroha { } /** - * @given command + * @given command * @when trying to remove signatory from an account, after which it will * have signatories less, than its quorum * @then signatory is not removed @@ -1298,7 +1300,7 @@ namespace iroha { }; /** - * @given command + * @given command * @when trying to revoke permission * @then permission is revoked */ @@ -1333,7 +1335,7 @@ namespace iroha { } /** - * @given command + * @given command * @when trying to revoke permission without permission * @then permission is revoked */ @@ -1381,7 +1383,7 @@ namespace iroha { }; /** - * @given command + * @given command * @when trying to set kv * @then kv is set */ @@ -1395,7 +1397,7 @@ namespace iroha { } /** - * @given command + * @given command * @when trying to set kv when has grantable permission * @then kv is set */ @@ -1418,7 +1420,7 @@ namespace iroha { } /** - * @given command + * @given command * @when trying to set kv when has role permission * @then kv is set */ @@ -1435,7 +1437,7 @@ namespace iroha { } /** - * @given command + * @given command * @when trying to set kv to non-existing account * @then corresponding error code is returned */ @@ -1450,7 +1452,7 @@ namespace iroha { } /** - * @given command + * @given command * @when trying to set kv while having no permissions * @then corresponding error code is returned */ @@ -1486,7 +1488,7 @@ namespace iroha { }; /** - * @given command + * @given command * @when trying to set quorum * @then quorum is set */ @@ -1498,7 +1500,7 @@ namespace iroha { } /** - * @given command + * @given command * @when trying to set quorum * @then quorum is set */ @@ -1519,7 +1521,7 @@ namespace iroha { } /** - * @given command + * @given command * @when trying to set quorum more than amount of signatories * @then quorum is not set */ @@ -1539,7 +1541,7 @@ namespace iroha { } /** - * @given command + * @given command * @when trying to set quorum without perms * @then quorum is not set */ @@ -1588,7 +1590,7 @@ namespace iroha { }; /** - * @given command + * @given command * @when trying to subtract account asset * @then account asset is successfully subtracted */ @@ -1622,7 +1624,7 @@ namespace iroha { } /** - * @given command + * @given command * @when trying to subtract account asset with non-existing asset * @then account asset fails to be subtracted */ @@ -1636,7 +1638,7 @@ namespace iroha { } /** - * @given command + * @given command * @when trying to add account asset with wrong precision * @then account asset fails to be added */ @@ -1651,7 +1653,7 @@ namespace iroha { } /** - * @given command + * @given command * @when trying to subtract more account asset than account has * @then account asset fails to be subtracted */ @@ -1671,7 +1673,7 @@ namespace iroha { } /** - * @given command + * @given command * @when trying to subtract account asset without permissions * @then corresponding error code is returned */ @@ -1747,7 +1749,7 @@ namespace iroha { }; /** - * @given command + * @given command * @when trying to add transfer asset * @then account asset is successfully transferred */ @@ -1787,7 +1789,7 @@ namespace iroha { } /** - * @given command + * @given command * @when trying to add transfer asset * @then account asset is successfully transferred */ @@ -1829,7 +1831,7 @@ namespace iroha { } /** - * @given command + * @given command * @when trying to transfer asset back and forth with non-existing account * @then account asset fails to be transferred */ @@ -1855,7 +1857,7 @@ namespace iroha { } /** - * @given command + * @given command * @when trying to transfer account asset with non-existing asset * @then account asset fails to be transferred */ @@ -1872,7 +1874,7 @@ namespace iroha { } /** - * @given command + * @given command * @when trying to transfer account asset with no permissions * @then account asset fails to be transferred */ @@ -1887,7 +1889,7 @@ namespace iroha { } /** - * @given command + * @given command * @when trying to transfer account asset, but has insufficient amount of it * @then account asset fails to be transferred */ @@ -1919,10 +1921,6 @@ namespace iroha { addAllPerms(); addAllPerms(account2->accountId(), "all2"); addAsset(); - std::string uint256_halfmax = - "57896044618658097711785492504343953926634992332820282019728792003956" - "5648" - "19966.0"; ASSERT_TRUE(val( execute(buildCommand(TestTransactionBuilder() .addAssetQuantity(asset_id, uint256_halfmax) @@ -1941,7 +1939,7 @@ namespace iroha { "desc", uint256_halfmax)), true); - CHECK_ERROR_CODE(cmd_result, 11); + CHECK_ERROR_CODE(cmd_result, 15); } } // namespace ametsuchi } // namespace iroha From af8a100d237f743695cc1b22813938fcac92a3e0 Mon Sep 17 00:00:00 2001 From: Akvinikym Date: Wed, 31 Oct 2018 14:48:45 +0300 Subject: [PATCH 11/17] Added ToDos Signed-off-by: Akvinikym --- irohad/ametsuchi/impl/postgres_command_executor.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/irohad/ametsuchi/impl/postgres_command_executor.cpp b/irohad/ametsuchi/impl/postgres_command_executor.cpp index 16d7691db1..27c3b48a21 100644 --- a/irohad/ametsuchi/impl/postgres_command_executor.cpp +++ b/irohad/ametsuchi/impl/postgres_command_executor.cpp @@ -83,6 +83,8 @@ namespace { std::move(command_name), code, error_extra}); } + // TODO [IR-1830] Akvinikym 31.10.18: make benchmarks to compare exception + // parsing vs nested queries /** * Match the given substring against given SQL error and form a command error, * if match was successful @@ -99,8 +101,6 @@ namespace { iroha::ametsuchi::CommandError::ErrorCodeType error_code, const std::string &key, const std::string &to_be_presented) { - // TODO [IR-1830] Akvinikym 31.10.18: make benchmarks to compare exception - // parsing vs nested queries auto errors_matched = error.find(key) != std::string::npos and error.find(to_be_presented) != std::string::npos; if (errors_matched) { @@ -251,8 +251,8 @@ namespace { namespace iroha { namespace ametsuchi { - // TODO [IR-1830] Akvinikym 31.10.18: make benchmarks to compare exception - // parsing vs nested queries + // TODO [IR-1830] Akvinikym 31.10.18: make benchmarks to compare exception + // parsing vs nested queries const std::string PostgresCommandExecutor::addAssetQuantityBase = R"( PREPARE %s (text, text, int, text) AS WITH has_account AS (SELECT account_id FROM account From c2264cd096711e8adbf92adc4699fbd719181d10 Mon Sep 17 00:00:00 2001 From: Akvinikym Date: Wed, 7 Nov 2018 11:00:49 +0300 Subject: [PATCH 12/17] Some after-POW issues Signed-off-by: Akvinikym --- .../impl/postgres_command_executor.cpp | 3 +-- .../ametsuchi/postgres_executor_test.cpp | 18 ------------------ 2 files changed, 1 insertion(+), 20 deletions(-) diff --git a/irohad/ametsuchi/impl/postgres_command_executor.cpp b/irohad/ametsuchi/impl/postgres_command_executor.cpp index 27c3b48a21..bba1f2f787 100644 --- a/irohad/ametsuchi/impl/postgres_command_executor.cpp +++ b/irohad/ametsuchi/impl/postgres_command_executor.cpp @@ -96,7 +96,7 @@ namespace { * @return optional with command error */ boost::optional parseSqlError( - std::string command_name, + std::string &command_name, const std::string &error, iroha::ametsuchi::CommandError::ErrorCodeType error_code, const std::string &key, @@ -291,7 +291,6 @@ namespace iroha { SELECT CASE WHEN EXISTS (SELECT * FROM inserted LIMIT 1) THEN 0 %s - WHEN NOT EXISTS (SELECT * FROM has_account LIMIT 1) THEN 2 WHEN NOT EXISTS (SELECT * FROM has_asset LIMIT 1) THEN 3 WHEN NOT EXISTS (SELECT value FROM new_value WHERE value < 2::decimal ^ (256 - $3) diff --git a/test/module/irohad/ametsuchi/postgres_executor_test.cpp b/test/module/irohad/ametsuchi/postgres_executor_test.cpp index 18b318bd21..0231facaa7 100644 --- a/test/module/irohad/ametsuchi/postgres_executor_test.cpp +++ b/test/module/irohad/ametsuchi/postgres_executor_test.cpp @@ -191,24 +191,6 @@ namespace iroha { ASSERT_EQ("2.0", account_asset.get()->balance().toStringRepr()); } - /** - * @given command - * @when trying to add account asset with non-existing account - * @then account asset fails to be added - */ - TEST_F(AddAccountAssetTest, InvalidAccount) { - addAsset(); - auto cmd_result = - execute(buildCommand(TestTransactionBuilder() - .addAssetQuantity(asset_id, "1.0") - .creatorAccountId("some@domain")), - true, - "some@domain"); - // TODO [IR-1816] Akvinikym 29.10.18: replace magic numbers with named - // constants - CHECK_ERROR_CODE(cmd_result, 2); - } - /** * @given command * @when trying to add account asset with non-existing asset From 6ad3a943a2bf4ee28aa89a88389ac2e4a6d1164e Mon Sep 17 00:00:00 2001 From: Akvinikym Date: Wed, 7 Nov 2018 12:25:38 +0300 Subject: [PATCH 13/17] Error codes has good ordering Signed-off-by: Akvinikym --- .../impl/postgres_command_executor.cpp | 191 +++++---- .../ametsuchi/postgres_executor_test.cpp | 401 +++++++++--------- 2 files changed, 316 insertions(+), 276 deletions(-) diff --git a/irohad/ametsuchi/impl/postgres_command_executor.cpp b/irohad/ametsuchi/impl/postgres_command_executor.cpp index bba1f2f787..b3666bd1d7 100644 --- a/irohad/ametsuchi/impl/postgres_command_executor.cpp +++ b/irohad/ametsuchi/impl/postgres_command_executor.cpp @@ -83,31 +83,67 @@ namespace { std::move(command_name), code, error_extra}); } - // TODO [IR-1830] Akvinikym 31.10.18: make benchmarks to compare exception - // parsing vs nested queries /** - * Match the given substring against given SQL error and form a command error, - * if match was successful - * @param command_name - name of the failed command - * @param error to be matched against - * @param error_code to be inserted into a command error - * @param key - first element to be in an error - * @param to_be_presented - second element to be in an error - * @return optional with command error + * Get a real error code based on the fake one and a command name + * @param fake_error_code - inner error code to be translated into the user's + * one + * @param command_name of the failed command + * @return real error code */ - boost::optional parseSqlError( - std::string &command_name, - const std::string &error, - iroha::ametsuchi::CommandError::ErrorCodeType error_code, - const std::string &key, - const std::string &to_be_presented) { - auto errors_matched = error.find(key) != std::string::npos - and error.find(to_be_presented) != std::string::npos; - if (errors_matched) { - return boost::make_optional( - makeCommandError(std::move(command_name), error_code)); + size_t getRealErrorCode(size_t fake_error_code, + const std::string &command_name) { + switch (fake_error_code) { + case 0: + // we need inner if-s to prevent incorrect behaviour of the system; + // for example, if we are under 0, but command name does not match + // any of those below, it is an internal error + if (command_name == "AddSignatory" or command_name == "AppendRole" + or command_name == "DetachRole" or command_name == "RemoveSignatory" + or command_name == "SetAccountDetail" + or command_name == "SetQuorum") { + return 3; + } + case 1: + if (command_name == "GrantPermission" + or command_name == "RevokePermission") { + return 3; + } + case 2: + if (command_name == "AppendRole") { + return 4; + } else if (command_name == "DetachRole") { + return 5; + } + case 3: + if (command_name == "CreateAccount" or command_name == "CreateAsset") { + return 3; + } + case 4: + if (command_name == "CreateAsset") { + return 4; + } + case 5: + if (command_name == "CreateDomain") { + return 3; + } + case 6: + if (command_name == "CreateRole") { + return 3; + } + case 7: + if (command_name == "AddSignatory") { + return 4; + } + case 8: + if (command_name == "CreateAccount") { + return 4; + } + case 9: + if (command_name == "CreateDomain") { + return 4; + } } - return {}; + return 1; } /// mapping between pairs of SQL error substrings and related error codes @@ -115,20 +151,20 @@ namespace { std::string, std::string>> sql_to_error_code = { - std::make_tuple(2, "Key (account_id)=", "is not present in table"), - std::make_tuple( - 2, "Key (permittee_account_id)", "is not present in table"), - std::make_tuple(6, "Key (role_id)=", "is not present in table"), - std::make_tuple(7, "Key (domain_id)=", "is not present in table"), - std::make_tuple(9, "Key (asset_id)=", "already exists"), - std::make_tuple(10, "Key (domain_id)=", "already exists"), - std::make_tuple(11, "Key (role_id)=", "already exists"), - std::make_tuple( - 12, "Key (account_id, public_key)=", "already exists"), - std::make_tuple(13, "Key (account_id)=", "already exists"), + std::make_tuple(0, "Key (account_id)=", "is not present in table"), std::make_tuple( - 14, "Key (default_role)=", "is not present in table")}; + 1, "Key (permittee_account_id)", "is not present in table"), + std::make_tuple(2, "Key (role_id)=", "is not present in table"), + std::make_tuple(3, "Key (domain_id)=", "is not present in table"), + std::make_tuple(4, "Key (asset_id)=", "already exists"), + std::make_tuple(5, "Key (domain_id)=", "already exists"), + std::make_tuple(6, "Key (role_id)=", "already exists"), + std::make_tuple(7, "Key (account_id, public_key)=", "already exists"), + std::make_tuple(8, "Key (account_id)=", "already exists"), + std::make_tuple(9, "Key (default_role)=", "is not present in table")}; + // TODO [IR-1830] Akvinikym 31.10.18: make benchmarks to compare exception + // parsing vs nested queries /** * Get an error code from the text SQL error * @param command_name - name of the failed command @@ -137,20 +173,21 @@ namespace { */ iroha::ametsuchi::CommandResult getCommandError( std::string &&command_name, const std::string &error) noexcept { - boost::optional result_opt; - iroha::ametsuchi::CommandError::ErrorCodeType err_code; std::string key, to_be_presented; + bool errors_matched; + // go through mapping of SQL errors and related error codes for (auto error_tuple : sql_to_error_code) { std::tie(err_code, key, to_be_presented) = error_tuple; - result_opt = - parseSqlError(command_name, error, err_code, key, to_be_presented); - if (result_opt) { - return *result_opt; + errors_matched = error.find(key) != std::string::npos + and error.find(to_be_presented) != std::string::npos; + if (errors_matched) { + return makeCommandError(std::move(command_name), + getRealErrorCode(err_code, command_name), + error); } } - // parsing is not successful, return the general error return makeCommandError(std::move(command_name), 1, error); } @@ -350,7 +387,7 @@ namespace iroha { ) SELECT CASE WHEN EXISTS (SELECT * FROM inserted) THEN 0 - WHEN NOT EXISTS (SELECT * FROM role_exists) THEN 6 + WHEN NOT EXISTS (SELECT * FROM role_exists) THEN 4 %s ELSE 1 END AS result)"; @@ -401,7 +438,7 @@ namespace iroha { SELECT CASE WHEN EXISTS (SELECT * FROM insert_account_role) THEN 0 %s - WHEN NOT EXISTS (SELECT * FROM get_domain_default_role) THEN 7 + WHEN NOT EXISTS (SELECT * FROM get_domain_default_role) THEN 3 ELSE 1 END AS result)"; @@ -452,7 +489,7 @@ namespace iroha { SELECT CASE WHEN EXISTS (SELECT * FROM insert_role_permissions) THEN 0 %s - WHEN EXISTS (SELECT * FROM role WHERE role_id = $2) THEN 5 + WHEN EXISTS (SELECT * FROM role WHERE role_id = $2) THEN 2 ELSE 1 END AS result)"; @@ -469,11 +506,11 @@ namespace iroha { ) SELECT CASE WHEN EXISTS (SELECT * FROM deleted) THEN 0 WHEN NOT EXISTS (SELECT * FROM account - WHERE account_id = $2) THEN 2 + WHERE account_id = $2) THEN 3 WHEN NOT EXISTS (SELECT * FROM role - WHERE role_id = $3) THEN 6 + WHERE role_id = $3) THEN 5 WHEN NOT EXISTS (SELECT * FROM account_has_roles - WHERE account_id=$2 AND role_id=$3) THEN 3 + WHERE account_id=$2 AND role_id=$3) THEN 4 %s ELSE 1 END AS result)"; @@ -555,7 +592,7 @@ namespace iroha { SELECT CASE WHEN EXISTS (SELECT * FROM inserted) THEN 0 %s WHEN NOT EXISTS - (SELECT * FROM account WHERE account_id=$2) THEN 2 + (SELECT * FROM account WHERE account_id=$2) THEN 3 ELSE 1 END AS result)"; const std::string PostgresCommandExecutor::setQuorumBase = R"( @@ -689,14 +726,14 @@ namespace iroha { SELECT CASE WHEN EXISTS (SELECT * FROM insert_dest LIMIT 1) THEN 0 %s - WHEN NOT EXISTS (SELECT * FROM has_dest_account LIMIT 1) THEN 3 - WHEN NOT EXISTS (SELECT * FROM has_src_account LIMIT 1) THEN 2 - WHEN NOT EXISTS (SELECT * FROM has_asset LIMIT 1) THEN 4 + WHEN NOT EXISTS (SELECT * FROM has_dest_account LIMIT 1) THEN 4 + WHEN NOT EXISTS (SELECT * FROM has_src_account LIMIT 1) THEN 3 + WHEN NOT EXISTS (SELECT * FROM has_asset LIMIT 1) THEN 5 WHEN NOT EXISTS (SELECT value FROM new_src_value - WHERE value >= 0 LIMIT 1) THEN 8 + WHERE value >= 0 LIMIT 1) THEN 6 WHEN NOT EXISTS (SELECT value FROM new_dest_value WHERE value < 2::decimal ^ (256 - $5) - LIMIT 1) THEN 15 + LIMIT 1) THEN 7 ELSE 1 END AS result)"; @@ -985,7 +1022,7 @@ namespace iroha { "$1")) .str(), "AND (SELECT * from has_perm)", - "WHEN NOT (SELECT * from has_perm) THEN 5"}}); + "WHEN NOT (SELECT * from has_perm) THEN 2"}}); statements.push_back( {"addPeer", @@ -995,7 +1032,7 @@ namespace iroha { shared_model::interface::permissions::Role::kAddPeer, "$1")) .str(), "WHERE (SELECT * FROM has_perm)", - "WHEN NOT (SELECT * from has_perm) THEN 5"}}); + "WHEN NOT (SELECT * from has_perm) THEN 2"}}); statements.push_back( {"addSignatory", @@ -1011,7 +1048,7 @@ namespace iroha { .str(), " WHERE (SELECT * FROM has_perm)", " AND (SELECT * FROM has_perm)", - "WHEN NOT (SELECT * from has_perm) THEN 5"}}); + "WHEN NOT (SELECT * from has_perm) THEN 2"}}); const auto bits = shared_model::interface::RolePermissionSet::size(); const auto grantable_bits = @@ -1047,9 +1084,9 @@ namespace iroha { (SELECT * FROM account_has_role_permissions) AND (SELECT * FROM has_perm))", R"( - WHEN NOT EXISTS (SELECT * FROM account_roles) THEN 5 - WHEN NOT (SELECT * FROM account_has_role_permissions) THEN 5 - WHEN NOT (SELECT * FROM has_perm) THEN 5)"}}); + WHEN NOT EXISTS (SELECT * FROM account_roles) THEN 2 + WHEN NOT (SELECT * FROM account_has_role_permissions) THEN 2 + WHEN NOT (SELECT * FROM has_perm) THEN 2)"}}); statements.push_back( {"createAccount", @@ -1061,7 +1098,7 @@ namespace iroha { "$1")) .str(), R"(AND (SELECT * FROM has_perm))", - R"(WHEN NOT (SELECT * FROM has_perm) THEN 5)"}}); + R"(WHEN NOT (SELECT * FROM has_perm) THEN 2)"}}); statements.push_back( {"createAsset", @@ -1073,7 +1110,7 @@ namespace iroha { "$1")) .str(), R"(WHERE (SELECT * FROM has_perm))", - R"(WHEN NOT (SELECT * FROM has_perm) THEN 5)"}}); + R"(WHEN NOT (SELECT * FROM has_perm) THEN 2)"}}); statements.push_back( {"createDomain", @@ -1085,7 +1122,7 @@ namespace iroha { "$1")) .str(), R"(WHERE (SELECT * FROM has_perm))", - R"(WHEN NOT (SELECT * FROM has_perm) THEN 5)"}}); + R"(WHEN NOT (SELECT * FROM has_perm) THEN 2)"}}); statements.push_back( {"createRole", @@ -1106,8 +1143,8 @@ namespace iroha { R"(WHERE (SELECT * FROM account_has_role_permissions) AND (SELECT * FROM has_perm))", R"(WHEN NOT (SELECT * FROM - account_has_role_permissions) THEN 5 - WHEN NOT (SELECT * FROM has_perm) THEN 5)"}}); + account_has_role_permissions) THEN 2 + WHEN NOT (SELECT * FROM has_perm) THEN 2)"}}); statements.push_back( {"detachRole", @@ -1119,7 +1156,7 @@ namespace iroha { "$1")) .str(), R"(AND (SELECT * FROM has_perm))", - R"(WHEN NOT (SELECT * FROM has_perm) THEN 5)"}}); + R"(WHEN NOT (SELECT * FROM has_perm) THEN 2)"}}); statements.push_back({"grantPermission", grantPermissionBase, @@ -1131,7 +1168,7 @@ namespace iroha { % bits) .str(), R"( WHERE (SELECT * FROM has_perm))", - R"(WHEN NOT (SELECT * FROM has_perm) THEN 5)"}}); + R"(WHEN NOT (SELECT * FROM has_perm) THEN 2)"}}); statements.push_back( {"removeSignatory", @@ -1168,10 +1205,10 @@ namespace iroha { AND EXISTS (SELECT * FROM check_account_signatories) )", R"( - WHEN NOT EXISTS (SELECT * FROM get_account) THEN 2 - WHEN NOT (SELECT * FROM has_perm) THEN 5 - WHEN NOT EXISTS (SELECT * FROM get_signatory) THEN 3 - WHEN NOT EXISTS (SELECT * FROM check_account_signatories) THEN 8 + WHEN NOT EXISTS (SELECT * FROM get_account) THEN 3 + WHEN NOT (SELECT * FROM has_perm) THEN 2 + WHEN NOT EXISTS (SELECT * FROM get_signatory) THEN 4 + WHEN NOT EXISTS (SELECT * FROM check_account_signatories) THEN 5 )"}}); statements.push_back({"revokePermission", @@ -1184,7 +1221,7 @@ namespace iroha { % grantable_bits) .str(), R"( AND (SELECT * FROM has_perm))", - R"( WHEN NOT (SELECT * FROM has_perm) THEN 5 )"}}); + R"( WHEN NOT (SELECT * FROM has_perm) THEN 2 )"}}); statements.push_back( {"setAccountDetail", @@ -1208,7 +1245,7 @@ namespace iroha { "$2")) .str(), R"( AND (SELECT * FROM has_perm))", - R"( WHEN NOT (SELECT * FROM has_perm) THEN 5 )"}}); + R"( WHEN NOT (SELECT * FROM has_perm) THEN 2 )"}}); statements.push_back( {"setQuorum", @@ -1236,9 +1273,9 @@ namespace iroha { AND EXISTS (SELECT * FROM check_account_signatories) AND (SELECT * FROM has_perm))", R"( - WHEN NOT (SELECT * FROM has_perm) THEN 5 - WHEN NOT EXISTS (SELECT * FROM get_signatories) THEN 3 - WHEN NOT EXISTS (SELECT * FROM check_account_signatories) THEN 4 + WHEN NOT (SELECT * FROM has_perm) THEN 2 + WHEN NOT EXISTS (SELECT * FROM get_signatories) THEN 4 + WHEN NOT EXISTS (SELECT * FROM check_account_signatories) THEN 5 )"}}); statements.push_back( @@ -1251,7 +1288,7 @@ namespace iroha { "$1")) .str(), R"( AND (SELECT * FROM has_perm))", - R"( WHEN NOT (SELECT * FROM has_perm) THEN 5 )"}}); + R"( WHEN NOT (SELECT * FROM has_perm) THEN 2 )"}}); statements.push_back( {"transferAsset", @@ -1286,7 +1323,7 @@ namespace iroha { .str(), R"( AND (SELECT * FROM has_perm))", R"( AND (SELECT * FROM has_perm))", - R"( WHEN NOT (SELECT * FROM has_perm) THEN 5 )"}}); + R"( WHEN NOT (SELECT * FROM has_perm) THEN 2 )"}}); for (const auto &st : statements) { prepareStatement(sql, st); diff --git a/test/module/irohad/ametsuchi/postgres_executor_test.cpp b/test/module/irohad/ametsuchi/postgres_executor_test.cpp index 0231facaa7..748ebcdc41 100644 --- a/test/module/irohad/ametsuchi/postgres_executor_test.cpp +++ b/test/module/irohad/ametsuchi/postgres_executor_test.cpp @@ -191,6 +191,30 @@ namespace iroha { ASSERT_EQ("2.0", account_asset.get()->balance().toStringRepr()); } + /** + * @given command + * @when trying to add account asset without permission + * @then account asset not added + */ + TEST_F(AddAccountAssetTest, NoPerms) { + addAsset(); + ASSERT_TRUE( + val(execute(buildCommand(TestTransactionBuilder() + .addAssetQuantity(asset_id, "1.0") + .creatorAccountId(account->accountId())), + true))); + auto account_asset = + query->getAccountAsset(account->accountId(), asset_id); + ASSERT_TRUE(account_asset); + ASSERT_EQ("1.0", account_asset.get()->balance().toStringRepr()); + + auto cmd_result = + execute(buildCommand(TestTransactionBuilder() + .addAssetQuantity(asset_id, "1.0") + .creatorAccountId(account->accountId()))); + CHECK_ERROR_CODE(cmd_result, 2); + } + /** * @given command * @when trying to add account asset with non-existing asset @@ -225,30 +249,6 @@ namespace iroha { CHECK_ERROR_CODE(cmd_result, 4); } - /** - * @given command - * @when trying to add account asset without permission - * @then account asset not added - */ - TEST_F(AddAccountAssetTest, NoPerms) { - addAsset(); - ASSERT_TRUE( - val(execute(buildCommand(TestTransactionBuilder() - .addAssetQuantity(asset_id, "1.0") - .creatorAccountId(account->accountId())), - true))); - auto account_asset = - query->getAccountAsset(account->accountId(), asset_id); - ASSERT_TRUE(account_asset); - ASSERT_EQ("1.0", account_asset.get()->balance().toStringRepr()); - - auto cmd_result = - execute(buildCommand(TestTransactionBuilder() - .addAssetQuantity(asset_id, "1.0") - .creatorAccountId(account->accountId()))); - CHECK_ERROR_CODE(cmd_result, 5); - } - class AddPeer : public CommandExecutorTest { public: void SetUp() override { @@ -289,7 +289,7 @@ namespace iroha { TEST_F(AddPeer, NoPerms) { auto cmd_result = execute(buildCommand( TestTransactionBuilder().addPeer(peer->address(), peer->pubkey()))); - CHECK_ERROR_CODE(cmd_result, 5); + CHECK_ERROR_CODE(cmd_result, 2); } class AddSignatory : public CommandExecutorTest { @@ -358,22 +358,6 @@ namespace iroha { != signatories->end()); } - /** - * @given command - * @when trying to add signatory to non-existing account - * @then signatory is not added - */ - TEST_F(AddSignatory, NoAccount) { - auto perm = - shared_model::interface::permissions::Grantable::kAddMySignatory; - auto cmd_result = - execute(buildCommand(TestTransactionBuilder().grantPermission( - account->accountId(), perm)), - true, - "id2@domain"); - CHECK_ERROR_CODE(cmd_result, 2); - } - /** * @given command * @when trying to add signatory without permissions @@ -383,7 +367,7 @@ namespace iroha { auto cmd_result = execute(buildCommand(TestTransactionBuilder().addSignatory( account->accountId(), *pubkey))); - CHECK_ERROR_CODE(cmd_result, 5); + CHECK_ERROR_CODE(cmd_result, 2); auto signatories = query->getSignatories(account->accountId()); ASSERT_TRUE(signatories); @@ -391,6 +375,22 @@ namespace iroha { == signatories->end()); } + /** + * @given command + * @when trying to add signatory to non-existing account + * @then signatory is not added + */ + TEST_F(AddSignatory, NoAccount) { + auto perm = + shared_model::interface::permissions::Grantable::kAddMySignatory; + auto cmd_result = + execute(buildCommand(TestTransactionBuilder().grantPermission( + account->accountId(), perm)), + true, + "id2@domain"); + CHECK_ERROR_CODE(cmd_result, 3); + } + /** * @given command * @when successfully adding signatory to the account @and trying to add the @@ -406,7 +406,7 @@ namespace iroha { auto cmd_result = execute(buildCommand(TestTransactionBuilder().addSignatory( account->accountId(), *pubkey))); - CHECK_ERROR_CODE(cmd_result, 12); + CHECK_ERROR_CODE(cmd_result, 4); } class AppendRole : public CommandExecutorTest { @@ -486,21 +486,6 @@ namespace iroha { != roles->end()); } - /** - * @given command - * @when trying to append role to non-existing account - * @then role is not appended - */ - TEST_F(AppendRole, NoAccount) { - addAllPerms(); - ASSERT_TRUE(val(execute( - buildCommand(TestTransactionBuilder().createRole(another_role, {})), - true))); - auto cmd_result = execute(buildCommand( - TestTransactionBuilder().appendRole("doge@noaccount", another_role))); - CHECK_ERROR_CODE(cmd_result, 2); - } - /** * @given command * @when trying to append role having no permission to do so @@ -513,7 +498,7 @@ namespace iroha { auto cmd_result = execute(buildCommand(TestTransactionBuilder().appendRole( account->accountId(), another_role))); - CHECK_ERROR_CODE(cmd_result, 5); + CHECK_ERROR_CODE(cmd_result, 2); auto roles = query->getAccountRoles(account->accountId()); ASSERT_TRUE(roles); @@ -535,7 +520,22 @@ namespace iroha { auto cmd_result = execute(buildCommand(TestTransactionBuilder().appendRole( account->accountId(), another_role))); - CHECK_ERROR_CODE(cmd_result, 5); + CHECK_ERROR_CODE(cmd_result, 2); + } + + /** + * @given command + * @when trying to append role to non-existing account + * @then role is not appended + */ + TEST_F(AppendRole, NoAccount) { + addAllPerms(); + ASSERT_TRUE(val(execute( + buildCommand(TestTransactionBuilder().createRole(another_role, {})), + true))); + auto cmd_result = execute(buildCommand( + TestTransactionBuilder().appendRole("doge@noaccount", another_role))); + CHECK_ERROR_CODE(cmd_result, 3); } /** @@ -548,7 +548,7 @@ namespace iroha { auto cmd_result = execute(buildCommand(TestTransactionBuilder().appendRole( account->accountId(), another_role))); - CHECK_ERROR_CODE(cmd_result, 6); + CHECK_ERROR_CODE(cmd_result, 4); } class CreateAccount : public CommandExecutorTest { @@ -608,7 +608,7 @@ namespace iroha { auto cmd_result = execute(buildCommand(TestTransactionBuilder().createAccount( "id2", domain->domainId(), *pubkey))); - CHECK_ERROR_CODE(cmd_result, 5); + CHECK_ERROR_CODE(cmd_result, 2); auto acc = query->getAccount(account2->accountId()); ASSERT_FALSE(acc); } @@ -622,7 +622,7 @@ namespace iroha { addAllPerms(); auto cmd_result = execute(buildCommand( TestTransactionBuilder().createAccount("doge", "domain6", *pubkey))); - CHECK_ERROR_CODE(cmd_result, 7); + CHECK_ERROR_CODE(cmd_result, 3); } /** @@ -635,7 +635,7 @@ namespace iroha { auto cmd_result = execute(buildCommand(TestTransactionBuilder().createAccount( "id", domain->domainId(), *pubkey))); - CHECK_ERROR_CODE(cmd_result, 13); + CHECK_ERROR_CODE(cmd_result, 4); } class CreateAsset : public CommandExecutorTest { @@ -703,7 +703,7 @@ namespace iroha { true))); auto cmd_result = execute(buildCommand( TestTransactionBuilder().createAsset("coin", domain->domainId(), 1))); - CHECK_ERROR_CODE(cmd_result, 5); + CHECK_ERROR_CODE(cmd_result, 2); auto ass = query->getAsset(asset->assetId()); ASSERT_FALSE(ass); } @@ -734,7 +734,7 @@ namespace iroha { true))); auto cmd_result = execute(buildCommand( TestTransactionBuilder().createAsset(asset_name, "no_domain", 1))); - CHECK_ERROR_CODE(cmd_result, 7); + CHECK_ERROR_CODE(cmd_result, 3); } /** @@ -765,7 +765,7 @@ namespace iroha { "coin", domain->domainId(), 1))))); auto cmd_result = execute(buildCommand( TestTransactionBuilder().createAsset("coin", domain->domainId(), 1))); - CHECK_ERROR_CODE(cmd_result, 9); + CHECK_ERROR_CODE(cmd_result, 4); } class CreateDomain : public CommandExecutorTest { @@ -813,7 +813,7 @@ namespace iroha { TEST_F(CreateDomain, NoPerms) { auto cmd_result = execute(buildCommand( TestTransactionBuilder().createDomain(domain2->domainId(), role))); - CHECK_ERROR_CODE(cmd_result, 5); + CHECK_ERROR_CODE(cmd_result, 2); auto dom = query->getDomain(domain2->domainId()); ASSERT_FALSE(dom); } @@ -829,7 +829,7 @@ namespace iroha { TestTransactionBuilder().createDomain(domain2->domainId(), role))))); auto cmd_result = execute(buildCommand( TestTransactionBuilder().createDomain(domain2->domainId(), role))); - CHECK_ERROR_CODE(cmd_result, 10); + CHECK_ERROR_CODE(cmd_result, 3); } /** @@ -842,7 +842,7 @@ namespace iroha { auto cmd_result = execute(buildCommand(TestTransactionBuilder().createDomain( domain2->domainId(), another_role))); - CHECK_ERROR_CODE(cmd_result, 14); + CHECK_ERROR_CODE(cmd_result, 4); } class CreateRole : public CommandExecutorTest { @@ -890,7 +890,7 @@ namespace iroha { auto cmd_result = execute(buildCommand(TestTransactionBuilder().createRole( another_role, role_permissions2))); - CHECK_ERROR_CODE(cmd_result, 5); + CHECK_ERROR_CODE(cmd_result, 2); auto rl = query->getRolePermissions(another_role); ASSERT_TRUE(rl); ASSERT_TRUE(rl->none()); @@ -907,7 +907,7 @@ namespace iroha { another_role, role_permissions))))); auto cmd_result = execute(buildCommand( TestTransactionBuilder().createRole(another_role, role_permissions))); - CHECK_ERROR_CODE(cmd_result, 11); + CHECK_ERROR_CODE(cmd_result, 3); } class DetachRole : public CommandExecutorTest { @@ -952,6 +952,22 @@ namespace iroha { == roles->end()); } + /** + * @given command + * @when trying to detach role without permission + * @then role is detached + */ + TEST_F(DetachRole, NoPerms) { + auto cmd_result = + execute(buildCommand(TestTransactionBuilder().detachRole( + account->accountId(), another_role))); + CHECK_ERROR_CODE(cmd_result, 2); + auto roles = query->getAccountRoles(account->accountId()); + ASSERT_TRUE(roles); + ASSERT_TRUE(std::find(roles->begin(), roles->end(), another_role) + != roles->end()); + } + /** * @given command * @when trying to detach role from non-existing account @@ -961,7 +977,7 @@ namespace iroha { addAllPerms(); auto cmd_result = execute(buildCommand( TestTransactionBuilder().detachRole("doge@noaccount", another_role))); - CHECK_ERROR_CODE(cmd_result, 2); + CHECK_ERROR_CODE(cmd_result, 3); } /** @@ -976,23 +992,7 @@ namespace iroha { auto cmd_result = execute(buildCommand(TestTransactionBuilder().detachRole( account->accountId(), another_role))); - CHECK_ERROR_CODE(cmd_result, 3); - } - - /** - * @given command - * @when trying to detach role without permission - * @then role is detached - */ - TEST_F(DetachRole, NoPerms) { - auto cmd_result = - execute(buildCommand(TestTransactionBuilder().detachRole( - account->accountId(), another_role))); - CHECK_ERROR_CODE(cmd_result, 5); - auto roles = query->getAccountRoles(account->accountId()); - ASSERT_TRUE(roles); - ASSERT_TRUE(std::find(roles->begin(), roles->end(), another_role) - != roles->end()); + CHECK_ERROR_CODE(cmd_result, 4); } /** @@ -1005,7 +1005,7 @@ namespace iroha { auto cmd_result = execute(buildCommand(TestTransactionBuilder().detachRole( account->accountId(), "not_existing_role"))); - CHECK_ERROR_CODE(cmd_result, 6); + CHECK_ERROR_CODE(cmd_result, 5); } class GrantPermission : public CommandExecutorTest { @@ -1050,35 +1050,35 @@ namespace iroha { /** * @given command - * @when trying to grant permission to non-existent account - * @then corresponding error code is returned + * @when trying to grant permission without permission + * @then permission is not granted */ - TEST_F(GrantPermission, NoAccount) { - addAllPerms(); + TEST_F(GrantPermission, NoPerms) { auto perm = shared_model::interface::permissions::Grantable::kSetMyQuorum; auto cmd_result = + execute(buildCommand(TestTransactionBuilder() - .grantPermission("doge@noaccount", perm) + .grantPermission(account->accountId(), perm) .creatorAccountId(account->accountId()))); CHECK_ERROR_CODE(cmd_result, 2); + auto has_perm = query->hasAccountGrantablePermission( + account->accountId(), account->accountId(), perm); + ASSERT_FALSE(has_perm); } /** * @given command - * @when trying to grant permission without permission - * @then permission is not granted + * @when trying to grant permission to non-existent account + * @then corresponding error code is returned */ - TEST_F(GrantPermission, NoPerms) { + TEST_F(GrantPermission, NoAccount) { + addAllPerms(); auto perm = shared_model::interface::permissions::Grantable::kSetMyQuorum; auto cmd_result = - execute(buildCommand(TestTransactionBuilder() - .grantPermission(account->accountId(), perm) + .grantPermission("doge@noaccount", perm) .creatorAccountId(account->accountId()))); - CHECK_ERROR_CODE(cmd_result, 5); - auto has_perm = query->hasAccountGrantablePermission( - account->accountId(), account->accountId(), perm); - ASSERT_FALSE(has_perm); + CHECK_ERROR_CODE(cmd_result, 3); } class RemoveSignatory : public CommandExecutorTest { @@ -1166,6 +1166,30 @@ namespace iroha { == signatories->end()); } + /** + * @given command + * @when trying to remove signatory without permission + * @then signatory is not removed + */ + TEST_F(RemoveSignatory, NoPerms) { + shared_model::interface::types::PubkeyType pk(std::string('5', 32)); + ASSERT_TRUE( + val(execute(buildCommand(TestTransactionBuilder().addSignatory( + account->accountId(), pk)), + true))); + auto cmd_result = + execute(buildCommand(TestTransactionBuilder().removeSignatory( + account->accountId(), *pubkey))); + CHECK_ERROR_CODE(cmd_result, 2); + + auto signatories = query->getSignatories(account->accountId()); + ASSERT_TRUE(signatories); + ASSERT_TRUE(std::find(signatories->begin(), signatories->end(), *pubkey) + != signatories->end()); + ASSERT_TRUE(std::find(signatories->begin(), signatories->end(), pk) + != signatories->end()); + } + /** * @given command * @when trying to remove signatory from a non existing account @@ -1181,7 +1205,7 @@ namespace iroha { auto cmd_result = execute(buildCommand( TestTransactionBuilder().removeSignatory("hello", *pubkey))); - CHECK_ERROR_CODE(cmd_result, 2); + CHECK_ERROR_CODE(cmd_result, 3); } /** @@ -1208,31 +1232,7 @@ namespace iroha { auto cmd_result = execute(buildCommand(TestTransactionBuilder().removeSignatory( account->accountId(), *another_pubkey))); - CHECK_ERROR_CODE(cmd_result, 3); - } - - /** - * @given command - * @when trying to remove signatory without permission - * @then signatory is not removed - */ - TEST_F(RemoveSignatory, NoPerms) { - shared_model::interface::types::PubkeyType pk(std::string('5', 32)); - ASSERT_TRUE( - val(execute(buildCommand(TestTransactionBuilder().addSignatory( - account->accountId(), pk)), - true))); - auto cmd_result = - execute(buildCommand(TestTransactionBuilder().removeSignatory( - account->accountId(), *pubkey))); - CHECK_ERROR_CODE(cmd_result, 5); - - auto signatories = query->getSignatories(account->accountId()); - ASSERT_TRUE(signatories); - ASSERT_TRUE(std::find(signatories->begin(), signatories->end(), *pubkey) - != signatories->end()); - ASSERT_TRUE(std::find(signatories->begin(), signatories->end(), pk) - != signatories->end()); + CHECK_ERROR_CODE(cmd_result, 4); } /** @@ -1253,7 +1253,7 @@ namespace iroha { account->accountId(), *pubkey))))); auto cmd_result = execute(buildCommand( TestTransactionBuilder().removeSignatory(account->accountId(), pk))); - CHECK_ERROR_CODE(cmd_result, 8); + CHECK_ERROR_CODE(cmd_result, 5); } class RevokePermission : public CommandExecutorTest { @@ -1328,7 +1328,7 @@ namespace iroha { execute(buildCommand(TestTransactionBuilder() .revokePermission(account->accountId(), perm) .creatorAccountId(account->accountId()))); - CHECK_ERROR_CODE(cmd_result, 5); + CHECK_ERROR_CODE(cmd_result, 2); } class SetAccountDetail : public CommandExecutorTest { @@ -1420,34 +1420,34 @@ namespace iroha { /** * @given command - * @when trying to set kv to non-existing account + * @when trying to set kv while having no permissions * @then corresponding error code is returned */ - TEST_F(SetAccountDetail, NoAccount) { - addAllPerms(); + TEST_F(SetAccountDetail, NoPerms) { auto cmd_result = execute(buildCommand(TestTransactionBuilder().setAccountDetail( - "doge@noaccount", "key", "value")), + account2->accountId(), "key", "value")), false, account->accountId()); CHECK_ERROR_CODE(cmd_result, 2); + auto kv = query->getAccountDetail(account2->accountId()); + ASSERT_TRUE(kv); + ASSERT_EQ(kv.get(), "{}"); } /** * @given command - * @when trying to set kv while having no permissions + * @when trying to set kv to non-existing account * @then corresponding error code is returned */ - TEST_F(SetAccountDetail, NoPerms) { + TEST_F(SetAccountDetail, NoAccount) { + addAllPerms(); auto cmd_result = execute(buildCommand(TestTransactionBuilder().setAccountDetail( - account2->accountId(), "key", "value")), + "doge@noaccount", "key", "value")), false, account->accountId()); - CHECK_ERROR_CODE(cmd_result, 5); - auto kv = query->getAccountDetail(account2->accountId()); - ASSERT_TRUE(kv); - ASSERT_EQ(kv.get(), "{}"); + CHECK_ERROR_CODE(cmd_result, 3); } class SetQuorum : public CommandExecutorTest { @@ -1502,6 +1502,17 @@ namespace iroha { TestTransactionBuilder().setAccountQuorum("id2@domain", 3))))); } + /** + * @given command + * @when trying to set quorum without perms + * @then quorum is not set + */ + TEST_F(SetQuorum, NoPerms) { + auto cmd_result = execute(buildCommand( + TestTransactionBuilder().setAccountQuorum(account->accountId(), 3))); + CHECK_ERROR_CODE(cmd_result, 2); + } + /** * @given command * @when trying to set quorum more than amount of signatories @@ -1519,17 +1530,6 @@ namespace iroha { true))); auto cmd_result = execute(buildCommand( TestTransactionBuilder().setAccountQuorum(account->accountId(), 1))); - CHECK_ERROR_CODE(cmd_result, 4); - } - - /** - * @given command - * @when trying to set quorum without perms - * @then quorum is not set - */ - TEST_F(SetQuorum, NoPerms) { - auto cmd_result = execute(buildCommand( - TestTransactionBuilder().setAccountQuorum(account->accountId(), 3))); CHECK_ERROR_CODE(cmd_result, 5); } @@ -1605,6 +1605,34 @@ namespace iroha { ASSERT_EQ("1.0", account_asset.get()->balance().toStringRepr()); } + /** + * @given command + * @when trying to subtract account asset without permissions + * @then corresponding error code is returned + */ + TEST_F(SubtractAccountAssetTest, NoPerms) { + addAsset(); + ASSERT_TRUE( + val(execute(buildCommand(TestTransactionBuilder() + .addAssetQuantity(asset_id, "1.0") + .creatorAccountId(account->accountId())), + true))); + auto account_asset = + query->getAccountAsset(account->accountId(), asset_id); + ASSERT_TRUE(account_asset); + ASSERT_EQ("1.0", account_asset.get()->balance().toStringRepr()); + + auto cmd_result = + execute(buildCommand(TestTransactionBuilder() + .subtractAssetQuantity(asset_id, "1.0") + .creatorAccountId(account->accountId()))); + CHECK_ERROR_CODE(cmd_result, 2); + + account_asset = query->getAccountAsset(account->accountId(), asset_id); + ASSERT_TRUE(account_asset); + ASSERT_EQ("1.0", account_asset.get()->balance().toStringRepr()); + } + /** * @given command * @when trying to subtract account asset with non-existing asset @@ -1654,31 +1682,6 @@ namespace iroha { CHECK_ERROR_CODE(cmd_result, 4); } - /** - * @given command - * @when trying to subtract account asset without permissions - * @then corresponding error code is returned - */ - TEST_F(SubtractAccountAssetTest, NoPerms) { - addAsset(); - ASSERT_TRUE( - val(execute(buildCommand(TestTransactionBuilder() - .addAssetQuantity(asset_id, "1.0") - .creatorAccountId(account->accountId())), - true))); - auto account_asset = - query->getAccountAsset(account->accountId(), asset_id); - ASSERT_TRUE(account_asset); - ASSERT_EQ("1.0", account_asset.get()->balance().toStringRepr()); - ASSERT_TRUE(err( - execute(buildCommand(TestTransactionBuilder() - .subtractAssetQuantity(asset_id, "1.0") - .creatorAccountId(account->accountId()))))); - account_asset = query->getAccountAsset(account->accountId(), asset_id); - ASSERT_TRUE(account_asset); - ASSERT_EQ("1.0", account_asset.get()->balance().toStringRepr()); - } - class TransferAccountAssetTest : public CommandExecutorTest { void SetUp() override { CommandExecutorTest::SetUp(); @@ -1812,6 +1815,21 @@ namespace iroha { ASSERT_EQ("1.0", account_asset.get()->balance().toStringRepr()); } + /** + * @given command + * @when trying to transfer account asset with no permissions + * @then account asset fails to be transferred + */ + TEST_F(TransferAccountAssetTest, NoPerms) { + auto cmd_result = execute(buildCommand( + TestTransactionBuilder().transferAsset(account->accountId(), + account2->accountId(), + asset_id, + "desc", + "1.0"))); + CHECK_ERROR_CODE(cmd_result, 2); + } + /** * @given command * @when trying to transfer asset back and forth with non-existing account @@ -1830,12 +1848,12 @@ namespace iroha { buildCommand(TestTransactionBuilder().transferAsset( "some@domain", account2->accountId(), asset_id, "desc", "1.0")), true); - CHECK_ERROR_CODE(cmd_result, 2); + CHECK_ERROR_CODE(cmd_result, 3); cmd_result = execute( buildCommand(TestTransactionBuilder().transferAsset( account->accountId(), "some@domain", asset_id, "desc", "1.0")), true); - CHECK_ERROR_CODE(cmd_result, 3); + CHECK_ERROR_CODE(cmd_result, 4); } /** @@ -1846,21 +1864,6 @@ namespace iroha { TEST_F(TransferAccountAssetTest, NoAsset) { addAllPerms(); addAllPerms(account2->accountId(), "all2"); - auto cmd_result = execute(buildCommand( - TestTransactionBuilder().transferAsset(account->accountId(), - account2->accountId(), - asset_id, - "desc", - "1.0"))); - CHECK_ERROR_CODE(cmd_result, 4); - } - - /** - * @given command - * @when trying to transfer account asset with no permissions - * @then account asset fails to be transferred - */ - TEST_F(TransferAccountAssetTest, NoPerms) { auto cmd_result = execute(buildCommand( TestTransactionBuilder().transferAsset(account->accountId(), account2->accountId(), @@ -1890,7 +1893,7 @@ namespace iroha { asset_id, "desc", "2.0"))); - CHECK_ERROR_CODE(cmd_result, 8); + CHECK_ERROR_CODE(cmd_result, 6); } /** @@ -1921,7 +1924,7 @@ namespace iroha { "desc", uint256_halfmax)), true); - CHECK_ERROR_CODE(cmd_result, 15); + CHECK_ERROR_CODE(cmd_result, 7); } } // namespace ametsuchi } // namespace iroha From ef391cb506118986e159fa5010f1fcf6a6177028 Mon Sep 17 00:00:00 2001 From: Akvinikym Date: Wed, 7 Nov 2018 12:43:32 +0300 Subject: [PATCH 14/17] More fixes Signed-off-by: Akvinikym --- .../ametsuchi/impl/mutable_storage_impl.cpp | 7 +--- .../ametsuchi/impl/mutable_storage_impl.hpp | 5 +-- .../impl/postgres_command_executor.cpp | 41 +------------------ .../impl/postgres_command_executor.hpp | 21 +--------- irohad/ametsuchi/impl/storage_impl.cpp | 6 +-- irohad/ametsuchi/impl/temporary_wsv_impl.cpp | 7 +--- irohad/ametsuchi/impl/temporary_wsv_impl.hpp | 5 +-- .../ametsuchi/postgres_executor_test.cpp | 3 +- 8 files changed, 12 insertions(+), 83 deletions(-) diff --git a/irohad/ametsuchi/impl/mutable_storage_impl.cpp b/irohad/ametsuchi/impl/mutable_storage_impl.cpp index 152b53e48a..01b09006b1 100644 --- a/irohad/ametsuchi/impl/mutable_storage_impl.cpp +++ b/irohad/ametsuchi/impl/mutable_storage_impl.cpp @@ -19,16 +19,13 @@ namespace iroha { MutableStorageImpl::MutableStorageImpl( shared_model::interface::types::HashType top_hash, std::unique_ptr sql, - std::shared_ptr factory, - std::shared_ptr - perm_converter) + std::shared_ptr factory) : top_hash_(top_hash), sql_(std::move(sql)), peer_query_(std::make_unique( std::make_shared(*sql_, std::move(factory)))), block_index_(std::make_unique(*sql_)), - command_executor_(std::make_shared( - *sql_, std::move(perm_converter))), + command_executor_(std::make_shared(*sql_)), committed(false), log_(logger::log("MutableStorage")) { *sql_ << "BEGIN"; diff --git a/irohad/ametsuchi/impl/mutable_storage_impl.hpp b/irohad/ametsuchi/impl/mutable_storage_impl.hpp index a877bdc0c0..3b53cbbb97 100644 --- a/irohad/ametsuchi/impl/mutable_storage_impl.hpp +++ b/irohad/ametsuchi/impl/mutable_storage_impl.hpp @@ -13,7 +13,6 @@ #include #include "ametsuchi/command_executor.hpp" #include "interfaces/common_objects/common_objects_factory.hpp" -#include "interfaces/permission_to_string.hpp" #include "logger/logger.hpp" namespace iroha { @@ -28,9 +27,7 @@ namespace iroha { shared_model::interface::types::HashType top_hash, std::unique_ptr sql, std::shared_ptr - factory, - std::shared_ptr - perm_converter); + factory); bool apply(const shared_model::interface::Block &block) override; diff --git a/irohad/ametsuchi/impl/postgres_command_executor.cpp b/irohad/ametsuchi/impl/postgres_command_executor.cpp index 68912d2840..d75d984d22 100644 --- a/irohad/ametsuchi/impl/postgres_command_executor.cpp +++ b/irohad/ametsuchi/impl/postgres_command_executor.cpp @@ -742,13 +742,8 @@ namespace iroha { .str(); } - PostgresCommandExecutor::PostgresCommandExecutor( - soci::session &sql, - std::shared_ptr - perm_converter) - : sql_(sql), - do_validation_(true), - perm_converter_(std::move(perm_converter)) {} + PostgresCommandExecutor::PostgresCommandExecutor(soci::session &sql) + : sql_(sql), do_validation_(true) {} void PostgresCommandExecutor::setCreatorAccountId( const shared_model::interface::types::AccountIdType @@ -1334,37 +1329,5 @@ namespace iroha { } }; - std::string PostgresCommandExecutor::missRolePerm( - shared_model::interface::types::AccountIdType account, - shared_model::interface::permissions::Role perm) { - return (boost::format("command validation failed: account %s" - " does not have permission %s (role)") - % account % perm_converter_->toString(perm)) - .str(); - } - - std::string PostgresCommandExecutor::missGrantablePerm( - shared_model::interface::types::AccountIdType account, - shared_model::interface::types::AccountIdType permittee, - shared_model::interface::permissions::Grantable perm) { - return (boost::format( - "command validation failed: account %s" - " does not have permission %s (grantable) for account %s") - % account % perm_converter_->toString(perm) % permittee) - .str(); - } - - std::string PostgresCommandExecutor::missRoleOrGrantablePerm( - shared_model::interface::types::AccountIdType account, - shared_model::interface::types::AccountIdType permittee, - shared_model::interface::permissions::Role role_perm, - shared_model::interface::permissions::Grantable grantable_perm) { - return (boost::format("command validation failed: account %s" - " does not have permission %s (role)" - " and permission %s (grantable) for account %s") - % account % perm_converter_->toString(role_perm) - % perm_converter_->toString(grantable_perm) % permittee) - .str(); - } } // namespace ametsuchi } // namespace iroha diff --git a/irohad/ametsuchi/impl/postgres_command_executor.hpp b/irohad/ametsuchi/impl/postgres_command_executor.hpp index 80f1b4d4f6..4656e9fddd 100644 --- a/irohad/ametsuchi/impl/postgres_command_executor.hpp +++ b/irohad/ametsuchi/impl/postgres_command_executor.hpp @@ -8,17 +8,13 @@ #include "ametsuchi/command_executor.hpp" #include "ametsuchi/impl/soci_utils.hpp" -#include "interfaces/permission_to_string.hpp" namespace iroha { namespace ametsuchi { class PostgresCommandExecutor : public CommandExecutor { public: - PostgresCommandExecutor( - soci::session &transaction, - std::shared_ptr - perm_converter); + PostgresCommandExecutor(soci::session &transaction); void setCreatorAccountId( const shared_model::interface::types::AccountIdType @@ -82,21 +78,6 @@ namespace iroha { bool do_validation_; shared_model::interface::types::AccountIdType creator_account_id_; - std::shared_ptr - perm_converter_; - - std::string missRolePerm( - shared_model::interface::types::AccountIdType account, - shared_model::interface::permissions::Role perm); - std::string missGrantablePerm( - shared_model::interface::types::AccountIdType account, - shared_model::interface::types::AccountIdType permittee, - shared_model::interface::permissions::Grantable perm); - std::string missRoleOrGrantablePerm( - shared_model::interface::types::AccountIdType account, - shared_model::interface::types::AccountIdType permittee, - shared_model::interface::permissions::Role role_perm, - shared_model::interface::permissions::Grantable grantable_perm); // 14.09.18 nickaleks: IR-1708 Load SQL from separate files static const std::string addAssetQuantityBase; diff --git a/irohad/ametsuchi/impl/storage_impl.cpp b/irohad/ametsuchi/impl/storage_impl.cpp index 6380aca036..8968d171dd 100644 --- a/irohad/ametsuchi/impl/storage_impl.cpp +++ b/irohad/ametsuchi/impl/storage_impl.cpp @@ -74,8 +74,7 @@ namespace iroha { auto sql = std::make_unique(*connection_); return expected::makeValue>( - std::make_unique( - std::move(sql), factory_, perm_converter_)); + std::make_unique(std::move(sql), factory_)); } expected::Result, std::string> @@ -100,8 +99,7 @@ namespace iroha { return shared_model::interface::types::HashType(""); }), std::move(sql), - factory_, - perm_converter_)); + factory_)); } boost::optional> StorageImpl::createPeerQuery() diff --git a/irohad/ametsuchi/impl/temporary_wsv_impl.cpp b/irohad/ametsuchi/impl/temporary_wsv_impl.cpp index e57335cd8e..48ebc2e436 100644 --- a/irohad/ametsuchi/impl/temporary_wsv_impl.cpp +++ b/irohad/ametsuchi/impl/temporary_wsv_impl.cpp @@ -15,12 +15,9 @@ namespace iroha { namespace ametsuchi { TemporaryWsvImpl::TemporaryWsvImpl( std::unique_ptr sql, - std::shared_ptr factory, - std::shared_ptr - perm_converter) + std::shared_ptr factory) : sql_(std::move(sql)), - command_executor_(std::make_unique( - *sql_, std::move(perm_converter))), + command_executor_(std::make_unique(*sql_)), log_(logger::log("TemporaryWSV")) { *sql_ << "BEGIN"; } diff --git a/irohad/ametsuchi/impl/temporary_wsv_impl.hpp b/irohad/ametsuchi/impl/temporary_wsv_impl.hpp index 54beb6ec44..8a666ad78d 100644 --- a/irohad/ametsuchi/impl/temporary_wsv_impl.hpp +++ b/irohad/ametsuchi/impl/temporary_wsv_impl.hpp @@ -11,7 +11,6 @@ #include #include "ametsuchi/command_executor.hpp" #include "interfaces/common_objects/common_objects_factory.hpp" -#include "interfaces/permission_to_string.hpp" #include "logger/logger.hpp" namespace iroha { @@ -36,9 +35,7 @@ namespace iroha { TemporaryWsvImpl( std::unique_ptr sql, std::shared_ptr - factory, - std::shared_ptr - perm_converter); + factory); expected::Result apply( const shared_model::interface::Transaction &transaction) override; diff --git a/test/module/irohad/ametsuchi/postgres_executor_test.cpp b/test/module/irohad/ametsuchi/postgres_executor_test.cpp index 9e82aefdbb..748ebcdc41 100644 --- a/test/module/irohad/ametsuchi/postgres_executor_test.cpp +++ b/test/module/irohad/ametsuchi/postgres_executor_test.cpp @@ -50,8 +50,7 @@ namespace iroha { shared_model::validation::FieldValidator>>(); query = std::make_unique(*sql, factory); PostgresCommandExecutor::prepareStatements(*sql); - executor = - std::make_unique(*sql, perm_converter_); + executor = std::make_unique(*sql); *sql << init_; } From 10213b78409dcc02053c7082e77234c4d1c9421a Mon Sep 17 00:00:00 2001 From: Akvinikym Date: Wed, 7 Nov 2018 12:55:11 +0300 Subject: [PATCH 15/17] Build fix Signed-off-by: Akvinikym --- test/module/irohad/ametsuchi/postgres_query_executor_test.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/module/irohad/ametsuchi/postgres_query_executor_test.cpp b/test/module/irohad/ametsuchi/postgres_query_executor_test.cpp index f2acd165a1..b694ccb34e 100644 --- a/test/module/irohad/ametsuchi/postgres_query_executor_test.cpp +++ b/test/module/irohad/ametsuchi/postgres_query_executor_test.cpp @@ -78,7 +78,7 @@ namespace iroha { query_executor = storage; PostgresCommandExecutor::prepareStatements(*sql); executor = - std::make_unique(*sql, perm_converter_); + std::make_unique(*sql); pending_txs_storage = std::make_shared(); auto result = execute(buildCommand(TestTransactionBuilder().createRole( From c4407e4e44df1919abf8e8835a4f86c0fd6e0ecd Mon Sep 17 00:00:00 2001 From: Akvinikym Date: Wed, 7 Nov 2018 12:59:20 +0300 Subject: [PATCH 16/17] And more Signed-off-by: Akvinikym --- irohad/ametsuchi/impl/postgres_command_executor.cpp | 10 +++++----- .../irohad/ametsuchi/postgres_executor_test.cpp | 12 +----------- 2 files changed, 6 insertions(+), 16 deletions(-) diff --git a/irohad/ametsuchi/impl/postgres_command_executor.cpp b/irohad/ametsuchi/impl/postgres_command_executor.cpp index d75d984d22..772f20aab8 100644 --- a/irohad/ametsuchi/impl/postgres_command_executor.cpp +++ b/irohad/ametsuchi/impl/postgres_command_executor.cpp @@ -146,10 +146,10 @@ namespace { } /// mapping between pairs of SQL error substrings and related error codes - std::vector> - sql_to_error_code = { + const std::vector> + kSqlToErrorCode = { std::make_tuple(0, "Key (account_id)=", "is not present in table"), std::make_tuple( 1, "Key (permittee_account_id)", "is not present in table"), @@ -177,7 +177,7 @@ namespace { bool errors_matched; // go through mapping of SQL errors and related error codes - for (auto error_tuple : sql_to_error_code) { + for (auto error_tuple : kSqlToErrorCode) { std::tie(err_code, key, to_be_presented) = error_tuple; errors_matched = error.find(key) != std::string::npos and error.find(to_be_presented) != std::string::npos; diff --git a/test/module/irohad/ametsuchi/postgres_executor_test.cpp b/test/module/irohad/ametsuchi/postgres_executor_test.cpp index 748ebcdc41..9b70b42676 100644 --- a/test/module/irohad/ametsuchi/postgres_executor_test.cpp +++ b/test/module/irohad/ametsuchi/postgres_executor_test.cpp @@ -125,7 +125,7 @@ namespace iroha { std::string uint256_halfmax = "57896044618658097711785492504343953926634992332820282019728792003956" "5648" - "19966.0"; // 2**255 - 2tra + "19966.0"; // 2**255 }; class AddAccountAssetTest : public CommandExecutorTest { @@ -723,11 +723,6 @@ namespace iroha { val(execute(buildCommand(TestTransactionBuilder().createDomain( domain->domainId(), role)), true))); - auto asset = clone(TestAccountAssetBuilder() - .domainId(domain->domainId()) - .assetId(asset_id) - .precision(1) - .build()); ASSERT_TRUE( val(execute(buildCommand(TestTransactionBuilder().createAccount( "id", domain->domainId(), *pubkey)), @@ -752,11 +747,6 @@ namespace iroha { val(execute(buildCommand(TestTransactionBuilder().createDomain( domain->domainId(), role)), true))); - auto asset = clone(TestAccountAssetBuilder() - .domainId(domain->domainId()) - .assetId(asset_id) - .precision(1) - .build()); ASSERT_TRUE( val(execute(buildCommand(TestTransactionBuilder().createAccount( "id", domain->domainId(), *pubkey)), From 47c55e7806b13ec7d8ffe32a071ddd7175f9ba3f Mon Sep 17 00:00:00 2001 From: Akvinikym Date: Thu, 8 Nov 2018 10:44:48 +0300 Subject: [PATCH 17/17] Refactored switch-case Signed-off-by: Akvinikym --- .../impl/postgres_command_executor.cpp | 146 +++++++++--------- .../ametsuchi/postgres_executor_test.cpp | 16 -- 2 files changed, 69 insertions(+), 93 deletions(-) diff --git a/irohad/ametsuchi/impl/postgres_command_executor.cpp b/irohad/ametsuchi/impl/postgres_command_executor.cpp index 772f20aab8..8bf8c1bc65 100644 --- a/irohad/ametsuchi/impl/postgres_command_executor.cpp +++ b/irohad/ametsuchi/impl/postgres_command_executor.cpp @@ -82,6 +82,52 @@ namespace { std::move(command_name), code, error_extra}); } + /// mapping between pairs of SQL error substrings and related fake error + /// codes, which are indices in this collection + const std::vector> kSqlToFakeErrorCode = + {std::make_tuple("Key (account_id)=", "is not present in table"), + std::make_tuple("Key (permittee_account_id)", "is not present in table"), + std::make_tuple("Key (role_id)=", "is not present in table"), + std::make_tuple("Key (domain_id)=", "is not present in table"), + std::make_tuple("Key (asset_id)=", "already exists"), + std::make_tuple("Key (domain_id)=", "already exists"), + std::make_tuple("Key (role_id)=", "already exists"), + std::make_tuple("Key (account_id, public_key)=", "already exists"), + std::make_tuple("Key (account_id)=", "already exists"), + std::make_tuple("Key (default_role)=", "is not present in table")}; + + /// mapping between command name, fake error code and related real error code + const std::map> kCmdNameToErrorCode{ + std::make_pair( + "AddSignatory", + std::map{std::make_pair(0, 3), std::make_pair(7, 4)}), + std::make_pair( + "AppendRole", + std::map{std::make_pair(0, 3), std::make_pair(2, 4)}), + std::make_pair( + "DetachRole", + std::map{std::make_pair(0, 3), std::make_pair(2, 5)}), + std::make_pair("RemoveSignatory", + std::map{std::make_pair(0, 3)}), + std::make_pair("SetAccountDetail", + std::map{std::make_pair(0, 3)}), + std::make_pair("SetQuorum", std::map{std::make_pair(0, 3)}), + std::make_pair("GrantPermission", + std::map{std::make_pair(1, 3)}), + std::make_pair("RevokePermission", + std::map{std::make_pair(1, 3)}), + std::make_pair( + "CreateAccount", + std::map{std::make_pair(3, 3), std::make_pair(8, 4)}), + std::make_pair( + "CreateAsset", + std::map{std::make_pair(3, 3), std::make_pair(4, 4)}), + std::make_pair( + "CreateDomain", + std::map{std::make_pair(5, 3), std::make_pair(9, 4)}), + std::make_pair("CreateRole", std::map{std::make_pair(6, 3)}), + std::make_pair("AddSignatory", std::map{std::make_pair(7, 4)})}; + /** * Get a real error code based on the fake one and a command name * @param fake_error_code - inner error code to be translated into the user's @@ -89,78 +135,20 @@ namespace { * @param command_name of the failed command * @return real error code */ - size_t getRealErrorCode(size_t fake_error_code, - const std::string &command_name) { - switch (fake_error_code) { - case 0: - // we need inner if-s to prevent incorrect behaviour of the system; - // for example, if we are under 0, but command name does not match - // any of those below, it is an internal error - if (command_name == "AddSignatory" or command_name == "AppendRole" - or command_name == "DetachRole" or command_name == "RemoveSignatory" - or command_name == "SetAccountDetail" - or command_name == "SetQuorum") { - return 3; - } - case 1: - if (command_name == "GrantPermission" - or command_name == "RevokePermission") { - return 3; - } - case 2: - if (command_name == "AppendRole") { - return 4; - } else if (command_name == "DetachRole") { - return 5; - } - case 3: - if (command_name == "CreateAccount" or command_name == "CreateAsset") { - return 3; - } - case 4: - if (command_name == "CreateAsset") { - return 4; - } - case 5: - if (command_name == "CreateDomain") { - return 3; - } - case 6: - if (command_name == "CreateRole") { - return 3; - } - case 7: - if (command_name == "AddSignatory") { - return 4; - } - case 8: - if (command_name == "CreateAccount") { - return 4; - } - case 9: - if (command_name == "CreateDomain") { - return 4; - } + boost::optional + getRealErrorCode(size_t fake_error_code, const std::string &command_name) { + auto fake_to_real_code = kCmdNameToErrorCode.find(command_name); + if (fake_to_real_code == kCmdNameToErrorCode.end()) { + return {}; } - return 1; - } - /// mapping between pairs of SQL error substrings and related error codes - const std::vector> - kSqlToErrorCode = { - std::make_tuple(0, "Key (account_id)=", "is not present in table"), - std::make_tuple( - 1, "Key (permittee_account_id)", "is not present in table"), - std::make_tuple(2, "Key (role_id)=", "is not present in table"), - std::make_tuple(3, "Key (domain_id)=", "is not present in table"), - std::make_tuple(4, "Key (asset_id)=", "already exists"), - std::make_tuple(5, "Key (domain_id)=", "already exists"), - std::make_tuple(6, "Key (role_id)=", "already exists"), - std::make_tuple(7, "Key (account_id, public_key)=", "already exists"), - std::make_tuple(8, "Key (account_id)=", "already exists"), - std::make_tuple(9, "Key (default_role)=", "is not present in table")}; + auto real_code = fake_to_real_code->second.find(fake_error_code); + if (real_code == fake_to_real_code->second.end()) { + return {}; + } + + return real_code->second; + } // TODO [IR-1830] Akvinikym 31.10.18: make benchmarks to compare exception // parsing vs nested queries @@ -172,19 +160,23 @@ namespace { */ iroha::ametsuchi::CommandResult getCommandError( std::string &&command_name, const std::string &error) noexcept { - iroha::ametsuchi::CommandError::ErrorCodeType err_code; std::string key, to_be_presented; bool errors_matched; - // go through mapping of SQL errors and related error codes - for (auto error_tuple : kSqlToErrorCode) { - std::tie(err_code, key, to_be_presented) = error_tuple; + // go through mapping of SQL errors and get index of the current error - it + // is "fake" error code + for (size_t fakeErrorCode = 0; fakeErrorCode < kSqlToFakeErrorCode.size(); + ++fakeErrorCode) { + std::tie(key, to_be_presented) = kSqlToFakeErrorCode[fakeErrorCode]; errors_matched = error.find(key) != std::string::npos and error.find(to_be_presented) != std::string::npos; if (errors_matched) { - return makeCommandError(std::move(command_name), - getRealErrorCode(err_code, command_name), - error); + if (auto real_error_code = + getRealErrorCode(fakeErrorCode, command_name)) { + return makeCommandError( + std::move(command_name), *real_error_code, error); + } + break; } } // parsing is not successful, return the general error diff --git a/test/module/irohad/ametsuchi/postgres_executor_test.cpp b/test/module/irohad/ametsuchi/postgres_executor_test.cpp index 9b70b42676..bee7a9aa3b 100644 --- a/test/module/irohad/ametsuchi/postgres_executor_test.cpp +++ b/test/module/irohad/ametsuchi/postgres_executor_test.cpp @@ -375,22 +375,6 @@ namespace iroha { == signatories->end()); } - /** - * @given command - * @when trying to add signatory to non-existing account - * @then signatory is not added - */ - TEST_F(AddSignatory, NoAccount) { - auto perm = - shared_model::interface::permissions::Grantable::kAddMySignatory; - auto cmd_result = - execute(buildCommand(TestTransactionBuilder().grantPermission( - account->accountId(), perm)), - true, - "id2@domain"); - CHECK_ERROR_CODE(cmd_result, 3); - } - /** * @given command * @when successfully adding signatory to the account @and trying to add the