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

Exception-only option when registering views #2660

Merged
merged 8 commits into from Sep 29, 2016

Conversation

Projects
None yet
4 participants
@latteier
Copy link
Contributor

commented Jun 30, 2016

I don't think that this work is fully done yet, but I'm creating a PR to start collaboration and discussion.

I think that code could use some re-factoring. I think that there's too much shared between the exception view registration and the not-found view and forbidden view registration. Probably it could be changed to have less repeated code between those three.

I have some time to work on this going forward, but unfortunately a lot less than I'd like.

Discussion that started this work here: #1646

):
""" Add a view for an exception to the current configuration state.
The view will be called when Pyramid or application code raises an
the given exception.

This comment has been minimized.

Copy link
@stevepiercy

stevepiercy Jun 30, 2016

Member

Remove "an", rewrap "the".

additionally accepts most of the same arguments as the constructor of
:class:`pyramid.view.view_config`. It can be used in the same places,
and behaves in largely the same way, except it always registers an exception
view instead of a 'normal' view.

This comment has been minimized.

Copy link
@stevepiercy

stevepiercy Jun 30, 2016

Member

Should use " instead of ' around "normal".

@mmerickel

This comment has been minimized.

Copy link
Member

commented Jun 30, 2016

Nice, this is an excellent start. Thanks @latteier!

I agree that both forbidden and notfound can be rewritten to use config.add_exception_view internally.

The remaining work to be done on this is some subtle under-the-hood stuff:

  1. Handle conflict resolution / view overriding between exc-only and non-exc-only variants of the same view. A lot of this is to continue to handle config.add_view(context=SomeExceptionType, ...) and config.add_view(context=SomeExceptionType, exception_only=True, ...) where predicates may or may not be different. This may require some complexity on the MultiView to know which is which.

  2. Add ViewDeriverInfo.is_exception_only so that derivers can optimize their pipeline based on the view they're wrapping. This will allow the secured_view and csrf_view to drop the if request.exception checks completely in most cases.

if settings.get('attr') is None:
settings['attr'] = wrapped.__name__

settings['_info'] = info.codeinfo # fbo "action_method"

This comment has been minimized.

Copy link
@stevepiercy

stevepiercy Jun 30, 2016

Member

PEP8: two spaces between code and # for inline comments. Also what is "fbo"? Please avoid acronyms, or whatever it is, and be explicit.

This comment has been minimized.

Copy link
@mmerickel

mmerickel Jun 30, 2016

Member

he's just copying from other pre-existing pyramid code here, steve

This comment has been minimized.

Copy link
@stevepiercy

stevepiercy Jun 30, 2016

Member

OK, @mmerickel, so it needs to be fixed in two places.

This comment has been minimized.

Copy link
@mmerickel

mmerickel Jun 30, 2016

Member

fbo is similar to bbb in that they have standard understandings in software but are not exactly acronyms, at least afaik... the comment is basically saying it's there for the benefit of pyramid's action feature

This comment has been minimized.

Copy link
@mmerickel

mmerickel Sep 20, 2016

Member

@stevepiercy all I meant is that this does not need to be fixed as part of this PR... if the acronyms bother you then they can be fixed in a separate PR later but they're consistent with the current conventions and I don't want unrelated things changed in this PR.

if info.scope == 'class':
# if the decorator was attached to a method in a class, or
# otherwise executed at class scope, we need to set an
# 'attr' into the settings if one isn't already in there

This comment has been minimized.

Copy link
@stevepiercy

stevepiercy Jun 30, 2016

Member

s/into/in

@mmerickel mmerickel added this to the 1.8 milestone Jul 17, 2016

@mmerickel

This comment has been minimized.

Copy link
Member

commented Sep 20, 2016

I have dropped exception_only=True/False and instead added exception=some_exception_type which conflicts with context=some_type. I think this works to make context= accessible via request.context or view(context, request) analogous to the exception-version with exception= accessible via request.exception or view(exception, request) and we can update the docs similarly.

I'd say at this point this PR is mostly complete except for some subtle complexities in the MultiView and updating the permission/csrf view derivers to no-op themselves if they are wrapping exception-only views.

@stevepiercy

This comment has been minimized.

Copy link
Member

commented Sep 20, 2016

@mmerickel I'll stand aside so this can be merged, then I'll clean up the docstrings per #2768.

@bertjwregeer

This comment has been minimized.

Copy link
Member

commented Sep 20, 2016

Does this still allow containment to work as before @mmerickel?

Right now I have:

context=SomeException, containment=MyContext

in some projects, would that get translated to:

exception=SomeException, containment=MyContext

And things will work as before?

@mmerickel

This comment has been minimized.

Copy link
Member

commented Sep 20, 2016

Yeah, the exception is translated to context under the hood. It's just that now in derivers there's an exception key alongside context which will only be there if exception= was used.

@bertjwregeer

This comment has been minimized.

Copy link
Member

commented Sep 21, 2016

Can we add b/w compat by checking if context isexc, and then automatically registering it as an exception view?

@mmerickel

This comment has been minimized.

Copy link
Member

commented Sep 21, 2016

That's already done by the current codebase. We are just adding a path for optimization if you want the view to only serve exceptions by enabling view derivers to do config-time structure optimizations instead of runtime checks of request.exception. Currently if you add context=exc_type it is registered both for normal and exception paths - we haven't changed that in this PR. Here's the rub that I've been working on though when I say "subtle things with MultiView" above. The issue is that we want to maintain bw-compat and support someone adding views via context=exc_type and exception=exc_type. For example:

config.add_view(view, context=Exception, request_method='GET')
config.add_view(view, exception=Exception, request_method='POST')

In the old code (s/exception/context/), these two views would be part of the same MultiView and they would both be registered for normal requests and exception requests. In the new code this is an issue because you are declaring that you don't want the second view to be served for normal requests, however the context is the same and MultiViews must be unique for (context_iface, request_iface, name) triads. This is a corner case and I don't mind making it a bit slower if necessary, but it's one I want to keep working to ensure no issues upgrading, but it's proving quite complex to work out.

@mmerickel

This comment has been minimized.

Copy link
Member

commented Sep 21, 2016

I actually figured out a pretty decent fix for this which is just to have separate MultiViews for exceptions and normal requests instead of sharing them. The memory impact of this should be almost nil since in most cases we are creating one or the other... only in bw-compat scenarios are we creating two multiviews.

@mmerickel

This comment has been minimized.

Copy link
Member

commented Sep 21, 2016

So this is done. If you look at the view deriver implementations they are slightly hairy due to the new api.

We expose info.options['context'] and info.options['exception']. It is easy to tell if the view deriver serves only exceptions by testing for info.options['exception'] is not None, however it is less easy to tell if the deriver serves only non-exceptions which requires testing for isexception(info.options['context']).

I guess it might be better to come up with something slightly more clear to avoid the isexception test.

@mmerickel mmerickel removed the do-not-merge label Sep 21, 2016

@stevepiercy

This comment has been minimized.

Copy link
Member

commented Sep 21, 2016

I didn't notice anything that would require a change to the first diagram in Request Processing, but it would be good to get verification.

@bertjwregeer bertjwregeer self-assigned this Sep 21, 2016

@bertjwregeer

This comment has been minimized.

Copy link
Member

commented Sep 21, 2016

Will review this later today!

isexc = isexception(context)
if not isexc and exception is not None:
raise ConfigurationError(
'view "exception" must be an exception type')

This comment has been minimized.

Copy link
@bertjwregeer

bertjwregeer Sep 22, 2016

Member

Should we have docs that are linked to here that explain that it must either be a subclass of Exception or IException? Is IException something a user would ever create?

This comment has been minimized.

Copy link
@mmerickel

mmerickel Sep 23, 2016

Member

I think that would be a good idea. I considered making pyramid.exceptions.isexception a public API as well. What do you think?

This comment has been minimized.

Copy link
@bertjwregeer

bertjwregeer Sep 24, 2016

Member

I think making that public API would probably be a good idea so that people could use that in various other parts of their code as needed.

@bertjwregeer
Copy link
Member

left a comment

There's just one thing that I am confused about and that is the CSRF view deriver. It seems there are two different ways of dealing with exceptions depending on how they are registered.

order, phash)

self.registry._clear_view_lookup_cache()
renderer_type = getattr(renderer, 'type', None) # gard against None

This comment has been minimized.

Copy link
@bertjwregeer

bertjwregeer Sep 22, 2016

Member

s/gard/guard/

view_classifiers = []
if exception is None:
view_classifiers.append(IViewClassifier)
if isexc:

This comment has been minimized.

Copy link
@bertjwregeer

bertjwregeer Sep 22, 2016

Member

This was confusing at first. My brain didn't immediately click that isexc could potentially be true when the user is registering a context that also happens to be an exception without using exception=. I'd recommend a comment of some sort to make it more clear what is going on so that in the future someone doesn't think isexc means it is an exception so why would exception be None.

This comment has been minimized.

Copy link
@mmerickel

mmerickel Sep 23, 2016

Member

I will rename exception is not None to isexc_only so the test is more clear.

.. versionadded:: 1.3
.. versionchanged:: 1.6

This comment has been minimized.

Copy link
@bertjwregeer

bertjwregeer Sep 22, 2016

Member

What exactly changed in 1.6? Wonder if we should add what changed underneath it, otherwise I am not sure that this versionchanged is all that helpful.

This comment has been minimized.

Copy link
@mmerickel

mmerickel Sep 28, 2016

Member

I pushed changes to master, 1.6-branch and 1.7-branch to fill in this version changed information. It is not yet merged into this branch.

This comment has been minimized.

Copy link
@bertjwregeer
@@ -286,29 +287,38 @@ def _secured_view(view, info):
authn_policy = info.registry.queryUtility(IAuthenticationPolicy)
authz_policy = info.registry.queryUtility(IAuthorizationPolicy)

# no-op on exception-only views without an explicit permission

This comment has been minimized.

Copy link
@bertjwregeer
explicit_val is not None
)
):
if request.method not in safe_methods:

This comment has been minimized.

Copy link
@bertjwregeer

bertjwregeer Sep 22, 2016

Member

Isn't this also missing a check for exceptions? or do you want to enable CSRF checks on exceptions always?

Because down below it is disabled unless it is explicitly set...

This comment has been minimized.

Copy link
@mmerickel

mmerickel Sep 26, 2016

Member

Excellent catch, I messed up the logic here. Thank you.

@mmerickel mmerickel force-pushed the exception_only branch from aaaebb4 to d8b36cd Sep 28, 2016

@mmerickel mmerickel force-pushed the exception_only branch from e90548f to 8ce5736 Sep 29, 2016

@mmerickel

This comment has been minimized.

Copy link
Member

commented Sep 29, 2016

I decided not to change the existing behavior as it's pretty hairy and requires changing the multiview a lot so that it can remove views instead of just replacing them. Maybe we can address this in a separate PR. I have rebased my changes into a single commit and merged master so everything should be good to go.

@mmerickel mmerickel merged commit 8ce5736 into master Sep 29, 2016

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

mmerickel added a commit that referenced this pull request Sep 29, 2016

@bertjwregeer

This comment has been minimized.

Copy link
Member

commented Sep 29, 2016

And travis did the test for the merge now that it is merge-able again ;-)

@bertjwregeer bertjwregeer deleted the exception_only branch Sep 29, 2016

@mmerickel

This comment has been minimized.

Copy link
Member

commented Sep 29, 2016

travis is smart

stevepiercy added a commit to stevepiercy/pyramid that referenced this pull request Sep 29, 2016

stevepiercy added a commit that referenced this pull request Sep 29, 2016

Merge pull request #2776 from stevepiercy/master
Clean up docstrings/narr docs from PR #2660
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.