Feature/soci #1505

vdrobnyi · 2018-06-28T08:09:31Z

Description of the Change

SOCI instead of pqxx.

Signed-off-by: Victor Drobny <[email protected]>

* Initial SOCI integration to CMake build * Add download step for SOCI * Replace pqxx with soci in dockerfiles * Enable packaging for SOCI * Fix new directory path when downloading * Fix pg_config and include not found by SOCI when pq not installed * Fix docs, snapcraft; remove pqxx include * Remove extra ; and add missing link library Signed-off-by: Andrei Lebedev <[email protected]>

Signed-off-by: Victor Drobny <[email protected]>

…nto feature/soci

nickaleks · 2018-07-02T08:13:53Z

irohad/ametsuchi/impl/postgres_block_index.cpp

-                        + transaction_.quote(command.assetId()) + ", "
-                        + transaction_.quote(index) + ");");
-                    status &= res != boost::none;
+                    try {


Need to put all SQL queries into a separate function to avoid try-catch on any query

nickaleks · 2018-07-02T08:14:07Z

irohad/ametsuchi/impl/postgres_block_index.cpp

@@ -68,18 +65,19 @@ namespace iroha {
                  auto ids = {account_id,
                              command.srcAccountId(),
                              command.destAccountId()};
+                  auto asset_id = command.assetId();


use reference?

nickaleks · 2018-07-02T08:35:10Z

irohad/ametsuchi/impl/postgres_block_query.cpp

+      sql_ << "SELECT height FROM height_by_hash WHERE hash = :hash",
+          soci::into(block_str), soci::use(hash_str);
+      if (block_str) {
+        blockId = std::stoull(block_str.get());


Maybe use row::get to retrieve?

It fails. Height is stored as a string in postgres. Maybe it's better to store it as an integer?

Signed-off-by: Victor Drobny <[email protected]>

nickaleks · 2018-07-03T12:17:36Z

irohad/ametsuchi/impl/postgres_wsv_command.cpp

+      st.exchange(soci::use(account_id, "account_id"));
+      st.exchange(soci::use(perm_str, "perm"));
+
+      auto msg =


Please make that into a lambda. We do not want to instantiate error message in the successful path.

nickaleks · 2018-07-03T12:20:02Z

irohad/ametsuchi/impl/postgres_wsv_command.cpp

+      st.exchange(soci::use(account_id, "account_id"));
+      st.exchange(soci::use(perm_str, "perm"));
+
+      auto msg =


Please make that into a lambda so it is evaluated only when we fail to perform the query.

nickaleks · 2018-07-03T12:38:15Z

irohad/ametsuchi/impl/postgres_wsv_query.hpp

-      PostgresWsvQuery(std::unique_ptr<pqxx::lazyconnection> connection,
-                       std::unique_ptr<pqxx::nontransaction> transaction);
+      explicit PostgresWsvQuery(soci::session &sql);
+      PostgresWsvQuery(std::unique_ptr<soci::session> sql_ptr);


Make it explicit since it accepts single parameter now

Signed-off-by: Victor Drobny <[email protected]>

nickaleks · 2018-07-04T09:39:30Z

irohad/ametsuchi/impl/postgres_block_query.cpp

-                  .template as<shared_model::interface::types::HeightType>();
-            });
-      };
+      std::vector<shared_model::interface::types::HeightType> result;


This sequence of statements is repeated when you select several rows from the database, maybe refactor it to a separate function?

nickaleks · 2018-07-04T09:40:40Z

irohad/ametsuchi/impl/postgres_block_query.hpp

-      PostgresBlockQuery(std::unique_ptr<pqxx::lazyconnection> connection,
-                         std::unique_ptr<pqxx::nontransaction> transaction,
+      PostgresBlockQuery(soci::session &sql, KeyValueStorage &file_store);
+      PostgresBlockQuery(std::unique_ptr<soci::session> sql_ptr,


Please make explicit

nickaleks · 2018-07-04T09:45:50Z

irohad/ametsuchi/impl/postgres_wsv_command.cpp

        return (boost::format("failed to insert role permissions, role "
                              "id: '%s', permissions: [%s]")
-                % role_id % perm_debug_str)
+                % role_id % perm_str)


need to print perm_debug_str here

nickaleks · 2018-07-04T09:56:59Z

irohad/ametsuchi/impl/postgres_wsv_common.hpp

     * @param transform_func - function which transforms result row to T
     * @return vector of target type
     */
    template <typename T, typename Operator>
-    std::vector<T> transform(const pqxx::result &result,
+    std::vector<T> transform(const soci::rowset<soci::row> &result,


Please move these functions out of command executor

nickaleks · 2018-07-04T10:00:40Z

irohad/ametsuchi/impl/postgres_wsv_common.hpp

+    }
+
+    template <typename T, typename Operator>
+    std::vector<T> transform(const std::vector<soci::row> &result,


What this function is supposed to do?

nickaleks · 2018-07-04T10:05:37Z

scripts/local-develop.sh

@@ -36,6 +36,8 @@ pghost=127.0.0.1

 pgid=$(docker run -p $pghost:$pgport:$pgport --rm --name $pgname -e POSTGRES_USER=$user -e POSTGRES_PASSSWORD=$password -d postgres:9.5)

+docker run -p 127.0.0.1:5432:5432 --rm --name postgrestestimage -e POSTGRES_USER=postgres -e POSTGRES_PASSSWORD=mysecretpassword -d postgres:9.5


Should it be here?

l4l

Why the operator , isn't used for statements? E.g
sql << "SELECT * FROM table", into(val);

l4l · 2018-07-04T09:05:24Z

irohad/ametsuchi/impl/postgres_block_index.cpp

-                        + transaction_.quote(command.assetId()) + ", "
-                        + transaction_.quote(index) + ");");
-                    status &= res != boost::none;
+                    soci::statement st =


Doesn' the = operator have the lowest priority? I mean the braces around prepare seems redundant

No, comma has lower priority

l4l · 2018-07-04T09:07:29Z

irohad/ametsuchi/impl/postgres_block_index.cpp

@@ -94,26 +106,31 @@ namespace iroha {
          block.transactions() | boost::adaptors::indexed(0),
          [&](const auto &tx) {
            const auto &creator_id = tx.value().creatorAccountId();
-            const auto &hash = tx.value().hash().blob();
+            const auto &hash = tx.value().hash().hex();


What about using soci::blob instead?

Blob in postgres is designed for large files. I'm not sure that it is necessary here.

That fixes changes schema and should be considered (if necessary) in other pr at least. It better to minimize the fixes here

l4l · 2018-07-04T09:09:55Z

irohad/ametsuchi/impl/postgres_block_index.cpp

-        : transaction_(transaction),
-          log_(logger::log("PostgresBlockIndex")),
-          execute_{makeExecuteOptional(transaction_, log_)} {}
+    bool executeSOCI(soci::statement &st) {


executeSoci please

l4l · 2018-07-04T09:34:08Z

irohad/ametsuchi/impl/postgres_wsv_common.hpp

@@ -106,43 +80,81 @@ namespace iroha {
      }
    }

+    template <typename Function, typename ParamType>
+    void processSOCI(soci::statement &st,


Again, processSoci

l4l · 2018-07-04T09:47:02Z

irohad/ametsuchi/impl/postgres_ordering_service_persistent_state.cpp

+      *sql_ << "CREATE TABLE IF NOT EXISTS ordering_service_state "
+               "(proposal_height bigserial)";
+      *sql_ << "INSERT INTO ordering_service_state VALUES (2)";
+      return true;


What about false case? Same for the following

l4l · 2018-07-04T10:08:16Z

irohad/ametsuchi/impl/postgres_wsv_common.hpp

            .build();
      });
    }

    static inline shared_model::builder::BuilderResult<
        shared_model::interface::AccountAsset>
-    makeAccountAsset(const pqxx::row &row) noexcept {
+    makeAccountAsset(const soci::row &row) noexcept {


It's better change the signature as well: pass 3 strings
Or maybe all similar function can use soci::row
Anyway that inconsistency should be resolved

l4l · 2018-07-04T10:19:28Z

irohad/ametsuchi/impl/postgres_wsv_query.cpp

+      processSOCI(st, ind, row, [&assets, &account_id](soci::row &row) {
+        auto result = fromResult(makeAccountAsset(
+            account_id, row.get<std::string>(1), row.get<std::string>(2)));
+        if (result) {


monadic bind?

l4l · 2018-07-04T10:21:21Z

irohad/ametsuchi/impl/storage_impl.cpp

+            soci::into(size), soci::use(name);
+
+        if (size == 0) {
+          std::string query = "CREATE DATABASE ";


CREATE DATABASE :dbname?

For some reason it does not work

l4l · 2018-07-04T10:28:19Z

test/module/irohad/ametsuchi/CMakeLists.txt

@@ -57,6 +57,5 @@ target_link_libraries(postgres_options_test

 add_library(ametsuchi_fixture INTERFACE)
 target_link_libraries(ametsuchi_fixture INTERFACE
-    pqxx


seems soci still need to be here

l4l · 2018-07-04T10:30:52Z

irohad/ametsuchi/impl/storage_impl.hpp

@@ -20,9 +20,10 @@

 #include "ametsuchi/storage.hpp"

+#include <soci/postgresql/soci-postgresql.h>


Signed-off-by: Victor Drobny <[email protected]>

vdrobnyi · 2018-07-02T13:29:54Z

irohad/ametsuchi/impl/postgres_block_query.cpp

+      sql_ << "SELECT height FROM height_by_hash WHERE hash = :hash",
+          soci::into(block_str), soci::use(hash_str);
+      if (block_str) {
+        blockId = std::stoull(block_str.get());


It fails. Height is stored as a string in postgres. Maybe it's better to store it as an integer?

vdrobnyi · 2018-07-04T14:45:04Z

irohad/ametsuchi/impl/postgres_block_index.cpp

-                        + transaction_.quote(command.assetId()) + ", "
-                        + transaction_.quote(index) + ");");
-                    status &= res != boost::none;
+                    soci::statement st =


No, comma has lower priority

vdrobnyi · 2018-07-04T14:52:27Z

irohad/ametsuchi/impl/postgres_block_index.cpp

@@ -94,26 +106,31 @@ namespace iroha {
          block.transactions() | boost::adaptors::indexed(0),
          [&](const auto &tx) {
            const auto &creator_id = tx.value().creatorAccountId();
-            const auto &hash = tx.value().hash().blob();
+            const auto &hash = tx.value().hash().hex();


Blob in postgres is designed for large files. I'm not sure that it is necessary here.

vdrobnyi · 2018-07-04T15:16:09Z

irohad/ametsuchi/impl/postgres_wsv_common.hpp

@@ -151,24 +163,24 @@ namespace iroha {

    static inline shared_model::builder::BuilderResult<
        shared_model::interface::Peer>
-    makePeer(const pqxx::row &row) noexcept {
+    makePeer(const soci::row &row) noexcept {


It simplifies code in getPeers() in wsv query, but it is not possible to use soci::row everywhere here.

vdrobnyi · 2018-07-04T16:02:44Z

irohad/ametsuchi/impl/storage_impl.cpp

+            soci::into(size), soci::use(name);
+
+        if (size == 0) {
+          std::string query = "CREATE DATABASE ";


For some reason it does not work

Signed-off-by: Victor Drobny <[email protected]>

Issues addressed.

Signed-off-by: Victor Drobny <[email protected]>

l4l · 2018-07-05T11:21:11Z

irohad/ametsuchi/impl/postgres_wsv_command.cpp

+        const auto &str =
+            shared_model::proto::permissions::toString(permissions);
+        const auto perm_debug_str =
+            std::accumulate(str.begin(), str.end(), std::string());


That would be barely readable because it doesn't have spaces, e.g would look like "can_add_peercan_create_domaincan_create_user"

l4l · 2018-07-05T11:22:14Z

irohad/ametsuchi/impl/postgres_wsv_command.cpp

-      st.exchange(soci::use(empty_json));
-      st.exchange(soci::use(filled_json));
-      st.exchange(soci::use(value));
+      st.exchange(soci::use(std::string("{" + creator_account_id + "}")));


operator""s might be handy, anyway for now is also ok

l4l · 2018-07-05T11:25:01Z

irohad/ametsuchi/impl/temporary_wsv_impl.cpp

-      return result;
+      *sql_ <<"SAVEPOINT savepoint_";
+
+      return apply_function(tx, *wsv_) | [this, &execute_command,


looks unformatted for me

l4l · 2018-07-05T11:29:39Z

irohad/ametsuchi/impl/temporary_wsv_impl.cpp

+                          [&cmd_error](
+                              expected::Error<validation::CommandError>
+                                  &error) {
+                            cmd_error = error.error;


use std::move

l4l · 2018-07-05T11:31:11Z

irohad/ametsuchi/impl/postgres_ordering_service_persistent_state.cpp

-                   return false;
-                 });
+      *sql_ << "DROP TABLE IF EXISTS ordering_service_state";
+      return true;


False case again

l4l · 2018-07-05T11:31:19Z

irohad/ametsuchi/impl/postgres_ordering_service_persistent_state.cpp

+      *sql_ << "DELETE FROM ordering_service_state";
+      *sql_ << "INSERT INTO ordering_service_state VALUES (:height)",
+          soci::use(height);
+      return true;


False case again

Signed-off-by: Victor Drobny <[email protected]>

issues fixed

l4l · 2018-07-06T09:08:39Z

irohad/ametsuchi/impl/postgres_ordering_service_persistent_state.cpp

@@ -17,22 +17,40 @@

 #include "ametsuchi/impl/postgres_ordering_service_persistent_state.hpp"

+#include <soci/postgresql/soci-postgresql.h>


should be below :(

l4l · 2018-07-06T09:13:31Z

irohad/ametsuchi/impl/postgres_wsv_command.cpp

-            std::accumulate(str.begin(), str.end(), std::string());
+        std::string perm_debug_str;
+        std::for_each(
+            str.begin(), str.end(), [&perm_debug_str](const auto &elem) {


use std::string perm_debug_str = std::acumulate(str.begin(), str.end(), std::string(), [](auto acc, const auto &elem) { return acc + " " + elem; }); instead

l4l

I have failing test, please update the branch, maybe it will help

The following tests FAILED:
3 - module_wsv_query_command_test (Failed)
7 - module_kv_storage_test (Failed)
118 - integration_grant_permission_test (Failed)
126 - integration_set_account_detail_test (Failed)

Signed-off-by: Victor Drobny <[email protected]>

Issues fixed

l4l

LGTM

Signed-off-by: Andrei Lebedev <[email protected]>

sorabot · 2018-07-13T07:30:16Z

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#L79: The class 'BlockQueryTransferTest' defines member variable with name 'sql' also defined in its parent class 'AmetsuchiTest'.
ametsuchi_fixture.hpp#L79: The class 'WsvQueryCommandTest' defines member variable with name 'sql' also defined in its parent class 'AmetsuchiTest'.
storage_init_test.cpp#L21: Variable 'pg_opt_without_dbname_' is assigned in constructor body. Consider performing initialization in initialization list.

Pqxx replaced by SOCI. Some bugs are fixed by Andrei Signed-off-by: Victor Drobny <[email protected]>

Pqxx replaced by SOCI. Some bugs are fixed by Andrei Signed-off-by: victordrobny <[email protected]>

vdrobnyi added 7 commits June 22, 2018 10:18

soci in progress

441db4c

Signed-off-by: Victor Drobny <[email protected]>

soci integration WIP

b8a4e40

Signed-off-by: Victor Drobny <[email protected]>

rollback

d21d48e

Signed-off-by: Victor Drobny <[email protected]>

postgres_ordering_service_persistent_state done

7672c8f

Signed-off-by: Victor Drobny <[email protected]>

block index/query soci

6d5c8b0

Signed-off-by: Victor Drobny <[email protected]>

remove pqxx

21cbc80

Signed-off-by: Victor Drobny <[email protected]>

refactor selects, clang-format

b7b79b0

Signed-off-by: Victor Drobny <[email protected]>

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

vdrobnyi and others added 5 commits June 29, 2018 09:26

merge from develop

ed7d86a

Signed-off-by: Victor Drobny <[email protected]>

fix sql

9498394

Signed-off-by: Victor Drobny <[email protected]>

remove std::cout output

16160e1

Signed-off-by: Victor Drobny <[email protected]>

Merge branch 'develop' into feature/soci

47ef28a

vdrobnyi requested review from nickaleks and lebdron July 2, 2018 07:22

vdrobnyi changed the title ~~Feature/soci WIP~~ Feature/soci Jul 2, 2018

vdrobnyi added 2 commits July 2, 2018 11:44

refactor soci multiple selection

959fe32

Signed-off-by: Victor Drobny <[email protected]>

Merge branch 'feature/soci' of https://github.com/hyperledger/iroha i…

0ee8837

…nto feature/soci

nickaleks suggested changes Jul 2, 2018

View reviewed changes

vdrobnyi added 2 commits July 2, 2018 16:30

fix pr issues

49d6766

Signed-off-by: Victor Drobny <[email protected]>

fix block index

6f8430d

Signed-off-by: Victor Drobny <[email protected]>

nickaleks suggested changes Jul 3, 2018

View reviewed changes

fix message passing

a46b7d9

Signed-off-by: Victor Drobny <[email protected]>

nickaleks previously requested changes Jul 4, 2018

View reviewed changes

l4l previously requested changes Jul 4, 2018

View reviewed changes

vdrobnyi added 3 commits July 4, 2018 16:04

fix issues

fee1dc4

Signed-off-by: Victor Drobny <[email protected]>

merge develop

44334ca

Signed-off-by: Victor Drobny <[email protected]>

fix issues

f3f7fb1

Signed-off-by: Victor Drobny <[email protected]>

vdrobnyi commented Jul 4, 2018

View reviewed changes

merge develop

515167c

Signed-off-by: Victor Drobny <[email protected]>

lebdron requested a review from nickaleks July 5, 2018 07:20

make constructor explicit

957c768

Signed-off-by: Victor Drobny <[email protected]>

l4l previously requested changes Jul 5, 2018

View reviewed changes

nickaleks approved these changes Jul 5, 2018

View reviewed changes

fix issues

c24e10b

Signed-off-by: Victor Drobny <[email protected]>

l4l approved these changes Jul 6, 2018

View reviewed changes

l4l removed the needs-correction pr/rfc is not completed and might be updated label Jul 6, 2018

l4l previously requested changes Jul 6, 2018

View reviewed changes

vdrobnyi added 2 commits July 6, 2018 12:58

fix pr issues

895a495

Signed-off-by: Victor Drobny <[email protected]>

merge develop

061b06e

Signed-off-by: Victor Drobny <[email protected]>

l4l approved these changes Jul 6, 2018

View reviewed changes

Fix quorum type and amount get

563eed1

Signed-off-by: Andrei Lebedev <[email protected]>

lebdron force-pushed the feature/soci branch from 1921b12 to 563eed1 Compare July 12, 2018 17:16

Merge branch 'develop' into feature/soci

1bb1ba3

vdrobnyi merged commit 061a3a2 into develop Jul 13, 2018

vdrobnyi deleted the feature/soci branch July 13, 2018 09:52

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

Feature/soci (#1505)

473a994

Pqxx replaced by SOCI. Some bugs are fixed by Andrei Signed-off-by: Victor Drobny <[email protected]>

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

Feature/soci (#1505)

5ff4faa

Pqxx replaced by SOCI. Some bugs are fixed by Andrei Signed-off-by: Victor Drobny <[email protected]>

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

Feature/soci (#1505)

1600005

Pqxx replaced by SOCI. Some bugs are fixed by Andrei Signed-off-by: Victor Drobny <[email protected]>

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

Feature/soci (#1505)

9d12009

Pqxx replaced by SOCI. Some bugs are fixed by Andrei Signed-off-by: Victor Drobny <[email protected]>

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

Feature/soci (#1505)

77be9df

Pqxx replaced by SOCI. Some bugs are fixed by Andrei Signed-off-by: Victor Drobny <[email protected]>

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

Feature/soci (#1505)

8a1f5c2

Pqxx replaced by SOCI. Some bugs are fixed by Andrei Signed-off-by: victordrobny <[email protected]>

		@@ -36,6 +36,8 @@ pghost=127.0.0.1

		pgid=$(docker run -p $pghost:$pgport:$pgport --rm --name $pgname -e POSTGRES_USER=$user -e POSTGRES_PASSSWORD=$password -d postgres:9.5)

		docker run -p 127.0.0.1:5432:5432 --rm --name postgrestestimage -e POSTGRES_USER=postgres -e POSTGRES_PASSSWORD=mysecretpassword -d postgres:9.5

		@@ -20,9 +20,10 @@

		#include "ametsuchi/storage.hpp"

		#include <soci/postgresql/soci-postgresql.h>

		@@ -17,22 +17,40 @@

		#include "ametsuchi/impl/postgres_ordering_service_persistent_state.hpp"

		#include <soci/postgresql/soci-postgresql.h>

Feature/soci #1505

Feature/soci #1505

Conversation

vdrobnyi commented Jun 28, 2018 • edited Loading

Description of the Change

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 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

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 Jul 4, 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

Choose a reason for hiding this comment

l4l left a comment

Choose a reason for hiding this comment

l4l left a comment

Choose a reason for hiding this comment

sorabot commented Jul 13, 2018

vdrobnyi commented Jun 28, 2018 •

edited

Loading

l4l Jul 4, 2018 •

edited

Loading