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

Respect what the APIs consume #289

Merged
merged 8 commits into from
Sep 29, 2016
Merged

Respect what the APIs consume #289

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
14bfab1
Respect what the APIs consume
31z4 Sep 14, 2016
927e8a0
Fix test
31z4 Sep 19, 2016
6abd065
Little fix in App._resolver_error_handler()
31z4 Sep 19, 2016
91aeb69
Add a test
31z4 Sep 29, 2016
58da2b3
Fix a string issue
31z4 Sep 29, 2016
1fb776e
Errors fixed
31z4 Sep 29, 2016
e01171a
Changes after review
31z4 Sep 29, 2016
0d014d0
Fix isort-check issues
31z4 Sep 29, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion connexion/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,11 @@ def __init__(self, swagger_yaml_path, base_url=None, arguments=None,

# A list of MIME types the APIs can produce. This is global to all APIs but can be overridden on specific
# API calls.
self.produces = self.specification.get('produces', list()) # type: List[str]
self.produces = self.specification.get('produces', ['application/json']) # 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.

self.consumes = self.specification.get('consumes', ['application/json']) # type: List[str]

self.security = self.specification.get('security')
self.security_definitions = self.specification.get('securityDefinitions', dict())
Expand Down Expand Up @@ -179,6 +183,7 @@ def add_operation(self, method, path, swagger_operation, path_parameters):
path_parameters=path_parameters,
operation=swagger_operation,
app_produces=self.produces,
app_consumes=self.consumes,
app_security=self.security,
security_definitions=self.security_definitions,
definitions=self.definitions,
Expand Down
1 change: 1 addition & 0 deletions connexion/app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ def _resolver_error_handler(self, *args, **kwargs):
kwargs['operation'] = {
'operationId': 'connexion.handlers.ResolverErrorHandler',
}
kwargs.setdefault('app_consumes', ['application/json'])
return ResolverErrorHandler(self.resolver_error, *args, **kwargs)

def add_error_handler(self, error_code, function):
Expand Down
15 changes: 10 additions & 5 deletions connexion/decorators/parameter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,13 +58,15 @@ def get_val_from_param(value, query_param):
return make_type(value, query_param["type"])


def parameter_to_arg(parameters, function):
def parameter_to_arg(parameters, consumes, function):
"""
Pass query and body parameters as keyword arguments to handler function.

See (https://github.com/zalando/connexion/issues/59)
:param parameters: All the parameters of the handler functions
:type parameters: dict|None
:param consumes: The list of content types the operation consumes
:type consumes: list
:param function: The handler function for the REST endpoint.
:type function: function|None
"""
Expand All @@ -87,10 +89,13 @@ def parameter_to_arg(parameters, function):
def wrapper(*args, **kwargs):
logger.debug('Function Arguments: %s', arguments)

try:
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.

try:
request_body = flask.request.json
except exceptions.BadRequest:
request_body = None
else:
request_body = flask.request.data

if default_body and not request_body:
request_body = default_body
Expand Down
7 changes: 6 additions & 1 deletion connexion/decorators/validation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,12 +97,14 @@ def validate_parameter_list(parameter_type, request_params, spec_params):


class RequestBodyValidator(object):
def __init__(self, schema, is_null_value_valid=False):
def __init__(self, schema, consumes, is_null_value_valid=False):
"""
:param schema: The schema of the request body
:param consumes: The list of content types the operation consumes
:param is_nullable: Flag to indicate if null is accepted as valid value.
"""
self.schema = schema
self.consumes = consumes
self.has_default = schema.get('default', False)
self.is_null_value_valid = is_null_value_valid

Expand All @@ -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.

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.


data = flask.request.json

logger.debug("%s validating schema...", flask.request.url)
Expand Down
9 changes: 6 additions & 3 deletions connexion/operation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ class Operation(SecureOperation):
A single API operation on a path.
"""

def __init__(self, method, path, operation, resolver, app_produces,
def __init__(self, method, path, operation, resolver, app_produces, app_consumes,
path_parameters=None, app_security=None, security_definitions=None,
definitions=None, parameter_definitions=None, response_definitions=None,
validate_responses=False, strict_validation=False, randomize_endpoint=None):
Expand All @@ -128,6 +128,8 @@ def __init__(self, method, path, operation, resolver, app_produces,
:param resolver: Callable that maps operationID to a function
:param app_produces: list of content types the application can return by default
:type app_produces: list
:param app_consumes: list of content types the application consumes by default
:type app_consumes: list
:param path_parameters: Parameters defined in the path level
:type path_parameters: list
:param app_security: list of security rules the application uses by default
Expand Down Expand Up @@ -172,6 +174,7 @@ def __init__(self, method, path, operation, resolver, app_produces,

self.security = operation.get('security', app_security)
self.produces = operation.get('produces', app_produces)
self.consumes = operation.get('consumes', app_consumes)

resolution = resolver.resolve(self)
self.operation_id = resolution.operation_id
Expand Down Expand Up @@ -325,7 +328,7 @@ def function(self):
:rtype: types.FunctionType
"""

function = parameter_to_arg(self.parameters, self.__undecorated_function)
function = parameter_to_arg(self.parameters, self.consumes, self.__undecorated_function)

if self.validate_responses:
logger.debug('... Response validation enabled.')
Expand Down Expand Up @@ -390,7 +393,7 @@ def __validation_decorators(self):
if self.parameters:
yield ParameterValidator(self.parameters, strict_validation=self.strict_validation)
if self.body_schema:
yield RequestBodyValidator(self.body_schema,
yield RequestBodyValidator(self.body_schema, self.consumes,
is_nullable(self.body_definition))

@property
Expand Down
26 changes: 19 additions & 7 deletions tests/test_operation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,7 @@ def test_operation():
path_parameters=[],
operation=OPERATION1,
app_produces=['application/json'],
app_consumes=['application/json'],
app_security=[],
security_definitions=SECURITY_DEFINITIONS,
definitions=DEFINITIONS,
Expand All @@ -255,6 +256,7 @@ def test_operation():

assert operation.method == 'GET'
assert operation.produces == ['application/json']
assert operation.consumes == ['application/json']
assert operation.security == [{'oauth': ['uid']}]

expected_body_schema = {
Expand All @@ -270,6 +272,7 @@ def test_operation_array():
path_parameters=[],
operation=OPERATION9,
app_produces=['application/json'],
app_consumes=['application/json'],
app_security=[],
security_definitions=SECURITY_DEFINITIONS,
definitions=DEFINITIONS,
Expand All @@ -283,6 +286,7 @@ def test_operation_array():

assert operation.method == 'GET'
assert operation.produces == ['application/json']
assert operation.consumes == ['application/json']
assert operation.security == [{'oauth': ['uid']}]
expected_body_schema = {
'type': 'array',
Expand All @@ -298,6 +302,7 @@ def test_operation_composed_definition():
path_parameters=[],
operation=OPERATION10,
app_produces=['application/json'],
app_consumes=['application/json'],
app_security=[],
security_definitions=SECURITY_DEFINITIONS,
definitions=DEFINITIONS,
Expand All @@ -311,6 +316,7 @@ def test_operation_composed_definition():

assert operation.method == 'GET'
assert operation.produces == ['application/json']
assert operation.consumes == ['application/json']
assert operation.security == [{'oauth': ['uid']}]
expected_body_schema = {
'$ref': '#/definitions/composed',
Expand All @@ -326,6 +332,7 @@ def test_non_existent_reference():
path_parameters=[],
operation=OPERATION1,
app_produces=['application/json'],
app_consumes=['application/json'],
app_security=[],
security_definitions={},
definitions={},
Expand All @@ -345,6 +352,7 @@ def test_multi_body():
path_parameters=[],
operation=OPERATION2,
app_produces=['application/json'],
app_consumes=['application/json'],
app_security=[],
security_definitions={},
definitions=DEFINITIONS,
Expand All @@ -364,6 +372,7 @@ def test_invalid_reference():
path_parameters=[],
operation=OPERATION3,
app_produces=['application/json'],
app_consumes=['application/json'],
app_security=[],
security_definitions={},
definitions=DEFINITIONS,
Expand All @@ -382,6 +391,7 @@ def test_no_token_info():
path_parameters=[],
operation=OPERATION1,
app_produces=['application/json'],
app_consumes=['application/json'],
app_security=SECURITY_DEFINITIONS_WO_INFO,
security_definitions=SECURITY_DEFINITIONS_WO_INFO,
definitions=DEFINITIONS,
Expand All @@ -392,6 +402,7 @@ def test_no_token_info():

assert operation.method == 'GET'
assert operation.produces == ['application/json']
assert operation.consumes == ['application/json']
assert operation.security == [{'oauth': ['uid']}]

expected_body_schema = {
Expand All @@ -407,6 +418,7 @@ def test_parameter_reference():
path_parameters=[],
operation=OPERATION4,
app_produces=['application/json'],
app_consumes=['application/json'],
app_security=[],
security_definitions={},
definitions={},
Expand All @@ -419,9 +431,9 @@ def test_resolve_invalid_reference():
with pytest.raises(InvalidSpecification) as exc_info:
Operation(method='GET', path='endpoint', path_parameters=[],
operation=OPERATION5, app_produces=['application/json'],
app_security=[], security_definitions={}, definitions={},
parameter_definitions=PARAMETER_DEFINITIONS,
resolver=Resolver())
app_consumes=['application/json'], app_security=[],
security_definitions={}, definitions={},
parameter_definitions=PARAMETER_DEFINITIONS, resolver=Resolver())

exception = exc_info.value # type: InvalidSpecification
assert exception.reason == "GET endpoint '$ref' needs to start with '#/'"
Expand All @@ -431,14 +443,14 @@ def test_default():
op = OPERATION6.copy()
op['parameters'][1]['default'] = 1
Operation(method='GET', path='endpoint', path_parameters=[], operation=op,
app_produces=['application/json'], app_security=[],
security_definitions={}, definitions=DEFINITIONS,
app_produces=['application/json'], app_consumes=['application/json'],
app_security=[], security_definitions={}, definitions=DEFINITIONS,
parameter_definitions=PARAMETER_DEFINITIONS,
resolver=Resolver())
op = OPERATION8.copy()
op['parameters'][0]['default'] = {
'keep_stacks': 1, 'image_version': 'one', 'senza_yaml': 'senza.yaml', 'new_traffic': 100
}
Operation(method='POST', path='endpoint', path_parameters=[], operation=op, app_produces=['application/json'],
app_security=[], security_definitions={}, definitions=DEFINITIONS, parameter_definitions={},
resolver=Resolver())
app_consumes=['application/json'], app_security=[], security_definitions={},
definitions=DEFINITIONS, parameter_definitions={}, resolver=Resolver())
12 changes: 12 additions & 0 deletions tests/test_resolver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ def test_standard_resolve_x_router_controller():
'operationId': 'post_greeting',
},
app_produces=['application/json'],
app_consumes=['application/json'],
app_security=[],
security_definitions={},
definitions={},
Expand All @@ -61,6 +62,7 @@ def test_resty_resolve_operation_id():
'operationId': 'fakeapi.hello.post_greeting',
},
app_produces=['application/json'],
app_consumes=['application/json'],
app_security=[],
security_definitions={},
definitions={},
Expand All @@ -78,6 +80,7 @@ def test_resty_resolve_x_router_controller_with_operation_id():
'operationId': 'post_greeting',
},
app_produces=['application/json'],
app_consumes=['application/json'],
app_security=[],
security_definitions={},
definitions={},
Expand All @@ -92,6 +95,7 @@ def test_resty_resolve_x_router_controller_without_operation_id():
path_parameters=[],
operation={'x-swagger-router-controller': 'fakeapi.hello'},
app_produces=['application/json'],
app_consumes=['application/json'],
app_security=[],
security_definitions={},
definitions={},
Expand All @@ -106,6 +110,7 @@ def test_resty_resolve_with_default_module_name():
path_parameters=[],
operation={},
app_produces=['application/json'],
app_consumes=['application/json'],
app_security=[],
security_definitions={},
definitions={},
Expand All @@ -120,6 +125,7 @@ def test_resty_resolve_with_default_module_name_lowercase_verb():
path_parameters=[],
operation={},
app_produces=['application/json'],
app_consumes=['application/json'],
app_security=[],
security_definitions={},
definitions={},
Expand All @@ -134,6 +140,7 @@ def test_resty_resolve_with_default_module_name_will_translate_dashes_in_resourc
path_parameters=[],
operation={},
app_produces=['application/json'],
app_consumes=['application/json'],
app_security=[],
security_definitions={},
definitions={},
Expand All @@ -148,6 +155,7 @@ def test_resty_resolve_with_default_module_name_can_resolve_api_root():
path_parameters=[],
operation={},
app_produces=['application/json'],
app_consumes=['application/json'],
app_security=[],
security_definitions={},
definitions={},
Expand All @@ -162,6 +170,7 @@ def test_resty_resolve_with_default_module_name_will_resolve_resource_root_get_a
path_parameters=[],
operation={},
app_produces=['application/json'],
app_consumes=['application/json'],
app_security=[],
security_definitions={},
definitions={},
Expand All @@ -178,6 +187,7 @@ def test_resty_resolve_with_default_module_name_and_x_router_controller_will_res
'x-swagger-router-controller': 'fakeapi.hello',
},
app_produces=['application/json'],
app_consumes=['application/json'],
app_security=[],
security_definitions={},
definitions={},
Expand All @@ -192,6 +202,7 @@ def test_resty_resolve_with_default_module_name_will_resolve_resource_root_as_co
path_parameters=[],
operation={},
app_produces=['application/json'],
app_consumes=['application/json'],
app_security=[],
security_definitions={},
definitions={},
Expand All @@ -206,6 +217,7 @@ def test_resty_resolve_with_default_module_name_will_resolve_resource_root_post_
path_parameters=[],
operation={},
app_produces=['application/json'],
app_consumes=['application/json'],
app_security=[],
security_definitions={},
definitions={},
Expand Down