View selection breaks when using multiple base classes

[wichert]

rest-toolkit uses ABCs to add default behaviour to resources. For example if a resource class adds the ViewableResource abc to its base classes the default GET view from rest-toolkit will be used for that resource. Under the hood this is implemented by adding views for the ABC to the route. The user can then also add a view for the exact resource class to override the default view.

Unfortunately this system breaks if a resource uses multiple ABCs as well as a non-abc base class. This is a test case that demonstrates this:

import abc
from rest_toolkit.compat import add_metaclass  # This is a copy of six.add_metaclass
from webtest import TestApp
from pyramid.config import Configurator


@add_metaclass(abc.ABCMeta)
class Int1(object):
    pass


class OtherBase(object):
    pass


@add_metaclass(abc.ABCMeta)
class Int2(object):
    pass


class Resource(OtherBase, Int1, Int2):
    def __init__(self, request):
        pass


def unknown(context, request):
    return 'unknown method'


def view(context, request):
    return 'hello'


def test_multiple_abcs():
    config = Configurator()
    config.add_route('root', '/', factory=Resource)
    config.add_view(unknown, route_name='root', renderer='string')
    config.add_view(view, route_name='root', context=Int1, request_method='GET', renderer='string')
    config.add_view(view, route_name='root', context=Int2, request_method='POST', renderer='string')
    app = TestApp(config.make_wsgi_app())
    assert 'hello' in app.get('/')
    assert 'hello' in app.post('/')
    assert 'unknown' in app.delete('/')

The failure is:

tests/test_abc.py:40: in test_multiple_abcs
    assert 'hello' in app.post('/')
E   assert 'hello' in <200 OK text/plain body=b'unknown method'>
E    +  where <200 OK text/plain body=b'unknown method'> = <bound method TestApp.post of <webtest.app.TestApp object at 0x111c6e898>>('/')
E    +    where <bound method TestApp.post of <webtest.app.TestApp object at 0x111c6e898>> = <webtest.app.TestApp object at 0x111c6e898>.post

If you remove the OtherBase base class from Resource the test does not fail.

[wichert]

The use of ABCs turns out to be a red herring: I get the exact same error if I use normal base classes.

[wichert]

The use of ABCs turns out to be a red herring: I get the exact same error if I use normal base classes.

[wichert]

Simplest possible test case:

from webtest import TestApp
from pyramid.config import Configurator

class OtherBase(object): pass
class Int1(object): pass
class Int2(object): pass

class Resource(OtherBase, Int1, Int2):
    def __init__(self, request):
        pass

def unknown(context, request):
    return 'unknown'

def view(context, request):
    return 'hello'

def test_views_for_multiple_base_classes():
    config = Configurator()
    config.add_route('root', '/', factory=Resource)
    config.add_view(unknown, route_name='root', renderer='string')
    config.add_view(view, route_name='root', renderer='string',
            context=Int1, request_method='GET')
    config.add_view(view, route_name='root', renderer='string',
            context=Int2, request_method='POST')
    app = TestApp(config.make_wsgi_app())
    assert 'hello' in app.get('/')
    assert 'hello' in app.post('/')
    assert 'unknown' in app.delete('/')

[wichert]

Simplest possible test case:

from webtest import TestApp
from pyramid.config import Configurator

class OtherBase(object): pass
class Int1(object): pass
class Int2(object): pass

class Resource(OtherBase, Int1, Int2):
    def __init__(self, request):
        pass

def unknown(context, request):
    return 'unknown'

def view(context, request):
    return 'hello'

def test_views_for_multiple_base_classes():
    config = Configurator()
    config.add_route('root', '/', factory=Resource)
    config.add_view(unknown, route_name='root', renderer='string')
    config.add_view(view, route_name='root', renderer='string',
            context=Int1, request_method='GET')
    config.add_view(view, route_name='root', renderer='string',
            context=Int2, request_method='POST')
    app = TestApp(config.make_wsgi_app())
    assert 'hello' in app.get('/')
    assert 'hello' in app.post('/')
    assert 'unknown' in app.delete('/')

[mmerickel]

Yeah this looks like a bug. I can confirm what you're seeing on 1.5-branch and master.

[mmerickel]

Yeah this looks like a bug. I can confirm what you're seeing on 1.5-branch and master.

[wichert]

I seem to have found a workaround: instead of registering the unknown view without a request_method I can register it for all request methods separately:

for method in ['DELETE', 'GET', 'PATCH', 'POST', 'PUT']:
    config.add_view(unknown, route_name='root', request_method=method, renderer='string')

[wichert]

I seem to have found a workaround: instead of registering the unknown view without a request_method I can register it for all request methods separately:

for method in ['DELETE', 'GET', 'PATCH', 'POST', 'PUT']:
    config.add_view(unknown, route_name='root', request_method=method, renderer='string')

[wichert]

I spoke too soon: that workaround also doesn't work :(

[wichert]

I spoke too soon: that workaround also doesn't work :(

[sontek]

So I believe this is a legit problem with your code and not Pyramid (although an unfortunate one) because the traverser is going to resolve the OtherBase class first, since its the first in the inheritance chain and since that will perfectly match the unknown view it calls it.

If you move the OtherBase inheritance to the end of the chain the code will work because the traverser will match the classes that will hit the other views first.

[sontek]

So I believe this is a legit problem with your code and not Pyramid (although an unfortunate one) because the traverser is going to resolve the OtherBase class first, since its the first in the inheritance chain and since that will perfectly match the unknown view it calls it.

If you move the OtherBase inheritance to the end of the chain the code will work because the traverser will match the classes that will hit the other views first.

[wichert]

I don't think this is a problem with my code: the view logic should find the most specific view for the route. In this case it can choose between a view which only filters on route name, and a view which filters on route name, context class and request method, but it still picks the first one.

Reordering the inheritance order is not possible: the other classes are ABCs, and if you list those first their abstract methods will override the real method implementations, breaking the entire class.

[wichert]

I don't think this is a problem with my code: the view logic should find the most specific view for the route. In this case it can choose between a view which only filters on route name, and a view which filters on route name, context class and request method, but it still picks the first one.

Reordering the inheritance order is not possible: the other classes are ABCs, and if you list those first their abstract methods will override the real method implementations, breaking the entire class.

[sontek]

@wichert The chunk of code that is causing this is:
https://github.com/Pylons/pyramid/blob/master/pyramid/router.py#L164-L181

You would have to change that chunk to detect most specific rather than going in order

[sontek]

@wichert The chunk of code that is causing this is:
https://github.com/Pylons/pyramid/blob/master/pyramid/router.py#L164-L181

You would have to change that chunk to detect most specific rather than going in order

[sontek]

Pushed the fix as a PR

[sontek]

Pushed the fix as a PR

[mcdonc]

This is fixed as the result of #1557

[mcdonc]

This is fixed as the result of #1557

[wichert]

I'm very happy to hear that. Thanks!

[wichert]

I'm very happy to hear that. Thanks!