Merge lp:~knitzsche/scope-aggregator/fixes-fallback-bug-1599948 into lp:scope-aggregator

Proposed by Kyle Nitzsche
Status: Merged
Approved by: Kyle Nitzsche
Approved revision: 170
Merge reported by: Kyle Nitzsche
Merged at revision: not available
Proposed branch: lp:~knitzsche/scope-aggregator/fixes-fallback-bug-1599948
Merge into: lp:scope-aggregator
Diff against target: 109 lines (+21/-14)
3 files modified
CMakeLists.txt (+1/-1)
src/query.cpp (+19/-13)
src/utils.cpp (+1/-0)
To merge this branch: bzr merge lp:~knitzsche/scope-aggregator/fixes-fallback-bug-1599948
Reviewer Review Type Date Requested Status
Zhang Enwei (community) Approve
Penk Chen Pending
Gary.Wang Pending
Review via email: mp+299481@code.launchpad.net

Description of the change

Fixes the fallback mechanism through which the aggregator scope
can attempt to find, for example, art in other result fields, for
example mascot, or emblem. There was an exception when a result
did not contain the primary field because in some cases the code
tried to access a field that did not exist.

This problem causes exceptions when News scope aggregates El Pais
(which sometimes lacks expected attributes) and Yahoo, which now
always lacks "art", "mascot" and "emblem" -- see:
https://bugs.launchpad.net/news-scope/+bug/1600026

To post a comment you must log in.
Revision history for this message
Kyle Nitzsche (knitzsche) wrote :

FYI: I am adding googletest unit tests to scope-aggregator and I will definitely add tests for fallback mechanism.

Revision history for this message
Zhang Enwei (zhangew401) wrote :

Look good to me

review: Approve
170. By Kyle Nitzsche

When testing photos scope I found an issue that this commit fixes.

Please review. The key change is line 1192.

DETAILS

The issue I noticed when testing this MP in photos-scope is that the art for
Douban LOGIN result did not display in photos agg scope.

I found that the fallback code (check_result_fallbacks()) was not called
for Duoban in the login case.

The general case I found is: For scopes that are declared in child_scopes.json
using the "scope" key, the check_results_callbacks() method was not called.

(Please note that fallbacks code/feature is only currently implemented
for scopes that use a renderer defined in common_templates.)

Teseting. Modify photos scope child_scopes.json to only this for Duoban:

        "scope": {
            "_category_title": "My Douban Photos",
            "id": "com.canonical.scopes.douban_doubanalbum",
            "local_id": "douban",
            "result_category_id_to_common_template": [
              {
                "result_category_id": "login",
                "common_template": "not_logged_in"
              }
            ],
            "renderer_common_id": "normal_renderer"
        }

This ^ means:
 * if Douban result category is "login": use common_template "not logged_in".
    This template uses a fallback:
            "id": "not_logged_in",
            "fallbacks":
            [
                {
                    "key": "mascot",
                    "fields":
                    [
                        {"field": "mascot"},
                        {"field": "art"}
                    ]
                }
            ],
    [...]
    Result is that Douban's login result which provides "art" is converted to
    "mascot".
* A result with any other category uses "normal_renderer". This does not use a
  fallback and expects the result to contain 'art".

Revision history for this message
Kyle Nitzsche (knitzsche) wrote :

Note that the douban login art previously did not display in photos agg, so this is not a regression caused by the MP. Good time to fix it though.

Revision history for this message
Zhang Enwei (zhangew401) wrote :

LGTM

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'CMakeLists.txt'
2--- CMakeLists.txt 2016-06-29 10:29:37 +0000
3+++ CMakeLists.txt 2016-07-12 17:15:04 +0000
4@@ -1,4 +1,4 @@
5-set(VERSION "4.8")
6+set(VERSION "4.9")
7
8 # Supress qDebug() output
9 ADD_DEFINITIONS( -DQT_NO_DEBUG_OUTPUT )
10
11=== modified file 'src/query.cpp'
12--- src/query.cpp 2016-06-29 10:29:37 +0000
13+++ src/query.cpp 2016-07-12 17:15:04 +0000
14@@ -392,15 +392,19 @@
15 const auto & map = *common_templates_fallbacks[common_id];
16 for(auto iter = map.begin(); iter != map.end(); ++iter)
17 {
18- qDebug() << "==== check FALLB first: " << qstr(iter->first);
19+ qDebug() << "==== check FALLB field to populate: " << qstr(iter->first);
20+ if (res.contains(iter->first))
21+ {
22+ qDebug() << "=== check FALLB. res has field: " << qstr(iter->first);
23+ return;
24+ }
25 for (const auto & field : iter->second)
26 {
27- qDebug() << "==== check FALLB field: " << qstr(field);
28- if (res[iter->first].get_string().empty())
29+ qDebug() << "==== check FALLB field exists: " << qstr(field);
30+ if (res.contains(field))
31 {
32- qDebug() << "==== check FALLB first is empty: " << qstr(res[iter->first].get_string());
33- if (res.contains(field))
34- res[iter->first] = res[field];
35+ res[iter->first] = res[field].get_string();
36+ return;
37 }
38 }
39 }
40@@ -557,7 +561,8 @@
41
42 // save bits of incoming result
43 std::string inc_res_cat_id = res.category()->id();
44- us::Category::SCPtr res_cat = res.category();
45+ qDebug() <<"=== incoming result cat rdr: " << qstr(res.category()->renderer_template().data());
46+ qDebug() <<"=== incoming result: " << qstr(us::Variant(res.serialize()).serialize_json());
47 std::string inc_res_cat_title = res.category()->title();
48
49 qDebug() << "==== RESULT scope: " << qstr(scope->id());
50@@ -1175,14 +1180,16 @@
51 // if needed, get renderer from result_category_to_common_template
52 if (common_templates.find(child_scopes_m[scope->local_id()]->result_category_id_to_common_template[res.category()->id()]) != common_templates.end())
53 {
54- qDebug() << "==== DECLARED. category to renderer";
55+ qDebug() << "==== DECLARED. current category:" << qstr(res.category()->id());
56 std::string common_template_id = child_scopes_m[scope->local_id()]->result_category_id_to_common_template[res.category()->id()];
57+ qDebug() << "==== DECLARED. using common_template_id: " << qstr(common_template_id);
58 std::string cat_id = common_template_id + ":" + scope->local_id() + cat_title;
59
60 // if the common category has not yet been registered, register it
61 if (!upstream_reply->lookup_category(cat_id))
62 {
63 std::string template_to_use = common_templates[child_scopes_m[scope->local_id()]->result_category_id_to_common_template[res.category()->id()]];
64+ check_result_fallbacks(res, common_template_id);
65 if (scope->category_link_to_child())
66 {
67 if (!upstream_reply->lookup_category(cat_id))
68@@ -1287,7 +1294,7 @@
69 for (size_t i = 0; i < replies.size(); ++i) {
70 us::SearchMetadata metadata(search_metadata());
71 std::string locID = scopes_m_int_localId[i];
72- qDebug() << "==== locID: " << QString::fromStdString(locID);
73+ qDebug() << "==== subsearch locID: " << qstr(locID);
74 auto scope = scopes_m[locID];
75
76 if (scope->keyword_scope())
77@@ -1317,9 +1324,7 @@
78 metadata.set_cardinality( scope->cardinality() );
79 }
80
81-
82- qDebug() << "===- LOCAL_ID: " << qstr(scope->local_id());
83- metadata.set_hint(scope->local_id(), us::Variant("valueMeansNothing"));
84+ metadata.set_hint(scope->id(), us::Variant("valueMeansNothing"));
85
86 // dispatch subsearches differently depending on whether is keyword or declared scope
87 qDebug() << "==== QUERY STRING: " << QString::fromStdString(query_string);
88@@ -1403,7 +1408,8 @@
89 adj_qry,
90 scope->child_department(),
91 us::FilterState(),
92- metadata, replies[i]
93+ metadata,
94+ replies[i]
95 );
96 break;
97 }
98
99=== modified file 'src/utils.cpp'
100--- src/utils.cpp 2016-06-29 10:29:37 +0000
101+++ src/utils.cpp 2016-07-12 17:15:04 +0000
102@@ -61,6 +61,7 @@
103 {
104 if (root.contains(QStringLiteral("cardinality_settings")))
105 {
106+ setting_cardinalities.clear();
107 QJsonArray card_a = root[QStringLiteral("cardinality_settings")].toArray();
108 for (const auto & item : card_a)
109 {

Subscribers

People subscribed via source and target branches