Add async app #1613

RobbeSneyders · 2022-12-23T10:29:44Z

Changes proposed in this pull request:

Adds a "native" async app based on lower level Starlette tools. It leverages the routing already done in the middleware.
Extracts code related to the parameter decorator out of the Operation objects

There's some more work to be done, but this PR is already quite huge, so I wanted to stop at a point where I got all the tests and a simple example working.

Still some general refactoring. I left some TODOs, and the Operation classes are still relied on in too many places.
Sync routes on an Async app should receive async decorators, so they can access the body using await. The decorators should then run the sync route in a threadpool to prevent blocking.
We should be able to reuse the parameter decorator for all ASGI apps since we're working with a Starlette Request which we can get directly from the scope and receive channel, which we can make available as context. This means we'll pass Starlette Datastructures to the view functions (eg. FormData, UploadFiles, ...), but if this is opt-in, I don't see any issue with this.
We should be able to reuse the response parsing for Starlette apps as well since it returns a StarletteResponse.
We should test the AsyncApp as well. I'm hoping we can do this quite quickly by generating a test client for it in the fixtures as well, but in the long term some more work will be needed.

RobbeSneyders

Self review containing some explanations and TODOs still for this PR.

connexion/apis/abstract.py

RobbeSneyders · 2022-12-23T10:32:11Z

connexion/apis/flask_api.py

-            body = cls.jsonifier.dumps(data)
-        elif not (isinstance(data, bytes) or isinstance(data, str)):
-            warnings.warn(
-                "Implicit (flask) JSON serialization will change in the next major version. "


Removed this deprecated behavior and lifted the method to the base class.

RobbeSneyders · 2022-12-23T10:34:14Z

connexion/apis/flask_api.py

-        return request
+    def get_request(cls, **kwargs) -> ConnexionRequest:
+        uri_parser = kwargs.pop("uri_parser")
+        return ConnexionRequest(flask.request, uri_parser=uri_parser)


I simplified the ConnexionRequest since I don't believe we should still provide such a high abstraction layer here anymore. This should only be implemented for Flask and Starlette. Other frameworks are supported via the ASGI interface.

RobbeSneyders · 2022-12-23T10:34:45Z

connexion/apis/flask_api.py

-    return getattr(flask.globals.request_ctx, "connexion_context")
-
-
-context = LocalProxy(_get_context)


Connexion context is now provided by the connexion.context module and the Context Middleware.

RobbeSneyders · 2022-12-23T10:36:32Z

connexion/apps/async_app.py

+            )
+
+        api_base_path = connexion_context.get("api_base_path")
+        if api_base_path and not api_base_path == self.base_path:


We might be able to leverage the RoutedMiddleware base classes here, will try to rework.

This might be possible, but there's some specific differences between the app and middleware which I'm not sure we can or should modify. Eg. apis on the app are nestable, while on the middleware they are not. More work would be needed to properly support this on the middleware.

Still something we can look at, but would do so in a separate PR.

RobbeSneyders · 2022-12-23T10:48:51Z

tests/api/test_parameters.py

@@ -172,7 +172,7 @@ def test_path_parameter_someint__bad(simple_app):
    # non-integer values will not match Flask route
    app_client = simple_app.app.test_client()
    resp = app_client.get("/v1.0/test-int-path/foo")  # type: flask.Response
-    assert resp.status_code == 400, resp.text
+    assert resp.status_code == 404, resp.text


Due to the fix for the typed path params, we now return a 404 again instead of a 400 when sending the wrong type of path parameter. This is because the request is no longer matched by the router, so it doesn't hit the parameter validation. It is either rejected by the Middleware router, or directly passed to the App router, where it is rejected.

RobbeSneyders · 2022-12-23T10:49:56Z

tests/fixtures/different_schemas/openapi.yaml

@@ -59,7 +59,7 @@ paths:
        '200':
          description: Requested new_stack data model
          content:
-            text/plain:
+            application/json:


We no longer parse the response with json for non json response types.

RobbeSneyders · 2022-12-23T10:50:16Z

connexion/apis/abstract.py

    def _serialize_data(cls, data, mimetype):
-        pass
+        if isinstance(mimetype, str) and is_json_mimetype(mimetype):


Would be nice if we could make this pluggable.

RobbeSneyders · 2022-12-23T10:51:10Z

tests/test_datastructures.py

+from connexion.datastructures import MediaTypeDict
+
+
+def test_media_type_dict():


This file wasn't added to git yet in the PR where I introduced the MediaTypeDict type, so including it here.

tests/fixtures/simple/swagger.yaml

coveralls · 2022-12-23T21:18:15Z

Pull Request Test Coverage Report for Build 3783178053

396 of 509 (77.8%) changed or added relevant lines in 23 files are covered.
3 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-3.05%) to 91.356%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
connexion/apis/abstract.py	24	25	96.0%
connexion/apps/abstract.py	6	7	85.71%
connexion/context.py	7	8	87.5%
connexion/middleware/routing.py	22	23	95.65%
connexion/utils.py	7	8	87.5%
connexion/lifecycle.py	29	34	85.29%
connexion/decorators/parameter.py	157	168	93.45%
connexion/apps/async_app.py	55	147	37.41%

Files with Coverage Reduction	New Missed Lines	%
connexion/apps/flask_app.py	1	94.59%
connexion/utils.py	2	96.45%

Totals
Change from base Build 3764824637:	-3.05%
Covered Lines:	2991
Relevant Lines:	3274

💛 - Coveralls

RobbeSneyders · 2022-12-28T17:36:04Z

Merging this so I can continue the work on fresh PRs.

RobbeSneyders added 4 commits December 23, 2022 11:12

Remove empty decorators

32bb8db

Rename and move decorators

2438114

Move parameter decorator related methods out of operation classes

7acbad0

Drop deprecated response serialization

db6ebec

RobbeSneyders requested review from Ruwann and nielsbox December 23, 2022 10:29

RobbeSneyders commented Dec 23, 2022

View reviewed changes

RobbeSneyders force-pushed the feature/async-app branch from 3f9343d to 33c9726 Compare December 23, 2022 11:29

RobbeSneyders changed the title ~~Feature/async app~~ Add async app Dec 23, 2022

RobbeSneyders added this to the Connexion 3.0 milestone Dec 23, 2022

RobbeSneyders force-pushed the feature/async-app branch 2 times, most recently from cf88d51 to 406feae Compare December 23, 2022 21:04

RobbeSneyders added 2 commits December 26, 2022 20:51

Add async app

5a0c292

Add custom WSGIMiddleware

0add539

RobbeSneyders force-pushed the feature/async-app branch from 406feae to 0add539 Compare December 26, 2022 19:51

RobbeSneyders merged commit e5784c5 into main Dec 28, 2022

RobbeSneyders deleted the feature/async-app branch December 28, 2022 17:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add async app #1613

Add async app #1613

RobbeSneyders commented Dec 23, 2022 •

edited

Loading

RobbeSneyders left a comment

RobbeSneyders Dec 23, 2022

RobbeSneyders Dec 23, 2022

RobbeSneyders Dec 23, 2022

RobbeSneyders Dec 23, 2022

RobbeSneyders Dec 26, 2022

RobbeSneyders Dec 23, 2022

RobbeSneyders Dec 23, 2022

RobbeSneyders Dec 23, 2022

RobbeSneyders Dec 23, 2022

coveralls commented Dec 23, 2022 •

edited

Loading

RobbeSneyders commented Dec 28, 2022

		return getattr(flask.globals.request_ctx, "connexion_context")


		context = LocalProxy(_get_context)

		from connexion.datastructures import MediaTypeDict


		def test_media_type_dict():

Add async app #1613

Add async app #1613

Conversation

RobbeSneyders commented Dec 23, 2022 • edited Loading

RobbeSneyders 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 23, 2022 • edited Loading

Pull Request Test Coverage Report for Build 3783178053

💛 - Coveralls

RobbeSneyders commented Dec 28, 2022

RobbeSneyders commented Dec 23, 2022 •

edited

Loading

coveralls commented Dec 23, 2022 •

edited

Loading