From d61ad613c9d3d1aa6319ae9c19fc00317d94a8a2 Mon Sep 17 00:00:00 2001 From: Jesse Shapiro Date: Tue, 28 Feb 2017 11:34:10 -0500 Subject: [PATCH] Add many=True kwarg to get_edx_api_data to modify empty return value; make resource_id checks slightly more strict --- openedx/core/lib/edx_api_utils.py | 10 ++- openedx/core/lib/tests/test_edx_api_utils.py | 84 ++++++++++++++++++++ 2 files changed, 90 insertions(+), 4 deletions(-) diff --git a/openedx/core/lib/edx_api_utils.py b/openedx/core/lib/edx_api_utils.py index 0a561e4bf2..3ae4449a73 100644 --- a/openedx/core/lib/edx_api_utils.py +++ b/openedx/core/lib/edx_api_utils.py @@ -15,7 +15,7 @@ log = logging.getLogger(__name__) def get_edx_api_data(api_config, user, resource, - api=None, resource_id=None, querystring=None, cache_key=None): + api=None, resource_id=None, querystring=None, cache_key=None, many=True): """GET data from an edX REST API. DRY utility for handling caching and pagination. @@ -31,19 +31,21 @@ def get_edx_api_data(api_config, user, resource, querystring (dict): Optional query string parameters. cache_key (str): Where to cache retrieved data. The cache will be ignored if this is omitted (neither inspected nor updated). + many (bool): Whether the resource requested is a collection of objects, or a single object. + If false, an empty dict will be returned in cases of failure rather than the default empty list. Returns: Data returned by the API. When hitting a list endpoint, extracts "results" (list of dict) returned by DRF-powered APIs. """ - no_data = [] + no_data = [] if many else {} if not api_config.enabled: log.warning('%s configuration is disabled.', api_config.API_NAME) return no_data if cache_key: - cache_key = '{}.{}'.format(cache_key, resource_id) if resource_id else cache_key + cache_key = '{}.{}'.format(cache_key, resource_id) if resource_id is not None else cache_key cached = cache.get(cache_key) if cached: @@ -75,7 +77,7 @@ def get_edx_api_data(api_config, user, resource, querystring = querystring if querystring else {} response = endpoint(resource_id).get(**querystring) - if resource_id: + if resource_id is not None: results = response else: results = _traverse_pagination(response, endpoint, querystring, no_data) diff --git a/openedx/core/lib/tests/test_edx_api_utils.py b/openedx/core/lib/tests/test_edx_api_utils.py index 27db7d151e..0c5ccb355f 100644 --- a/openedx/core/lib/tests/test_edx_api_utils.py +++ b/openedx/core/lib/tests/test_edx_api_utils.py @@ -124,6 +124,35 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach self._assert_num_requests(1) + def test_get_specific_resource_with_falsey_id(self): + """ + Verify that a specific resource can be retrieved, and pagination parsing is + not attempted, if the ID passed is falsey (e.g., 0). The expected resource contains + a "results" key, as a paginatable item would have, so if the function looks for falsey + values in the resource_id field, rather than specifically None, the function will + return the value of that "results" key. + """ + catalog_integration = self.create_catalog_integration() + api = create_catalog_api_client(self.user, catalog_integration) + + resource_id = 0 + url = '{api_root}/programs/{resource_id}/'.format( + api_root=CatalogIntegration.current().internal_api_url.strip('/'), + resource_id=resource_id, + ) + + expected_resource = {'key': 'value', 'results': []} + + self._mock_catalog_api( + [httpretty.Response(body=json.dumps(expected_resource), content_type='application/json')], + url=url + ) + + actual_resource = get_edx_api_data(catalog_integration, self.user, 'programs', api=api, resource_id=resource_id) + self.assertEqual(actual_resource, expected_resource) + + self._assert_num_requests(1) + def test_cache_utilization(self): """Verify that when enabled, the cache is used.""" catalog_integration = self.create_catalog_integration(cache_ttl=5) @@ -210,6 +239,61 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach self.assertTrue(mock_exception.called) self.assertEqual(actual, []) + @mock.patch(UTILITY_MODULE + '.log.warning') + def test_api_config_disabled_with_id_and_not_collection(self, mock_warning): + """Verify that no data is retrieved if the provided config model is disabled.""" + catalog_integration = self.create_catalog_integration(enabled=False) + + actual = get_edx_api_data( + catalog_integration, + self.user, + 'programs', + resource_id=100, + many=False, + ) + + self.assertTrue(mock_warning.called) + self.assertEqual(actual, {}) + + @mock.patch('edx_rest_api_client.client.EdxRestApiClient.__init__') + @mock.patch(UTILITY_MODULE + '.log.exception') + def test_client_initialization_failure_with_id(self, mock_exception, mock_init): + """Verify that an exception is logged when the API client fails to initialize.""" + mock_init.side_effect = Exception + + catalog_integration = self.create_catalog_integration() + + actual = get_edx_api_data( + catalog_integration, + self.user, + 'programs', + resource_id=100, + many=False, + ) + self.assertTrue(mock_exception.called) + self.assertEqual(actual, {}) + + @mock.patch(UTILITY_MODULE + '.log.exception') + def test_data_retrieval_failure_with_id(self, mock_exception): + """Verify that an exception is logged when data can't be retrieved.""" + catalog_integration = self.create_catalog_integration() + api = create_catalog_api_client(self.user, catalog_integration) + + self._mock_catalog_api( + [httpretty.Response(body='clunk', content_type='application/json', status_code=500)] + ) + + actual = get_edx_api_data( + catalog_integration, + self.user, + 'programs', + api=api, + resource_id=100, + many=False, + ) + self.assertTrue(mock_exception.called) + self.assertEqual(actual, {}) + def test_api_client_not_provided(self): """Verify that an API client doesn't need to be provided.""" ClientFactory(name=CredentialsApiConfig.OAUTH2_CLIENT_NAME, client_type=CONFIDENTIAL)