Query Error Responses #1770

Akvinikym · 2018-10-10T12:11:31Z

Description of the Change

Introducing stateful error query responses.

Benefits

Users will see, why their queries fail.

Possible Drawbacks

None.

Signed-off-by: Akvinikym <anarant12@gmail.com>

nickaleks · 2018-10-12T05:52:35Z

irohad/ametsuchi/impl/postgres_query_executor.cpp

-            return *static_cast<shared_model::proto::Transaction *>(
-                clone(tx).get());
-          });
+      boost::transform(range_gen(boost::size(block->transactions()))


maybe move out the range creation so the code is more readable?

nickaleks · 2018-10-12T06:01:04Z

irohad/ametsuchi/impl/postgres_query_executor.hpp


      /**
-       * Execute query in F and return builder result from B
+       * Execute query in F and return query response from B
       * Q is query tuple, P is permission tuple


Maybe name Q a QueryTuple instead? Same for the PermissionTuple

I am always for giving template parameters meaningful names :)

irohad/torii/processor/impl/query_processor_impl.cpp

nickaleks · 2018-10-12T06:33:40Z

irohad/ametsuchi/impl/postgres_query_executor.cpp

      using T = concat<Q, P>;
      try {
        soci::rowset<T> st = std::forward<F>(f)();
        auto range = boost::make_iterator_range(st.begin(), st.end());

        return apply(
-            viewPermissions<P>(range.front()), [range, &b](auto... perms) {
+            viewPermissions<P>(range.front()),
+            [this, range, &b, err_response = std::move(err_response)](


Since err_response is a universal reference, better to forward it?

nickaleks · 2018-10-12T06:43:23Z

irohad/ametsuchi/impl/postgres_query_executor.cpp

+                        return query_response_factory_
+                            ->createErrorQueryResponse(
+                                QueryErrorType::kStatefulFailed,
+                                "could not create asset object: " + err.error,


the same error is duplicated for log and here, please move it to a common variable.

irohad/ametsuchi/impl/postgres_query_executor.cpp

Signed-off-by: Akvinikym <anarant12@gmail.com>

Fixed

nickaleks · 2018-10-12T12:10:14Z

irohad/ametsuchi/impl/postgres_query_executor.cpp

 #include "common/types.hpp"
+#include "cryptography/public_key.hpp"


Is this include needed?

Yes, it's used in line 411:
return shared_model::interface::types::PubkeyType{ shared_model::crypto::Blob::fromHexString(public_key)};

nickaleks · 2018-10-12T12:10:44Z

irohad/ametsuchi/impl/postgres_query_executor.cpp

+                                                           RangeGen &&range_gen,
+                                                           Pred &&pred) {
+      std::vector<std::unique_ptr<shared_model::interface::Transaction>>
+          result{};


Unnecessary {}

nickaleks · 2018-10-12T12:12:32Z

irohad/ametsuchi/impl/postgres_query_executor.cpp

-        return stateful_failed;
+        return query_response_factory_->createErrorQueryResponse(
+            QueryErrorType::kStatefulFailed,
+            "failed to execute query: " + std::string(e.what()),


Replication of strings. Also you seem to log some errors, but not others. Is this intentional?
If you want to log all errors you can wrap logging and error construction in a function to reduce code duplication.

nickaleks · 2018-10-12T12:14:01Z

irohad/ametsuchi/impl/postgres_query_executor.cpp

          log_(logger::log("PostgresQueryExecutorVisitor")) {}

    void PostgresQueryExecutorVisitor::setCreatorId(
        const shared_model::interface::types::AccountIdType &creator_id) {
      creator_id_ = creator_id;
    }

-    QueryResponseBuilderDone PostgresQueryExecutorVisitor::operator()(
+    void PostgresQueryExecutorVisitor::setQueryHash(
+        const shared_model::crypto::Hash &query_hash) {


You can use shared_model::interface::types::HashType, so that you do not depend on cryptography directly

l4l · 2018-10-13T12:35:39Z

Please update your branch and make sure that CI become green

Signed-off-by: Akvinikym <anarant12@gmail.com>

Fixed

nickaleks · 2018-10-15T08:55:42Z

irohad/ametsuchi/impl/postgres_query_executor.cpp

+                return logAndReturnErrorResponse(
+                    QueryErrorType::kStatefulFailed,
+                    err_response(),
+                    this->query_hash_,


You pass three parameters which are class members. Maybe make logAndReturnErrorResponse a member of executor, so you do not need to pass parameters each time?

The function is used in both PostgresQueryExecutorVisitor and PostgresQueryExecutor, thus making it member will force having two identical function definitions

lebdron · 2018-10-15T09:25:16Z

irohad/ametsuchi/impl/postgres_query_executor.hpp

+       * @tparam QueryExecutor - function, which executes the query
+       * @tparam ResponseCreator - function, which creates response of the query
+       * @tparam ErrResponse - function, which creates error response
+       * @param query_executor


Please describe parameters and return, or make a description without detailed descriptions with @.

lebdron · 2018-10-15T14:38:14Z

irohad/ametsuchi/impl/postgres_query_executor.cpp

+      iroha::ametsuchi::QueryErrorType error_type,
+      std::string error_body,
+      const shared_model::crypto::Hash &query_hash,
+      std::shared_ptr<shared_model::interface::QueryResponseFactory>


This function does not need shared ownership of QueryResponseFactory. Const reference can be used instead

lebdron · 2018-10-15T14:40:30Z

irohad/ametsuchi/impl/postgres_query_executor.cpp

+
+    std::string error;
+    switch (error_type) {
+      case QueryErrorType::kNoAccount: {


Code block should not be required in case

lebdron · 2018-10-15T14:46:37Z

irohad/ametsuchi/impl/postgres_query_executor.cpp

+            QueryErrorType::kNoAccount, std::move(error), query_hash);
+      }
+      case QueryErrorType::kNoSignatories: {
+        error = "no signatories found in account with such id: " + error_body;


Consider making a lambda function, since each case looks almost identical. For example:

return make_error("no signatories found in account with such id: " + error_body, QueryErrorType::kNoSignatories);

This lambda can capture query_hash, and other necessary variables.

lebdron · 2018-10-15T14:48:26Z

irohad/ametsuchi/impl/postgres_query_executor.cpp

-            return *static_cast<shared_model::proto::Transaction *>(
-                clone(tx).get());
-          });
+      auto filtered_txs = range_gen(boost::size(block->transactions()))


There were some issues with ranges when such assignment was made. Please make sure that Linux CI passes.

lebdron · 2018-10-15T14:51:54Z

irohad/ametsuchi/impl/postgres_query_executor.cpp

+            [this,
+             range,
+             &response_creator,
+             err_response =


&err_response should suffice here.

lebdron · 2018-10-15T14:54:50Z

irohad/ametsuchi/impl/postgres_query_executor.cpp

-                return stateful_failed;
+                return logAndReturnErrorResponse(
+                    QueryErrorType::kStatefulFailed,
+                    err_response(),


However, it is required to use forward here since there could be &- and &&-modified methods.

lebdron · 2018-10-15T14:59:14Z

irohad/ametsuchi/impl/postgres_query_executor.cpp

+                  return logAndReturnErrorResponse(
+                      QueryErrorType::kStatefulFailed,
+                      std::move(e.error),
+                      this->query_hash_,


This can be moved to member method:

auto PostgresQueryExecutorVisitor::makeErrorResponse(Type type, Error error) { return logAndReturnErrorResponse(type, error, query_hash_, query_response_factory_, log_); }

Duplication in PostgresQueryExecutor may not be as bad.

lebdron · 2018-10-15T15:01:10Z

irohad/ametsuchi/impl/postgres_query_executor.cpp

-                                    shared_model::proto::AccountAsset *>(
-                                    v.value.get());
-                                account_assets.push_back(proto);
+                                account_assets.push_back(clone(*v.value.get()));


Is it possible to avoid clone here?

lebdron · 2018-10-15T15:08:34Z

irohad/ametsuchi/impl/postgres_query_executor.hpp


      soci::session &sql_;
      KeyValueStorage &block_store_;
      shared_model::interface::types::AccountIdType creator_id_;
-      std::shared_ptr<shared_model::interface::CommonObjectsFactory> factory_;
+      shared_model::crypto::Hash query_hash_;


-> HashType?

Signed-off-by: Akvinikym <anarant12@gmail.com>

l4l · 2018-10-16T11:38:46Z

Roses are red,
Violets are blue
and red CI saddens you

Signed-off-by: Akvinikym <anarant12@gmail.com>

lebdron · 2018-10-17T06:09:04Z

irohad/ametsuchi/impl/postgres_query_executor.cpp

-                    this->query_hash_,
-                    query_response_factory_,
-                    log_);
+                    std::forward<std::string>(err_response()));


It should be std::forward<ErrResponse>(err_response)(), since return type is std::string value anyway, but err_response can be reference or value.

Signed-off-by: Akvinikym <anarant12@gmail.com>

Akvinikym added 4 commits October 8, 2018 16:12

Most features ase added

8eaf120

Signed-off-by: Akvinikym <anarant12@gmail.com>

Some minor fixes

27e06d0

Signed-off-by: Akvinikym <anarant12@gmail.com>

Added response for lacking permissions

2d00e95

Signed-off-by: Akvinikym <anarant12@gmail.com>

Added response for noX

a767d2c

Signed-off-by: Akvinikym <anarant12@gmail.com>

Akvinikym requested review from nickaleks and lebdron October 10, 2018 12:11

nickaleks previously requested changes Oct 12, 2018

View reviewed changes

Akvinikym added 2 commits October 12, 2018 14:06

Gave some template parameters meaningful names

a03e75b

Signed-off-by: Akvinikym <anarant12@gmail.com>

Other review issues

b15aad5

Signed-off-by: Akvinikym <anarant12@gmail.com>

Akvinikym requested a review from nickaleks October 12, 2018 11:16

nickaleks previously requested changes Oct 12, 2018

View reviewed changes

l4l added needs-correction pr/rfc is not completed and might be updated ametsuchi labels Oct 13, 2018

Akvinikym added 2 commits October 15, 2018 09:59

Merge branch 'dev' into feature/qry-err-rsp

66b83eb

Review issues

aeceef9

Signed-off-by: Akvinikym <anarant12@gmail.com>

Akvinikym requested a review from nickaleks October 15, 2018 08:11

nickaleks suggested changes Oct 15, 2018

View reviewed changes

nickaleks approved these changes Oct 15, 2018

View reviewed changes

l4l added needs-review pr awaits review from maintainers and removed needs-correction pr/rfc is not completed and might be updated labels Oct 15, 2018

lebdron approved these changes Oct 15, 2018

View reviewed changes

Akvinikym added 2 commits October 16, 2018 12:12

More review issues

13ebd1d

Signed-off-by: Akvinikym <anarant12@gmail.com>

Fix build

55deb8f

Signed-off-by: Akvinikym <anarant12@gmail.com>

Fix build

583f651

Signed-off-by: Akvinikym <anarant12@gmail.com>

lebdron reviewed Oct 17, 2018

View reviewed changes

Fixed forward

b65864a

Signed-off-by: Akvinikym <anarant12@gmail.com>

Akvinikym merged commit 497f520 into dev Oct 17, 2018

Akvinikym deleted the feature/qry-err-rsp branch October 17, 2018 09:18

l4l removed the needs-review pr awaits review from maintainers label Oct 17, 2018

kamilsa mentioned this pull request Nov 1, 2018

Hyperledger Iroha v1.0 beta-5 version #1825

Closed

bakhtin pushed a commit that referenced this pull request Nov 2, 2018

Query Error Responses (#1770)

1d3a4bf

Signed-off-by: Akvinikym <anarant12@gmail.com>

neewy mentioned this pull request Nov 2, 2018

Hyperledger Iroha v1.0 beta-5 version #1832

Closed

kamilsa mentioned this pull request Dec 12, 2018

Hyperledger Iroha v1.0 Release Candidate 1 #1941

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Query Error Responses #1770

Query Error Responses #1770

Akvinikym commented Oct 10, 2018

nickaleks Oct 12, 2018

nickaleks Oct 12, 2018

Akvinikym Oct 12, 2018

nickaleks Oct 12, 2018

nickaleks Oct 12, 2018

nickaleks Oct 12, 2018

Akvinikym Oct 15, 2018

nickaleks Oct 12, 2018

nickaleks Oct 12, 2018

nickaleks Oct 12, 2018

l4l commented Oct 13, 2018

nickaleks Oct 15, 2018

Akvinikym Oct 15, 2018

lebdron Oct 15, 2018

lebdron Oct 15, 2018

lebdron Oct 15, 2018

lebdron Oct 15, 2018

lebdron Oct 15, 2018

lebdron Oct 15, 2018

lebdron Oct 15, 2018

lebdron Oct 15, 2018

lebdron Oct 15, 2018

lebdron Oct 15, 2018

l4l commented Oct 16, 2018

lebdron Oct 17, 2018

		#include "common/types.hpp"
		#include "cryptography/public_key.hpp"

Query Error Responses #1770

Query Error Responses #1770

Conversation

Akvinikym commented Oct 10, 2018

Description of the Change

Benefits

Possible Drawbacks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l4l commented Oct 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l4l commented Oct 16, 2018

Choose a reason for hiding this comment