Merge lp:~stolowski/unity-scopes-api/user-agent into lp:unity-scopes-api/devel

Proposed by Paweł Stołowski
Status: Merged
Approved by: Michi Henning
Approved revision: 261
Merged at revision: 495
Proposed branch: lp:~stolowski/unity-scopes-api/user-agent
Merge into: lp:unity-scopes-api/devel
Prerequisite: lp:~michihenning/unity-scopes-api/fix-amd64-symbol
Diff against target: 383 lines (+113/-17)
11 files modified
STRUCTS (+19/-0)
include/unity/scopes/internal/smartscopes/HttpClientInterface.h (+6/-1)
include/unity/scopes/internal/smartscopes/HttpClientQt.h (+4/-2)
include/unity/scopes/internal/smartscopes/HttpClientQtThread.h (+4/-1)
include/unity/scopes/internal/smartscopes/SmartScopesClient.h (+3/-1)
src/scopes/internal/smartscopes/HttpClientQt.cpp (+7/-5)
src/scopes/internal/smartscopes/HttpClientQtThread.cpp (+6/-1)
src/scopes/internal/smartscopes/SmartScope.cpp (+16/-3)
src/scopes/internal/smartscopes/SmartScopesClient.cpp (+18/-2)
test/gtest/scopes/internal/smartscopes/SmartScopesClient/FakeSss.py (+3/-0)
test/gtest/scopes/internal/smartscopes/SmartScopesClient/SmartScopesClient_test.cpp (+27/-1)
To merge this branch: bzr merge lp:~stolowski/unity-scopes-api/user-agent
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
Michi Henning (community) Approve
Pete Woods Pending
Review via email: mp+235913@code.launchpad.net

This proposal supersedes a proposal from 2014-09-24.

Commit message

Pass User-Agent http header to SSS if recieved with SearchMetadata or PreviewMetadata.

Description of the change

Pass User-Agent http header to SSS if recieved with SearchMetadata or PreviewMetadata.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
Revision history for this message
Pete Woods (pete-woods) wrote : Posted in a previous version of this proposal

LGTM

review: Approve
Revision history for this message
Pete Woods (pete-woods) wrote : Posted in a previous version of this proposal

Apart from the symbols file stuff. This could be another example of the compiler producing amd64-only symbols.

i.e. (c++|arch=amd64) should be used as the symbol tag

Revision history for this message
Michi Henning (michihenning) wrote : Posted in a previous version of this proposal

BIG problem here: there is a redundant semicolon ;-)

247 - }
248 + };

I think the user-agent attribute should be documented in the STRUCTS file?

Other than that, this looks good!

review: Needs Fixing
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
Revision history for this message
Paweł Stołowski (stolowski) wrote : Posted in a previous version of this proposal

> BIG problem here: there is a redundant semicolon ;-)
>
> 247 - }
> 248 + };
>
> I think the user-agent attribute should be documented in the STRUCTS file?
>

Right, I noticed we were missing descriptions of the 3 *metadata structures that. I fixed that, however this particular value as well as e.g. session-id are transported using general hints container, and we don't really care what's there in the API, so these are not documented.

Revision history for this message
Paweł Stołowski (stolowski) wrote : Posted in a previous version of this proposal

I've merged Michi's symbols fix into this branch and am trying to resubmit it with new prerequisite branch, but LP gives me OOPses all the time. Setting to WIP till it succeeds.

Revision history for this message
Paweł Stołowski (stolowski) wrote :

Ok, it finally worked, it;s ready for review again.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Michi Henning (michihenning) wrote :

Looking good, thanks!

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'STRUCTS'
2--- STRUCTS 2014-06-26 05:59:38 +0000
3+++ STRUCTS 2014-09-25 08:50:05 +0000
4@@ -154,3 +154,22 @@
5 ============================================
6 'status' : int (corresponds to ActivationResponse::Status enum)
7 'scope_data' : variant
8+
9+QueryMetadata (returned by serialize())
10+=======================================
11+ 'type' : string (metadata type)
12+ 'locale' : string
13+ 'form_factor' : string
14+ 'hints' : dict
15+ 'internet_connectivity' : bool (optional)
16+
17+SearchMetadata (returned by serialize())
18+========================================
19+ all QueryMetadata attributes
20+ 'cardinality' : int
21+ 'location' : dict
22+
23+ActionMetadata (returned by serialize())
24+========================================
25+ all QueryMetadata attributes
26+ 'scope_data' : variant
27
28=== modified file 'include/unity/scopes/internal/smartscopes/HttpClientInterface.h'
29--- include/unity/scopes/internal/smartscopes/HttpClientInterface.h 2014-08-28 15:27:19 +0000
30+++ include/unity/scopes/internal/smartscopes/HttpClientInterface.h 2014-09-25 08:50:05 +0000
31@@ -25,6 +25,8 @@
32 #include <future>
33 #include <string>
34 #include <functional>
35+#include <list>
36+#include <tuple>
37
38 namespace unity
39 {
40@@ -40,6 +42,8 @@
41
42 class HttpResponseHandle;
43
44+typedef std::list<std::pair<std::string, std::string>> HttpHeaders;
45+
46 class HttpClientInterface : public std::enable_shared_from_this<HttpClientInterface>
47 {
48 public:
49@@ -50,7 +54,8 @@
50 virtual ~HttpClientInterface() = default;
51
52 virtual std::shared_ptr<HttpResponseHandle> get(std::string const& request_url,
53- std::function<void(std::string const&)> const& lineData = [](std::string const&) {}) = 0;
54+ std::function<void(std::string const&)> const& lineData = [](std::string const&) {},
55+ HttpHeaders const& headers = HttpHeaders()) = 0;
56
57 virtual std::string to_percent_encoding(std::string const& string) = 0;
58
59
60=== modified file 'include/unity/scopes/internal/smartscopes/HttpClientQt.h'
61--- include/unity/scopes/internal/smartscopes/HttpClientQt.h 2014-09-02 11:58:04 +0000
62+++ include/unity/scopes/internal/smartscopes/HttpClientQt.h 2014-09-25 08:50:05 +0000
63@@ -44,7 +44,8 @@
64 explicit HttpClientQt(unsigned int no_reply_timeout);
65 ~HttpClientQt();
66
67- HttpResponseHandle::SPtr get(std::string const& request_url, std::function<void(std::string const&)> const& lineData = [](std::string const&) {}) override;
68+ HttpResponseHandle::SPtr get(std::string const& request_url, std::function<void(std::string const&)> const& lineData = [](std::string const&) {},
69+ HttpHeaders const& headers = HttpHeaders()) override;
70
71 std::string to_percent_encoding(std::string const& string) override;
72
73@@ -55,7 +56,8 @@
74 class HttpSession
75 {
76 public:
77- HttpSession(std::string const& request_url, unsigned int timeout, std::function<void(std::string const&)> const& lineData);
78+ HttpSession(std::string const& request_url, unsigned int timeout, std::function<void(std::string const&)> const& lineData, HttpHeaders const& headers =
79+ HttpHeaders());
80 ~HttpSession();
81
82 std::future<void> get_future();
83
84=== modified file 'include/unity/scopes/internal/smartscopes/HttpClientQtThread.h'
85--- include/unity/scopes/internal/smartscopes/HttpClientQtThread.h 2014-08-26 12:17:18 +0000
86+++ include/unity/scopes/internal/smartscopes/HttpClientQtThread.h 2014-09-25 08:50:05 +0000
87@@ -36,6 +36,7 @@
88 #include <QUrl>
89 #endif
90
91+#include <unity/scopes/internal/smartscopes/HttpClientInterface.h>
92 #include <unity/util/NonCopyable.h>
93
94 #include <mutex>
95@@ -62,7 +63,8 @@
96 public:
97 NONCOPYABLE(HttpClientQtThread);
98
99- HttpClientQtThread(const QUrl& url, unsigned int timeout, std::function<void(std::string const&)> const& lineData);
100+ HttpClientQtThread(const QUrl& url, unsigned int timeout, std::function<void(std::string const&)> const& lineData, const HttpHeaders& headers =
101+ HttpHeaders());
102 ~HttpClientQtThread();
103
104 bool get_reply(std::string& reply);
105@@ -82,6 +84,7 @@
106 private:
107 QUrl url_;
108 const std::function<void(std::string const&)> lineDataCallback_;
109+ HttpHeaders headers_;
110 unsigned int timeout_;
111 std::mutex reply_mutex_;
112
113
114=== modified file 'include/unity/scopes/internal/smartscopes/SmartScopesClient.h'
115--- include/unity/scopes/internal/smartscopes/SmartScopesClient.h 2014-09-03 14:34:14 +0000
116+++ include/unity/scopes/internal/smartscopes/SmartScopesClient.h 2014-09-25 08:50:05 +0000
117@@ -174,6 +174,7 @@
118 VariantMap const& filter_state = VariantMap(),
119 std::string const& locale = "",
120 std::string const& country = "",
121+ std::string const& user_agent_hdr = "",
122 const unsigned int limit = 0);
123
124 PreviewHandle::UPtr preview(PreviewReplyHandler const& handler,
125@@ -184,7 +185,8 @@
126 const unsigned int widgets_api_version,
127 VariantMap const& settings = VariantMap(),
128 std::string const& locale = "",
129- std::string const& country = "");
130+ std::string const& country = "",
131+ std::string const& user_agent_hdr = "");
132
133 private:
134 friend class SearchHandle;
135
136=== modified file 'src/scopes/internal/smartscopes/HttpClientQt.cpp'
137--- src/scopes/internal/smartscopes/HttpClientQt.cpp 2014-09-02 11:58:04 +0000
138+++ src/scopes/internal/smartscopes/HttpClientQt.cpp 2014-09-25 08:50:05 +0000
139@@ -48,12 +48,13 @@
140 {
141 }
142
143-HttpResponseHandle::SPtr HttpClientQt::get(std::string const& request_url, std::function<void(std::string const&)> const& lineData)
144+HttpResponseHandle::SPtr HttpClientQt::get(std::string const& request_url, std::function<void(std::string const&)> const& lineData,
145+ HttpHeaders const& headers)
146 {
147 std::lock_guard<std::mutex> lock(sessions_mutex_);
148
149 // start new session
150- auto session = std::make_shared<HttpSession>(request_url, no_reply_timeout_, lineData);
151+ auto session = std::make_shared<HttpSession>(request_url, no_reply_timeout_, lineData, headers);
152 sessions_[session_index_] = session;
153
154 return std::make_shared<HttpResponseHandle>(shared_from_this(), session_index_++, session->get_future());
155@@ -79,17 +80,18 @@
156
157 //-- HttpClientQt::HttpSession
158
159-HttpClientQt::HttpSession::HttpSession(std::string const& request_url, uint timeout, std::function<void(std::string const&)> const& lineData)
160+HttpClientQt::HttpSession::HttpSession(std::string const& request_url, uint timeout, std::function<void(std::string const&)> const& lineData,
161+ HttpHeaders const& headers)
162 : qt_thread_(nullptr)
163 {
164 get_thread_ =
165- std::thread([this, request_url, timeout, lineData]()
166+ std::thread([this, request_url, headers, timeout, lineData]()
167 {
168 QUrl url(request_url.c_str());
169
170 {
171 std::lock_guard<std::mutex> lock(qt_thread_mutex_);
172- qt_thread_ = std::unique_ptr<HttpClientQtThread>(new HttpClientQtThread(url, timeout, lineData));
173+ qt_thread_ = std::unique_ptr<HttpClientQtThread>(new HttpClientQtThread(url, timeout, lineData, headers));
174 }
175
176 QEventLoop loop;
177
178=== modified file 'src/scopes/internal/smartscopes/HttpClientQtThread.cpp'
179--- src/scopes/internal/smartscopes/HttpClientQtThread.cpp 2014-08-28 15:51:48 +0000
180+++ src/scopes/internal/smartscopes/HttpClientQtThread.cpp 2014-09-25 08:50:05 +0000
181@@ -38,10 +38,11 @@
182 namespace smartscopes
183 {
184
185-HttpClientQtThread::HttpClientQtThread(const QUrl& url, uint timeout, std::function<void(std::string const&)> const& lineData)
186+HttpClientQtThread::HttpClientQtThread(const QUrl& url, uint timeout, std::function<void(std::string const&)> const& lineData, HttpHeaders const& headers)
187 : QThread()
188 , url_(url)
189 , lineDataCallback_(lineData)
190+ , headers_(headers)
191 , timeout_(timeout)
192 , success_(false)
193 {
194@@ -67,6 +68,10 @@
195 QNetworkAccessManager* manager = new QNetworkAccessManager();
196
197 QNetworkRequest request(url_);
198+ for (auto const& hdr: headers_)
199+ {
200+ request.setRawHeader(QString::fromStdString(hdr.first).toUtf8(), QString::fromStdString(hdr.second).toUtf8());
201+ }
202
203 QNetworkReply* reply = manager->get(request);
204 reply->setReadBufferSize(0); // unlimited buffer
205
206=== modified file 'src/scopes/internal/smartscopes/SmartScope.cpp'
207--- src/scopes/internal/smartscopes/SmartScope.cpp 2014-09-03 14:34:14 +0000
208+++ src/scopes/internal/smartscopes/SmartScope.cpp 2014-09-25 08:50:05 +0000
209@@ -163,6 +163,7 @@
210 ///! TODO: country (+location data)
211 int query_id = 0;
212 std::string session_id;
213+ std::string agent;
214 auto const metadata = search_metadata();
215 if (metadata.contains_hint("session-id") && metadata["session-id"].which() == Variant::String)
216 {
217@@ -181,8 +182,13 @@
218 {
219 std::cout << "SmartScope: missing or invalid query id for \"" << scope_id_ << "\": \"" << query_.query_string() << "\"" << std::endl;
220 }
221+ if (metadata.contains_hint("user-agent") && metadata["user-agent"].which() == Variant::String)
222+ {
223+ agent = metadata["user-agent"].get_string();
224+ }
225
226- search_handle_ = ss_client_->search(handler, base_url_, query_.query_string(), query_.department_id(), session_id, query_id, hints_.form_factor(), settings(), query_.filter_state().serialize(), hints_.locale(), "", hints_.cardinality());
227+ search_handle_ = ss_client_->search(handler, base_url_, query_.query_string(), query_.department_id(), session_id, query_id, hints_.form_factor(),
228+ settings(), query_.filter_state().serialize(), hints_.locale(), "", agent, hints_.cardinality());
229 search_handle_->wait();
230
231 std::cout << "SmartScope: query for \"" << scope_id_ << "\": \"" << query_.query_string() << "\" complete" << std::endl;
232@@ -250,8 +256,9 @@
233 }
234 };
235
236- ///! TODO: widgets_api_version, country (+location data)
237+ ///! TODO: country (+location data)
238 std::string session_id;
239+ std::string agent;
240 auto const metadata = action_metadata();
241 if (metadata.contains_hint("session-id") && metadata["session-id"].which() == Variant::String)
242 {
243@@ -263,7 +270,13 @@
244 std::cout << "SmartScope: missing or invalid session id for \"" << scope_id_ << "\" preview: \"" << result().uri() << "\"" << std::endl;
245 }
246
247- preview_handle_ = ss_client_->preview(handler, base_url_, result_["result_json"].get_string(), session_id, hints_.form_factor(), 0, settings(), hints_.locale(), "");
248+ if (metadata.contains_hint("user-agent") && metadata["user-agent"].which() == Variant::String)
249+ {
250+ agent = metadata["user-agent"].get_string();
251+ }
252+
253+ preview_handle_ = ss_client_->preview(handler, base_url_, result_["result_json"].get_string(), session_id, hints_.form_factor(), 0, settings(),
254+ hints_.locale(), "", agent);
255
256 preview_handle_->wait();
257
258
259=== modified file 'src/scopes/internal/smartscopes/SmartScopesClient.cpp'
260--- src/scopes/internal/smartscopes/SmartScopesClient.cpp 2014-09-03 14:34:14 +0000
261+++ src/scopes/internal/smartscopes/SmartScopesClient.cpp 2014-09-25 08:50:05 +0000
262@@ -335,6 +335,7 @@
263 VariantMap const& filter_state,
264 std::string const& locale,
265 std::string const& country,
266+ std::string const& user_agent_hdr,
267 uint limit)
268 {
269 std::ostringstream search_uri;
270@@ -381,6 +382,14 @@
271 uint search_id = ++query_counter_;
272
273 std::cout << "SmartScopesClient.search(): GET " << search_uri.str() << std::endl;
274+
275+ HttpHeaders headers;
276+ if (!user_agent_hdr.empty())
277+ {
278+ std::cout << "User agent: " << user_agent_hdr;
279+ headers.push_back(std::make_pair("User-Agent", user_agent_hdr));
280+ }
281+
282 query_results_[search_id] = http_client_->get(search_uri.str(), [this, handler](std::string const& lineData) {
283 try
284 {
285@@ -390,7 +399,7 @@
286 {
287 std::cerr << "Failed to parse: " << e.what() << std::endl;
288 }
289- });
290+ }, headers);
291
292 return SearchHandle::UPtr(new SearchHandle(search_id, shared_from_this()));
293 }
294@@ -403,6 +412,7 @@
295 const uint widgets_api_version,
296 VariantMap const& settings,
297 std::string const& locale,
298+ std::string const& user_agent_hdr,
299 std::string const& country)
300 {
301 std::ostringstream preview_uri;
302@@ -415,6 +425,12 @@
303 preview_uri << "&platform=" << platform;
304 preview_uri << "&widgets_api_version=" << std::to_string(widgets_api_version);
305
306+ HttpHeaders headers;
307+ if (!user_agent_hdr.empty())
308+ {
309+ headers.push_back(std::make_pair("User-Agent", user_agent_hdr));
310+ }
311+
312 // optional parameters
313
314 if (!settings.empty())
315@@ -447,7 +463,7 @@
316 {
317 std::cerr << "Failed to parse: " << e.what() << std::endl;
318 }
319- });
320+ }, headers);
321
322 return PreviewHandle::UPtr(new PreviewHandle(preview_id, shared_from_this()));
323 }
324
325=== modified file 'test/gtest/scopes/internal/smartscopes/SmartScopesClient/FakeSss.py'
326--- test/gtest/scopes/internal/smartscopes/SmartScopesClient/FakeSss.py 2014-08-05 15:08:30 +0000
327+++ test/gtest/scopes/internal/smartscopes/SmartScopesClient/FakeSss.py 2014-09-25 08:50:05 +0000
328@@ -32,6 +32,9 @@
329 if environ['PATH_INFO'] == '/remote-scopes' and (environ['QUERY_STRING'] == '' or environ['QUERY_STRING'] == 'locale=test_TEST'):
330 return [remote_scopes_response]
331
332+ if environ['PATH_INFO'] == '/demo/search' and environ['QUERY_STRING'].find('test_user_agent_header') >= 0:
333+ return [search_response + '\r\n{"result": {"cat_id": "cat1", "art": "https://dash.ubuntu.com/imgs/cat.png", "uri": "URI", "title": "' + environ['HTTP_USER_AGENT'] + '"}}']
334+
335 if environ['PATH_INFO'] == '/demo/search' and environ['QUERY_STRING'] != '':
336 return [search_response]
337
338
339=== modified file 'test/gtest/scopes/internal/smartscopes/SmartScopesClient/SmartScopesClient_test.cpp'
340--- test/gtest/scopes/internal/smartscopes/SmartScopesClient/SmartScopesClient_test.cpp 2014-08-28 15:51:48 +0000
341+++ test/gtest/scopes/internal/smartscopes/SmartScopesClient/SmartScopesClient_test.cpp 2014-09-25 08:50:05 +0000
342@@ -140,7 +140,7 @@
343 dept = deptinfo;
344 };
345
346- auto search_handle = ssc_->search(handler, sss_url_ + "/demo", "stuff", "", "session_id", 0, "platform");
347+ auto search_handle = ssc_->search(handler, sss_url_ + "/demo", "stuff", "", "session_id", 0, "platform", VariantMap(), VariantMap(), "en_US", "", "ThisIsUserAgentHeader");
348 search_handle->wait();
349
350 ASSERT_EQ(3u, results.size());
351@@ -221,6 +221,32 @@
352 EXPECT_EQ(active_option->id(), "salesrank");
353 }
354
355+TEST_F(SmartScopesClientTest, userAgentHeader)
356+{
357+ std::vector<SearchResult> results;
358+
359+ SearchReplyHandler handler;
360+ handler.filters_handler = [](Filters const &) {
361+ };
362+ handler.filter_state_handler = [](FilterState const&) {
363+ };
364+ handler.category_handler = [](std::shared_ptr<SearchCategory> const&) {
365+ };
366+ handler.result_handler = [&results](SearchResult const& result) {
367+ results.push_back(result);
368+ };
369+ handler.departments_handler = [](std::shared_ptr<DepartmentInfo> const&) {
370+ };
371+
372+ auto search_handle = ssc_->search(handler, sss_url_ + "/demo", "test_user_agent_header", "", "session_id", 0, "platform", VariantMap(), VariantMap(), "en_US", "", "ThisIsUserAgentHeader");
373+ search_handle->wait();
374+
375+ ASSERT_EQ(4u, results.size());
376+
377+ // user agent string is expected in the result title
378+ EXPECT_EQ("ThisIsUserAgentHeader", results[3].other_params["title"]->as_string());
379+}
380+
381 TEST_F(SmartScopesClientTest, preview)
382 {
383 PreviewReplyHandler handler;

Subscribers

People subscribed via source and target branches

to all changes: