This repository has been archived by the owner on Apr 17, 2019. It is now read-only.
Stateful Validation Error Codes in Endpoint #1821
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Akvinikym <[email protected]>
Signed-off-by: Akvinikym <[email protected]>
Signed-off-by: Akvinikym <[email protected]>
Signed-off-by: Akvinikym <[email protected]>
Signed-off-by: Akvinikym <[email protected]>
Signed-off-by: Akvinikym <[email protected]>
Signed-off-by: Akvinikym <[email protected]>
Signed-off-by: Akvinikym <[email protected]>
Signed-off-by: Akvinikym <[email protected]>
Signed-off-by: Akvinikym <[email protected]>
Signed-off-by: Akvinikym <[email protected]>
Signed-off-by: Akvinikym <[email protected]>
Signed-off-by: Akvinikym <[email protected]>
Signed-off-by: Akvinikym <[email protected]>
Signed-off-by: Akvinikym <[email protected]>
Signed-off-by: Akvinikym <[email protected]>
Signed-off-by: Akvinikym <[email protected]>
Signed-off-by: Akvinikym <[email protected]>
Signed-off-by: Akvinikym <[email protected]>
… feature/stf-vld-err-codes
…feature/pcs-err-cds
muratovv
previously requested changes
Nov 9, 2018
| @@ -58,12 +58,12 @@ namespace torii { | |||
|
|
|||
| if (is_present) { | |||
| std::shared_ptr<shared_model::interface::TransactionResponse> response = | |||
| status_factory_->makeCommitted(request, ""); | |||
| status_factory_->makeCommitted(request, "", 0, 0); | |||
There was a problem hiding this comment.
I have two suggestions here:
- Use default parameters from the factory.
- And move error plus codes to one entity.
It should be like
Suggested change
| status_factory_->makeCommitted(request, "", 0, 0); | |
| status_factory_->makeCommitted(request, TxStatusFactory::emptyError()); |
Where empty error returns class which contains: {error_msg, index, code}
| if (not error.empty()) { | ||
| builder = builder.errorMsg(error); | ||
| if (not cmd_error.name.empty()) { | ||
| builder = builder.statelessErrorOrCmdName(cmd_error.name) |
There was a problem hiding this comment.
Pls, avoid builders at all.
| virtual const StatelessErrorOrFailedCommandNameType & | ||
| statelessErrorOrCommandName() const override; | ||
|
|
||
| virtual FailedCommandIndexType failedCommandIndex() const override; |
There was a problem hiding this comment.
Maybe hash of the transaction is more appropriate here?
There was a problem hiding this comment.
The index can help during understanding, exactly which command failed in the given transaction, so this information is useful
| @@ -67,13 +67,28 @@ namespace shared_model { | |||
| */ | |||
| virtual const ResponseVariantType &get() const = 0; | |||
|
|
|||
| /// Message type | |||
| using ErrorMessageType = std::string; | |||
| /// Type of stateless validation error or of command name, which failed | |||
There was a problem hiding this comment.
seems the second of is redundant.
Signed-off-by: Akvinikym <[email protected]>
Akvinikym
changed the base branch from
feature/stf-vld-err-codes
to
trunk/stf-rsp-err-codes
muratovv
approved these changes
Nov 12, 2018
l4l
approved these changes
Nov 12, 2018
| auto status_factory = | ||
| std::make_shared<shared_model::proto::ProtoTxStatusFactory>(); | ||
| auto tx_processor = std::make_shared<TransactionProcessorImpl>( |
There was a problem hiding this comment.
Because we need status_factory to be in TransactionProcessor, so the latter is to be initialized after the factory
| FailedCommandIndexType cmd_index_; | ||
| ErrorCodeType error_code_; | ||
|
|
||
| TransactionError() : cmd_name_{""}, cmd_index_{0}, error_code_{0} {} |
There was a problem hiding this comment.
Quite redundant initialization
| and errorMessage() == rhs.errorMessage() and get() == rhs.get(); | ||
| and statelessErrorOrCommandName() == rhs.statelessErrorOrCommandName() | ||
| and failedCommandIndex() == rhs.failedCommandIndex() | ||
| and errorCode() == rhs.errorCode() and get() == rhs.get(); |
There was a problem hiding this comment.
is comparison on get result is required?
There was a problem hiding this comment.
Why not? Two responses in theory can have similar values of those fields, but different response type, I think
| @@ -292,19 +295,18 @@ TEST_F(ClientServerTest, SendTxWhenStatefulInvalid) { | |||
| ASSERT_EQ(client.sendTx(tx).answer, iroha_cli::CliClient::OK); | |||
|
|
|||
| // fail the tx | |||
| auto cmd_name = "CommandName"; | |||
| size_t cmd_index = 2; | |||
| uint32_t error_code = 2; | |||
There was a problem hiding this comment.
Set please that value different from the cmd_index one
Test is a bit more precise in that case
Signed-off-by: Akvinikym <[email protected]>
Signed-off-by: Akvinikym <[email protected]>
Akvinikym
changed the base branch from
feature/stf-vld-err-codes
to
trunk/stf-rsp-err-codes
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of the Change
Final part of introducing stateful validation error codes for commands - now they are added into transaction_processor, responses and endpoint.
Benefits
Error codes are in Iroha.
Possible Drawbacks
None.