Merge lp:~thomas-voss/unity-scopes-api/leverage-net-cpp-streaming-api into lp:unity-scopes-api

[Thomas Voß]

Leverage the streaming API introduced in net-cpp 1.2.0.

Leverage the streaming API introduced in net-cpp 1.2.0.

[lp:~thomas-voss/unity-scopes-api/leverage-net-cpp-streaming-api]

325. By Thomas Voß on 2015-04-02

Clean up commented out code.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:325
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-ci/607/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-vivid-amd64-ci/141/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-vivid-armhf-ci/132/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-vivid-i386-ci/133/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-ci/607/rebuild

review: Needs Fixing (continuous-integration)

[Michi Henning (michihenning)]

Looks good to me, thanks! How do we get the missing dependency into CI?

Marcus, could you look this over too please? You know that code much better than me.

review: Approve

[Marcus Tomlinson (marcustomlinson)]

In theory this is correct, however the SSS does not currently send chunked data in complete lines (as we are assuming here, and rightfully so). The SSS needs to be fixed to respond in clean chucks, each terminated by "\r\n", before we can go ahead and merge this.

[Marcus Tomlinson (marcustomlinson)]

Marking rejected as this branch has been superseded.

[Marcus Tomlinson (marcustomlinson)]

Hi Thomas, could you please either delete this branch or change the status under "Branch information" to Abandoned. Thanks

[Marcus Tomlinson (marcustomlinson)]

review: Disapprove

Unmerged revisions

325. By Thomas Voß on 2015-04-02

Clean up commented out code.

324. By Thomas Voß on 2015-04-02

Move responsibility for interpreting chunks of data arriving from the smart scopes server to smart scopes client.
Rename parse_line to handle_line.
Introduce a function handle_chunk that splits up a given chunk line by line and feeds it to handle_line(...).

323. By Thomas Voß on 2015-04-01

Pass on the unmodified line arriving from the server.

322. By Thomas Voß on 2015-04-01

Do not hand the entire result to the line handler.

321. By Thomas Voß on 2015-04-01

Leverage the streaming API introduced in net-cpp 1.2.0.

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2015-02-13 09:20:20 +0000
+++ CMakeLists.txt	2015-04-02 10:40:26 +0000
@@ -70,7 +70,7 @@
 pkg_check_modules(LIBACCOUNTS libaccounts-glib REQUIRED)
 pkg_check_modules(LIBSIGNON libsignon-glib REQUIRED)
 pkg_check_modules(ZMQLIB libzmq REQUIRED)
-pkg_check_modules(NET_CPP net-cpp REQUIRED)
+pkg_check_modules(NET_CPP net-cpp>=1.2.0 REQUIRED)
 
 find_library(ZMQPPLIB zmqpp)
 if(NOT ZMQPPLIB)

=== modified file 'debian/control'
--- debian/control	2015-02-05 14:57:01 +0000
+++ debian/control	2015-04-02 10:40:26 +0000
@@ -25,7 +25,7 @@
                libdbustest1-dev,
                libjsoncpp-dev,
                liblttng-ust-dev,
-               libnet-cpp-dev,
+               libnet-cpp-dev (>= 1.2.0),
                libprocess-cpp-dev (>= 1.0.1),
                libsignon-glib-dev,
                libunity-api-dev (>= 7.80.7~),

=== modified file 'include/unity/scopes/internal/smartscopes/HttpClientNetCpp.h'
--- include/unity/scopes/internal/smartscopes/HttpClientNetCpp.h	2015-01-16 06:13:57 +0000
+++ include/unity/scopes/internal/smartscopes/HttpClientNetCpp.h	2015-04-02 10:40:26 +0000
@@ -30,7 +30,7 @@
 {
 namespace http
 {
-class Client;
+class StreamingClient;
 }
 }
 }
@@ -63,7 +63,7 @@
     void cancel_get(unsigned int session_id) override;
 
     unsigned int no_reply_timeout;
-    std::shared_ptr<core::net::http::Client> client;
+    std::shared_ptr<core::net::http::StreamingClient> client;
     std::thread worker;
 };
 

=== modified file 'include/unity/scopes/internal/smartscopes/SmartScopesClient.h'
--- include/unity/scopes/internal/smartscopes/SmartScopesClient.h	2015-01-28 09:03:59 +0000
+++ include/unity/scopes/internal/smartscopes/SmartScopesClient.h	2015-04-02 10:40:26 +0000
@@ -215,8 +215,10 @@
     std::shared_ptr<DepartmentInfo> parse_departments(JsonNodeInterface::SPtr node);
     Filters parse_filters(JsonNodeInterface::SPtr node);
     FilterState parse_filter_state(JsonNodeInterface::SPtr node);
-    void parse_line(std::string const& json, SearchReplyHandler const& handler);
-    void parse_line(std::string const& json, PreviewReplyHandler const& handler);
+
+    void handle_chunk(const std::string& chunk, std::function<void(const std::string&)> line_handler);
+    void handle_line(std::string const& json, SearchReplyHandler const& handler);
+    void handle_line(std::string const& json, PreviewReplyHandler const& handler);
 
     std::vector<std::string> extract_json_stream(std::string const& json_stream);
 

=== modified file 'src/scopes/internal/smartscopes/HttpClientNetCpp.cpp'
--- src/scopes/internal/smartscopes/HttpClientNetCpp.cpp	2015-01-19 09:15:06 +0000
+++ src/scopes/internal/smartscopes/HttpClientNetCpp.cpp	2015-04-02 10:40:26 +0000
@@ -20,8 +20,8 @@
 
 #include <unity/UnityExceptions.h>
 
-#include <core/net/http/client.h>
-#include <core/net/http/request.h>
+#include <core/net/http/streaming_client.h>
+#include <core/net/http/streaming_request.h>
 #include <core/net/http/response.h>
 #include <core/net/http/status.h>
 
@@ -102,7 +102,7 @@
 
 HttpClientNetCpp::HttpClientNetCpp(unsigned int no_reply_timeout)
     : no_reply_timeout{no_reply_timeout},
-      client{http::make_client()},
+      client{http::make_streaming_client()},
       worker([this]() { client->run(); })
 {
 }
@@ -129,7 +129,7 @@
     }
     http_config.header = http_header;
 
-    auto request = client->get(http_config);
+    auto request = client->streaming_get(http_config);
     request->set_timeout(std::chrono::milliseconds{no_reply_timeout});
 
     auto promise = std::make_shared<std::promise<void>>();
@@ -157,12 +157,6 @@
                         }
                         else
                         {
-                            std::istringstream in(response.body);
-                            std::string line;
-                            while (std::getline(in, line))
-                            {
-                                line_data(line);
-                            }
                             promise->set_value();
                         }
                     })
@@ -170,7 +164,11 @@
                     {
                         unity::ResourceException re(e.what());
                         promise->set_exception(std::make_exception_ptr(re));
-                    }));
+                    }),
+                [line_data](const std::string& const_data)
+                {    
+                    line_data(const_data);                     
+                });
 
     return std::make_shared<HttpResponseHandle>(
                 shared_from_this(),

=== modified file 'src/scopes/internal/smartscopes/SmartScopesClient.cpp'
--- src/scopes/internal/smartscopes/SmartScopesClient.cpp	2015-01-28 09:03:59 +0000
+++ src/scopes/internal/smartscopes/SmartScopesClient.cpp	2015-04-02 10:40:26 +0000
@@ -472,18 +472,12 @@
         headers.push_back(std::make_pair("User-Agent", user_agent_hdr));
     }
 
-    auto reponse_mutex = std::make_shared<std::mutex>();
-    query_results_[search_id] = http_client_->get(search_uri.str(), [this, handler, reponse_mutex](std::string const& line_data)
+    query_results_[search_id] = http_client_->get(search_uri.str(), [this, handler](std::string const& chunk)
     {
-        std::lock_guard<std::mutex> lock(*reponse_mutex);
-        try
-        {
-            parse_line(line_data, handler);
-        }
-        catch (std::exception const &e)
-        {
-            BOOST_LOG_SEV(logger_, Logger::Error) << "SmartScopesClient.search(): Failed to parse: " << e.what();
-        }
+        handle_chunk(chunk, [this, handler](const std::string& line)
+        {
+            handle_line(line, handler);
+        });
     }, headers);
 
     return SearchHandle::UPtr(new SearchHandle(search_id, shared_from_this()));
@@ -540,24 +534,42 @@
 
     BOOST_LOG_SEV(logger_, Logger::Info) << "SmartScopesClient.preview(): GET " << preview_uri.str();
 
-    auto reponse_mutex = std::make_shared<std::mutex>();
-    query_results_[preview_id] = http_client_->get(preview_uri.str(), [this, handler, reponse_mutex](std::string const& line_data)
+    query_results_[preview_id] = http_client_->get(preview_uri.str(), [this, handler](std::string const& chunk)
     {
-        std::lock_guard<std::mutex> lock(*reponse_mutex);
-        try
-        {
-            parse_line(line_data, handler);
-        }
-        catch (std::exception const &e)
-        {
-            BOOST_LOG_SEV(logger_, Logger::Error) << "SmartScopesClient.preview(): Failed to parse: " << e.what();
-        }
+        handle_chunk(chunk, [this, handler](const std::string& line)
+        {
+            handle_line(line, handler);
+        });        
     }, headers);
 
     return PreviewHandle::UPtr(new PreviewHandle(preview_id, shared_from_this()));
 }
 
-void SmartScopesClient::parse_line(std::string const& json, PreviewReplyHandler const& handler)
+void SmartScopesClient::handle_chunk(const std::string& chunk, std::function<void(const std::string&)> line_handler)
+{
+    // According to the docs, we expect: 
+    // The response will have Content-Type
+    // application/json, it will be a chunked response, in practice a series of
+    // “\r\n” delimited lines, each containing one JSON object, with the
+    // possible forms, matching what currently can be pushed into a reply in the
+    // new scopes API
+    static constexpr const char separator{'\n'};
+
+    std::istringstream ss{chunk}; std::string line;
+    while (std::getline(ss, line, separator))
+    {
+        try
+        {
+            line_handler(line);
+        }
+        catch (std::exception const &e)
+        {
+            BOOST_LOG_SEV(logger_, Logger::Error) << "SmartScopesClient.handle_chunk(): Failed to parse line: " << e.what();
+        }
+    }        
+}
+
+void SmartScopesClient::handle_line(std::string const& json, PreviewReplyHandler const& handler)
 {
     JsonNodeInterface::SPtr root_node;
     JsonNodeInterface::SPtr child_node;
@@ -605,7 +617,7 @@
     }
 }
 
-void SmartScopesClient::parse_line(std::string const& json, SearchReplyHandler const& handler)
+void SmartScopesClient::handle_line(std::string const& json, SearchReplyHandler const& handler)
 {
     JsonNodeInterface::SPtr root_node;
     JsonNodeInterface::SPtr child_node;
@@ -667,7 +679,7 @@
             }
         }
         handler.result_handler(result);
-    }
+    }    
     else if (root_node->has_node("departments"))
     {
         auto departments = parse_departments(root_node->get_node("departments"));
@@ -720,6 +732,7 @@
 std::shared_ptr<DepartmentInfo> SmartScopesClient::parse_departments(JsonNodeInterface::SPtr node)
 {
     static std::array<std::string, 2> const mandatory = { { "label", "canned_query" } };
+    
     for (auto const& field : mandatory)
     {
         if (!node->has_node(field))