Added aiohttp support #530

dutradda · 2017-10-19T15:55:32Z

Hello Guys, I implemented an App and Api classes supporting aiohttp framework. It is fully tested.
I had did some modifications on the base code as well.
Please, take a look on it

coveralls · 2017-10-19T16:06:41Z

Coverage remained the same at 100.0% when pulling 30b7528 on dutradda:aiohttp_support into f846126 on zalando:master.

hjacobs · 2017-10-19T16:15:30Z

Oh, wow, thanks!

prawn-cake

Hi! Thanks for the PR!
Overall it looks promising. I'm still reviewing the code. Meantime sending some suggestion to improve the code.

prawn-cake · 2017-10-21T10:41:14Z

connexion/__init__.py

@@ -26,5 +27,21 @@ def _required_lib(exec_info, *args, **kwargs):
 App = FlaskApp
 Api = FlaskApi

+if sys.version_info[0:2] >= (3, 4): # pragma: no cover


I guess you don't need to slice version info

prawn-cake · 2017-10-21T10:47:51Z

connexion/apis/aiohttp_api.py

+            response = yield from response
+
+        if request:
+            url = str(request.url)


More concise version: url = str(request.url) if request else ''

prawn-cake · 2017-10-21T10:53:48Z

connexion/apis/aiohttp_api.py

+    ujson.dumps = partial(ujson.dumps, escape_forward_slashes=True)
+
+except ImportError:  # pragma: no cover
+    import json as ujson


To me it feels that any 3rd party json parsers should mimic the standard json and not vice-versa, i.e

try: import ujson as json except ImportError: import json ... json.dumps(...)

We don't want to be coupled with any particular implementation using different names

prawn-cake · 2017-10-22T07:38:18Z

connexion/apis/aiohttp_api.py

+class AioHttpApi(AbstractAPI):
+
+    def __init__(self, *args, **kwargs):
+        self.aiohttp_api = AioHtppApplication(


This is quite confusing. It's an app instance in fact, not an api. self.app = AnyAppImpl(...) would be better

prawn-cake · 2017-10-23T06:09:53Z

connexion/__init__.py

@@ -26,5 +27,21 @@ def _required_lib(exec_info, *args, **kwargs):
 App = FlaskApp
 Api = FlaskApi

+if sys.version_info[0:2] >= (3, 4): # pragma: no cover
+    try:
+        from .apis.aiohttp_api import AioHttpApi


The code above does almost exactly the same, except import paths. Let's refactor it to not repeat yourself.

try: from .apis.flask_api import FlaskApi from .apps.flask_app import FlaskApp from flask import request # NOQA except ImportError as e: # pragma: no cover import six import functools def _required_lib(exec_info, *args, **kwargs): six.reraise(*exec_info) _flask_not_installed_error = functools.partial(_required_lib, sys.exc_info()) FlaskApi = _flask_not_installed_error FlaskApp = _flask_not_installed_error ... if sys.version_info[0:2] >= (3, 4): # pragma: no cover try: from .apis.aiohttp_api import AioHttpApi from .apps.aiohttp_app import AioHttpApp except ImportError as e: import six import functools def _required_lib(exec_info, *args, **kwargs): six.reraise(*exec_info) _aiohttp_not_installed_error = functools.partial(_required_lib, sys.exc_info()) AioHttpApi = _aiohttp_not_installed_error AioHttpApp = _aiohttp_not_installed_error

I see it as

# We can make this as a high-order function and reuse def not_installed_error(): import six import functools def _required_lib(exec_info, *args, **kwargs): six.reraise(*exec_info) return functools.partial(_required_lib, sys.exc_info()) ... try: ... except ImportError: WhateverApi = not_installed_error()

This will make it more readable

prawn-cake · 2017-10-23T06:15:33Z

General note: the PR lacks some docs about how to plug-in aiohttp server backend, plus cli.py need to be updated to support it as well

dutradda · 2017-10-23T14:53:06Z

Hello!

@prawn-cake the CLI was already updated. see here

I don't know what you means with "how to plug-in aiohttp server backend".
It is how to implement aiohttp handlers? Or is how to run an aiohttp server? Because the AioHttpApp.run method runs the aiohttp server. What exact I must to document better?

And about your comments I will update the PR

coveralls · 2017-10-23T17:42:51Z

Coverage remained the same at 100.0% when pulling 8c17d19 on dutradda:aiohttp_support into f846126 on zalando:master.

coveralls · 2017-10-23T18:01:03Z

Coverage remained the same at 100.0% when pulling 2bf4e04 on dutradda:aiohttp_support into f846126 on zalando:master.

coveralls · 2017-10-23T19:01:45Z

Coverage remained the same at 100.0% when pulling f6b9447 on dutradda:aiohttp_support into f846126 on zalando:master.

prawn-cake

@dutradda
Yea. About cli.py. It's a bit tricky. It's not visible in the PR.
But there is an option --wsgi-server which sets it to 'flask' by default and no aiohttp option is available. AioHttpApp.run runs if server == 'aiohttp', right?
We can add 'aiohttp' to the choices list, but it would be wrong interpretation because aiohttp doesn't implement WSGI at all.

@hjacobs what do you think?

About documentation. We want let the users know that they now able to pick the app class. So some hello world examples with some aiohttp title would be enough I think.
Currently we have docs here and in the README

prawn-cake · 2017-10-23T19:45:12Z

connexion/apis/aiohttp_api.py

+                     extra=vars(operation))
+
+        handler = operation.function
+        endpoint_name = '{}_{}'.format(self._api_name,


If the same resource handles multiple methods, e.g

paths: /pets/{pet_id}: get: ... put: ... delete: ...

It will crash on add_api(...) call with

File "/home/maxi/myPython/tryConnexion/.env/lib/python3.5/site-packages/connexion/apis/abstract.py", line 242, in add_paths self.add_operation(method, path, endpoint, path_parameters) File "/home/maxi/myPython/tryConnexion/.env/lib/python3.5/site-packages/connexion/apis/abstract.py", line 196, in add_operation self._add_operation_internal(method, path, operation) File "/home/maxi/myPython/tryConnexion/.env/lib/python3.5/site-packages/connexion/apis/aiohttp_api.py", line 155, in _add_operation_internal method, path, handler, name=endpoint_name File "/home/maxi/myPython/tryConnexion/.env/lib/python3.5/site-packages/aiohttp/web_urldispatcher.py", line 881, in add_route resource = self.add_resource(path, name=name) File "/home/maxi/myPython/tryConnexion/.env/lib/python3.5/site-packages/aiohttp/web_urldispatcher.py", line 876, in add_resource self.register_resource(resource) File "/home/maxi/myPython/tryConnexion/.env/lib/python3.5/site-packages/aiohttp/web_urldispatcher.py", line 863, in register_resource .format(name, self._named_resources[name])) ValueError: Duplicate '_pets__pet_id_', already handled by <DynamicResource '_pets__pet_id_' /pets/{pet_id}

And tests don't cover such cases at all. Good to add it.
The fix can be to add a method name to a resource (endpoint) name

prawn-cake · 2017-10-23T19:48:21Z

connexion/__init__.py

    FlaskApi = _flask_not_installed_error
    FlaskApp = _flask_not_installed_error

 App = FlaskApp
 Api = FlaskApi

+if sys.version_info[0] >= 3:  # pragma: no cover


What I meant here is that everywhere else in the code the version check looks like if sys.version_info >= (3, 4): ...
Not sure if there is any special reason to do it differently. Not critical though.

prawn-cake · 2017-10-23T19:55:41Z

connexion/apis/aiohttp_api.py

+
+    def _add_operation_internal(self, method, path, operation):
+        method = method.upper()
+        operation_id = \


Another neat trick:
operation_id = operation.operation_id or path gives you the same result (if the left hand expression is evaluated as false then the right side takes precedence)

dutradda · 2017-11-09T19:07:16Z

@prawn-cake
I did the requested changes.
About the cli I did one option as deprecated, I don't know if what I did is the best solution.
And about the docs what I did is sufficient?

coveralls · 2017-11-09T19:15:21Z

Coverage remained the same at 100.0% when pulling 742a80d on dutradda:aiohttp_support into f846126 on zalando:master.

coveralls · 2017-11-10T14:52:14Z

Coverage remained the same at 100.0% when pulling 1fff7ea on dutradda:aiohttp_support into f846126 on zalando:master.

coveralls · 2017-11-10T15:13:14Z

Coverage remained the same at 100.0% when pulling a0184e6 on dutradda:aiohttp_support into f846126 on zalando:master.

prawn-cake

I haven't run my tests yet, will do it soon.
Commented about some small changes and clarifications.

prawn-cake · 2017-11-13T09:01:15Z

connexion/cli.py

@@ -1,6 +1,7 @@
 import importlib
 import logging
 import sys
+from itertools import chain


Redundant import?

I already remove this line, this code is outdated

prawn-cake · 2017-11-13T09:10:51Z

connexion/cli.py

@@ -108,6 +133,23 @@ def run(spec_file,

    - BASE_MODULE_PATH (optional): filesystem path where the API endpoints handlers are going to be imported from.
    """
+    if wsgi_server and server:
+        raise click.BadParameter(
+            "these options are mutually excludent",


Minor typo, I guess "mutually exclusive" is the proper one =)

I will fix my typo :)

prawn-cake · 2017-11-13T09:26:03Z

connexion/cli.py

-
-
-def validate_wsgi_server_requirements(ctx, param, value):
+FLASK_APP = 'flask'


Not sure if I understood purpose of these constants properly.

We divide our config into connexion apps, app frameworks and app servers we serve our web-app with, right?

Each app framework can be an app server at the same time, i.e serve itself

We want to resolve dependencies between app servers and app frameworks (e.g flask can't be run with aiohttp by default although it has 3rd party wsgi adapters)

Just trying to understand why we need so many configs which repeat the same names

I don't know if I understand your points, but I will try to explain why I created these variables.

I created the AVAILABLE_SERVERS variable to ensure that the user input is right (but this validation will be done by the app when the App.run method is called). So this validation can be removed as well.

The AVAILABLE_APPS is for mapping the "--app-framework" input with the right class.

The DEFAULT_SERVERS was created because some frameworks has no default server (like the falcon framework, there's no way to run the application directly from falcon, a external wsgi server must be used), but for now the defaults are the same name of the framework, so I can remove this variable too if you want.

Yea, I just wanted to know what problem you are trying to solve with this, because it's not obvious at first glance =)
No need to remove.

prawn-cake · 2017-11-14T18:44:43Z

connexion/cli.py

    if value == 'gevent':
        try:
            import gevent  # NOQA
-        except:
+        except Exception:


Let's be specific here and below and catch ImportError

This one was resolved with this commit. Pull latest changes, please.

prawn-cake · 2017-11-14T18:45:18Z

connexion/cli.py

            fatal_error('gevent library is not installed')
    elif value == 'tornado':
        try:
            import tornado  # NOQA
-        except:
+        except Exception:


Should we add import aiohttp check as well?

prawn-cake · 2017-11-14T18:54:17Z

connexion/apis/aiohttp_api.py

+
+    @classmethod
+    def _get_aiohttp_response_from_connexion(cls, response, mimetype):
+        content_type = response.content_type if response.content_type else \


I think such complex conditions should be written as a regular multiline if else block

prawn-cake · 2017-11-14T18:55:46Z

connexion/cli.py

-
-
-def validate_wsgi_server_requirements(ctx, param, value):
+FLASK_APP = 'flask'


Yea, I just wanted to know what problem you are trying to solve with this, because it's not obvious at first glance =)
No need to remove.

prawn-cake · 2017-11-14T18:58:33Z

connexion/cli.py

+        )
+        raise click.UsageError(message)
+
+    if app_framework == AIOHTTP_APP:


Similar checks are done in the validate_server_requirements. Is it enough to delegate this check to validate_server_requirements (add aiohttp check there) and remove it from here?

I did this check here because if a user input is "--app-framework=flask --server=aiohttp" the error that will raise is "invalid server for 'flask'".

But, if the check is done on "validate_server_requirements" the error that will raise is "aiohttp not found".

But I can move this check to the "validate_server_requirements" function if you prefer

The code generates the error you are talking about should be run first. So it shouldn't affect app-framework - server validation.
The issue I faced is when you move if app_framework == AIOHTTP_APP check to the validate_server_requirements and attach that callback for the run cli method, it eats up app_framework value and misbehave after.

The reason is that callback returns None after the check. Which doesn't seem right.

I updated it to this (always return the value in case of no failures) and this should work.

def validate_server_requirements(ctx, param, value): if value == 'gevent': try: import gevent # NOQA except Exception: fatal_error('gevent library is not installed') elif value == 'tornado': try: import tornado # NOQA except Exception: fatal_error('tornado library is not installed') elif value == 'aiohttp': try: import aiohttp except ImportError: fatal_error('aiohttp library is not installed') return value

Could you check it for yourself?

prawn-cake · 2017-11-15T07:37:13Z

Also found aiohttp level issue with the base_path when it adds a subapp.

So basically this will fail

app = connexion.AioHttpApp(__name__)
app.add_api('swagger.yaml', base_path='/')
app.run(port=8080)

with

  File "/home/maxi/myPython/tryConnexion/app.py", line 53, in <module>
    app.add_api('swagger.yaml', base_path='/')
  File "/home/maxi/myPython/connexion/connexion/apps/aiohttp_app.py", line 43, in add_api
    self.app.add_subapp(api.base_path, api.subapp)
  File "/home/maxi/myPython/tryConnexion/.env/lib/python3.5/site-packages/aiohttp/web.py", line 186, in add_subapp
    raise ValueError("Prefix cannot be empty")
ValueError: Prefix cannot be empty

This works ok

...
app.add_api('swagger.yaml', base_path='/api')

prawn-cake · 2017-11-16T12:18:40Z

I also created simple project to test your changes and try to use aiohttp-backed connexion service.
I had to struggle a bit with the handlers, took me some time to debug errors. Initially I expected that it would work transparently if I switch the app and turn my handlers into coroutines, not really. Errors it spits out are not obvious at all.

Thought of may be we could extend our hello world example with the proper aiohttp handler or at least make a note and point to the aiohttp handlers documentation.

dutradda · 2017-11-21T14:53:57Z

@prawn-cake
you suggest some modification to the base_path issue?

and about the docs issues
where are the hello world example that I can modify?

prawn-cake · 2017-11-21T20:25:22Z

README.rst

+
+    app = connexion.AioHttpApp(__name__, specification_dir='swagger/')
+    app.run(port=8080)
+


Here we could add something like
NOTE: Also check aiohttp handler examples

prawn-cake · 2017-11-21T20:37:12Z

Regarding the base_path. At least we could intercept that exception and re-raise it with the more meaningul message in the AioHttpApp.add_api method.
For example "aiohttp doesn't allow to set empty base_path ({base_path}), use non-empty instead, e.g /api"

Added comment in the README

dutradda · 2018-02-19T17:55:35Z

@prawn-cake, I think I did the requested changes. Can you take a look?

prawn-cake · 2018-02-19T20:05:01Z

Can we bump aiohttp version to 2.3.10? The rest of the stuff looks good to me

dutradda · 2018-02-19T20:11:36Z

@prawn-cake can be the aiohttp 3.0.1?
it is the latest one

prawn-cake · 2018-02-19T20:14:58Z

3.0.1 has too many changes and probably backward incompatible.

At least this one bugs me:

Minimal supported Python versions are 3.5.3 and 3.6.0

prawn-cake · 2018-02-19T20:33:33Z

👍

prawn-cake · 2018-02-19T20:34:53Z

@hjacobs I guess we are done here.
@dutradda Thanks for the great contribution.

dutradda · 2018-04-02T18:51:32Z

@prawn-cake
Hello guys, do you know when this PR will be merged to the master?

haneefkassam · 2018-04-09T17:56:41Z

This is amazing! (and I'm assuming paves the path to also using uvloop with connexion and flask, and async handler functions, right? please correct me if I'm wrong)

prawn-cake · 2018-04-09T18:03:16Z

@haneefkassam almost correct, this allows to switch the backend from flask to aiohttp, basically to make connexion async

hjacobs · 2018-04-09T19:08:16Z

@dutradda let me check regarding merging, sorry for not getting back earlier.

dutradda · 2018-04-09T19:34:06Z

@hjacobs thank you!

hjacobs · 2018-04-09T19:46:03Z

Released in https://github.com/zalando/connexion/releases/tag/1.4

haneefkassam · 2018-04-09T20:40:24Z

@prawn-cake Would this also mean swapping the backend from flask to sanic is much more trivial?

prawn-cake · 2018-04-10T08:42:05Z

@haneefkassam it's definitely easier since we added api abstraction, but not sure how trivial it is to add sanic, need to check implementation details.

dutradda · 2018-04-10T10:04:06Z

@haneefkassam
I alreadly used sanic and I can say that is very easy to plug it on connexion.
But the aiohttp is not good for you?
I switched from sanic to aiohttp because its maturity. And it peforms well too

haneefkassam · 2018-04-10T15:07:52Z

@dutradda No no, it's not that aiohttp is not good; I've just seen sanic vs aiohttp go in favour of sanic (without considering replacing the purely python asyncio with uvloop in aiohttp).

I'm not one to be un-appreciative though; I'm loving the fact this PR was merged and love the fact that I now have the option of using python3 async features with connexion!

roman-telepathy-ai · 2018-06-18T14:43:27Z

@dutradda do you have some examples for connexion + aiohttp?

async get / post
async get / post with blocking calls
websockets

Would be interesting how websockets would be represented in swagger.yaml.

thx in advance!

maitham · 2019-04-10T16:29:33Z

Has anyone managed to get a websocket example working?

dutradda changed the title ~~Adde aiohttp support~~ Added aiohttp support Oct 19, 2017

prawn-cake suggested changes Oct 23, 2017

View reviewed changes

spec-first deleted a comment Oct 23, 2017

dutradda force-pushed the aiohttp_support branch from 2bf4e04 to f6b9447 Compare October 23, 2017 18:50

spec-first deleted a comment Oct 23, 2017

prawn-cake reviewed Oct 24, 2017

View reviewed changes

spec-first deleted a comment Nov 10, 2017

dutradda force-pushed the aiohttp_support branch from 1fff7ea to a0184e6 Compare November 10, 2017 14:53

spec-first deleted a comment Nov 10, 2017

prawn-cake reviewed Nov 13, 2017

View reviewed changes

prawn-cake reviewed Nov 14, 2017

View reviewed changes

mvalkon added the waiting contributor feedback label Nov 21, 2017

prawn-cake reviewed Nov 21, 2017

View reviewed changes

Addded aiohttp support

5ce5145

dutradda force-pushed the aiohttp_support branch from cfe55f0 to 8f4ff03 Compare February 5, 2018 15:36

spec-first deleted a comment Feb 5, 2018

updated the aiohttp version

afbd478

dutradda force-pushed the aiohttp_support branch from 661b0dd to afbd478 Compare February 19, 2018 20:21

spec-first deleted a comment Feb 19, 2018

hjacobs self-assigned this Apr 3, 2018

hjacobs removed the waiting contributor feedback label Apr 4, 2018

hjacobs merged commit afbd478 into spec-first:master Apr 9, 2018

dutradda mentioned this pull request Apr 9, 2018

Naive switch from Flask to AIOHTTP fails #578

Closed

hjacobs mentioned this pull request Sep 25, 2018

String query params are being chopped off leaving only last character standing #696

Closed



		def validate_wsgi_server_requirements(ctx, param, value):
		FLASK_APP = 'flask'


		app = connexion.AioHttpApp(__name__, specification_dir='swagger/')
		app.run(port=8080)

Added aiohttp support #530

Added aiohttp support #530

Conversation

dutradda commented Oct 19, 2017

coveralls commented Oct 19, 2017 • edited Loading

hjacobs commented Oct 19, 2017

prawn-cake left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prawn-cake Oct 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prawn-cake commented Oct 23, 2017

dutradda commented Oct 23, 2017 • edited Loading

coveralls commented Oct 23, 2017 • edited Loading

coveralls commented Oct 23, 2017 • edited Loading

coveralls commented Oct 23, 2017 • edited Loading

prawn-cake left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dutradda commented Nov 9, 2017 • edited Loading

coveralls commented Nov 9, 2017 • edited Loading

coveralls commented Nov 10, 2017 • edited Loading

coveralls commented Nov 10, 2017 • edited Loading

prawn-cake left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prawn-cake Nov 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prawn-cake Nov 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dutradda Nov 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prawn-cake commented Nov 15, 2017 • edited Loading

prawn-cake commented Nov 16, 2017

dutradda commented Nov 21, 2017

Choose a reason for hiding this comment

prawn-cake commented Nov 21, 2017

dutradda commented Feb 19, 2018

prawn-cake commented Feb 19, 2018

dutradda commented Feb 19, 2018

prawn-cake commented Feb 19, 2018

prawn-cake commented Feb 19, 2018

prawn-cake commented Feb 19, 2018

dutradda commented Apr 2, 2018

haneefkassam commented Apr 9, 2018

prawn-cake commented Apr 9, 2018

hjacobs commented Apr 9, 2018

dutradda commented Apr 9, 2018

hjacobs commented Apr 9, 2018

haneefkassam commented Apr 9, 2018

prawn-cake commented Apr 10, 2018

dutradda commented Apr 10, 2018 • edited Loading

haneefkassam commented Apr 10, 2018

roman-telepathy-ai commented Jun 18, 2018

maitham commented Apr 10, 2019

coveralls commented Oct 19, 2017 •

edited

Loading

prawn-cake Oct 21, 2017 •

edited

Loading

dutradda commented Oct 23, 2017 •

edited

Loading

coveralls commented Oct 23, 2017 •

edited

Loading

coveralls commented Oct 23, 2017 •

edited

Loading

coveralls commented Oct 23, 2017 •

edited

Loading

dutradda commented Nov 9, 2017 •

edited

Loading

coveralls commented Nov 9, 2017 •

edited

Loading

coveralls commented Nov 10, 2017 •

edited

Loading

coveralls commented Nov 10, 2017 •

edited

Loading

prawn-cake Nov 14, 2017 •

edited

Loading

prawn-cake Nov 14, 2017 •

edited

Loading

dutradda Nov 21, 2017 •

edited

Loading

prawn-cake commented Nov 15, 2017 •

edited

Loading

dutradda commented Apr 10, 2018 •

edited

Loading