From fcfac09d009b11bec88dad7a5e2650db11a97f54 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Laur=C3=A9line=20Gu=C3=A9rin?= <zebuline@entrouvert.com>
Date: Mon, 5 Oct 2020 10:06:54 +0200
Subject: [PATCH] wcs: handle network error on requests (#31438)

---
 combo/apps/wcs/models.py |  90 +++++++++++-----------
 combo/apps/wcs/utils.py  |  22 ++++--
 tests/test_wcs.py        | 161 ++++++++++++++++++++++++++++++++++-----
 3 files changed, 203 insertions(+), 70 deletions(-)

diff --git a/combo/apps/wcs/models.py b/combo/apps/wcs/models.py
index c8475e56..fffa71b1 100644
--- a/combo/apps/wcs/models.py
+++ b/combo/apps/wcs/models.py
@@ -27,6 +27,7 @@ from django.utils.text import slugify
 from django.utils.translation import ugettext_lazy as _
 
 from jsonfield import JSONField
+from requests.exceptions import RequestException
 
 from combo.data.models import CellBase, Page
 from combo.data.library import register_cell_class
@@ -94,17 +95,13 @@ class WcsFormCell(CellBase):
                 wcs_site = get_wcs_services().get(wcs_key)
                 forms_response_json = get_wcs_json(wcs_site, 'api/formdefs/')
 
-                if not forms_response_json:
+                if not forms_response_json or forms_response_json.get('err') == 1:
                     # can not retrieve data, don't report cell as invalid
                     self.mark_as_valid()
                     return
 
-                if isinstance(forms_response_json, dict):
-                    # forward compability with future w.c.s. API
-                    forms_response_json = forms_response_json.get('data')
-
                 form_found = False
-                for form in forms_response_json or []:
+                for form in forms_response_json.get('data') or []:
                     slug = form.get('slug')
                     if slug == form_slug:
                         self.cached_title = form.get('title')
@@ -200,13 +197,13 @@ class WcsCommonCategoryCell(CellBase):
                 wcs_site = get_wcs_services().get(wcs_key)
                 categories_response_json = get_wcs_json(wcs_site, 'api/categories/')
 
-                if not categories_response_json:
+                if not categories_response_json or categories_response_json.get('err') == 1:
                     # can not retrieve data, don't report cell as invalid
                     self.mark_as_valid()
                     return
 
                 category_found = False
-                for category in categories_response_json.get('data', []):
+                for category in categories_response_json.get('data') or []:
                     slug = category.get('slug')
                     if slug == category_slug:
                         self.cached_title = category.get('title')
@@ -305,29 +302,29 @@ class WcsBlurpMixin(object):
             wcs_site['base_url'] = url
             wcs_site['slug'] = wcs_slug
 
-            response = requests.get(
-                    api_url,
-                    remote_service=wcs_site,
-                    user=getattr(context.get('request'), 'user', None),
-                    cache_duration=self.cache_duration,
-                    raise_if_not_cached=not(context.get('synchronous')),
-                    log_errors=False)
-            returns.add(response.status_code)
-            if response.status_code == 200:
-                json_response = response.json()
-                if isinstance(json_response, list):
-                    # backward compat with older w.c.s.
-                    wcs_site['data'] = json_response
-                elif json_response.get('err', 0) == 0:
-                    wcs_site['data'] = json_response['data']
-                else:
-                    # skip errors
-                    continue
-                # and mark all items with the site info
-                for item in wcs_site['data']:
-                    item['site_slug'] = wcs_slug
+            try:
+                response = requests.get(
+                        api_url,
+                        remote_service=wcs_site,
+                        user=getattr(context.get('request'), 'user', None),
+                        cache_duration=self.cache_duration,
+                        raise_if_not_cached=not(context.get('synchronous')),
+                        log_errors=False)
+                returns.add(response.status_code)
+                response.raise_for_status()
+            except RequestException:
+                continue
+            json_response = response.json()
+            if json_response.get('err', 0) == 0:
+                wcs_site['data'] = json_response['data']
+            else:
+                # skip errors
+                continue
+            # and mark all items with the site info
+            for item in wcs_site['data']:
+                item['site_slug'] = wcs_slug
 
-        if 200 not in returns:  # not a single valid answer
+        if returns and 200 not in returns:  # not a single valid answer
             logging.debug('failed to get data from any %s (%r)', api_url, returns)
             if all([400 <= r < 500 for r in returns]) and self.warn_on_4xx:
                 # only 4xx errors, not a user cell, report the cell as invalid
@@ -409,13 +406,13 @@ class CategoriesValidityMixin(object):
             wcs_site = get_wcs_services().get(wcs_key)
             categories_response_json = get_wcs_json(wcs_site, 'api/categories/')
 
-            if not categories_response_json:
+            if not categories_response_json or categories_response_json.get('err') == 1:
                 # can not retrieve data, don't report cell as invalid
                 continue
 
             category_found = any(
                 [c.get('slug') == category_slug
-                 for c in categories_response_json.get('data', [])])
+                 for c in categories_response_json.get('data') or []])
             if not category_found:
                 self.mark_as_invalid('wcs_category_not_found')
                 return
@@ -616,13 +613,13 @@ class WcsFormsOfCategoryCell(WcsCommonCategoryCell, WcsBlurpMixin):
         wcs_site = get_wcs_services().get(wcs_key)
         categories_response_json = get_wcs_json(wcs_site, 'api/categories/')
 
-        if not categories_response_json:
+        if not categories_response_json or categories_response_json.get('err') == 1:
             # can not retrieve data, don't report cell as invalid
             return
 
         category_found = any(
             [c.get('slug') == category_slug
-             for c in categories_response_json.get('data', [])])
+             for c in categories_response_json.get('data') or []])
         if not category_found:
             self.mark_as_invalid('wcs_category_not_found')
             return
@@ -754,13 +751,13 @@ class WcsCardsCell(CardMixin, WcsBlurpMixin, CellBase):
                 wcs_site = get_wcs_services().get(wcs_key)
                 card_models = get_wcs_json(wcs_site, 'api/cards/@list')
 
-                if not card_models:
+                if not card_models or card_models.get('err') == 1:
                     # can not retrieve data, don't report cell as invalid
                     self.mark_as_valid()
                     return
 
                 card_found = False
-                for card in card_models.get('data', []):
+                for card in card_models.get('data') or []:
                     slug = card['slug']
                     if slug == card_slug:
                         self.cached_title = card['title']
@@ -838,6 +835,8 @@ class WcsCardInfosCell(CardMixin, CellBase):
                 if card_schema.get('err') == 1:
                     if card_schema.get('err_class') == 'Page not found':
                         self.mark_as_invalid('wcs_card_not_found')
+                    else:
+                        self.mark_as_valid()
                     return
 
                 self.cached_title = card_schema['name']
@@ -862,13 +861,18 @@ class WcsCardInfosCell(CardMixin, CellBase):
 
         wcs_site = get_wcs_services().get(self.wcs_site)
 
-        response = requests.get(
-                api_url,
-                remote_service=wcs_site,
-                user=getattr(context.get('request'), 'user', None),
-                cache_duration=5,
-                raise_if_not_cached=not(context.get('synchronous')),
-                log_errors=False)
+        try:
+            response = requests.get(
+                    api_url,
+                    remote_service=wcs_site,
+                    user=getattr(context.get('request'), 'user', None),
+                    cache_duration=5,
+                    raise_if_not_cached=not(context.get('synchronous')),
+                    log_errors=False)
+            response.raise_for_status()
+        except RequestException:
+            return extra_context
+
         if response.status_code == 200:
             extra_context['card'] = response.json()
 
diff --git a/combo/apps/wcs/utils.py b/combo/apps/wcs/utils.py
index ceb4b6a9..619de7e6 100644
--- a/combo/apps/wcs/utils.py
+++ b/combo/apps/wcs/utils.py
@@ -16,11 +16,15 @@
 
 from django.conf import settings
 
+from requests.exceptions import RequestException
+
 from combo.utils import requests
 
+
 def is_wcs_enabled(cls):
     return hasattr(settings, 'KNOWN_SERVICES') and settings.KNOWN_SERVICES.get('wcs')
 
+
 def get_wcs_services():
     if not is_wcs_enabled(None):
         return {}
@@ -32,22 +36,24 @@ def get_wcs_json(wcs_site, path):
         # no site specified (probably an import referencing a not yet deployed
         # site)
         return {'err': 1, 'err_desc': 'no-wcs-site'}
-    response = requests.get(path, remote_service=wcs_site, without_user=True,
+    try:
+        response = requests.get(
+            path, remote_service=wcs_site, without_user=True,
             headers={'accept': 'application/json'})
-    if response.status_code != 200:
-        # the exact error will have been logged during request.
-        return {}
+        response.raise_for_status()
+    except RequestException:
+        return {'err': 1, 'data': None}
     return response.json()
 
+
 def get_wcs_options(url, include_category_slug=False):
     references = []
     for wcs_key, wcs_site in sorted(get_wcs_services().items(), key=lambda x: x[1]['title']):
         site_title = wcs_site.get('title')
         response_json = get_wcs_json(wcs_site, url)
-        if type(response_json) is dict:
-            if response_json.get('err') == 1:
-                continue
-            response_json = response_json.get('data')
+        if response_json.get('err') == 1:
+            continue
+        response_json = response_json.get('data')
         if not response_json:
             continue
         for element in response_json:
diff --git a/tests/test_wcs.py b/tests/test_wcs.py
index 28ab0fb2..8ef4dbf2 100644
--- a/tests/test_wcs.py
+++ b/tests/test_wcs.py
@@ -16,6 +16,8 @@ from django.test.utils import CaptureQueriesContext
 from django.utils.six.moves.urllib import parse as urlparse
 
 import mock
+from requests.exceptions import ConnectionError
+from requests.models import Response
 
 from combo.data.library import get_cell_classes
 from combo.data.models import CellBase, LinkListCell, Page, ValidityInfo
@@ -272,10 +274,23 @@ def test_form_cell_validity(mock_send):
     cell.save()
     assert ValidityInfo.objects.exists() is False
 
+    # can not retrieve data, don't set cell as invalid
+    with mock.patch('combo.apps.wcs.models.requests.get') as requests_get:
+        mock_resp = Response()
+        mock_resp.status_code = 500
+        requests_get.return_value = mock_resp
+        cell.save()
+    assert ValidityInfo.objects.exists() is False
+    with mock.patch('combo.apps.wcs.models.requests.get') as requests_get:
+        requests_get.side_effect = ConnectionError()
+        cell.save()
+    assert ValidityInfo.objects.exists() is False
+
     # can not retrieve formdefs, don't set cell as invalid
     with mock.patch('combo.apps.wcs.models.requests.get') as requests_get:
-        mock_json = mock.Mock(status_code=404)
-        requests_get.return_value = mock_json
+        mock_resp = Response()
+        mock_resp.status_code = 404
+        requests_get.return_value = mock_resp
         cell.save()
     assert ValidityInfo.objects.exists() is False
 
@@ -328,10 +343,23 @@ def test_category_cell_validity(mock_send):
     cell.save()
     assert ValidityInfo.objects.exists() is False
 
+    # can not retrieve data, don't set cell as invalid
+    with mock.patch('combo.apps.wcs.models.requests.get') as requests_get:
+        mock_resp = Response()
+        mock_resp.status_code = 500
+        requests_get.return_value = mock_resp
+        cell.save()
+    assert ValidityInfo.objects.exists() is False
+    with mock.patch('combo.apps.wcs.models.requests.get') as requests_get:
+        requests_get.side_effect = ConnectionError()
+        cell.save()
+    assert ValidityInfo.objects.exists() is False
+
     # can not retrieve categories, don't set cell as invalid
     with mock.patch('combo.apps.wcs.models.requests.get') as requests_get:
-        mock_json = mock.Mock(status_code=404)
-        requests_get.return_value = mock_json
+        mock_resp = Response()
+        mock_resp.status_code = 404
+        requests_get.return_value = mock_resp
         cell.save()
     assert ValidityInfo.objects.exists() is False
 
@@ -474,9 +502,22 @@ def test_current_forms_cell_validity(mock_send, context):
     cell.get_data(context)
     assert ValidityInfo.objects.exists() is False
 
+    # can not retrieve data, don't set cell as invalid
     with mock.patch('combo.apps.wcs.models.requests.get') as requests_get:
-        mock_json = mock.Mock(status_code=404)
-        requests_get.return_value = mock_json
+        mock_resp = Response()
+        mock_resp.status_code = 500
+        requests_get.return_value = mock_resp
+        cell.get_data(context)
+    assert ValidityInfo.objects.exists() is False
+    with mock.patch('combo.apps.wcs.models.requests.get') as requests_get:
+        requests_get.side_effect = ConnectionError()
+        cell.get_data(context)
+    assert ValidityInfo.objects.exists() is False
+
+    with mock.patch('combo.apps.wcs.models.requests.get') as requests_get:
+        mock_resp = Response()
+        mock_resp.status_code = 404
+        requests_get.return_value = mock_resp
         cell.get_data(context)
     assert ValidityInfo.objects.exists() is False
 
@@ -496,10 +537,23 @@ def test_current_forms_cell_check_validity(mock_send, context):
     cell.check_validity()
     assert ValidityInfo.objects.exists() is False
 
+    # can not retrieve data, don't set cell as invalid
+    with mock.patch('combo.apps.wcs.models.requests.get') as requests_get:
+        mock_resp = Response()
+        mock_resp.status_code = 500
+        requests_get.return_value = mock_resp
+        cell.check_validity()
+    assert ValidityInfo.objects.exists() is False
+    with mock.patch('combo.apps.wcs.models.requests.get') as requests_get:
+        requests_get.side_effect = ConnectionError()
+        cell.check_validity()
+    assert ValidityInfo.objects.exists() is False
+
     # can not retrieve categories, don't set cell as invalid
     with mock.patch('combo.apps.wcs.models.requests.get') as requests_get:
-        mock_json = mock.Mock(status_code=404)
-        requests_get.return_value = mock_json
+        mock_resp = Response()
+        mock_resp.status_code = 404
+        requests_get.return_value = mock_resp
         cell.check_validity()
     assert ValidityInfo.objects.exists() is False
 
@@ -608,6 +662,18 @@ def test_care_forms_cell_validity(context):
         cell.get_data(context)
     assert ValidityInfo.objects.exists() is False
 
+    # can not retrieve data, don't set cell as invalid
+    with mock.patch('combo.apps.wcs.models.requests.get') as requests_get:
+        mock_resp = Response()
+        mock_resp.status_code = 500
+        requests_get.return_value = mock_resp
+        cell.get_data(context)
+    assert ValidityInfo.objects.exists() is False
+    with mock.patch('combo.apps.wcs.models.requests.get') as requests_get:
+        requests_get.side_effect = ConnectionError()
+        cell.get_data(context)
+    assert ValidityInfo.objects.exists() is False
+
     # can not retrieve data, don't set cell as invalid
     with mock.patch('combo.apps.wcs.models.requests.get') as requests_get:
         mock_json = mock.Mock(status_code=500)
@@ -723,7 +789,8 @@ def test_forms_of_category_cell_render(mock_send, context):
         context.pop('combo_display_even_empty_categories')
 
 
-def test_forms_of_category_cell_validity(context):
+@mock.patch('combo.apps.wcs.utils.requests.send', side_effect=mocked_requests_send)
+def test_forms_of_category_cell_validity(mock_send, context):
     page = Page.objects.create(title='xxx', slug='test_forms_of_category_cell_render', template_name='standard')
     cell = WcsFormsOfCategoryCell.objects.create(page=page, placeholder='content', order=0)
     validity_info = ValidityInfo.objects.latest('pk')
@@ -748,11 +815,16 @@ def test_forms_of_category_cell_validity(context):
         requests_get.return_value = mock_json
         cell.render(context)
     assert ValidityInfo.objects.exists() is False
+    with mock.patch('combo.apps.wcs.models.requests.get') as requests_get:
+        requests_get.side_effect = ConnectionError()
+        cell.render(context)
+    assert ValidityInfo.objects.exists() is False
 
     # can not retrieve categories, don't set cell as invalid
     with mock.patch('combo.apps.wcs.models.requests.get') as requests_get:
-        mock_json = mock.Mock(status_code=404)
-        requests_get.return_value = mock_json
+        mock_resp = Response()
+        mock_resp.status_code = 404
+        requests_get.return_value = mock_resp
         cell.save()
     assert ValidityInfo.objects.exists() is False
 
@@ -763,7 +835,8 @@ def test_forms_of_category_cell_validity(context):
     assert validity_info.invalid_since is not None
 
 
-def test_forms_of_category_cell_check_validity(context):
+@mock.patch('combo.apps.wcs.utils.requests.send', side_effect=mocked_requests_send)
+def test_forms_of_category_cell_check_validity(mock_send, context):
     page = Page.objects.create(title='xxx', slug='test_forms_of_category_cell_render', template_name='standard')
     cell = WcsFormsOfCategoryCell.objects.create(page=page, placeholder='content', order=0)
 
@@ -861,10 +934,23 @@ def test_current_drafts_cell_check_validity(mock_send, context):
     cell.check_validity()
     assert ValidityInfo.objects.exists() is False
 
-    # can not retrieve vategories, don't set cell as invalid
+    # can not retrieve data, don't set cell as invalid
     with mock.patch('combo.apps.wcs.models.requests.get') as requests_get:
-        mock_json = mock.Mock(status_code=404)
-        requests_get.return_value = mock_json
+        mock_resp = Response()
+        mock_resp.status_code = 500
+        requests_get.return_value = mock_resp
+        cell.check_validity()
+    assert ValidityInfo.objects.exists() is False
+    with mock.patch('combo.apps.wcs.models.requests.get') as requests_get:
+        requests_get.side_effect = ConnectionError()
+        cell.check_validity()
+    assert ValidityInfo.objects.exists() is False
+
+    # can not retrieve categories, don't set cell as invalid
+    with mock.patch('combo.apps.wcs.models.requests.get') as requests_get:
+        mock_resp = Response()
+        mock_resp.status_code = 404
+        requests_get.return_value = mock_resp
         cell.check_validity()
     assert ValidityInfo.objects.exists() is False
 
@@ -984,10 +1070,23 @@ def test_cards_cell_validity(mock_send):
     cell.save()
     assert ValidityInfo.objects.exists() is False
 
+    # can not retrieve data, don't set cell as invalid
+    with mock.patch('combo.apps.wcs.models.requests.get') as requests_get:
+        mock_resp = Response()
+        mock_resp.status_code = 500
+        requests_get.return_value = mock_resp
+        cell.save()
+    assert ValidityInfo.objects.exists() is False
+    with mock.patch('combo.apps.wcs.models.requests.get') as requests_get:
+        requests_get.side_effect = ConnectionError()
+        cell.save()
+    assert ValidityInfo.objects.exists() is False
+
     # can not retrieve carddefs, don't set cell as invalid
     with mock.patch('combo.apps.wcs.models.requests.get') as requests_get:
-        mock_json = mock.Mock(status_code=404)
-        requests_get.return_value = mock_json
+        mock_resp = Response()
+        mock_resp.status_code = 404
+        requests_get.return_value = mock_resp
         cell.save()
     assert ValidityInfo.objects.exists() is False
 
@@ -1081,10 +1180,23 @@ def test_card_cell_validity(mock_send):
     cell.save()
     assert ValidityInfo.objects.exists() is False
 
+    # can not retrieve data, don't set cell as invalid
+    with mock.patch('combo.apps.wcs.models.requests.get') as requests_get:
+        mock_resp = Response()
+        mock_resp.status_code = 500
+        requests_get.return_value = mock_resp
+        cell.save()
+    assert ValidityInfo.objects.exists() is False
+    with mock.patch('combo.apps.wcs.models.requests.get') as requests_get:
+        requests_get.side_effect = ConnectionError()
+        cell.save()
+    assert ValidityInfo.objects.exists() is False
+
     # can not retrieve carddefs, don't set cell as invalid
     with mock.patch('combo.apps.wcs.models.requests.get') as requests_get:
-        mock_json = mock.Mock(status_code=404)
-        requests_get.return_value = mock_json
+        mock_resp = Response()
+        mock_resp.status_code = 404
+        requests_get.return_value = mock_resp
         cell.save()
     assert ValidityInfo.objects.exists() is False
 
@@ -1135,6 +1247,17 @@ def test_card_cell_render(mock_send, context):
 
     context['synchronous'] = True  # to get fresh content
 
+    with mock.patch('combo.apps.wcs.models.requests.get') as requests_get:
+        mock_resp = Response()
+        mock_resp.status_code = 500
+        requests_get.return_value = mock_resp
+        result = cell.render(context)
+    assert '<p>Unknown Card</p>' in result
+    with mock.patch('combo.apps.wcs.models.requests.get') as requests_get:
+        requests_get.side_effect = ConnectionError()
+        result = cell.render(context)
+    assert '<p>Unknown Card</p>' in result
+
     result = cell.render(context)
     assert '<h2>Card Model 1 - aa</h2>' in result
     assert '<span class="label">Field A</span>\n    \n    <span class="value">a</span>' in result
-- 
2.28.0

