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
Made the set_cors_headers method in core.py compatible with resp.head… #172
Made the set_cors_headers method in core.py compatible with resp.head… #172
Conversation
…ers objects that are standard python dicts, by copying the underlying data into a new MultiDict.
Changes Unknown when pulling b45aa4a on BillBrower:set_cors_headers_updates into * on corydolphin:master*. |
1 similar comment
Changes Unknown when pulling b45aa4a on BillBrower:set_cors_headers_updates into * on corydolphin:master*. |
@@ -226,6 +226,14 @@ def set_cors_headers(resp, options): | |||
LOG.debug('CORS have been already evaluated, skipping') | |||
return resp | |||
|
|||
# If resp.headers is not a MultiDict, set resp.headers to a new | |||
# MultiDict, copying the underlying dict. | |||
if type(resp.headers) is MultiDict: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome start!
Three quick pieces of feedback:
- It is more Pythonic to check isinstance, e.g.
isinstance(resp.headers, MultiDict)
, since anything that extends MultiDict will also (by extension) have the methods we need. - I think this check is inverted. If headers is not a MultiDict, then we want to set it to be a MultiDict.
- Instead of manually iterating the
headers
object, we can let MultiDict do the work for us by passing it to the constructor.
I think combined, it would simplify a little bit to something like:
# OauthLib sets resp.headers to a non MultiDict. Weird, but let's work around it.
if not isinstance(resp.headers, MultiDict):
resp.headers = MultiDict(resp.headers)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for testing, could you please add a test like:
class ResponseHeadersOverrideTest(FlaskCorsTestCase):
def test_override_headers(self):
'''
Ensure we work even if response.headers is set to something other than a MultiDict.
'''
self.app = Flask(__name__)
CORS(self.app)
@self.app.route('/')
def index():
response.headers = {"custom", "dictionary"}
return 'Welcome'
for resp in self.iter_responses('/):
self.assertTrue(ACL_ORIGIN in resp.headers)
…d in core.py and made it more pythonic.
Awesome work here, thanks so much for taking the time to not only report the issue, but even fix it :-) ! I had a few quick pieces of feedback, and a suggestion/request for a test. |
Copying my comment which was lost in the diff (my fault!) As for testing, could you please add a test like: class ResponseHeadersOverrideTest(FlaskCorsTestCase):
def test_override_headers(self):
'''
Ensure we work even if response.headers is set to something other than a MultiDict.
'''
self.app = Flask(__name__)
CORS(self.app)
@self.app.route('/')
def index():
response.headers = {"custom", "dictionary"}
return 'Welcome'
for resp in self.iter_responses('/):
self.assertTrue(ACL_ORIGIN in resp.headers) And double check that it fails before hand? I didn't actually run this code, so knowing me, there's probably a few syntax errors :-) |
Nice catch, the check was definitely inverted. Thanks for walking me through this. It looks like that last commit failed so I'll fix that and add a test or two. |
…set_cors_headers method in core.py to respect Werkzeug Headers objects as well (because they allow repeated values and some of the pre-existing tests use them).
Changes Unknown when pulling 989fced on BillBrower:set_cors_headers_updates into * on corydolphin:master*. |
# Some libraries, like OAuthlib, set resp.headers to non Multidict | ||
# objects (Werkzeug Headers work as well). This is a problem because | ||
# headers allow repeated values. | ||
if (not isinstance(resp.headers, Headers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome catch that there are multiple types here. This makes me realize that checking the actual type is not necessarily what we want, rather we can duck-type this and make it a little more flexible for the future.
I'd suggest we change this to be:
if (not hasattr(resp.headers, 'add')):
Feel free to push back if that seems to nit-picky!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@corydolphin No, that sounds fine to me. I didn't realize you could do that!
Edit: Does having an add method necessarily imply that the object can handle repeated values?
@BillBrower awesome work!!! I had one nit-picky-comment, I'd like your feedback on. Otherwise, this looks great. I'll plan to merge it up and push out a 3.0.0 release tonight, which I've been neglecting to do for a few weeks. |
@corydolphin thanks for merging this! Did you see my line comment regarding duck typing above? |
Ahh shoot! Apologies, I didn't respond directly. That is a good point, and a deficiency of duck typing in general. Especially with a name as generic as "add", it is possible that someone (much like OAuthLib does) would use their own class, and have an 'add' method which doesn't support repeated values. In my opinion, your current approach is solid. In cases where it doesn't quite work, worst case is a new MultiDict is constructed. |
…ers objects that are standard python dicts, by copying the underlying data into a new MultiDict.