Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fixes bug 846347 - Allowed implementation overriding in configman middle... #1106

Merged
merged 1 commit into from

2 participants

@AdrianGaudebert

...ware app.

@peterbe What do you think?

socorro/middleware/middleware_app.py
((12 lines not shown))
+ impl_code = params['force_api_impl']
+
+ file_name, class_name = self.file_and_class.rsplit('.', 1)
+ implementations = dict(
+ self.context.implementations.implementation_list
+ )
+ base_module_path = implementations[impl_code]
+ try:
+ module = __import__(
+ '%s.%s' % (base_module_path, file_name),
+ globals(),
+ locals(),
+ [class_name]
+ )
+ except ImportError:
+ raise ImportError(
@peterbe Owner
peterbe added a note

Is this really necessary? How would the traceback look like "naturally"?

In fact, perhaps we should do something like raise RuntimeError('%s.%s not importable' % (base_module_path, file_name)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
socorro/middleware/middleware_app.py
@@ -305,16 +315,43 @@ def wrap(cls):
class ImplementationWrapper(JsonWebServiceBase):
def GET(self, *args, **kwargs):
+ # prepare parameters
+ params = kwargs
+ if len(args) > 0:
+ params.update(self.parse_url_path(args[-1]))
+ self._correct_signature_parameters(params)
+
+ # override implementation class if needed
+ if 'force_api_impl' in params:
@peterbe Owner
peterbe added a note

Change this to _force_api_impl to indicate that it's something beyond the normal.

@peterbe Owner
peterbe added a note

Also, change it to if params.get('_force_api_impl'): otherwise you'll allow people to enter /crash/uuid/xxxx/_force_api_impl//

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@peterbe
Owner

I love it! Well done!

However, we need to think about the except ImportError case.
Either we just let it be the first ImportError (assuming it's not too cryptic) or we need at least a unit test that causes an ImportError and with that, something like self.assertRaises(ImportError, self.get, server ...)

@AdrianGaudebert

@peterbe I changed the ImportError to a BadRequest, does that sound right to you? It's indeed a bad request as the user manually entered a URL that is not valid.

@peterbe peterbe merged commit 504ac2b into from
@AdrianGaudebert AdrianGaudebert deleted the branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
68 socorro/middleware/middleware_app.py
@@ -28,7 +28,7 @@
# Here's the list of URIs mapping to classes and the files they belong to.
# The final lookup depends on the `implementation_list` option inside the app.
SERVICES_LIST = (
- (r'/bugs/', 'bugs.Bugs'),
+ (r'/bugs/(.*)', 'bugs.Bugs'),
(r'/crash_data/(.*)', 'crash_data.CrashData'),
(r'/crash/(.*)', 'crash.Crash'),
(r'/crashes/(comments|daily|frequency|paireduuid|signatures)/(.*)',
@@ -257,7 +257,7 @@ class MiddlewareApp(App):
#--------------------------------------------------------------------------
def main(self):
- # Apache modwsgi requireds a module level name 'applicaiton'
+ # Apache modwsgi requireds a module level name 'application'
global application
## 1 turn these names of classes into real references to classes
@@ -284,12 +284,22 @@ def lookup(file_and_class):
return getattr(module, class_name)
raise ImplementationConfigurationError(file_and_class)
- services_list = ((x, lookup(y)) for x, y in SERVICES_LIST)
-
## 2 wrap each class with the ImplementationWrapper class
- def wrap(cls):
- return type(cls.__name__, (ImplementationWrapper,), {'cls': cls})
- services_list = ((x, wrap(y)) for x, y in services_list)
+ def wrap(cls, file_and_class):
+ return type(
+ cls.__name__,
+ (ImplementationWrapper,),
+ {
+ 'cls': cls,
+ 'file_and_class': file_and_class
+ }
+ )
+
+ services_list = []
+ for url, impl_class in SERVICES_LIST:
+ impl_instance = lookup(impl_class)
+ wrapped_impl = wrap(impl_instance, impl_class)
+ services_list.append((url, wrapped_impl))
self.web_server = self.config.web_server.wsgi_server_class(
self.config, # needs the whole config not the local namespace
@@ -305,16 +315,50 @@ def wrap(cls):
class ImplementationWrapper(JsonWebServiceBase):
def GET(self, *args, **kwargs):
+ # prepare parameters
+ params = kwargs
+ if len(args) > 0:
+ params.update(self.parse_url_path(args[-1]))
+ self._correct_signature_parameters(params)
+
+ # override implementation class if needed
+ if params.get('_force_api_impl'):
+ impl_code = params['_force_api_impl']
+
+ file_name, class_name = self.file_and_class.rsplit('.', 1)
+ implementations = dict(
+ self.context.implementations.implementation_list
+ )
+
+ try:
+ base_module_path = implementations[impl_code]
+ except KeyError:
+ raise BadRequest(
+ 'Implementation code "%s" does not exist' % impl_code
+ )
+
+ try:
+ module = __import__(
+ '%s.%s' % (base_module_path, file_name),
+ globals(),
+ locals(),
+ [class_name]
+ )
+ except ImportError:
+ raise BadRequest(
+ "Unable to import %s.%s.%s (implementation code is %s)" %
+ (base_module_path, file_name, class_name, impl_code)
+ )
+ instance = getattr(module, class_name)(config=self.context)
+ else:
+ instance = self.cls(config=self.context)
+
+ # find the method to call
default_method = kwargs.pop('default_method', 'get')
assert default_method in ('get', 'post', 'put'), default_method
method_name = default_method
if len(args) > 1:
method_name = args[0]
- params = kwargs
- if len(args) > 0:
- params.update(self.parse_url_path(args[-1]))
- self._correct_signature_parameters(params)
- instance = self.cls(config=self.context)
try:
method = getattr(instance, method_name)
except AttributeError:
View
4 socorro/middleware/service.py
@@ -49,9 +49,9 @@ def get_module(self, params):
Return the imported module otherwise.
"""
- if "force_api_impl" in params:
+ if params.get("_force_api_impl"):
module_name = ".".join(("socorro.external",
- params["force_api_impl"],
+ params["_force_api_impl"],
self.service_name))
try:
__import__(module_name)
View
37 socorro/unittest/middleware/test_middleware_app.py
@@ -6,7 +6,7 @@
import unittest
import logging
import urllib
-from paste.fixture import TestApp
+from paste.fixture import TestApp, AppError
import mock
from nose.plugins.attrib import attr
from mock import patch
@@ -369,6 +369,41 @@ def test_overriding_implementation_class(self):
response = self.get(server, '/crash/uuid/' + self.uuid)
self.assertEqual(response.data, ['all', 'your', 'base'])
+ def test_overriding_implementation_class_at_runtime(self):
+ default = (middleware_app.MiddlewareApp.required_config
+ .implementations.implementation_list.default)
+ previous_as_str = ', '.join('%s: %s' % (x, y) for (x, y) in default)
+
+ config_manager = self._setup_config_manager({
+ 'implementations.service_overrides': 'CrashData: fs',
+ 'implementations.implementation_list': (
+ previous_as_str + ', testy: socorro.unittest.middleware'
+ )
+ })
+
+ with config_manager.context() as config:
+ app = middleware_app.MiddlewareApp(config)
+ app.main()
+ server = middleware_app.application
+
+ # normal call
+ url = '/crash/uuid/%s/'
+ response = self.get(server, url % self.uuid)
+ self.assertEqual(response.data, {'hits': [], 'total': 0})
+
+ # forcing implementation at runtime
+ url = '/crash/uuid/%s/_force_api_impl/testy/'
+ response = self.get(server, url % self.uuid)
+ self.assertEqual(response.data, ['all', 'your', 'base'])
+
+ # forcing unexisting implementation at runtime
+ url = '/crash/uuid/%s/_force_api_impl/TYPO/'
+ self.assertRaises(
+ AppError,
+ self.get,
+ server, url % self.uuid
+ )
+
def test_crash(self):
config_manager = self._setup_config_manager()
View
2  socorro/unittest/middleware/test_service.py
@@ -70,7 +70,7 @@ def test_get_module(self):
# Test forced module
import_failed = False
- params["force_api_impl"] = "elasticsearch"
+ params["_force_api_impl"] = "elasticsearch"
try:
mod = service.get_module(params)
except NotImplementedError:
Something went wrong with that request. Please try again.