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

Perform request.after as long as the response is okay #346

Closed
href opened this Issue Oct 2, 2015 · 5 comments

Comments

Projects
None yet
2 participants
@href
Copy link
Member

href commented Oct 2, 2015

We've had this before a number of times and we even have a doc entry about it. I encountered this again today and I think we can improve on it.

As it stands, request.after will only execute if the response is created by the renderer:

content = self(request, obj)
if isinstance(content, BaseResponse):
    # the view took full control over the response
    return content
response = self.render(content, request)
request.run_after(response)
return response

This is a bit of a gotcha. I think it does make sense not to run it if the response is a permission error for example, but if the response is something that is okay, we should execute this code.

Apart from making it less of a gotcha it would enable us to write functions which given a request, can manipulate the response if it is successful. An example would be a login function. I could pass the request to the function and it could log me in based on that request. All without knowing about the response:

def login(request):
    if request.params.get('token') == 'my-token':
        # <- get the identity
        @request.after
        def remember_identity(response):
           # <- remember the identity
          return True
    return False

Which would enable this nice little view:

if login(request):
    return morepath.redirect(request.link(self))
return content

I think we can even do this rather elegantly, because webob exceptions have nice hierarchy to differentiate between good and bad responses:

HTTPOk
  * 200 - :class:`HTTPOk`
  * 201 - :class:`HTTPCreated`
  * 202 - :class:`HTTPAccepted`
  * 203 - :class:`HTTPNonAuthoritativeInformation`
  * 204 - :class:`HTTPNoContent`
  * 205 - :class:`HTTPResetContent`
  * 206 - :class:`HTTPPartialContent`
HTTPRedirection
  * 300 - :class:`HTTPMultipleChoices`
  * 301 - :class:`HTTPMovedPermanently`
  * 302 - :class:`HTTPFound`
  * 303 - :class:`HTTPSeeOther`
  * 304 - :class:`HTTPNotModified`
  * 305 - :class:`HTTPUseProxy`
  * 307 - :class:`HTTPTemporaryRedirect`

How about we check if the response is an instance of HTTPOk or HTTPRedirection and if so, we call request.after, even if the response was not created by the render function?

@href

This comment has been minimized.

Copy link
Member

href commented Dec 8, 2015

I'm getting more and more convinced this is the right thing to do. See also http://racksburg.com/choosing-an-http-status-code/

2XX and 3XX responses of any kind should trigger a request.after. Any other response should not. @faassen any objections? I think this would be a good change and if we do it we should do it before 1.0 as it might possibly break things.

@href href self-assigned this Dec 8, 2015

@href href added this to the 1.0 milestone Dec 8, 2015

@faassen

This comment has been minimized.

Copy link
Member

faassen commented Dec 8, 2015

I'm +1 to do this. It will make after functions run in a few more cases which is strictly incompatible but I think it won't create too many issues.

@href

This comment has been minimized.

Copy link
Member

href commented Dec 8, 2015

Yay, I'll commit the code, test and docs this week :-)

href added a commit that referenced this issue Dec 9, 2015

@href

This comment has been minimized.

Copy link
Member

href commented Dec 9, 2015

I did it more or less the way i thought I would. I didn't anticipate the exception views though. I decided to be consistent and make sure that all 2XX and 3XX requests result in a call to after, even if they are the result of a webob exception. I'd appreciate it if you had a look at my implementation for feedback.

@faassen

This comment has been minimized.

Copy link
Member

faassen commented Apr 4, 2016

This looks okay to me, so closing issue.

@faassen faassen closed this Apr 4, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment