Query Responses Factory #1724

Akvinikym · 2018-09-17T07:55:59Z

Description of the Change

This PR adds a factory for query responses and tests, which cover all factory's methods.

Benefits

Factory will be able to substitute builders.

Possible Drawbacks

None

Usage Examples or Tests [optional]

New test target proto_query_response_factory_test was added to check the factory.

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

l4l · 2018-09-17T18:47:24Z

shared_model/backend/protobuf/proto_query_response_factory.hpp

+#ifndef IROHA_PROTO_QUERY_RESPONSE_FACTORY_HPP
+#define IROHA_PROTO_QUERY_RESPONSE_FACTORY_HPP
+
+#include "backend/protobuf/query_responses/proto_block_query_response.hpp"


Is it used?

If so fdecl can be used

l4l · 2018-09-17T20:20:12Z

shared_model/backend/protobuf/impl/proto_query_response_factory.cpp

+  protocol_specific_response->set_reason(reason);
+
+  return std::make_unique<shared_model::proto::ErrorQueryResponse>(
+      std::move(protocol_query_response));


what about setting the message?

l4l · 2018-09-17T20:22:34Z

shared_model/backend/protobuf/impl/proto_query_response_factory.cpp

+  iroha::protocol::BlockResponse *protocol_specific_response =
+      protocol_query_response.mutable_block_response();
+  const auto &proto_block =
+      static_cast<const shared_model::proto::Block &>(block);


Maybe explicitly set field without dependency on backend proto?

This class already depends on proto backend in other methods, could you please elaborate?

Well, yes there's no significant problem here. Although the code with casting to internal representation looks not that good

l4l · 2018-09-17T20:24:53Z

shared_model/interfaces/iroha_internal/query_response_factory.hpp

+      /**
+       * Describes type of error to be placed inside the error query response
+       */
+      enum class ErrorQueryType {


Is it possible to reuse this list from somewhere?

Such enumeration exists also in the form of variant inside ErroQueryResponse, but it's impossible, I think, to reuse it

l4l · 2018-09-17T20:26:05Z

test/module/shared_model/backend_proto/CMakeLists.txt

@@ -113,3 +112,13 @@ target_link_libraries(proto_block_factory_test
    shared_model_proto_backend
    shared_model_stateless_validation
    )
+
+if (IROHA_ROOT_PROJECT)


Why it depends on root project?

Because it uses internal part of shared_model_proto_backend, which, in turn, depends on root project. You can find it in "shared_model/backend/protobuf/CMakeLists.txt", line 47

l4l · 2018-09-17T20:27:33Z

test/module/shared_model/backend_proto/proto_query_response_factory_test.cpp

+   * @return shared_ptr to result value or to nullptr, if result containts error
+   */
+  template <typename ResultType, typename ErrorType>
+  std::shared_ptr<ResultType> unwrapResult(


Check out framework/result_fixture.hpp

It cannot be applied here, because object_factory, after which the method unwrapResult is used, returns unique_ptr, and val from framework will try to copy it

l4l · 2018-09-17T20:36:15Z

test/module/shared_model/backend_proto/proto_query_response_factory_test.cpp

+  ASSERT_EQ(response->accountAssets().front().accountId(), kAccountId);
+  ASSERT_EQ(response->accountAssets().front().assetId(), kAssetId);
+  for (auto i = 1; i < kAccountAssetsNumber; i++) {
+    ASSERT_EQ(response->accountAssets()[i - 1].balance(),


Maybe just start with 0?

I can, but in that case this 1 will be always added to balance, because the former cannot be equal to zero. I think, there's no difference

l4l · 2018-09-17T20:39:53Z

test/module/shared_model/backend_proto/proto_query_response_factory_test.cpp

+ * @then that response is created @and is well-formed
+ */
+TEST_F(ProtoQueryResponseFactoryTest, CreateAccountDetailResponse) {
+  const DetailType account_details = "{ fav_meme : doge }";


Doesn't really matter, but just for you knowledge, it's not a valid json

lebdron · 2018-09-18T11:45:44Z

shared_model/backend/protobuf/impl/proto_query_response_factory.cpp

+  iroha::protocol::BlockResponse *protocol_specific_response =
+      protocol_query_response.mutable_block_response();
+  const auto &proto_block =
+      static_cast<const shared_model::proto::Block &>(block);


This class already depends on proto backend in other methods, could you please elaborate?

lebdron · 2018-09-18T11:56:43Z

shared_model/interfaces/iroha_internal/query_response_factory.hpp

+       * @return account asset response
+       */
+      virtual std::unique_ptr<AccountAssetResponse> createAccountAssetResponse(
+          const std::vector<


Consider passing a value instead, so it is possible to either move or copy it from outer scope. Same applies to other methods.

lebdron · 2018-09-18T12:07:12Z

shared_model/interfaces/iroha_internal/query_response_factory.hpp

+       * @param assets to be inserted into the response
+       * @return account asset response
+       */
+      virtual std::unique_ptr<AccountAssetResponse> createAccountAssetResponse(


AccountAssetResponse -> QueryResponse, same for other methods, other than block query response.
Query hash also has to be passed as arguments.

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

Fixed

l4l · 2018-09-23T16:00:19Z

Build is failing :(

l4l

and this plz^

l4l · 2018-09-23T16:27:12Z

shared_model/backend/protobuf/impl/proto_query_response_factory.cpp

-      static_cast<const shared_model::proto::Account &>(account)
-          .getTransport());
+  *protocol_specific_response->mutable_account() =
+      static_cast<shared_model::proto::Account *>(account.release())


Looks like a memleak

l4l · 2018-09-23T16:28:35Z

shared_model/interfaces/iroha_internal/query_response_factory.hpp

+      virtual std::unique_ptr<QueryResponse> createAccountAssetResponse(
+          std::vector<std::shared_ptr<shared_model::interface::AccountAsset>>
+              assets,
+          const crypto::Hash &query_hash) = 0;

      /**
       * Create response for account detail query
       * @param account_detail to be inserted into the response
       * @return account detail response


missed query_hash notice in doc

l4l · 2018-09-23T16:29:27Z

shared_model/interfaces/iroha_internal/query_response_factory.hpp

-      createRolePermissionsResponse(
-          const RolePermissionSet &role_permissions) = 0;
+      virtual std::unique_ptr<QueryResponse> createRolePermissionsResponse(
+          RolePermissionSet role_permissions,


Why by value?
Same for the rest in that file

We decided to pass args by values, so that client will decide, to copy or to move them, and we can do whatever we want with them. Move by ourselves, for example, to initialize some of factory's results. It's not possible now, but later it may become useful

l4l · 2018-09-23T16:34:54Z

test/module/shared_model/backend_proto/proto_query_response_factory_test.cpp

-  }
+  ASSERT_TRUE(query_response);
+  ASSERT_EQ(query_response->queryHash(), kQueryHash);
+  ASSERT_NO_THROW({


It is really hard to determine with such block where is the issue. If exception is triggered, than the line of that won't be noticed. It may leave "as is", though would be nice if you come up with the better idea of such situation

I will add SCOPED_TRACE-s, this will help to at least understand, if SpecifiedVisitor worked correctly

l4l · 2018-09-23T16:37:55Z

test/module/shared_model/backend_proto/proto_query_response_factory_test.cpp

+
+  auto stateless_invalid_response = response_factory->createErrorQueryResponse(
+      ErrorTypes::kStatelessFailed, kStatelessErrorMsg, kQueryHash);
+  auto no_signatories_response = response_factory->createErrorQueryResponse(


out of the pr, but strange that both of methods used in one test

l4l · 2018-09-23T16:39:10Z

test/module/shared_model/backend_proto/proto_query_response_factory_test.cpp

@@ -59,6 +60,8 @@ class ProtoQueryResponseFactoryTest : public ::testing::Test {
 * @then that response is created @and is well-formed
 */
 TEST_F(ProtoQueryResponseFactoryTest, CreateAccountAssetResponse) {
+  const HashType kQueryHash{Blob{"my_super_hash"}};


there's Hash(const std::string&) afair

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

Fixed

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

lebdron · 2018-09-24T12:09:54Z

shared_model/backend/protobuf/impl/proto_query_response_factory.cpp

+  iroha::protocol::AccountAssetResponse *protocol_specific_response =
+      protocol_query_response.mutable_account_assets_response();
+  for (const auto &asset : assets) {
+    protocol_specific_response->add_account_assets()->CopyFrom(


No need to CopyFrom, *protocol_specific_response->add_account_assets() = getTransport() calls copy constructor. Same for other methods.

lebdron · 2018-09-24T12:11:54Z

shared_model/backend/protobuf/impl/proto_query_response_factory.cpp

+
+std::unique_ptr<shared_model::interface::QueryResponse>
+shared_model::proto::ProtoQueryResponseFactory::createAccountAssetResponse(
+    std::vector<std::shared_ptr<shared_model::interface::AccountAsset>> assets,


Why not vector<unique_ptr>, so that it is possible to move the assets to query response? Same in createTransactionsResponse

unique_ptr seems not that handy to use, since most of the methods in iroha works with shared_ptr

lebdron · 2018-09-24T12:13:32Z

shared_model/backend/protobuf/impl/proto_query_response_factory.cpp

+    std::vector<std::shared_ptr<shared_model::interface::AccountAsset>> assets,
+    const crypto::Hash &query_hash) {
+  iroha::protocol::QueryResponse protocol_query_response;
+  protocol_query_response.set_query_hash(query_hash.hex());


Please create a common method for this, so these lines are not duplicated in all the methods.

iroha::protocol::QueryResponse protocol_query_response; protocol_query_response.set_query_hash(query_hash.hex()); ... return std::make_unique<shared_model::proto::QueryResponse>( std::move(protocol_query_response));

Maybe create a method, so that it is possible to pass something in place of ...?

Imo it's better leave as is, otherwise code will be harder to read and modify

lebdron · 2018-09-24T12:17:44Z

shared_model/backend/protobuf/impl/proto_query_response_factory.cpp

+  iroha::protocol::RolesResponse *protocol_specific_response =
+      protocol_query_response.mutable_roles_response();
+  for (const auto &role : roles) {
+    protocol_specific_response->add_roles(role);


It could be add_roles(std::move(role)) if const is removed above.

it also possible to use auto &&role, btw

lebdron · 2018-09-24T12:23:10Z

test/module/shared_model/backend_proto/proto_query_response_factory_test.cpp

+
+#include <gtest/gtest.h>
+#include <boost/optional.hpp>
+


This empty line is redundant according to codestyle.

lebdron · 2018-09-24T12:24:28Z

test/module/shared_model/backend_proto/proto_query_response_factory_test.cpp

+      std::make_shared<ProtoCommonObjectsFactory<FieldValidator>>();
+
+  /**
+   * Put value of Result<unique_ptr<_>, _> into a shared_ptr


Doc is outdated, please fix. Methods returns unique_ptr instead of shared_ptr.

lebdron · 2018-09-24T12:26:14Z

test/module/shared_model/backend_proto/proto_query_response_factory_test.cpp

+  }
+
+  void SetUp() override {}
+  void TearDown() override {}


These two overrides do nothing, please remove.

lebdron · 2018-09-24T12:38:15Z

test/module/shared_model/backend_proto/proto_query_response_factory_test.cpp

+  ASSERT_EQ(query_response->queryHash(), kQueryHash);
+  ASSERT_NO_THROW({
+    const auto &response = boost::apply_visitor(
+        SpecifiedVisitor<shared_model::interface::AccountAssetResponse>(),


SpecifiedVisitor can be replaced with boost::get<const T&> (const & in case of interface), since both return value or throw an exception.

A scoped trace will not be required if boost::get is used.

lebdron · 2018-09-24T12:39:16Z

test/module/shared_model/backend_proto/proto_query_response_factory_test.cpp

+
+  std::vector<std::shared_ptr<shared_model::interface::AccountAsset>> assets;
+  for (auto i = 1; i < kAccountAssetsNumber; ++i) {
+    auto asset = unwrapResult(objects_factory->createAccountAsset(


It should be possible to also replace unwrapResult with boost::get<Value<>> for simplicity.

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

Akvinikym added 8 commits September 13, 2018 20:38

Started implementing the factory

7978025

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

Completed factory

8a31c52

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

Started tests

4bf2ba9

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

Fixed some things with factory

6960c7d

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

Finished tests

7e76a93

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

All tests are up

f1a1d3f

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

Brushing before the PR

7c9bc9d

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

Merged with dev

bb78344

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

Akvinikym requested review from l4l and lebdron September 17, 2018 07:55

Added error responses

ce1f5ca

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

l4l added needs-review pr awaits review from maintainers query All that relates to the iroha querying labels Sep 17, 2018

l4l previously requested changes Sep 17, 2018

View reviewed changes

lebdron previously requested changes Sep 18, 2018

View reviewed changes

Akvinikym added 3 commits September 19, 2018 15:18

Passing arguments as values or unique pointers

a713d9e

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

More review issues fixed

791a82b

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

Added query hashes

6bf4193

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

Akvinikym requested review from l4l and lebdron September 19, 2018 13:09

l4l previously requested changes Sep 23, 2018

View reviewed changes

Akvinikym added 3 commits September 24, 2018 11:01

Review issues

16bdc4d

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

Merge branch 'dev' into feature/qry-resp-factory

58708e2

Fixed memory leak

685f127

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

Akvinikym requested a review from l4l September 24, 2018 09:47

Fixed memory leak(2)

2b23679

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

l4l approved these changes Sep 24, 2018

View reviewed changes

lebdron reviewed Sep 24, 2018

View reviewed changes

Many review issues and refactoring

722d149

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

Akvinikym requested a review from lebdron September 25, 2018 10:22

Akvinikym added 2 commits September 25, 2018 14:00

Fix build

935ce77

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

Got rid of SpecifiedVisitor

bc1b8ee

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

lebdron approved these changes Sep 26, 2018

View reviewed changes

Akvinikym merged commit 5b4e5b2 into dev Sep 26, 2018

Akvinikym deleted the feature/qry-resp-factory branch September 26, 2018 11:12

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 Responses Factory (#1724)

fb7a384

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


		#include <gtest/gtest.h>
		#include <boost/optional.hpp>

Query Responses Factory #1724

Query Responses Factory #1724

Conversation

Akvinikym commented Sep 17, 2018

Description of the Change

Benefits

Possible Drawbacks

Usage Examples or Tests [optional]

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 Sep 18, 2018 • edited Loading

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

Akvinikym Sep 19, 2018 • edited Loading

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 Sep 23, 2018

l4l left a 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

Akvinikym Sep 24, 2018 • edited Loading

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

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 Sep 18, 2018 •

edited

Loading

Akvinikym Sep 19, 2018 •

edited

Loading

Akvinikym Sep 24, 2018 •

edited

Loading