Revert "Do not expose exceptions from requests library"
This reverts commit 2961e82bfa6e375ac3c17933fac532ed316ac976 This broke cinder unit tests that have a timeout test for novaclient where they mock the requests library raising a Timeout exception. That test expects to get a requests Timeout passed through but with the change being reverted it was getting a RequestTimeout exception from novaclient, which breaks the test and fails the gate runs. We could change the cinder unit test to handle both the requests.Timeout and the new novaclient RequestTimeout, but that's just papering over the cinder failure, we wouldn't know what other clients are failing in the same way because they have code working around the requests exceptions getting passed through, so we should treat this like an API change. I'd be in favor of making the same change again iff the novaclient exceptions extended the requests exception types so it would be transparent to consumers, since novaclient.exceptions.RequestTimeout would be a requests.Timeout via inheritance. But given the gate breakage and it being summit week I'm inclined to revert first and think about a better long term fix later when people are back to discuss. Change-Id: I368728588e5997eef860a168539eb66c58f2e72a Closes-Bug: #1510790
This commit is contained in:
Matt Riedemann
parent
217e7c1849
commit
63c7a576e1
@ -84,12 +84,13 @@ class SessionClient(adapter.LegacyJsonAdapter):
|
||||
# NOTE(jamielennox): The standard call raises errors from
|
||||
# keystoneclient, where we need to raise the novaclient errors.
|
||||
raise_exc = kwargs.pop('raise_exc', True)
|
||||
with utils.converted_exceptions():
|
||||
with utils.record_time(self.times, self.timings, method, url):
|
||||
resp, body = super(SessionClient, self).request(
|
||||
url, method, raise_exc=False, **kwargs)
|
||||
if raise_exc and resp.status_code >= 400:
|
||||
raise exceptions.from_response(resp, body, url, method)
|
||||
with utils.record_time(self.times, self.timings, method, url):
|
||||
resp, body = super(SessionClient, self).request(url,
|
||||
method,
|
||||
raise_exc=False,
|
||||
**kwargs)
|
||||
if raise_exc and resp.status_code >= 400:
|
||||
raise exceptions.from_response(resp, body, url, method)
|
||||
|
||||
return resp, body
|
||||
|
||||
@ -359,11 +360,10 @@ class HTTPClient(object):
|
||||
if session:
|
||||
request_func = session.request
|
||||
|
||||
with utils.converted_exceptions():
|
||||
resp = request_func(
|
||||
method,
|
||||
url,
|
||||
**kwargs)
|
||||
resp = request_func(
|
||||
method,
|
||||
url,
|
||||
**kwargs)
|
||||
|
||||
self.http_log_resp(resp)
|
||||
|
||||
|
||||
@ -283,26 +283,3 @@ def from_response(response, body, url, method=None):
|
||||
class ResourceNotFound(Exception):
|
||||
"""Error in getting the resource."""
|
||||
pass
|
||||
|
||||
|
||||
class RequestTimeout(ClientException):
|
||||
"""
|
||||
HTTP 408 - Request Timeout
|
||||
"""
|
||||
http_status = 408
|
||||
message = "Request Timeout"
|
||||
|
||||
|
||||
class TooManyRedirects(Exception):
|
||||
"""Too many redirects."""
|
||||
pass
|
||||
|
||||
|
||||
class ConnectionError(Exception):
|
||||
"""A Connection error occurred."""
|
||||
pass
|
||||
|
||||
|
||||
class RequestException(Exception):
|
||||
"""An ambiguous exception occurred."""
|
||||
pass
|
||||
|
||||
@ -21,7 +21,6 @@ import fixtures
|
||||
from keystoneclient import adapter
|
||||
import mock
|
||||
import requests
|
||||
import six
|
||||
|
||||
import novaclient.client
|
||||
import novaclient.extension
|
||||
@ -439,35 +438,6 @@ class ClientTest(utils.TestCase):
|
||||
self.assertEqual(1, len(client.times))
|
||||
self.assertEqual('GET http://no.where', client.times[0][0])
|
||||
|
||||
@mock.patch.object(requests, 'request')
|
||||
def test_request_exception_conversions(self, m_request):
|
||||
client = novaclient.client.HTTPClient(user='zqfan', password='')
|
||||
|
||||
exc = requests.HTTPError()
|
||||
exc.response = requests.Response()
|
||||
exc.response.status_code = 12345
|
||||
m_request.side_effect = exc
|
||||
exc = self.assertRaises(novaclient.exceptions.ClientException,
|
||||
client.request, "http://www.openstack.org",
|
||||
'GET')
|
||||
self.assertIn("HTTP 12345", six.text_type(exc))
|
||||
|
||||
m_request.side_effect = requests.ConnectionError()
|
||||
self.assertRaises(novaclient.exceptions.ConnectionError,
|
||||
client.request, "http://www.openstack.org", 'GET')
|
||||
|
||||
m_request.side_effect = requests.Timeout()
|
||||
self.assertRaises(novaclient.exceptions.RequestTimeout,
|
||||
client.request, "http://www.openstack.org", 'GET')
|
||||
|
||||
m_request.side_effect = requests.TooManyRedirects()
|
||||
self.assertRaises(novaclient.exceptions.TooManyRedirects,
|
||||
client.request, "http://www.openstack.org", 'GET')
|
||||
|
||||
m_request.side_effect = requests.RequestException()
|
||||
self.assertRaises(novaclient.exceptions.RequestException,
|
||||
client.request, "http://www.openstack.org", 'GET')
|
||||
|
||||
|
||||
class SessionClientTest(utils.TestCase):
|
||||
|
||||
@ -484,32 +454,3 @@ class SessionClientTest(utils.TestCase):
|
||||
client.request("http://no.where", 'GET')
|
||||
self.assertEqual(1, len(client.times))
|
||||
self.assertEqual('GET http://no.where', client.times[0][0])
|
||||
|
||||
@mock.patch.object(adapter.LegacyJsonAdapter, 'request')
|
||||
def test_request_exception_conversions(self, m_request):
|
||||
client = novaclient.client.SessionClient(session=mock.MagicMock())
|
||||
|
||||
exc = requests.HTTPError()
|
||||
exc.response = requests.Response()
|
||||
exc.response.status_code = 12345
|
||||
m_request.side_effect = exc
|
||||
exc = self.assertRaises(novaclient.exceptions.ClientException,
|
||||
client.request, "http://www.openstack.org",
|
||||
'GET')
|
||||
self.assertIn("HTTP 12345", six.text_type(exc))
|
||||
|
||||
m_request.side_effect = requests.ConnectionError()
|
||||
self.assertRaises(novaclient.exceptions.ConnectionError,
|
||||
client.request, "http://www.openstack.org", 'GET')
|
||||
|
||||
m_request.side_effect = requests.Timeout()
|
||||
self.assertRaises(novaclient.exceptions.RequestTimeout,
|
||||
client.request, "http://www.openstack.org", 'GET')
|
||||
|
||||
m_request.side_effect = requests.TooManyRedirects()
|
||||
self.assertRaises(novaclient.exceptions.TooManyRedirects,
|
||||
client.request, "http://www.openstack.org", 'GET')
|
||||
|
||||
m_request.side_effect = requests.RequestException()
|
||||
self.assertRaises(novaclient.exceptions.RequestException,
|
||||
client.request, "http://www.openstack.org", 'GET')
|
||||
|
||||
@ -22,7 +22,6 @@ from oslo_serialization import jsonutils
|
||||
from oslo_utils import encodeutils
|
||||
import pkg_resources
|
||||
import prettytable
|
||||
import requests
|
||||
import six
|
||||
|
||||
from novaclient import exceptions
|
||||
@ -377,29 +376,6 @@ def record_time(times, enabled, *args):
|
||||
times.append((' '.join(args), start, end))
|
||||
|
||||
|
||||
@contextlib.contextmanager
|
||||
def converted_exceptions():
|
||||
try:
|
||||
yield
|
||||
|
||||
except requests.HTTPError as e:
|
||||
status_code = e.response.status_code
|
||||
raise exceptions.ClientException(status_code,
|
||||
encodeutils.exception_to_unicode(e))
|
||||
|
||||
except requests.ConnectionError as e:
|
||||
raise exceptions.ConnectionError(encodeutils.exception_to_unicode(e))
|
||||
|
||||
except requests.Timeout as e:
|
||||
raise exceptions.RequestTimeout(encodeutils.exception_to_unicode(e))
|
||||
|
||||
except requests.TooManyRedirects as e:
|
||||
raise exceptions.TooManyRedirects(encodeutils.exception_to_unicode(e))
|
||||
|
||||
except requests.RequestException as e:
|
||||
raise exceptions.RequestException(encodeutils.exception_to_unicode(e))
|
||||
|
||||
|
||||
def get_function_name(func):
|
||||
if six.PY2:
|
||||
if hasattr(func, "im_class"):
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user