Introduce queries pagination #1927

nickaleks · 2018-12-07T13:56:43Z

Description of the Change

This Pull Request adds pagination to several queries. This allows to work on large data sets without returning all the data in a single request

Benefits

Better interface for queries

Possible Drawbacks

Nonne

* Add transactions page response Signed-off-by: kamilsa <kamilsa16@gmail.com>

* tx pagination in proto * shared_model::interface::TxPaginationMeta class * shared_model::proto::TxPaginationMeta class * make PrettyStringBuilder constants static * fix validators module tests * added pagination_meta to queries' toString() Signed-off-by: Mikhail Boldyrev <miboldyrev@gmail.com>

* FieldValidator::validateTxPaginationMeta * validation::TxPaginationMetaValidator * updated the paged query building Signed-off-by: Mikhail Boldyrev <miboldyrev@gmail.com>

merged commits add: * Pimpl for QueryResponse * ITF: PortGuard Signed-off-by: Mikhail Boldyrev <miboldyrev@gmail.com>

* removed TxPaginationMetaValidator * constref first_hash Signed-off-by: Mikhail Boldyrev <miboldyrev@gmail.com>

Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com>

…or_test (#1915) * postgres_query_executor_test: common tx pagination tests Signed-off-by: Mikhail Boldyrev <miboldyrev@gmail.com>

* Add documentation for queries pagination Signed-off-by: kamilsa <kamilsa16@gmail.com>

…/tx-queries-pagination

…rledger/iroha into trunk/tx-queries-pagination Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com>

Akvinikym · 2018-12-10T06:48:00Z

irohad/ametsuchi/impl/postgres_query_executor.cpp

+                            % first_hash->hex())
+                               .str();
+              // TODO: IR-82 nickaleks 7.12.18
+              // add status code for invalid pagination


Remove TODO please

…/tx-queries-pagination

lebdron

As discussed offline, the issue with bindings can be ignored, but a warning note will definitely be useful.

lebdron · 2018-12-10T07:33:53Z

shared_model/backend/protobuf/query_responses/proto_transaction_page_response.hpp

+#ifndef IROHA_SHARED_MODEL_PROTO_TRANSACTION_PAGE_RESPONSE_HPP
+#define IROHA_SHARED_MODEL_PROTO_TRANSACTION_PAGE_RESPONSE_HPP
+
+#include "backend/protobuf/common_objects/trivial_proto.hpp"


#include "interfaces/query_responses/transactions_page_response.hpp" #include "backend/protobuf/common_objects/trivial_proto.hpp" #include "backend/protobuf/transaction.hpp" #include "interfaces/common_objects/types.hpp" #include "qry_responses.pb.h"

lebdron · 2018-12-10T07:34:23Z

shared_model/backend/protobuf/query_responses/proto_transaction_page_response.hpp

+
+namespace shared_model {
+  namespace proto {
+    class TransactionsPageResponse final


File has to be renamed to proto_transactions_page_response.hpp

lebdron · 2018-12-10T08:02:34Z

shared_model/interfaces/iroha_internal/query_response_factory.hpp

@@ -124,6 +124,34 @@ namespace shared_model {
              transactions,
          const crypto::Hash &query_hash) const = 0;

+      /**
+       * Create response for transactions pagination query
+       * @param transactions to be inserted into the response


Is it really necessary to list the parameters with copied "x to be inserted into the response"? Please add some description to fields, or merge them into something like "Create response for transactions pagination query with given transactions, next_tx_hash..."
Same for method below.

lebdron · 2018-12-10T09:11:53Z

shared_model/interfaces/queries/impl/tx_pagination_meta.cpp

+                            .append("page_size", std::to_string(pageSize()));
+  auto first_tx_hash = firstTxHash();
+  if (first_tx_hash) {
+    pretty_builder = std::move(


String builder is mutable, so there is no need to reassign it. append should be enough.

lebdron · 2018-12-10T10:12:59Z

shared_model/interfaces/query_responses/transactions_page_response.hpp

+        : public ModelPrimitive<TransactionsPageResponse> {
+     public:
+      /**
+       * @return Attached transactions


Why does this method require a doc while other methods do not?

lebdron · 2018-12-12T08:27:47Z

shared_model/backend/protobuf/query_responses/impl/proto_transaction_page_response.cpp

+        : CopyableProto(std::forward<QueryResponseType>(queryResponse)),
+          transactionPageResponse_{proto_->transactions_page_response()},
+          transactions_{[this] {
+            return std::vector<proto::Transaction>(


Looks like lambda can be removed.

lebdron · 2018-12-12T09:10:09Z

shared_model/bindings/model_query_builder.cpp

-      return ModelQueryBuilder(builder_.getAccountTransactions(account_id));
+        const interface::types::AccountIdType &account_id,
+        interface::types::TransactionsNumberType page_size,
+        const boost::optional<interface::types::HashType> &first_hash) {


It is highly likely that optional is not wrapped with SWIG in an expected way. Please check java bindings and comment if it was successful or not.

lebdron · 2018-12-12T09:12:38Z

shared_model/interfaces/query_responses/impl/transactions_page_response.cpp

+    }
+
+    bool TransactionsPageResponse::operator==(const ModelType &rhs) const {
+      return transactions() == rhs.transactions();


Why is it not required to also compare nextTxHash and allTransactionsSize?

lebdron · 2018-12-12T09:31:18Z

test/module/irohad/ametsuchi/postgres_query_executor_test.cpp

+
+  // old compilers fail to deduce that a variant contains reference
+  template <typename T, typename Variant>
+  ConstRef<T> getRef(Variant &&v) {


Method is unused, please remove.

lebdron · 2018-12-12T09:31:34Z

test/module/irohad/ametsuchi/postgres_query_executor_test.cpp

+                .build());
+
+  template <typename T>
+  using ConstRef = const T &;


Same with unused, please remove.

Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com>

…/tx-queries-pagination

kamilsa and others added 12 commits November 30, 2018 11:52

Add transactions page response (#1882)

0545bee

* Add transactions page response Signed-off-by: kamilsa <kamilsa16@gmail.com>

Feature/update qry response factory (#1884)

7c64bb0

* Add transactions page response Signed-off-by: kamilsa <kamilsa16@gmail.com>

Transactins pagination in queries, part 2 (#1896)

4792972

* FieldValidator::validateTxPaginationMeta * validation::TxPaginationMetaValidator * updated the paged query building Signed-off-by: Mikhail Boldyrev <miboldyrev@gmail.com>

Merged dev to transaction query pagination tunk (#1900)

3facaa9

merged commits add: * Pimpl for QueryResponse * ITF: PortGuard Signed-off-by: Mikhail Boldyrev <miboldyrev@gmail.com>

Fix transaction queries pagination (#1904)

8055cd2

* removed TxPaginationMetaValidator * constref first_hash Signed-off-by: Mikhail Boldyrev <miboldyrev@gmail.com>

GetAccountTransactions Pagination (#1903)

d6eaaf4

Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com>

Get Account Asset transactions pagination (#1917)

5071cd4

Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com>

Testing: common transaction pagination tests in postgres_query_execut…

98eb0a3

…or_test (#1915) * postgres_query_executor_test: common tx pagination tests Signed-off-by: Mikhail Boldyrev <miboldyrev@gmail.com>

Add documentation for queries pagination (#1920)

eff9320

* Add documentation for queries pagination Signed-off-by: kamilsa <kamilsa16@gmail.com>

Merge branch 'dev' of https://github.com/hyperledger/iroha into trunk…

3f4ef53

…/tx-queries-pagination

Merge branch 'trunk/tx-queries-pagination' of https://github.com/hype…

7e9a1d8

…rledger/iroha into trunk/tx-queries-pagination Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com>

nickaleks requested review from Akvinikym and lebdron December 7, 2018 13:59

Akvinikym approved these changes Dec 10, 2018

View reviewed changes

Merge branch 'dev' of https://github.com/hyperledger/iroha into trunk…

a2e158b

…/tx-queries-pagination

lebdron approved these changes Dec 12, 2018

View reviewed changes

nickaleks added 3 commits December 12, 2018 12:39

fix tests compilation

b9b285b

Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com>

fix review issues

ded8393

Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com>

Merge branch 'dev' of https://github.com/hyperledger/iroha into trunk…

11d5d23

…/tx-queries-pagination

nickaleks merged commit 265ab6f into dev Dec 12, 2018

kamilsa mentioned this pull request Dec 12, 2018

Hyperledger Iroha v1.0 Release Candidate 1 #1941

Merged

lebdron deleted the trunk/tx-queries-pagination branch December 18, 2018 05:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce queries pagination #1927

Introduce queries pagination #1927

nickaleks commented Dec 7, 2018

Akvinikym Dec 10, 2018

lebdron left a comment

lebdron Dec 10, 2018

lebdron Dec 10, 2018

lebdron Dec 10, 2018

lebdron Dec 10, 2018

lebdron Dec 10, 2018

lebdron Dec 12, 2018

lebdron Dec 12, 2018

lebdron Dec 12, 2018

lebdron Dec 12, 2018

lebdron Dec 12, 2018

Introduce queries pagination #1927

Introduce queries pagination #1927

Conversation

nickaleks commented Dec 7, 2018

Description of the Change

Benefits

Possible Drawbacks

Choose a reason for hiding this comment

lebdron 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment