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

Respect what the APIs consume #289

Merged
merged 8 commits into from
Sep 29, 2016
Merged

Respect what the APIs consume #289

merged 8 commits into from
Sep 29, 2016

Conversation

31z4
Copy link
Contributor

@31z4 31z4 commented Sep 14, 2016

Fixes #287.

Changes proposed in this pull request:

  • If application/json is not listed in the consumes then don't parse and validate the request body.

@@ -125,6 +125,10 @@ def __init__(self, swagger_yaml_path, base_url=None, arguments=None,
# API calls.
self.produces = self.specification.get('produces', list()) # type: List[str]

# A list of MIME types the APIs can consume. This is global to all APIs but can be overridden on specific
# API calls.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please lets not duplicate comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, what exactly do you mean? The comment relates to the consumes param yet it's almost similar to the produces. Should I rewrite it somehow or just remove?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its ok.

@rafaelcaricio
Copy link
Collaborator

@31z4 Thanks for you PR. Could you please write at least one test covering this issue?

@31z4
Copy link
Contributor Author

31z4 commented Sep 14, 2016

@rafaelcaricio of course. Will do this a bit later as well as fix the emerged failures.

Produce and consume `application/json` by default
@coveralls
Copy link

coveralls commented Sep 19, 2016

Coverage Status

Coverage decreased (-0.3%) to 99.725% when pulling 927e8a0 on 31z4:consumes into 58c99a0 on zalando:master.

@coveralls
Copy link

coveralls commented Sep 19, 2016

Coverage Status

Coverage decreased (-0.3%) to 99.725% when pulling 6abd065 on 31z4:consumes into 58c99a0 on zalando:master.

Copy link
Collaborator

@rafaelcaricio rafaelcaricio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need a test when app_produces=[], "produces" definition is optional in the OpenAPI spec.

@@ -114,6 +116,9 @@ def __call__(self, function):

@functools.wraps(function)
def wrapper(*args, **kwargs):
if 'application/json' not in self.consumes:
return function(*args, **kwargs)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary? We should have a test that covers this scenario.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is. Without it validation fails on the plain text request. Added a test.

@coveralls
Copy link

coveralls commented Sep 29, 2016

Coverage Status

Coverage decreased (-0.2%) to 99.817% when pulling 91aeb69 on 31z4:consumes into 58c99a0 on zalando:master.

@coveralls
Copy link

coveralls commented Sep 29, 2016

Coverage Status

Coverage decreased (-0.2%) to 99.817% when pulling 58da2b3 on 31z4:consumes into 58c99a0 on zalando:master.

@coveralls
Copy link

coveralls commented Sep 29, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 1fb776e on 31z4:consumes into 58c99a0 on zalando:master.

request_body = flask.request.json
except exceptions.BadRequest:
request_body = None
if 'application/json' in consumes:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the right check. There are lots of "+json" mimetypes that have to be parsed as json

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use is_json_mimetype for this check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -114,6 +116,9 @@ def __call__(self, function):

@functools.wraps(function)
def wrapper(*args, **kwargs):
if 'application/json' not in self.consumes:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again the wrong check. And I would put the validation logic inside the if to avoid repeating function(*args, **kwargs)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@31z4
Copy link
Contributor Author

31z4 commented Sep 29, 2016

@jmcs Thanks for your feedback!

@coveralls
Copy link

coveralls commented Sep 29, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling e01171a on 31z4:consumes into 58c99a0 on zalando:master.

@coveralls
Copy link

coveralls commented Sep 29, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 0d014d0 on 31z4:consumes into 58c99a0 on zalando:master.

@rafaelcaricio
Copy link
Collaborator

👍

@hjacobs hjacobs merged commit 92e4645 into spec-first:master Sep 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants