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

feat(api): Add CORS support #611

Closed
wants to merge 1 commit into from
Closed

feat(api): Add CORS support #611

wants to merge 1 commit into from

Conversation

lwcolton
Copy link
Contributor

Add first-class support for CORS to falcon. Includes a CORS object
for specifying CORS configuration, CORS middleware for applying a
CORS object, and a kwarg to the API class that will take a CORS
object and configure the middleware for you. Allows over-riding
the CORS configuration on a per-resource basis.

Resolves issue #303

Add first-class support for CORS to falcon. Includes a CORS object
for specifying CORS configuration, CORS middleware for applying a
CORS object, and a kwarg to the API class that will take a CORS
object and configure the middleware for you. Allows over-riding
the CORS configuration on a per-resource basis.

Resolves issue #303
@lwcolton
Copy link
Contributor Author

Closed and re-opened to add a change suggested by another contributor:

In falcon/api.py:

>          self._sinks = []
>          self._media_type = media_type
>  
>          self._before = helpers.prepare_global_hooks(before)
>          self._after = helpers.prepare_global_hooks(after)
>  
> +        if cors is not None:
> +            if not isinstance(cors, CORS):
> +                raise ValueError('cors argument must be an instance of '
> +                                 'falcon.cors.CORS')
> +            cors_middleware = CORSMiddleware(cors)
> +            if type(middleware) == list:

its recommended to use isinstance:
if not isinstance(middleware, list):

thanks @ueg1990 !

@ueg1990
Copy link
Contributor

ueg1990 commented Sep 13, 2015

You are welcome. Hope you do not mind if i send you a few more suggestions....I do not have much experience in frameworks and their internals; so trying to contribute however i can :)

@lwcolton
Copy link
Contributor Author

@ueg1990 comments / suggestions are always welcome. I noticed you had some interest in #600 but like I said we can't complete that without Kurt or someone making a decision and editing RTD info. So if you wanted to do something else related to documentation, a tutorial / howto section on CORS somewhere in the manual would be AWESOME, and I won't have to implement it with this changeset.

@gformich
Copy link

Thanks for working on this @lwcolton! We were about to replace our own after hook implementation with something better and it seems we should hold out for this to merge into Falcon.

Any ETA on a merge for your PR?

@lwcolton
Copy link
Contributor Author

Haven't heard anything from the core devs yet.

On Wed, Oct 21, 2015 at 4:57 PM, Greg notifications@github.com wrote:

Thanks for working on this @lwcolton https://github.com/lwcolton! We
were about to replace our own after hook implementation with something
better and it seems we should hold out for this to merge into Falcon.

Any ETA on a merge for your PR?


Reply to this email directly or view it on GitHub
#611 (comment).

Sincerely,

Colton Leekley-Winslow
651-242-9559
lwcolton@gmail.com

@kgriffs
Copy link
Member

kgriffs commented Oct 21, 2015

Hi guys, apologies for the delays. I'm trying to rally the crew to get caught up on all the PRs. Expect reviews soon.

Default is ``None`` (no header sent).

Note:
The arguments above are inclusie, meaning a header, origin, or method
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: 'inclusie'

@painterjd
Copy link
Contributor

As @kgriffs mentioned, we are actively reviewing this patch. It's large, so please bear with us as we dig in deep on this one. So far, it looks like some great work! Kudos.

@yohanboniface
Copy link
Contributor

@lwcolton hi, why not releasing this as a third party plugin instead (something like falcon-cors, see also pytest-falcon or falcon-oauth in this track)?

CORS is very important feature, but IMHO there is no need to change falcon API for adding CORS support, given that this is at the end only a middleware that will do the job. So IMHO falcon current hooks are enough for a full CORS support :)

Also, I think having a more plugins based falcon ecosystem will help keeping the falcon core small and optimized, also putting less pressure over the core maintainers, that can concentrate about core falcon optimisation and hooks needed by plugins.

Last thing, I'd make the CORS API signature a bit simpler, making the middleware to instantiate itself the CORS class, so the final user only need to create a CORSMiddleware() instance.

What do you think? :)

@lwcolton
Copy link
Contributor Author

@yohanboniface Originally Kurt asked for CORS support to be in falcon proper, so that's what I did. However I do like the idea of a plugin based falcon ecosystem, and I am able to release the CORS feature as a standalone package if thats what the community wants. Lastly, making CORSMiddleware instantiate a CORS config for you is a good idea and I can definitely add a helper for that.

@kgriffs
Copy link
Member

kgriffs commented Jan 13, 2016

@painterjd Have you had a chance to review the implementation?

I like the idea of encouraging growth of the ecosystem, as long as we can help community members find what they need; perhaps we could add some notes to the docs that encourage people to search on PyPI for "falcon" to view a list of (potentially useful) add-ons. I hesitate to include a concrete list of packages in the docs, since that might imply favoritism (not to mention the difficulty of keeping that list up to date).

That being said, as a web developer, I would like a way to tell which add-ons are well-tested, and that have been tuned to minimize the amount of overhead they add to the processing of each request. At least in the former case, the usual Travis+Codecov badges can provide some measure of confidence.

Anyway, @lwcolton what do you think about releasing your CORS work as a standalone package?

@lwcolton
Copy link
Contributor Author

@kgriffs Yes absolutely I want to release this a falcon-* package! I have some others that are soon to come, great utility libraries for stuff like decoding JSON and HTTP error handling. All of these are dependencies of an open-source HTTP API framework I'm developing on top of falcon. The name may change and there isn't enough documentation to show off the cool parts, but here's where I'm at: https://github.com/lwcolton/falcon_mongo

Anyway, I have one fix for compatibility with sink classes that I need to add, and then I'll get a falcon-cors package on PyPi. Update with that url coming soon.

@kgriffs
Copy link
Member

kgriffs commented Jan 20, 2016

@lwcolton cool, I always like to see people building cool stuff on top of Falcon. 😄

@kgriffs
Copy link
Member

kgriffs commented Feb 3, 2016

OK, just to bring everyone up to date, I discussed this with a few people in IRC (#falconframework) and we are leaning toward creating a standalone package for this, although it may still live under the falconry umbrella. I'd like to get a final decision on this ASAP.

@davidbgk
Copy link
Contributor

I second the need for such feature, as a dependency or not.

@lwcolton
Copy link
Contributor Author

I am going to release this as its own package, falcon-cors. Everything is ready, could a couple of you please take a look and let me know what you think before I do a release:

https://github.com/lwcolton/falcon-cors/tree/develop

You can install it with:

pip install git+https://github.com/lwcolton/falcon-cors.git@develop

@yohanboniface
Copy link
Contributor

@lwcolton excellent, I'll use it in my projects :)

Out of curiosity, why keeping two classes (CORS and CORSMiddleware)? From a quick look, API would be simpler with only CORSMiddleware to implement. But that's a detail :)

@yohanboniface
Copy link
Contributor

Created a dedicated wiki page to start referencing available plugins https://github.com/falconry/falcon/wiki/Plugins cc @kgriffs

@lwcolton
Copy link
Contributor Author

@yohanboniface The CORS class is separate because it can be attached to a resource to override cors configuration on a per-resource basis. Also, one day I hope to create middlewares for other frameworks

@lwcolton
Copy link
Contributor Author

@kgriffs Hope you are happy with my decision on how to release this, I agree it had to happen ASAP so folks can start using it. If I don't get any other feedback by the end of the day I plan on merging develop into master and releasing on pypi as version 1.0.0

@lwcolton
Copy link
Contributor Author

OK, I released on pypi. https://github.com/lwcolton/falcon-cors

This can be closed and so can #303

@kgriffs
Copy link
Member

kgriffs commented Feb 17, 2016

@lwcolton sounds good, thanks for working on this! You may want to send a note to the mailing list to let everyone know it's available and to ask for additional feedback.

@kgriffs kgriffs closed this Feb 17, 2016
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.

None yet

8 participants