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 support for CORS in WebMessageHandler #106

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

Conversation

lkraider
Copy link
Collaborator

Allow easy support for Cross-Origin Resource Sharing requests by
simply returning a list of hosts on the cors_allow_origin method.

@gone
Copy link
Collaborator

gone commented Feb 19, 2013

Dude awesome job!

I really like the idea of using prepare to determine if the resource is CORS-able or not - but can you split that out into a separate decorator? My understanding is that prepare is really meant for subclasses to overwrite - I don't want to depend on functionality there. Also making it into a decorator allows people to explicitly mark which resources are for cors and which ones are not.

Can you make origin, headers, methods, exposed headers, and allowed credentials properties, rather than methods? Keeping those static as much as possible should make things faster.

What do you think about turning the part that handles credentials/allow-origin = * into a function so it's not duplicated? I could go either way on that one.

Thoughts?

@lkraider
Copy link
Collaborator Author

Thanks for reviewing it!

I was already pending towards defining it as a decorator, makes sense.

Turning those methods into simple instance attributes is better. I think I'll override __init__ for that then, as initialize is called on clear_payload and it's unnecessary to reset those values there. Although subclasses will probably redefine them there.

I thought about turning that snippet into a function, but since on runtime it's basically a condition check and an assignment, I thought a function call would be more overhead than repeating a little code.

@gone
Copy link
Collaborator

gone commented Feb 19, 2013

that's probably true that a function is more overhead.

I think since they're static they should be class properties, rather than
using init. If subclasses need dynamic attributes, they can use
@Property

I think that will be faster/less overhead, and semantically I think it's
better - resources generally don't have different CORS rules depending on
the request.

On Tue, Feb 19, 2013 at 4:38 PM, Paul Eipper notifications@github.comwrote:

Thanks for reviewing it!

I was already pending towards defining it as a decorator, makes sense.

Turning those methods into simple instance attributes is better. I think
I'll override init for that then, as initialize is called on
clear_payload and it's unnecessary to reset those values there. Although
subclasses will probably redefine them there.

I thought about turning that snippet into a function, but since on runtime
it's basically a condition check and an assignment, I thought a function
call would be more overhead than repeating a little code.


Reply to this email directly or view it on GitHubhttps://github.com//pull/106#issuecomment-13801306.

@gone
Copy link
Collaborator

gone commented Feb 19, 2013

Also this is so, so slick. I have a bunch of crappy patched @cors_support
resources in an app that just set allowed headers -I'd love to replace
those with something better designed.

On Tue, Feb 19, 2013 at 4:50 PM, Ben Beecher benbeecher@gmail.com wrote:

that's probably true that a function is more overhead.

I think since they're static they should be class properties, rather than
using init. If subclasses need dynamic attributes, they can use
@Property

I think that will be faster/less overhead, and semantically I think it's
better - resources generally don't have different CORS rules depending on
the request.

On Tue, Feb 19, 2013 at 4:38 PM, Paul Eipper notifications@github.comwrote:

Thanks for reviewing it!

I was already pending towards defining it as a decorator, makes sense.

Turning those methods into simple instance attributes is better. I think
I'll override init for that then, as initialize is called on
clear_payload and it's unnecessary to reset those values there. Although
subclasses will probably redefine them there.

I thought about turning that snippet into a function, but since on
runtime it's basically a condition check and an assignment, I thought a
function call would be more overhead than repeating a little code.


Reply to this email directly or view it on GitHubhttps://github.com//pull/106#issuecomment-13801306.

@lkraider
Copy link
Collaborator Author

lkraider commented Mar 8, 2013

I believe I addressed your points with these commits, although I didn't test the code thoroughly after the refactoring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants