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.

Sign up for GitHub

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

Jump to bottom

Made the set_cors_headers method in core.py compatible with resp.head… #172

Merged
merged 3 commits into from Aug 17, 2016
Merged

Made the set_cors_headers method in core.py compatible with resp.head… #172

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b45aa4a
Made the set_cors_headers method in core.py compatible with resp.head…
BillBrower Aug 16, 2016
d1251b4
Simplified the non MultiDict workaround in the set_cors_headers metho…
BillBrower Aug 16, 2016
989fced
Added test coverage for overridden response headers and modified the …
BillBrower Aug 17, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions flask_cors/core.py
View file
Open in desktop
Expand Up @@ -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:
Copy link
Owner

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:

  1. 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.
  2. I think this check is inverted. If headers is not a MultiDict, then we want to set it to be a MultiDict.
  3. 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)

Copy link
Owner

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)

multidict = MultiDict()
for k, v in resp.headers.items():
multidict.add(k, v)
resp.headers = multidict

headers_to_set = get_cors_headers(options, request.headers, request.method)

LOG.debug('Settings CORS headers: %s', str(headers_to_set))
Expand Down