From 160ce83f96b963e02c534dfa0eb52cfa0aac9cfb Mon Sep 17 00:00:00 2001 From: Andy Chosak Date: Tue, 19 Dec 2017 21:51:09 -0500 Subject: [PATCH 1/2] pull from regulations-core with python, not http This change makes it possible for regulations-site to pull content from a regulations-core API running in the same Django instance by directly making Python calls instead of using HTTP requests. By defining the Django setting `EREGS_REGCORE_URLS` and setting it to the path to your regulations-core URLconf (likely `regcore.urls`), the API will, instead of using HTTP to make requests, just call the Django views directly. When running in the same Django installation as regulations-core this saves time and reduces load on the Apache server. If this setting is not defined then behavior falls back to the current method of using HTTP requests based on the EREGS_API_BASE environment variable. The best way to test this in action is to observe that if you run the Django debug server with this setting, you won't see any requests to the regcore API when making regsite requests. I've added several unit tests that attempt to cover this change. Behavior should be similar to the current HTTP-based approach. If the Django view returns a 404 or raises an `Http404` then the API returns `None`. If the view raises an exception, then the API will pass that along. Otherwise, the view just passes back the JSON content of the regcore view response as if it had been retrieved via HTTP. --- regulations/generator/api_client.py | 77 +++++++++++++++++++++----- regulations/settings/base.py | 9 ++- regulations/tests/api_client_tests.py | 44 ++++++++++++++- regulations/tests/mock_regcore_urls.py | 33 +++++++++++ 4 files changed, 147 insertions(+), 16 deletions(-) create mode 100644 regulations/tests/mock_regcore_urls.py diff --git a/regulations/generator/api_client.py b/regulations/generator/api_client.py index d49759c2..2ed44125 100644 --- a/regulations/generator/api_client.py +++ b/regulations/generator/api_client.py @@ -1,16 +1,29 @@ +from importlib import import_module import json import os + from django.conf import settings +from django.core.urlresolvers import Resolver404, resolve +from django.http import Http404 +from django.test import RequestFactory +from django.utils.functional import cached_property import requests class ApiClient: - """ Actually go out and make the GET request, or read the disk to acquire - the required data. """ + """Retrieve regulations data via Python, HTTP, or disk. + Optionally define settings.EREGS_REGCORE_URLS to the module name of the + related cfpb/regulations-core project (e.g. 'regcore.urls') to use a + runtime import to handle requests instead of HTTP. + """ def __init__(self): self.base_url = settings.API_BASE + self.regcore_urls = getattr(settings, 'EREGS_REGCORE_URLS', None) + if self.regcore_urls: + self.regcore_urls = import_module(self.regcore_urls) + def get_from_file_system(self, suffix): if os.path.isdir(self.base_url + suffix): suffix = suffix + "/index.html" @@ -19,17 +32,55 @@ def get_from_file_system(self, suffix): f.close() return json.loads(content) + @cached_property + def request_factory(self): + return RequestFactory() + + def get_from_http(self, suffix, params={}): + url = self.base_url + suffix + r = requests.get(url, params=params) + if r.status_code == requests.codes.ok: + return r.json() + elif r.status_code == 404: + return None + else: + r.raise_for_status() + + def get_from_regcore(self, suffix, params={}): + path = '/' + suffix + + try: + func, args, kwargs = resolve(path, urlconf=self.regcore_urls) + except Resolver404: + return None + + request = self.request_factory.get(path, data=params) + + try: + response = func(request, *args, **kwargs) + except Http404: + return None + + if response.status_code == 404: + return None + + # This mimics the behavior of requests.raise_for_status: + # https://github.com/requests/requests/blob/v2.12.4/requests/models.py#L870 + if 400 <= response.status_code < 600: + raise RuntimeError( + 'regcore path {} returned status code {}: {}'.format( + path, + response.status_code, + response.content + ) + ) + + return json.loads(response.content) + def get(self, suffix, params={}): - """Make the GET request. Assume the result is JSON. Right now, there is - no error handling""" - - if self.base_url.startswith('http'): - r = requests.get(self.base_url + suffix, params=params) - if r.status_code == requests.codes.ok: - return r.json() - elif r.status_code == 404: - return None - else: - r.raise_for_status() + if self.regcore_urls: + return self.get_from_regcore(suffix, params=params) + elif self.base_url.startswith('http'): + return self.get_from_http(suffix, params=params) else: return self.get_from_file_system(suffix) diff --git a/regulations/settings/base.py b/regulations/settings/base.py index 29594acf..03e8695f 100644 --- a/regulations/settings/base.py +++ b/regulations/settings/base.py @@ -16,7 +16,14 @@ MANAGERS = ADMINS -DATABASES = {} + +# This default database is required to use full Django unit test functionality. +DATABASES = { + 'default': { + 'ENGINE': 'django.db.backends.sqlite3', + 'NAME': ':memory:', + } +} # Hosts/domain names that are valid for this site; required if DEBUG is False # See https://docs.djangoproject.com/en/1.5/ref/settings/#allowed-hosts diff --git a/regulations/tests/api_client_tests.py b/regulations/tests/api_client_tests.py index b9ecf52d..d050c125 100644 --- a/regulations/tests/api_client_tests.py +++ b/regulations/tests/api_client_tests.py @@ -1,9 +1,11 @@ -import tempfile import os import shutil +import tempfile + +from django.conf import settings +from django.test import TestCase, override_settings from regulations.generator.api_client import ApiClient -from unittest import TestCase class ClientTest(TestCase): @@ -20,3 +22,41 @@ def test_local_filesystem(self): results = client.get('notice') shutil.rmtree(tmp_root) self.assertEqual(["example"], results['results']) + + +@override_settings(EREGS_REGCORE_URLS='regulations.tests.mock_regcore_urls') +class ClientUsingRegCoreTests(TestCase): + @override_settings() + def test_no_setting_doesnt_set_regcore_urls(self): + del settings.EREGS_REGCORE_URLS + self.assertIsNone(ApiClient().regcore_urls) + + @override_settings(EREGS_REGCORE_URLS='this.does.not.exist') + def test_invalid_setting_raises_import_error(self): + with self.assertRaises(ImportError): + ApiClient() + + def test_valid_setting_sets_regcore_urls(self): + self.assertEqual( + ApiClient().regcore_urls.__name__, + 'regulations.tests.mock_regcore_urls' + ) + + def test_valid_request_returns_content(self): + self.assertEqual(ApiClient().get('returns-200'), {'foo': 'bar'}) + + def test_valid_request_passes_params(self): + self.assertEqual( + ApiClient().get('returns-get', params={'zap': 'boom'}), + {'zap': 'boom'} + ) + + def test_request_returning_404_returns_none(self): + self.assertIsNone(ApiClient().get('returns-404')) + + def test_request_raising_http404_returns_none(self): + self.assertIsNone(ApiClient().get('raises-http404')) + + def test_request_raising_exception_returns_that_exception(self): + with self.assertRaises(RuntimeError): + ApiClient().get('raises-exception') diff --git a/regulations/tests/mock_regcore_urls.py b/regulations/tests/mock_regcore_urls.py new file mode 100644 index 00000000..8981d99e --- /dev/null +++ b/regulations/tests/mock_regcore_urls.py @@ -0,0 +1,33 @@ +from __future__ import absolute_import, unicode_literals + +from django.conf.urls import url +from django.http import Http404, HttpResponseNotFound, JsonResponse + + +def returns200(request): + return JsonResponse({'foo': 'bar'}) + + +def returnsGet(request): + return JsonResponse(request.GET) + + +def returns404(request): + return HttpResponseNotFound('not found') + + +def raisesHttp404(request): + raise Http404('not found') + + +def raisesException(request): + raise RuntimeError('something bad happened') + + +urlpatterns = [ + url('returns-200', returns200), + url('returns-get', returnsGet), + url('returns-404', returns404), + url('raises-http404', raisesHttp404), + url('raises-exception', raisesException), +] From cc2a88ec23da5fbf43d4e1801741a2fa4fad59d7 Mon Sep 17 00:00:00 2001 From: Andy Chosak Date: Tue, 19 Dec 2017 23:23:50 -0500 Subject: [PATCH 2/2] add an additional unit test --- regulations/generator/api_client.py | 2 ++ regulations/tests/api_client_tests.py | 3 +++ 2 files changed, 5 insertions(+) diff --git a/regulations/generator/api_client.py b/regulations/generator/api_client.py index 2ed44125..f2bd4e42 100644 --- a/regulations/generator/api_client.py +++ b/regulations/generator/api_client.py @@ -52,6 +52,8 @@ def get_from_regcore(self, suffix, params={}): try: func, args, kwargs = resolve(path, urlconf=self.regcore_urls) except Resolver404: + # This mimics the behavior of a 404 from the regcore API for + # an invalid request that doesn't match a URL pattern. return None request = self.request_factory.get(path, data=params) diff --git a/regulations/tests/api_client_tests.py b/regulations/tests/api_client_tests.py index d050c125..bd527e42 100644 --- a/regulations/tests/api_client_tests.py +++ b/regulations/tests/api_client_tests.py @@ -60,3 +60,6 @@ def test_request_raising_http404_returns_none(self): def test_request_raising_exception_returns_that_exception(self): with self.assertRaises(RuntimeError): ApiClient().get('raises-exception') + + def test_unresolvable_request_returns_none(self): + self.assertIsNone(ApiClient().get('this-doesnt-resolve'))