Pylons / pyramid

Pyramid - A Python web framework
https://trypyramid.com/
Other
3.95k stars 883 forks source link

View lookup using context= does not correctly handle abstract base classes #3596

Open waweber opened 4 years ago

waweber commented 4 years ago

Describe the bug View lookup using context= does not support abstract base classes in Pyramid 1.10.4 on Python 3.8.3.

To Reproduce Experiment with the following code:


# A CoolClass has an attribute is_cool = True
class CoolClass(ABC):
    is_cool = True

    # This abstract base class does not require subclassing it
    # Any class with an attribute is_cool = True is a CoolClass
    @classmethod
    def __subclasshook__(cls, other):
        if getattr(other, "is_cool", None) is True:
            return True
        else:
            return False

# Add a view that handles instances of CoolClass
@view_config(context=CoolClass)
def cool_view(context, request):
    return "Too cool!"

# Let's make a traversal resource
class MyCoolClass:
    is_cool = True

    def __init__(self, req):
        self.request = req

# Let's check that MyCoolClass is a subclass of CoolClass
print(issubclass(MyCoolClass, CoolClass))  # True

# Let's check that an instance of MyCoolClass is an instance of CoolClass
cc = MyCoolClass(None)
print(isinstance(cc, CoolClass))  # True

Now set up a Pyramid application with MyCoolClass in the resource tree. You will find that Pyramid will return 404 due to no view callable being found.

Now let's try with a less realistic use case:

class CoolClass(ABC):
    is_cool = True

    # CoolClass's __subclasshook__ now has an additional check
    # If the class has an attribute not_cool = True, it is not a subclass no matter what!
    @classmethod
    def __subclasshook__(cls, other):
        if getattr(other, "not_cool", None) is True:
            return False
        elif getattr(other, "is_cool", None) is True:
            return True
        else:
            return False

# Now let's make a resource explicitly subclassing CoolClass
class NotCoolClass(CoolClass):
    is_cool = True
    not_cool = True  # Makes this class NOT a subclass of CoolClass

    def __init__(self, req):
        self.request = req

# Let's check that NotCoolClass is NOT a subclass of CoolClass
print(issubclass(NotCoolClass, CoolClass))  # False

# Let's check that an instance of NotCoolClass is NOT an instance of CoolClass
cc = NotCoolClass(None)
print(isinstance(cc, CoolClass))  # False

Now set up a Pyramid application with a NotCoolClass resource in the tree. You will find that Pyramid matches cool_view even though NotCoolClass is NOT a CoolClass.

Expected behavior When view lookup happens with a MyCoolClass as the context, cool_view should be selected. Instead, no appropriate view callable is found.

Additional context

The documentation for view configuration states:

An object representing a Python class of which the context resource must be an instance or the interface that the context resource must provide in order for this view to be found and called. This predicate is true when the context resource is an instance of the represented class or if the context resource provides the represented interface; it is otherwise false.

I have confirmed that MyCoolClass is in fact an instance of CoolClass. isinstance() on an instance of MyCoolClass returns True, and issubclass() of its type also returns True. As far as I can tell, the predicate for context= should return True and select this view callable.

Note that everything behaves correctly if MyCoolClass explicitly subclasses CoolClass. I am assuming Pyramid explicitly looks for CoolClass in its hierarchy instead of just using isinstance/issubclass.

All that said, I don't even know if this is a good thing to support or if its a good idea to select views this way, it's just confusing that Pyramid/isinstance disagree. On one hand, if a programmer declares class MyCoolClass:, they might find it surprising that isinstance returns True on something other than itself/object. On the other hand, if isinstance returns True on an object, they might find it surprising that a Pyramid view does not match it. Supporting this case, though, means that one could write some really advanced predicates using just the context= argument.

mmerickel commented 4 years ago

This is probably because zope.interface doesn't support implicit ABCs and I have no clue if that's on their roadmap. Personally I'm not a big fan of them so getting them supported is not high on my list but that is more than likely the issue here. Pyramid defers to the zope component registry to traverse the class SRO and then lookup instances by those interfaces.

It is related to getting the following chunk of code to work: https://github.com/Pylons/pyramid/blob/48a04855ad4f1f1ae6af934090f35a4ad035ed67/src/pyramid/view.py#L613-L622