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

Better handling of cors_enable=True + added CORSMiddleware #1977

Closed
vytas7 opened this issue Oct 17, 2021 · 5 comments · Fixed by #2201
Closed

Better handling of cors_enable=True + added CORSMiddleware #1977

vytas7 opened this issue Oct 17, 2021 · 5 comments · Fixed by #2201

Comments

@vytas7
Copy link
Member

vytas7 commented Oct 17, 2021

Although similar to #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).

@vytas7 vytas7 added good first issue Comment on this issue if you'd like to volunteer to work on this. Thanks! enhancement proposal needs contributor Comment on this issue if you'd like to volunteer to work on this. Thanks! Hacktoberfest labels Oct 17, 2021
@vytas7 vytas7 added this to the Triaged (Non-Breaking Changes) milestone Oct 17, 2021
@CaselIT
Copy link
Member

CaselIT commented Oct 18, 2021

connected to #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 vytas7 removed needs contributor Comment on this issue if you'd like to volunteer to work on this. Thanks! Hacktoberfest good first issue Comment on this issue if you'd like to volunteer to work on this. Thanks! labels Oct 21, 2021
@vytas7
Copy link
Member Author

vytas7 commented Oct 21, 2021

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
Copy link
Member Author

vytas7 commented Aug 15, 2022

See also #2095. Rescheduling for 4.0.

@vytas7
Copy link
Member Author

vytas7 commented Dec 16, 2023

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

@CaselIT
Copy link
Member

CaselIT commented Dec 16, 2023

makes sense

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

Successfully merging a pull request may close this issue.

2 participants