diff --git a/sushy/connector.py b/sushy/connector.py index c3e50e9f..df88ea17 100644 --- a/sushy/connector.py +++ b/sushy/connector.py @@ -61,22 +61,20 @@ class Connector(object): url = parse.urljoin(self._url, path) # TODO(lucasagomes): We should mask the data to remove sensitive # information - LOG.debug('Issuing a HTTP %(method)s request at %(url)s with ' - 'the headers "%(headers)s" and data "%(data)s"', + LOG.debug('HTTP request: %(method)s %(url)s; ' + 'headers: %(headers)s; body: %(data)s', {'method': method, 'url': url, 'headers': headers, - 'data': data or ''}) + 'data': data}) try: response = session.request(method, url, data=data) except requests.ConnectionError as e: raise exceptions.ConnectionError(url=url, error=e) - LOG.debug('Response: Status code: %d', response.status_code) - try: - response.raise_for_status() - except requests.HTTPError as e: - raise exceptions.HTTPError( - method=method, url=url, error=e, - status_code=e.response.status_code) + exceptions.raise_for_response(method, url, response) + LOG.debug('HTTP response for %(method)s %(url)s: ' + 'status code: %(code)s', + {'method': method, 'url': url, + 'code': response.status_code}) return response diff --git a/sushy/exceptions.py b/sushy/exceptions.py index 696708ac..c3391f34 100644 --- a/sushy/exceptions.py +++ b/sushy/exceptions.py @@ -13,6 +13,11 @@ # License for the specific language governing permissions and limitations # under the License. +import logging + + +LOG = logging.getLogger(__name__) + class SushyError(Exception): """Basic exception for errors raised by Sushy""" @@ -49,15 +54,71 @@ class HTTPError(SushyError): """Basic exception for HTTP errors""" status_code = None - message = ('Error issuing a %(method)s request at %(url)s. ' - 'Error: %(error)s') + """HTTP status code.""" - def __init__(self, status_code=None, **kwargs): + body = None + """Error JSON body, if present.""" + + code = 'Base.1.0.GeneralError' + """Error code defined in the Redfish specification, if present.""" + + detail = None + """Error message defined in the Redfish specification, if present.""" + + message = ('HTTP %(method)s %(url)s returned code %(code)s. %(error)s') + + def __init__(self, method, url, response): + self.status_code = response.status_code + try: + body = response.json() + except ValueError: + LOG.warning('Error response from %(method)s %(url)s ' + 'with status code %(code)s has no JSON body', + {'method': method, 'url': url, 'code': + self.status_code}) + error = 'unknown error' + else: + # TODO(dtantsur): parse @Message.ExtendedInfo + self.body = body.get('error', {}) + self.code = self.body.get('code', 'Base.1.0.GeneralError') + self.detail = self.body.get('message') + error = '%s: %s' % (self.code, self.detail or 'unknown error') + + kwargs = {'method': method, 'url': url, 'code': self.status_code, + 'error': error} + LOG.debug('HTTP response for %(method)s %(url)s: ' + 'status code: %(code)s, error: %(error)s', kwargs) super(HTTPError, self).__init__(**kwargs) - if status_code is not None: - self.status_code = status_code + + +class BadRequestError(HTTPError): + pass class ResourceNotFoundError(HTTPError): - status_code = 404 - message = 'Resource %(resource)s not found' + # Overwrite the complex generic message with a simpler one. + message = 'Resource %(url)s not found' + + +class ServerSideError(HTTPError): + pass + + +class AccessError(HTTPError): + pass + + +def raise_for_response(method, url, response): + """Raise a correct error class, if needed.""" + if response.status_code < 400: + return + elif response.status_code == 404: + raise ResourceNotFoundError(method, url, response) + elif response.status_code == 400: + raise BadRequestError(method, url, response) + elif response.status_code in (401, 403): + raise AccessError(method, url, response) + elif response.status_code >= 500: + raise ServerSideError(method, url, response) + else: + raise HTTPError(method, url, response) diff --git a/sushy/resources/base.py b/sushy/resources/base.py index f0f7e493..32c524b9 100644 --- a/sushy/resources/base.py +++ b/sushy/resources/base.py @@ -14,13 +14,16 @@ # under the License. import abc +import logging import six -from sushy import exceptions from sushy import utils +LOG = logging.getLogger(__name__) + + @six.add_metaclass(abc.ABCMeta) class ResourceBase(object): @@ -52,14 +55,10 @@ class ResourceBase(object): :raises: ConnectionError :raises: HTTPError """ - try: - resp = self._conn.get(path=self._path) - except exceptions.HTTPError as e: - if e.status_code == 404: - raise exceptions.ResourceNotFoundError(resource=self._path) - raise - - self._json = resp.json() + self._json = self._conn.get(path=self._path).json() + LOG.debug('Received representation of %(type)s %(path)s: %(json)s', + {'type': self.__class__.__name__, + 'path': self._path, 'json': self._json}) self._parse_attributes() @property @@ -114,6 +113,9 @@ class ResourceCollectionBase(ResourceBase): self.name = self.json.get('Name') self.members_identities = ( utils.get_members_identities(self.json.get('Members', []))) + LOG.debug('Received %(count)d member(s) for %(type)s %(path)s', + {'count': len(self.members_identities), + 'type': self.__class__.__name__, 'path': self._path}) def get_member(self, identity): """Given the identity return a ``_resource_type`` object diff --git a/sushy/tests/unit/json_samples/error.json b/sushy/tests/unit/json_samples/error.json new file mode 100644 index 00000000..89598b18 --- /dev/null +++ b/sushy/tests/unit/json_samples/error.json @@ -0,0 +1,35 @@ +{ + "error": { + "code": "Base.1.0.GeneralError", + "message": "A general error has occurred. See ExtendedInfo for more information.", + "@Message.ExtendedInfo": [ + { + "@odata.type" : "/redfish/v1/$metadata#Message.1.0.0.Message", + "MessageId": "Base.1.0.PropertyValueNotInList", + "RelatedProperties": [ + "#/IndicatorLED" + ], + "Message": "The value Red for the property IndicatorLED is not in the list of acceptable values", + "MessageArgs": [ + "RED", + "IndicatorLED" + ], + "Severity": "Warning", + "Resolution": "Remove the property from the request body and resubmit the request if the operation failed" + }, + { + "@odata.type" : "/redfish/v1/$metadata#Message.1.0.0.Message", + "MessageId": "Base.1.0.PropertyNotWriteable", + "RelatedProperties": [ + "#/SKU" + ], + "Message": "The property SKU is a read only property and cannot be assigned a value", + "MessageArgs": [ + "SKU" + ], + "Severity": "Warning", + "Resolution": "Remove the property from the request body and resubmit the request if the operation failed" + } + ] + } +} diff --git a/sushy/tests/unit/resources/test_base.py b/sushy/tests/unit/resources/test_base.py index 569877eb..f419698e 100644 --- a/sushy/tests/unit/resources/test_base.py +++ b/sushy/tests/unit/resources/test_base.py @@ -20,7 +20,7 @@ from sushy.resources import base as resource_base from sushy.tests.unit import base -class BaseResouce(resource_base.ResourceBase): +class BaseResource(resource_base.ResourceBase): def _parse_attributes(self): pass @@ -31,8 +31,8 @@ class ResourceBaseTestCase(base.TestCase): def setUp(self): super(ResourceBaseTestCase, self).setUp() self.conn = mock.Mock() - self.base_resource = BaseResouce(connector=self.conn, path='/Foo', - redfish_version='1.0.2') + self.base_resource = BaseResource(connector=self.conn, path='/Foo', + redfish_version='1.0.2') # refresh() is called in the constructor self.conn.reset_mock() @@ -40,23 +40,8 @@ class ResourceBaseTestCase(base.TestCase): self.base_resource.refresh() self.conn.get.assert_called_once_with(path='/Foo') - def test_refresh_http_error_reraised(self): - self.conn.get.side_effect = exceptions.HTTPError( - method='GET', url='http://foo.bar:8000/redfish/v1', error='boom', - status_code=404) - self.assertRaises(exceptions.ResourceNotFoundError, - self.base_resource.refresh) - self.conn.get.assert_called_once_with(path='/Foo') - def test_refresh_resource_not_found(self): - self.conn.get.side_effect = exceptions.HTTPError( - method='GET', url='http://foo.bar:8000/redfish/v1', error='boom', - status_code=400) - self.assertRaises(exceptions.HTTPError, self.base_resource.refresh) - self.conn.get.assert_called_once_with(path='/Foo') - - -class TestResouce(resource_base.ResourceBase): +class TestResource(resource_base.ResourceBase): """A concrete Test Resource to test against""" def __init__(self, connector, identity, redfish_version=None): @@ -67,20 +52,20 @@ class TestResouce(resource_base.ResourceBase): :param redfish_version: The version of RedFish. Used to construct the object according to schema of the given version. """ - super(TestResouce, self).__init__(connector, 'Fakes/%s' % identity, - redfish_version) + super(TestResource, self).__init__(connector, 'Fakes/%s' % identity, + redfish_version) self.identity = identity def _parse_attributes(self): pass -class TestResouceCollection(resource_base.ResourceCollectionBase): +class TestResourceCollection(resource_base.ResourceCollectionBase): """A concrete Test Resource Collection to test against""" @property def _resource_type(self): - return TestResouce + return TestResource def __init__(self, connector, redfish_version=None): """Ctor of TestResourceCollection @@ -89,8 +74,8 @@ class TestResouceCollection(resource_base.ResourceCollectionBase): :param redfish_version: The version of RedFish. Used to construct the object according to schema of the given version. """ - super(TestResouceCollection, self).__init__(connector, 'Fakes', - redfish_version) + super(TestResourceCollection, self).__init__(connector, 'Fakes', + redfish_version) class ResourceCollectionBaseTestCase(base.TestCase): @@ -98,7 +83,7 @@ class ResourceCollectionBaseTestCase(base.TestCase): def setUp(self): super(ResourceCollectionBaseTestCase, self).setUp() self.conn = mock.MagicMock() - self.test_resource_collection = TestResouceCollection( + self.test_resource_collection = TestResourceCollection( self.conn, redfish_version='1.0.x') self.conn.reset_mock() @@ -109,7 +94,7 @@ class ResourceCollectionBaseTestCase(base.TestCase): # | WHEN | result = self.test_resource_collection.get_member('1') # | THEN | - self.assertTrue(isinstance(result, TestResouce)) + self.assertTrue(isinstance(result, TestResource)) self.assertEqual('1', result.identity) self.assertEqual('1.0.x', result.redfish_version) @@ -117,9 +102,9 @@ class ResourceCollectionBaseTestCase(base.TestCase): # | GIVEN | # setting a valid member identity self.test_resource_collection.members_identities = ('1',) - self.conn.get.side_effect = exceptions.HTTPError( + self.conn.get.side_effect = exceptions.ResourceNotFoundError( method='GET', url='http://foo.bar:8000/redfish/v1/Fakes/2', - error='boom', status_code=404) + response=mock.Mock(status_code=404)) # | WHEN & THEN | self.assertRaises(exceptions.ResourceNotFoundError, self.test_resource_collection.get_member, '2') @@ -135,6 +120,6 @@ class ResourceCollectionBaseTestCase(base.TestCase): # | THEN | self.assertTrue(isinstance(result, list)) for val in result: - self.assertTrue(isinstance(val, TestResouce)) + self.assertTrue(isinstance(val, TestResource)) self.assertTrue(val.identity in member_ids) self.assertEqual('1.0.x', val.redfish_version) diff --git a/sushy/tests/unit/test_connector.py b/sushy/tests/unit/test_connector.py index a91409dd..2e101e17 100644 --- a/sushy/tests/unit/test_connector.py +++ b/sushy/tests/unit/test_connector.py @@ -13,6 +13,9 @@ # License for the specific language governing permissions and limitations # under the License. +import json + +import fixtures import mock import requests @@ -21,10 +24,10 @@ from sushy import exceptions from sushy.tests.unit import base -class ConnectorTestCase(base.TestCase): +class ConnectorMethodsTestCase(base.TestCase): def setUp(self): - super(ConnectorTestCase, self).setUp() + super(ConnectorMethodsTestCase, self).setUp() self.conn = connector.Connector( 'http://foo.bar:1234', username='user', password='pass', verify=True) @@ -49,43 +52,98 @@ class ConnectorTestCase(base.TestCase): mock__op.assert_called_once_with(mock.ANY, 'PATCH', 'fake/path', self.data, self.headers) - @mock.patch('sushy.connector.requests.Session', autospec=True) - def _test_op(self, headers, expected_headers, mock_session): - fake_session = mock.Mock() - mock_session.return_value.__enter__.return_value = fake_session + +class ConnectorOpTestCase(base.TestCase): + + def setUp(self): + super(ConnectorOpTestCase, self).setUp() + self.conn = connector.Connector( + 'http://foo.bar:1234', username='user', + password='pass', verify=True) + self.data = {'fake': 'data'} + self.headers = {'X-Fake': 'header'} + self.session_fixture = self.useFixture( + fixtures.MockPatchObject(connector.requests, 'Session', + autospec=True) + ) + self.session = ( + self.session_fixture.mock.return_value.__enter__.return_value) + self.request = self.session.request + + def _test_op(self, headers, expected_headers): + self.request.return_value.status_code = 200 self.conn._op('GET', path='fake/path', data=self.data, headers=headers) - mock_session.assert_called_once_with() - fake_session.request.assert_called_once_with( + self.request.assert_called_once_with( 'GET', 'http://foo.bar:1234/fake/path', data='{"fake": "data"}') - self.assertEqual(expected_headers, fake_session.headers) + self.assertEqual(expected_headers, self.session.headers) - def test__op(self): + def test_ok_with_headers(self): expected_headers = self.headers.copy() expected_headers['Content-Type'] = 'application/json' self._test_op(self.headers, expected_headers) - def test__op_no_headers(self): + def test_ok_no_headers(self): expected_headers = {'Content-Type': 'application/json'} self._test_op(None, expected_headers) - @mock.patch('sushy.connector.requests.Session', autospec=True) - def test__op_connection_error(self, mock_session): - fake_session = mock.Mock() - mock_session.return_value.__enter__.return_value = fake_session - fake_session.request.side_effect = requests.exceptions.ConnectionError - + def test_connection_error(self): + self.request.side_effect = requests.exceptions.ConnectionError self.assertRaises(exceptions.ConnectionError, self.conn._op, 'GET') - @mock.patch('sushy.connector.requests.Session', autospec=True) - def test__op_http_error(self, mock_session): - fake_session = mock.Mock() - mock_session.return_value.__enter__.return_value = fake_session - fake_response = fake_session.request.return_value - fake_response.status_code = 400 - fake_response.raise_for_status.side_effect = ( - requests.exceptions.HTTPError(response=fake_response)) + def test_unknown_http_error(self): + self.request.return_value.status_code = 409 + self.request.return_value.json.side_effect = ValueError('no json') - self.assertRaises(exceptions.HTTPError, self.conn._op, 'GET') + with self.assertRaisesRegex(exceptions.HTTPError, + 'unknown error') as cm: + self.conn._op('GET', 'http://foo.bar') + exc = cm.exception + self.assertEqual(409, exc.status_code) + self.assertIsNone(exc.body) + self.assertIsNone(exc.detail) + + def test_known_http_error(self): + self.request.return_value.status_code = 400 + with open('sushy/tests/unit/json_samples/error.json', 'r') as f: + self.request.return_value.json.return_value = json.load(f) + + with self.assertRaisesRegex(exceptions.BadRequestError, + 'A general error has occurred') as cm: + self.conn._op('GET', 'http://foo.bar') + exc = cm.exception + self.assertEqual(400, exc.status_code) + self.assertIsNotNone(exc.body) + self.assertIn('A general error has occurred', exc.detail) + + def test_not_found_error(self): + self.request.return_value.status_code = 404 + self.request.return_value.json.side_effect = ValueError('no json') + + with self.assertRaisesRegex(exceptions.ResourceNotFoundError, + 'Resource http://foo.bar not found') as cm: + self.conn._op('GET', 'http://foo.bar') + exc = cm.exception + self.assertEqual(404, exc.status_code) + + def test_server_error(self): + self.request.return_value.status_code = 500 + self.request.return_value.json.side_effect = ValueError('no json') + + with self.assertRaisesRegex(exceptions.ServerSideError, + 'unknown error') as cm: + self.conn._op('GET', 'http://foo.bar') + exc = cm.exception + self.assertEqual(500, exc.status_code) + + def test_access_error(self): + self.request.return_value.status_code = 403 + self.request.return_value.json.side_effect = ValueError('no json') + + with self.assertRaisesRegex(exceptions.AccessError, + 'unknown error') as cm: + self.conn._op('GET', 'http://foo.bar') + exc = cm.exception + self.assertEqual(403, exc.status_code) diff --git a/test-requirements.txt b/test-requirements.txt index 13b54a8c..3383c297 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -12,6 +12,7 @@ oslotest>=1.10.0 # Apache-2.0 testrepository>=0.0.18 # Apache-2.0/BSD testscenarios>=0.4 # Apache-2.0/BSD testtools>=1.4.0 # MIT +fixtures>=3.0.0 # Apache-2.0/BSD # releasenotes reno>=1.8.0 # Apache-2.0