Extract basic request call

Create a function out of the standard request call that handles common
headers like user agent and logging.
This makes future changes easier to digest.

Change-Id: Ia25f997df64efdce27c8fb815e544922940145c3

keystoneclient/exceptions.py

@@ -4,6 +4,8 @@
 Exception definitions.
 """
 
+from keystoneclient.openstack.common import jsonutils
+
 
 class CommandError(Exception):
     pass
@@ -143,7 +145,7 @@ _code_map = dict((c.http_status, c) for c in [BadRequest,
                                               ServiceUnavailable])
 
 
-def from_response(response, body):
+def from_response(response, body=None):
     """
     Return an instance of an ClientException or subclass
     based on an requests response.
@@ -155,6 +157,12 @@ def from_response(response, body):
             raise exception_from_response(resp, resp.text)
     """
     cls = _code_map.get(response.status_code, ClientException)
+    if body is None:
+        try:
+            body = jsonutils.loads(response.text)
+        except Exception:
+            body = response.text
+
     if body:
         if hasattr(body, 'keys'):
             error = body[body.keys()[0]]

keystoneclient/httpclient.py

@@ -39,9 +39,84 @@ from keystoneclient import exceptions
 _logger = logging.getLogger(__name__)
 
 
-class HTTPClient(object):
+USER_AGENT = 'python-keystoneclient'
 
-    USER_AGENT = 'python-keystoneclient'
+
+def request(url, method='GET', headers=None, original_ip=None, debug=False,
+            logger=None, **kwargs):
+    """Perform a http request with standard settings.
+
+    A wrapper around requests.request that adds standard headers like
+    User-Agent and provides optional debug logging of the request.
+
+    Arguments that are not handled are passed through to the requests library.
+
+    :param string url: The url to make the request of.
+    :param string method: The http method to use. (eg. 'GET', 'POST')
+    :param dict headers: Headers to be included in the request. (optional)
+    :param string original_ip: Mark this request as forwarded for this ip.
+                               (optional)
+    :param bool debug: Enable debug logging. (Defaults to False)
+    :param logging.Logger logger: A logger to output to. (optional)
+
+    :raises exceptions.ClientException: For connection failure, or to indicate
+                                        an error response code.
+
+    :returns: The response to the request.
+    """
+
+    if not headers:
+        headers = dict()
+
+    if not logger:
+        logger = _logger
+
+    headers.setdefault('User-Agent', USER_AGENT)
+
+    if original_ip:
+        headers['Forwarded'] = "for=%s;by=%s" % (original_ip, USER_AGENT)
+
+    if debug:
+        string_parts = ['curl -i']
+
+        if method:
+            string_parts.append(' -X %s' % method)
+
+        string_parts.append(' %s' % url)
+
+        if headers:
+            for header in headers.iteritems():
+                string_parts.append(' -H "%s: %s"' % header)
+
+        logger.debug("REQ: %s" % "".join(string_parts))
+
+        data = kwargs.get('data')
+        if data:
+            logger.debug("REQ BODY: %s\n" % data)
+
+    try:
+        resp = requests.request(
+            method,
+            url,
+            headers=headers,
+            **kwargs)
+    except requests.ConnectionError:
+        msg = 'Unable to establish connection to %s' % url
+        raise exceptions.ClientException(msg)
+
+    if debug:
+        logger.debug("RESP: [%s] %s\nRESP BODY: %s\n",
+                     resp.status_code, resp.headers, resp.text)
+
+    if resp.status_code >= 400:
+        logger.debug("Request returned failure status: %s",
+                     resp.status_code)
+        raise exceptions.from_response(resp)
+
+    return resp
+
+
+class HTTPClient(object):
 
     def __init__(self, username=None, tenant_id=None, tenant_name=None,
                  password=None, auth_url=None, region_name=None, timeout=None,
@@ -483,33 +558,6 @@ class HTTPClient(object):
         """
         raise NotImplementedError
 
-    def http_log_req(self, args, kwargs):
-        if not self.debug_log:
-            return
-
-        string_parts = ['curl -i']
-        for element in args:
-            if element in ('GET', 'POST'):
-                string_parts.append(' -X %s' % element)
-            else:
-                string_parts.append(' %s' % element)
-
-        for element in kwargs['headers']:
-            header = ' -H "%s: %s"' % (element, kwargs['headers'][element])
-            string_parts.append(header)
-
-        _logger.debug("REQ: %s" % "".join(string_parts))
-        if 'data' in kwargs:
-            _logger.debug("REQ BODY: %s\n" % (kwargs['data']))
-
-    def http_log_resp(self, resp):
-        if self.debug_log:
-            _logger.debug(
-                "RESP: [%s] %s\nRESP BODY: %s\n",
-                resp.status_code,
-                resp.headers,
-                resp.text)
-
     def serialize(self, entity):
         return json.dumps(entity)
 
@@ -522,7 +570,7 @@ class HTTPClient(object):
         """Returns True if this client provides a service catalog."""
         return self.auth_ref.has_service_catalog()
 
-    def request(self, url, method, **kwargs):
+    def request(self, url, method, body=None, **kwargs):
         """Send an http request with the specified characteristics.
 
         Wrapper around requests.request to handle tasks such as
@@ -531,54 +579,37 @@ class HTTPClient(object):
         # Copy the kwargs so we can reuse the original in case of redirects
         request_kwargs = copy.copy(kwargs)
         request_kwargs.setdefault('headers', kwargs.get('headers', {}))
-        request_kwargs['headers']['User-Agent'] = self.USER_AGENT
+
-        if self.original_ip:
+        if body:
-            request_kwargs['headers']['Forwarded'] = "for=%s;by=%s" % (
-                self.original_ip, self.USER_AGENT)
-        if 'body' in kwargs:
             request_kwargs['headers']['Content-Type'] = 'application/json'
-            request_kwargs['data'] = self.serialize(kwargs['body'])
+            request_kwargs['data'] = self.serialize(body)
-            del request_kwargs['body']
+
         if self.cert:
-            request_kwargs['cert'] = self.cert
+            request_kwargs.setdefault('cert', self.cert)
         if self.timeout is not None:
             request_kwargs.setdefault('timeout', self.timeout)
 
-        self.http_log_req((url, method,), request_kwargs)
+        resp = request(url, method, original_ip=self.original_ip,
-
+                       verify=self.verify_cert, debug=self.debug_log,
-        try:
-            resp = requests.request(
-                method,
-                url,
-                verify=self.verify_cert,
                        **request_kwargs)
-        except requests.ConnectionError:
-            msg = 'Unable to establish connection to %s' % url
-            raise exceptions.ClientException(msg)
-
-        self.http_log_resp(resp)
 
         if resp.text:
             try:
-                body = json.loads(resp.text)
+                body_resp = json.loads(resp.text)
             except (ValueError, TypeError):
-                body = None
+                body_resp = None
                 _logger.debug("Could not decode JSON from body: %s"
                               % resp.text)
         else:
             _logger.debug("No body was returned.")
-            body = None
+            body_resp = None
 
-        if resp.status_code >= 400:
+        if resp.status_code in (301, 302, 305):
-            _logger.debug(
-                "Request returned failure status: %s",
-                resp.status_code)
-            raise exceptions.from_response(resp, body or resp.text)
-        elif resp.status_code in (301, 302, 305):
             # Redirected. Reissue the request to the new location.
-            return self.request(resp.headers['location'], method, **kwargs)
+            return self.request(resp.headers['location'], method, body,
+                                **request_kwargs)
 
-        return resp, body
+        return resp, body_resp
 
     def _cs_request(self, url, method, **kwargs):
         """Makes an authenticated request to keystone endpoint by

test-requirements.txt

@@ -3,6 +3,7 @@ hacking>=0.5.6,<0.7
 coverage>=3.6
 discover
 fixtures>=0.3.12
+httpretty>=0.6.3
 keyring>=1.6.1
 mock>=0.8.0
 mox>=0.5.3

tests/test_http.py

@@ -1,7 +1,26 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+
+# Copyright 2013 OpenStack LLC
+#
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License. You may obtain
+# a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+
 import json
 import mock
 
+import httpretty
 import requests
+import testtools
+from testtools import matchers
 
 from keystoneclient import exceptions
 from keystoneclient import httpclient
@@ -28,6 +47,18 @@ def get_authed_client():
     return cl
 
 
+class FakeLog(object):
+    def __init__(self):
+        self.warn_log = str()
+        self.debug_log = str()
+
+    def warn(self, msg=None, *args, **kwargs):
+        self.warn_log = "%s\n%s" % (self.warn_log, (msg % args))
+
+    def debug(self, msg=None, *args, **kwargs):
+        self.debug_log = "%s\n%s" % (self.debug_log, (msg % args))
+
+
 class ClientTest(utils.TestCase):
 
     def test_unauthorized_client_requests(self):
@@ -44,7 +75,7 @@ class ClientTest(utils.TestCase):
             with mock.patch('time.time', mock.Mock(return_value=1234)):
                 resp, body = cl.get("/hi")
                 headers = {"X-Auth-Token": "token",
-                           "User-Agent": cl.USER_AGENT}
+                           "User-Agent": httpclient.USER_AGENT}
                 MOCK_REQUEST.assert_called_with(
                     "GET",
                     "http://127.0.0.1:5000/hi",
@@ -97,7 +128,7 @@ class ClientTest(utils.TestCase):
             headers = {
                 "X-Auth-Token": "token",
                 "Content-Type": "application/json",
-                "User-Agent": cl.USER_AGENT
+                "User-Agent": httpclient.USER_AGENT
             }
             MOCK_REQUEST.assert_called_with(
                 "POST",
@@ -117,5 +148,69 @@ class ClientTest(utils.TestCase):
 
             args, kwargs = MOCK_REQUEST.call_args
             self.assertIn(
-                ('Forwarded', "for=%s;by=%s" % (ORIGINAL_IP, cl.USER_AGENT)),
+                ('Forwarded', "for=%s;by=%s" % (ORIGINAL_IP,
+                                                httpclient.USER_AGENT)),
                 kwargs['headers'].items())
+
+
+class BasicRequestTests(testtools.TestCase):
+
+    url = 'http://keystone.test.com/'
+
+    def setUp(self):
+        super(BasicRequestTests, self).setUp()
+        self.logger = FakeLog()
+        httpretty.enable()
+
+    def tearDown(self):
+        httpretty.disable()
+        super(BasicRequestTests, self).tearDown()
+
+    def request(self, method='GET', response='Test Response', status=200,
+                url=None, **kwargs):
+        if not url:
+            url = self.url
+
+        httpretty.register_uri(method, url, body=response, status=status)
+
+        return httpclient.request(url, method, debug=True,
+                                  logger=self.logger, **kwargs)
+
+    @property
+    def last_request(self):
+        return httpretty.httpretty.last_request
+
+    def test_basic_params(self):
+        method = 'GET'
+        response = 'Test Response'
+        status = 200
+
+        resp = self.request(method=method, status=status, response=response)
+
+        self.assertEqual(self.last_request.method, method)
+
+        self.assertThat(self.logger.debug_log, matchers.Contains('curl'))
+        self.assertThat(self.logger.debug_log, matchers.Contains('-X %s' %
+                                                                 method))
+        self.assertThat(self.logger.debug_log, matchers.Contains(self.url))
+
+        self.assertThat(self.logger.debug_log, matchers.Contains(str(status)))
+        self.assertThat(self.logger.debug_log, matchers.Contains(response))
+
+    def test_headers(self):
+        headers = {'key': 'val', 'test': 'other'}
+
+        self.request(headers=headers)
+
+        for k, v in headers.iteritems():
+            self.assertEqual(self.last_request.headers[k], v)
+
+        for header in headers.iteritems():
+            self.assertThat(self.logger.debug_log,
+                            matchers.Contains('-H "%s: %s"' % header))
+
+    def test_body(self):
+        data = "BODY DATA"
+        resp = self.request(response=data)
+        self.assertThat(self.logger.debug_log, matchers.Contains('BODY:'))
+        self.assertThat(self.logger.debug_log, matchers.Contains(data))

tests/test_https.py

@@ -37,7 +37,7 @@ class ClientTest(utils.TestCase):
             with mock.patch('time.time', mock.Mock(return_value=1234)):
                 resp, body = cl.get("/hi")
                 headers = {"X-Auth-Token": "token",
-                           "User-Agent": cl.USER_AGENT}
+                           "User-Agent": httpclient.USER_AGENT}
                 kwargs = copy.copy(self.TEST_REQUEST_BASE)
                 kwargs['cert'] = ('cert.pem', 'key.pem')
                 kwargs['verify'] = 'ca.pem'
@@ -57,7 +57,7 @@ class ClientTest(utils.TestCase):
             headers = {
                 "X-Auth-Token": "token",
                 "Content-Type": "application/json",
-                "User-Agent": cl.USER_AGENT
+                "User-Agent": httpclient.USER_AGENT
             }
             kwargs = copy.copy(self.TEST_REQUEST_BASE)
             kwargs['cert'] = ('cert.pem', 'key.pem')
@@ -81,7 +81,7 @@ class ClientTest(utils.TestCase):
             headers = {
                 "X-Auth-Token": "token",
                 "Content-Type": "application/json",
-                "User-Agent": cl.USER_AGENT
+                "User-Agent": httpclient.USER_AGENT
             }
             kwargs = copy.copy(self.TEST_REQUEST_BASE)
             kwargs['cert'] = ('cert.pem', 'key.pem')