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

Add a RouteFound event which will fire after a route is found #1876

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
6 participants
@dstufft
Copy link
Contributor

dstufft commented Aug 23, 2015

Fixes #1875

Opened this up for discussion on the implementation, this allows something like:

from pyramid.events import subscriber, RouteFound

class ReadOnlyPredicate(object):

    def __init__(self, val, config):
        self.val = val

    def text(self):
        return "read_only = {!r}".format(self.val)

    phash = text

    def __call__(self, info, request):
        return True


@subscriber(RouteFound)
def set_transaction_isolation(event):
    for predicate in event.request.matched_route.predicates:
        if isinstance(predicate, ReadOnlyPredicate) and predicate.val:
            event.request.db.execute(
                """ SET TRANSACTION
                    ISOLATION LEVEL SERIALIZABLE READ ONLY DEFERRABLE
                """
            )

You can then set a route as a "read only" route that will have a read only transaction for it. This can't be done in a decorator or a view mapper or a tween because it needs to occur before the context is looked up (because that may or may not trigger a database query, and if it does you can't change the isolation level). This is important if you're using a strict isolation level so you can have read only views take a less invasive lock.

@wichert

This comment has been minimized.

Copy link
Member

wichert commented Aug 23, 2015

That sounds incredibly useful.

@tseaver

This comment has been minimized.

Copy link
Member

tseaver commented Aug 23, 2015

Hmm, I guess I would have solved the problem by configuring a "root factory" for the read-only routes which set up the correct isolation.

If others don't object to the event (I don't mind it), the PR will need test coverage and narrative / API docs to accompany the new feature.

@dstufft

This comment has been minimized.

Copy link
Contributor Author

dstufft commented Aug 23, 2015

Yea, I will absolutely write docs and tests! Just wanted to get opinions before I bothered.

@bertjwregeer

This comment has been minimized.

Copy link
Member

bertjwregeer commented Aug 23, 2015

👍 I like this idea.

@sontek

This comment has been minimized.

Copy link
Member

sontek commented Oct 12, 2015

@dstufft You should definitely write the docs and tests :) 👍

@bertjwregeer

This comment has been minimized.

Copy link
Member

bertjwregeer commented Jan 3, 2016

Reading the issue #1875 it looks like @mmerickel might be onboard if this was renamed to BeforeTraversal rather than RouteFound. So I propose a s/RouteFound/BeforeTraversal/ on this PR.

@mmerickel

This comment has been minimized.

Copy link
Member

mmerickel commented Jan 16, 2016

I'm going back and forth right now but I think RouteFound might be better. The issue with BeforeTraversal is that it would probably imply that it gets invoked during other traversal methods like pyramid.traversal.traverse similar to how BeforeRender is invoked for pyramid.renderers.render (more than one invocation possible per request).

The issue with RouteFound is that it is invoked whether or not a route is actually found. Remember a pattern may not match. Thus RouteFound would currently be emitted even if the router did not match a route!

@dstufft

This comment has been minimized.

Copy link
Contributor Author

dstufft commented Jan 16, 2016

In my PR it is only executed when a route is found I believe, I don't have a major opinion on BeforeTraversal vs RouteFound except that in my original use case, BeforeTraversal would have been "more correct" (I think).

@bertjwregeer bertjwregeer added this to the 1.7 milestone Jan 18, 2016

@mmerickel

This comment has been minimized.

Copy link
Member

mmerickel commented Feb 28, 2016

I've chewed on this some more. If you rename it to BeforeTraversal and make sure the PR makes sense with those semantics then I'm +1. We also need to update the docs to expose these new APIs (event object, interface, and possibly some list of events somewhere if it exists).

@bertjwregeer

This comment has been minimized.

Copy link
Member

bertjwregeer commented Apr 11, 2016

I've pulled this down, and renamed it, and pushed it as a new branch to this repo so that any further changes can be made on this PR directly: #2469.

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.