Cannot subclass Request and use add_request_method

[tiberiuichim]

See this test:

class TestPyramidRequestProperties:

    def test_it(self):
        from pyramid.request import Request
        from zope.interface import providedBy, implementedBy

        req = Request({})
        assert providedBy(req) == implementedBy(Request)

        req._set_properties({'b':'b'})

        assert providedBy(req) == implementedBy(Request)

    def test_subclassing(self):
        from pyramid.request import Request
        from zope.interface import providedBy, implementedBy

        class Subclass(Request):
            pass

        req = Subclass({})

        #calling providedBy(req) makes the test pass
        #providedBy(req)

        #assert providedBy(req) == implementedBy(Subclass)

        req._set_properties({'b':'b'})

        assert providedBy(req) == implementedBy(Subclass)

This is the output from the test:

Traceback (most recent call last):
  File "test_request.py", line 111, in test_subclassing
    assert providedBy(req) == implementedBy(Subclass)
AssertionError: assert <implementedB...uiltin__.type> == <implementedBy...uest.Subclass>
  Full diff:
  - <implementedBy __builtin__.type>
  + <implementedBy kotti.tests.test_request.Subclass>

And this is the difference from a normal request to one that has been "decorated" by _set_properties:

(Pdb) pp vars(providedBy(req))
{'__bases__': (<implementedBy __builtin__.object>,),
 '__iro__': (<InterfaceClass zope.interface.Interface>,),
 '__name__': '__builtin__.type',
 '__sro__': (<implementedBy __builtin__.type>,
             <implementedBy __builtin__.object>,
             <InterfaceClass zope.interface.Interface>),
 '_implied': {<InterfaceClass zope.interface.Interface>: (),
              <implementedBy __builtin__.object>: (),
              <implementedBy __builtin__.type>: ()},
 'dependents': <WeakKeyDictionary at 140434285597760>,
 'inherit': <type 'type'>}

(Pdb) pp vars(providedBy(Subclass({})))
{'__bases__': (<implementedBy pyramid.request.Request>,),
 '__iro__': (<InterfaceClass pyramid.interfaces.IRequest>,
             <InterfaceClass zope.interface.Interface>),
 '__name__': 'kotti.tests.test_request.Subclass',
 '__sro__': (<implementedBy kotti.tests.test_request.Subclass>,
             <implementedBy pyramid.request.Request>,
             <InterfaceClass pyramid.interfaces.IRequest>,
             <InterfaceClass zope.interface.Interface>,
             <implementedBy webob.request.BaseRequest>,
             <implementedBy pyramid.url.URLMethodsMixin>,
             <implementedBy pyramid.request.CallbackMethodsMixin>,
             <implementedBy pyramid.util.InstancePropertyMixin>,
             <implementedBy pyramid.i18n.LocalizerRequestMixin>,
             <implementedBy pyramid.security.AuthenticationAPIMixin>,
             <implementedBy pyramid.security.AuthorizationAPIMixin>,
             <implementedBy __builtin__.object>),
 '_implied': {<InterfaceClass pyramid.interfaces.IRequest>: (),
              <InterfaceClass zope.interface.Interface>: (),
              <implementedBy kotti.tests.test_request.Subclass>: (),
              <implementedBy pyramid.request.Request>: (),
              <implementedBy webob.request.BaseRequest>: (),
              <implementedBy pyramid.url.URLMethodsMixin>: (),
              <implementedBy pyramid.request.CallbackMethodsMixin>: (),
              <implementedBy pyramid.util.InstancePropertyMixin>: (),
              <implementedBy pyramid.i18n.LocalizerRequestMixin>: (),
              <implementedBy pyramid.security.AuthenticationAPIMixin>: (),
              <implementedBy pyramid.security.AuthorizationAPIMixin>: (),
              <implementedBy __builtin__.object>: ()},
 'dependents': <WeakKeyDictionary at 140434219857464>,
 'inherit': <class 'kotti.tests.test_request.Subclass'>}

In the first test, test_it, the req variable has 'proper' providedBy information, only by subclassing the problem appears.

[tiberiuichim]

There's a line in the second test, providedBy(req). If it's called before the _set_properties is called, then the test passes. Also, after the line self.__class__ = cls from pyramid.util is called, self will be missing the __provides__ attribute.

[mmerickel]

I originally marked this as a bug because I thought you weren't able to subclass request. It appears that actually works fine. I'm not sure what the concern is here. It's entirely valid for an object to have a different list of interfaces than its class object. Request._set_properties is actually changing the object's hierarchy to have a phony class in the middle containing new descriptors.

Can you clarify what your concern is?

[tiberiuichim]

@mmerickel It is possible to subclass Request, but the provided interfaces by that subclass are no longer the same after calling set_properties. This violates expected zope.interface behaviour.

If IRequest.providedBy(Request()) is True and equal to IRequest.providedBy(SubclassofRequest()) then why should this be false after triggering _set_properties on the request?

That phony class in the middle that you talk about should implement the same interfaces as the original request. Why is that important? Because if I call pyramid.view.render_view_to_response() with a broken request, then I won't get the proper response, because the view will not be found, the interfaces provided by the request are used as discriminators for the view.

[mmerickel]

Okay I see what you're saying. I did not realize that the new class did not have the IRequest etc interfaces attached. That's definitely an issue.

[digitalresistor]

Okay, I was able to whittle the test down to the following, this issue has nothing to do with _set_properties() or the like, the following test fails on the second assertTrue:

class Test_subclassing_Request(unittest.TestCase):
    def test_subclass(self):
        from pyramid.request import Request
        from zope.interface import providedBy, implementedBy

        class RequestSub(Request):
            pass

        self.assertTrue(hasattr(Request, '__provides__'))
        self.assertTrue(hasattr(RequestSub, '__provides__'))

What happens is that when you sub-class Request it does not inherit __provides__. I am not familiar enough with zope.interface to say if it should or shouldn't (and I am guessing it shouldn't), but maybe @tseaver or @mcdonc could shed some more light on that.

The way this is added by zope.interface is when you call @implementer(IRequest), here is the full test I ended up with, that does work:

class Test_subclassing_Request(unittest.TestCase):
    def test_subclass_with_implementer(self):
        from pyramid.interfaces import IRequest
        from pyramid.request import Request
        from zope.interface import providedBy, implementedBy, implementer

        @implementer(IRequest)
        class RequestSub(Request):
            pass

        self.assertTrue(hasattr(Request, '__provides__'))
        self.assertTrue(hasattr(RequestSub, '__provides__'))

        req = RequestSub({})
        req._set_properties({'b': 'b'})
        self.assertEqual(providedBy(req), implementedBy(RequestSub))

I don't know if sub-classing Request and using it as the request class without also decorating it with the implementer to state that it implements IRequest is even supported. To fix your issue, decorate your sub-class with @implementer(IRequest).

---

From what I can gather, the reason calling providedBy on req works is that apparently there is enough information to look up the required information, attaching the __provides__ attr as necessary, and then when _set_properties() creates the new class it can correctly copy that information into the new class: https://github.com/Pylons/pyramid/blob/master/pyramid/util.py#L102-108

[davisagli]

A few things:

• I think you're mixing the concepts of implements and provides a bit. Generally if a class implements an interface, that means that instances of the class provide the same interface. In this case, Request and RequestSub aren't expected to provide any interface other than Interface, because they are classes. However Request is declared to implement IRequest, so instances of Request should provide IRequest. Since RequestSub is a subclass of Request, it should inherit the implemented interfaces of Request, and instances of it should also provide them.
• __provides__ is an implementation detail and is created lazily. The canonical way to look up interfaces is with implementedBy (for types) or providedBy (for instances). Or the implementedBy and providedBy methods of a particular interface.

So I would rewrite the test like this:

class Test_subclassing_Request(unittest.TestCase):
    def test_subclass_with_implementer(self):
        from pyramid.interfaces import IRequest
        from pyramid.request import Request

        class RequestSub(Request):
            pass

        self.assertTrue(IRequest.implementedBy(Request))
        self.assertTrue(IRequest.implementedBy(RequestSub))

        request = Request({})
        request_sub = RequestSub({})
        self.assertTrue(IRequest.providedBy(request))
        self.assertTrue(IRequest.providedBy(request_sub))

which passes.

I need to take a closer look at the original issue, but have to head out the door at the moment.

[digitalresistor]

@davisagli That test passes, but is not showcasing the issue that was originally reported.

Specifically, when you create RequestSub() and don't call implementedBy() on it before mutating it with _set_properties(), and THEN call providedBy() on request_sub, it fails because during the properties mutation it can't copy __provided__.

See this minimal test case that show cases the issue:

def test_subclass_as_op(self):
    from pyramid.interfaces import IRequest
    from pyramid.request import Request
    from zope.interface import providedBy, implementedBy, implementer

    class RequestSub(Request):
        pass

    req = RequestSub({})
    req._set_properties({'b': 'b'})

    # This fails because _set_properties didn't have an __provided__ to
    # copy to the new class
    self.assertTrue(IRequest.providedBy(req))

    # This also fails, for obvious reasons
    self.assertEqual(providedBy(req), implementedBy(RequestSub))

This does not pass. If __provides__ is lazy initialized and does not need to exist on the new class as an attribute, why does providedBy() fail?

[digitalresistor]

Figured out that if I remove __providedBy__ from the list of attributes copied from the parent in _set_properties(), the following tests all pass:

class Test_subclassing_Request(unittest.TestCase):
    def test_subclass(self):
        from pyramid.interfaces import IRequest
        from pyramid.request import Request
        from zope.interface import providedBy, implementedBy

        class RequestSub(Request):
            pass

        self.assertTrue(hasattr(Request, '__provides__'))
        self.assertTrue(hasattr(Request, '__implemented__'))
        self.assertTrue(hasattr(Request, '__providedBy__'))
        self.assertFalse(hasattr(RequestSub, '__provides__'))
        self.assertTrue(hasattr(RequestSub, '__providedBy__'))
        self.assertTrue(hasattr(RequestSub, '__implemented__'))

        self.assertTrue(IRequest.implementedBy(RequestSub))
        # The call to implementedBy will add __provides__ to the class
        self.assertTrue(hasattr(RequestSub, '__provides__'))


    def test_subclass_with_implementer(self):
        from pyramid.interfaces import IRequest
        from pyramid.request import Request
        from zope.interface import providedBy, implementedBy, implementer

        @implementer(IRequest)
        class RequestSub(Request):
            pass

        self.assertTrue(hasattr(Request, '__provides__'))
        self.assertTrue(hasattr(Request, '__implemented__'))
        self.assertTrue(hasattr(Request, '__providedBy__'))
        self.assertTrue(hasattr(RequestSub, '__provides__'))
        self.assertTrue(hasattr(RequestSub, '__providedBy__'))
        self.assertTrue(hasattr(RequestSub, '__implemented__'))

        req = RequestSub({})
        req._set_properties({'b': 'b'})

        self.assertTrue(IRequest.providedBy(req))
        self.assertTrue(IRequest.implementedBy(RequestSub))

    def test_subclass_mutate_before_providedBy(self):
        from pyramid.interfaces import IRequest
        from pyramid.request import Request
        from zope.interface import providedBy, implementedBy, implementer

        class RequestSub(Request):
            pass

        req = RequestSub({})
        req._set_properties({'b': 'b'})

        self.assertTrue(IRequest.providedBy(req))
        self.assertTrue(IRequest.implementedBy(RequestSub))

The reason __providedBy__ was copied was an attempt to stop a memory leak in #1212, however as long as we copy over __implemented__ and __provides__ (if it exists) I am not seeing any increase in memory usage (and __providedBy__ even after a call to providedBy() and friends still have the same id()).

I've committed a fix for this, and will push shortly. This should hopefully fix this issue.

[mcdonc]

This was closed by the merging of #1568.