Fix for aiohttp and multipart/form-data uploads (#1222)

* Added unit tests to demonstrate the problems of #975
    - Taken mostly from existing PR: #987

* now splitting out multipart POSTs into files[] and form[], handling duplicate keys as the rest of connexion expects
    - Based parly on existing PR: #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

@@ -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

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 {})
 

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')
+    ]

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()

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 ]
+        },
+    )

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 = {}