From eb97cf9f742b1b81f36e453e1de07a28778f963f Mon Sep 17 00:00:00 2001 From: Davy Durham Date: Fri, 18 Feb 2022 10:44:51 -0600 Subject: [PATCH] Fix for aiohttp and multipart/form-data uploads (#1222) * Added unit tests to demonstrate the problems of https://github.com/zalando/connexion/issues/975 - Taken mostly from existing PR: https://github.com/zalando/connexion/pull/987 * now splitting out multipart POSTs into files[] and form[], handling duplicate keys as the rest of connexion expects - Based parly on existing PR: https://github.com/zalando/connexion/pull/987 * rewrote how operations/openapi.py::_get_body_argument() works to better build the arguments[] list according to what the spec says and what the handler accepts. This fixes a bug when requests contain mixed files and form values and the handler is expecting variable names matching the request property names. * Adding unit tests to improve code converage test * post merge fixes - using 'async' keyword now in new unit test file * unit test improvements -- now testing the contents of the files we upload too * making some code a bit clearer regarding duplicate names of file submissions * fixing up unit tests since merging main * fixing isort-check-tests and flake8 * clarified a comment * comment correction * after discussions with maintainer, reverted _get_body_argument back to the original where it does not attempt to break out the body into individual arguments for the handler. But left in changes that make the normal behavior of not passing a body argument to a handler without one more consistent when the body itself is empty or not an object type. * fixing unit tests after after reverting _get_body_argument behavior --- connexion/apis/aiohttp_api.py | 48 ++++++- connexion/operations/openapi.py | 15 +- tests/aiohttp/test_aiohttp_multipart.py | 118 ++++++++++++++++ tests/api/test_parameters.py | 13 ++ tests/fakeapi/aiohttp_handlers.py | 42 ++++++ tests/fakeapi/hello/__init__.py | 6 + tests/fixtures/aiohttp/openapi_multipart.yaml | 132 ++++++++++++++++++ tests/fixtures/simple/openapi.yaml | 37 +++++ tests/fixtures/simple/swagger.yaml | 44 ++++++ 9 files changed, 449 insertions(+), 6 deletions(-) create mode 100644 tests/aiohttp/test_aiohttp_multipart.py create mode 100644 tests/fixtures/aiohttp/openapi_multipart.yaml diff --git a/connexion/apis/aiohttp_api.py b/connexion/apis/aiohttp_api.py index 0fdffc604..6adaf5101 100644 --- a/connexion/apis/aiohttp_api.py +++ b/connexion/apis/aiohttp_api.py @@ -305,13 +305,52 @@ async def get_request(cls, req): :rtype: ConnexionRequest """ url = str(req.url) - logger.debug('Getting data and status code', - extra={'can_read_body': req.can_read_body, 'url': url}) + + logger.debug( + 'Getting data and status code', + extra={ + # has_body | can_read_body report if + # body has been read or not + # body_exists refers to underlying stream of data + 'body_exists': req.body_exists, + 'can_read_body': req.can_read_body, + 'content_type': req.content_type, + 'url': url, + }, + ) query = parse_qs(req.rel_url.query_string) headers = req.headers body = None - if req.body_exists: + + # Note: if request is not 'application/x-www-form-urlencoded' nor 'multipart/form-data', + # then `post_data` will be left an empty dict and the stream will not be consumed. + post_data = await req.post() + + files = {} + form = {} + + if post_data: + logger.debug('Reading multipart data from request') + for k, v in post_data.items(): + if isinstance(v, web.FileField): + if k in files: + # if multiple files arrive under the same name in the + # request, downstream requires that we put them all into + # a list under the same key in the files dict. + if isinstance(files[k], list): + files[k].append(v) + else: + files[k] = [files[k], v] + else: + files[k] = v + else: + # put normal fields as an array, that's how werkzeug does that for Flask + # and that's what Connexion expects in its processing functions + form[k] = [v] + body = b'' + else: + logger.debug('Reading data from request') body = await req.read() return ConnexionRequest(url=url, @@ -321,7 +360,8 @@ async def get_request(cls, req): headers=headers, body=body, json_getter=lambda: cls.jsonifier.loads(body), - files={}, + form=form, + files=files, context=req) @classmethod diff --git a/connexion/operations/openapi.py b/connexion/operations/openapi.py index d6f958b5e..f29877217 100644 --- a/connexion/operations/openapi.py +++ b/connexion/operations/openapi.py @@ -272,12 +272,21 @@ def body_definition(self): return {} def _get_body_argument(self, body, arguments, has_kwargs, sanitize): + if len(arguments) <= 0 and not has_kwargs: + return {} + x_body_name = sanitize(self.body_schema.get('x-body-name', 'body')) + + # if the body came in null, and the schema says it can be null, we decide + # to include no value for the body argument, rather than the default body if is_nullable(self.body_schema) and is_null(body): - return {x_body_name: None} + if x_body_name in arguments or has_kwargs: + return {x_body_name: None} + return {} + # now determine the actual value for the body (whether it came in or is default) default_body = self.body_schema.get('default', {}) - body_props = {k: {"schema": v} for k, v + body_props = {sanitize(k): {"schema": v} for k, v in self.body_schema.get("properties", {}).items()} # by OpenAPI specification `additionalProperties` defaults to `true` @@ -287,11 +296,13 @@ def _get_body_argument(self, body, arguments, has_kwargs, sanitize): if body is None: body = deepcopy(default_body) + # if the body isn't even an object, then none of the concerns below matter if self.body_schema.get("type") != "object": if x_body_name in arguments or has_kwargs: return {x_body_name: body} return {} + # supply the initial defaults and convert all values to the proper types by schema body_arg = deepcopy(default_body) body_arg.update(body or {}) diff --git a/tests/aiohttp/test_aiohttp_multipart.py b/tests/aiohttp/test_aiohttp_multipart.py new file mode 100644 index 000000000..7f80b7388 --- /dev/null +++ b/tests/aiohttp/test_aiohttp_multipart.py @@ -0,0 +1,118 @@ +import os +from pathlib import Path + +import pytest +from connexion import AioHttpApp + +import aiohttp + +try: + import ujson as json +except ImportError: + import json + + +@pytest.fixture +def aiohttp_app(aiohttp_api_spec_dir): + app = AioHttpApp(__name__, port=5001, + specification_dir=aiohttp_api_spec_dir, + debug=True) + app.add_api( + 'openapi_multipart.yaml', + validate_responses=True, + strict_validation=True, + pythonic_params=True, + pass_context_arg_name='request_ctx', + ) + return app + + +async def test_single_file_upload(aiohttp_app, aiohttp_client): + app_client = await aiohttp_client(aiohttp_app.app) + + resp = await app_client.post( + '/v1.0/upload_file', + data=aiohttp.FormData(fields=[('myfile', open(__file__, 'rb'))])(), + ) + + data = await resp.json() + assert resp.status == 200 + assert data['fileName'] == f'{__name__}.py' + assert data['myfile_content'] == Path(__file__).read_bytes().decode('utf8') + + +async def test_many_files_upload(aiohttp_app, aiohttp_client): + app_client = await aiohttp_client(aiohttp_app.app) + + dir_name = os.path.dirname(__file__) + files_field = [ + ('myfiles', open(f'{dir_name}/{file_name}', 'rb')) \ + for file_name in sorted(os.listdir(dir_name)) if file_name.endswith('.py') + ] + + form_data = aiohttp.FormData(fields=files_field) + + resp = await app_client.post( + '/v1.0/upload_files', + data=form_data(), + ) + + data = await resp.json() + + assert resp.status == 200 + assert data['files_count'] == len(files_field) + assert data['myfiles_content'] == [ + Path(f'{dir_name}/{file_name}').read_bytes().decode('utf8') \ + for file_name in sorted(os.listdir(dir_name)) if file_name.endswith('.py') + ] + + +async def test_mixed_multipart_single_file(aiohttp_app, aiohttp_client): + app_client = await aiohttp_client(aiohttp_app.app) + + form_data = aiohttp.FormData() + form_data.add_field('dir_name', os.path.dirname(__file__)) + form_data.add_field('myfile', open(__file__, 'rb')) + + resp = await app_client.post( + '/v1.0/mixed_single_file', + data=form_data(), + ) + + data = await resp.json() + + assert resp.status == 200 + assert data['dir_name'] == os.path.dirname(__file__) + assert data['fileName'] == f'{__name__}.py' + assert data['myfile_content'] == Path(__file__).read_bytes().decode('utf8') + + + +async def test_mixed_multipart_many_files(aiohttp_app, aiohttp_client): + app_client = await aiohttp_client(aiohttp_app.app) + + dir_name = os.path.dirname(__file__) + files_field = [ + ('myfiles', open(f'{dir_name}/{file_name}', 'rb')) \ + for file_name in sorted(os.listdir(dir_name)) if file_name.endswith('.py') + ] + + form_data = aiohttp.FormData(fields=files_field) + form_data.add_field('dir_name', os.path.dirname(__file__)) + form_data.add_field('test_count', str(len(files_field))) + + resp = await app_client.post( + '/v1.0/mixed_many_files', + data=form_data(), + ) + + data = await resp.json() + + assert resp.status == 200 + assert data['dir_name'] == os.path.dirname(__file__) + assert data['test_count'] == len(files_field) + assert data['files_count'] == len(files_field) + assert data['myfiles_content'] == [ + Path(f'{dir_name}/{file_name}').read_bytes().decode('utf8') \ + for file_name in sorted(os.listdir(dir_name)) if file_name.endswith('.py') + ] diff --git a/tests/api/test_parameters.py b/tests/api/test_parameters.py index b60e75580..ea1e48a54 100644 --- a/tests/api/test_parameters.py +++ b/tests/api/test_parameters.py @@ -387,6 +387,9 @@ def test_nullable_parameter(simple_app): resp = app_client.put('/v1.0/nullable-parameters', data="None", headers=headers) assert json.loads(resp.data.decode('utf-8', 'replace')) == 'it was None' + resp = app_client.put('/v1.0/nullable-parameters-noargs', data="None", headers=headers) + assert json.loads(resp.data.decode('utf-8', 'replace')) == 'hello' + def test_args_kwargs(simple_app): app_client = simple_app.app.test_client() @@ -398,6 +401,16 @@ def test_args_kwargs(simple_app): assert resp.status_code == 200 assert json.loads(resp.data.decode('utf-8', 'replace')) == {'foo': 'a'} + if simple_app._spec_file == 'openapi.yaml': + body = { 'foo': 'a', 'bar': 'b' } + resp = app_client.post( + '/v1.0/body-params-as-kwargs', + data=json.dumps(body), + headers={'Content-Type': 'application/json'}) + assert resp.status_code == 200 + # having only kwargs, the handler would have been passed 'body' + assert json.loads(resp.data.decode('utf-8', 'replace')) == {'body': {'foo': 'a', 'bar': 'b'}, } + def test_param_sanitization(simple_app): app_client = simple_app.app.test_client() diff --git a/tests/fakeapi/aiohttp_handlers.py b/tests/fakeapi/aiohttp_handlers.py index e37af67fc..a61cc0019 100755 --- a/tests/fakeapi/aiohttp_handlers.py +++ b/tests/fakeapi/aiohttp_handlers.py @@ -105,3 +105,45 @@ async def get_date(): async def get_uuid(): return ConnexionResponse(body={'value': uuid.UUID(hex='e7ff66d0-3ec2-4c4e-bed0-6e4723c24c51')}) + + +async def aiohttp_multipart_single_file(myfile): + return aiohttp.web.json_response( + data={ + 'fileName': myfile.filename, + 'myfile_content': myfile.file.read().decode('utf8') + }, + ) + + +async def aiohttp_multipart_many_files(myfiles): + return aiohttp.web.json_response( + data={ + 'files_count': len(myfiles), + 'myfiles_content': [ f.file.read().decode('utf8') for f in myfiles ] + }, + ) + + +async def aiohttp_multipart_mixed_single_file(myfile, body): + dir_name = body['dir_name'] + return aiohttp.web.json_response( + data={ + 'dir_name': dir_name, + 'fileName': myfile.filename, + 'myfile_content': myfile.file.read().decode('utf8'), + }, + ) + + +async def aiohttp_multipart_mixed_many_files(myfiles, body): + dir_name = body['dir_name'] + test_count = body['test_count'] + return aiohttp.web.json_response( + data={ + 'files_count': len(myfiles), + 'dir_name': dir_name, + 'test_count': test_count, + 'myfiles_content': [ f.file.read().decode('utf8') for f in myfiles ] + }, + ) diff --git a/tests/fakeapi/hello/__init__.py b/tests/fakeapi/hello/__init__.py index 773d64055..0645b5ef7 100644 --- a/tests/fakeapi/hello/__init__.py +++ b/tests/fakeapi/hello/__init__.py @@ -402,6 +402,9 @@ def test_nullable_param_put(contents): return 'it was None' return contents +def test_nullable_param_put_noargs(dummy=''): + return 'hello' + def test_custom_json_response(): return {'theResult': DummyClass()}, 200 @@ -459,6 +462,9 @@ def optional_auth(**kwargs): def test_args_kwargs(*args, **kwargs): return kwargs +def test_args_kwargs_post(*args, **kwargs): + return kwargs + def test_param_sanitization(query=None, form=None): result = {} diff --git a/tests/fixtures/aiohttp/openapi_multipart.yaml b/tests/fixtures/aiohttp/openapi_multipart.yaml new file mode 100644 index 000000000..f4374b6a3 --- /dev/null +++ b/tests/fixtures/aiohttp/openapi_multipart.yaml @@ -0,0 +1,132 @@ +--- +openapi: 3.0.0 +servers: + - url: /v1.0 +info: + title: "{{title}}" + version: "1.0" +paths: + "/upload_file": + post: + summary: Uploads single file + description: Handles multipart file upload. + operationId: fakeapi.aiohttp_handlers.aiohttp_multipart_single_file + responses: + "200": + description: OK response + content: + 'application/json': + schema: + type: object + properties: + fileName: + type: string + default: + description: unexpected error + requestBody: + required: true + content: + multipart/form-data: + schema: + type: object + properties: + myfile: + type: string + format: binary + "/upload_files": + post: + summary: Uploads many files + description: Handles multipart file upload. + operationId: fakeapi.aiohttp_handlers.aiohttp_multipart_many_files + responses: + "200": + description: OK response + content: + 'application/json': + schema: + type: object + properties: + files_count: + type: number + default: + description: unexpected error + requestBody: + required: true + content: + multipart/form-data: + schema: + type: object + properties: + myfiles: + type: array + items: + type: string + format: binary + "/mixed_single_file": + post: + summary: Reads multipart data + description: Handles multipart data reading + operationId: fakeapi.aiohttp_handlers.aiohttp_multipart_mixed_single_file + responses: + "200": + description: OK response + content: + 'application/json': + schema: + type: object + properties: + dir_name: + type: string + fileName: + type: string + default: + description: unexpected error + requestBody: + required: true + content: + multipart/form-data: + schema: + type: object + properties: + dir_name: + type: string + myfile: + type: string + format: binary + "/mixed_many_files": + post: + summary: Reads multipart data + description: Handles multipart data reading + operationId: fakeapi.aiohttp_handlers.aiohttp_multipart_mixed_many_files + responses: + "200": + description: OK response + content: + 'application/json': + schema: + type: object + properties: + dir_name: + type: string + test_count: + type: number + files_count: + type: number + default: + description: unexpected error + requestBody: + required: true + content: + multipart/form-data: + schema: + type: object + properties: + dir_name: + type: string + test_count: + type: number + myfiles: + type: array + items: + type: string + format: binary diff --git a/tests/fixtures/simple/openapi.yaml b/tests/fixtures/simple/openapi.yaml index 7ff1ad91e..82e253340 100644 --- a/tests/fixtures/simple/openapi.yaml +++ b/tests/fixtures/simple/openapi.yaml @@ -845,6 +845,24 @@ paths: responses: '200': description: OK + /nullable-parameters-noargs: + put: + operationId: fakeapi.hello.test_nullable_param_put_noargs + responses: + '200': + description: OK + requestBody: + content: + application/json: + schema: + nullable: true + x-body-name: contents + type: object + properties: + name: + type: string + description: Just a testing parameter. + required: true /custom-json-response: get: operationId: fakeapi.hello.test_custom_json_response @@ -896,6 +914,25 @@ paths: application/json: schema: type: object + /body-params-as-kwargs: + post: + operationId: fakeapi.hello.test_args_kwargs_post + requestBody: + content: + application/json: + schema: + type: object + properties: + foo: + type: string + additionalProperties: true + responses: + '200': + description: Return kwargs + content: + application/json: + schema: + type: object /text-request: post: operationId: fakeapi.hello.get_data_as_text diff --git a/tests/fixtures/simple/swagger.yaml b/tests/fixtures/simple/swagger.yaml index de3db771e..1030b0685 100644 --- a/tests/fixtures/simple/swagger.yaml +++ b/tests/fixtures/simple/swagger.yaml @@ -677,6 +677,26 @@ paths: 200: description: OK + /nullable-parameters-noargs: + put: + operationId: fakeapi.hello.test_nullable_param_put_noargs + produces: + - application/json + parameters: + - name: contents + description: Just a testing parameter. + in: body + x-nullable: true + required: true + schema: + type: object + properties: + name: + type: string + responses: + 200: + description: OK + /custom-json-response: get: operationId: fakeapi.hello.test_custom_json_response @@ -731,6 +751,30 @@ paths: schema: type: object + /body-params-as-kwargs: + post: + operationId: fakeapi.hello.test_args_kwargs_post + produces: + - application/json + parameters: + - name: $body + description: Just a testing parameter in the body + in: body + required: true + schema: + type: object + properties: + foo: + type: string + bar: + type: string + additionalProperties: true + responses: + 200: + description: Return kwargs + schema: + type: object + /text-request: post: operationId: fakeapi.hello.get_data_as_text