Revert "Do not expose exceptions from requests library"
This reverts commit 2961e82bfa
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…
Reference in New Issue