openstack/python-keystoneclient
Code Issues Proposed changes

Remove debug specific handling

Browse Source 
I think debug handling was initially done this way for CLI handling
where we wanted to make sure only the correct information was printed to
the console. However as logging.basicConfig sets up a stream handler on
the root logging object I can't see any purpose to the debug handling in
the actual HTTPClient. Further than this it is completely wrong that a
client library is messing with it's logging level, this should be handled
by an application.

The debug flag is maintained and deprecated in HTTPClient and removed
from the session object. There has been no release since the addition of
session so there is no problem with compatibility.

Change-Id: Ib00f3d93d099ed1a9dd25f17121610a7289f0061
This commit is contained in:
Jamie Lennox
2013-12-17 15:30:51 +10:00
parent 6ad5f2f4cc
commit a8adb476f7
4 changed files with 46 additions and 57 deletions
Download Patch File Download Diff File Expand all files Collapse all files

17
keystoneclient/httpclient.py
View File

@@ -24,8 +24,6 @@ OpenStack Client interface. Handles the REST calls and responses.
import logging import logging
from six.moves.urllib import parse as urlparse from six.moves.urllib import parse as urlparse
import requests
try: try:
import keyring import keyring
import pickle import pickle
@@ -96,9 +94,7 @@ class HTTPClient(object):
:param string cert: DEPRECATED: use session. (optional) :param string cert: DEPRECATED: use session. (optional)
:param boolean insecure: DEPRECATED: use session. (optional) :param boolean insecure: DEPRECATED: use session. (optional)
:param string original_ip: DEPRECATED: use session. (optional) :param string original_ip: DEPRECATED: use session. (optional)
:param boolean debug: Enables debug logging of all request and :param boolean debug: DEPRECATED: use logging configuration. (optional)
responses to identity service.
default False (optional)
:param dict auth_ref: To allow for consumers of the client to manage :param dict auth_ref: To allow for consumers of the client to manage
their own caching strategy, you may initialize a their own caching strategy, you may initialize a
client with a previously captured auth_reference client with a previously captured auth_reference
@@ -240,20 +236,11 @@ class HTTPClient(object):
session = client_session.Session(verify=verify, session = client_session.Session(verify=verify,
cert=session_cert, cert=session_cert,
original_ip=original_ip, original_ip=original_ip,
timeout=timeout, timeout=timeout)
debug=debug)
self.session = session self.session = session
self.domain = '' self.domain = ''
# logging setup
self.debug_log = debug self.debug_log = debug
if self.debug_log and not _logger.handlers:
ch = logging.StreamHandler()
_logger.setLevel(logging.DEBUG)
_logger.addHandler(ch)
if hasattr(requests, 'logging'):
requests.logging.getLogger(requests.__name__).addHandler(ch)
# keyring setup # keyring setup
if use_keyring and keyring is None: if use_keyring and keyring is None:

25
keystoneclient/session.py
View File

@@ -34,7 +34,7 @@ class Session(object):
user_agent = None user_agent = None
def __init__(self, session=None, original_ip=None, verify=True, cert=None, def __init__(self, session=None, original_ip=None, verify=True, cert=None,
timeout=None, debug=False, user_agent=None): timeout=None, user_agent=None):
"""Maintains client communication state and common functionality. """Maintains client communication state and common functionality.
As much as possible the parameters to this class reflect and are passed As much as possible the parameters to this class reflect and are passed
@@ -70,7 +70,6 @@ class Session(object):
self.verify = verify self.verify = verify
self.cert = cert self.cert = cert
self.timeout = None self.timeout = None
self.debug = debug
if timeout is not None: if timeout is not None:
self.timeout = float(timeout) self.timeout = float(timeout)
@@ -79,8 +78,8 @@ class Session(object):
if user_agent is not None: if user_agent is not None:
self.user_agent = user_agent self.user_agent = user_agent
def request(self, url, method, json=None, original_ip=None, debug=None, def request(self, url, method, json=None, original_ip=None,
logger=None, user_agent=None, **kwargs): user_agent=None, **kwargs):
"""Send an HTTP request with the specified characteristics. """Send an HTTP request with the specified characteristics.
Wrapper around `requests.Session.request` to handle tasks such as Wrapper around `requests.Session.request` to handle tasks such as
@@ -94,11 +93,9 @@ class Session(object):
:param string original_ip: Mark this request as forwarded for this ip. :param string original_ip: Mark this request as forwarded for this ip.
(optional) (optional)
:param dict headers: Headers to be included in the request. (optional) :param dict headers: Headers to be included in the request. (optional)
:param bool debug: Enable debug logging. (Defaults to False)
:param kwargs: any other parameter that can be passed to :param kwargs: any other parameter that can be passed to
requests.Session.request (such as `headers`) or `json` requests.Session.request (such as `headers`) or `json`
that will be encoded as JSON and used as `data` argument that will be encoded as JSON and used as `data` argument
:param logging.Logger logger: A logger to output to. (optional)
:param json: Some data to be represented as JSON. (optional) :param json: Some data to be represented as JSON. (optional)
:param string user_agent: A user_agent to use for the request. If :param string user_agent: A user_agent to use for the request. If
present will override one present in headers. present will override one present in headers.
@@ -133,15 +130,8 @@ class Session(object):
headers['Content-Type'] = 'application/json' headers['Content-Type'] = 'application/json'
kwargs['data'] = jsonutils.dumps(json) kwargs['data'] = jsonutils.dumps(json)
if not logger:
logger = _logger
if debug is None:
debug = self.debug
kwargs.setdefault('verify', self.verify) kwargs.setdefault('verify', self.verify)
if debug:
string_parts = ['curl -i'] string_parts = ['curl -i']
if method: if method:
@@ -153,11 +143,11 @@ class Session(object):
for header in six.iteritems(headers): for header in six.iteritems(headers):
string_parts.append(' -H "%s: %s"' % header) string_parts.append(' -H "%s: %s"' % header)
logger.debug('REQ: %s', ''.join(string_parts)) _logger.debug('REQ: %s', ''.join(string_parts))
data = kwargs.get('data') data = kwargs.get('data')
if data: if data:
logger.debug('REQ BODY: %s', data) _logger.debug('REQ BODY: %s', data)
try: try:
resp = self.session.request(method, url, **kwargs) resp = self.session.request(method, url, **kwargs)
@@ -171,12 +161,11 @@ class Session(object):
msg = 'Unable to establish connection to %s' % url msg = 'Unable to establish connection to %s' % url
raise exceptions.ConnectionError(msg) raise exceptions.ConnectionError(msg)
if debug: _logger.debug('RESP: [%s] %s\nRESP BODY: %s\n',
logger.debug('RESP: [%s] %s\nRESP BODY: %s\n',
resp.status_code, resp.headers, resp.text) resp.status_code, resp.headers, resp.text)
if resp.status_code >= 400: if resp.status_code >= 400:
logger.debug('Request returned failure status: %s', _logger.debug('Request returned failure status: %s',
resp.status_code) resp.status_code)
raise exceptions.from_response(resp, method, url) raise exceptions.from_response(resp, method, url)

1
keystoneclient/shell.py
View File

@@ -395,7 +395,6 @@ class OpenStackIdentityShell(object):
key=args.os_key, key=args.os_key,
cert=args.os_cert, cert=args.os_cert,
insecure=args.insecure, insecure=args.insecure,
debug=args.debug,
timeout=args.timeout) timeout=args.timeout)
else: else:
self.auth_check(args) self.auth_check(args)

38
keystoneclient/tests/test_http.py
View File

@@ -15,11 +15,13 @@
# under the License. # under the License.
import httpretty import httpretty
import logging
import six import six
from testtools import matchers from testtools import matchers
from keystoneclient import exceptions from keystoneclient import exceptions
from keystoneclient import httpclient from keystoneclient import httpclient
from keystoneclient import session
from keystoneclient.tests import utils from keystoneclient.tests import utils
RESPONSE_BODY = '{"hi": "there"}' RESPONSE_BODY = '{"hi": "there"}'
@@ -151,7 +153,17 @@ class BasicRequestTests(utils.TestCase):
def setUp(self): def setUp(self):
super(BasicRequestTests, self).setUp() super(BasicRequestTests, self).setUp()
self.logger = FakeLog() self.logger_message = six.moves.cStringIO()
handler = logging.StreamHandler(self.logger_message)
handler.setLevel(logging.DEBUG)
self.logger = logging.getLogger(session.__name__)
level = self.logger.getEffectiveLevel()
self.logger.setLevel(logging.DEBUG)
self.logger.addHandler(handler)
self.addCleanup(self.logger.removeHandler, handler)
self.addCleanup(self.logger.setLevel, level)
def request(self, method='GET', response='Test Response', status=200, def request(self, method='GET', response='Test Response', status=200,
url=None, **kwargs): url=None, **kwargs):
@@ -160,8 +172,7 @@ class BasicRequestTests(utils.TestCase):
httpretty.register_uri(method, url, body=response, status=status) httpretty.register_uri(method, url, body=response, status=status)
return httpclient.request(url, method, debug=True, return httpclient.request(url, method, **kwargs)
logger=self.logger, **kwargs)
@httpretty.activate @httpretty.activate
def test_basic_params(self): def test_basic_params(self):
@@ -173,13 +184,15 @@ class BasicRequestTests(utils.TestCase):
self.assertEqual(httpretty.last_request().method, method) self.assertEqual(httpretty.last_request().method, method)
self.assertThat(self.logger.debug_log, matchers.Contains('curl')) logger_message = self.logger_message.getvalue()
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(logger_message, matchers.Contains('curl'))
self.assertThat(self.logger.debug_log, matchers.Contains(response)) self.assertThat(logger_message, matchers.Contains('-X %s' %
method))
self.assertThat(logger_message, matchers.Contains(self.url))
self.assertThat(logger_message, matchers.Contains(str(status)))
self.assertThat(logger_message, matchers.Contains(response))
@httpretty.activate @httpretty.activate
def test_headers(self): def test_headers(self):
@@ -191,12 +204,13 @@ class BasicRequestTests(utils.TestCase):
self.assertRequestHeaderEqual(k, v) self.assertRequestHeaderEqual(k, v)
for header in six.iteritems(headers): for header in six.iteritems(headers):
self.assertThat(self.logger.debug_log, self.assertThat(self.logger_message.getvalue(),
matchers.Contains('-H "%s: %s"' % header)) matchers.Contains('-H "%s: %s"' % header))
@httpretty.activate @httpretty.activate
def test_body(self): def test_body(self):
data = "BODY DATA" data = "BODY DATA"
self.request(response=data) self.request(response=data)
self.assertThat(self.logger.debug_log, matchers.Contains('BODY:')) logger_message = self.logger_message.getvalue()
self.assertThat(self.logger.debug_log, matchers.Contains(data)) self.assertThat(logger_message, matchers.Contains('BODY:'))
self.assertThat(logger_message, matchers.Contains(data))