Revert "Return a descriptive error message for controllers"
This reverts commit 6512c71a49.
While this fixes the problem it doesn't do it in a comprehensive way.
The right way would be to implement real validation.
The reverted patch breaks on methods marked as deprecated because the
deprecated decorator is a function that accepts *args and **kwargs.
This bypasses the check and we go back to the same TypeError from the
original bug.
Related-Bug: #1149987
Change-Id: I0efbcce9abc23bf79eb970e441e5f7644b1c8453
This commit is contained in:
David Stanek
parent
f72f369538
commit
be069f646f
@ -20,7 +20,6 @@
|
||||
|
||||
"""Utility methods for working with WSGI servers."""
|
||||
|
||||
import inspect
|
||||
import re
|
||||
|
||||
import routes.middleware
|
||||
@ -233,7 +232,6 @@ class Application(BaseApplication):
|
||||
params = self._normalize_dict(params)
|
||||
|
||||
try:
|
||||
self._method_inspect(method, params)
|
||||
result = method(context, **params)
|
||||
except exception.Unauthorized as e:
|
||||
LOG.warning(
|
||||
@ -243,9 +241,10 @@ class Application(BaseApplication):
|
||||
except exception.Error as e:
|
||||
LOG.warning(e)
|
||||
return render_exception(e, user_locale=req.best_match_language())
|
||||
except exception.ControllerArgsError as e:
|
||||
except TypeError as e:
|
||||
LOG.exception(e)
|
||||
return render_exception(e, user_locale=req.best_match_language())
|
||||
return render_exception(exception.ValidationError(e),
|
||||
user_locale=req.best_match_language())
|
||||
except Exception as e:
|
||||
LOG.exception(e)
|
||||
return render_exception(exception.UnexpectedError(exception=e),
|
||||
@ -263,39 +262,6 @@ class Application(BaseApplication):
|
||||
response_code = self._get_response_code(req)
|
||||
return render_response(body=result, status=response_code)
|
||||
|
||||
def _method_inspect(self, method, params):
|
||||
"""Validates the number of params passed to controller methods.
|
||||
|
||||
A `ControllerArgsError` will be raised if:
|
||||
1. Required arguments are missing
|
||||
2. Unexpected keyword args are provided to a method not
|
||||
expecting **kwargs
|
||||
"""
|
||||
params = set(params)
|
||||
|
||||
spec = inspect.getargspec(method)
|
||||
if spec.defaults:
|
||||
required_args = set(spec.args[:-len(spec.defaults)])
|
||||
else:
|
||||
required_args = set(spec.args)
|
||||
|
||||
required_args.discard('self') # provided by Python
|
||||
required_args.discard('context') # provided by caller
|
||||
|
||||
err_args = {}
|
||||
|
||||
missing_required_args = required_args - params
|
||||
if missing_required_args:
|
||||
err_args['missing_required_args'] = missing_required_args
|
||||
|
||||
if not spec.keywords:
|
||||
extra_params = params - set(spec.args)
|
||||
if extra_params:
|
||||
err_args['extra_params'] = extra_params
|
||||
|
||||
if err_args:
|
||||
raise exception.ControllerArgsError(**err_args)
|
||||
|
||||
def _get_response_code(self, req):
|
||||
req_method = req.environ['REQUEST_METHOD']
|
||||
controller = importutils.import_class('keystone.common.controller')
|
||||
|
||||
@ -78,24 +78,6 @@ class ValidationTimeStampError(Error):
|
||||
title = 'Bad Request'
|
||||
|
||||
|
||||
class ControllerArgsError(ValidationError):
|
||||
"""Raised when controller methods receive incorrect argumemnts.
|
||||
|
||||
"""
|
||||
|
||||
def _build_message(self, message, missing_required_args=None,
|
||||
extra_params=None):
|
||||
errors = []
|
||||
|
||||
for param in sorted(extra_params or []):
|
||||
errors.append(_('%s is not allowed.') % param)
|
||||
|
||||
for arg in sorted(missing_required_args or []):
|
||||
errors.append(_('%s is required.') % arg)
|
||||
|
||||
return u' '.join(str(e) for e in errors)
|
||||
|
||||
|
||||
class StringLengthExceeded(ValidationError):
|
||||
message_format = _("String length exceeded.The length of"
|
||||
" string '%(string)s' exceeded the limit"
|
||||
|
||||
@ -18,8 +18,6 @@ import uuid
|
||||
|
||||
from babel import localedata
|
||||
import gettext
|
||||
from testtools import matchers
|
||||
|
||||
|
||||
from keystone.common import wsgi
|
||||
from keystone import exception
|
||||
@ -316,168 +314,3 @@ class LocalizedResponseTest(tests.TestCase):
|
||||
# are lazy-translated.
|
||||
self.assertIsInstance(_('The resource could not be found.'),
|
||||
gettextutils.Message)
|
||||
|
||||
|
||||
class TestControllerMethodInspection(tests.TestCase):
|
||||
|
||||
def setUp(self):
|
||||
super(TestControllerMethodInspection, self).setUp()
|
||||
self.app = wsgi.Application()
|
||||
self.inspect = self.app._method_inspect
|
||||
|
||||
def assertSuccess(self, func_user_test, params):
|
||||
self.inspect(func_user_test, params)
|
||||
|
||||
def assertFailure(self, func_user_test, params,
|
||||
expected_extra=None, expected_missing=None):
|
||||
# NOTE(dstanek): In Python2.7+ assertRaises can be used as a context
|
||||
# manager. This would make the code much cleaner.
|
||||
try:
|
||||
self.inspect(func_user_test, params)
|
||||
except exception.ControllerArgsError as e:
|
||||
message = str(e)
|
||||
|
||||
for param in expected_extra or []:
|
||||
expected_msg = '%s is not allowed.' % param
|
||||
self.assertThat(message, matchers.Contains(expected_msg))
|
||||
|
||||
for param in expected_missing or []:
|
||||
expected_msg = '%s is required.' % param
|
||||
self.assertThat(message, matchers.Contains(expected_msg))
|
||||
|
||||
else:
|
||||
raise self.failureException('ControllerArgsError not raised')
|
||||
|
||||
def test_all_required_parameters_provided(self):
|
||||
|
||||
def func_under_test(a):
|
||||
pass
|
||||
self.assertSuccess(func_under_test, params=['a'])
|
||||
|
||||
def func_under_test(a, b):
|
||||
pass
|
||||
self.assertSuccess(func_under_test, params=['a', 'b'])
|
||||
|
||||
def test_optional_parameters_provided(self):
|
||||
|
||||
def func_under_test(a=None):
|
||||
pass
|
||||
self.assertSuccess(func_under_test, params=['a'])
|
||||
|
||||
def test_optional_parameters_not_provided(self):
|
||||
|
||||
def func_under_test(a=None):
|
||||
pass
|
||||
self.assertSuccess(func_under_test, params=[])
|
||||
|
||||
def func_under_test(a, b=None):
|
||||
pass
|
||||
self.assertSuccess(func_under_test, params=['a'])
|
||||
|
||||
def test_some_required_parameters_missing(self):
|
||||
|
||||
def func_under_test(a, b):
|
||||
pass
|
||||
self.assertFailure(func_under_test, params=['b'],
|
||||
expected_missing=['a'])
|
||||
|
||||
def test_extra_parameter_supplied(self):
|
||||
|
||||
def func_under_test(a):
|
||||
pass
|
||||
self.assertFailure(func_under_test, params=['a', 'b'],
|
||||
expected_extra=['b'])
|
||||
|
||||
def test_extra_parameter_supplied_with_kwargs_defined(self):
|
||||
|
||||
def func_under_test(a, **kw):
|
||||
pass
|
||||
self.assertSuccess(func_under_test, params=['a', 'b'])
|
||||
|
||||
def test_some_required_parameters_missing_with_kwargs_defined(self):
|
||||
|
||||
def func_under_test(a, b, **kw):
|
||||
pass
|
||||
self.assertFailure(func_under_test, params=['a'],
|
||||
expected_missing=['b'])
|
||||
|
||||
def test_a_method_works(self):
|
||||
|
||||
class AppUnderTest(object):
|
||||
def index(self, a):
|
||||
pass
|
||||
self.assertSuccess(AppUnderTest().index, params=['a'])
|
||||
|
||||
class AppUnderTest(object):
|
||||
def index(self, a, b):
|
||||
pass
|
||||
self.assertFailure(AppUnderTest().index, params=['a'],
|
||||
expected_missing=['b'])
|
||||
|
||||
def test_auto_provided_params_are_ignored(self):
|
||||
|
||||
class AppUnderTest(object):
|
||||
def index(self, context):
|
||||
pass
|
||||
self.assertSuccess(AppUnderTest().index, params=[])
|
||||
|
||||
class AppUnderTest(object):
|
||||
def index(self, context, a):
|
||||
pass
|
||||
self.assertSuccess(AppUnderTest().index, params=['a'])
|
||||
|
||||
|
||||
class TestWSGIControllerMethodInspection(BaseWSGITest):
|
||||
|
||||
class FakeAppWithArgs(wsgi.Application):
|
||||
def index(self, context, arg0, arg1):
|
||||
return arg0, arg1
|
||||
|
||||
def _execute_test(self, app, expected_body, params=None):
|
||||
req = self._make_request()
|
||||
if params:
|
||||
req.environ['openstack.params'] = params
|
||||
resp = req.get_response(app())
|
||||
self.assertEqual(jsonutils.loads(resp.body), expected_body)
|
||||
|
||||
def test_controller_method_with_no_args(self):
|
||||
class FakeApp(wsgi.Application):
|
||||
def index(self, context):
|
||||
return ['index']
|
||||
|
||||
self._execute_test(FakeApp, ['index'])
|
||||
|
||||
def test_controller_method_with_correct_args(self):
|
||||
self._execute_test(self.FakeAppWithArgs, ['value0', 'value1'],
|
||||
{'arg0': 'value0', 'arg1': 'value1'})
|
||||
|
||||
def test_controller_method_with_missing_arg(self):
|
||||
expected_body = {
|
||||
"error": {
|
||||
"message": "arg1 is required.",
|
||||
"code": 400,
|
||||
"title": "Bad Request"
|
||||
}
|
||||
}
|
||||
self._execute_test(self.FakeAppWithArgs, expected_body,
|
||||
{'arg0': 'value0'})
|
||||
|
||||
def test_controller_method_with_multiple_errors(self):
|
||||
expected_body = {
|
||||
"error": {
|
||||
"message": "arg3 is not allowed. "
|
||||
"arg0 is required. "
|
||||
"arg1 is required.",
|
||||
"code": 400,
|
||||
"title": "Bad Request"
|
||||
}
|
||||
}
|
||||
self._execute_test(self.FakeAppWithArgs, expected_body,
|
||||
{'arg3': 'value3'})
|
||||
|
||||
def test_controller_method_with_default_args(self):
|
||||
class FakeApp(wsgi.Application):
|
||||
def index(self, context, arg0, arg1='1'):
|
||||
return arg0, arg1
|
||||
|
||||
self._execute_test(FakeApp, ['0', '1'], {'arg0': '0'})
|
||||
|
||||
Loading…
Reference in New Issue
Block a user