From 6b2529ea4512842a52f3255b72b0f590fa42dda3 Mon Sep 17 00:00:00 2001 From: sbauza Date: Mon, 25 Nov 2013 14:49:44 +0100 Subject: [PATCH] Fix API exceptions handling for message and error code API exceptions were not returning correct errorcode. A bug will be opened for moving these exceptions into climate.api but for this commit, the idea is just to quickly fix it. Change-Id: I5563c93dac45e22dc3348f5e380521cf8537b440 --- climate/api/utils.py | 14 ++++--- climate/api/validation.py | 5 +-- climate/exceptions.py | 64 ++++++++++++++++++++----------- climate/tests/api/test_utils.py | 18 +++++++-- climate/tests/test_exceptions.py | 66 ++++++++++++++++++++++++++++++++ 5 files changed, 131 insertions(+), 36 deletions(-) create mode 100644 climate/tests/test_exceptions.py diff --git a/climate/api/utils.py b/climate/api/utils.py index 6bdbd3f2..5477a215 100644 --- a/climate/api/utils.py +++ b/climate/api/utils.py @@ -211,21 +211,23 @@ def internal_error(status_code, descr, exc=None): def bad_request(error): """Called if Climate exception occurred.""" - error_code = 400 + if not error.code: + error.code = 400 LOG.debug("Validation Error occurred: " "error_code=%s, error_message=%s, error_name=%s", - error_code, error.message, error.code) + error.code, error.message, error.code) - return render_error_message(error_code, error.message, error.code) + return render_error_message(error.code, error.message, error.code) def not_found(error): """Called if object was not found.""" - error_code = 404 + if not error.code: + error.code = 404 LOG.debug("Not Found exception occurred: " "error_code=%s, error_message=%s, error_name=%s", - error_code, error.message, error.code) + error.code, error.message, error.code) - return render_error_message(error_code, error.message, error.code) + return render_error_message(error.code, error.message, error.code) diff --git a/climate/api/validation.py b/climate/api/validation.py index 16376abc..f84bd3e7 100644 --- a/climate/api/validation.py +++ b/climate/api/validation.py @@ -49,10 +49,7 @@ def check_exists(get_function, object_id=None, **get_args): except exceptions.NotFound: obj = None if obj is None: - e = exceptions.NotFound( - get_kwargs, - template='Object with %s not found', - ) + e = exceptions.NotFound(object=get_kwargs) return api_utils.not_found(e) return func(*args, **kwargs) diff --git a/climate/exceptions.py b/climate/exceptions.py index eb2546d0..0e94a845 100644 --- a/climate/exceptions.py +++ b/climate/exceptions.py @@ -1,4 +1,5 @@ # Copyright (c) 2013 Mirantis Inc. +# Copyright (c) 2013 Bull. # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -13,38 +14,57 @@ # See the License for the specific language governing permissions and # limitations under the License. +from climate.openstack.common.gettextutils import _ # noqa +from climate.openstack.common import log as logging + + +LOG = logging.getLogger(__name__) + class ClimateException(Exception): - """Base Exception for the Climate. + """Base Climate Exception. To correctly use this class, inherit from it and define - a 'message' and 'code' properties. + a 'msg_fmt' and 'code' properties. """ - template = "An unknown exception occurred" - code = "UNKNOWN_EXCEPTION" + msg_fmt = _("An unknown exception occurred") + code = 500 - def __init__(self, *args, **kwargs): - super(ClimateException, self).__init__(*args) - template = kwargs.pop('template', None) - if template: - self.template = template + def __init__(self, message=None, **kwargs): + self.kwargs = kwargs - def __str__(self): - return self.template % self.args + if 'code' not in self.kwargs: + self.kwargs['code'] = self.code - def __repr__(self): - if self.template != type(self).template: - tmpl = ", template=%r" % (self.template,) - else: - tmpl = "" - args = ", ".join(map(repr, self.args)) - return "%s(%s%s)" % (type(self).__name__, args, tmpl) + if not message: + try: + message = self.msg_fmt % kwargs + except KeyError: + # kwargs doesn't match a variable in the message + # log the issue and the kwargs + LOG.exception(_('Exception in string format operation')) + for name, value in kwargs.iteritems(): + LOG.error("%s: %s" % (name, value)) + + message = self.msg_fmt + + super(ClimateException, self).__init__(message) class NotFound(ClimateException): """Object not found exception.""" - template = "Object not found" - code = "NOT_FOUND" + msg_fmt = _("Object with %(object)s not found") + code = 404 - def __init__(self, *args, **kwargs): - super(NotFound, self).__init__(*args, **kwargs) + +class NotAuthorized(ClimateException): + msg_fmt = _("Not authorized") + code = 403 + + +class PolicyNotAuthorized(NotAuthorized): + msg_fmt = _("Policy doesn't allow %(action)s to be performed") + + +class ConfigNotFound(ClimateException): + msg_fmt = _("Could not find config at %(path)s") diff --git a/climate/tests/api/test_utils.py b/climate/tests/api/test_utils.py index ad50e7e7..04399016 100644 --- a/climate/tests/api/test_utils.py +++ b/climate/tests/api/test_utils.py @@ -122,11 +122,21 @@ class UtilsTestCase(tests.TestCase): def test_bad_request(self): error_message = self.patch(self.utils, 'render_error_message') self.utils.bad_request(self.error) - error_message.assert_called_once_with( - 400, 'message', 'code') + error_message.assert_called_once_with('code', 'message', 'code') + + def test_bad_request_with_default_errorcode(self): + error_message = self.patch(self.utils, 'render_error_message') + error = Error("message") + self.utils.bad_request(error) + error_message.assert_called_once_with(400, 'message', 400) def test_not_found(self): error_message = self.patch(self.utils, 'render_error_message') self.utils.not_found(self.error) - error_message.assert_called_once_with( - 404, 'message', 'code') + error_message.assert_called_once_with('code', 'message', 'code') + + def test_not_found_with_default_errorcode(self): + error_message = self.patch(self.utils, 'render_error_message') + error = Error("message") + self.utils.not_found(error) + error_message.assert_called_once_with(404, 'message', 404) diff --git a/climate/tests/test_exceptions.py b/climate/tests/test_exceptions.py new file mode 100644 index 00000000..2ac8640f --- /dev/null +++ b/climate/tests/test_exceptions.py @@ -0,0 +1,66 @@ +# Copyright (c) 2013 Bull. +# +# 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. + +from climate import exceptions +from climate import tests + + +class ClimateExceptionTestCase(tests.TestCase): + def test_default_error_msg(self): + class FakeClimateException(exceptions.ClimateException): + msg_fmt = "default message" + + exc = FakeClimateException() + self.assertEqual(unicode(exc), 'default message') + + def test_error_msg(self): + self.assertEqual(unicode(exceptions.ClimateException('test')), + 'test') + + def test_default_error_msg_with_kwargs(self): + class FakeClimateException(exceptions.ClimateException): + msg_fmt = "default message: %(code)s" + + exc = FakeClimateException(code=500) + self.assertEqual(unicode(exc), 'default message: 500') + self.assertEqual(exc.message, 'default message: 500') + + def test_error_msg_exception_with_kwargs(self): + class FakeClimateException(exceptions.ClimateException): + msg_fmt = "default message: %(mispelled_code)s" + + exc = FakeClimateException(code=500, mispelled_code='blah') + self.assertEqual(unicode(exc), 'default message: blah') + self.assertEqual(exc.message, 'default message: blah') + + def test_default_error_code(self): + class FakeClimateException(exceptions.ClimateException): + code = 404 + + exc = FakeClimateException() + self.assertEqual(exc.kwargs['code'], 404) + + def test_error_code_from_kwarg(self): + class FakeClimateException(exceptions.ClimateException): + code = 500 + + exc = FakeClimateException(code=404) + self.assertEqual(exc.kwargs['code'], 404) + + def test_policynotauthorized_exception(self): + exc = exceptions.PolicyNotAuthorized(action='foo') + self.assertEqual(unicode(exc.message), + "Policy doesn't allow foo to be performed") + self.assertEqual(exc.kwargs['code'], 403)