From 1f421e7928f5295a21e87f43a94faded5a72bc13 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Tue, 2 Apr 2013 16:32:17 +0200 Subject: [PATCH] [FIX] redefinition and in-place extension scenarios for controllers bzr revid: xmo@openerp.com-20130402143217-pfe2288iodw9r81g --- addons/web/http.py | 54 ++++++++-- addons/web/tests/test_dispatch.py | 165 +++++++++++++++++++++++++++--- 2 files changed, 197 insertions(+), 22 deletions(-) diff --git a/addons/web/http.py b/addons/web/http.py index e05d74e0873..f08d7c64c4d 100644 --- a/addons/web/http.py +++ b/addons/web/http.py @@ -354,18 +354,58 @@ def httprequest(f): #---------------------------------------------------------- addons_module = {} addons_manifest = {} -controllers_class = [] -controllers_object = {} +controllers_class = {} controllers_path = {} class ControllerType(type): def __init__(cls, name, bases, attrs): super(ControllerType, cls).__init__(name, bases, attrs) - controllers_class.append(("%s.%s" % (cls.__module__, cls.__name__), cls)) + # Only for root "Controller" + if bases == (object,): + assert name == 'Controller' + return + + path = attrs.get('_cp_path') + if Controller in bases: + assert path, "Controller subclass %s missing a _cp_path" % cls + else: + parent_paths = set(base._cp_path for base in bases + if issubclass(base, Controller)) + assert len(parent_paths) == 1,\ + "%s inheriting from multiple controllers is not supported" % ( + name) + [parent_path] = parent_paths + [parent] = [ + controller for controller in controllers_class.itervalues() + if controller._cp_path == parent_path] + + # inherit from a Controller subclass + if path: + _logger.warn("Re-exposing %s at %s.\n" + "\tThis usage is unsupported.", + parent.__name__, + attrs['_cp_path']) + + if path: + assert path not in controllers_class,\ + "Trying to expose %s at the same URL as %s" % ( + cls, controllers_class[path]) + controllers_class[path] = cls + class Controller(object): __metaclass__ = ControllerType + def __new__(cls, *args, **kwargs): + subclasses = [c for c in cls.__subclasses__() + if c._cp_path is cls._cp_path] + if subclasses: + name = "%s (+%s)" % ( + cls.__name__, + '+'.join(sub.__name__ for sub in subclasses)) + cls = type(name, tuple(reversed(subclasses)), {}) + return object.__new__(cls) + #---------------------------------------------------------- # Session context manager #---------------------------------------------------------- @@ -558,12 +598,8 @@ class Root(object): addons_manifest[module] = manifest self.statics['/%s/static' % module] = path_static - for k, v in controllers_class: - if k not in controllers_object: - o = v() - controllers_object[k] = o - if hasattr(o, '_cp_path'): - controllers_path[o._cp_path] = o + for c in controllers_class.itervalues(): + controllers_path[c._cp_path] = c() app = werkzeug.wsgi.SharedDataMiddleware(self.dispatch, self.statics) self.dispatch = DisableCacheMiddleware(app) diff --git a/addons/web/tests/test_dispatch.py b/addons/web/tests/test_dispatch.py index 54b987cf555..516345bfa13 100644 --- a/addons/web/tests/test_dispatch.py +++ b/addons/web/tests/test_dispatch.py @@ -1,5 +1,9 @@ # -*- coding: utf-8 -*- +import contextlib import json +import logging +import logging.handlers +import types import unittest2 from .. import http @@ -20,16 +24,13 @@ class DispatchCleanup(unittest2.TestCase): """ def setUp(self): self.classes = http.controllers_class - self.objects = http.controllers_object self.paths = http.controllers_path - http.controllers_class = [] - http.controllers_object = {} + http.controllers_class = {} http.controllers_path = {} def tearDown(self): http.controllers_path = self.paths - http.controllers_object = self.objects http.controllers_class = self.classes @@ -56,6 +57,30 @@ def jsonrpc_response(result=None): } +class TestHandler(logging.handlers.BufferingHandler): + def __init__(self): + logging.handlers.BufferingHandler.__init__(self, 0) + + def shouldFlush(self, record): + return False + +@contextlib.contextmanager +def capture_logging(level=logging.DEBUG): + logger = logging.getLogger('openerp') + old_level = logger.level + old_handlers = logger.handlers + + test_handler = TestHandler() + logger.handlers = [test_handler] + logger.setLevel(level) + + try: + yield test_handler + finally: + logger.setLevel(old_level) + logger.handlers = old_handlers + + class TestDispatching(DispatchCleanup): def setUp(self): super(TestDispatching, self).setUp() @@ -104,6 +129,13 @@ class TestDispatching(DispatchCleanup): jsonrpc_response('no place paws in path of da sinnerz,'), json.loads(''.join(body))) + +class TestSubclassing(DispatchCleanup): + def setUp(self): + super(TestSubclassing, self).setUp() + self.app = http.Root() + self.client = werkzeug.test.Client(self.app) + def test_add_method(self): class CatController(http.Controller): _cp_path = '/cat' @@ -180,7 +212,38 @@ class TestDispatching(DispatchCleanup): body, status, headers = self.client.post('/cat') self.assertEqual('404 NOT FOUND', status) - def test_extend(self): + def test_extends(self): + """ + When subclassing an existing Controller new classes are "merged" into + the base one + """ + class A(http.Controller): + _cp_path = '/foo' + @http.httprequest + def index(self, req): + return '1' + + class B(A): + @http.httprequest + def index(self, req): + return "%s 2" % super(B, self).index(req) + + class C(A): + @http.httprequest + def index(self, req): + return "%s 3" % super(C, self).index(req) + + self.app.load_addons() + + body, status, headers = self.client.get('/foo') + self.assertEqual('200 OK', status) + self.assertEqual('1 2 3', ''.join(body)) + + def test_re_expose(self): + """ + An existing Controller should not be extended with a new cp_path + (re-exposing somewhere else) + """ class CatController(http.Controller): _cp_path = '/cat' @@ -191,18 +254,94 @@ class TestDispatching(DispatchCleanup): def speak(self): return 'Yu ordered cheezburgerz,' - class DogController(CatController): - _cp_path = '/dog' + with capture_logging() as handler: + class DogController(CatController): + _cp_path = '/dog' - def speak(self): - return 'Woof woof woof woof' + def speak(self): + return 'Woof woof woof woof' + + [record] = handler.buffer + self.assertEqual(logging.WARN, record.levelno) + self.assertEqual("Re-exposing CatController at /dog.\n" + "\tThis usage is unsupported.", + record.getMessage()) + + def test_fail_redefine(self): + """ + An existing Controller can't be overwritten by a new one on the same + path (? or should this generate a warning and still work as if it was + an extend?) + """ + class FooController(http.Controller): + _cp_path = '/foo' + + with self.assertRaises(AssertionError): + class BarController(http.Controller): + _cp_path = '/foo' + + def test_fail_no_path(self): + """ + A Controller must have a path (and thus be exposed) + """ + with self.assertRaises(AssertionError): + class FooController(http.Controller): + pass + + def test_mixin(self): + """ + Can mix "normal" python classes into a controller directly + """ + class Mixin(object): + @http.httprequest + def index(self, req): + return 'ok' + + class FooController(http.Controller, Mixin): + _cp_path = '/foo' + + class BarContoller(Mixin, http.Controller): + _cp_path = '/bar' self.app.load_addons() - body, status, headers = self.client.get('/cat') + body, status, headers = self.client.get('/foo') self.assertEqual('200 OK', status) - self.assertEqual('[Yu ordered cheezburgerz,]', ''.join(body)) + self.assertEqual('ok', ''.join(body)) - body, status, headers = self.client.get('/dog') + body, status, headers = self.client.get('/bar') self.assertEqual('200 OK', status) - self.assertEqual('[Woof woof woof woof]', ''.join(body)) + self.assertEqual('ok', ''.join(body)) + + def test_mixin_extend(self): + """ + Can mix "normal" python class into a controller by extension + """ + class FooController(http.Controller): + _cp_path = '/foo' + + class M1(object): + @http.httprequest + def m1(self, req): + return 'ok 1' + + class M2(object): + @http.httprequest + def m2(self, req): + return 'ok 2' + + class AddM1(FooController, M1): + pass + + class AddM2(M2, FooController): + pass + + self.app.load_addons() + + body, status, headers = self.client.get('/foo/m1') + self.assertEqual('200 OK', status) + self.assertEqual('ok 1', ''.join(body)) + + body, status, headers = self.client.get('/foo/m2') + self.assertEqual('200 OK', status) + self.assertEqual('ok 2', ''.join(body))