common objects factory for wsv query #1573

nickaleks · 2018-07-17T16:43:23Z

Description of the Change

Replace builders in wsv_query with common objects factory

Benefits

No dependency on builders

Possible Drawbacks

none

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

This allows addition of validator in the future Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

Signed-off-by: Nikita Alekseev <[email protected]>

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # irohad/ametsuchi/impl/mutable_storage_impl.cpp # irohad/ametsuchi/impl/mutable_storage_impl.hpp # irohad/ametsuchi/impl/postgres_block_query.cpp # irohad/ametsuchi/impl/postgres_wsv_common.hpp # irohad/ametsuchi/impl/postgres_wsv_query.cpp # irohad/ametsuchi/impl/postgres_wsv_query.hpp # irohad/ametsuchi/impl/storage_impl.cpp # irohad/ametsuchi/impl/storage_impl.hpp # irohad/ametsuchi/impl/temporary_wsv_impl.cpp # irohad/ametsuchi/impl/temporary_wsv_impl.hpp # test/module/irohad/ametsuchi/CMakeLists.txt # test/module/irohad/ametsuchi/ametsuchi_fixture.hpp # test/module/irohad/ametsuchi/wsv_query_command_test.cpp # test/module/shared_model/backend_proto/CMakeLists.txt

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/module/shared_model/backend_proto/CMakeLists.txt

…ects_factory_wsv_query Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # shared_model/backend/protobuf/common_objects/proto_common_objects_factory.hpp # shared_model/interfaces/common_objects/common_objects_factory.hpp # test/module/shared_model/backend_proto/common_objects/proto_common_objects_factory_test.cpp

vdrobnyi · 2018-07-18T12:29:13Z

irohad/ametsuchi/impl/storage_impl.hpp

@@ -24,8 +24,12 @@
 #include <boost/optional.hpp>
 #include <cmath>
 #include <shared_mutex>
+
+#include <boost/optional.hpp>


Is it the same include at line 24?

vdrobnyi · 2018-07-18T12:33:31Z

shared_model/interfaces/common_objects/common_objects_factory.hpp

+#include "interfaces/common_objects/account_asset.hpp"
+#include "interfaces/common_objects/asset.hpp"
+#include "interfaces/common_objects/domain.hpp"
+#include "interfaces/common_objects/signature.hpp"


Maybe replace it with a forward declaration since it has only declarations?

Also domain include seems to be duplication

The issues is with signature::SignedType. We cannot forward-declare inner classes.

vdrobnyi · 2018-07-18T12:40:14Z

test/module/irohad/ametsuchi/ametsuchi_test.cpp

-  storageResult.match(
+  auto new_storage_result =
+      StorageImpl::create(block_store_path, pgopt_, factory);
+  new_storage_result.match(


Akvinikym · 2018-07-18T12:43:02Z

irohad/main/application.cpp

+  auto factory =
+      std::make_shared<shared_model::proto::ProtoCommonObjectsFactory<
+          shared_model::validation::FieldValidator>>();
+  auto storageResult = StorageImpl::create(block_store_dir_, pg_conn_, factory);


Maybe create factory variable directly inside StorageImpl::create(..)? It's not used in code, and the space will be saved

We want to isolate protobuf specific implementations from the rest of the system.

Akvinikym · 2018-07-18T12:44:01Z

test/module/irohad/ametsuchi/ametsuchi_fixture.hpp

@@ -78,6 +80,12 @@ namespace iroha {

      std::shared_ptr<soci::session> sql;

+      std::shared_ptr<shared_model::proto::ProtoCommonObjectsFactory<
+          shared_model::validation::FieldValidator>>
+      factory =


Why not auto?

It is a class member

Akvinikym · 2018-07-18T12:44:24Z

test/module/irohad/ametsuchi/ametsuchi_test.cpp

@@ -216,7 +216,7 @@ TEST_F(AmetsuchiTest, SampleTest) {
                   .build()}))
          .height(2)
          .prevHash(block1.hash())
-          .build();
+          .build(); 


Empty space?

Akvinikym · 2018-07-18T12:45:16Z

test/module/irohad/ametsuchi/storage_init_test.cpp

@@ -34,6 +36,11 @@ class StorageInitTest : public ::testing::Test {
  std::string pg_opt_without_dbname_;
  std::string pgopt_;

+  std::shared_ptr<shared_model::proto::ProtoCommonObjectsFactory<
+      shared_model::validation::FieldValidator>>
+      factory = std::make_shared<shared_model::proto::ProtoCommonObjectsFactory<


Again not auto, is there a reason?

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # shared_model/backend/protobuf/common_objects/proto_common_objects_factory.hpp # shared_model/interfaces/common_objects/common_objects_factory.hpp # test/module/shared_model/backend_proto/common_objects/proto_common_objects_factory_test.cpp

vdrobnyi · 2018-07-19T08:23:15Z

irohad/ametsuchi/impl/postgres_wsv_query.cpp

@@ -17,6 +17,7 @@

 #include "ametsuchi/impl/postgres_wsv_query.hpp"
 #include "backend/protobuf/permissions.hpp"
+#include "common/result.hpp"


This include is presented in postgres_wsv_common.hpp. It is not necessary to have it here

It is better not to have transitive includes, so that if we change one header, we won't break stuff

vdrobnyi · 2018-07-19T08:26:20Z

irohad/ametsuchi/impl/postgres_wsv_query.cpp

@@ -284,16 +295,16 @@ namespace iroha {
          (sql_.prepare << "SELECT public_key, address FROM peer");
      std::vector<std::shared_ptr<shared_model::interface::Peer>> peers;

-      auto results = transform<


Is 'transform' from postgres_wsv_common.hpp called from somewhere? Can the method be removed?

vdrobnyi · 2018-07-19T08:31:06Z

shared_model/interfaces/common_objects/common_objects_factory.hpp

@@ -13,6 +13,10 @@
 #include "interfaces/common_objects/peer.hpp"
 #include "interfaces/common_objects/types.hpp"
 #include "interfaces/common_objects/domain.hpp"
+#include "interfaces/common_objects/account_asset.hpp"
+#include "interfaces/common_objects/asset.hpp"
+#include "interfaces/common_objects/domain.hpp"


It is the duplication of line 15

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # shared_model/backend/protobuf/common_objects/proto_common_objects_factory.hpp # shared_model/interfaces/common_objects/common_objects_factory.hpp # test/module/shared_model/backend_proto/common_objects/proto_common_objects_factory_test.cpp

sorabot · 2018-07-19T11:37:15Z

SonarQube analysis reported 7 issues

postgres_block_index.hpp#L54: Unused private function: 'PostgresBlockIndex::indexAccountAssets'
postgres_block_query.hpp#L78: Unused private function: 'PostgresBlockQuery::getBlockIds'
postgres_block_query.hpp#L86: Unused private function: 'PostgresBlockQuery::getBlockId'
postgres_block_query.hpp#L96: Unused private function: 'PostgresBlockQuery::callback'
ametsuchi_fixture.hpp#L81: The class 'BlockQueryTransferTest' defines member variable with name 'sql' also defined in its parent class 'AmetsuchiTest'.
ametsuchi_fixture.hpp#L81: The class 'WsvQueryCommandTest' defines member variable with name 'sql' also defined in its parent class 'AmetsuchiTest'.
storage_init_test.cpp#L23: Variable 'pg_opt_without_dbname_' is assigned in constructor body. Consider performing initialization in initialization list.

Signed-off-by: Nikita Alekseev <[email protected]>

nickaleks added 16 commits July 9, 2018 18:41

Implement Abstract factory for peer

43d7902

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

Factory returns Result instead of just pointer

f7eabe8

This allows addition of validator in the future Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

Add validation logic to factory

e904964

This allows addition of validator in the future Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

Add account creation to factory

781cb39

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

Add account asset creation to factory

961fe11

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

Add amount creation to factory

a75f7fc

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

Add asset and domain creation to factory

48b7605

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

Add signature creation to factory

34a11ab

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

Add comments

2f30e41

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

Fix review issues

e40cb10

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

change ametsuchi interfaces for new factory

8a73c24

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

Fix compilation issues

b6b9b3c

Signed-off-by: Nikita Alekseev <[email protected]>

Merge develop

bb438e0

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/module/shared_model/backend_proto/CMakeLists.txt

Remove postgres converter

257ebe9

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/module/shared_model/backend_proto/CMakeLists.txt

nickaleks force-pushed the feature/common_objects_factory_wsv_query branch from 89b23bf to 40d7996 Compare July 18, 2018 11:03

Fix issues

c436031

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/module/shared_model/backend_proto/CMakeLists.txt

nickaleks force-pushed the feature/common_objects_factory_wsv_query branch from 40d7996 to c436031 Compare July 18, 2018 11:08

nickaleks requested a review from Akvinikym July 18, 2018 12:18

nickaleks force-pushed the feature/common_objects_factory_wsv_query branch from cbd122a to 482e712 Compare July 18, 2018 12:18

nickaleks requested a review from vdrobnyi July 18, 2018 12:18

vdrobnyi suggested changes Jul 18, 2018

View reviewed changes

Akvinikym suggested changes Jul 18, 2018

View reviewed changes

vdrobnyi suggested changes Jul 19, 2018

View reviewed changes

Akvinikym approved these changes Jul 19, 2018

View reviewed changes

vdrobnyi approved these changes Jul 19, 2018

View reviewed changes

nickaleks force-pushed the feature/common_objects_factory_wsv_query branch from 8970d8f to be92c27 Compare July 19, 2018 11:06

nickaleks merged commit 4debf4e into develop Jul 19, 2018

nickaleks deleted the feature/common_objects_factory_wsv_query branch July 19, 2018 13:43

neewy mentioned this pull request Jul 25, 2018

Hyperledger Iroha v1.0 beta-4 version #1597

Merged

l4l pushed a commit that referenced this pull request Jul 25, 2018

Replace builders with common_objects_factory in wsv_query (#1573)

68fae26

Signed-off-by: Nikita Alekseev <[email protected]>

l4l pushed a commit that referenced this pull request Jul 25, 2018

Replace builders with common_objects_factory in wsv_query (#1573)

777488a

Signed-off-by: Nikita Alekseev <[email protected]>

l4l pushed a commit that referenced this pull request Jul 25, 2018

Replace builders with common_objects_factory in wsv_query (#1573)

308b7cf

Signed-off-by: Nikita Alekseev <[email protected]>

l4l pushed a commit that referenced this pull request Jul 25, 2018

Replace builders with common_objects_factory in wsv_query (#1573)

4bf347a

Signed-off-by: Nikita Alekseev <[email protected]>

l4l pushed a commit that referenced this pull request Jul 25, 2018

Replace builders with common_objects_factory in wsv_query (#1573)

05c9b2a

Signed-off-by: Nikita Alekseev <[email protected]>

l4l pushed a commit that referenced this pull request Jul 25, 2018

Replace builders with common_objects_factory in wsv_query (#1573)

c9a652b

Signed-off-by: Nikita Alekseev <[email protected]>

l4l pushed a commit that referenced this pull request Jul 25, 2018

Replace builders with common_objects_factory in wsv_query (#1573)

719d344

Signed-off-by: Nikita Alekseev <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

common objects factory for wsv query #1573

common objects factory for wsv query #1573

nickaleks commented Jul 17, 2018

vdrobnyi Jul 18, 2018

vdrobnyi Jul 18, 2018

vdrobnyi Jul 18, 2018

nickaleks Jul 18, 2018

vdrobnyi Jul 18, 2018

Akvinikym Jul 18, 2018

nickaleks Jul 18, 2018

Akvinikym Jul 18, 2018

nickaleks Jul 18, 2018

Akvinikym Jul 18, 2018

Akvinikym Jul 18, 2018

vdrobnyi Jul 19, 2018

nickaleks Jul 19, 2018

vdrobnyi Jul 19, 2018

vdrobnyi Jul 19, 2018

sorabot commented Jul 19, 2018

common objects factory for wsv query #1573

common objects factory for wsv query #1573

Conversation

nickaleks commented Jul 17, 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

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

sorabot commented Jul 19, 2018