falconry / falcon

The no-magic web data plane API and microservices framework for Python developers, with a focus on reliability, correctness, and performance at scale.
https://falcon.readthedocs.io/en/stable/
Apache License 2.0
9.53k stars 945 forks source link

Better handling of `cors_enable=True` + added `CORSMiddleware` #1977

Closed vytas7 closed 11 months ago

vytas7 commented 3 years ago

Although similar to https://github.com/falconry/falcon/issues/1930, this issue proposes to improve not only documentation, but also provide a clearer indication to the developer who has happened to pass this unsupported combination of parameters.

Maybe we could emit a warning, silently disable the implicitly added middleware component if we detect that any instance of CORSMiddleware is already present, or even raise an Exception? (Although the latter might be seen as a breaking change -- discuss).

CaselIT commented 3 years ago

connected to https://github.com/falconry/falcon/pull/1976

Probably the best choice here is to deprecate the flag, since any other case would not be immediately clear to understand. An alternative may be to raise a deprecation warning and the raise an exception in the next major

vytas7 commented 3 years ago

Hm, on the one hand, it may make sense to be explicit, on the other, it's convenient to have a flag to quickly enable permissive CORS, since it is what quite many users are after.

Maybe it wouldn't be too complicated to only prepend CORSMiddleware when updating the middleware list only if there is no other object where isinstance(component, CORSMiddleware)?

In either case, I have removed the contributor/Hacktoberfest labels before we make a decision on this, not to get any potential contributor dragged into this beforehand.

vytas7 commented 2 years ago

See also https://github.com/falconry/falcon/discussions/2095. Rescheduling for 4.0.

vytas7 commented 11 months ago

I went ahead and decided to simply raise a ValueError in this case.

CaselIT commented 11 months ago

makes sense