Merge lp:~facundo/ubuntu-rest-scopes/corrupt-reddit-response into lp:ubuntu-rest-scopes

[Facundo Batista]

Support corrupt/broken responses from reddit.

Support corrupt/broken responses from reddit.

[Roberto Alsina (ralsina)]

Code LGTM

review: Approve

=== modified file 'src/scopes/reddit.py'
--- src/scopes/reddit.py	2016-06-07 14:20:44 +0000
+++ src/scopes/reddit.py	2016-07-11 14:30:27 +0000
@@ -1,10 +1,11 @@
-# Copyright 2014-2015 Canonical
+# Copyright 2014-2016 Canonical
 # All Rights Reserved
 
 """The Reddit scope."""
 
 import urllib
 import json
+import logging
 
 from ..scopes import SimpleApp
 from ..translator import translator
@@ -13,6 +14,8 @@
 # this module is generated by a "make build"
 from versioninfo import version_info
 
+logger = logging.getLogger("restscopes.reddit")
+
 
 # the different URIs to get info from Reddit
 POPULAR_SUBREDDITS = 'http://www.reddit.com/subreddits/popular.json?limit=50'
@@ -176,9 +179,15 @@
     def _get_results(self, url):
         """Call the Reddit API."""
         hdr = {'User-Agent': "Reddit Ubuntu Rest Scope " + str(version_info['revno'])}
-        response = self.urlread(url, headers=hdr)
-        results = json.loads(response.decode('utf-8'))
-        return results
+        try:
+            response = self.urlread(url, headers=hdr)
+            results = json.loads(response.decode('utf-8'))
+        except Exception as err:
+            logger.debug("Error %s(%s) when hitting: %r", err.__class__.__name__, err, url)
+            return []
+
+        children = results['data']['children']
+        return children
 
     def _get_available_departments(self, department, locale):
         """Use popular subreddits to build the departments list."""
@@ -188,7 +197,7 @@
         current['label'] = translator('Front page', locale)
         current['has_subdepartments'] = True
         # Departments for reddit are a flat list, always display them
-        for r in self._get_results(POPULAR_SUBREDDITS)['data']['children']:
+        for r in self._get_results(POPULAR_SUBREDDITS):
             sub = {}
             name = r['data']['display_name']
             sub['label'] = name
@@ -221,14 +230,14 @@
                 uri = SURFACING_DEP_URI % dict(dep=department, limit=limit)
                 yield dict(category=localized_category(THREAD_CATEGORY,
                                                        locale))
-                for post in self._get_results(uri)['data']['children']:
+                for post in self._get_results(uri):
                     yield dict(result=_parse_post(post, query,
                                                   locale, platform))
             else:
                 uri = SURFACING_URI % dict(limit=limit)
                 yield dict(category=localized_category(THREAD_CATEGORY,
                                                        locale))
-                for post in self._get_results(uri)['data']['children']:
+                for post in self._get_results(uri):
                     yield dict(result=_parse_post(post, query,
                                                   locale, platform))
         else:
@@ -238,14 +247,14 @@
             subs_uri = SUBREDDIT_SEARCH_URI % dict(query=query_cleaned,
                                                    limit=limit)
             yield dict(category=localized_category(SUBREDDIT_CATEGORY, locale))
-            for subreddit in self._get_results(subs_uri)['data']['children']:
+            for subreddit in self._get_results(subs_uri):
                 yield dict(result=_parse_subreddit(subreddit, query, locale))
 
             # Search posts
             posts_uri = POST_SEARCH_URI % dict(query=query_cleaned,
                                                limit=limit)
             yield dict(category=localized_category(S_THREAD_CATEGORY, locale))
-            for post in self._get_results(posts_uri)['data']['children']:
+            for post in self._get_results(posts_uri):
                 yield dict(result=_parse_post(post, query,
                                               locale, platform))
 
@@ -301,7 +310,7 @@
                     'id': 'image',
                     'type': 'image',
                     'source': result['art'],
-                    })
+                })
             # create a 'header' widget
             header = dict(widget={
                 'id': 'header',

=== modified file 'src/scopes/tests/test_reddit.py'
--- src/scopes/tests/test_reddit.py	2016-03-14 07:51:29 +0000
+++ src/scopes/tests/test_reddit.py	2016-07-11 14:30:27 +0000
@@ -1,6 +1,6 @@
 # -*- coding: utf-8 -*-
 
-# Copyright 2014-2015 Canonical
+# Copyright 2014-2016 Canonical
 # All Rights Reserved
 
 """Tests for the Simple App base stuff."""
@@ -11,7 +11,7 @@
 from mock import patch
 
 from src.scopes import reddit
-from src.scopes.tests import get_fixture
+from src.scopes.tests import get_fixture, SetupLogChecker
 
 from versioninfo import version_info
 
@@ -22,22 +22,34 @@
 class SearchTestCase(TestCase):
     """Tests for the search function."""
 
-    def test_result_retrieval(self):
+    def setUp(self):
+        super(SearchTestCase, self).setUp()
+        SetupLogChecker(self, 'restscopes')
+
+    def test_result_retrieval_ok(self):
         app = reddit.App(CONFIG)
-        fake_data = {'test': 123}
+        fake_data = {'data': {'children': ['test1', 'test2']}}
         with patch.object(app, 'urlread') as mock_urlread:
             mock_urlread.return_value = json.dumps(fake_data)
             resp = app._get_results('test url')
-        self.assertEqual(resp, fake_data)
+        self.assertEqual(resp, ['test1', 'test2'])
         ua = "Reddit Ubuntu Rest Scope " + str(version_info['revno'])
         mock_urlread.assert_called_with('test url', headers={'User-Agent': ua})
 
+    def test_result_retrieval_broken(self):
+        app = reddit.App(CONFIG)
+        with patch.object(app, 'urlread') as mock_urlread:
+            mock_urlread.return_value = "not a json"
+            resp = app._get_results('test url')
+        self.assertEqual(resp, [])
+        self.assertLogged("Error", "when hitting", "test url", "ValueError")
+
     def test_surfacing(self):
         app = reddit.App(CONFIG)
+        results_data = json.loads(get_fixture('reddit-front.json'))['data']['children']
         with patch.object(app, '_get_available_departments') as mockdep:
             with patch.object(app, '_get_results') as mock:
-                mock.return_value = json.loads(
-                    get_fixture('reddit-front.json'))
+                mock.return_value = results_data
                 mockdep.return_value = {}
                 response = list(app.search(query='',
                                            platform='desktop',
@@ -139,14 +151,14 @@
             'attributes': [
                 {'style': 'highlighted', 'value': '171'},
                 {'style': 'default', 'value': '242 comments'}]
-            }
+        }
         self.assertEqual(response, should)
 
     def test_available_departments(self):
         app = reddit.App(CONFIG)
+        results_data = json.loads(get_fixture('reddit-departments.json'))['data']['children']
         with patch.object(app, '_get_results') as mock:
-            mock.return_value = json.loads(
-                get_fixture('reddit-departments.json'))
+            mock.return_value = results_data
             response = app._get_available_departments(None, 'EN')
 
         should = {'canned_query': 'scope://com.canonical.scopes.reddit',
@@ -169,9 +181,9 @@
 
     def test_available_departments_with_new_dep(self):
         app = reddit.App(CONFIG)
+        results_data = json.loads(get_fixture('reddit-departments.json'))['data']['children']
         with patch.object(app, '_get_results') as mock:
-            mock.return_value = json.loads(
-                get_fixture('reddit-departments.json'))
+            mock.return_value = results_data
             response = app._get_available_departments('halflife3', 'EN')
 
         should = {'canned_query': 'scope://com.canonical.scopes.reddit',