- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 772
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
Fix KeyError if no Content-Type was provided in request #497
Conversation
@f11r looks good, thanks for the PR! Could you add a test for this case please? Thanks! |
@f11r answering your question: yes, we can quickly release it, but please add a test 😄 |
@hjacobs |
@f11r I think you can add it here: https://github.com/zalando/connexion/blob/6668835420231c15095a71e264b2e2b0e1a68bf3/tests/api/test_responses.py#L203 It currently only tests the status code (415) on "wrong" content type (i.e. not the case where |
It actually looks like you already did have the correct test here https://github.com/zalando/connexion/blob/6668835420231c15095a71e264b2e2b0e1a68bf3/tests/api/test_responses.py#L213, but werkzeug always sets |
@f11r maybe you can do an "artificial" manual HTTP request in the test and not use the test client? |
Added a test, not particularly pretty. Not sure if something like |
Hmm, test is failing now: https://travis-ci.org/zalando/connexion/jobs/260553758 |
I seem to be on a roll here :D In a clean python3.5 environment running I'll try to figure out why tomorrow. |
…for no-content-type-test since old flask<=0.12.2 does not support it.
Figured out what is going on: |
tests/api/test_responses.py
Outdated
# (https://github.com/pallets/werkzeug/issues/1159) | ||
# so that content-type is added to every request, we remove it here manually for our test | ||
# this test can be removed once the werkzeug issue is addressed | ||
from werkzeug.test import Client, EnvironBuilder |
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.
Can you move this import to the top level (beginning of the file)?
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.
done
👍 |
At the moment if no
Content-Type
is provided in the request the validation wrapper raises aKeyError
.Can we expect a fix to make it into a release soon or should we look for a temporary workaround until that happens?