Add routing middleware #1497

RobbeSneyders · 2022-03-17T23:05:19Z

Part of #1489

This PR adds a routing middleware, which adds the connexion operation to the scope passed to the next middlewares.

The middleware is implemented with a starlette router on which the api endpoints are registered. For all other routes, the router acts ass a pass-through.

Ruwann · 2022-03-18T20:14:46Z

Some more context for possible issues of subclassing starlette.BaseHTTPMiddleware: encode/starlette#1029

Also this comment and the corresponding PR: encode/starlette#1519 (comment)

RobbeSneyders · 2022-03-20T12:44:28Z

Thanks for the additional context @Ruwann, I was indeed already running into some of these issues.
I was able to refactor and simplify the approach of the routing middleware, and I removed our own BaseHTTPMiddleware.

Note that there's some quick fixes in this PR that I still want to tackle later. Right now, I'm trying to extract all functionality into middleware with minimal effort via maximal reuse of existing connexion code (like the Api and Operation classes), and maximal usage of Starlette tools.

RobbeSneyders

This one is ready for review @Ruwann

RobbeSneyders · 2022-04-10T15:20:08Z

tests/api/test_errors.py

@@ -27,7 +27,6 @@ def test_errors(problem_app):
    error405 = json.loads(get_greeting.data.decode('utf-8', 'replace'))
    assert error405['type'] == 'about:blank'
    assert error405['title'] == 'Method Not Allowed'
-    assert error405['detail'] == 'The method is not allowed for the requested URL.'


This is Flask specific and not set by Starlette.

RobbeSneyders · 2022-04-10T15:20:35Z

tests/fakeapi/auth.py

@@ -13,15 +13,3 @@ def fake_json_auth(token, required_scopes=None):
        return json.loads(token)
    except ValueError:
        return None
-
-
-async def async_basic_auth(username, password, required_scopes=None, request=None):


Just some cleanup, these functions are not actually used in the tests.

coveralls · 2022-04-11T20:47:59Z

Pull Request Test Coverage Report for Build 2163779258

147 of 154 (95.45%) changed or added relevant lines in 11 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.008%) to 93.292%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
connexion/apis/abstract.py	45	46	97.83%
connexion/exceptions.py	3	4	75.0%
connexion/middleware/exceptions.py	12	14	85.71%
connexion/middleware/routing.py	70	73	95.89%

Totals
Change from base Build 2157288191:	-0.008%
Covered Lines:	2712
Relevant Lines:	2907

💛 - Coveralls

Ruwann · 2022-04-11T15:57:58Z

connexion/apis/abstract.py

+        :param specification: OpenAPI specification. Can be provided either as dict, or as path
+        to file.
+        :param base_path: Base path to host the API.
+        :param arguments: Jinja arguments to resolve in specification.
+        :param resolver: Callable that maps operationID to a function
+        :param resolver_error_handler: Callable that generates an Operation used for handling
+        ResolveErrors
+        :param debug: Flag to run in debug mode


The argument docstrings are a bit unintuitive to me: (a subset of) the parameters of the superclass are described here and some of this class' arguments are described.

Indeed, wanted to describe parameters of both this class and superclass, but seems like I forgot to update some. Will clean it up.

Ruwann · 2022-04-11T16:04:07Z

connexion/apps/abstract.py

        self.app = self.create_app()
-        self.middleware = self._apply_middleware()
+
+        middlewares = middlewares or ConnexionMiddleware.default_middlewares


Can't immediately think of a concrete example/usecase, but if the app should be created without any middlewares, this is now not really possible (unless overriding ConnexionMiddleware.default_middlewares). Should this be allowed by e.g. making a distinction between None and []?

Don't really see a use case for it, but the current code can indeed at least lead to some unexpected behavior. I'll put it as default for the argument or handle the [] explicitly.

Ruwann · 2022-04-11T16:24:27Z

connexion/middleware/routing.py

+        api_base_path = scope.get('root_path', '')[len(original_scope.get('root_path', '')):]
+
+        if CONNEXION_CONTEXT not in original_scope:
+            original_scope[CONNEXION_CONTEXT] = {}


I'm not sure what the convention is, but should we put it under extensions in the scope instead of directly at the top level?

Didn't know about this. Makes sense.

Ruwann · 2022-04-12T18:24:34Z

connexion/middleware/routing.py

+        if CONNEXION_CONTEXT not in original_scope:
+            original_scope[CONNEXION_CONTEXT] = {}
+
+        original_scope[CONNEXION_CONTEXT].update({


FYI, the dict.setdefault() simplifies this a bit (although perhaps a bit less untuitive as it's less known):

original_scope.setdefault(CONNEXION_CONTEXT, {}).update({'api_base_path': api_base_path})

Not a fan of the one-liner, but can indeed use setdefault for the first part.

Factor out starlette BaseHTTPMiddleware Fix exceptions for starlette < 0.19 Fix docstring formatting Rename middleware/base.py to abstract.py Rework routing middleware

RobbeSneyders · 2022-04-17T16:53:10Z

Is this one good to go @Ruwann?

Ruwann · 2022-04-19T19:50:29Z

tests/test_middleware.py

+    response = app_client.post("/v1.0/greeting/robbe")
+
+    assert response.headers.get('operation_id') == 'fakeapi.hello.post_greeting', \
+        response.status_code


Why the additional response.status_code here? Should this be a separate assert?

It just raises the status code as info if the test fails.

RobbeSneyders added this to the Connexion 3.0 milestone Mar 17, 2022

RobbeSneyders requested a review from Ruwann March 17, 2022 23:05

RobbeSneyders changed the base branch from main to feature/swagger-ui-middleware March 17, 2022 23:05

RobbeSneyders changed the title ~~Feature/routing middleware~~ Add routing middleware Mar 17, 2022

RobbeSneyders force-pushed the feature/routing-middleware branch from f0c4549 to bbf50f1 Compare March 18, 2022 07:27

RobbeSneyders force-pushed the feature/swagger-ui-middleware branch from 9222986 to 2980818 Compare March 18, 2022 07:35

RobbeSneyders force-pushed the feature/routing-middleware branch from bbf50f1 to 42da4ba Compare March 18, 2022 07:36

RobbeSneyders force-pushed the feature/swagger-ui-middleware branch from 2980818 to db12393 Compare March 18, 2022 13:56

RobbeSneyders force-pushed the feature/routing-middleware branch from 42da4ba to 37ab6ce Compare March 18, 2022 14:25

RobbeSneyders force-pushed the feature/swagger-ui-middleware branch from c5db83d to 2de9894 Compare March 20, 2022 12:32

RobbeSneyders force-pushed the feature/routing-middleware branch from 37ab6ce to 776ee8c Compare March 20, 2022 12:38

This was referenced Mar 22, 2022

Proof of concept to move connexion functionality into middleware #1477

Closed

Move functionality into pluggable ASGI middleware stack #1489

Closed

RobbeSneyders force-pushed the feature/swagger-ui-middleware branch 3 times, most recently from f67a8dd to bbeb817 Compare March 31, 2022 19:45

Base automatically changed from feature/swagger-ui-middleware to main April 10, 2022 15:15

RobbeSneyders force-pushed the feature/routing-middleware branch from e5fd226 to f0485fe Compare April 10, 2022 15:18

RobbeSneyders commented Apr 10, 2022

View reviewed changes

RobbeSneyders force-pushed the feature/routing-middleware branch from f0485fe to 40c9abe Compare April 11, 2022 20:40

Ruwann reviewed Apr 12, 2022

View reviewed changes

RobbeSneyders added 4 commits April 13, 2022 23:07

Add routing middleware

841a4b9

Factor out starlette BaseHTTPMiddleware Fix exceptions for starlette < 0.19 Fix docstring formatting Rename middleware/base.py to abstract.py Rework routing middleware

Clean up abstract API docstrings

09dc641

Move connexion context into extensions

74661a1

Allow empty middleware list

ec954cf

RobbeSneyders force-pushed the feature/routing-middleware branch from 0bea279 to ec954cf Compare April 13, 2022 21:08

Ruwann approved these changes Apr 19, 2022

View reviewed changes

RobbeSneyders merged commit 84e33e5 into main Apr 19, 2022

RobbeSneyders deleted the feature/routing-middleware branch April 19, 2022 20:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add routing middleware #1497

Add routing middleware #1497

RobbeSneyders commented Mar 17, 2022

Ruwann commented Mar 18, 2022 •

edited

Loading

RobbeSneyders commented Mar 20, 2022

RobbeSneyders left a comment

RobbeSneyders Apr 10, 2022

RobbeSneyders Apr 10, 2022

coveralls commented Apr 11, 2022 •

edited

Loading

Ruwann Apr 11, 2022

RobbeSneyders Apr 12, 2022

Ruwann Apr 11, 2022

RobbeSneyders Apr 12, 2022

Ruwann Apr 11, 2022

RobbeSneyders Apr 12, 2022

Ruwann Apr 12, 2022

RobbeSneyders Apr 12, 2022

RobbeSneyders commented Apr 17, 2022

Ruwann Apr 19, 2022

RobbeSneyders Apr 19, 2022

Add routing middleware #1497

Add routing middleware #1497

Conversation

RobbeSneyders commented Mar 17, 2022

Ruwann commented Mar 18, 2022 • edited Loading

RobbeSneyders commented Mar 20, 2022

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

coveralls commented Apr 11, 2022 • edited Loading

Pull Request Test Coverage Report for Build 2163779258

💛 - Coveralls

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

RobbeSneyders commented Apr 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ruwann commented Mar 18, 2022 •

edited

Loading

coveralls commented Apr 11, 2022 •

edited

Loading