Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
waweber opened this issue Jun 9, 2020 · 1 comment

Comments

@waweber
Copy link

waweber commented Jun 9, 2020

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
Copy link
Member

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:

pyramid/src/pyramid/view.py

Lines 613 to 622 in 48a0485

for req_type, ctx_type in itertools.product(
request_iface.__sro__, context_iface.__sro__
):
source_ifaces = (view_classifier, req_type, ctx_type)
for view_type in view_types:
view_callable = registered(
source_ifaces, view_type, name=view_name
)
if view_callable is not None:
views.append(view_callable)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants