unity-scopes-api

Merge lp:~michihenning/unity-scopes-api/loop-fix into lp:unity-scopes-api/devel

  1. loop-fix
  2. Merge into devel
Proposed by Michi Henning
Status: Merged
Approved by: Paweł Stołowski
Approved revision: 593
Merged at revision: 592
Proposed branch: lp:~michihenning/unity-scopes-api/loop-fix
Merge into: lp:unity-scopes-api/devel
Diff against target: 123 lines (+26/-10)
5 files modified
debian/libunity-scopes3.symbols (+2/-2)
include/unity/scopes/internal/SearchQueryBaseImpl.h (+4/-2)
src/scopes/internal/ScopeImpl.cpp (+1/-0)
src/scopes/internal/ScopeObject.cpp (+2/-1)
src/scopes/internal/SearchQueryBaseImpl.cpp (+17/-5)
To merge this branch: bzr merge lp:~michihenning/unity-scopes-api/loop-fix
Reviewer Review Type Date Requested Status
Paweł Stołowski (community) Approve
PS Jenkins bot (community) continuous-integration Approve
Review via email: mp+259999@code.launchpad.net

Commit message

Fix for over-zealous loop detection. A loop is now detected only if
a query loops back with the exact same details, that is, query string,
department, filter state, user data, and metadata must all be the same.

Description of the change

Fix for over-zealous loop detection. A loop is now detected only if
a query loops back with the exact same details, that is, query string,
department, filter state, user data, and metadata must all be the same.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:592
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-ci/1457/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-amd64-ci/167/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-armhf-ci/167/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-i386-ci/167/console

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

review: Needs Fixing (continuous-integration)
lp:~michihenning/unity-scopes-api/loop-fix updated
593. By Michi Henning

Fixed symbols file.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:593
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-ci/1458/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-amd64-ci/168
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-armhf-ci/168
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-armhf-ci/168/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-i386-ci/168

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

review: Approve (continuous-integration)
Revision history for this message
Paweł Stołowski (stolowski) wrote :

Looks good, thank you! It's a pitty we don't pass CannedQuery everywhere, that would greatly simplify the creation of "details" variant (and also would transparently support anything we may throw into CannedQuery in the future). We need to remember to updated loop detection whenever we add any other extra parameter to search queries.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'debian/libunity-scopes3.symbols'
--- debian/libunity-scopes3.symbols 2015-04-23 16:02:37 +0000
+++ debian/libunity-scopes3.symbols 2015-05-23 02:49:17 +0000
@@ -516,12 +516,12 @@
516 (c++)"unity::scopes::internal::ScopeImpl::runtime() const@Base" 0.6.12+15.04.20150127.2516 (c++)"unity::scopes::internal::ScopeImpl::runtime() const@Base" 0.6.12+15.04.20150127.2
517 (c++)"unity::scopes::internal::ScopeImpl::~ScopeImpl()@Base" 0.4.0+14.04.20140312.1517 (c++)"unity::scopes::internal::ScopeImpl::~ScopeImpl()@Base" 0.4.0+14.04.20140312.1
518 (c++)"unity::scopes::internal::ScopeImpl::ScopeImpl(std::shared_ptr<unity::scopes::internal::MWScope> const&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)@Base" 0.6.11+15.04.20150119518 (c++)"unity::scopes::internal::ScopeImpl::ScopeImpl(std::shared_ptr<unity::scopes::internal::MWScope> const&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)@Base" 0.6.11+15.04.20150119
519 (c++)"unity::scopes::internal::ScopeImpl::search(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, unity::scopes::FilterState const&, std::unique_ptr<unity::scopes::Variant, std::default_delete<unity::scopes::Variant> >, unity::scopes::SearchMetadata const&, std::vector<std::tuple<std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::tuple<std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > const&, std::shared_ptr<unity::scopes::SearchListenerBase> const&)@Base" 0.6.16+15.04.20150410.3519 (c++)"unity::scopes::internal::ScopeImpl::search(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, unity::scopes::FilterState const&, std::unique_ptr<unity::scopes::Variant, std::default_delete<unity::scopes::Variant> >, unity::scopes::SearchMetadata const&, std::vector<std::tuple<std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::tuple<std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > const&, std::shared_ptr<unity::scopes::SearchListenerBase> const&)@Base" 0replaceme
520 (c++)"unity::scopes::internal::ScopeImpl::search(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, unity::scopes::FilterState const&, unity::scopes::SearchMetadata const&, std::shared_ptr<unity::scopes::SearchListenerBase> const&)@Base" 0.4.0+14.04.20140312.1520 (c++)"unity::scopes::internal::ScopeImpl::search(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, unity::scopes::FilterState const&, unity::scopes::SearchMetadata const&, std::shared_ptr<unity::scopes::SearchListenerBase> const&)@Base" 0.4.0+14.04.20140312.1
521 (c++)"unity::scopes::internal::ScopeImpl::search(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, unity::scopes::FilterState const&, unity::scopes::Variant const&, unity::scopes::SearchMetadata const&, std::shared_ptr<unity::scopes::SearchListenerBase> const&)@Base" 0.6.16+15.04.20150410.3521 (c++)"unity::scopes::internal::ScopeImpl::search(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, unity::scopes::FilterState const&, unity::scopes::Variant const&, unity::scopes::SearchMetadata const&, std::shared_ptr<unity::scopes::SearchListenerBase> const&)@Base" 0.6.16+15.04.20150410.3
522 (c++)"unity::scopes::internal::ScopeImpl::search(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, unity::scopes::FilterState const&, unity::scopes::SearchMetadata const&, std::shared_ptr<unity::scopes::SearchListenerBase> const&)@Base" 0.4.0+14.04.20140312.1522 (c++)"unity::scopes::internal::ScopeImpl::search(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, unity::scopes::FilterState const&, unity::scopes::SearchMetadata const&, std::shared_ptr<unity::scopes::SearchListenerBase> const&)@Base" 0.4.0+14.04.20140312.1
523 (c++)"unity::scopes::internal::ScopeImpl::search(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, unity::scopes::SearchMetadata const&, std::shared_ptr<unity::scopes::SearchListenerBase> const&)@Base" 0.4.0+14.04.20140312.1523 (c++)"unity::scopes::internal::ScopeImpl::search(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, unity::scopes::SearchMetadata const&, std::shared_ptr<unity::scopes::SearchListenerBase> const&)@Base" 0.4.0+14.04.20140312.1
524 (c++)"unity::scopes::internal::ScopeImpl::search(unity::scopes::CannedQuery const&, unity::scopes::SearchMetadata const&, std::vector<std::tuple<std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::tuple<std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > const&, std::shared_ptr<unity::scopes::SearchListenerBase> const&)@Base" 0.6.12+15.04.20150127.2524 (c++)"unity::scopes::internal::ScopeImpl::search(unity::scopes::CannedQuery const&, unity::scopes::SearchMetadata const&, std::vector<std::tuple<std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::tuple<std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > const&, std::shared_ptr<unity::scopes::SearchListenerBase> const&)@Base" 0replaceme
525 (c++)"unity::scopes::internal::ScopeImpl::set_child_scopes(std::vector<unity::scopes::ChildScope, std::allocator<unity::scopes::ChildScope> > const&)@Base" 0.6.15+15.04.20150407525 (c++)"unity::scopes::internal::ScopeImpl::set_child_scopes(std::vector<unity::scopes::ChildScope, std::allocator<unity::scopes::ChildScope> > const&)@Base" 0.6.15+15.04.20150407
526 (c++)"unity::scopes::internal::ScopeLoader::libpath() const@Base" 0.4.0+14.04.20140312.1526 (c++)"unity::scopes::internal::ScopeLoader::libpath() const@Base" 0.4.0+14.04.20140312.1
527 (c++)"unity::scopes::internal::ScopeLoader::load(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)@Base" 0.6.0+14.10.20140804.1527 (c++)"unity::scopes::internal::ScopeLoader::load(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)@Base" 0.6.0+14.10.20140804.1
528528
=== modified file 'include/unity/scopes/internal/SearchQueryBaseImpl.h'
--- include/unity/scopes/internal/SearchQueryBaseImpl.h 2015-04-09 15:59:17 +0000
+++ include/unity/scopes/internal/SearchQueryBaseImpl.h 2015-05-23 02:49:17 +0000
@@ -48,7 +48,7 @@
4848
49 void set_client_id(const std::string& id);49 void set_client_id(const std::string& id);
5050
51 typedef std::tuple<std::string, std::string, std::string> HistoryData;51 typedef std::tuple<std::string, std::string, std::string, std::string> HistoryData;
52 typedef std::vector<HistoryData> History;52 typedef std::vector<HistoryData> History;
5353
54 void set_history(History const& h);54 void set_history(History const& h);
@@ -78,7 +78,9 @@
78 History history_;78 History history_;
79 std::vector<QueryCtrlProxy> subqueries_;79 std::vector<QueryCtrlProxy> subqueries_;
8080
81 QueryCtrlProxy check_for_query_loop(ScopeProxy const& scope, SearchListenerBase::SPtr const& reply);81 QueryCtrlProxy check_for_query_loop(ScopeProxy const& scope,
82 SearchListenerBase::SPtr const& reply,
83 VariantMap const& details);
82};84};
8385
84} // namespace internal86} // namespace internal
8587
=== modified file 'src/scopes/internal/ScopeImpl.cpp'
--- src/scopes/internal/ScopeImpl.cpp 2015-04-09 15:59:17 +0000
+++ src/scopes/internal/ScopeImpl.cpp 2015-05-23 02:49:17 +0000
@@ -157,6 +157,7 @@
157 d["c"] = get<0>(tuple); // Client157 d["c"] = get<0>(tuple); // Client
158 d["a"] = get<1>(tuple); // Aggregator158 d["a"] = get<1>(tuple); // Aggregator
159 d["r"] = get<2>(tuple); // Receiver159 d["r"] = get<2>(tuple); // Receiver
160 d["d"] = get<3>(tuple); // Details (query string, department, filter state, user data, metadata)
160 hist.push_back(Variant(d));161 hist.push_back(Variant(d));
161 }162 }
162 context["history"] = Variant(hist);163 context["history"] = Variant(hist);
163164
=== modified file 'src/scopes/internal/ScopeObject.cpp'
--- src/scopes/internal/ScopeObject.cpp 2015-04-01 12:31:57 +0000
+++ src/scopes/internal/ScopeObject.cpp 2015-05-23 02:49:17 +0000
@@ -184,7 +184,8 @@
184 string client_id = t.get_dict()["c"].get_string();184 string client_id = t.get_dict()["c"].get_string();
185 string agg = t.get_dict()["a"].get_string();185 string agg = t.get_dict()["a"].get_string();
186 string recv = t.get_dict()["r"].get_string();186 string recv = t.get_dict()["r"].get_string();
187 SearchQueryBaseImpl::HistoryData hd = make_tuple(client_id, agg, recv);187 string details = t.get_dict()["d"].get_string();
188 SearchQueryBaseImpl::HistoryData hd = make_tuple(client_id, agg, recv, details);
188 history.push_back(hd);189 history.push_back(hd);
189 }190 }
190 sqb->set_history(history);191 sqb->set_history(history);
191192
=== modified file 'src/scopes/internal/SearchQueryBaseImpl.cpp'
--- src/scopes/internal/SearchQueryBaseImpl.cpp 2015-04-09 23:41:21 +0000
+++ src/scopes/internal/SearchQueryBaseImpl.cpp 2015-05-23 02:49:17 +0000
@@ -80,7 +80,13 @@
80 throw InvalidArgumentException("QueryBase::subsearch(): reply cannot be nullptr");80 throw InvalidArgumentException("QueryBase::subsearch(): reply cannot be nullptr");
81 }81 }
8282
83 auto query_ctrl = check_for_query_loop(scope, reply);83 VariantMap details;
84 details["query_string"] = query_string;
85 details["department_id"] = department_id;
86 details["filter_state"] = filter_state.serialize();
87 details["user_data"] = user_data ? *user_data : Variant();
88 details["metadata"] = metadata.serialize();
89 auto query_ctrl = check_for_query_loop(scope, reply, move(details));
84 if (query_ctrl)90 if (query_ctrl)
85 {91 {
86 return query_ctrl; // Loop was detected, return dummy QueryCtrlProxy.92 return query_ctrl; // Loop was detected, return dummy QueryCtrlProxy.
@@ -179,17 +185,22 @@
179// It is possible for an aggregator to send a query to itself, but only once.185// It is possible for an aggregator to send a query to itself, but only once.
180186
181QueryCtrlProxy SearchQueryBaseImpl::check_for_query_loop(ScopeProxy const& scope,187QueryCtrlProxy SearchQueryBaseImpl::check_for_query_loop(ScopeProxy const& scope,
182 SearchListenerBase::SPtr const& reply)188 SearchListenerBase::SPtr const& reply,
189 VariantMap const& details)
183{190{
184 shared_ptr<QueryCtrlImpl> ctrl_proxy;191 shared_ptr<QueryCtrlImpl> ctrl_proxy;
185192
186 lock_guard<mutex> lock(mutex_);193 lock_guard<mutex> lock(mutex_);
187194
188 HistoryData tuple = make_tuple(client_id_, canned_query_.scope_id(), scope->identity());195 HistoryData tuple = make_tuple(client_id_,
196 canned_query_.scope_id(),
197 scope->identity(),
198 Variant(details).serialize_json());
189 auto it = find(history_.begin(), history_.end(), tuple);199 auto it = find(history_.begin(), history_.end(), tuple);
190 if (it != history_.end())200 if (it != history_.end())
191 {201 {
192 // Query has been here before from the same client and with the same child scope as the target.202 // Query has been here before from the same client and with the same child scope as the target,
203 // and with the same query details (query, department, filter state, user data, and metadata).
193 reply->finished(CompletionDetails(CompletionDetails::OK,204 reply->finished(CompletionDetails(CompletionDetails::OK,
194 "empty result set due to aggregator loop or repeated query on aggregating scope "205 "empty result set due to aggregator loop or repeated query on aggregating scope "
195 + get<1>(tuple)));206 + get<1>(tuple)));
@@ -204,7 +215,8 @@
204 << "query loop for query \"" << canned_query_.query_string()215 << "query loop for query \"" << canned_query_.query_string()
205 << "\", client: " << get<0>(tuple)216 << "\", client: " << get<0>(tuple)
206 << ", aggregator: " << get<1>(tuple)217 << ", aggregator: " << get<1>(tuple)
207 << ", receiver: " << get<2>(tuple) << endl;218 << ", receiver: " << get<2>(tuple)
219 << ", details: " << get<3>(tuple) << endl;
208 }220 }
209 }221 }
210 else222 else

Subscribers

People subscribed via source and target branches

to all changes: