Fix @crossdomain decorator #3113
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hello @amCap1712! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2025-01-09 18:25:56 UTC |
|
As this touches almost all views, deployed on beta for testing for a while. |
When handling OPTIONS requests, Flask can either generate an automatic response internally or route the request to the view function for handling. The request is routed to the view function if the view function has a provides_automatic_options attribute set to False or if the methods list passed to route definition includes OPTIONS. OPTIONS requests handled by Flask internally don't have CORS headers. Hence, API endpoints that need to support CORS should ensure that the OPTIONS request is routed to the view function. The @crossdomain decorator sets the provides_automatic_options attribute on the view function which should have ensured this. However, due to the way decorators work the attribute should have instead been set on the decorator function itself for Flask to pick it up. However, we often list OPTIONS in the methods list of the route anyway when applying @crossdomain. As a result, Flask would still route the request as expected. The errors would only happen when the OPTIONS method was not mentioned in the route's methods list. @crossdomain decorator has now been fixed to set the attribute correctly and also add OPTIONS to the methods list automatically. Further, a test has been added to ensure that all API endpoints support CORS.
amCap1712
force-pushed
the
update-decorator
branch
from
960c153 to
718f342
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When handling OPTIONS requests, Flask can either generate an automatic response internally or route the request to the view function for handling. The request is routed to the view function if the view function has a provides_automatic_options attribute set to False or if the methods list passed to route definition includes OPTIONS. OPTIONS requests handled by Flask internally don't have CORS headers. Hence, API endpoints that need to support CORS should ensure that the OPTIONS request is routed to the view function.
The https://github.com/crossdomain decorator sets the provides_automatic_options attribute on the view function which should have ensured this. However, due to the way decorators work the attribute should have instead been set on the decorator function itself for Flask to pick it up. However, we often list OPTIONS in the methods list of the route anyway when applying https://github.com/crossdomain. As a result, Flask would still route the request as expected. The errors would only happen when the OPTIONS method was not mentioned in the route's methods list.
https://github.com/crossdomain decorator has now been fixed to set the attribute correctly and also add OPTIONS to the methods list automatically. Further, a test has been added to ensure that all API endpoints support CORS.
While at it, also refactor the views to use newer .get/.post decorators insteads of methods list.