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
1=== modified file 'debian/libunity-scopes3.symbols'
2--- debian/libunity-scopes3.symbols 2015-04-23 16:02:37 +0000
3+++ debian/libunity-scopes3.symbols 2015-05-23 02:49:17 +0000
4@@ -516,12 +516,12 @@
5 (c++)"unity::scopes::internal::ScopeImpl::runtime() const@Base" 0.6.12+15.04.20150127.2
6 (c++)"unity::scopes::internal::ScopeImpl::~ScopeImpl()@Base" 0.4.0+14.04.20140312.1
7 (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
8- (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.3
9+ (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
10 (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
11 (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
12 (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
13 (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
14- (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.2
15+ (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
16 (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
17 (c++)"unity::scopes::internal::ScopeLoader::libpath() const@Base" 0.4.0+14.04.20140312.1
18 (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
19
20=== modified file 'include/unity/scopes/internal/SearchQueryBaseImpl.h'
21--- include/unity/scopes/internal/SearchQueryBaseImpl.h 2015-04-09 15:59:17 +0000
22+++ include/unity/scopes/internal/SearchQueryBaseImpl.h 2015-05-23 02:49:17 +0000
23@@ -48,7 +48,7 @@
24
25 void set_client_id(const std::string& id);
26
27- typedef std::tuple<std::string, std::string, std::string> HistoryData;
28+ typedef std::tuple<std::string, std::string, std::string, std::string> HistoryData;
29 typedef std::vector<HistoryData> History;
30
31 void set_history(History const& h);
32@@ -78,7 +78,9 @@
33 History history_;
34 std::vector<QueryCtrlProxy> subqueries_;
35
36- QueryCtrlProxy check_for_query_loop(ScopeProxy const& scope, SearchListenerBase::SPtr const& reply);
37+ QueryCtrlProxy check_for_query_loop(ScopeProxy const& scope,
38+ SearchListenerBase::SPtr const& reply,
39+ VariantMap const& details);
40 };
41
42 } // namespace internal
43
44=== modified file 'src/scopes/internal/ScopeImpl.cpp'
45--- src/scopes/internal/ScopeImpl.cpp 2015-04-09 15:59:17 +0000
46+++ src/scopes/internal/ScopeImpl.cpp 2015-05-23 02:49:17 +0000
47@@ -157,6 +157,7 @@
48 d["c"] = get<0>(tuple); // Client
49 d["a"] = get<1>(tuple); // Aggregator
50 d["r"] = get<2>(tuple); // Receiver
51+ d["d"] = get<3>(tuple); // Details (query string, department, filter state, user data, metadata)
52 hist.push_back(Variant(d));
53 }
54 context["history"] = Variant(hist);
55
56=== modified file 'src/scopes/internal/ScopeObject.cpp'
57--- src/scopes/internal/ScopeObject.cpp 2015-04-01 12:31:57 +0000
58+++ src/scopes/internal/ScopeObject.cpp 2015-05-23 02:49:17 +0000
59@@ -184,7 +184,8 @@
60 string client_id = t.get_dict()["c"].get_string();
61 string agg = t.get_dict()["a"].get_string();
62 string recv = t.get_dict()["r"].get_string();
63- SearchQueryBaseImpl::HistoryData hd = make_tuple(client_id, agg, recv);
64+ string details = t.get_dict()["d"].get_string();
65+ SearchQueryBaseImpl::HistoryData hd = make_tuple(client_id, agg, recv, details);
66 history.push_back(hd);
67 }
68 sqb->set_history(history);
69
70=== modified file 'src/scopes/internal/SearchQueryBaseImpl.cpp'
71--- src/scopes/internal/SearchQueryBaseImpl.cpp 2015-04-09 23:41:21 +0000
72+++ src/scopes/internal/SearchQueryBaseImpl.cpp 2015-05-23 02:49:17 +0000
73@@ -80,7 +80,13 @@
74 throw InvalidArgumentException("QueryBase::subsearch(): reply cannot be nullptr");
75 }
76
77- auto query_ctrl = check_for_query_loop(scope, reply);
78+ VariantMap details;
79+ details["query_string"] = query_string;
80+ details["department_id"] = department_id;
81+ details["filter_state"] = filter_state.serialize();
82+ details["user_data"] = user_data ? *user_data : Variant();
83+ details["metadata"] = metadata.serialize();
84+ auto query_ctrl = check_for_query_loop(scope, reply, move(details));
85 if (query_ctrl)
86 {
87 return query_ctrl; // Loop was detected, return dummy QueryCtrlProxy.
88@@ -179,17 +185,22 @@
89 // It is possible for an aggregator to send a query to itself, but only once.
90
91 QueryCtrlProxy SearchQueryBaseImpl::check_for_query_loop(ScopeProxy const& scope,
92- SearchListenerBase::SPtr const& reply)
93+ SearchListenerBase::SPtr const& reply,
94+ VariantMap const& details)
95 {
96 shared_ptr<QueryCtrlImpl> ctrl_proxy;
97
98 lock_guard<mutex> lock(mutex_);
99
100- HistoryData tuple = make_tuple(client_id_, canned_query_.scope_id(), scope->identity());
101+ HistoryData tuple = make_tuple(client_id_,
102+ canned_query_.scope_id(),
103+ scope->identity(),
104+ Variant(details).serialize_json());
105 auto it = find(history_.begin(), history_.end(), tuple);
106 if (it != history_.end())
107 {
108- // Query has been here before from the same client and with the same child scope as the target.
109+ // Query has been here before from the same client and with the same child scope as the target,
110+ // and with the same query details (query, department, filter state, user data, and metadata).
111 reply->finished(CompletionDetails(CompletionDetails::OK,
112 "empty result set due to aggregator loop or repeated query on aggregating scope "
113 + get<1>(tuple)));
114@@ -204,7 +215,8 @@
115 << "query loop for query \"" << canned_query_.query_string()
116 << "\", client: " << get<0>(tuple)
117 << ", aggregator: " << get<1>(tuple)
118- << ", receiver: " << get<2>(tuple) << endl;
119+ << ", receiver: " << get<2>(tuple)
120+ << ", details: " << get<3>(tuple) << endl;
121 }
122 }
123 else

Subscribers

People subscribed via source and target branches

to all changes: