Skip to content

Conversation

@nourspace
Copy link
Contributor

Description

Determining the version and performing content negotiation should be done before ensuring the permission of the request. The reason is that these information can be used in handling the exceptions. For example different versions may return different error scheme. Also, the rendering class can be used to determine how to exception handler response should be rendered.

Determining the version and performing content negotiation should be done before ensuring the permission of the request. The reason is that these information can be used in handling the exceptions. For example different versions may return different error scheme. Also, the rendering class can be used to determine how to exception handler response should be rendered.
@jpadilla
Copy link
Contributor

At a first glance this looks like a small valid enhancement. Anyone else care to review/comment?

@xordoquy
Copy link
Contributor

Seems valid too. Need to think about the side effects related to security of performing computations before permissions checks.

@jpadilla
Copy link
Contributor

Can't think of any atm, since it's just content neg and determining correct version.

@lovelydinosaur
Copy link
Contributor

Consideration: Any point we push permissions later, there are more opportunities to consume resources etc. Having said that I don't think I've any great problems with this change.

@jpadilla
Copy link
Contributor

I can see how useful it might be to have access to request.version when performing authentication, checking permissions and throttling.

@lovelydinosaur
Copy link
Contributor

I'm 👍 with this pull request.

@xordoquy xordoquy added this to the 3.4.0 Release milestone Mar 31, 2016
@xordoquy
Copy link
Contributor

Ok, let's get this moving.

@xordoquy xordoquy merged commit 67ac048 into encode:master Mar 31, 2016
@xordoquy
Copy link
Contributor

nice work, thanks :)

@nourspace
Copy link
Contributor Author

Thank you

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants