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

Avoid re-executing the same view when looking up context base views. #1046

Merged
merged 1 commit into from Jul 16, 2013

Conversation

Projects
None yet
3 participants
@lrowe
Copy link
Contributor

commented Jul 15, 2013

This is a tweak of #1004.

Really we should be using subscribers here instead of adapters, but
zope.interface doesn't yet suppport named subscribers.

Avoid re-executing the same view when looking up context base views.
This is a tweak of #1004.

Really we should be using subscribers here instead of adapters, but
zope.interface doesn't yet suppport named subscribers.
@mmerickel

This comment has been minimized.

Copy link
Member

commented Jul 15, 2013

Should this be a set of previous view callables, or do you just need to lookup the immediately previous callable? What you're suggesting seems correct due to the predicate resolution order but I wanted to double check.

@lrowe

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2013

Yes, the specification resolution order means it's only interesting to look at the previous callable. Example:

class A: pass
class B(A): pass
class C(B): pass

@view_config(context=A)
def viewA(): pass

@view_config(context=B, permission='some_permission')
def viewB(): pass

Given a context with class C. The view first view lookup will return viewB which might be disallowed due to a permission check. The next view lookup will be on specification B, returning the same viewB.

@mmerickel

This comment has been minimized.

Copy link
Member

commented Jul 15, 2013

Okay. To be clear a permission failure is not a PredicateMismatch, so your example will not actually work. However if you use a real predicate such as request_method='GET' then what you're saying is fine.

@lrowe

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2013

Right, permissions are special. I actually ended up implementing an additional_permissions predicate for my code...

@mmerickel

This comment has been minimized.

Copy link
Member

commented Jul 15, 2013

Would you mind submitting a test to demonstrate why this pull request is necessary?

@lrowe

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2013

It's not really necessary, just an optimization. I'd argue that that means the tests continuing to pass should be sufficient ;)

@bertjwregeer

This comment has been minimized.

Copy link
Member

commented Jul 15, 2013

Off-topic: if you could, would you put a snippet up somewhere showing how you made permissions into a predicate? I'd be interested in seeing that.

@mmerickel

This comment has been minimized.

Copy link
Member

commented Jul 15, 2013

Feel free to throw that into a cookbook recipe. I think it's dangerous to add it to pyramid's core as permissions have no inherent ordering and therefore requires you to think carefully about what permissions you specify on a collection of views. However some people would find it useful.

@lrowe

This comment has been minimized.

@mmerickel

This comment has been minimized.

Copy link
Member

commented Jul 16, 2013

How do you convert that to a 403? Or do you just always have a default view in the context tree?

mmerickel added a commit that referenced this pull request Jul 16, 2013

Merge pull request #1046 from lrowe/skip-same-views
Avoid re-executing the same view when looking up context base views.

@mmerickel mmerickel merged commit ab457f8 into Pylons:master Jul 16, 2013

1 check passed

default The Travis CI build passed
Details
@lrowe

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2013

@mmerickel they're for views registered in addition to another view. For instance, displaying more information on a user's profile to that user and admins: https://github.com/ENCODE-DCC/encoded/blob/f8cb65b27292ed06396609f414343204aed7d5e6/src/encoded/views/user.py#L57

@mmerickel

This comment has been minimized.

Copy link
Member

commented Jul 16, 2013

Yep, makes a lot of sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.