Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Make predicate mismatches not hide valid views #786

Merged
merged 2 commits into from

4 participants

@latteier

This is mostly an issue with views that use request_method predicates. See Issue #409

Here's a motivating example:

class IResource(Interface):
    ...

@view_config(context=IResource)
def get(context, request):
    ...

@view_config(context=IResource, request_method='POST')
def post(context, request):
    ...

@view_config(context=IResource, request_method='DELETE')
def delete(context, request):
    ...

@implementor(IResource)
class MyResource:
    ...

@view_config(context=MyResource, request_method='POST')
def override_post(context, request):
    ...

Currently the override_post view registration hides the get and delete views in the context of MyResource -- leading to a predicate mismatch error when trying to use GET or DELETE methods. With my patch the views are found and no predicate mismatch is raised.

latteier added some commits
@latteier latteier Make predicate mismatches not hide other possible valid views.
This is mostly an issue for REST style views that use request_method
predicates and are registered for context interfaces.

See Issue #409
b3643d8
@latteier latteier Change log note. 9e1e6d3
@mcdonc
Owner

Hey Amos! Long time no see.

Yeah, for better or worse, as you've found, "context" binds more tightly than any other predicate, so you can tend to think of the set of views that mention a particular context as the subset of views that will be searched when the context of the request matches. It's more equal than all the other predicates.

This is actually mostly an optimization so that we don't need to match every request against every view configuration; only those that match the most specific view registrations made against the request's context. In your example, the most specific registration is for context=MyResource, so it will only grub through the set of views registered for MyResource (and, as you've found, not through the set of views registered for IResource, because, although it matches, it's less specific).

I'm a bit loath to change this, as it really is meant to be an optimization, even if it only meant falling back to sort of looking up the iro as your patch does. That said, I'm willing to hear arguments to the contrary.

Hope you're good!

@latteier

Hey Chris,

Would love to catch up with you sometime soon. Lot's to tell.

I hear what you're saying about context binding, but in practice registering views for an interface and then overriding one or more view for a specific class that implements that interface works well.

For example::

@view_config(context=IResource, name='foo')
def foo(context, request):
    return Response('foo')

@view_config(context=IResource, name='bar')
def bar(context, request):
    return Response('bar')

@view_config(context=MyResource, name='bar')
def override_bar(context, request):
    return Response('overridden bar')

This works fine, and seems like a nice style to me. So it's not just the context that's important, but the view name too.

The issue that my patch tries to address is that this style of view configuration doesn't work when you have multiple views registered for the same name and only want to override one of them, e.g. REST views. The fact that this doesn't work was surprising and frustrating to me, especially since everything works so nicely in other cases.

I'm curious about the optimization issue. Does this patch slow existing sites down? The only thing I think that it would slow down would be returning predicate mismatch errors, since it would do some more checking before giving up. It's true that views that took advantage of the new behavior could be a little slower to serve since they might require multiple adapter look ups. I guess that I should do some performance testing rather than speculation.

-Amos

@latteier

I've been thinking about this a bit more.

I believe that pyramid basically deals with predicates by using a multiview to collect all the various views that differ only by predicate, then it tries various views when a request comes in, catches PredicateMismatches and returning the first view whose predicates are OK.

The issue that I'm trying to address is that when you register views by context interface the multiviews don't get created. Instead view registrations can hide other ones since the contexts aren't the same.

An alternate solution would be to create multiviews by walking the iro at configuration time rather than view lookup time. So for example, if I register a view with a context of MyResource (using the example above) then we could look up the interfaces that MyResource implements and create a multiview with any existing views registered for any of the interfaces it implements that have the same view name. Unfortunately this solution wouldn't address the use case presented in issue #409 since there would be no way to know that IReader and IWriter views belong in a MultiView together. I'm not wild about this solution, but If you like it I'd be happy to create a pull request for it.

Thinking about the performance issue I believe that it makes sense to continue to use context and name as the primary predicates in order to limit the search space for views. However, I don't see the performance issue of walking the iro looking for valid views if you can't find one at first. The alternative is to fail. So basically the change I'm proposing would slow down the speed at which users would receive PredicateMismatch errors. My understanding is that these errors aren't normally supposed to occur. They aren't the sort of thing that an application should regularly return. So I believe that slowing them down a bit shouldn't be a big deal.

Another way of thinking about the performance issue is to say that the change would encourage people to adopt a less efficient method of creating their application. I can understand this argument. However I think that suggesting people use the contextpred solution from issue #409 is even worse from the point of view of encouraging inefficient usage.

In the end I would like to see this change adopted, but if it's not acceptable, that's OK. I can work around the issue.

-Amos

@zefciu

+1

I had a design for a simple CMS where there is a frontend and admin view for a resource collection. The frontend view is type-specific, as it has a custom mako template. The admin view is not - it simply sends all the data to be rendered on browser side with dgrid. This optimisation however doesn't allow me to define a superclass views with xhr=True and subclass views with xhr=False, which seems like the most natural way to do it.

@mmerickel
Owner

For what purpose is Interface excluded from the matching in the fallback? I may be misunderstanding the adapter lookup.

Anyway, @latteier 's comment about this only being used in the error path is a good one and performance issues shouldn't be such a worry when it's making view lookup more sane.

@latteier

There's no good reason to exclude Interface. My original idea was that no views could possibly registered for it, but later I realized that this was a bad assumption.

-Amos

@mmerickel mmerickel merged commit 9e1e6d3 into from
@mmerickel
Owner

I merged this, forcing @mcdonc to think about it. Thanks for the PR!

@latteier

Thanks!

-Amos

@mcdonc
Owner

I surrender.

@lrowe lrowe referenced this pull request from a commit in lrowe/pyramid
@lrowe lrowe Consider superclass views after predicate mismatch
The merged fix for #786 only worked for views registered to an
interface.
f3bffdf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 10, 2013
  1. @latteier

    Make predicate mismatches not hide other possible valid views.

    latteier authored
    This is mostly an issue for REST style views that use request_method
    predicates and are registered for context interfaces.
    
    See Issue #409
  2. @latteier

    Change log note.

    latteier authored
This page is out of date. Refresh to see the latest.
Showing with 129 additions and 2 deletions.
  1. +37 −0 CHANGES.txt
  2. +20 −2 pyramid/router.py
  3. +72 −0 pyramid/tests/test_router.py
View
37 CHANGES.txt
@@ -1,3 +1,40 @@
+Bug Fixes
+---------
+
+- Now predicate mismatches don't hide valid views. This is mostly an
+ issue with views that use request_method predicates. Here's an
+ example that now works::
+
+ class IResource(Interface):
+ ...
+
+ @view_config(context=IResource)
+ def get(context, request):
+ ...
+
+ @view_config(context=IResource, request_method='POST')
+ def post(context, request):
+ ...
+
+ @view_config(context=IResource, request_method='DELETE')
+ def delete(context, request):
+ ...
+
+ @implementor(IResource)
+ class MyResource:
+ ...
+
+ @view_config(context=MyResource, request_method='POST')
+ def override_post(context, request):
+ ...
+
+ Previously the override_post view registration would hide the get
+ and delete views in the context of MyResource -- leading to a
+ predicate mismatch error when trying to use GET or DELETE
+ methods. Now the views are found and no predicate mismatch is
+ raised.
+
+
1.4 (2012-12-18)
================
View
22 pyramid/router.py
@@ -1,4 +1,5 @@
from zope.interface import (
+ Interface,
implementer,
providedBy,
)
@@ -24,6 +25,7 @@
NewResponse,
)
+from pyramid.exceptions import PredicateMismatch
from pyramid.httpexceptions import HTTPNotFound
from pyramid.request import Request
from pyramid.threadlocal import manager
@@ -158,8 +160,24 @@ def handle_request(self, request):
msg = request.path_info
raise HTTPNotFound(msg)
else:
- response = view_callable(context, request)
-
+ try:
+ response = view_callable(context, request)
+ except PredicateMismatch:
+ # look for other views that meet the predicate
+ # criteria
+ for iface in [i for i in context_iface.flattened()
+ if i != Interface]:
+ view_callable = adapters.lookup(
+ (IViewClassifier, request.request_iface, iface),
+ IView, name=view_name, default=None)
+ if view_callable is not None:
+ try:
+ response = view_callable(context, request)
+ break
+ except PredicateMismatch:
+ pass
+ else:
+ raise
return response
def invoke_subrequest(self, request, use_tweens=False):
View
72 pyramid/tests/test_router.py
@@ -1164,6 +1164,78 @@ def test_call_view_raises_exception_view_route(self):
start_response = DummyStartResponse()
self.assertRaises(RuntimeError, router, environ, start_response)
+ def test_call_view_raises_predicate_mismatch(self):
+ from pyramid.exceptions import PredicateMismatch
+ from pyramid.interfaces import IViewClassifier
+ from pyramid.interfaces import IRequest
+ view = DummyView(DummyResponse(), raise_exception=PredicateMismatch)
+ self._registerView(view, '', IViewClassifier, IRequest, None)
+ environ = self._makeEnviron()
+ router = self._makeOne()
+ start_response = DummyStartResponse()
+ self.assertRaises(PredicateMismatch, router, environ, start_response)
+
+ def test_call_view_predicate_mismatch_doesnt_hide_views(self):
+ from pyramid.exceptions import PredicateMismatch
+ from pyramid.interfaces import IViewClassifier
+ from pyramid.interfaces import IRequest, IResponse
+ from pyramid.response import Response
+ from zope.interface import Interface, implementer
+ class IContext(Interface):
+ pass
+ @implementer(IContext)
+ class DummyContext:
+ pass
+ context = DummyContext()
+ self._registerTraverserFactory(context)
+ view = DummyView(DummyResponse(), raise_exception=PredicateMismatch)
+ self._registerView(view, '', IViewClassifier, IRequest,
+ DummyContext)
+ good_view = DummyView('abc')
+ self._registerView(self.config.derive_view(good_view),
+ '', IViewClassifier, IRequest, IContext)
+ router = self._makeOne()
+ def make_response(s):
+ return Response(s)
+ router.registry.registerAdapter(make_response, (str,), IResponse)
+ environ = self._makeEnviron()
+ start_response = DummyStartResponse()
+ app_iter = router(environ, start_response)
+ self.assertEqual(app_iter, [b'abc'])
+
+ def test_call_view_multiple_predicate_mismatches_dont_hide_views(self):
+ from pyramid.exceptions import PredicateMismatch
+ from pyramid.interfaces import IViewClassifier
+ from pyramid.interfaces import IRequest, IResponse
+ from pyramid.response import Response
+ from zope.interface import Interface, implementer
+ class IBaseContext(Interface):
+ pass
+ class IContext(IBaseContext):
+ pass
+ @implementer(IContext)
+ class DummyContext:
+ pass
+ context = DummyContext()
+ self._registerTraverserFactory(context)
+ view1 = DummyView(DummyResponse(), raise_exception=PredicateMismatch)
+ self._registerView(view1, '', IViewClassifier, IRequest,
+ DummyContext)
+ view2 = DummyView(DummyResponse(), raise_exception=PredicateMismatch)
+ self._registerView(view2, '', IViewClassifier, IRequest,
+ IContext)
+ good_view = DummyView('abc')
+ self._registerView(self.config.derive_view(good_view),
+ '', IViewClassifier, IRequest, IBaseContext)
+ router = self._makeOne()
+ def make_response(s):
+ return Response(s)
+ router.registry.registerAdapter(make_response, (str,), IResponse)
+ environ = self._makeEnviron()
+ start_response = DummyStartResponse()
+ app_iter = router(environ, start_response)
+ self.assertEqual(app_iter, [b'abc'])
+
class DummyPredicate(object):
def __call__(self, info, request):
return True
Something went wrong with that request. Please try again.